1. Patchew
  2. QEMU
  3. [PULL v2 00/19] pc,virtio,pci: fixes, features
  4. View patch

[PULL v2 11/19] pci: acpi: ensure that acpi-index is unique

Michael S. Tsirkin posted 19 patches 4 years, 10 months ago
Only 18 patches received!
[PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Michael S. Tsirkin 4 years, 10 months ago 
From: Igor Mammedov <imammedo@redhat.com>

it helps to avoid device naming conflicts when guest OS is
configured to use acpi-index for naming.
Spec ialso says so:

PCI Firmware Specification Revision 3.2
4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
"
Instance number must be unique under \_SB scope. This instance number does not have to
be sequential in a given system configuration.
"

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index ceab287bd3..f4cb3c979d 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
     PCIBus *bus;
 } AcpiPciHpFind;
 
+static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
+{
+    return a - b;
+}
+
+static GSequence *pci_acpi_index_list(void)
+{
+    static GSequence *used_acpi_index_list;
+
+    if (!used_acpi_index_list) {
+        used_acpi_index_list = g_sequence_new(NULL);
+    }
+    return used_acpi_index_list;
+}
+
 static int acpi_pcihp_get_bsel(PCIBus *bus)
 {
     Error *local_err = NULL;
@@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
                    ONBOARD_INDEX_MAX);
         return;
     }
+
+    /*
+     * make sure that acpi-index is unique across all present PCI devices
+     */
+    if (pdev->acpi_index) {
+        GSequence *used_indexes = pci_acpi_index_list();
+
+        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
+                              g_cmp_uint32, NULL)) {
+            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
+                       " already exist", pdev->acpi_index);
+            return;
+        }
+        g_sequence_insert_sorted(used_indexes,
+                                 GINT_TO_POINTER(pdev->acpi_index),
+                                 g_cmp_uint32, NULL);
+    }
 }
 
 void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
@@ -315,8 +347,22 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
 void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
                                  DeviceState *dev, Error **errp)
 {
+    PCIDevice *pdev = PCI_DEVICE(dev);
+
     trace_acpi_pci_unplug(PCI_SLOT(PCI_DEVICE(dev)->devfn),
                           acpi_pcihp_get_bsel(pci_get_bus(PCI_DEVICE(dev))));
+
+    /*
+     * clean up acpi-index so it could reused by another device
+     */
+    if (pdev->acpi_index) {
+        GSequence *used_indexes = pci_acpi_index_list();
+
+        g_sequence_remove(g_sequence_lookup(used_indexes,
+                          GINT_TO_POINTER(pdev->acpi_index),
+                          g_cmp_uint32, NULL));
+    }
+
     qdev_unrealize(dev);
 }
 
-- 
MST
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Daniel P. Berrangé 4 years, 10 months ago 
On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> it helps to avoid device naming conflicts when guest OS is
> configured to use acpi-index for naming.
> Spec ialso says so:
> 
> PCI Firmware Specification Revision 3.2
> 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> "
> Instance number must be unique under \_SB scope. This instance number does not have to
> be sequential in a given system configuration.
> "
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index ceab287bd3..f4cb3c979d 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
>      PCIBus *bus;
>  } AcpiPciHpFind;
>  
> +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> +{
> +    return a - b;
> +}
> +
> +static GSequence *pci_acpi_index_list(void)
> +{
> +    static GSequence *used_acpi_index_list;
> +
> +    if (!used_acpi_index_list) {
> +        used_acpi_index_list = g_sequence_new(NULL);
> +    }
> +    return used_acpi_index_list;
> +}
> +
>  static int acpi_pcihp_get_bsel(PCIBus *bus)
>  {
>      Error *local_err = NULL;
> @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                     ONBOARD_INDEX_MAX);
>          return;
>      }
> +
> +    /*
> +     * make sure that acpi-index is unique across all present PCI devices
> +     */
> +    if (pdev->acpi_index) {
> +        GSequence *used_indexes = pci_acpi_index_list();
> +
> +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> +                              g_cmp_uint32, NULL)) {
> +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> +                       " already exist", pdev->acpi_index);
> +            return;
> +        }
> +        g_sequence_insert_sorted(used_indexes,
> +                                 GINT_TO_POINTER(pdev->acpi_index),
> +                                 g_cmp_uint32, NULL);
> +    }

