Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code

[Roger Pau Monné]

On Tue, Jan 10, 2017 at 05:54:49PM +0100, Laszlo Ersek wrote:
> On 01/10/17 17:18, Anthony PERARD wrote:
> > On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote:
> >> On 12/08/16 16:33, Anthony PERARD wrote:
> >>> diff --git a/OvmfPkg/XenPlatformPei/Platform.c 
> >>> b/OvmfPkg/XenPlatformPei/Platform.c
> >>> index bf78878..9fc713c 100644
> >>> --- a/OvmfPkg/XenPlatformPei/Platform.c
> >>> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> >>> @@ -362,6 +362,10 @@ MiscInitialization (
> >>>AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL);
> >>>AcpiEnBit  = ICH9_ACPI_CNTL_ACPI_EN;
> >>>break;
> >>> +case 0x:
> >>> +  // xen PVH, ignore
> >>> +  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, 
> >>> mHostBridgeDevId);
> >>> +  return;
> >>
> >> Can we create a new macro for 0x in
> >> "OvmfPkg/Include/OvmfPlatforms.h", and use that here?
> >>
> >> Normally I would suggest a separate header file under "OvmfPkg/Include",
> >> similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new
> >> file in "OvmfPlatforms.h", but here we only need a single "chipset
> >> identifier", so I guess that can go directly into "OvmfPlatforms.h".
> >>
> >> I mainly suggest this because in patch 08/14, the same 0x case label
> >> is being added to code shared with QEMU, and using a verbose macro there
> >> is much better than a magic number. In turn, we should use the same
> >> macro here, I think.
> > 
> > This value is just the result of a read to an incorrect location, and
> > the return value is -1. There is no PCI when running in Xen PVH, at
> > least for now. I think I should try harder to find out if OVMF is
> > running in PVH, and so I would not need this case 0x at all.
> > 
> 
> Right, that would be best.
> 
> In fact, this affects two modules, (Xen)PlatformPei and
> PlatformBootManagerLib. In both, we already have the XenDetected()
> function. Maybe you can add a XenPvhDetected() function as well (which
> would return TRUE only for PVH, while the original XenDetected() would
> continue returning TRUE for both HVM and PVH). Then the PCI host bridge
> ID checking could be entirely short-circuited if XenPvhDetected()
> returned TRUE first.

It's very likely that when doing PCI-passthrough to a PVH guest an emulated
PCI host bridge is going to be available, so short-circuiting the bridge check
just based on PVH detection would then be wrong.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code

[Laszlo Ersek]

