Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> -Original Message-
> From: Gerd Hoffmann
> Sent: Tuesday, April 19, 2022 9:15 PM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
>
> Cc: Leif Lindholm ; Ard Biesheuvel
> ; Jordan Justen ;
> Jian J Wang ; Ray Ni ; Hao A Wu
> ; Jiewen Yao ; Liming Gao
> ; Pawel Polawski ;
> Oliver Steffen ; Ard Biesheuvel
> Subject: Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io
> range is not mandatory
>
> On Wed, Apr 13, 2022 at 01:44:55PM +, Abner Chang wrote:
> >
> >
> > > -Original Message-
> > > From: Gerd Hoffmann
> > > Sent: Wednesday, April 13, 2022 4:12 PM
> > > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> > >
> > > Cc: Leif Lindholm ; Ard Biesheuvel
> > > ; Jordan Justen
> ;
> > > Jian J Wang ; Ray Ni ; Hao A
> Wu
> > > ; Jiewen Yao ; Liming
> Gao
> > > ; Pawel Polawski ;
> > > Oliver Steffen ; Ard Biesheuvel
>
> > > Subject: Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge:
> io
> > > range is not mandatory
> > >
> > > Hi,
> > >
> > > > > diff --git
> a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > > index b20bcd310ad5..51a3b987967f 100644
> > > > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > > @@ -1085,6 +1085,9 @@ NotifyPhase (
> > > > >RootBridge->ResAllocNode[Index].Base = BaseAddress;
> > > > >RootBridge->ResAllocNode[Index].Status = ResAllocated;
> > > > >DEBUG ((DEBUG_INFO, "Success\n"));
> > > > > +} else if (Index == TypeIo) {
> > > > How do we tell the BaseAddress is set to UINT64 because
> "(BaseAddress <
> > > Limit)" or "gDS->AllocateMemorySpace()" returns error in
> AllocateResource()
> > > for TypeIo ?
> > >
> > > Does the reason matter?
> > >
> > > io resources are optional, so if there is no io address space available
> > > it should not be a fatal error, no matter what the root cause is.
> > If the device requires I/O resource however the io address space is not
> available, shouldn't this an error?
>
> How do you figure that?
>
> The PCIe spec requires devices being fully functional without io address
> space resources, so in theory this case should not exist. In practice
> things are not that simple unfortunately. But the pure presence of an
> io bar doesn't imply it is actually required.
>
> I think we can't do much about that at this point. An actual driver for
> the device which has more knowledge about the device would be in a
> better position to figure whenever not having io resources is a fatal
> error or not.
Ok.
>
> > > > Is "else if (Index == TypeIo && RootBridge->Io.Base == MAX_UINT64)"
> > > more reliable? Or I missed the code logic here?
> > >
> > > I think it is not needed, but if you think it is better that way I can
> > > change it.
> > This is more clear to reader. That means that we don't have to treat it as
> > an
> error on the TypeIo if the base address for it is MAX_UINT64.
>
> Ok, I'll change it.
Thanks
Abner
>
> take care,
> Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89135): https://edk2.groups.io/g/devel/message/89135
Mute This Topic: https://groups.io/mt/90309345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
On Wed, Apr 13, 2022 at 01:44:55PM +, Abner Chang wrote:
>
>
> > -Original Message-
> > From: Gerd Hoffmann
> > Sent: Wednesday, April 13, 2022 4:12 PM
> > To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
> >
> > Cc: Leif Lindholm ; Ard Biesheuvel
> > ; Jordan Justen ;
> > Jian J Wang ; Ray Ni ; Hao A Wu
> > ; Jiewen Yao ; Liming Gao
> > ; Pawel Polawski ;
> > Oliver Steffen ; Ard Biesheuvel
> > Subject: Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io
> > range is not mandatory
> >
> > Hi,
> >
> > > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > index b20bcd310ad5..51a3b987967f 100644
> > > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > > @@ -1085,6 +1085,9 @@ NotifyPhase (
> > > >RootBridge->ResAllocNode[Index].Base = BaseAddress;
> > > >RootBridge->ResAllocNode[Index].Status = ResAllocated;
> > > >DEBUG ((DEBUG_INFO, "Success\n"));
> > > > +} else if (Index == TypeIo) {
> > > How do we tell the BaseAddress is set to UINT64 because "(BaseAddress <
> > Limit)" or "gDS->AllocateMemorySpace()" returns error in AllocateResource()
> > for TypeIo ?
> >
> > Does the reason matter?
> >
> > io resources are optional, so if there is no io address space available
> > it should not be a fatal error, no matter what the root cause is.
> If the device requires I/O resource however the io address space is not
> available, shouldn't this an error?
How do you figure that?
The PCIe spec requires devices being fully functional without io address
space resources, so in theory this case should not exist. In practice
things are not that simple unfortunately. But the pure presence of an
io bar doesn't imply it is actually required.
I think we can't do much about that at this point. An actual driver for
the device which has more knowledge about the device would be in a
better position to figure whenever not having io resources is a fatal
error or not.
> > > Is "else if (Index == TypeIo && RootBridge->Io.Base == MAX_UINT64)"
> > more reliable? Or I missed the code logic here?
> >
> > I think it is not needed, but if you think it is better that way I can
> > change it.
> This is more clear to reader. That means that we don't have to treat it as an
> error on the TypeIo if the base address for it is MAX_UINT64.
Ok, I'll change it.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89073): https://edk2.groups.io/g/devel/message/89073
Mute This Topic: https://groups.io/mt/90309345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> -Original Message-
> From: Gerd Hoffmann
> Sent: Wednesday, April 13, 2022 4:12 PM
> To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist)
>
> Cc: Leif Lindholm ; Ard Biesheuvel
> ; Jordan Justen ;
> Jian J Wang ; Ray Ni ; Hao A Wu
> ; Jiewen Yao ; Liming Gao
> ; Pawel Polawski ;
> Oliver Steffen ; Ard Biesheuvel
> Subject: Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io
> range is not mandatory
>
> Hi,
>
> > > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > index b20bcd310ad5..51a3b987967f 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > > @@ -1085,6 +1085,9 @@ NotifyPhase (
> > >RootBridge->ResAllocNode[Index].Base = BaseAddress;
> > >RootBridge->ResAllocNode[Index].Status = ResAllocated;
> > >DEBUG ((DEBUG_INFO, "Success\n"));
> > > +} else if (Index == TypeIo) {
> > How do we tell the BaseAddress is set to UINT64 because "(BaseAddress <
> Limit)" or "gDS->AllocateMemorySpace()" returns error in AllocateResource()
> for TypeIo ?
>
> Does the reason matter?
>
> io resources are optional, so if there is no io address space available
> it should not be a fatal error, no matter what the root cause is.
If the device requires I/O resource however the io address space is not
available, shouldn't this an error?
>
> > Is "else if (Index == TypeIo && RootBridge->Io.Base == MAX_UINT64)"
> more reliable? Or I missed the code logic here?
>
> I think it is not needed, but if you think it is better that way I can
> change it.
This is more clear to reader. That means that we don't have to treat it as an
error on the TypeIo if the base address for it is MAX_UINT64.
Abner
>
> > > + /* optional on PCIe */
> > We should use double back slash for the comment in the function if my
> understanding of the coding standard is correct.
>
> I'll fix in v4.
>
> take care,
> Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88870): https://edk2.groups.io/g/devel/message/88870
Mute This Topic: https://groups.io/mt/90309345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
Hi,
> > diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > index b20bcd310ad5..51a3b987967f 100644
> > --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> > @@ -1085,6 +1085,9 @@ NotifyPhase (
> >RootBridge->ResAllocNode[Index].Base = BaseAddress;
> >RootBridge->ResAllocNode[Index].Status = ResAllocated;
> >DEBUG ((DEBUG_INFO, "Success\n"));
> > +} else if (Index == TypeIo) {
> How do we tell the BaseAddress is set to UINT64 because "(BaseAddress <
> Limit)" or "gDS->AllocateMemorySpace()" returns error in AllocateResource()
> for TypeIo ?
Does the reason matter?
io resources are optional, so if there is no io address space available
it should not be a fatal error, no matter what the root cause is.
> Is "else if (Index == TypeIo && RootBridge->Io.Base == MAX_UINT64)" more
> reliable? Or I missed the code logic here?
I think it is not needed, but if you think it is better that way I can
change it.
> > + /* optional on PCIe */
> We should use double back slash for the comment in the function if my
> understanding of the coding standard is correct.
I'll fix in v4.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88858): https://edk2.groups.io/g/devel/message/88858
Mute This Topic: https://groups.io/mt/90309345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io range is not mandatory
> -Original Message-
> From: Gerd Hoffmann
> Sent: Thursday, April 7, 2022 5:33 PM
> To: devel@edk2.groups.io
> Cc: Leif Lindholm ; Ard Biesheuvel
> ; Jordan Justen ;
> Jian J Wang ; Ray Ni ; Hao A Wu
> ; Gerd Hoffmann ; Jiewen Yao
> ; Liming Gao ; Pawel
> Polawski ; Chang, Abner (HPS SW/FW Technologist)
> ; Oliver Steffen ; Ard
> Biesheuvel
> Subject: [PATCH v3 1/6] MdeModulePkg/PciHostBridge: io range is not
> mandatory
>
> io range is not mandatory according to pcie spec,
> so allow bridge configurations without io address
> space assigned.
>
> Signed-off-by: Gerd Hoffmann
> Reviewed-by: Ard Biesheuvel
> ---
> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> index b20bcd310ad5..51a3b987967f 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> @@ -1085,6 +1085,9 @@ NotifyPhase (
>RootBridge->ResAllocNode[Index].Base = BaseAddress;
>RootBridge->ResAllocNode[Index].Status = ResAllocated;
>DEBUG ((DEBUG_INFO, "Success\n"));
> +} else if (Index == TypeIo) {
How do we tell the BaseAddress is set to UINT64 because "(BaseAddress < Limit)"
or "gDS->AllocateMemorySpace()" returns error in AllocateResource() for TypeIo ?
Is "else if (Index == TypeIo && RootBridge->Io.Base == MAX_UINT64)" more
reliable? Or I missed the code logic here?
> + /* optional on PCIe */
We should use double back slash for the comment in the function if my
understanding of the coding standard is correct.
Abner
> + DEBUG ((DEBUG_INFO, "No IO\n"));
> } else {
>ReturnStatus = EFI_OUT_OF_RESOURCES;
>DEBUG ((DEBUG_ERROR, "Out Of Resource!\n"));
> --
> 2.35.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88838): https://edk2.groups.io/g/devel/message/88838
Mute This Topic: https://groups.io/mt/90309345/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
