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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#82664): https://edk2.groups.io/g/devel/message/82664 Mute This Topic: https://groups.io/mt/86085771/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
