[PATCH 0/2] pc: acpi: revert back to 5.2 PCI slot enumeration

[PATCH 0/2] pc: acpi: revert back to 5.2 PCI slot enumeration

[Igor Mammedov]

Commit b7f23f62e40 (pci: acpi: add _DSM method to PCI devices),
regressed network interface naming for Linux guests in some cases.
This reverts PCI slot enumeration to its state before 6.0.
For details see 2/3 patch.

Thanks Stefan for troubleshooting!

Reported-by: john.sucaet@ekinops.com  
CC: stefanha@redhat.com
CC: qemu-stable@nongnu.org
CC: mst@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: berrange@redhat.com

Igor Mammedov (2):
  tests: acpi: prepare for changing DSDT tables
  acpi: pc: revert back to v5.2 PCI slot enumeration

 tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
 hw/i386/acpi-build.c                        |  9 +++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.27.0

[PATCH 1/2] tests: acpi: prepare for changing DSDT tables

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..6c83a3ef76 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,11 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/DSDT",
+"tests/data/acpi/pc/DSDT.bridge",
+"tests/data/acpi/pc/DSDT.ipmikcs",
+"tests/data/acpi/pc/DSDT.cphp",
+"tests/data/acpi/pc/DSDT.memhp",
+"tests/data/acpi/pc/DSDT.numamem",
+"tests/data/acpi/pc/DSDT.nohpet",
+"tests/data/acpi/pc/DSDT.dimmpxm",
+"tests/data/acpi/pc/DSDT.acpihmat",
+"tests/data/acpi/pc/DSDT.hpbridge",
-- 
2.27.0

[PATCH 2/2] acpi: pc: revert back to v5.2 PCI slot enumeration

[Igor Mammedov]

Commit [1] moved _SUN variable from only hot-pluggable to
all devices. This made linux kernel enumerate extra slots
that weren't present before. If extra slot happens to be
be enumerated first and there is a device in th same slot
but on other bridge, linux kernel will add -N suffix to
slot name of the later, thus changing NIC name compared to
QEMU 5.2. This in some case confuses systemd, if it is
using SLOT NIC naming scheme and interface name becomes
not the same as it was under QEMU-5.2.

Reproducer QEMU CLI:
  -M pc-i440fx-5.2 -nodefaults \
  -device pci-bridge,chassis_nr=1,id=pci.1,bus=pci.0,addr=0x3 \
  -device virtio-net-pci,id=nic1,bus=pci.1,addr=0x1 \
  -device virtio-net-pci,id=nic2,bus=pci.1,addr=0x2 \
  -device virtio-net-pci,id=nic3,bus=pci.1,addr=0x3

with RHEL8 guest produces following results:
  v5.2:
     kernel: virtio_net virtio0 ens1: renamed from eth0
     kernel: virtio_net virtio2 ens3: renamed from eth2
     kernel: virtio_net virtio1 enp1s2: renamed from eth1
      (slot 2 is assigned to empty bus 0 slot and virtio1
       is assigned to 2-2 slot, and renaming falls back,
       for some reason, to path based naming scheme)

  v6.0:
     kernel: virtio_net virtio0 ens1: renamed from eth0
     kernel: virtio_net virtio2 ens3: renamed from eth2
     systemd-udevd[299]: Error changing net interface name 'eth1' to 'ens3': File exists
     systemd-udevd[299]: could not rename interface '3' from 'eth1' to 'ens3': File exists
      (with commit [1] kernel assigns virtio2 to 3-2 slot
       since bridge advertises _SUN=0x3 and kernel assigns
       slot 3 to bridge. Still it manages to rename virtio2
       correctly to ens3, however systemd gets confused with virtio1
       where slot allocation exactly the same (2-2) as in 5.2 case
       and tries to rename it to ens3 which is rightfully taken by
       virtio2)

