| Message ID | 20161208153340.2285-11-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Anthony, Can you provide more details on why you want to expose internal APIs in the library class? What is the specific issue? Is the Local APIC in your environment not behaving the same as real HW? Thanks, Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Anthony PERARD > Sent: Thursday, December 8, 2016 7:34 AM > To: edk2-devel@lists.01.org; xen-devel@lists.xenproject.org > Cc: Anthony PERARD <anthony.perard@citrix.com> > Subject: [edk2] [PATCH RFC 10/14] UefiCpuPkg/BaseXApicX2ApicLib: Fix initialisation > on my system and ... > > declare more exported function, ReadLocalApicReg and WriteLocalApicReg. > > On my system (running OVMF inside a Xen PVH guest), writing the > interrupt information (to XAPIC_LVT_TIMER_OFFSET) would also reset the > init-count register, thus disabling the timer. > --- > UefiCpuPkg/Include/Library/LocalApicLib.h | 40 ++++++++++++++++++++++ > .../BaseXApicX2ApicLib/BaseXApicX2ApicLib.c | 10 +++--- > 2 files changed, 45 insertions(+), 5 deletions(-) > > diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h > b/UefiCpuPkg/Include/Library/LocalApicLib.h > index fc980bc..ab621f2 100644 > --- a/UefiCpuPkg/Include/Library/LocalApicLib.h > +++ b/UefiCpuPkg/Include/Library/LocalApicLib.h > @@ -48,6 +48,46 @@ SetLocalApicBaseAddress ( > ); > > /** > + Read from a local APIC register. > + > + This function reads from a local APIC register either in xAPIC or x2APIC mode. > + It is required that in xAPIC mode wider registers (64-bit or 256-bit) must be > + accessed using multiple 32-bit loads or stores, so this function only performs > + 32-bit read. > + > + @param MmioOffset The MMIO offset of the local APIC register in xAPIC mode. > + It must be 16-byte aligned. > + > + @return 32-bit Value read from the register. > +**/ > +UINT32 > +EFIAPI > +ReadLocalApicReg ( > + IN UINTN MmioOffset > + ); > + > +/** > + Write to a local APIC register. > + > + This function writes to a local APIC register either in xAPIC or x2APIC mode. > + It is required that in xAPIC mode wider registers (64-bit or 256-bit) must be > + accessed using multiple 32-bit loads or stores, so this function only performs > + 32-bit write. > + > + if the register index is invalid or unsupported in current APIC mode, then ASSERT. > + > + @param MmioOffset The MMIO offset of the local APIC register in xAPIC mode. > + It must be 16-byte aligned. > + @param Value Value to be written to the register. > +**/ > +VOID > +EFIAPI > +WriteLocalApicReg ( > + IN UINTN MmioOffset, > + IN UINT32 Value > + ); > + > +/** > Get the current local APIC mode. > > If local APIC is disabled, then ASSERT. > diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c > b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c > index e690d2a..f84a40a 100644 > --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c > +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c > @@ -820,11 +820,6 @@ InitializeApicTimer ( > // > InitializeLocalApicSoftwareEnable (TRUE); > > - // > - // Program init-count register. > - // > - WriteLocalApicReg (XAPIC_TIMER_INIT_COUNT_OFFSET, InitCount); > - > if (DivideValue != 0) { > ASSERT (DivideValue <= 128); > ASSERT (DivideValue == GetPowerOfTwo32((UINT32)DivideValue)); > @@ -848,6 +843,11 @@ InitializeApicTimer ( > LvtTimer.Bits.Mask = 0; > LvtTimer.Bits.Vector = Vector; > WriteLocalApicReg (XAPIC_LVT_TIMER_OFFSET, LvtTimer.Uint32); > + > + // > + // Program init-count register. > + // > + WriteLocalApicReg (XAPIC_TIMER_INIT_COUNT_OFFSET, InitCount); > } > > /** > -- > Anthony PERARD > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel
On 09/12/16 06:48, Kinney, Michael D wrote: > Hi Anthony, > > Can you provide more details on why you want to expose internal APIs > in the library class? > > What is the specific issue? Is the Local APIC in your environment > not behaving the same as real HW? Xen's LAPIC emulation should match real hardware (because we might even substitute it for real hardware APIC virtualisation support). If Xen's LAPIC doesn't behave like real hardware, we should fix that, but first we should understand what the correct behaviour should be. ~Andrew
On Fri, Dec 09, 2016 at 06:48:47AM +0000, Kinney, Michael D wrote: > Hi Anthony, > > Can you provide more details on why you want to expose internal APIs > in the library class? I think I'm only using WriteLocalApicReg() to change the init counter. Maybe I can just call InitializeApicTimer() again instead.
On Fri, Dec 09, 2016 at 10:43:30AM +0000, Andrew Cooper wrote: > On 09/12/16 06:48, Kinney, Michael D wrote: > > Hi Anthony, > > > > Can you provide more details on why you want to expose internal APIs > > in the library class? > > > > What is the specific issue? Is the Local APIC in your environment > > not behaving the same as real HW? > > Xen's LAPIC emulation should match real hardware (because we might even > substitute it for real hardware APIC virtualisation support). > > If Xen's LAPIC doesn't behave like real hardware, we should fix that, > but first we should understand what the correct behaviour should be. I'll try to find out what the real hardware is doing. At least, I think there where nothing in the Intel manual about the initial counter been reset.
diff --git a/UefiCpuPkg/Include/Library/LocalApicLib.h b/UefiCpuPkg/Include/Library/LocalApicLib.h index fc980bc..ab621f2 100644 --- a/UefiCpuPkg/Include/Library/LocalApicLib.h +++ b/UefiCpuPkg/Include/Library/LocalApicLib.h @@ -48,6 +48,46 @@ SetLocalApicBaseAddress ( ); /** + Read from a local APIC register. + + This function reads from a local APIC register either in xAPIC or x2APIC mode. + It is required that in xAPIC mode wider registers (64-bit or 256-bit) must be + accessed using multiple 32-bit loads or stores, so this function only performs + 32-bit read. + + @param MmioOffset The MMIO offset of the local APIC register in xAPIC mode. + It must be 16-byte aligned. + + @return 32-bit Value read from the register. +**/ +UINT32 +EFIAPI +ReadLocalApicReg ( + IN UINTN MmioOffset + ); + +/** + Write to a local APIC register. + + This function writes to a local APIC register either in xAPIC or x2APIC mode. + It is required that in xAPIC mode wider registers (64-bit or 256-bit) must be + accessed using multiple 32-bit loads or stores, so this function only performs + 32-bit write. + + if the register index is invalid or unsupported in current APIC mode, then ASSERT. + + @param MmioOffset The MMIO offset of the local APIC register in xAPIC mode. + It must be 16-byte aligned. + @param Value Value to be written to the register. +**/ +VOID +EFIAPI +WriteLocalApicReg ( + IN UINTN MmioOffset, + IN UINT32 Value + ); + +/** Get the current local APIC mode. If local APIC is disabled, then ASSERT. diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c index e690d2a..f84a40a 100644 --- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c +++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c @@ -820,11 +820,6 @@ InitializeApicTimer ( // InitializeLocalApicSoftwareEnable (TRUE); - // - // Program init-count register. - // - WriteLocalApicReg (XAPIC_TIMER_INIT_COUNT_OFFSET, InitCount); - if (DivideValue != 0) { ASSERT (DivideValue <= 128); ASSERT (DivideValue == GetPowerOfTwo32((UINT32)DivideValue)); @@ -848,6 +843,11 @@ InitializeApicTimer ( LvtTimer.Bits.Mask = 0; LvtTimer.Bits.Vector = Vector; WriteLocalApicReg (XAPIC_LVT_TIMER_OFFSET, LvtTimer.Uint32); + + // + // Program init-count register. + // + WriteLocalApicReg (XAPIC_TIMER_INIT_COUNT_OFFSET, InitCount); } /**