Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe: Duplicate OvmfPkg/LocalApicTimerDxe driver

["Michael D Kinney" <michael.d.kinney@intel.com>]

Unit tests for the math calculations would help with reviews too.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, January 16, 2024 2:03 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Pedro Falcato
> <pedro.falcato@gmail.com>; devel@edk2.groups.io; Ni, Ray
> <ray.ni@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kumar, Rahul
> R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/6] UefiCpuPkg/LocalApicTimerDxe:
> Duplicate OvmfPkg/LocalApicTimerDxe driver
> 
> On 1/15/24 19:10, Kinney, Michael D wrote:
> > Hi Ray,
> >
> > I think nesting may be possible in physical platforms, but very hard
> > to induce.
> >
> > One option is to consolidate to a single LocalApicTimerDxe
> > implementation in the UefiCpuPkg, but allow the platform DSC to either
> > specify a Null NestedInterruptTplLib for physical platforms or the
> > full one from the OvmfPkg for VM use cases.
> >
> > The other changes could be included for OvmfPkg use cases.  If the VM
> > does not support the CPUID leafs, then the PCD value should be used.
> 
> All of this sounds great!
> 
> (And I do think that *some* nesting should not be a problem, on either
> physical or virtual platforms, as (IIRC) the interrupt handler stack can
> accommodate a certain level of reentrancy. It's the "storm" that is a
> problem, but that should never occur on physical machines, I reckon.)
> 
> Assuming a v2 is coming up, my advance request would be for Ray to
> re-review the math in patch #4, before posting v2, focusing on integer
> overflows, and SafeIntLib (if needed). I've not looked at the patch in
> detail yet, but I reviewed something similar not so long ago [1]. The
> latter frequency calculation code had been quite commonly used in edk2,
> and I needed hours to review just that one patch. Plus, the review
> turned up a number of issues to fix. So, preferably such a patch should
> not only prevent any integer overflows, but also clarify, in comments,
> why overflows are impossible, and/or how they are prevented.
> 
> [1] https://edk2.groups.io/g/devel/message/111613
>     http://mid.mail-archive.com/2e42db7c-a927-f47b-7d80-
> 632895b11e1b@redhat.com
> 
> Thanks!
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113922): https://edk2.groups.io/g/devel/message/113922
Mute This Topic: https://groups.io/mt/103734961/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-