BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3711

XenTimerDxe is a local Apic timer driver and it has nothing to do
with Xen. So rename it to LocalApicTimerDxe.

After renaming, LocalApicTimerDxe is used in OvmfPkg if CSM_ENABLE=FALSE.
Otherwise 8254 timer is used.

Patch #1:
Rename XenTimerDxe to LocalApicTimerDxe

Patch #2:
Switch timer in build time for OvmfPkg. If CSM_ENABLE=TRUE, 8254 timer
is used, otherwise the timer is LocalApicTimerDxe.

Code at: https://github.com/mxu9/edk2/tree/ovmf_lapic_timer.v2

v2 changes:
 - Add gEfiMdePkgTokenSpaceGuid.PcdFSBClock in *.dsc

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min Xu (2):
  OvmfPkg: Rename XenTimerDxe to LocalApicTimerDxe
  OvmfPkg: Switch timer in build time for OvmfPkg

 OvmfPkg/AmdSev/AmdSevX64.dsc                           |  5 +++--
 OvmfPkg/AmdSev/AmdSevX64.fdf                           |  3 +--
 .../LocalApicTimerDxe.c}                               |  7 +++----
 .../LocalApicTimerDxe.h}                               |  4 ++--
 .../LocalApicTimerDxe.inf}                             |  6 +++---
 OvmfPkg/Microvm/MicrovmX64.dsc                         |  2 +-
 OvmfPkg/Microvm/MicrovmX64.fdf                         |  2 +-
 OvmfPkg/OvmfPkgIa32.dsc                                | 10 +++++++++-
 OvmfPkg/OvmfPkgIa32.fdf                                |  8 ++++++--
 OvmfPkg/OvmfPkgIa32X64.dsc                             | 10 +++++++++-
 OvmfPkg/OvmfPkgIa32X64.fdf                             |  8 ++++++--
 OvmfPkg/OvmfPkgX64.dsc                                 | 10 +++++++++-
 OvmfPkg/OvmfPkgX64.fdf                                 |  8 ++++++--
 OvmfPkg/OvmfXen.dsc                                    |  2 +-
 OvmfPkg/OvmfXen.fdf                                    |  2 +-
 15 files changed, 61 insertions(+), 26 deletions(-)
 rename OvmfPkg/{XenTimerDxe/XenTimerDxe.c => LocalApicTimerDxe/LocalApicTimerDxe.c} (95%)
 rename OvmfPkg/{XenTimerDxe/XenTimerDxe.h => LocalApicTimerDxe/LocalApicTimerDxe.h} (96%)
 rename OvmfPkg/{XenTimerDxe/XenTimerDxe.inf => LocalApicTimerDxe/LocalApicTimerDxe.inf} (86%)

-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#82964): https://edk2.groups.io/g/devel/message/82964
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, Nov 01, 2021 at 04:46:03PM +0800, Min Xu wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3711
> 
> XenTimerDxe is a local Apic timer driver and it has nothing to do
> with Xen. So rename it to LocalApicTimerDxe.
> 
> After renaming, LocalApicTimerDxe is used in OvmfPkg if CSM_ENABLE=FALSE.
> Otherwise 8254 timer is used.
> 
> Patch #1:
> Rename XenTimerDxe to LocalApicTimerDxe
> 
> Patch #2:
> Switch timer in build time for OvmfPkg. If CSM_ENABLE=TRUE, 8254 timer
> is used, otherwise the timer is LocalApicTimerDxe.
> 
> Code at: https://github.com/mxu9/edk2/tree/ovmf_lapic_timer.v2
> 
> v2 changes:
>  - Add gEfiMdePkgTokenSpaceGuid.PcdFSBClock in *.dsc
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> Min Xu (2):
>   OvmfPkg: Rename XenTimerDxe to LocalApicTimerDxe
>   OvmfPkg: Switch timer in build time for OvmfPkg

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83107): https://edk2.groups.io/g/devel/message/83107
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Min
I think it is good idea to rename to LocalApicTimeDxe.