On 01/10/17 17:18, Anthony PERARD wrote:
> On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote:
>> On 12/08/16 16:33, Anthony PERARD wrote:
>>> - learn about memory size from the E820
>>> - ignore error if host bridge devid is 0x, PVH does not have PCI and
>>>   reading from unexisting device return -1.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Anthony PERARD 
>>> ---
>>>  OvmfPkg/XenPlatformPei/MemDetect.c | 71 
>>> ++
>>>  OvmfPkg/XenPlatformPei/Platform.c  |  5 +++
>>>  OvmfPkg/XenPlatformPei/Platform.h  | 10 ++
>>>  3 files changed, 86 insertions(+)
>>>
>>> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c 
>>> b/OvmfPkg/XenPlatformPei/MemDetect.c
>>> index 4ecdf5e..0d80775 100644
>>> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
>>> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
>>> @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth;
>>>  STATIC UINT32 mS3AcpiReservedMemoryBase;
>>>  STATIC UINT32 mS3AcpiReservedMemorySize;
>>>  
>>> +STATIC UINT32 mXenLowerMemorySize = 0;
>>> +STATIC UINT64 mXenHighMemorySize = 0;
>>> +
>>> +VOID
>>> +XenReadMemorySizes (
>>> +  VOID
>>> +  )
>>> +{
>>> +  EFI_E820_ENTRY64  *E820Map;
>>> +  UINT32E820EntriesCount;
>>> +  EFI_STATUS Status;
>>> +
>>> +  Status = XenGetE820Map (, );
>>> +  ASSERT_EFI_ERROR (Status);
>>> +
>>> +  mXenLowerMemorySize = 0;
>>> +  mXenHighMemorySize = 0;
>>> +
>>> +  if (E820EntriesCount > 0) {
>>> +EFI_E820_ENTRY64 *Entry;
>>> +UINT32 Loop;
>>> +
>>> +for (Loop = 0; Loop < E820EntriesCount; Loop++) {
>>> +  Entry = E820Map + Loop;
>>> +
>>> +  //
>>> +  // Only care about RAM
>>> +  //
>>> +  if (Entry->Type != EfiAcpiAddressRangeMemory) {
>>> +continue;
>>> +  }
>>> +
>>> +  if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) {
>>> +continue;
>>> +  }
>>> +
>>> +  if (Entry->BaseAddr < SIZE_16MB) {
>>> +UINT64 bottom = Entry->BaseAddr;
>>> +UINT64 size = Entry->Length;
>>> +size -= SIZE_16MB - bottom;
>>> +bottom = SIZE_16MB;
>>> +mXenLowerMemorySize += size;
>>> +continue;
>>> +  }
>>> +  if (Entry->BaseAddr <= SIZE_4GB) {
>>> +UINT64 size = Entry->Length;
>>> +mXenLowerMemorySize += size;
>>> +continue;
>>> +  }
>>> +
>>> +  if (Entry->BaseAddr == SIZE_4GB) {
>>> +mXenHighMemorySize = Entry->Length;
>>> +continue;
>>> +  }
>>> +
>>> +  DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n",
>>> +  __FUNCTION__,
>>> +  Entry->BaseAddr,
>>> +  Entry->Length));
>>> +}
>>> +mXenLowerMemorySize += SIZE_16MB;
>>> +  }
>>> +}
>>> +
>>>  UINT32
>>>  GetSystemMemorySizeBelow4gb (
>>>VOID
>>> @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb (
>>>UINT8 Cmos0x34;
>>>UINT8 Cmos0x35;
>>>  
>>> +  if (mXen && mXenLowerMemorySize)
>>> +return mXenLowerMemorySize;
>>> +
>>>//
>>>// CMOS 0x34/0x35 specifies the system memory above 16 MB.
>>>// * CMOS(0x35) is the high byte
>>> @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb (
>>>UINT32 Size;
>>>UINTN  CmosIndex;
>>>  
>>> +  // if lower memory is specified that way, return also high memory
>>> +  if (mXen && mXenLowerMemorySize)
>>> +return mXenHighMemorySize;
>>> +
>>>//
>>>// CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
>>>// * CMOS(0x5d) is the most significant size byte
>>> diff --git a/OvmfPkg/XenPlatformPei/Platform.c 
>>> b/OvmfPkg/XenPlatformPei/Platform.c
>>> index bf78878..9fc713c 100644
>>> --- a/OvmfPkg/XenPlatformPei/Platform.c
>>> +++ b/OvmfPkg/XenPlatformPei/Platform.c
>>> @@ -362,6 +362,10 @@ MiscInitialization (
>>>AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL);
>>>AcpiEnBit  = ICH9_ACPI_CNTL_ACPI_EN;
>>>break;
>>> +case 0x:
>>> +  // xen PVH, ignore
>>> +  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
>>> +  return;
>>
>> Can we create a new macro for 0x in
>> "OvmfPkg/Include/OvmfPlatforms.h", and use that here?
>>
>> Normally I would suggest a separate header file under "OvmfPkg/Include",
>> similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new
>> file in "OvmfPlatforms.h", but here we only need a single "chipset
>> identifier", so I guess that can go directly into "OvmfPlatforms.h".
>>
>> I mainly suggest this because in patch 08/14, the same 0x case label
>> is being added to code shared with QEMU, and using a verbose macro there
>> is much better than a magic number. In turn, we should use the same
>> macro here, I think.
> 
> This value is just the result of a read to an incorrect location, and
> the return value is -1. There is no PCI when running in Xen PVH, at
> least for now. I think I should try harder to find out if OVMF is
> running in PVH, and so I would not need this case 

Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code

[Anthony PERARD]

On Thu, Jan 05, 2017 at 11:30:32AM +0100, Laszlo Ersek wrote:
> On 12/08/16 16:33, Anthony PERARD wrote:
> > - learn about memory size from the E820
> > - ignore error if host bridge devid is 0x, PVH does not have PCI and
> >   reading from unexisting device return -1.
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Anthony PERARD 
> > ---
> >  OvmfPkg/XenPlatformPei/MemDetect.c | 71 
> > ++
> >  OvmfPkg/XenPlatformPei/Platform.c  |  5 +++
> >  OvmfPkg/XenPlatformPei/Platform.h  | 10 ++
> >  3 files changed, 86 insertions(+)
> > 
> > diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c 
> > b/OvmfPkg/XenPlatformPei/MemDetect.c
> > index 4ecdf5e..0d80775 100644
> > --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> > +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> > @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth;
> >  STATIC UINT32 mS3AcpiReservedMemoryBase;
> >  STATIC UINT32 mS3AcpiReservedMemorySize;
> >  
> > +STATIC UINT32 mXenLowerMemorySize = 0;
> > +STATIC UINT64 mXenHighMemorySize = 0;
> > +
> > +VOID
> > +XenReadMemorySizes (
> > +  VOID
> > +  )
> > +{
> > +  EFI_E820_ENTRY64  *E820Map;
> > +  UINT32E820EntriesCount;
> > +  EFI_STATUS Status;
> > +
> > +  Status = XenGetE820Map (, );
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  mXenLowerMemorySize = 0;
> > +  mXenHighMemorySize = 0;
> > +
> > +  if (E820EntriesCount > 0) {
> > +EFI_E820_ENTRY64 *Entry;
> > +UINT32 Loop;
> > +
> > +for (Loop = 0; Loop < E820EntriesCount; Loop++) {
> > +  Entry = E820Map + Loop;
> > +
> > +  //
> > +  // Only care about RAM
> > +  //
> > +  if (Entry->Type != EfiAcpiAddressRangeMemory) {
> > +continue;
> > +  }
> > +
> > +  if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) {
> > +continue;
> > +  }
> > +
> > +  if (Entry->BaseAddr < SIZE_16MB) {
> > +UINT64 bottom = Entry->BaseAddr;
> > +UINT64 size = Entry->Length;
> > +size -= SIZE_16MB - bottom;
> > +bottom = SIZE_16MB;
> > +mXenLowerMemorySize += size;
> > +continue;
> > +  }
> > +  if (Entry->BaseAddr <= SIZE_4GB) {
> > +UINT64 size = Entry->Length;
> > +mXenLowerMemorySize += size;
> > +continue;
> > +  }
> > +
> > +  if (Entry->BaseAddr == SIZE_4GB) {
> > +mXenHighMemorySize = Entry->Length;
> > +continue;
> > +  }
> > +
> > +  DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n",
> > +  __FUNCTION__,
> > +  Entry->BaseAddr,
> > +  Entry->Length));
> > +}
> > +mXenLowerMemorySize += SIZE_16MB;
> > +  }
> > +}
> > +
> >  UINT32
> >  GetSystemMemorySizeBelow4gb (
> >VOID
> > @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb (
> >UINT8 Cmos0x34;
> >UINT8 Cmos0x35;
> >  
> > +  if (mXen && mXenLowerMemorySize)
> > +return mXenLowerMemorySize;
> > +
> >//
> >// CMOS 0x34/0x35 specifies the system memory above 16 MB.
> >// * CMOS(0x35) is the high byte
> > @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb (
> >UINT32 Size;
> >UINTN  CmosIndex;
> >  
> > +  // if lower memory is specified that way, return also high memory
> > +  if (mXen && mXenLowerMemorySize)
> > +return mXenHighMemorySize;
> > +
> >//
> >// CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
> >// * CMOS(0x5d) is the most significant size byte
> > diff --git a/OvmfPkg/XenPlatformPei/Platform.c 
> > b/OvmfPkg/XenPlatformPei/Platform.c
> > index bf78878..9fc713c 100644
> > --- a/OvmfPkg/XenPlatformPei/Platform.c
> > +++ b/OvmfPkg/XenPlatformPei/Platform.c
> > @@ -362,6 +362,10 @@ MiscInitialization (
> >AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL);
> >AcpiEnBit  = ICH9_ACPI_CNTL_ACPI_EN;
> >break;
> > +case 0x:
> > +  // xen PVH, ignore
> > +  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> > +  return;
> 
> Can we create a new macro for 0x in
> "OvmfPkg/Include/OvmfPlatforms.h", and use that here?
> 
> Normally I would suggest a separate header file under "OvmfPkg/Include",
> similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new
> file in "OvmfPlatforms.h", but here we only need a single "chipset
> identifier", so I guess that can go directly into "OvmfPlatforms.h".
> 
> I mainly suggest this because in patch 08/14, the same 0x case label
> is being added to code shared with QEMU, and using a verbose macro there
> is much better than a magic number. In turn, we should use the same
> macro here, I think.

This value is just the result of a read to an incorrect location, and
the return value is -1. There is no PCI when running in Xen PVH, at
least for now. I think I should try harder to find out if OVMF is
running in PVH, and so I would not need this case 0x at all.

-- 
Anthony PERARD

Re: [Xen-devel] [edk2] [PATCH RFC 06/14] OvmfPkg/XenPlatformPei: Add xen PVH specific code

[Laszlo Ersek]

On 12/08/16 16:33, Anthony PERARD wrote:
> - learn about memory size from the E820
> - ignore error if host bridge devid is 0x, PVH does not have PCI and
>   reading from unexisting device return -1.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Anthony PERARD 
> ---
>  OvmfPkg/XenPlatformPei/MemDetect.c | 71 
> ++
>  OvmfPkg/XenPlatformPei/Platform.c  |  5 +++
>  OvmfPkg/XenPlatformPei/Platform.h  | 10 ++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/MemDetect.c 
> b/OvmfPkg/XenPlatformPei/MemDetect.c
> index 4ecdf5e..0d80775 100644
> --- a/OvmfPkg/XenPlatformPei/MemDetect.c
> +++ b/OvmfPkg/XenPlatformPei/MemDetect.c
> @@ -42,6 +42,70 @@ UINT8 mPhysMemAddressWidth;
>  STATIC UINT32 mS3AcpiReservedMemoryBase;
>  STATIC UINT32 mS3AcpiReservedMemorySize;
>  
> +STATIC UINT32 mXenLowerMemorySize = 0;
> +STATIC UINT64 mXenHighMemorySize = 0;
> +
> +VOID
> +XenReadMemorySizes (
> +  VOID
> +  )
> +{
> +  EFI_E820_ENTRY64  *E820Map;
> +  UINT32E820EntriesCount;
> +  EFI_STATUS Status;
> +
> +  Status = XenGetE820Map (, );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  mXenLowerMemorySize = 0;
> +  mXenHighMemorySize = 0;
> +
> +  if (E820EntriesCount > 0) {
> +EFI_E820_ENTRY64 *Entry;
> +UINT32 Loop;
> +
> +for (Loop = 0; Loop < E820EntriesCount; Loop++) {
> +  Entry = E820Map + Loop;
> +
> +  //
> +  // Only care about RAM
> +  //
> +  if (Entry->Type != EfiAcpiAddressRangeMemory) {
> +continue;
> +  }
> +
> +  if ((Entry->BaseAddr + Entry->Length) <= SIZE_16MB) {
> +continue;
> +  }
> +
> +  if (Entry->BaseAddr < SIZE_16MB) {
> +UINT64 bottom = Entry->BaseAddr;
> +UINT64 size = Entry->Length;
> +size -= SIZE_16MB - bottom;
> +bottom = SIZE_16MB;
> +mXenLowerMemorySize += size;
> +continue;
> +  }
> +  if (Entry->BaseAddr <= SIZE_4GB) {
> +UINT64 size = Entry->Length;
> +mXenLowerMemorySize += size;
> +continue;
> +  }
> +
> +  if (Entry->BaseAddr == SIZE_4GB) {
> +mXenHighMemorySize = Entry->Length;
> +continue;
> +  }
> +
> +  DEBUG ((EFI_D_INFO, "%a: ignored XenE820 entry (0x%llx, 0x%llx)\n",
> +  __FUNCTION__,
> +  Entry->BaseAddr,
> +  Entry->Length));
> +}
> +mXenLowerMemorySize += SIZE_16MB;
> +  }
> +}
> +
>  UINT32
>  GetSystemMemorySizeBelow4gb (
>VOID
> @@ -50,6 +114,9 @@ GetSystemMemorySizeBelow4gb (
>UINT8 Cmos0x34;
>UINT8 Cmos0x35;
>  
> +  if (mXen && mXenLowerMemorySize)
> +return mXenLowerMemorySize;
> +
>//
>// CMOS 0x34/0x35 specifies the system memory above 16 MB.
>// * CMOS(0x35) is the high byte
> @@ -74,6 +141,10 @@ GetSystemMemorySizeAbove4gb (
>UINT32 Size;
>UINTN  CmosIndex;
>  
> +  // if lower memory is specified that way, return also high memory
> +  if (mXen && mXenLowerMemorySize)
> +return mXenHighMemorySize;
> +
>//
>// CMOS 0x5b-0x5d specifies the system memory above 4GB MB.
>// * CMOS(0x5d) is the most significant size byte
> diff --git a/OvmfPkg/XenPlatformPei/Platform.c 
> b/OvmfPkg/XenPlatformPei/Platform.c
> index bf78878..9fc713c 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.c
> +++ b/OvmfPkg/XenPlatformPei/Platform.c
> @@ -362,6 +362,10 @@ MiscInitialization (
>AcpiCtlReg = POWER_MGMT_REGISTER_Q35 (ICH9_ACPI_CNTL);
>AcpiEnBit  = ICH9_ACPI_CNTL_ACPI_EN;
>break;
> +case 0x:
> +  // xen PVH, ignore
> +  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> +  return;

Can we create a new macro for 0x in
"OvmfPkg/Include/OvmfPlatforms.h", and use that here?

Normally I would suggest a separate header file under "OvmfPkg/Include",
similar to "Q35MchIch9.h" and "I440FxPiix4.h", and to include that new
file in "OvmfPlatforms.h", but here we only need a single "chipset
identifier", so I guess that can go directly into "OvmfPlatforms.h".

I mainly suggest this because in patch 08/14, the same 0x case label
is being added to code shared with QEMU, and using a verbose macro there
is much better than a magic number. In turn, we should use the same
macro here, I think.

Looks OK otherwise.

Thanks
Laszlo

>  default:
>DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",
>  __FUNCTION__, mHostBridgeDevId));
> @@ -503,6 +507,7 @@ InitializeXenPlatform (
>DebugDumpCmos ();
>  
>XenDetect ();
> +  XenReadMemorySizes ();
>  
>BootModeInitialization ();
>AddressWidthInitialization ();
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h 
> b/OvmfPkg/XenPlatformPei/Platform.h
> index eda765b..2948853 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -101,4 +101,14 @@ extern BOOLEAN mS3Supported;
>  
>  extern UINT8