Re: [PATCH v2 05/25] hw/pci-host/gpex-acpi: Split host bridge OSC and DSM generation

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 30 May 2025 14:05:16 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 30 May 2025 11:02:27 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > On Tue, 27 May 2025 09:40:07 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >   
> > > acpi_dsdt_add_pci_osc() name is confusing as it gives the impression
> > > it appends the _OSC method but in fact it also appends the _DSM method
> > > for the host bridge. Let's split the function into two separate ones
> > > and let them return the method Aml pointer instead. This matches the
> > > way it is done on x86 (build_q35_osc_method). In a subsequent patch
> > > we will replace the gpex method by the q35 implementation that will
> > > become shared between ARM and x86.
> > > 
> > > acpi_dsdt_add_host_bridge_methods is a new top helper that generates
> > > both the _OSC and _DSM methods.
> > > 
> > > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > > Reviewed-by: Gustavo Romero <gustavo.romero@linaro.org>    
> > 
> > Makes complete sense. I've had local equivalent of this on the CXL
> > tree for a while as without it we don't register the _DSM for the
> > CXL path (and we should).  However, can you modify it a little to
> > make that easier for me?  Basically make sure the _DSM is registered
> > for the CXL path as well.
> >   
> [...]
> unless CXL is root host bridge, current _DSM shouldn't be added to it.
> read on comment below.

I'm not clear how this is different from pxb-pcie where we do have
the _DSM. Both are pretending to be real host bridges.

> 
> > > @@ -124,7 +125,17 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, bool enable_native_pcie_hotplug)
> > >      byte_list[0] = 0;
> > >      buf = aml_buffer(1, byte_list);
> > >      aml_append(method, aml_return(buf));
> > > -    aml_append(dev, method);
> > > +    return method;
> > > +}
> > > +
> > > +static void acpi_dsdt_add_host_bridge_methods(Aml *dev,
> > > +                                              bool enable_native_pcie_hotplug)
> > > +{
> > > +    aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
> > > +    aml_append(dev, aml_name_decl("CTRL", aml_int(0)));    
> > 
> > These two declarations seem to be very much part of the _OSC build though not
> > within the the method.  I 'think' you get left with them later with no users.
> > So move them into the osc build here and they will naturally go away when
> > you move to the generic code.
> > 
> > They end up unused in the DSDT at the end of the series.
> > 
> > I ran a quick GPEX + pxb-pcie test and we do get the odd mix that the OSC for
> > the GPEX say no native hotplug but the OSC for the PXB allows it.  
> 
> It's fine for each PXB to have it's own _OSC.
> Also current incarnation of ACPI pcihp doesn't support PXBs at all,
> it would be wrong to enable the on PXBs.
> 
> Thus I'd avoid touching CXL related code paths from this series.
> 
> I'm working on extending ACPI pcihp to PXBs
> (for the same reason as Eric does for arm/virt, i.e. enable acpi-index support there).
> I can add CXL bits then if there is a need/demand for that in CXL land.

Ok.  My original motivation for _DSM on CXL was function 5 to stop Linux messing up
the reenumeration which I know has been rejected upstream for a bunch of
compatibility reasons.  Anyhow, that's a future problem.

Thanks,

Jonathan

> 
> [...]
> 
>