I double checked dependency in INF file. But I did not know why it depends upon OvmfPkg.

=======================
[LibraryClasses]
  UefiBootServicesTableLib
  BaseLib
  DebugLib
  UefiDriverEntryPoint
  LocalApicLib

...

[Protocols]
  gEfiCpuArchProtocolGuid       ## CONSUMES
  gEfiTimerArchProtocolGuid     ## PRODUCES
[Pcd]
  gEfiMdePkgTokenSpaceGuid.PcdFSBClock  ## CONSUMES
[Depex]
  gEfiCpuArchProtocolGuid
=======================


Do you think it should so generic that it can remove OvmfPkg dependency and be moved to UefiCpuPkg?

Thank you
Yao Jiewen


> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Monday, November 1, 2021 4:46 PM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH V2 0/2] Rename XenTimerDxe to LocalApicTimerDxe
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3711
> 
> XenTimerDxe is a local Apic timer driver and it has nothing to do
> with Xen. So rename it to LocalApicTimerDxe.
> 
> After renaming, LocalApicTimerDxe is used in OvmfPkg if CSM_ENABLE=FALSE.
> Otherwise 8254 timer is used.
> 
> Patch #1:
> Rename XenTimerDxe to LocalApicTimerDxe
> 
> Patch #2:
> Switch timer in build time for OvmfPkg. If CSM_ENABLE=TRUE, 8254 timer
> is used, otherwise the timer is LocalApicTimerDxe.
> 
> Code at: https://github.com/mxu9/edk2/tree/ovmf_lapic_timer.v2
> 
> v2 changes:
>  - Add gEfiMdePkgTokenSpaceGuid.PcdFSBClock in *.dsc
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> Min Xu (2):
>   OvmfPkg: Rename XenTimerDxe to LocalApicTimerDxe
>   OvmfPkg: Switch timer in build time for OvmfPkg
> 
>  OvmfPkg/AmdSev/AmdSevX64.dsc                           |  5 +++--
>  OvmfPkg/AmdSev/AmdSevX64.fdf                           |  3 +--
>  .../LocalApicTimerDxe.c}                               |  7 +++----
>  .../LocalApicTimerDxe.h}                               |  4 ++--
>  .../LocalApicTimerDxe.inf}                             |  6 +++---
>  OvmfPkg/Microvm/MicrovmX64.dsc                         |  2 +-
>  OvmfPkg/Microvm/MicrovmX64.fdf                         |  2 +-
>  OvmfPkg/OvmfPkgIa32.dsc                                | 10 +++++++++-
>  OvmfPkg/OvmfPkgIa32.fdf                                |  8 ++++++--
>  OvmfPkg/OvmfPkgIa32X64.dsc                             | 10 +++++++++-
>  OvmfPkg/OvmfPkgIa32X64.fdf                             |  8 ++++++--
>  OvmfPkg/OvmfPkgX64.dsc                                 | 10 +++++++++-
>  OvmfPkg/OvmfPkgX64.fdf                                 |  8 ++++++--
>  OvmfPkg/OvmfXen.dsc                                    |  2 +-
>  OvmfPkg/OvmfXen.fdf                                    |  2 +-
>  15 files changed, 61 insertions(+), 26 deletions(-)
>  rename OvmfPkg/{XenTimerDxe/XenTimerDxe.c =>
> LocalApicTimerDxe/LocalApicTimerDxe.c} (95%)
>  rename OvmfPkg/{XenTimerDxe/XenTimerDxe.h =>
> LocalApicTimerDxe/LocalApicTimerDxe.h} (96%)
>  rename OvmfPkg/{XenTimerDxe/XenTimerDxe.inf =>
> LocalApicTimerDxe/LocalApicTimerDxe.inf} (86%)
> 
> --
> 2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83109): https://edk2.groups.io/g/devel/message/83109
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> Do you think it should so generic that it can remove OvmfPkg dependency and be moved to UefiCpuPkg?

