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

[Daniel P . Berrangé]

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 :|