[Xen-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic

Anthony PERARD posted 5 patches 6 years ago
There is a newer version of this series
[Xen-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
Posted by Anthony PERARD 6 years ago
We are going to want to change the value of PcdFSBClock at run time in
OvmfXen, so move it to the PcdsDynamic section.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
---
CC: Bob Feng <bob.c.feng@intel.com>
CC: Liming Gao <liming.gao@intel.com>
---
 MdePkg/MdePkg.dec | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index d022cc5e3ef2..8f5a48346e50 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -2194,10 +2194,6 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
   # @ValidList  0x80000001 | 8, 16, 32
   gEfiMdePkgTokenSpaceGuid.PcdPort80DataWidth|8|UINT8|0x0000002d
 
-  ## This value is used to configure X86 Processor FSB clock.
-  # @Prompt FSB Clock.
-  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
-
   ## The maximum printable number of characters. UefLib functions: AsciiPrint(), AsciiErrorPrint(),
   #  PrintXY(), AsciiPrintXY(), Print(), ErrorPrint() base on this PCD value to print characters.
   # @Prompt Maximum Printable Number of Characters.
@@ -2297,5 +2293,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   # @Prompt Boot Timeout (s)
   gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
 
+  ## This value is used to configure X86 Processor FSB clock.
+  # @Prompt FSB Clock.
+  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
+
 [UserExtensions.TianoCore."ExtraFiles"]
   MdePkgExtra.uni
-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
Posted by Laszlo Ersek 6 years ago
On 01/29/20 13:12, Anthony PERARD wrote:
> We are going to want to change the value of PcdFSBClock at run time in
> OvmfXen, so move it to the PcdsDynamic section.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> CC: Bob Feng <bob.c.feng@intel.com>
> CC: Liming Gao <liming.gao@intel.com>
> ---
>  MdePkg/MdePkg.dec | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> index d022cc5e3ef2..8f5a48346e50 100644
> --- a/MdePkg/MdePkg.dec
> +++ b/MdePkg/MdePkg.dec
> @@ -2194,10 +2194,6 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>    # @ValidList  0x80000001 | 8, 16, 32
>    gEfiMdePkgTokenSpaceGuid.PcdPort80DataWidth|8|UINT8|0x0000002d
>  
> -  ## This value is used to configure X86 Processor FSB clock.
> -  # @Prompt FSB Clock.
> -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> -
>    ## The maximum printable number of characters. UefLib functions: AsciiPrint(), AsciiErrorPrint(),
>    #  PrintXY(), AsciiPrintXY(), Print(), ErrorPrint() base on this PCD value to print characters.
>    # @Prompt Maximum Printable Number of Characters.
> @@ -2297,5 +2293,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    # @Prompt Boot Timeout (s)
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
>  
> +  ## This value is used to configure X86 Processor FSB clock.
> +  # @Prompt FSB Clock.
> +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    MdePkgExtra.uni
> 

Looks good to me:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Mike or Liming will have to ACK.

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53527): https://edk2.groups.io/g/devel/message/53527
Mute This Topic: https://groups.io/mt/70239981/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
Posted by Liming Gao 6 years ago
Anthony:
  This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take some time and cause the inaccurate time delay. Have you measured its impact?

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 30, 2020 12:11 AM
> To: Anthony PERARD <anthony.perard@citrix.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; xen-devel@lists.xenproject.org;
> Gao, Liming <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Feng, Bob C
> <bob.c.feng@intel.com>
> Subject: Re: [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
> 
> On 01/29/20 13:12, Anthony PERARD wrote:
> > We are going to want to change the value of PcdFSBClock at run time in
> > OvmfXen, so move it to the PcdsDynamic section.
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2490
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > CC: Bob Feng <bob.c.feng@intel.com>
> > CC: Liming Gao <liming.gao@intel.com>
> > ---
> >  MdePkg/MdePkg.dec | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
> > index d022cc5e3ef2..8f5a48346e50 100644
> > --- a/MdePkg/MdePkg.dec
> > +++ b/MdePkg/MdePkg.dec
> > @@ -2194,10 +2194,6 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
> >    # @ValidList  0x80000001 | 8, 16, 32
> >    gEfiMdePkgTokenSpaceGuid.PcdPort80DataWidth|8|UINT8|0x0000002d
> >
> > -  ## This value is used to configure X86 Processor FSB clock.
> > -  # @Prompt FSB Clock.
> > -  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> > -
> >    ## The maximum printable number of characters. UefLib functions: AsciiPrint(), AsciiErrorPrint(),
> >    #  PrintXY(), AsciiPrintXY(), Print(), ErrorPrint() base on this PCD value to print characters.
> >    # @Prompt Maximum Printable Number of Characters.
> > @@ -2297,5 +2293,9 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> >    # @Prompt Boot Timeout (s)
> >    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|0xffff|UINT16|0x0000002c
> >
> > +  ## This value is used to configure X86 Processor FSB clock.
> > +  # @Prompt FSB Clock.
> > +  gEfiMdePkgTokenSpaceGuid.PcdFSBClock|200000000|UINT32|0x0000000c
> > +
> >  [UserExtensions.TianoCore."ExtraFiles"]
> >    MdePkgExtra.uni
> >
> 
> Looks good to me:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Mike or Liming will have to ACK.
> 
> Thanks!
> Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53638): https://edk2.groups.io/g/devel/message/53638
Mute This Topic: https://groups.io/mt/70239981/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
Posted by Anthony PERARD 6 years ago
On Mon, Feb 03, 2020 at 01:34:55AM +0000, Gao, Liming wrote:
> Anthony:
>   This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take some time and cause the inaccurate time delay. Have you measured its impact?

No, I haven't. But I don't think it matter in a Xen guest, the APIC timer is
emulated anyway, so reading from a register of the APIC is going to be
slower than getting the value from the PCD services, I think.
(Hopefully, I'm not too wrong.)

But I'll give it at measuring the difference, it would be interesting to
know.

Thanks,

-- 
Anthony PERARD

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53672): https://edk2.groups.io/g/devel/message/53672
Mute This Topic: https://groups.io/mt/70239981/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
Posted by Anthony PERARD 6 years ago
On Mon, Feb 03, 2020 at 03:34:07PM +0000, Anthony PERARD wrote:
> On Mon, Feb 03, 2020 at 01:34:55AM +0000, Gao, Liming wrote:
> > Anthony:
> >   This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take some time and cause the inaccurate time delay. Have you measured its impact?
> 
> No, I haven't. But I don't think it matter in a Xen guest, the APIC timer is
> emulated anyway, so reading from a register of the APIC is going to be
> slower than getting the value from the PCD services, I think.
> (Hopefully, I'm not too wrong.)
> 
> But I'll give it at measuring the difference, it would be interesting to
> know.

Now that I've given a try, having the value as Dynamic doesn't change
anything in a Xen guest.

On my test machine, simply running GetPerformanceCounter (); takes
between 10000 ns and 20000 ns. Reading the dynamic value from PCD on the
other hand takes about 350ns. (10ns if it's static.)

When I run NanoSecondDelay() with different values, I have:
  - with static pcd:
           63894 ns to delay by 1 ns
           66611 ns to delay by 10 ns
           43927 ns to delay by 100 ns
           71367 ns to delay by 1000 ns
           55881 ns to delay by 10000 ns
          147716 ns to delay by 100000 ns
         1048335 ns to delay by 1000000 ns
        10041179 ns to delay by 10000000 ns
  - with a dynamic pcd:
           40949 ns to delay by 1 ns
           84832 ns to delay by 10 ns
           82745 ns to delay by 100 ns
           59848 ns to delay by 1000 ns
           52647 ns to delay by 10000 ns
          137051 ns to delay by 100000 ns
         1042492 ns to delay by 1000000 ns
        10036306 ns to delay by 10000000 ns

So, the kind of PCD used for PcdFSBClock on Xen (with OvmfXen) doesn't
really matter.

Anyway, thanks for the feedback.

-- 
Anthony PERARD

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53675): https://edk2.groups.io/g/devel/message/53675
Mute This Topic: https://groups.io/mt/70239981/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [Xen-devel] [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
Posted by Gao, Liming 6 years ago
Thanks for your data. Seemly, those data is acceptable on OvmfXen. For this patch, Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks
Liming
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Anthony PERARD
> Sent: Tuesday, February 4, 2020 1:26 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; xen-devel@lists.xenproject.org; Justen, Jordan L <jordan.l.justen@intel.com>; Julien Grall
> <julien@xen.org>; Feng, Bob C <bob.c.feng@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/5] MdePkg: Allow PcdFSBClock to by Dynamic
> 
> On Mon, Feb 03, 2020 at 03:34:07PM +0000, Anthony PERARD wrote:
> > On Mon, Feb 03, 2020 at 01:34:55AM +0000, Gao, Liming wrote:
> > > Anthony:
> > >   This change is OK to me. But if this PCD is configured as Dynamic, its value will be got from PCD service. This operation will take
> some time and cause the inaccurate time delay. Have you measured its impact?
> >
> > No, I haven't. But I don't think it matter in a Xen guest, the APIC timer is
> > emulated anyway, so reading from a register of the APIC is going to be
> > slower than getting the value from the PCD services, I think.
> > (Hopefully, I'm not too wrong.)
> >
> > But I'll give it at measuring the difference, it would be interesting to
> > know.
> 
> Now that I've given a try, having the value as Dynamic doesn't change
> anything in a Xen guest.
> 
> On my test machine, simply running GetPerformanceCounter (); takes
> between 10000 ns and 20000 ns. Reading the dynamic value from PCD on the
> other hand takes about 350ns. (10ns if it's static.)
> 
> When I run NanoSecondDelay() with different values, I have:
>   - with static pcd:
>            63894 ns to delay by 1 ns
>            66611 ns to delay by 10 ns
>            43927 ns to delay by 100 ns
>            71367 ns to delay by 1000 ns
>            55881 ns to delay by 10000 ns
>           147716 ns to delay by 100000 ns
>          1048335 ns to delay by 1000000 ns
>         10041179 ns to delay by 10000000 ns
>   - with a dynamic pcd:
>            40949 ns to delay by 1 ns
>            84832 ns to delay by 10 ns
>            82745 ns to delay by 100 ns
>            59848 ns to delay by 1000 ns
>            52647 ns to delay by 10000 ns
>           137051 ns to delay by 100000 ns
>          1042492 ns to delay by 1000000 ns
>         10036306 ns to delay by 10000000 ns
> 
> So, the kind of PCD used for PcdFSBClock on Xen (with OvmfXen) doesn't
> really matter.
> 
> Anyway, thanks for the feedback.
> 
> --
> Anthony PERARD
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#53675): https://edk2.groups.io/g/devel/message/53675
> Mute This Topic: https://groups.io/mt/70239981/1759384
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [liming.gao@intel.com]
> -=-=-=-=-=-=-=-=-=-=-=-


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel