[libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices

Daniel P. Berrangé posted 6 patches 4 years, 10 months ago
There is a newer version of this series
[libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Daniel P. Berrangé 4 years, 10 months ago
PCI devices can be associated with a unique integer index that is
exposed via ACPI. In Linux OS with systemd, this value is used for
provide a NIC device naming scheme that is stable across changes
in PCI slot configuration.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/formatdomain.rst         |  6 +++
 docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
 src/conf/device_conf.h        |  3 ++
 src/conf/domain_conf.c        | 12 ++++++
 4 files changed, 94 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 7ba32ea9c1..5db0aac77a 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -4363,6 +4363,7 @@ Network interfaces
        <mac address='52:54:00:5d:c7:9e'/>
        <boot order='1'/>
        <rom bar='off'/>
+       <acpi index='4'/>
      </interface>
    </devices>
    ...
@@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
 to the ``<mac/>`` element. Note that this attribute is useless if the provided
 MAC address is outside of the reserved VMWare ranges.
 
+:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
+With some operating systems (eg Linux with systemd), the ACPI index is used
+to provide network interface device naming, that is stable across changes
+in PCI addresses assigned to the device.
+
 :anchor:`<a id="elementsNICSVirtual"/>`
 
 Virtual network
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 2ff7862539..30108b6d4c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -1441,6 +1441,9 @@
       <optional>
         <ref name="alias"/>
       </optional>
+      <optional>
+        <ref name="acpi"/>
+      </optional>
       <optional>
         <ref name="address"/>
       </optional>
@@ -2432,6 +2435,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -2860,6 +2866,9 @@
           <optional>
             <ref name="alias"/>
           </optional>
+          <optional>
+            <ref name="acpi"/>
+          </optional>
           <optional>
             <ref name="address"/>
           </optional>
@@ -3517,6 +3526,9 @@
       <optional>
         <ref name="alias"/>
       </optional>
+      <optional>
+        <ref name="acpi"/>
+      </optional>
       <optional>
         <ref name="address"/>
       </optional>
@@ -4143,6 +4155,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -4283,6 +4298,9 @@
       <optional>
         <ref name="alias"/>
       </optional>
+      <optional>
+        <ref name="acpi"/>
+      </optional>
       <optional>
         <ref name="address"/>
       </optional>
@@ -4535,6 +4553,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -4939,6 +4960,9 @@
       <optional>
         <ref name="alias"/>
       </optional>
+      <optional>
+        <ref name="acpi"/>
+      </optional>
       <optional>
         <ref name="address"/>
       </optional>
@@ -5008,6 +5032,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -5040,6 +5067,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -5118,6 +5148,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -5162,6 +5195,9 @@
       <optional>
         <ref name="alias"/>
       </optional>
+      <optional>
+        <ref name="acpi"/>
+      </optional>
       <optional>
         <ref name="address"/>
       </optional>
@@ -5189,6 +5225,9 @@
       <optional>
         <ref name="alias"/>
       </optional>
+      <optional>
+        <ref name="acpi"/>
+      </optional>
       <optional>
         <ref name="address"/>
       </optional>
@@ -5284,6 +5323,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <element name="driver">
             <ref name="virtioOptions"/>
@@ -5386,6 +5428,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -5401,6 +5446,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -5420,6 +5468,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -5451,6 +5502,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="deviceBoot"/>
         </optional>
@@ -6422,6 +6476,16 @@
     </element>
   </define>
 
+  <define name="acpi">
+    <element name="acpi">
+      <optional>
+        <attribute name="index">
+          <ref name="unsignedInt"/>
+        </attribute>
+      </optional>
+    </element>
+  </define>
+
   <define name="memorydev">
     <element name="memory">
       <attribute name="model">
@@ -6460,6 +6524,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
       </interleave>
     </element>
   </define>
@@ -6551,6 +6618,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
@@ -7530,6 +7600,9 @@
         <optional>
           <ref name="alias"/>
         </optional>
+        <optional>
+          <ref name="acpi"/>
+        </optional>
         <optional>
           <ref name="address"/>
         </optional>
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
index a51bdf10ee..af9a43bff2 100644
--- a/src/conf/device_conf.h
+++ b/src/conf/device_conf.h
@@ -159,6 +159,9 @@ struct _virDomainDeviceInfo {
     /* bootIndex is only used for disk, network interface, hostdev
      * and redirdev devices */
     unsigned int bootIndex;
+    /* Valid for any PCI device. Can be used for NIC to get
+     * stable numbering in Linux */
+    unsigned int acpiIndex;
 
     /* pciConnectFlags is only used internally during address
      * assignment, never saved and never reported.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1e72171586..ef921ae41a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6335,6 +6335,9 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
         virBufferAddLit(buf, "/>\n");
     }
 
+    if (info->acpiIndex != 0)
+        virBufferAsprintf(buf, "<acpi index='%u'/>\n", info->acpiIndex);
+
     if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
         info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)
         /* We're done here */
@@ -6661,6 +6664,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt,
     g_autofree char *romenabled = NULL;
     g_autofree char *rombar = NULL;
     g_autofree char *aliasStr = NULL;
+    g_autofree char *acpiIndex = NULL;
     VIR_XPATH_NODE_AUTORESTORE(ctxt)
 
     virDomainDeviceInfoClear(info);
@@ -6709,6 +6713,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt,
         }
     }
 
+    acpiIndex = virXPathString("string(./acpi/@index)", ctxt);
+    if (acpiIndex &&
+        virStrToLong_ui(acpiIndex, NULL, 10, &info->acpiIndex) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Cannot parse ACPI index value '%s'"), acpiIndex);
+        goto cleanup;
+    }
+
     if ((address = virXPathNode("./address", ctxt)) &&
         virDomainDeviceAddressParseXML(address, info) < 0)
         goto cleanup;
-- 
2.30.2

Re: [libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Peter Krempa 4 years, 10 months ago
On Tue, Apr 06, 2021 at 16:31:32 +0100, Daniel Berrange wrote:
> PCI devices can be associated with a unique integer index that is
> exposed via ACPI. In Linux OS with systemd, this value is used for
> provide a NIC device naming scheme that is stable across changes
> in PCI slot configuration.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/formatdomain.rst         |  6 +++
>  docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
>  src/conf/device_conf.h        |  3 ++
>  src/conf/domain_conf.c        | 12 ++++++
>  4 files changed, 94 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 7ba32ea9c1..5db0aac77a 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -4363,6 +4363,7 @@ Network interfaces
>         <mac address='52:54:00:5d:c7:9e'/>
>         <boot order='1'/>
>         <rom bar='off'/>
> +       <acpi index='4'/>
>       </interface>
>     </devices>
>     ...
> @@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
>  to the ``<mac/>`` element. Note that this attribute is useless if the provided
>  MAC address is outside of the reserved VMWare ranges.
>  
> +:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
> +With some operating systems (eg Linux with systemd), the ACPI index is used
> +to provide network interface device naming, that is stable across changes
> +in PCI addresses assigned to the device.

Any range limits or uniqueness requirements worth mentioning?

> +
>  :anchor:`<a id="elementsNICSVirtual"/>`
>  
>  Virtual network


[...]

> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index a51bdf10ee..af9a43bff2 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -159,6 +159,9 @@ struct _virDomainDeviceInfo {
>      /* bootIndex is only used for disk, network interface, hostdev
>       * and redirdev devices */
>      unsigned int bootIndex;
> +    /* Valid for any PCI device. Can be used for NIC to get
> +     * stable numbering in Linux */
> +    unsigned int acpiIndex;
>  
>      /* pciConnectFlags is only used internally during address
>       * assignment, never saved and never reported.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1e72171586..ef921ae41a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6335,6 +6335,9 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>          virBufferAddLit(buf, "/>\n");
>      }
>  
> +    if (info->acpiIndex != 0)
> +        virBufferAsprintf(buf, "<acpi index='%u'/>\n", info->acpiIndex);
> +
>      if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
>          info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390)
>          /* We're done here */
> @@ -6661,6 +6664,7 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt,
>      g_autofree char *romenabled = NULL;
>      g_autofree char *rombar = NULL;
>      g_autofree char *aliasStr = NULL;
> +    g_autofree char *acpiIndex = NULL;
>      VIR_XPATH_NODE_AUTORESTORE(ctxt)
>  
>      virDomainDeviceInfoClear(info);
> @@ -6709,6 +6713,14 @@ virDomainDeviceInfoParseXML(virDomainXMLOptionPtr xmlopt,
>          }
>      }
>  
> +    acpiIndex = virXPathString("string(./acpi/@index)", ctxt);
> +    if (acpiIndex &&
> +        virStrToLong_ui(acpiIndex, NULL, 10, &info->acpiIndex) < 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Cannot parse ACPI index value '%s'"), acpiIndex);
> +        goto cleanup;
> +    }
> +
>      if ((address = virXPathNode("./address", ctxt)) &&
>          virDomainDeviceAddressParseXML(address, info) < 0)
>          goto cleanup;

ABI stability check is missing.

Re: [libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Peter Krempa 4 years, 10 months ago
On Wed, Apr 07, 2021 at 09:17:36 +0200, Peter Krempa wrote:
> On Tue, Apr 06, 2021 at 16:31:32 +0100, Daniel Berrange wrote:
> > PCI devices can be associated with a unique integer index that is
> > exposed via ACPI. In Linux OS with systemd, this value is used for
> > provide a NIC device naming scheme that is stable across changes
> > in PCI slot configuration.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  docs/formatdomain.rst         |  6 +++
> >  docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
> >  src/conf/device_conf.h        |  3 ++
> >  src/conf/domain_conf.c        | 12 ++++++
> >  4 files changed, 94 insertions(+)
> > 
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 7ba32ea9c1..5db0aac77a 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -4363,6 +4363,7 @@ Network interfaces
> >         <mac address='52:54:00:5d:c7:9e'/>
> >         <boot order='1'/>
> >         <rom bar='off'/>
> > +       <acpi index='4'/>
> >       </interface>
> >     </devices>
> >     ...
> > @@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
> >  to the ``<mac/>`` element. Note that this attribute is useless if the provided
> >  MAC address is outside of the reserved VMWare ranges.
> >  
> > +:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
> > +With some operating systems (eg Linux with systemd), the ACPI index is used
> > +to provide network interface device naming, that is stable across changes
> > +in PCI addresses assigned to the device.
> 
> Any range limits or uniqueness requirements worth mentioning?

QEMU / ACPI spec seems to be enforcing unique indexes:

commit 4fd7da4c0336c8fd822cd808d62f7ff8c9936aef
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Mon Mar 15 14:00:59 2021 -0400

    pci: acpi: ensure that acpi-index is unique

    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.
    "


The code isn't checking whether they are declared as unique.

Re: [libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Daniel P. Berrangé 4 years, 10 months ago
On Wed, Apr 07, 2021 at 09:23:50AM +0200, Peter Krempa wrote:
> On Wed, Apr 07, 2021 at 09:17:36 +0200, Peter Krempa wrote:
> > On Tue, Apr 06, 2021 at 16:31:32 +0100, Daniel Berrange wrote:
> > > PCI devices can be associated with a unique integer index that is
> > > exposed via ACPI. In Linux OS with systemd, this value is used for
> > > provide a NIC device naming scheme that is stable across changes
> > > in PCI slot configuration.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  docs/formatdomain.rst         |  6 +++
> > >  docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
> > >  src/conf/device_conf.h        |  3 ++
> > >  src/conf/domain_conf.c        | 12 ++++++
> > >  4 files changed, 94 insertions(+)
> > > 
> > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > > index 7ba32ea9c1..5db0aac77a 100644
> > > --- a/docs/formatdomain.rst
> > > +++ b/docs/formatdomain.rst
> > > @@ -4363,6 +4363,7 @@ Network interfaces
> > >         <mac address='52:54:00:5d:c7:9e'/>
> > >         <boot order='1'/>
> > >         <rom bar='off'/>
> > > +       <acpi index='4'/>
> > >       </interface>
> > >     </devices>
> > >     ...
> > > @@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
> > >  to the ``<mac/>`` element. Note that this attribute is useless if the provided
> > >  MAC address is outside of the reserved VMWare ranges.
> > >  
> > > +:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
> > > +With some operating systems (eg Linux with systemd), the ACPI index is used
> > > +to provide network interface device naming, that is stable across changes
> > > +in PCI addresses assigned to the device.
> > 
> > Any range limits or uniqueness requirements worth mentioning?
> 
> QEMU / ACPI spec seems to be enforcing unique indexes:
> 
> commit 4fd7da4c0336c8fd822cd808d62f7ff8c9936aef
> Author: Igor Mammedov <imammedo@redhat.com>
> Date:   Mon Mar 15 14:00:59 2021 -0400
> 
>     pci: acpi: ensure that acpi-index is unique
> 
>     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.
>     "
> 
> 
> The code isn't checking whether they are declared as unique.

IMHO libvirt doesn't need to duplicate checking that is already done
by QEMU for this.


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: [libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Laine Stump 4 years, 10 months ago
On 4/7/21 8:40 AM, Daniel P. Berrangé wrote:
> On Wed, Apr 07, 2021 at 09:23:50AM +0200, Peter Krempa wrote:
>> On Wed, Apr 07, 2021 at 09:17:36 +0200, Peter Krempa wrote:
>>> On Tue, Apr 06, 2021 at 16:31:32 +0100, Daniel Berrange wrote:
>>>> PCI devices can be associated with a unique integer index that is
>>>> exposed via ACPI. In Linux OS with systemd, this value is used for
>>>> provide a NIC device naming scheme that is stable across changes
>>>> in PCI slot configuration.
>>>>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> ---
>>>>   docs/formatdomain.rst         |  6 +++
>>>>   docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
>>>>   src/conf/device_conf.h        |  3 ++
>>>>   src/conf/domain_conf.c        | 12 ++++++
>>>>   4 files changed, 94 insertions(+)
>>>>
>>>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>>>> index 7ba32ea9c1..5db0aac77a 100644
>>>> --- a/docs/formatdomain.rst
>>>> +++ b/docs/formatdomain.rst
>>>> @@ -4363,6 +4363,7 @@ Network interfaces
>>>>          <mac address='52:54:00:5d:c7:9e'/>
>>>>          <boot order='1'/>
>>>>          <rom bar='off'/>
>>>> +       <acpi index='4'/>
>>>>        </interface>
>>>>      </devices>
>>>>      ...
>>>> @@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
>>>>   to the ``<mac/>`` element. Note that this attribute is useless if the provided
>>>>   MAC address is outside of the reserved VMWare ranges.
>>>>   
>>>> +:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
>>>> +With some operating systems (eg Linux with systemd), the ACPI index is used
>>>> +to provide network interface device naming, that is stable across changes
>>>> +in PCI addresses assigned to the device.
>>>
>>> Any range limits or uniqueness requirements worth mentioning?
>>
>> QEMU / ACPI spec seems to be enforcing unique indexes:
>>
>> commit 4fd7da4c0336c8fd822cd808d62f7ff8c9936aef
>> Author: Igor Mammedov <imammedo@redhat.com>
>> Date:   Mon Mar 15 14:00:59 2021 -0400
>>
>>      pci: acpi: ensure that acpi-index is unique
>>
>>      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.
>>      "
>>
>>
>> The code isn't checking whether they are declared as unique.
> 
> IMHO libvirt doesn't need to duplicate checking that is already done
> by QEMU for this.

I agree with you (I think a lot of checks like that are just adding bulk 
to libvirt for no real value, since eventually the error would be 
revealed and reported anyway), but do want to point out that libvirt's 
checking would be done at the time that the domain is defined, but QEMU 
wouldn't be checking until runtime. Of course, from the PoV of a 
management application, domain definition time is "too late" anyway - 
they should be doing their own checking for unique indexes in order to 
give feedback to the user at a time when the topic is fresh in their 
mind and they can easily do something about it (i.e. while they're 
sitting in a "create domain definition" dialog box (probably called 
something else, but you get the idea))

So after my internal conversation on the topic (see above) I agree with 
you even more - why add a *third* check for uniqueness in between the 
other two?

Re: [libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Laine Stump 4 years, 10 months ago
On 4/6/21 11:31 AM, Daniel P. Berrangé wrote:
> PCI devices can be associated with a unique integer index that is
> exposed via ACPI. In Linux OS with systemd, this value is used for
> provide a NIC device naming scheme that is stable across changes
> in PCI slot configuration.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   docs/formatdomain.rst         |  6 +++
>   docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
>   src/conf/device_conf.h        |  3 ++
>   src/conf/domain_conf.c        | 12 ++++++
>   4 files changed, 94 insertions(+)
> 
> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> index 7ba32ea9c1..5db0aac77a 100644
> --- a/docs/formatdomain.rst
> +++ b/docs/formatdomain.rst
> @@ -4363,6 +4363,7 @@ Network interfaces
>          <mac address='52:54:00:5d:c7:9e'/>
>          <boot order='1'/>
>          <rom bar='off'/>
> +       <acpi index='4'/>
>        </interface>
>      </devices>
>      ...
> @@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
>   to the ``<mac/>`` element. Note that this attribute is useless if the provided
>   MAC address is outside of the reserved VMWare ranges.
>   
> +:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
> +With some operating systems (eg Linux with systemd), the ACPI index is used
> +to provide network interface device naming, that is stable across changes
> +in PCI addresses assigned to the device.
> +
>   :anchor:`<a id="elementsNICSVirtual"/>`
>   
>   Virtual network
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 2ff7862539..30108b6d4c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1441,6 +1441,9 @@
>         <optional>
>           <ref name="alias"/>
>         </optional>
> +      <optional>
> +        <ref name="acpi"/>
> +      </optional>
>         <optional>
>           <ref name="address"/>
>         </optional>

Looks like it's time to reorganize the schema to eliminate all of this 
repetition. You already put the acpi index into the virDomainDeviceInfo 
struct, and parse/format it with virDomainDeviceInfoParse/Format, so it 
would make sense to define a "deviceInfo" element to replace every 
"address" reference. The deviceInfo element could also include alias, 
since that too is part of the deviceInfo.

If you don't want to do that in this series, I can do it after you've 
pushed it all of this - just remind me.

Re: [libvirt PATCH 1/6] conf: add support for <acpi index='NNN'/> for PCI devices
Posted by Daniel P. Berrangé 4 years, 10 months ago
On Wed, Apr 07, 2021 at 08:53:12AM -0400, Laine Stump wrote:
> On 4/6/21 11:31 AM, Daniel P. Berrangé wrote:
> > PCI devices can be associated with a unique integer index that is
> > exposed via ACPI. In Linux OS with systemd, this value is used for
> > provide a NIC device naming scheme that is stable across changes
> > in PCI slot configuration.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   docs/formatdomain.rst         |  6 +++
> >   docs/schemas/domaincommon.rng | 73 +++++++++++++++++++++++++++++++++++
> >   src/conf/device_conf.h        |  3 ++
> >   src/conf/domain_conf.c        | 12 ++++++
> >   4 files changed, 94 insertions(+)
> > 
> > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> > index 7ba32ea9c1..5db0aac77a 100644
> > --- a/docs/formatdomain.rst
> > +++ b/docs/formatdomain.rst
> > @@ -4363,6 +4363,7 @@ Network interfaces
> >          <mac address='52:54:00:5d:c7:9e'/>
> >          <boot order='1'/>
> >          <rom bar='off'/>
> > +       <acpi index='4'/>
> >        </interface>
> >      </devices>
> >      ...
> > @@ -4389,6 +4390,11 @@ when it's in the reserved VMware range by adding a ``type="static"`` attribute
> >   to the ``<mac/>`` element. Note that this attribute is useless if the provided
> >   MAC address is outside of the reserved VMWare ranges.
> > +:since:`Since 7.3.0`, one can set the ACPI index against network interfaces.
> > +With some operating systems (eg Linux with systemd), the ACPI index is used
> > +to provide network interface device naming, that is stable across changes
> > +in PCI addresses assigned to the device.
> > +
> >   :anchor:`<a id="elementsNICSVirtual"/>`
> >   Virtual network
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index 2ff7862539..30108b6d4c 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -1441,6 +1441,9 @@
> >         <optional>
> >           <ref name="alias"/>
> >         </optional>
> > +      <optional>
> > +        <ref name="acpi"/>
> > +      </optional>
> >         <optional>
> >           <ref name="address"/>
> >         </optional>
> 
> Looks like it's time to reorganize the schema to eliminate all of this
> repetition. You already put the acpi index into the virDomainDeviceInfo
> struct, and parse/format it with virDomainDeviceInfoParse/Format, so it
> would make sense to define a "deviceInfo" element to replace every "address"
> reference. The deviceInfo element could also include alias, since that too
> is part of the deviceInfo.
> 
> If you don't want to do that in this series, I can do it after you've pushed
> it all of this - just remind me.

Yeah, I thought about doing this, but I'm not entirely confident about
the impact it will have on the "<interleave>" usage - we need the
interleavin to apply to all child elements and dont want the device
info stuff forced into a group. Maybe it just does the right thing,
but I've not had time to test it, so didn't tackle it here.

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