This doesn't appear to ensure uniqueness when using PCIe topologies:

$ ./build/x86_64-softmmu/qemu-system-x86_64 \
     -device virtio-net,acpi-index=100 \
     -device virtio-net,acpi-index=100
qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist

$ ./build/x86_64-softmmu/qemu-system-x86_64 \
     -M q35 \
     -device virtio-net,acpi-index=100
     -device virtio-net,acpi-index=100
....happily running....


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Daniel P. Berrangé 4 years, 10 months ago 
On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:
> On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:
> > From: Igor Mammedov <imammedo@redhat.com>
> > 
> > it helps to avoid device naming conflicts when guest OS is
> > configured to use acpi-index for naming.
> > Spec ialso says so:
> > 
> > PCI Firmware Specification Revision 3.2
> > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > "
> > Instance number must be unique under \_SB scope. This instance number does not have to
> > be sequential in a given system configuration.
> > "
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index ceab287bd3..f4cb3c979d 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> >      PCIBus *bus;
> >  } AcpiPciHpFind;
> >  
> > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > +{
> > +    return a - b;
> > +}
> > +
> > +static GSequence *pci_acpi_index_list(void)
> > +{
> > +    static GSequence *used_acpi_index_list;
> > +
> > +    if (!used_acpi_index_list) {
> > +        used_acpi_index_list = g_sequence_new(NULL);
> > +    }
> > +    return used_acpi_index_list;
> > +}
> > +
> >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> >  {
> >      Error *local_err = NULL;
> > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> >                     ONBOARD_INDEX_MAX);
> >          return;
> >      }
> > +
> > +    /*
> > +     * make sure that acpi-index is unique across all present PCI devices
> > +     */
> > +    if (pdev->acpi_index) {
> > +        GSequence *used_indexes = pci_acpi_index_list();
> > +
> > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > +                              g_cmp_uint32, NULL)) {
> > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > +                       " already exist", pdev->acpi_index);
> > +            return;
> > +        }
> > +        g_sequence_insert_sorted(used_indexes,
> > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > +                                 g_cmp_uint32, NULL);
> > +    }
> 
> This doesn't appear to ensure uniqueness when using PCIe topologies:
> 
> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
>      -device virtio-net,acpi-index=100 \
>      -device virtio-net,acpi-index=100
> qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> 
> $ ./build/x86_64-softmmu/qemu-system-x86_64 \
>      -M q35 \
>      -device virtio-net,acpi-index=100
>      -device virtio-net,acpi-index=100
> ....happily running....

In fact the entire concept doesn't appear to work with Q35 at all as
implemented.

The 'acpi_index' file in the guest OS never gets created and the NICs
are still called 'eth0', 'eth1'

Only with i440fx can I can the "enoNNN" based naming to work with
acpi-index set from QEMU


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Igor Mammedov 4 years, 10 months ago 
On Tue, 6 Apr 2021 16:07:25 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:  
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > it helps to avoid device naming conflicts when guest OS is
> > > configured to use acpi-index for naming.
> > > Spec ialso says so:
> > > 
> > > PCI Firmware Specification Revision 3.2
> > > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > > "
> > > Instance number must be unique under \_SB scope. This instance number does not have to
> > > be sequential in a given system configuration.
> > > "
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 46 insertions(+)
> > > 
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index ceab287bd3..f4cb3c979d 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> > >      PCIBus *bus;
> > >  } AcpiPciHpFind;
> > >  
> > > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > > +{
> > > +    return a - b;
> > > +}
> > > +
> > > +static GSequence *pci_acpi_index_list(void)
> > > +{
> > > +    static GSequence *used_acpi_index_list;
> > > +
> > > +    if (!used_acpi_index_list) {
> > > +        used_acpi_index_list = g_sequence_new(NULL);
> > > +    }
> > > +    return used_acpi_index_list;
> > > +}
> > > +
> > >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> > >  {
> > >      Error *local_err = NULL;
> > > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > >                     ONBOARD_INDEX_MAX);
> > >          return;
> > >      }
> > > +
> > > +    /*
> > > +     * make sure that acpi-index is unique across all present PCI devices
> > > +     */
> > > +    if (pdev->acpi_index) {
> > > +        GSequence *used_indexes = pci_acpi_index_list();
> > > +
> > > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > > +                              g_cmp_uint32, NULL)) {
> > > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > > +                       " already exist", pdev->acpi_index);
> > > +            return;
> > > +        }
> > > +        g_sequence_insert_sorted(used_indexes,
> > > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > > +                                 g_cmp_uint32, NULL);
> > > +    }  
> > 
> > This doesn't appear to ensure uniqueness when using PCIe topologies:
> > 
> > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> >      -device virtio-net,acpi-index=100 \
> >      -device virtio-net,acpi-index=100
> > qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> > 
> > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> >      -M q35 \
> >      -device virtio-net,acpi-index=100
> >      -device virtio-net,acpi-index=100
> > ....happily running....  
> 
> In fact the entire concept doesn't appear to work with Q35 at all as
> implemented.
> 
> The 'acpi_index' file in the guest OS never gets created and the NICs
> are still called 'eth0', 'eth1'
> 
> Only with i440fx can I can the "enoNNN" based naming to work with
> acpi-index set from QEMU

