From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH v5 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory To: Gerd Hoffmann ,devel@edk2.groups.io From: "Albecki, Mateusz" X-Originating-Location: Olsztyn, Warmia-Masuria, PL (134.191.221.84) X-Originating-Platform: Windows Chrome 102 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Wed, 25 May 2022 11:26:46 -0700 References: <20220524062446.x4gpibum74zwi4zu@sirius.home.kraxel.org> In-Reply-To: <20220524062446.x4gpibum74zwi4zu@sirius.home.kraxel.org> Message-ID: <4256.1653503206815963836@groups.io> Content-Type: multipart/alternative; boundary="ilmtUVjFr8pZxPPsF3bO" --ilmtUVjFr8pZxPPsF3bO Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Mon, May 23, 2022 at 11:24 PM, Gerd Hoffmann wrote: >=20 > On Mon, May 23, 2022 at 04:48:05AM -0700, Albecki, Mateusz wrote: >=20 >> @Ni, Ray >>=20 >> I think EDK2 needs to provide a way for root port to operate without IO >> space assigned in a platform-independent way. I can think of the followi= ng >> cases when root port didn't get IO space: >>=20 >> 1. We have run out of IO space but it's fine since the device under the >> root port doesn't use IO or has only non-critical functionalities under = IO >>=20 >> 2. We have run out of IO space and it's really not fine since device nee= ds >> IO >> 3. We are running on a CPU which doesn't support IO >>=20 >> For 1. the question is whether the device driver in EDK2 understands tha= t >> IO bar for that device is optional and will bother to check if it has be= en >> assigned and either fail gracefully or continue operation in limited >> capacity. For 2. the question is whether the driver will fail gracefully= . >> 3 is for completeness at this point I think since the only other >> architecture that uses EDK2 is ARM which has to deal with it in some way >> right now which I think maps IO region into MMIO so in a way it supports >> IO. >=20 > Well, the case I'm trying to handle here is qemu microvm. It's x86, but > io address space support for pcie devices is not wired up. So the pcie > host bridge doesn't support io, which is rather close to case (3). >=20 >=20 >> I've checked the device driver behavior in EDK2 for devices which use IO >> bar here is the rundown: >> 1. IDE - Doesn't check if IO has been assigned, not giving IO results in >> undefined behavior >> 2. SerialIo -> Doesn't check, will assert the system when IO is not >> assigned (although the logic there is really strange as it can use 3 >> different access methods) >> 3. UHCI -> Checks but too late, will most likely result in undefined >> behavior >=20 > Current edk2 behavior is that the initialization of the pcie host bridge > fails in case no io space is present (and all devices connected to it are > not initialized either of course). >=20 > With this patch applied pcie host bridge initialization works. PCIe > devices without io bars are enumerated and initialized sucessfully. > PCIe devices with io bar fail to initialize. That isn't much of a > problem tough as a qemu microvm typically has no pcie devices with io > bars. You mention that devices with IO bar fail to initialize but that is contrar= y to what I would expect from code review. I've run an experiment with your= change in which I am telling EDK2 that no IO space is available on the system by not updating the IO ran= ge in PciHostBridgeLib. Sure enough Devices that need an IO are still enume= rated, device path and PciIo are installed and in general everything works as it used to. If I had an UHCI controller = on that system UHCI driver would be loaded and it could potentially result = in some strange behavior since that driver isn't smart enough to check if IO space has been allocated for the device. To make things worse I see that if we return success there EDK2 will actual= ly go ahead and start assigning trash addresses to the device and enable IO= space decoding in case of the PCI root port which means that device will t= ry to decode invalid IO ranges. Not an issue for a system without an IO but for a system= in which we have run out of the IO and we have entered this code branch th= is new behavior is potentially more dangerous then simply not enumerating t= he device. Logs: PciBus: Resource Map for Root Bridge PciRoot(0x0) Type =3D=C2=A0 =C2=A0Io16; Base =3D 0xFFFFFFFFFFFFFFFF; Length =3D 0x1000; = Alignment =3D 0xFFF Base =3D 0x0; Length =3D 0x4; Alignment =3D 0x3; Owner =3D PPB [00|06|04:14= ] Base =3D 0x0; Length =3D 0x4; Alignment =3D 0x3; Owner =3D PPB [00|06|00:14= ] Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D 0x3; Owner =3D PCI [00|0= 4|00:24] Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D 0x3; Owner =3D PCI [00|0= 4|00:20] Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D 0x3; Owner =3D PCI [00|0= 4|00:1C] Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D 0x3; Owner =3D PCI [00|0= 4|00:18] >=20 >=20 >> Even with those bad device drivers I would agree that taking this >> change presents low risk given that those devices are pretty old and >> should be mostly unused on new systems(SerialIo being an exception but >> that one is usually an RCIEP). >=20 > Also note that for pcie root bridges which do support io address space > this patch changes nothing. It seems to me like it does. Specifically the error scenario where the syst= em has run out of IO space will not be handled properly I think. >=20 >=20 >> That said I think we are missing a larger issue here - why are we >> running out of IO when we have 16 root ports? >=20 > I don't think so. I see the *linux kernel* hand out io address space to > pcie root ports (until it runs out). edk2 doesn't. Ok I misunderstood previous mails >=20 > take care, > Gerd I think to really handle it we would have to have a more involved change. S= pecifically in the PciHostBridge.c:NoitfyPhase function we need to have a w= ay to tell the PciBus driver which resources specifically failed to allocat= e and treat this as a condition we need to handle instead of panicking and = asserting the system or dropping the entire host bridge. When PciBus driver= sees that IO failed to allocate it would skip IO bars and would not allow = to set the IO space enable bit in the root bridge. We also would need to ch= ange the logic in PciResourceSupport.c:ProgramBar because that function is = very optimistic and assumes that if we were able to program one BAR then su= rely all resources for the device are allocated: // // Indicate pci bus driver has allocated // resource for this device // It might be a temporary solution here since // pci device could have multiple bar // Node->PciDev->Allocated =3D TRUE; Which simply isn't the case. Thanks, Mateusz --ilmtUVjFr8pZxPPsF3bO Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Mon, May 23, 2022 at 11:24 PM, Gerd Hoffmann wrote:
On Mon, May 23, 2022 at 04:48:05AM -0700, Albecki, Mateusz wrot= e:
@Ni, Ray

