Re: [PATCH 1/3] gpex-acpi: Support PCI link devices outside the host bridge

[Sunil V L]

Hi Michael,

Thank you very much for the review!

On Wed, Jun 05, 2024 at 04:23:36AM -0400, Michael S. Tsirkin wrote:
> On Tue, May 28, 2024 at 01:01:01PM +0530, Sunil V L wrote:
> > Currently, PCI link devices (PNP0C0F) are always created within the
> > scope of the PCI root complex. However, RISC-V needs PCI link devices to
> > be outside the scope of the PCI host bridge to properly enable the probe
> > order. This matches the example given in the ACPI specification section
> > 6.2.13.1 as well.
> 
> Given that, what happens if we do this for all architectures?
> 
In theory and from my observation of linux on arm64, this should not
have any impact. However, I was bit hesitant to change for other
architectures since I was not sure how namespace changes like this will
affect different OS/architecture combination.

It looks like there is no real concern to make this change generic. That
should simplify the patch as well. Let me update in next version.
It would also warrant updating ACPI table blob for bios-table-test.

> > 
> > Enable creating link devices outside the scope of PCI root complex based
> > on the flag which gets set currently only for RISC-V.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/pci-host/gpex-acpi.c    | 29 ++++++++++++++++++++++++-----
> >  hw/riscv/virt-acpi-build.c |  8 +++++---
> >  include/hw/pci-host/gpex.h |  5 ++++-
> >  3 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/pci-host/gpex-acpi.c b/hw/pci-host/gpex-acpi.c
> > index f69413ea2c..cea89a3ed8 100644
> > --- a/hw/pci-host/gpex-acpi.c
> > +++ b/hw/pci-host/gpex-acpi.c
> > @@ -7,7 +7,7 @@
> >  #include "hw/pci/pcie_host.h"
> >  #include "hw/acpi/cxl.h"
> >  
> > -static void acpi_dsdt_add_pci_route_table(Aml *dev, uint32_t irq)
> > +static void acpi_dsdt_add_pci_route_table(Aml *scope, Aml *dev, uint32_t 
> > irq)
> >  {
> >      Aml *method, *crs;
> >      int i, slot_no;
> > @@ -45,7 +45,17 @@ static void acpi_dsdt_add_pci_route_table(Aml *dev, 
> > uint32_t irq)
> >          aml_append(dev_gsi, aml_name_decl("_CRS", crs));
> >          method = aml_method("_SRS", 1, AML_NOTSERIALIZED);
> >          aml_append(dev_gsi, method);
> > -        aml_append(dev, dev_gsi);
> > +
> > +        /*
> > +         * Some architectures like RISC-V
> 
> 
> Just risc-v for now right?
> > need PCI link devices created
> > +         * outside the scope of the PCI host bridge
> 
> .. in order to load the drivers in the correct order.
> Others .... .
> 
Okay.

> > similar to the example
> > +         * given in the section 6.2.13.1 of ACPI spec 6.5.
> 
> 
> This is not how we quote ACPI spec.
> 
> First you find the earliest spec version which has it.
> Then you mention that, section/table # and title.
> For example:
> 
>       ACPI 6.1: 18.3.2.8 Generic Hardware Error Source
> 
>
Thanks!. Let me update as you suggested.

Thanks,
Sunil