It is not supported on Q35 yet as it depends on ACPI PCI hotplug infrastructure.
Once Julia is done with porting it to Q35, acpi-index will be pulled along with it.


> Regards,
> Daniel
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Daniel P. Berrangé 4 years, 10 months ago 
On Tue, Apr 06, 2021 at 08:15:46PM +0200, Igor Mammedov wrote:
> On Tue, 6 Apr 2021 16:07:25 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:  
> > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > it helps to avoid device naming conflicts when guest OS is
> > > > configured to use acpi-index for naming.
> > > > Spec ialso says so:
> > > > 
> > > > PCI Firmware Specification Revision 3.2
> > > > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > > > "
> > > > Instance number must be unique under \_SB scope. This instance number does not have to
> > > > be sequential in a given system configuration.
> > > > "
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 46 insertions(+)
> > > > 
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index ceab287bd3..f4cb3c979d 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> > > >      PCIBus *bus;
> > > >  } AcpiPciHpFind;
> > > >  
> > > > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > > > +{
> > > > +    return a - b;
> > > > +}
> > > > +
> > > > +static GSequence *pci_acpi_index_list(void)
> > > > +{
> > > > +    static GSequence *used_acpi_index_list;
> > > > +
> > > > +    if (!used_acpi_index_list) {
> > > > +        used_acpi_index_list = g_sequence_new(NULL);
> > > > +    }
> > > > +    return used_acpi_index_list;
> > > > +}
> > > > +
> > > >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> > > >  {
> > > >      Error *local_err = NULL;
> > > > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > > >                     ONBOARD_INDEX_MAX);
> > > >          return;
> > > >      }
> > > > +
> > > > +    /*
> > > > +     * make sure that acpi-index is unique across all present PCI devices
> > > > +     */
> > > > +    if (pdev->acpi_index) {
> > > > +        GSequence *used_indexes = pci_acpi_index_list();
> > > > +
> > > > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > > > +                              g_cmp_uint32, NULL)) {
> > > > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > > > +                       " already exist", pdev->acpi_index);
> > > > +            return;
> > > > +        }
> > > > +        g_sequence_insert_sorted(used_indexes,
> > > > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > > > +                                 g_cmp_uint32, NULL);
> > > > +    }  
> > > 
> > > This doesn't appear to ensure uniqueness when using PCIe topologies:
> > > 
> > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > >      -device virtio-net,acpi-index=100 \
> > >      -device virtio-net,acpi-index=100
> > > qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> > > 
> > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > >      -M q35 \
> > >      -device virtio-net,acpi-index=100
> > >      -device virtio-net,acpi-index=100
> > > ....happily running....  
> > 
> > In fact the entire concept doesn't appear to work with Q35 at all as
> > implemented.
> > 
> > The 'acpi_index' file in the guest OS never gets created and the NICs
> > are still called 'eth0', 'eth1'
> > 
> > Only with i440fx can I can the "enoNNN" based naming to work with
> > acpi-index set from QEMU
> 
> It is not supported on Q35 yet as it depends on ACPI PCI hotplug infrastructure.
> Once Julia is done with porting it to Q35, acpi-index will be pulled along with it.