I think EDK2 needs to provide a way for roo= t port to operate without IO space assigned in a platform-independent way. = I can think of the following cases when root port didn't get IO space:

1. We have run out of IO space but it's fine since the device under = the root port doesn't use IO or has only non-critical functionalities under= IO
2. We have run out of IO space and it's really not fine since devi= ce needs IO
3. We are running on a CPU which doesn't support IO
<= br />For 1. the question is whether the device driver in EDK2 understands t= hat IO bar for that device is optional and will bother to check if it has b= een assigned and either fail gracefully or continue operation in limited ca= pacity. For 2. the question is whether the driver will fail gracefully. 3 i= s for completeness at this point I think since the only other architecture = that uses EDK2 is ARM which has to deal with it in some way right now which= I think maps IO region into MMIO so in a way it supports IO.
Well, the case I'm trying to handle here is qemu microvm. It's x86, but
io address space support for pcie devices is not wired up. So the pciehost bridge doesn't support io, which is rather close to case (3).
=
I've checked the device driver behavior in EDK2 for devices whi= ch use IO bar here is the rundown:
1. IDE - Doesn't check if IO has be= en assigned, not giving IO results in undefined behavior
2. SerialIo -= > Doesn't check, will assert the system when IO is not assigned (althoug= h the logic there is really strange as it can use 3 different access method= s)
3. UHCI -> Checks but too late, will most likely result in undef= ined behavior
Current edk2 behavior is that the initialization of the pcie host bridgefails in case no io space is present (and all devices connected to it ar= e
not initialized either of course).

