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

[Michael S. Tsirkin]

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