I'm not sure what breaks in systemd interface renaming (it probably
should be investigated), but on QEMU side we can safely revert
_SUN to 5.2 behavior (i.e. avoid cold-plugged bridges and non
hot-pluggable device classes), without breaking acpi-index, which uses
slot numbers but it doesn't have to use _SUN, it could use an arbitrary
variable name that has the same slot value).
It will help existing VMs to keep networking with non trivial
configs in working order since systemd will do its interface
renaming magic as it used to do.

1)
Fixes: b7f23f62e40 (pci: acpi: add _DSM method to PCI devices)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
shameless plug: one more reason to use new acpi-index for interface
naming, so naming won't depend on enumeration rules kernel or systemd
use (so far it's available only for 'pc' machine, but with Julia's
acpi pci hotplug reviewed, there is a hope for q35 also supporting it
since 6.1).
---
 hw/i386/acpi-build.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 796ffc6f5c..357437ff1d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -435,11 +435,15 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
         aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
 
         if (bsel) {
-            aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
+            /*
+             * Can't declare _SUN here for every device as it changes 'slot'
+             * enumeration order in linux kernel, so use another variable for it
+             */
+            aml_append(dev, aml_name_decl("ASUN", aml_int(slot)));
             method = aml_method("_DSM", 4, AML_SERIALIZED);
             aml_append(method, aml_return(
                 aml_call6("PDSM", aml_arg(0), aml_arg(1), aml_arg(2),
-                          aml_arg(3), aml_name("BSEL"), aml_name("_SUN"))
+                          aml_arg(3), aml_name("BSEL"), aml_name("ASUN"))
             ));
             aml_append(dev, method);
         }
