Re: [edk2-devel] [PATCH V2 28/28] OvmfPkg: Add LocalApicTimerDxe

["Min Xu" <min.m.xu@intel.com>]

On October 25, 2021 7:28 PM, Gerd Hoffmann wrote:
> On Mon, Oct 25, 2021 at 07:37:33AM +0000, Min Xu wrote:
> > On October 12, 2021 9:02 PM, Gerd Hoffmann wrote:
> > > On Tue, Oct 05, 2021 at 11:39:39AM +0800, Min Xu wrote:
> > > > RFC： https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> > > >
> > > > TDX guest supports LocalApicTimer. But in current OvmfPkg the
> > > > supported timer is 8254TimerDxe. So
> > > > gUefiOvmfPkgTokenSpaceGuid.PcdTimerSelector
> > > > is introduced to select the running Timer. The Timer driver will
> > > > check the TimerSelector in its entry point. The default Timer is 8254.
> > >
> > > Hmm.
> > >
> > > We already have a local apic timer implementation (XenTimerDxe).
> > > Works fine with kvm, microvm already uses that.  See commit
> > > 76602f45dcd9
> > > ("OvmfPkg/Microvm: use XenTimerDxe (lapic timer)").
> > >
> > > So, first I'd suggest to just use that (maybe rename the thing to
> > > avoid confusion as it isn't really Xen specific).
> >
> > Thanks for reminder. We can use XenTimerDxe as the LocalApicTimerDxe in
> Tdx guest. There will be a separate patch to rename XenTimerDxe to
> LocalApicTimerDxe in the next version.
> 
> You can also split this off into a separate patch series as it shouldn't have any tdx
> dependency.
> 
> > I am not quite sure if there will be any side effect if we switch ovmf
> > (X64) from 8254 to lapic unconditionally. Quick smoke test does show
> > no obvious problems (EDK2 CI shows no error either). But since 8254
> > timer has already been used in OvmfPkgX64, then there always a reason
> > why 8254 is used.
> 
> Note that 8254TimerDxe was not written for OVMF, it was moved over from
> PcAtChipsetPkg to OvmfPkg in 2019.  Probably because OVMF was the only user
> left.
> 
> Most likely the reason OVMF used 8254TimerDxe initially was that it could just
> use the existing driver in PcAtChipsetPkg.  And it simply hasn't been changed ever.
> 
> Hmm, CSM support was moved in 2019 too, checking ...
> 
> Yes, CSM support depends on 8254 (and 8259) drivers.
> 
> > I am thinking if it is a more secure way to introduce PcdTimerSelector
> > (to select timer in run-time) this time.  We can revisit this proposal
> > (switch ovmf from 8254 to lapic unconditionally) when we are pretty
> > sure there is no side effect in the future.
> 
> I still think we don't need a runtime switch.  Continue using 8254TimerDxe for
> CSM_ENABLE=TRUE builds should be enough.
> 
Thanks for your detailed explanation. I agree we don't need a runtime switch. Just use CSM_ENABLE=TRUE in *.dsc/*.fdf to switch 8254 and lapic in build time.
I will submit a separate patch series for this change.

There are 4 .dsc which include the 8254Timer. 
 - OvmfPkg/AmdSev/AmdSevX64.dsc
 - OvmfPkg/OvmfPkgIa32.dsc
 - OvmfPkg/OvmfPkgIa32X64.dsc
 - OvmfPkg/OvmfPkgX64.dsc

Do you think we should apply the changes to all above 4 .dsc?

Thanks
Min