[PATCH v3 0/2] hw/i386: QOM-attach CPUs/SGX-EPC objects to their parents

[PATCH v3 0/2] hw/i386: QOM-attach CPUs/SGX-EPC objects to their parents

[Philippe Mathieu-Daudé via]

Trying to fix the issue reported here:
https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html

First attach the CPUs and SGX-EPC objects to the machine.

Since v2:
- added missing QOM property auto-enum: [*] (Yang Zhong)

Since v1:
- addressed Paolo & Daniel review feedbacks

Philippe Mathieu-Daudé (2):
  hw/i386: Attach CPUs to machine
  hw/i386/sgx: Attach SGX-EPC objects to machine

 hw/i386/sgx.c | 2 ++
 hw/i386/x86.c | 1 +
 2 files changed, 3 insertions(+)

-- 
2.34.1

[PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Philippe Mathieu-Daudé via]

Previously CPUs were exposed in the QOM tree at a path

  /machine/unattached/device[nn]

where the 'nn' of the first CPU is usually zero, but can
vary depending on what devices were already created.

With this change the CPUs are now at

  /machine/cpu[nn]

where the 'nn' of the first CPU is always zero.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/x86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b84840a1bb9..50bf249c700 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
 {
     Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
 
+    object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu));
     if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
         goto out;
     }
-- 
2.34.1

[PATCH v3 2/2] hw/i386/sgx: Attach SGX-EPC objects to machine

[Philippe Mathieu-Daudé via]

Previously SGX-EPC objects were exposed in the QOM tree at a path

  /machine/unattached/device[nn]

where the 'nn' varies depending on what devices were already created.

With this change the SGX-EPC objects are now at

  /machine/sgx-epc[nn]

where the 'nn' of the first SGX-EPC object is always zero.

Reported-by: Yang Zhong <yang.zhong@intel.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i386/sgx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index a2b318dd938..3ab2217ca43 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -304,6 +304,8 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
     for (list = x86ms->sgx_epc_list; list; list = list->next) {
         obj = object_new("sgx-epc");
 
+        object_property_add_child(OBJECT(pcms), "sgx-epc[*]", OBJECT(obj));
+
         /* set the memdev link with memory backend */
         object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
                               &error_fatal);
-- 
2.34.1

Re: [PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Daniel P. Berrangé]

Cc libvir-list since this will (intentionally) break compatibility
with current libvirt code that looks for "/machine/unattached/device[0]"
in the assumption it is the first CPU.

On Tue, Feb 01, 2022 at 12:35:06AM +0100, Philippe Mathieu-Daudé wrote:
> Previously CPUs were exposed in the QOM tree at a path
> 
>   /machine/unattached/device[nn]
> 
> where the 'nn' of the first CPU is usually zero, but can
> vary depending on what devices were already created.
> 
> With this change the CPUs are now at
> 
>   /machine/cpu[nn]
> 
> where the 'nn' of the first CPU is always zero.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i386/x86.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb9..50bf249c700 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
>  {
>      Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
>  
> +    object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu));
>      if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
>          goto out;
>      }
> -- 
> 2.34.1
> 

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: [PATCH v3 2/2] hw/i386/sgx: Attach SGX-EPC objects to machine

[Daniel P. Berrangé]

On Tue, Feb 01, 2022 at 12:35:07AM +0100, Philippe Mathieu-Daudé wrote:
> Previously SGX-EPC objects were exposed in the QOM tree at a path
> 
>   /machine/unattached/device[nn]
> 
> where the 'nn' varies depending on what devices were already created.
> 
> With this change the SGX-EPC objects are now at
> 
>   /machine/sgx-epc[nn]
> 
> where the 'nn' of the first SGX-EPC object is always zero.
> 
> Reported-by: Yang Zhong <yang.zhong@intel.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i386/sgx.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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: [PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Paolo Bonzini]

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

This is causing breakage in the acpi-tables-test, sorry.

Paolo

Il mar 1 feb 2022, 00:35 Philippe Mathieu-Daudé <f4bug@amsat.org> ha
scritto:

> Previously CPUs were exposed in the QOM tree at a path
>
>   /machine/unattached/device[nn]
>
> where the 'nn' of the first CPU is usually zero, but can
> vary depending on what devices were already created.
>
> With this change the CPUs are now at
>
>   /machine/cpu[nn]
>
> where the 'nn' of the first CPU is always zero.
>
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i386/x86.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b84840a1bb9..50bf249c700 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t
> apic_id, Error **errp)
>  {
>      Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
>
> +    object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu));
>      if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
>          goto out;
>      }
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 1764 bytes --]

Re: [PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Philippe Mathieu-Daudé via]

On 4/2/22 18:59, Paolo Bonzini wrote:
> This is causing breakage in the acpi-tables-test, sorry.

$ make check-qtest-x86_64

Ok:                 49
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            7
Timeout:            0

$ make check-qtest-i386

Ok:                 52
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            4
Timeout:            0

I am confuse, how do you run it?

Re: [PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Paolo Bonzini]

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

It broke check-system-debian in CI.

Paolo

Il ven 4 feb 2022, 20:31 Philippe Mathieu-Daudé <f4bug@amsat.org> ha
scritto:

> On 4/2/22 18:59, Paolo Bonzini wrote:
> > This is causing breakage in the acpi-tables-test, sorry.
>
> $ make check-qtest-x86_64
>
> Ok:                 49
> Expected Fail:      0
> Fail:               0
> Unexpected Pass:    0
> Skipped:            7
> Timeout:            0
>
> $ make check-qtest-i386
>
> Ok:                 52
> Expected Fail:      0
> Fail:               0
> Unexpected Pass:    0
> Skipped:            4
> Timeout:            0
>
> I am confuse, how do you run it?
>
>

[-- Attachment #2: Type: text/html, Size: 1065 bytes --]

Re: [PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Philippe Mathieu-Daudé via]

+Igor

On 5/2/22 10:21, Paolo Bonzini wrote:
> It broke check-system-debian in CI.

OK. This is odd because I am using Ubuntu 20.04 x86_64 host and
can not reproduce. I'll investigate.

> Il ven 4 feb 2022, 20:31 Philippe Mathieu-Daudé <f4bug@amsat.org 
> <mailto:f4bug@amsat.org>> ha scritto:
> 
>     On 4/2/22 18:59, Paolo Bonzini wrote:
>      > This is causing breakage in the acpi-tables-test, sorry.
> 
>     $ make check-qtest-x86_64
> 
>     Ok:                 49
>     Expected Fail:      0
>     Fail:               0
>     Unexpected Pass:    0
>     Skipped:            7
>     Timeout:            0
> 
>     $ make check-qtest-i386
> 
>     Ok:                 52
>     Expected Fail:      0
>     Fail:               0
>     Unexpected Pass:    0
>     Skipped:            4
>     Timeout:            0
> 
>     I am confuse, how do you run it?
> 

Re: [PATCH v3 1/2] hw/i386: Attach CPUs to machine

[Philippe Mathieu-Daudé via]

On 5/2/22 11:32, Philippe Mathieu-Daudé wrote:
> +Igor
> 
> On 5/2/22 10:21, Paolo Bonzini wrote:
>> It broke check-system-debian in CI.
> 
> OK. This is odd because I am using Ubuntu 20.04 x86_64 host and
> can not reproduce. I'll investigate.

OK I could reproduce using the Docker image pulled from
registry.gitlab.com/qemu-project/qemu/qemu/debian-amd64:

acpi-test: Warning! VIOT binary file mismatch. Actual 
[aml:/tmp/aml-INVNG1], Expected [aml:tests/data/acpi/q35/VIOT.viot].
See source file tests/qtest/bios-tables-test.c for instructions on how 
to update expected files.
to see ASL diff between mismatched files install IASL, rebuild QEMU from 
scratch and re-run tests with V=1 environment variable set**
ERROR:../tests/qtest/bios-tables-test.c:531:test_acpi_asl: assertion 
failed: (all_tables_match)
Bail out! ERROR:../tests/qtest/bios-tables-test.c:531:test_acpi_asl: 
assertion failed: (all_tables_match)
Aborted (core dumped)

I'll post a v4 silencing this warning, but I have no much clue about
this.

Phil.