@@ -466,6 +470,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
             aml_append(method, aml_return(aml_int(s3d)));
             aml_append(dev, method);
         } else if (hotplug_enabled_dev) {
+            aml_append(dev, aml_name_decl("_SUN", aml_int(slot)));
             /* add _EJ0 to make slot hotpluggable  */
             method = aml_method("_EJ0", 1, AML_NOTSERIALIZED);
             aml_append(method,
-- 
2.27.0

[PATCH 3/3] tests: acpi: pc: update expected DSDT blobs

[Igor Mammedov]

@@ -930,20 +930,20 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPC    ", 0x00000001)
             Device (S00)
             {
                 Name (_ADR, Zero)  // _ADR: Address
-                Name (_SUN, Zero)  // _SUN: Slot User Number
+                Name (ASUN, Zero)
                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                 {
-                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
+                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                 }
             }

             Device (S10)
             {
                 Name (_ADR, 0x00020000)  // _ADR: Address
-                Name (_SUN, 0x02)  // _SUN: Slot User Number
+                Name (ASUN, 0x02)
                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                 {
-                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
+                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                 }

                 Method (_S1D, 0, NotSerialized)  // _S1D: S1 Device State

with a hank per bridge:

@@ -965,10 +965,10 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS ", "BXPC    ", 0x00000001)
             Device (S18)
             {
                 Name (_ADR, 0x00030000)  // _ADR: Address
-                Name (_SUN, 0x03)  // _SUN: Slot User Number
+                Name (ASUN, 0x03)
                 Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
                 {
-                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
+                    Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
                 }
             }

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |  10 ----------
 tests/data/acpi/pc/DSDT                     | Bin 6002 -> 6002 bytes
 tests/data/acpi/pc/DSDT.acpihmat            | Bin 7327 -> 7327 bytes
 tests/data/acpi/pc/DSDT.bridge              | Bin 8668 -> 8668 bytes
 tests/data/acpi/pc/DSDT.cphp                | Bin 6466 -> 6466 bytes
 tests/data/acpi/pc/DSDT.dimmpxm             | Bin 7656 -> 7656 bytes
 tests/data/acpi/pc/DSDT.hpbridge            | Bin 5969 -> 5969 bytes
 tests/data/acpi/pc/DSDT.ipmikcs             | Bin 6074 -> 6074 bytes
 tests/data/acpi/pc/DSDT.memhp               | Bin 7361 -> 7361 bytes
 tests/data/acpi/pc/DSDT.nohpet              | Bin 5860 -> 5860 bytes
 tests/data/acpi/pc/DSDT.numamem             | Bin 6008 -> 6008 bytes
 11 files changed, 10 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 6c83a3ef76..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,11 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/pc/DSDT",
-"tests/data/acpi/pc/DSDT.bridge",
-"tests/data/acpi/pc/DSDT.ipmikcs",
-"tests/data/acpi/pc/DSDT.cphp",
-"tests/data/acpi/pc/DSDT.memhp",
-"tests/data/acpi/pc/DSDT.numamem",
-"tests/data/acpi/pc/DSDT.nohpet",
-"tests/data/acpi/pc/DSDT.dimmpxm",
-"tests/data/acpi/pc/DSDT.acpihmat",
-"tests/data/acpi/pc/DSDT.hpbridge",
diff --git a/tests/data/acpi/pc/DSDT b/tests/data/acpi/pc/DSDT
index b9dd9b38e4ef720636ba19ccbdf262de8a6439d5..cc1223773e9c459a8d2f20666c051a74338d40b7 100644
GIT binary patch
delta 61
zcmeyQ_eqb-CD<jTNSuLzao$F*B3?$v$+f%^5{|*4e$h={EWw5b@f`7vE<rpD3``6h
Rj+58&N-{cbKF+(E695!d5Lf^J

delta 59
zcmeyQ_eqb-CD<jTNSuLzQEekv5ie_eaHt={WI;YDAupC-LxXsZct@8Y9tH*`29Eg2
PYk4IZ<2N7YUCjvq#N-aK

diff --git a/tests/data/acpi/pc/DSDT.acpihmat b/tests/data/acpi/pc/DSDT.acpihmat
index cba5a1dcb0464e56753bc0b931a4dd2e3b209787..2d0678eb83277088b156d386067429b0f29f9338 100644
GIT binary patch
delta 100
zcmbPlIp31YCD<ioz6=8cWAsL@`9j`~!J&Q(BI5Bb!M;380)S*jW>&USu&WP9BD%?o
fCD_m)o+IATC5VTCfr)_wB+SKxRm0{lLaR9eXLTFp

delta 62
zcmbPlIp31YCD<ioz6=8c<KvB7^MzRBgG2ooCff;1iF>gG8yduO#5=kK@h~tjF>rvS
QxR@rdlak#0LuffC015RFTL1t6

diff --git a/tests/data/acpi/pc/DSDT.bridge b/tests/data/acpi/pc/DSDT.bridge
index a9b4d5659457f6de30b993962bce673c9413d81d..77778c3a69946efd501e7eff0a73af309b553f13 100644
GIT binary patch
delta 73
zcmccPe8-u~CD<k8jv@mCW6wse5?)5f$@RPv5{|*4e$h={EWw5b@f`7vE<rpD3``6h
Yj+58(N&=Of<TV76zd`KHqI}1N0R$Tq1poj5

delta 71
zcmccPe8-u~CD<k8jv@mC<Bg46CA^IBlk0gUCg0~{6Y^pSHZ+Lmh<9`e;$dK5V&I6M
Xyq;GQsNf{8A&~qHVs94ZJ1z_W`ZyF*

diff --git a/tests/data/acpi/pc/DSDT.cphp b/tests/data/acpi/pc/DSDT.cphp
index 8d86155e275aa688f8767dd92c4b9df08b4a18ad..af046b40b0a150b4ab1e113bd9d77912c707074f 100644
GIT binary patch
delta 61
zcmX?PbjXOyCD<jzNs@tqk!d5B6F;NlWM6&>3CG}2zvw0}mS97Jc#e2SmmnSn1||j$
R$H}$)l8la<r}MAo1OUh?4t@Xt