With this patch applie= d pcie host bridge initialization works. PCIe
devices without io bars = are enumerated and initialized sucessfully.
PCIe devices with io bar f= ail to initialize. That isn't much of a
problem tough as a qemu microv= m typically has no pcie devices with io
bars.
You mention that devices with IO bar fail to initialize but that is contrar= y to what I would expect from code review. I've run an experiment with your= change in which I am telling
EDK2 that no IO space is available on th= e system by not updating the IO range in PciHostBridgeLib. Sure enough Devi= ces that need an IO are still enumerated, device path and PciIo are install= ed
and in general everything works as it used to. If I had an UHCI con= troller on that system UHCI driver would be loaded and it could potentially= result in some strange behavior since that driver isn't smart enough to ch= eck
if IO space has been allocated for the device.

To make = things worse I see that if we return success there EDK2 will actually go ah= ead and start assigning trash addresses to the device and enable IO space d= ecoding in case of the PCI root port which means that device will try to de= code
invalid IO ranges. Not an issue for a system without an IO but fo= r a system in which we have run out of the IO and we have entered this code= branch this new behavior is potentially more dangerous then simply not enu= merating the device.

Logs:
PciBus: Resource Map for Root Bridge PciRoot(0x0)
Type =3D   Io16; Base =3D 0xFFFFFFFFFFFFFFFF; Length =3D 0x1000; Alignment =3D 0xFFF
   Base =3D 0x0; Le= ngth =3D 0x4; Alignment =3D 0x3; Owner =3D PPB [00|06|04:14]
   Base =3D 0x0; Le= ngth =3D 0x4; Alignment =3D 0x3; Owner =3D PPB [00|06|00:14]
   Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D= 0x3; Owner =3D PCI [00|04|00:24]<= /div>
   Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D= 0x3; Owner =3D PCI [00|04|00:20]<= /div>
   Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D= 0x3; Owner =3D PCI [00|04|00:1C]<= /div>
   Base =3D 0xFFFFFFFC; Length =3D 0x4; Alignment =3D= 0x3; Owner =3D PCI [00|04|00:18]<= /div>


Even with those bad device drivers I would agree that taking th= is
change presents low risk given that those devices are pretty old an= d
should be mostly unused on new systems(SerialIo being an exception b= ut
that one is usually an RCIEP).
Also note that for pcie root bridges which do support io address space
this patch changes nothing.
It seems to me like it does. Specifically the error scenario where the syst= em has run out of IO space will not be handled properly I think.

That said I think we are missing a larger issue here - why are = we
running out of IO when we have 16 root ports?
I don't think so. I see the *linux kernel* hand out io address space to
pcie root ports (until it runs out). edk2 doesn't.
Ok I misunderstood previous mails

take care,
Gerd

I think to really handle it we would have to have a more involved cha= nge. Specifically in the PciHostBridge.c:NoitfyPhase function we need to ha= ve a way to tell the PciBus driver which resources specifically failed to a= llocate and treat this as a condition we need to handle instead of panickin= g and asserting the system or dropping the entire host bridge. When PciBus = driver sees that IO failed to allocate it would skip IO bars and would not = allow to set the IO space enable bit in the root bridge. We also would need= to change the logic in PciResourceSupport.c:ProgramBar because that functi= on is very optimistic and assumes that if we were able to program one BAR t= hen surely all resources for the device are allocated:

  //
  // Indicate pci bus driver has allocated
  // resource for this device
  // It might be a temporary solution here since
  // pci device could have multiple bar
  //
  Node->PciDev->Allocated =3D TRUE;

Which simply= isn't the case.

Thanks,
Mateusz --ilmtUVjFr8pZxPPsF3bO--