Will the PCI hotplug support work in the same way

Looking at this doc I see two options:

  https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/

 1. Names incorporating Firmware/BIOS provided index numbers for on-board devices (example: eno1)
 2. Names incorporating Firmware/BIOS provided PCI Express hotplug slot index numbers (example: ens1) 

Is the stuff Julia is implementing for Q35 going to end up
triggering scenario (1) still, or will it trigger scenario two
which mentions "hotplug slot index" as a distinct concept from
the ACPI index we're setting for i440fx ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Igor Mammedov 4 years, 10 months ago 
On Wed, 7 Apr 2021 09:29:45 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Tue, Apr 06, 2021 at 08:15:46PM +0200, Igor Mammedov wrote:
> > On Tue, 6 Apr 2021 16:07:25 +0100
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:  
> > > > On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:    
> > > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > it helps to avoid device naming conflicts when guest OS is
> > > > > configured to use acpi-index for naming.
> > > > > Spec ialso says so:
> > > > > 
> > > > > PCI Firmware Specification Revision 3.2
> > > > > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > > > > "
> > > > > Instance number must be unique under \_SB scope. This instance number does not have to
> > > > > be sequential in a given system configuration.
> > > > > "
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 46 insertions(+)
> > > > > 
> > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > index ceab287bd3..f4cb3c979d 100644
> > > > > --- a/hw/acpi/pcihp.c
> > > > > +++ b/hw/acpi/pcihp.c
> > > > > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> > > > >      PCIBus *bus;
> > > > >  } AcpiPciHpFind;
> > > > >  
> > > > > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > > > > +{
> > > > > +    return a - b;
> > > > > +}
> > > > > +
> > > > > +static GSequence *pci_acpi_index_list(void)
> > > > > +{
> > > > > +    static GSequence *used_acpi_index_list;
> > > > > +
> > > > > +    if (!used_acpi_index_list) {
> > > > > +        used_acpi_index_list = g_sequence_new(NULL);
> > > > > +    }
> > > > > +    return used_acpi_index_list;
> > > > > +}
> > > > > +
> > > > >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> > > > >  {
> > > > >      Error *local_err = NULL;
> > > > > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > > > >                     ONBOARD_INDEX_MAX);
> > > > >          return;
> > > > >      }
> > > > > +
> > > > > +    /*
> > > > > +     * make sure that acpi-index is unique across all present PCI devices
> > > > > +     */
> > > > > +    if (pdev->acpi_index) {
> > > > > +        GSequence *used_indexes = pci_acpi_index_list();
> > > > > +
> > > > > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > > > > +                              g_cmp_uint32, NULL)) {
> > > > > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > > > > +                       " already exist", pdev->acpi_index);
> > > > > +            return;
> > > > > +        }
> > > > > +        g_sequence_insert_sorted(used_indexes,
> > > > > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > > > > +                                 g_cmp_uint32, NULL);
> > > > > +    }    
> > > > 
> > > > This doesn't appear to ensure uniqueness when using PCIe topologies:
> > > > 
> > > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > > >      -device virtio-net,acpi-index=100 \
> > > >      -device virtio-net,acpi-index=100
> > > > qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> > > > 
> > > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > > >      -M q35 \
> > > >      -device virtio-net,acpi-index=100
> > > >      -device virtio-net,acpi-index=100
> > > > ....happily running....    
> > > 
> > > In fact the entire concept doesn't appear to work with Q35 at all as
> > > implemented.
> > > 
> > > The 'acpi_index' file in the guest OS never gets created and the NICs
> > > are still called 'eth0', 'eth1'
> > > 
> > > Only with i440fx can I can the "enoNNN" based naming to work with
> > > acpi-index set from QEMU  
> > 
> > It is not supported on Q35 yet as it depends on ACPI PCI hotplug infrastructure.
> > Once Julia is done with porting it to Q35, acpi-index will be pulled along with it.  
> 
> Will the PCI hotplug support work in the same way
> 
> Looking at this doc I see two options:
> 
>   https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/
> 
>  1. Names incorporating Firmware/BIOS provided index numbers for on-board devices (example: eno1)
>  2. Names incorporating Firmware/BIOS provided PCI Express hotplug slot index numbers (example: ens1) 
> 
> Is the stuff Julia is implementing for Q35 going to end up
> triggering scenario (1) still, or will it trigger scenario two
> which mentions "hotplug slot index" as a distinct concept from
> the ACPI index we're setting for i440fx ?

