| Message ID | 20190409110844.14746-26-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Specific platform to run OVMF in Xen PVH and HVM guests | expand |
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> Thanks Laszlo > + // > + return; > default: > DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n", > __FUNCTION__, mHostBridgeDevId)); >
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)); >> >
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",
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
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. + // + return; default: DEBUG ((EFI_D_ERROR, "%a: Unknown Host Bridge Device ID: 0x%04x\n", __FUNCTION__, mHostBridgeDevId));
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(+)