It's not fully standalone, the driver needs to know the lapic frequency
(that is the reason why PcdFSBClock exists).

For KVM this is easy, the lapic uses a fixed frequency so it can simply
be set in the .dsc file.

For Xen the lapic frequency is the same as the tsc frequency, so the xen
code (platform init I think) goes figure the tsc freq and sets
PcdFSBClock accordingly.

So, when you want use the driver elsewhere you need to fill that gap,
and there is little reason to go that extra mile because on physical
hardware you have other options like using the hpet timer.

[ qemu supports hpet emulation but it is disabled by default for
  performance reasons, other timers have less virtualization
  overhead. ]

I'd suggest to keep it in OvmfPkg.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83118): https://edk2.groups.io/g/devel/message/83118
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

We can state, this driver is to support fixed frequency.
If a real platform happens to have fixed frequency, then it can be used.

gEfiMdePkgTokenSpaceGuid.PcdFSBClock  is defined in MdePkg.
The consumer need set PcdFSBClock. 

I don't see a need to bind to OVMF.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Tuesday, November 2, 2021 5:59 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/2] Rename XenTimerDxe to
> LocalApicTimerDxe
> 
>   Hi,
> 
> > Do you think it should so generic that it can remove OvmfPkg dependency and
> be moved to UefiCpuPkg?
> 
> It's not fully standalone, the driver needs to know the lapic frequency
> (that is the reason why PcdFSBClock exists).
> 
> For KVM this is easy, the lapic uses a fixed frequency so it can simply
> be set in the .dsc file.
> 
> For Xen the lapic frequency is the same as the tsc frequency, so the xen
> code (platform init I think) goes figure the tsc freq and sets
> PcdFSBClock accordingly.
> 
> So, when you want use the driver elsewhere you need to fill that gap,
> and there is little reason to go that extra mile because on physical
> hardware you have other options like using the hpet timer.
> 
> [ qemu supports hpet emulation but it is disabled by default for
>   performance reasons, other timers have less virtualization
>   overhead. ]
> 
> I'd suggest to keep it in OvmfPkg.
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83124): https://edk2.groups.io/g/devel/message/83124
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, Nov 02, 2021 at 10:05:56AM +0000, Yao, Jiewen wrote:
> We can state, this driver is to support fixed frequency.
> If a real platform happens to have fixed frequency, then it can be used.
> 
> gEfiMdePkgTokenSpaceGuid.PcdFSBClock  is defined in MdePkg.
> The consumer need set PcdFSBClock. 
> 
> I don't see a need to bind to OVMF.

Not needed indeed, but I doubt it'll actually be used outside
OvmfPkg for the reasons outlined in the last reply.

When we move it out we should clearly document the PcdFSBClock
so any potential users are aware of it.  Maybe drop a short
README into the directory then?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83126): https://edk2.groups.io/g/devel/message/83126
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Usually, we put those information in the header of module INF file.


> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Tuesday, November 2, 2021 6:21 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Brijesh Singh <brijesh.singh@amd.com>; Erdem Aktas
> <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V2 0/2] Rename XenTimerDxe to
> LocalApicTimerDxe
> 
> On Tue, Nov 02, 2021 at 10:05:56AM +0000, Yao, Jiewen wrote:
> > We can state, this driver is to support fixed frequency.
> > If a real platform happens to have fixed frequency, then it can be used.
> >
> > gEfiMdePkgTokenSpaceGuid.PcdFSBClock  is defined in MdePkg.
> > The consumer need set PcdFSBClock.
> >
> > I don't see a need to bind to OVMF.
> 
> Not needed indeed, but I doubt it'll actually be used outside
> OvmfPkg for the reasons outlined in the last reply.
> 
> When we move it out we should clearly document the PcdFSBClock
> so any potential users are aware of it.  Maybe drop a short
> README into the directory then?
> 
> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#83127): https://edk2.groups.io/g/devel/message/83127
Mute This Topic: https://groups.io/mt/86735078/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-