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 16:40, Anthony PERARD wrote:
> 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.

Right, if we can detect Xen (both HVM+PVH, or just PVH, whichever is
appropriate) here, that sounds good. For that, I suggested elsewhere to
rebase PlatformBootManagerLib first on top of XenPlatformLib, to
eliminate the duplicate XenDetected() function definition.

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

That should work.

> Then, in PlatformBootManagerLib, simply ignore the error when
> mHostBridgeDevId (or PcdOvmfHostBridgePciDevId) isn't recognise and when
> XenDetected() is true.
> 
> Would that be fine?

Yes, sounds good.

> 
> 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",
> 
> 

Looks OK!

Thanks!
Laszlo