[PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB

Jonathan Cameron via posted 11 patches 5 months, 1 week ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Shannon Zhao <shannon.zhaosl@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB
Posted by Jonathan Cameron via 5 months, 1 week ago
This allows the ACPI SRAT Generic Port Affinity Structure
creation to be independent of PCI internals. Note that
the UID is currently the PCI bus number.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

---
v3: New patch
---
 hw/pci-bridge/pci_expander_bridge.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
index 0411ad31ea..92d39b917a 100644
--- a/hw/pci-bridge/pci_expander_bridge.c
+++ b/hw/pci-bridge/pci_expander_bridge.c
@@ -93,6 +93,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
     pbc->numa_node = pxb_bus_numa_node;
 }
 
+static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
+                             void *opaque, Error **errp)
+{
+    uint32_t uid = pci_bus_num(PCI_BUS(obj));
+
+    visit_type_uint32(v, name, &uid, errp);
+}
+
+static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
+{
+    pxb_bus_class_init(class, data);
+    object_class_property_add(class, "acpi_uid", "uint32",
+                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
+}
+
 static const TypeInfo pxb_bus_info = {
     .name          = TYPE_PXB_BUS,
     .parent        = TYPE_PCI_BUS,
@@ -111,7 +126,7 @@ static const TypeInfo pxb_cxl_bus_info = {
     .name          = TYPE_PXB_CXL_BUS,
     .parent        = TYPE_CXL_BUS,
     .instance_size = sizeof(PXBBus),
-    .class_init    = pxb_bus_class_init,
+    .class_init    = pxb_cxl_bus_class_init,
 };
 
 static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
-- 
2.43.0
Re: [PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB
Posted by Igor Mammedov 5 months ago
On Thu, 20 Jun 2024 17:03:15 +0100
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:

> This allows the ACPI SRAT Generic Port Affinity Structure
> creation to be independent of PCI internals. Note that
> the UID is currently the PCI bus number.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> ---
> v3: New patch
> ---
>  hw/pci-bridge/pci_expander_bridge.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> index 0411ad31ea..92d39b917a 100644
> --- a/hw/pci-bridge/pci_expander_bridge.c
> +++ b/hw/pci-bridge/pci_expander_bridge.c
> @@ -93,6 +93,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
>      pbc->numa_node = pxb_bus_numa_node;
>  }
>  
> +static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
> +                             void *opaque, Error **errp)
> +{
> +    uint32_t uid = pci_bus_num(PCI_BUS(obj));
> +
> +    visit_type_uint32(v, name, &uid, errp);
> +}
> +
> +static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
> +{
> +    pxb_bus_class_init(class, data);
> +    object_class_property_add(class, "acpi_uid", "uint32",
> +                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
> +}
> +
>  static const TypeInfo pxb_bus_info = {
>      .name          = TYPE_PXB_BUS,
>      .parent        = TYPE_PCI_BUS,
> @@ -111,7 +126,7 @@ static const TypeInfo pxb_cxl_bus_info = {
>      .name          = TYPE_PXB_CXL_BUS,
>      .parent        = TYPE_CXL_BUS,
>      .instance_size = sizeof(PXBBus),
> -    .class_init    = pxb_bus_class_init,
> +    .class_init    = pxb_cxl_bus_class_init,

why it's CXL only, doesn't the same UID rules apply to other PCI buses?
>  };
>  
>  static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,
Re: [PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB
Posted by Jonathan Cameron via 5 months ago
On Thu, 27 Jun 2024 15:27:58 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 20 Jun 2024 17:03:15 +0100
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> 
> > This allows the ACPI SRAT Generic Port Affinity Structure
> > creation to be independent of PCI internals. Note that
> > the UID is currently the PCI bus number.
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > ---
> > v3: New patch
> > ---
> >  hw/pci-bridge/pci_expander_bridge.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> > index 0411ad31ea..92d39b917a 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -93,6 +93,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
> >      pbc->numa_node = pxb_bus_numa_node;
> >  }
> >  
> > +static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
> > +                             void *opaque, Error **errp)
> > +{
> > +    uint32_t uid = pci_bus_num(PCI_BUS(obj));
> > +
> > +    visit_type_uint32(v, name, &uid, errp);
> > +}
> > +
> > +static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
> > +{
> > +    pxb_bus_class_init(class, data);
> > +    object_class_property_add(class, "acpi_uid", "uint32",
> > +                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
> > +}
> > +
> >  static const TypeInfo pxb_bus_info = {
> >      .name          = TYPE_PXB_BUS,
> >      .parent        = TYPE_PCI_BUS,
> > @@ -111,7 +126,7 @@ static const TypeInfo pxb_cxl_bus_info = {
> >      .name          = TYPE_PXB_CXL_BUS,
> >      .parent        = TYPE_CXL_BUS,
> >      .instance_size = sizeof(PXBBus),
> > -    .class_init    = pxb_bus_class_init,
> > +    .class_init    = pxb_cxl_bus_class_init,  
> 
> why it's CXL only, doesn't the same UID rules apply to other PCI buses?

In principle, yes.  My nervousness is that we can only test anything
using this infrastructure today with CXL root bridges.

So I was thinking we should keep it limited and broaden the scope
if anyone ever cares.  I don't mind broadening it from the start though.

Jonathan


> >  };
> >  
> >  static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,  
> 
>
Re: [PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB
Posted by Igor Mammedov 4 months, 4 weeks ago
On Thu, 27 Jun 2024 14:46:14 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 27 Jun 2024 15:27:58 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 20 Jun 2024 17:03:15 +0100
> > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> >   
> > > This allows the ACPI SRAT Generic Port Affinity Structure
> > > creation to be independent of PCI internals. Note that
> > > the UID is currently the PCI bus number.
> > > 
> > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > 
> > > ---
> > > v3: New patch
> > > ---
> > >  hw/pci-bridge/pci_expander_bridge.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> > > index 0411ad31ea..92d39b917a 100644
> > > --- a/hw/pci-bridge/pci_expander_bridge.c
> > > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > > @@ -93,6 +93,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
> > >      pbc->numa_node = pxb_bus_numa_node;
> > >  }
> > >  
> > > +static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
> > > +                             void *opaque, Error **errp)
> > > +{
> > > +    uint32_t uid = pci_bus_num(PCI_BUS(obj));
> > > +
> > > +    visit_type_uint32(v, name, &uid, errp);
> > > +}
> > > +
> > > +static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
> > > +{
> > > +    pxb_bus_class_init(class, data);
> > > +    object_class_property_add(class, "acpi_uid", "uint32",
> > > +                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
> > > +}
> > > +
> > >  static const TypeInfo pxb_bus_info = {
> > >      .name          = TYPE_PXB_BUS,
> > >      .parent        = TYPE_PCI_BUS,
> > > @@ -111,7 +126,7 @@ static const TypeInfo pxb_cxl_bus_info = {
> > >      .name          = TYPE_PXB_CXL_BUS,
> > >      .parent        = TYPE_CXL_BUS,
> > >      .instance_size = sizeof(PXBBus),
> > > -    .class_init    = pxb_bus_class_init,
> > > +    .class_init    = pxb_cxl_bus_class_init,    
> > 
> > why it's CXL only, doesn't the same UID rules apply to other PCI buses?  
> 
> In principle, yes.  My nervousness is that we can only test anything
> using this infrastructure today with CXL root bridges.
> 
> So I was thinking we should keep it limited and broaden the scope
> if anyone ever cares.  I don't mind broadening it from the start though.

Then I'd use it everywhere and cleanup ACPI code to use it as well
just to be consistent.
 
> Jonathan
> 
> 
> > >  };
> > >  
> > >  static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,    
> > 
> >   
>
Re: [PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB
Posted by Jonathan Cameron via 4 months, 3 weeks ago
On Fri, 28 Jun 2024 13:55:25 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 27 Jun 2024 14:46:14 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> > On Thu, 27 Jun 2024 15:27:58 +0200
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Thu, 20 Jun 2024 17:03:15 +0100
> > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > >     
> > > > This allows the ACPI SRAT Generic Port Affinity Structure
> > > > creation to be independent of PCI internals. Note that
> > > > the UID is currently the PCI bus number.
> > > > 
> > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > 
> > > > ---
> > > > v3: New patch
> > > > ---
> > > >  hw/pci-bridge/pci_expander_bridge.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> > > > index 0411ad31ea..92d39b917a 100644
> > > > --- a/hw/pci-bridge/pci_expander_bridge.c
> > > > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > > > @@ -93,6 +93,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
> > > >      pbc->numa_node = pxb_bus_numa_node;
> > > >  }
> > > >  
> > > > +static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
> > > > +                             void *opaque, Error **errp)
> > > > +{
> > > > +    uint32_t uid = pci_bus_num(PCI_BUS(obj));
> > > > +
> > > > +    visit_type_uint32(v, name, &uid, errp);
> > > > +}
> > > > +
> > > > +static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
> > > > +{
> > > > +    pxb_bus_class_init(class, data);
> > > > +    object_class_property_add(class, "acpi_uid", "uint32",
> > > > +                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
> > > > +}
> > > > +
> > > >  static const TypeInfo pxb_bus_info = {
> > > >      .name          = TYPE_PXB_BUS,
> > > >      .parent        = TYPE_PCI_BUS,
> > > > @@ -111,7 +126,7 @@ static const TypeInfo pxb_cxl_bus_info = {
> > > >      .name          = TYPE_PXB_CXL_BUS,
> > > >      .parent        = TYPE_CXL_BUS,
> > > >      .instance_size = sizeof(PXBBus),
> > > > -    .class_init    = pxb_bus_class_init,
> > > > +    .class_init    = pxb_cxl_bus_class_init,      
> > > 
> > > why it's CXL only, doesn't the same UID rules apply to other PCI buses?    
> > 
> > In principle, yes.  My nervousness is that we can only test anything
> > using this infrastructure today with CXL root bridges.
> > 
> > So I was thinking we should keep it limited and broaden the scope
> > if anyone ever cares.  I don't mind broadening it from the start though.  
> 
> Then I'd use it everywhere and cleanup ACPI code to use it as well
> just to be consistent.
That is easy to do for all the TYPE_PXB_BUS types and they have separate
handling in the various ACPI table builds from other host bridgesn anyway.

Ultimately it might be nice to do for the host bridges in general but
that needs to be separate I think as there isn't a simple common
ancestor to use.  For at least some cases (gpex-acpi.c) it's hard
coded as 0 directly with no look up at all.

Jonathan

>  
> > Jonathan
> > 
> >   
> > > >  };
> > > >  
> > > >  static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,      
> > > 
> > >     
> >   
> 
>
Re: [PATCH v3 07/11] hw/pci-bridge: Add acpi_uid property to CXL PXB
Posted by Jonathan Cameron via 4 months, 3 weeks ago
On Mon, 1 Jul 2024 18:52:03 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Fri, 28 Jun 2024 13:55:25 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Thu, 27 Jun 2024 14:46:14 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Thu, 27 Jun 2024 15:27:58 +0200
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >     
> > > > On Thu, 20 Jun 2024 17:03:15 +0100
> > > > Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> > > >       
> > > > > This allows the ACPI SRAT Generic Port Affinity Structure
> > > > > creation to be independent of PCI internals. Note that
> > > > > the UID is currently the PCI bus number.
> > > > > 
> > > > > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > 
> > > > > ---
> > > > > v3: New patch
> > > > > ---
> > > > >  hw/pci-bridge/pci_expander_bridge.c | 17 ++++++++++++++++-
> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/pci-bridge/pci_expander_bridge.c b/hw/pci-bridge/pci_expander_bridge.c
> > > > > index 0411ad31ea..92d39b917a 100644
> > > > > --- a/hw/pci-bridge/pci_expander_bridge.c
> > > > > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > > > > @@ -93,6 +93,21 @@ static void pxb_bus_class_init(ObjectClass *class, void *data)
> > > > >      pbc->numa_node = pxb_bus_numa_node;
> > > > >  }
> > > > >  
> > > > > +static void prop_pxb_cxl_uid_get(Object *obj, Visitor *v, const char *name,
> > > > > +                             void *opaque, Error **errp)
> > > > > +{
> > > > > +    uint32_t uid = pci_bus_num(PCI_BUS(obj));
> > > > > +
> > > > > +    visit_type_uint32(v, name, &uid, errp);
> > > > > +}
> > > > > +
> > > > > +static void pxb_cxl_bus_class_init(ObjectClass *class, void *data)
> > > > > +{
> > > > > +    pxb_bus_class_init(class, data);
> > > > > +    object_class_property_add(class, "acpi_uid", "uint32",
> > > > > +                              prop_pxb_cxl_uid_get, NULL, NULL, NULL);
> > > > > +}
> > > > > +
> > > > >  static const TypeInfo pxb_bus_info = {
> > > > >      .name          = TYPE_PXB_BUS,
> > > > >      .parent        = TYPE_PCI_BUS,
> > > > > @@ -111,7 +126,7 @@ static const TypeInfo pxb_cxl_bus_info = {
> > > > >      .name          = TYPE_PXB_CXL_BUS,
> > > > >      .parent        = TYPE_CXL_BUS,
> > > > >      .instance_size = sizeof(PXBBus),
> > > > > -    .class_init    = pxb_bus_class_init,
> > > > > +    .class_init    = pxb_cxl_bus_class_init,        
> > > > 
> > > > why it's CXL only, doesn't the same UID rules apply to other PCI buses?      
> > > 
> > > In principle, yes.  My nervousness is that we can only test anything
> > > using this infrastructure today with CXL root bridges.
> > > 
> > > So I was thinking we should keep it limited and broaden the scope
> > > if anyone ever cares.  I don't mind broadening it from the start though.    
> > 
> > Then I'd use it everywhere and cleanup ACPI code to use it as well
> > just to be consistent.  
> That is easy to do for all the TYPE_PXB_BUS types and they have separate
> handling in the various ACPI table builds from other host bridgesn anyway.
> 
> Ultimately it might be nice to do for the host bridges in general but
> that needs to be separate I think as there isn't a simple common
> ancestor to use.  For at least some cases (gpex-acpi.c) it's hard
> coded as 0 directly with no look up at all.
Also worth noting that we could take the approach of not using pci internals
in ACPI building further and deal with things like numa nodes.
I don't mind doing that in the longer term, but I don't want that to be
a dependency for this series.

Jonathan

> 
> Jonathan
> 
> >    
> > > Jonathan
> > > 
> > >     
> > > > >  };
> > > > >  
> > > > >  static const char *pxb_host_root_bus_path(PCIHostState *host_bridge,        
> > > > 
> > > >       
> > >     
> > 
> >   
> 
> 
>