it will trigger (1) unless she adds code to disable acpi-index machinery

> 
> Regards,
> Daniel
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Michael S. Tsirkin 4 years, 10 months ago 
On Tue, Apr 06, 2021 at 08:15:46PM +0200, Igor Mammedov wrote:
> On Tue, 6 Apr 2021 16:07:25 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:
> > > On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:  
> > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > it helps to avoid device naming conflicts when guest OS is
> > > > configured to use acpi-index for naming.
> > > > Spec ialso says so:
> > > > 
> > > > PCI Firmware Specification Revision 3.2
> > > > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > > > "
> > > > Instance number must be unique under \_SB scope. This instance number does not have to
> > > > be sequential in a given system configuration.
> > > > "
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 46 insertions(+)
> > > > 
> > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > index ceab287bd3..f4cb3c979d 100644
> > > > --- a/hw/acpi/pcihp.c
> > > > +++ b/hw/acpi/pcihp.c
> > > > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> > > >      PCIBus *bus;
> > > >  } AcpiPciHpFind;
> > > >  
> > > > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > > > +{
> > > > +    return a - b;
> > > > +}
> > > > +
> > > > +static GSequence *pci_acpi_index_list(void)
> > > > +{
> > > > +    static GSequence *used_acpi_index_list;
> > > > +
> > > > +    if (!used_acpi_index_list) {
> > > > +        used_acpi_index_list = g_sequence_new(NULL);
> > > > +    }
> > > > +    return used_acpi_index_list;
> > > > +}
> > > > +
> > > >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> > > >  {
> > > >      Error *local_err = NULL;
> > > > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > > >                     ONBOARD_INDEX_MAX);
> > > >          return;
> > > >      }
> > > > +
> > > > +    /*
> > > > +     * make sure that acpi-index is unique across all present PCI devices
> > > > +     */
> > > > +    if (pdev->acpi_index) {
> > > > +        GSequence *used_indexes = pci_acpi_index_list();
> > > > +
> > > > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > > > +                              g_cmp_uint32, NULL)) {
> > > > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > > > +                       " already exist", pdev->acpi_index);
> > > > +            return;
> > > > +        }
> > > > +        g_sequence_insert_sorted(used_indexes,
> > > > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > > > +                                 g_cmp_uint32, NULL);
> > > > +    }  
> > > 
> > > This doesn't appear to ensure uniqueness when using PCIe topologies:
> > > 
> > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > >      -device virtio-net,acpi-index=100 \
> > >      -device virtio-net,acpi-index=100
> > > qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> > > 
> > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > >      -M q35 \
> > >      -device virtio-net,acpi-index=100
> > >      -device virtio-net,acpi-index=100
> > > ....happily running....  
> > 
> > In fact the entire concept doesn't appear to work with Q35 at all as
> > implemented.
> > 
> > The 'acpi_index' file in the guest OS never gets created and the NICs
> > are still called 'eth0', 'eth1'
> > 
> > Only with i440fx can I can the "enoNNN" based naming to work with
> > acpi-index set from QEMU
> 
> It is not supported on Q35 yet as it depends on ACPI PCI hotplug infrastructure.
> Once Julia is done with porting it to Q35, acpi-index will be pulled along with it.


Right. But for now, should we make it fail instead of being ignored silently?
If we don't how will managament find out it's not really supported?
And if we make it fail how will management then find out when it's finally
supported?

