Re: [edk2-devel] [PATCH v2 25/31] OvmfPkg/PlatformBootManagerLib: Handle the absence of PCI bus on Xen PVH

["Anthony PERARD" <anthony.perard@citrix.com>]

On Mon, Apr 15, 2019 at 03:49:25PM +0200, Laszlo Ersek wrote:
> On 04/15/19 15:33, Laszlo Ersek wrote:
> > On 04/09/19 13:08, Anthony PERARD wrote:
> >> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> >> index 12303fb0f1..1a6d47732e 100644
> >> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> >> @@ -1213,6 +1213,11 @@ PciAcpiInitialization (
> >>        PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6a), 0x0b); // G
> >>        PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6b), 0x0b); // H
> >>        break;
> >> +    case XEN_PVH_PCI_HOST_BRIDGE_DEVICE_ID:
> (Sigh. I have to apologize for my comments that might look "rushed". The
> fact is that, despite it being only Monday, I'm already exhausted. It's
> Monday *afternoon*, after all.
> 
> A "normal" maintainer in my position would probably not look at patches
> from this series for days on end, possibly for multiple weeks even. I on
> the other hand intend to make a bit of progress every day I possibly
> can. The result is that my comments are not always 100% polished when I
> send them, due to fatigue. Sorry about that.)

No worries, take your time. And thanks for you details reviews.

> So, my thought on this is that XEN_PVH_PCI_HOST_BRIDGE_DEVICE_ID is too
> generic to accept here. It will alias at least *some* failures to read
> the underlying PCI config space register.
> 
> In XenPlatformPei (v2 24/31), that's not an issue, for two reasons:
> 
> - there is a specific, additional, XenPvhDetected() check,
> - even if there was an issue with the logic, it'd only affect
> XenPlatformPei; i.e. the OvmfXen platform.
> 
> (2) Can you
> 
> - introduce XEN_PVH_PCI_HOST_BRIDGE_DEVICE_ID with a value different
>   from 0xFFFF perhaps?
> 
> - or keep it as 0xFFFF, but rely on PcdPciDisableBusEnumeration, as a
>   further restriction?
> 
>   (I understand that exposing XenPvhDetected() to PlatformBootManagerLib
>   would require more PVH enlightenment in "common" code, and, indeed, I
>   don't desire that -- that's why I'm suggesting
>   PcdPciDisableBusEnumeration)

So, PlatformBootManagerLib already has a XenDetected() function, so
I could use that function.

I think we could stop using XEN_PVH_PCI_HOST_BRIDGE_DEVICE_ID (because I
don't know what value I could use) and not set
`PcdOvmfHostBridgePciDevId' at all, it would default to 0x0.

Then, in PlatformBootManagerLib, simply ignore the error when
mHostBridgeDevId (or PcdOvmfHostBridgePciDevId) isn't recognise and when
XenDetected() is true.

Would that be fine?

The patch would look something like this:

@@ -1222,6 +1222,12 @@ PciAcpiInitialization (
       PciWrite8 (PCI_LIB_ADDRESS (0, 0x1f, 0, 0x6b), 0x0b); // H
       break;
     default:
+      if (XenDetected ()) {
+        //
+        // There is no PCI bus in this case.
+        //
+        return;
+      }
       DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",


-- 
Anthony PERARD