delta 59
zcmX?PbjXOyCD<jzNs@tqv1=oj6F+NwaHt={<cIuHLS8Jvh6eE*@s2J*JPZs>3>@*3
PYxyM^<2O&|U(E>s%3KcC

diff --git a/tests/data/acpi/pc/DSDT.dimmpxm b/tests/data/acpi/pc/DSDT.dimmpxm
index e00a447f92b27f9a91be802eb11fe89dc0457e20..b56b2e089017f933f8a3089c4fd2389fb8ef1e40 100644
GIT binary patch
delta 100
zcmaE1{lc2dCD<k8g)9RD<C~3KFND1vgG2ooM8xA=f_-_G1OUm5%&csuU{@cIM0ArE
fOR%9qJV(5vOArqO0}}%WNSKQWtA@?CBC9z80jwPB

delta 62
zcmaE1{lc2dCD<k8g)9RDW64IY7s9OZ!J&Q(lleuZ#JyO84GrQs;vHRrco-O%7&t&u
OTueYYiOmioD>wlr;t!7i

diff --git a/tests/data/acpi/pc/DSDT.hpbridge b/tests/data/acpi/pc/DSDT.hpbridge
index 5d8ba195055f2eda74223323baeb88390ea36739..bb0593eeb8730d51a6f0fe51a00a00df9c83c419 100644
GIT binary patch
delta 78
zcmcbpcTtbaCD<h-P@I8*aq32{B3?$v$+f%^5{|*4e$h={EWw5b@f`7vE<rpD3``6h
fj+58&N&=M}=QUshDdA$C+{i1!=(t&k&z=(iB>@v8

delta 74
zcmcbpcTtbaCD<h-P@I8*@!>|UB3{<`;7~t?$&P$dLS8Jvh6eE*@s2J*JPZs>3>@*3
b*YZj-#!o)ZYrqCl#>EU&C%0Le&x{iQ&x;Zm

diff --git a/tests/data/acpi/pc/DSDT.ipmikcs b/tests/data/acpi/pc/DSDT.ipmikcs
index 01e53bd436698db6f6adfff584ec56cb99074a5f..2e618e49d357ae1d0ac20d822f71d676ea90f2fc 100644
GIT binary patch
delta 53
zcmdm`ze}IXCD<iompB6hqtQmLUA(N0!J&Q(lOOVlFgi}Y$1BDL;&U-U1UB>Yt>y#(
Dp7{>n

delta 49
zcmdm`ze}IXCD<iompB6h<F1WdyLegSgG2ooCO_npn*5kokPXD+Vghm{HVg5s-~<4L
CZ4W5`

diff --git a/tests/data/acpi/pc/DSDT.memhp b/tests/data/acpi/pc/DSDT.memhp
index b8103799b45224c08344369931b87cf3b7797d7e..c32d28575b967aff40afb9138822ecd3186dd4ce 100644
GIT binary patch
delta 65
zcmX?TdC-!}CD<k8pbP^8<AjY|2ZdN2gG2ooCNm0)NPt+;O<pX)h6eE*@s2J*JPZs>
T3>=P={|HGkI&KydUd;&rQ4bK$

delta 65
zcmX?TdC-!}CD<k8pbP^8qr^t8gF>wF!J&Q(lNp6YCOe3*33;&u8yduO#5=kK@h~tj
UF>u6B{v#v_lw#Q|DZGLc04$Uc4*&oF

diff --git a/tests/data/acpi/pc/DSDT.nohpet b/tests/data/acpi/pc/DSDT.nohpet
index d4f0050533f970128774f825274177096a46c3b8..623f06a900d12500d2197d101f76f6875e92ed64 100644
GIT binary patch
delta 61
zcmaE&`$U(^CD<k8i5LR|W5!0VCp?Ufli%}5NH_+E`b9T+u>>0$#B;<ux&-ksFfcK2
RI8K)5m1K0>Y{|Qt696|m5IX<>

delta 59
zcmaE&`$U(^CD<k8i5LR|<A;r0Pk0#PC%@;Bm@Lo7CgjBuY-kYA5%1^{#KXYA#J~|h
PS)NytF@Ccp?`lo}8wn4|

diff --git a/tests/data/acpi/pc/DSDT.numamem b/tests/data/acpi/pc/DSDT.numamem
index 8632dfe8a8bdd991871a1e633162eb9a2e1497ea..f0a3fa92de94f55d5406f0fafce331776dbb0317 100644
GIT binary patch
delta 100
zcmeyN_d}1%CD<jTLY#qt(PJZ51+TYbaHt=Hh<Ln9urJS&03ex>nU(Dn?CJxOh;H&?
f2{tr{=ZJT73F2X3U}E3^33D-F)v)<2?`lo}e)Jpa

delta 62
zcmeyN_d}1%CD<jTLY#qt@xn%~3SQRu;7~t?$@BT7#JyO84GrQs;vHRrco-O%7&t&u
PTueYYiOm;yS8xIVCQlHc

-- 
2.27.0

Re: [PATCH 0/2] pc: acpi: revert back to 5.2 PCI slot enumeration

[John Sucaet]

Tested-by: John Sucaet <john.sucaet@ekinops.com>

On 6/24/21 10:42 PM, Igor Mammedov wrote:
> Commit b7f23f62e40 (pci: acpi: add _DSM method to PCI devices),
> regressed network interface naming for Linux guests in some cases.
> This reverts PCI slot enumeration to its state before 6.0.
> For details see 2/3 patch.
>
> Thanks Stefan for troubleshooting!
>
> Reported-by: john.sucaet@ekinops.com
> CC: stefanha@redhat.com
> CC: qemu-stable@nongnu.org
> CC: mst@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: berrange@redhat.com
>
> Igor Mammedov (2):
>    tests: acpi: prepare for changing DSDT tables
>    acpi: pc: revert back to v5.2 PCI slot enumeration
>
>   tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
>   hw/i386/acpi-build.c                        |  9 +++++++--
>   2 files changed, 17 insertions(+), 2 deletions(-)
>

Re: [PATCH 0/2] pc: acpi: revert back to 5.2 PCI slot enumeration

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

On Thu, Jun 24, 2021 at 04:42:26PM -0400, Igor Mammedov wrote:
> Commit b7f23f62e40 (pci: acpi: add _DSM method to PCI devices),
> regressed network interface naming for Linux guests in some cases.
> This reverts PCI slot enumeration to its state before 6.0.
> For details see 2/3 patch.
> 
> Thanks Stefan for troubleshooting!
> 
> Reported-by: john.sucaet@ekinops.com  
> CC: stefanha@redhat.com
> CC: qemu-stable@nongnu.org
> CC: mst@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: berrange@redhat.com
> 
> Igor Mammedov (2):
>   tests: acpi: prepare for changing DSDT tables
>   acpi: pc: revert back to v5.2 PCI slot enumeration
> 
>  tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
>  hw/i386/acpi-build.c                        |  9 +++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> -- 
> 2.27.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] pc: acpi: revert back to 5.2 PCI slot enumeration

[Michael S. Tsirkin]

On Mon, Jun 28, 2021 at 02:06:01PM +0100, Stefan Hajnoczi wrote:
> On Thu, Jun 24, 2021 at 04:42:26PM -0400, Igor Mammedov wrote:
> > Commit b7f23f62e40 (pci: acpi: add _DSM method to PCI devices),
> > regressed network interface naming for Linux guests in some cases.
> > This reverts PCI slot enumeration to its state before 6.0.
> > For details see 2/3 patch.
> > 
> > Thanks Stefan for troubleshooting!
> > 
> > Reported-by: john.sucaet@ekinops.com  
> > CC: stefanha@redhat.com
> > CC: qemu-stable@nongnu.org
> > CC: mst@redhat.com
> > CC: marcel.apfelbaum@gmail.com
> > CC: berrange@redhat.com
> > 
> > Igor Mammedov (2):
> >   tests: acpi: prepare for changing DSDT tables
> >   acpi: pc: revert back to v5.2 PCI slot enumeration
> > 
> >  tests/qtest/bios-tables-test-allowed-diff.h | 10 ++++++++++
> >  hw/i386/acpi-build.c                        |  9 +++++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> > 
> > -- 
> > 2.27.0
> > 
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Tagged, thanks!