> 
> > Regards,
> > Daniel
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Igor Mammedov 4 years, 10 months ago 
On Wed, 7 Apr 2021 09:29:22 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 06, 2021 at 08:15:46PM +0200, Igor Mammedov wrote:
> > On Tue, 6 Apr 2021 16:07:25 +0100
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:  
> > > > On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:    
> > > > > From: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > it helps to avoid device naming conflicts when guest OS is
> > > > > configured to use acpi-index for naming.
> > > > > Spec ialso says so:
> > > > > 
> > > > > PCI Firmware Specification Revision 3.2
> > > > > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > > > > "
> > > > > Instance number must be unique under \_SB scope. This instance number does not have to
> > > > > be sequential in a given system configuration.
> > > > > "
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > > > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 46 insertions(+)
> > > > > 
> > > > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > > > index ceab287bd3..f4cb3c979d 100644
> > > > > --- a/hw/acpi/pcihp.c
> > > > > +++ b/hw/acpi/pcihp.c
> > > > > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> > > > >      PCIBus *bus;
> > > > >  } AcpiPciHpFind;
> > > > >  
> > > > > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > > > > +{
> > > > > +    return a - b;
> > > > > +}
> > > > > +
> > > > > +static GSequence *pci_acpi_index_list(void)
> > > > > +{
> > > > > +    static GSequence *used_acpi_index_list;
> > > > > +
> > > > > +    if (!used_acpi_index_list) {
> > > > > +        used_acpi_index_list = g_sequence_new(NULL);
> > > > > +    }
> > > > > +    return used_acpi_index_list;
> > > > > +}
> > > > > +
> > > > >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> > > > >  {
> > > > >      Error *local_err = NULL;
> > > > > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > > > >                     ONBOARD_INDEX_MAX);
> > > > >          return;
> > > > >      }
> > > > > +
> > > > > +    /*
> > > > > +     * make sure that acpi-index is unique across all present PCI devices
> > > > > +     */
> > > > > +    if (pdev->acpi_index) {
> > > > > +        GSequence *used_indexes = pci_acpi_index_list();
> > > > > +
> > > > > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > > > > +                              g_cmp_uint32, NULL)) {
> > > > > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > > > > +                       " already exist", pdev->acpi_index);
> > > > > +            return;
> > > > > +        }
> > > > > +        g_sequence_insert_sorted(used_indexes,
> > > > > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > > > > +                                 g_cmp_uint32, NULL);
> > > > > +    }    
> > > > 
> > > > This doesn't appear to ensure uniqueness when using PCIe topologies:
> > > > 
> > > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > > >      -device virtio-net,acpi-index=100 \
> > > >      -device virtio-net,acpi-index=100
> > > > qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> > > > 
> > > > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> > > >      -M q35 \
> > > >      -device virtio-net,acpi-index=100
> > > >      -device virtio-net,acpi-index=100
> > > > ....happily running....    
> > > 
> > > In fact the entire concept doesn't appear to work with Q35 at all as
> > > implemented.
> > > 
> > > The 'acpi_index' file in the guest OS never gets created and the NICs
> > > are still called 'eth0', 'eth1'
> > > 
> > > Only with i440fx can I can the "enoNNN" based naming to work with
> > > acpi-index set from QEMU  
> > 
> > It is not supported on Q35 yet as it depends on ACPI PCI hotplug infrastructure.
> > Once Julia is done with porting it to Q35, acpi-index will be pulled along with it.  
> 
> 
> Right. But for now, should we make it fail instead of being ignored silently?
> If we don't how will managament find out it's not really supported?
> And if we make it fail how will management then find out when it's finally
> supported?

I had an idea to add capability flag to MachineInfo in QMP schema
and then do ugly check from PCIDevice.realize()
1)
     if (acpi_index!=0 && current_machine->has_pci_acpi_index)
          error out

However Daniel said that he didn't think that MachineInfo was
the right place for it.

Problem is that we can't check acpi-index unsupported configuration
at PCIDevice.realize() time since we don't know about availability
of the feature before first reset event that overrides native PCI
hot-plug (SHPC or PCI-E) with ACPI one if it's enabled. Which is
too late, since all devices are already created.

Neither seems right to do check at PCIDevice.reset() time, as
 *) it would depend if device hosting ACPI hotplug were reset first
 *) make every PCI device query for ACPI hotplug controller
    which is the same current_machine->has_pci_acpi_index only uglier

Hence acpi-index is just ignored on machines that do not support it.

I don't see any good option to do this check without refactoring
ACPI hotplug the way where it's enabled at device creation time.
(I think Julia had similar issues with creation/reset ordering
in her last Q35 ACPI PCI hotplug series)

Any suggestions are welcome.

As a quick ugly temporary solution it could be MachineInfo QAPI schema
flag or (PC)Machine property with [1] check.
After all, It's a board feature and should originate from there
(instead of 'random' acpi hw we decided abuse as hotplug controller),
and later we can re-factor it internally to propagate flag along PCI
hierarchy properly (but external probing will stay the same).

