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

["Laszlo Ersek" <lersek@redhat.com>]

On 04/15/19 15:33, Laszlo Ersek wrote:
> On 04/09/19 13:08, Anthony PERARD wrote:
>> When running on PVH without PCI bus, the XenPlatformPei will set
>> PcdOvmfHostBridgePciDevId to XEN_PVH_PCI_HOST_BRIDGE_DEVICE_ID.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> 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:
>> +      //
>> +      // There are no PCI bus in this case.
> 
> (1) s/are/is/, please
> 
> with that:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(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.)

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)

Thanks,
Laszlo

>> +      //
>> +      return;
>>      default:
>>        DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n",
>>          __FUNCTION__, mHostBridgeDevId));
>>
>