[PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
on enumerated buses (like PCI in this case), _ADR is required (and is
already there).  And the _HID value is wrong.  Linux appears to ignore
the _HID entry, but it confuses Windows.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 7b8da62d41..ab73a8f4c8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
     Aml *scope = aml_scope("_SB.PCI0");
     Aml *dev = aml_device("SMB0");
 
-    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
     aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
     build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
     aml_append(scope, dev);
-- 
2.17.1

Re: [PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[Igor Mammedov]

On Mon,  6 Jan 2020 09:27:05 -0600
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> on enumerated buses (like PCI in this case), _ADR is required (and is
> already there).  And the _HID value is wrong.  Linux appears to ignore
> the _HID entry, but it confuses Windows.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/acpi-build.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 7b8da62d41..ab73a8f4c8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
>      Aml *scope = aml_scope("_SB.PCI0");
>      Aml *dev = aml_device("SMB0");
>  
> -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
>      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
>      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
>      aml_append(scope, dev);

Re: [PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[Igor Mammedov]

On Mon,  6 Jan 2020 09:27:05 -0600
minyard@acm.org wrote:

> From: Corey Minyard <cminyard@mvista.com>
> 
> Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> on enumerated buses (like PCI in this case), _ADR is required (and is
> already there).  And the _HID value is wrong.  Linux appears to ignore
> the _HID entry, but it confuses Windows.

Corey,

Could you clarify as what "confuses Windows" means?
s/confuses Windows/description of the observed problem and on what windows version/

> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 7b8da62d41..ab73a8f4c8 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
>      Aml *scope = aml_scope("_SB.PCI0");
>      Aml *dev = aml_device("SMB0");
>  
> -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
>      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
>      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
>      aml_append(scope, dev);

Re: [PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[Corey Minyard]

On Tue, Jan 07, 2020 at 05:58:21PM +0100, Igor Mammedov wrote:
> On Mon,  6 Jan 2020 09:27:05 -0600
> minyard@acm.org wrote:
> 
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> > on enumerated buses (like PCI in this case), _ADR is required (and is
> > already there).  And the _HID value is wrong.  Linux appears to ignore
> > the _HID entry, but it confuses Windows.
> 
> Corey,
> 
> Could you clarify as what "confuses Windows" means?
> s/confuses Windows/description of the observed problem and on what windows version/

Yeah, I should have done that.  The error is not given, but the report
says" "It is detected by Windows 10 as 'Unknown Device' and there is no
driver available."  Link is https://bugs.launchpad.net/qemu/+bug/1856724

I'll add that to the text, along with the link.

-corey

> 
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 7b8da62d41..ab73a8f4c8 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
> >      Aml *scope = aml_scope("_SB.PCI0");
> >      Aml *dev = aml_device("SMB0");
> >  
> > -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
> >      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
> >      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
> >      aml_append(scope, dev);
> 

Re: [PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[Michael S. Tsirkin]

On Tue, Jan 07, 2020 at 02:11:06PM -0600, Corey Minyard wrote:
> On Tue, Jan 07, 2020 at 05:58:21PM +0100, Igor Mammedov wrote:
> > On Mon,  6 Jan 2020 09:27:05 -0600
> > minyard@acm.org wrote:
> > 
> > > From: Corey Minyard <cminyard@mvista.com>
> > > 
> > > Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
> > > on enumerated buses (like PCI in this case), _ADR is required (and is
> > > already there).  And the _HID value is wrong.  Linux appears to ignore
> > > the _HID entry, but it confuses Windows.
> > 
> > Corey,
> > 
> > Could you clarify as what "confuses Windows" means?
> > s/confuses Windows/description of the observed problem and on what windows version/
> 
> Yeah, I should have done that.  The error is not given, but the report
> says" "It is detected by Windows 10 as 'Unknown Device' and there is no
> driver available."  Link is https://bugs.launchpad.net/qemu/+bug/1856724
> 
> I'll add that to the text, along with the link.
> 
> -corey


ok so you will repost with igor's ack and tweaked commit log?

> > 
> > > 
> > > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 7b8da62d41..ab73a8f4c8 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
> > >      Aml *scope = aml_scope("_SB.PCI0");
> > >      Aml *dev = aml_device("SMB0");
> > >  
> > > -    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
> > >      aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
> > >      build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
> > >      aml_append(scope, dev);
> > 

[PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Per the ACPI spec (version 6.1, section 6.1.5 _HID) it is not required
on enumerated buses (like PCI in this case), _ADR is required (and is
already there).  And the _HID value is wrong.  Linux appears to ignore
the _HID entry, but Windows 10 detects it as 'Unknown Device' and there
is no driver available.  See https://bugs.launchpad.net/qemu/+bug/1856724

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c             |   1 -
 tests/data/acpi/q35/DSDT         | Bin 7879 -> 7869 bytes
 tests/data/acpi/q35/DSDT.bridge  | Bin 7896 -> 7886 bytes
 tests/data/acpi/q35/DSDT.cphp    | Bin 8342 -> 8332 bytes
 tests/data/acpi/q35/DSDT.dimmpxm | Bin 9532 -> 9522 bytes
 tests/data/acpi/q35/DSDT.ipmibt  | Bin 7954 -> 7944 bytes
 tests/data/acpi/q35/DSDT.memhp   | Bin 9238 -> 9228 bytes
 tests/data/acpi/q35/DSDT.mmio64  | Bin 9009 -> 8999 bytes
 tests/data/acpi/q35/DSDT.numamem | Bin 7885 -> 7875 bytes
 9 files changed, 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 7b8da62d41..ab73a8f4c8 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1815,7 +1815,6 @@ static void build_smb0(Aml *table, I2CBus *smbus, int devnr, int func)
     Aml *scope = aml_scope("_SB.PCI0");
     Aml *dev = aml_device("SMB0");
 
-    aml_append(dev, aml_name_decl("_HID", aml_eisaid("APP0005")));
     aml_append(dev, aml_name_decl("_ADR", aml_int(devnr << 16 | func)));
     build_acpi_ipmi_devices(dev, BUS(smbus), "\\_SB.PCI0.SMB0");
     aml_append(scope, dev);
diff --git a/tests/data/acpi/q35/DSDT b/tests/data/acpi/q35/DSDT
index 77ea60ffed421c566138fe6341421f579129a582..1f91888d7a485850cf27f152e247a90b208003dc 100644
GIT binary patch
delta 42
xcmX?ZyVsV>CD<iouN(sdBf~~6V@W}2z4&0K_yA{5gXkvyU|%PL%@LCMtN{E}3u*uW

delta 52
zcmdmMd)$`GCD<k8xEuomWBo=hV@YXMz4&0K_yA{5gXkv7U|%N#j(87G7aleN2G-4f
HlKHFvhbs+g

diff --git a/tests/data/acpi/q35/DSDT.bridge b/tests/data/acpi/q35/DSDT.bridge
index fbc2d40000428b402586ea9302b5ccf36ef8de1e..eae3a2a8657e9986d8ac592958503c0b516faaef 100644
GIT binary patch
delta 42
xcmca%d(M{2CD<k8oE!rK<ARM`#*%{4dhx+d@d3`B2GLFY!M;ugn<FF}SOFEN3{C(5

delta 52
zcmX?Sd&8E?CD<k8h8zO}qx?oLV@YXMz4&0K_yA{5gXkv7U|%N#j(87G7aleN2G-4f
Hk`1f?g02lt

diff --git a/tests/data/acpi/q35/DSDT.cphp b/tests/data/acpi/q35/DSDT.cphp
index 6a896cb2142feadbcabc6276b59c138a7e93f540..53d735a4de25c4d8e23eed102fcd01376168c5b3 100644
GIT binary patch
delta 42
xcmbQ{*yG6M66_MvqrkwxSh<nQSW-}0FFx2QKET=2Ai9Y^*w@KmbA+TFI{@^p3o8Hs

delta 52
zcmeBioaV^o66_K(O@V=d@yA9kV@YXMz4&0K_yA{5gXkv7U|%N#j(87G7aleN2G-4f
Hl6LF>e&h`+

diff --git a/tests/data/acpi/q35/DSDT.dimmpxm b/tests/data/acpi/q35/DSDT.dimmpxm
index 23fdf5e60a5069f60d6c680ac9c68c4a8a81318e..02ccdd5f38d5b2356dcca89398c41dcf2595dfff 100644
GIT binary patch
delta 42
xcmdnvwaJUiCD<jzNR@$s(P<->v8151UVN}qe1Nm3L39&;u&<NB<_O6r+yL|Y3#R}8

delta 52
zcmdnwwa1IgCD<jzMwNkq@!&=-V@YXMz4&0K_yA{5gXkv7U|%N#j(87G7aleN2G-4f
Hl25n+d}a-&

diff --git a/tests/data/acpi/q35/DSDT.ipmibt b/tests/data/acpi/q35/DSDT.ipmibt
index c3fca0a71efa7b55c958a49f305389426fbe7922..9e2d4f785c54629d233924a503cfe81199e22aa0 100644
GIT binary patch
delta 42
xcmbPa*I~!y66_MfA<w|TsIrl(PEt@>FFx2QKET=2Ai9Y^*w@Km^J2+-Rsi7S3kU!J

delta 52
zcmeCMn`Fo366_KpB+tOWxOgL1ouss?UVN}qe1Nm3L3ER3u&<K=N4$rp3lEzB1MB9Q
HlKHFvWcdvU

diff --git a/tests/data/acpi/q35/DSDT.memhp b/tests/data/acpi/q35/DSDT.memhp
index 2abd0e36cd1344cbca3fa4ab59c5db2ea326d125..baefa611acadce4c6da5babdaafad533d19358e6 100644
GIT binary patch
delta 42
xcmbQ{(c{7866_Mfqr$+z_;Dkbv8151UVN}qe1Nm3L39&;u&<NB<_O7sTmba|3%CFP

delta 52
zcmeD2nC8Ld66_KprozC$Sg?`HSW;S5FFx2QKET=2Ai7C1*w@K`Bi_T)g@;XmfpxQ=
H<UTF{S(^;F

diff --git a/tests/data/acpi/q35/DSDT.mmio64 b/tests/data/acpi/q35/DSDT.mmio64
index b32034a11c1f8a0a156df3765df44b14a88dbb4d..aae0ea2110a54b02f772d99e66df0730d74b43d9 100644
GIT binary patch
delta 42
xcmdn!w%m=&CD<iIU73M_aqUJfV@W}2z4&0K_yA{5gXkvyU|%PL%@L9}IRW`53)%nx

delta 52
zcmZ4Pw$Y8tCD<jzP?>>&QD-BUv81%BUVN}qe1Nm3L3ER3u&<K=N4$rp3lEzB1M6l#
H$(x)2UJ(r1

diff --git a/tests/data/acpi/q35/DSDT.numamem b/tests/data/acpi/q35/DSDT.numamem
index d8b2b47f8b47067d375021a30086ca97d8aca43f..859a2e08710ba37f56c9c32b0235ff90cedb1905 100644
GIT binary patch
delta 42
xcmX?Wd)SuCCD<k8up9#e<Eo8Z#*%{4dhx+d@d3`B2GLFY!M;ugn<FGkSpgBb3@iWu

delta 52
zcmX?Xd)AiACD<k8tQ-Raqvl2~V@YXMz4&0K_yA{5gXkv7U|%N#j(87G7aleN2G-4f
HlBKKwec25x

-- 
2.17.1

Re: [PATCH] i386:acpi: Remove _HID from the SMBus ACPI entry

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200113144228.16660-1-minyard@acm.org/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Using expected file 'tests/data/acpi/q35/DSDT.acpihmat'
acpi-test: Warning! DSDT binary file mismatch. Actual [aml:/tmp/aml-PJHGE0], Expected [aml:tests/data/acpi/q35/DSDT.acpihmat].
to see ASL diff between mismatched files install IASL, rebuild QEMU from scratch and re-run tests with V=1 environment variable set**
ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:477:test_acpi_asl: assertion failed: (all_tables_match)
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/bios-tables-test.c:477:test_acpi_asl: assertion failed: (all_tables_match)
  TEST    iotest-qcow2: 037
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 038
qemu-system-aarch64: -accel kvm: invalid accelerator kvm
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=3c84ff1231e546e3bf21da6674ef6224', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-11hj6_il/src/docker-src.2020-01-13-10.58.46.23497:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=3c84ff1231e546e3bf21da6674ef6224
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-11hj6_il/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    9m20.765s
user    0m8.987s


The full log is available at
http://patchew.org/logs/20200113144228.16660-1-minyard@acm.org/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com