PS:
I also didn't consider rising a error in mixed configurations,
where only some of bridges support ACPI hotplug while some use
native one. So that's something to work on.


> > > Regards,
> > > Daniel  
>
Re: [PULL v2 11/19] pci: acpi: ensure that acpi-index is unique
Posted by Michael S. Tsirkin 4 years, 10 months ago 
On Tue, Apr 06, 2021 at 04:07:25PM +0100, Daniel P. Berrangé wrote:
> On Tue, Apr 06, 2021 at 03:54:24PM +0100, Daniel P. Berrangé wrote:
> > On Mon, Mar 22, 2021 at 07:00:18PM -0400, Michael S. Tsirkin wrote:
> > > From: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > it helps to avoid device naming conflicts when guest OS is
> > > configured to use acpi-index for naming.
> > > Spec ialso says so:
> > > 
> > > PCI Firmware Specification Revision 3.2
> > > 4.6.7.  _DSM for Naming a PCI or PCI Express Device Under Operating Systems
> > > "
> > > Instance number must be unique under \_SB scope. This instance number does not have to
> > > be sequential in a given system configuration.
> > > "
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Message-Id: <20210315180102.3008391-4-imammedo@redhat.com>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  hw/acpi/pcihp.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 46 insertions(+)
> > > 
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index ceab287bd3..f4cb3c979d 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -52,6 +52,21 @@ typedef struct AcpiPciHpFind {
> > >      PCIBus *bus;
> > >  } AcpiPciHpFind;
> > >  
> > > +static gint g_cmp_uint32(gconstpointer a, gconstpointer b, gpointer user_data)
> > > +{
> > > +    return a - b;
> > > +}
> > > +
> > > +static GSequence *pci_acpi_index_list(void)
> > > +{
> > > +    static GSequence *used_acpi_index_list;
> > > +
> > > +    if (!used_acpi_index_list) {
> > > +        used_acpi_index_list = g_sequence_new(NULL);
> > > +    }
> > > +    return used_acpi_index_list;
> > > +}
> > > +
> > >  static int acpi_pcihp_get_bsel(PCIBus *bus)
> > >  {
> > >      Error *local_err = NULL;
> > > @@ -277,6 +292,23 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > >                     ONBOARD_INDEX_MAX);
> > >          return;
> > >      }
> > > +
> > > +    /*
> > > +     * make sure that acpi-index is unique across all present PCI devices
> > > +     */
> > > +    if (pdev->acpi_index) {
> > > +        GSequence *used_indexes = pci_acpi_index_list();
> > > +
> > > +        if (g_sequence_lookup(used_indexes, GINT_TO_POINTER(pdev->acpi_index),
> > > +                              g_cmp_uint32, NULL)) {
> > > +            error_setg(errp, "a PCI device with acpi-index = %" PRIu32
> > > +                       " already exist", pdev->acpi_index);
> > > +            return;
> > > +        }
> > > +        g_sequence_insert_sorted(used_indexes,
> > > +                                 GINT_TO_POINTER(pdev->acpi_index),
> > > +                                 g_cmp_uint32, NULL);
> > > +    }
> > 
> > This doesn't appear to ensure uniqueness when using PCIe topologies:
> > 
> > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> >      -device virtio-net,acpi-index=100 \
> >      -device virtio-net,acpi-index=100
> > qemu-system-x86_64: -device virtio-net,acpi-index=100: a PCI device with acpi-index = 100 already exist
> > 
> > $ ./build/x86_64-softmmu/qemu-system-x86_64 \
> >      -M q35 \
> >      -device virtio-net,acpi-index=100
> >      -device virtio-net,acpi-index=100
> > ....happily running....
> 
> In fact the entire concept doesn't appear to work with Q35 at all as
> implemented.
> 
> The 'acpi_index' file in the guest OS never gets created and the NICs
> are still called 'eth0', 'eth1'
> 
> Only with i440fx can I can the "enoNNN" based naming to work with
> acpi-index set from QEMU

Good point, this I think is due to the fact that q35 does not
have ACPI description of PCI devices ATM.
It would be nice if this aborted instead of failing silently.
Similar for hotplug.

Igor?


> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.