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

[PATCH v4 0/4] 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 v3:
- update VIOT ACPI table (Paolo)

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

Since v1:
- addressed Paolo & Daniel review feedbacks

Philippe Mathieu-Daudé (4):
  tests/qtest/acpi: Temporary allow VIOT table changes
  hw/i386: Attach CPUs to machine
  tests/qtest/acpi: Update VIOT table blob
  hw/i386/sgx: Attach SGX-EPC objects to machine

 hw/i386/sgx.c                 |   2 ++
 hw/i386/x86.c                 |   1 +
 tests/data/acpi/q35/VIOT.viot | Bin 112 -> 112 bytes
 3 files changed, 3 insertions(+)

-- 
2.34.1

[PATCH v4 1/4] tests/qtest/acpi: Temporary allow VIOT table changes

[Philippe Mathieu-Daudé via]

In preparation of changing the VIOT table, add it to the ACPI tests
allowlist.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8bf..27ab8d3ba8d 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/VIOT.viot",
-- 
2.34.1

[PATCH v4 2/4] 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.

Note: This (intentionally) breaks compatibility with current
libvirt code that looks for "/machine/unattached/device[0]"
in the assumption it is the first CPU.

Cc: libvir-list@redhat.com
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-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 v4 3/4] tests/qtest/acpi: Update VIOT table blob

[Philippe Mathieu-Daudé via]

Empty bios-tables-test-allowed-diff.h and run rebuild-expected-aml.sh
to update the VIOT blob. The iasl output diff is:

 /*
  * Intel ACPI Component Architecture
  * AML/ASL+ Disassembler version 20210604 (64-bit version)
  * Copyright (c) 2000 - 2021 Intel Corporation
  *
  * Disassembly of tests/data/acpi/q35/VIOT.viot, Sat Feb  5 11:11:11 2022
  *
  * ACPI Data Table [VIOT]
  *
  * Format: [HexOffset DecimalOffset ByteLength]  FieldName : FieldValue
  */

 [000h 0000   4]                    Signature : "VIOT"    [Virtual I/O Translation Table]
 [004h 0004   4]                 Table Length : 00000070
 [008h 0008   1]                     Revision : 00
 [009h 0009   1]                     Checksum : 3D
 [00Ah 0010   6]                       Oem ID : "BOCHS "
 [010h 0016   8]                 Oem Table ID : "BXPC    "
 [018h 0024   4]                 Oem Revision : 00000001
 [01Ch 0028   4]              Asl Compiler ID : "BXPC"
 [020h 0032   4]        Asl Compiler Revision : 00000001

 [024h 0036   2]                   Node count : 0003
 [026h 0038   2]                  Node offset : 0030
 [028h 0040   8]                     Reserved : 0000000000000000

 [030h 0048   1]                         Type : 03 [VirtIO-PCI IOMMU]
 [031h 0049   1]                     Reserved : 00
 [032h 0050   2]                       Length : 0010

 [034h 0052   2]                  PCI Segment : 0000
 [036h 0054   2]               PCI BDF number : 0010
 [038h 0056   8]                     Reserved : 0000000000000000

 [040h 0064   1]                         Type : 01 [PCI Range]
 [041h 0065   1]                     Reserved : 00
 [042h 0066   2]                       Length : 0018

-[044h 0068   4]               Endpoint start : 00003000
+[044h 0068   4]               Endpoint start : 00001000
 [048h 0072   2]            PCI Segment start : 0000
 [04Ah 0074   2]              PCI Segment end : 0000
-[04Ch 0076   2]                PCI BDF start : 3000
-[04Eh 0078   2]                  PCI BDF end : 30FF
+[04Ch 0076   2]                PCI BDF start : 1000
+[04Eh 0078   2]                  PCI BDF end : 10FF
 [050h 0080   2]                  Output node : 0030
 [052h 0082   6]                     Reserved : 000000000000

 [058h 0088   1]                         Type : 01 [PCI Range]
 [059h 0089   1]                     Reserved : 00
 [05Ah 0090   2]                       Length : 0018

-[05Ch 0092   4]               Endpoint start : 00001000
+[05Ch 0092   4]               Endpoint start : 00003000
 [060h 0096   2]            PCI Segment start : 0000
 [062h 0098   2]              PCI Segment end : 0000
-[064h 0100   2]                PCI BDF start : 1000
-[066h 0102   2]                  PCI BDF end : 10FF
+[064h 0100   2]                PCI BDF start : 3000
+[066h 0102   2]                  PCI BDF end : 30FF
 [068h 0104   2]                  Output node : 0030
 [06Ah 0106   6]                     Reserved : 000000000000

 Raw Table Data: Length 112 (0x70)

     0000: 56 49 4F 54 70 00 00 00 00 3D 42 4F 43 48 53 20  // VIOTp....=BOCHS
     0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43  // BXPC    ....BXPC
     0020: 01 00 00 00 03 00 30 00 00 00 00 00 00 00 00 00  // ......0.........
     0030: 03 00 10 00 00 00 10 00 00 00 00 00 00 00 00 00  // ................
-    0040: 01 00 18 00 00 30 00 00 00 00 00 00 00 30 FF 30  // .....0.......0.0
-    0050: 30 00 00 00 00 00 00 00 01 00 18 00 00 10 00 00  // 0...............
-    0060: 00 00 00 00 00 10 FF 10 30 00 00 00 00 00 00 00  // ........0.......
+    0040: 01 00 18 00 00 10 00 00 00 00 00 00 00 10 FF 10  // ................
+    0050: 30 00 00 00 00 00 00 00 01 00 18 00 00 30 00 00  // 0............0..
+    0060: 00 00 00 00 00 30 FF 30 30 00 00 00 00 00 00 00  // .....0.00.......

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   1 -
 tests/data/acpi/q35/VIOT.viot               | Bin 112 -> 112 bytes
 2 files changed, 1 deletion(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 27ab8d3ba8d..dfb8523c8bf 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,2 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/VIOT.viot",
diff --git a/tests/data/acpi/q35/VIOT.viot b/tests/data/acpi/q35/VIOT.viot
index 9b179266ccbf84f1c250ee646812d17e27987764..275c78fbe8e93190321d957c91c3f17551f865d4 100644
GIT binary patch
delta 10
RcmXRYnBY1wR(PU=1OOI`1E2r^

delta 10
RcmXRYnBY1wR(PU=1OOI`1E2r^

-- 
2.34.1

[PATCH v4 4/4] 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>
Reviewed-by: Daniel P. Berrangé <berrange@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 v4 0/4] hw/i386: QOM-attach CPUs/SGX-EPC objects to their parents

[Philippe Mathieu-Daudé via]

On 5/2/22 13:45, Philippe Mathieu-Daudé wrote:
> 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 v3:
> - update VIOT ACPI table (Paolo)

This now works on Debian, but fails on OpenSUSE:
https://gitlab.com/philmd/qemu/-/jobs/2058210813

Any idea what is going on?

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Igor Mammedov]

On Sat,  5 Feb 2022 13:45:24 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> 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.

Could you add to commit message the reason behind the change?


> Note: This (intentionally) breaks compatibility with current
> libvirt code that looks for "/machine/unattached/device[0]"
> in the assumption it is the first CPU.
Why libvirt does this in the first place?

 
> Cc: libvir-list@redhat.com
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-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));

that will take in account only initial cpus, -device/device_add cpus
will still go to wherever device_add attaches them (see qdev_set_id)

>      if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
>          goto out;
>      }

Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Igor Mammedov]

On Sat,  5 Feb 2022 13:45:26 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> 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.

yet again, why it's necessary?

> 
> Reported-by: Yang Zhong <yang.zhong@intel.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@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);

Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Philippe Mathieu-Daudé via]

On 7/2/22 09:37, Igor Mammedov wrote:
> On Sat,  5 Feb 2022 13:45:26 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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.
> 
> yet again, why it's necessary?

I'll defer that question to Yang & Daniel.

>>
>> Reported-by: Yang Zhong <yang.zhong@intel.com>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berrange@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);
> 

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Igor Mammedov]

On Mon, 7 Feb 2022 09:14:37 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Sat,  5 Feb 2022 13:45:24 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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.  
> 
> Could you add to commit message the reason behind the change?

regardless, it looks like unwarranted movement to me
prompted by livirt accessing/expecting a QOM patch which is
not stable ABI. I'd rather get it fixed on libvirt side.

If libvirt needs for some reason access a CPU instance,
it should use @query-hotpluggable-cpus to get a list of CPUs
(which includes QOM path of already present CPUs) instead of
hard-codding some 'well-known' path as there is no any guarantee 
that it will stay stable whatsoever.
 
> > Note: This (intentionally) breaks compatibility with current
> > libvirt code that looks for "/machine/unattached/device[0]"
> > in the assumption it is the first CPU.  
> Why libvirt does this in the first place?
> 
>  
> > Cc: libvir-list@redhat.com
> > Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> > Reviewed-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));  
> 
> that will take in account only initial cpus, -device/device_add cpus
> will still go to wherever device_add attaches them (see qdev_set_id)
> 
> >      if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) {
> >          goto out;
> >      }  
> 

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Peter Krempa]

On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> On Mon, 7 Feb 2022 09:14:37 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Sat,  5 Feb 2022 13:45:24 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.  
> > 
> > Could you add to commit message the reason behind the change?
> 
> regardless, it looks like unwarranted movement to me
> prompted by livirt accessing/expecting a QOM patch which is
> not stable ABI. I'd rather get it fixed on libvirt side.
> 
> If libvirt needs for some reason access a CPU instance,
> it should use @query-hotpluggable-cpus to get a list of CPUs
> (which includes QOM path of already present CPUs) instead of
> hard-codding some 'well-known' path as there is no any guarantee 
> that it will stay stable whatsoever.

I don't disagree with you about the use of hardcoded path, but the way
of using @query-hotpluggable-cpus is not really aligning well for how
it's being used.

To shed a bit more light, libvirt uses the following hardcoded path

#define QOM_CPU_PATH  "/machine/unattached/device[0]"

in code which is used to query CPU flags. That code doesn't care at all
which cpus are present but wants to get any of them. So yes, calling
query-hotpluggable-cpus is possible but a bit pointless.

In general the code probing cpu flags via qom-get is very cumbersome as
it ends up doing ~400 QMP calls at startup of a VM in cases when we deem
it necessary to probe the cpu fully.

It would be much better (And would sidestep the issue altoghether) if we
had a more sane interface to probe all cpu flags in one go, and ideally
the argument specifying the cpu being optional.

Libvirt can do the adjustment, but for now IMO the path to the first cpu
(/machine/unattached/device[0]) became de-facto ABI by the virtue that
it was used by libvirt and if I remember correctly it was suggested by
the folks dealing with the CPU when the code was added originally.

Even if we change it in libvirt right away, changing qemu will break
forward compatibility. While we don't guarantee it, it still creates
user grief.

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Peter Krempa]

On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote:
> On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > On Mon, 7 Feb 2022 09:14:37 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:

[...]

> Even if we change it in libvirt right away, changing qemu will break
> forward compatibility. While we don't guarantee it, it still creates
> user grief.

I've filed an upstream issue:

https://gitlab.com/libvirt/libvirt/-/issues/272

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Daniel P. Berrangé]

On Mon, Feb 07, 2022 at 10:36:42AM +0100, Peter Krempa wrote:
> On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > On Mon, 7 Feb 2022 09:14:37 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> > > On Sat,  5 Feb 2022 13:45:24 +0100
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.  
> > > 
> > > Could you add to commit message the reason behind the change?
> > 
> > regardless, it looks like unwarranted movement to me
> > prompted by livirt accessing/expecting a QOM patch which is
> > not stable ABI. I'd rather get it fixed on libvirt side.
> > 
> > If libvirt needs for some reason access a CPU instance,
> > it should use @query-hotpluggable-cpus to get a list of CPUs
> > (which includes QOM path of already present CPUs) instead of
> > hard-codding some 'well-known' path as there is no any guarantee 
> > that it will stay stable whatsoever.
> 
> I don't disagree with you about the use of hardcoded path, but the way
> of using @query-hotpluggable-cpus is not really aligning well for how
> it's being used.
> 
> To shed a bit more light, libvirt uses the following hardcoded path
> 
> #define QOM_CPU_PATH  "/machine/unattached/device[0]"
> 
> in code which is used to query CPU flags. That code doesn't care at all
> which cpus are present but wants to get any of them. So yes, calling
> query-hotpluggable-cpus is possible but a bit pointless.
> 
> In general the code probing cpu flags via qom-get is very cumbersome as
> it ends up doing ~400 QMP calls at startup of a VM in cases when we deem
> it necessary to probe the cpu fully.

Yes, that's one QMP call per CPUID feature bit that QEMU knows
about. It is a massive performance bottleneck that we need a
much better solution for.  We really should have raised this
with QEMU right away when we found we had this need for 100's
of QMP commands.

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 v4 2/4] hw/i386: Attach CPUs to machine

[Ani Sinha]

On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa <pkrempa@redhat.com> wrote:
>
> On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote:
> > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > > On Mon, 7 Feb 2022 09:14:37 +0100
> > > Igor Mammedov <imammedo@redhat.com> wrote:
>
> [...]
>
> > Even if we change it in libvirt right away, changing qemu will break
> > forward compatibility. While we don't guarantee it, it still creates
> > user grief.
>
> I've filed an upstream issue:
>
> https://gitlab.com/libvirt/libvirt/-/issues/272

I can look into this bug. Feel free to assign it to me.

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Igor Mammedov]

On Mon, 7 Feb 2022 10:36:42 +0100
Peter Krempa <pkrempa@redhat.com> wrote:

> On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > On Mon, 7 Feb 2022 09:14:37 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >   
> > > On Sat,  5 Feb 2022 13:45:24 +0100
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.    
> > > 
> > > Could you add to commit message the reason behind the change?  
> > 
> > regardless, it looks like unwarranted movement to me
> > prompted by livirt accessing/expecting a QOM patch which is
> > not stable ABI. I'd rather get it fixed on libvirt side.
> > 
> > If libvirt needs for some reason access a CPU instance,
> > it should use @query-hotpluggable-cpus to get a list of CPUs
> > (which includes QOM path of already present CPUs) instead of
> > hard-codding some 'well-known' path as there is no any guarantee 
> > that it will stay stable whatsoever.  
> 
> I don't disagree with you about the use of hardcoded path, but the way
> of using @query-hotpluggable-cpus is not really aligning well for how
> it's being used.
>
> To shed a bit more light, libvirt uses the following hardcoded path
> 
> #define QOM_CPU_PATH  "/machine/unattached/device[0]"
> 
> in code which is used to query CPU flags. That code doesn't care at all
> which cpus are present but wants to get any of them. So yes, calling
> query-hotpluggable-cpus is possible but a bit pointless.

Even though query-hotpluggable-cpus is cumbersome
it still lets you avoid hardcodding QOM path and let you
get away with keeping "_400 QMP calls" probing while
something better comes along.


> In general the code probing cpu flags via qom-get is very cumbersome as
> it ends up doing ~400 QMP calls at startup of a VM in cases when we deem
> it necessary to probe the cpu fully.
> 
> It would be much better (And would sidestep the issue altoghether) if we
> had a more sane interface to probe all cpu flags in one go, and ideally
> the argument specifying the cpu being optional.
> 
> Libvirt can do the adjustment, but for now IMO the path to the first cpu
> (/machine/unattached/device[0]) became de-facto ABI by the virtue that
> it was used by libvirt and if I remember correctly it was suggested by
> the folks dealing with the CPU when the code was added originally.
I would've argued against that back then as well,
there weren't any guarantee and I wouldn't like precedent of
QOM abuse becoming de-facto ABI.
Note: this patch breaks this so called ABI as well and introduces
yet another harcodded path without any stability guarantee whatsoever.

> 
> Even if we change it in libvirt right away, changing qemu will break
> forward compatibility. While we don't guarantee it, it still creates
> user grief.
> 

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Peter Krempa]

On Mon, Feb 07, 2022 at 16:50:28 +0530, Ani Sinha wrote:
> On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa <pkrempa@redhat.com> wrote:
> >
> > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote:
> > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > > > On Mon, 7 Feb 2022 09:14:37 +0100
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > [...]
> >
> > > Even if we change it in libvirt right away, changing qemu will break
> > > forward compatibility. While we don't guarantee it, it still creates
> > > user grief.
> >
> > I've filed an upstream issue:
> >
> > https://gitlab.com/libvirt/libvirt/-/issues/272
> 
> I can look into this bug. Feel free to assign it to me.

No need to. I've noticed that we already call query-hotpluggable-cpus
so with a simple modification the VM startup code path can be easily
fixed so I've done so.

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Ani Sinha]

On Mon, Feb 7, 2022 at 4:58 PM Peter Krempa <pkrempa@redhat.com> wrote:
>
> On Mon, Feb 07, 2022 at 16:50:28 +0530, Ani Sinha wrote:
> > On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa <pkrempa@redhat.com> wrote:
> > >
> > > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote:
> > > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > > > > On Mon, 7 Feb 2022 09:14:37 +0100
> > > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > [...]
> > >
> > > > Even if we change it in libvirt right away, changing qemu will break
> > > > forward compatibility. While we don't guarantee it, it still creates
> > > > user grief.
> > >
> > > I've filed an upstream issue:
> > >
> > > https://gitlab.com/libvirt/libvirt/-/issues/272
> >
> > I can look into this bug. Feel free to assign it to me.
>
> No need to. I've noticed that we already call query-hotpluggable-cpus
> so with a simple modification the VM startup code path can be easily
> fixed so I've done so.

Ok I will look for your patch in the mailing list and review them.

>

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Daniel P. Berrangé]

On Mon, Feb 07, 2022 at 12:22:22PM +0100, Igor Mammedov wrote:
> On Mon, 7 Feb 2022 10:36:42 +0100
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:
> > > On Mon, 7 Feb 2022 09:14:37 +0100
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > >   
> > > > On Sat,  5 Feb 2022 13:45:24 +0100
> > > > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.    
> > > > 
> > > > Could you add to commit message the reason behind the change?  
> > > 
> > > regardless, it looks like unwarranted movement to me
> > > prompted by livirt accessing/expecting a QOM patch which is
> > > not stable ABI. I'd rather get it fixed on libvirt side.
> > > 
> > > If libvirt needs for some reason access a CPU instance,
> > > it should use @query-hotpluggable-cpus to get a list of CPUs
> > > (which includes QOM path of already present CPUs) instead of
> > > hard-codding some 'well-known' path as there is no any guarantee 
> > > that it will stay stable whatsoever.  
> > 
> > I don't disagree with you about the use of hardcoded path, but the way
> > of using @query-hotpluggable-cpus is not really aligning well for how
> > it's being used.
> >
> > To shed a bit more light, libvirt uses the following hardcoded path
> > 
> > #define QOM_CPU_PATH  "/machine/unattached/device[0]"
> > 
> > in code which is used to query CPU flags. That code doesn't care at all
> > which cpus are present but wants to get any of them. So yes, calling
> > query-hotpluggable-cpus is possible but a bit pointless.
> 
> Even though query-hotpluggable-cpus is cumbersome
> it still lets you avoid hardcodding QOM path and let you
> get away with keeping "_400 QMP calls" probing while
> something better comes along.
> 
> 
> > In general the code probing cpu flags via qom-get is very cumbersome as
> > it ends up doing ~400 QMP calls at startup of a VM in cases when we deem
> > it necessary to probe the cpu fully.
> > 
> > It would be much better (And would sidestep the issue altoghether) if we
> > had a more sane interface to probe all cpu flags in one go, and ideally
> > the argument specifying the cpu being optional.
> > 
> > Libvirt can do the adjustment, but for now IMO the path to the first cpu
> > (/machine/unattached/device[0]) became de-facto ABI by the virtue that
> > it was used by libvirt and if I remember correctly it was suggested by
> > the folks dealing with the CPU when the code was added originally.
> I would've argued against that back then as well,
> there weren't any guarantee and I wouldn't like precedent of
> QOM abuse becoming de-facto ABI.
> Note: this patch breaks this so called ABI as well and introduces
> yet another harcodded path without any stability guarantee whatsoever.

AFAIK, we've never defined anything about QOM paths wrt ABI one way
or the other ? In the absence of guidelines then it comes down to
what are reasonable expectations of the mgmt app. These expectations
will be influenced by what it is actually possible to acheive given
our API as exposed.

I think it is unreasonable to expect /machine/unattached to be
stable because by its very nature it is just a dumping ground
for anything where the dev hasn't put in any thought to the path
placement.  IOW, it was/is definitely a bad idea for libvirt to
rely on /machine/unattached in any way. That we're liable to be
broken is not nice, but its really our own fault - we should 
have asked for a better solution from day one here.


I think it is somewhat reasonable to expect other QOM paths to
be stable as there's been some degree of thought put into their
placement. If we don't want apps to be considering other
paths to be stable, then we need to explain exactly what they
can and can't rely on, and most importantly actually provide
a way for them to do what they need


For example, libvirt needs a QOM path to query memory balloon
stats. All libvirt knows is that it set 'id=balloon0' when
creating it on the CLI. To find the balloon device in QOM it
then looks for all paths under '/machine/peripheral', and
tries to find one called 'child<$ID>' where $ID is the id=xxx
value from the CLI.

We do the same dance when we need to find out where thue
default VGA device we created lives.

This all feels kinda silly as we're going through a dance to
dynamically find the device, but in practice it is no better
than just hardcoding it.

The problem we face in these examp\les is that as an input we
are giving QMEU a device 'id' but at runtime we're needing to
then use a QOM path instead of the 'id'. So we need a way to
translate an 'id' to a QOM path. What is the right way to do
this in a supported manner without making any assumptions
about QOM paths ?


For the CPUs cases, we don't have any 'id' on the CLI since
CPUs aren't configured that way, so we just hardcoded the
full path. You've pointed out query-hotpluggable-cpus which
is a possible solution.

In another case we're assuming that '/machine' has a property
called 'rtc-time'. IMHO it is reasonable to assume that
'/machine' as a QOM path is stable.

It isn't as simple as just saying "all QOM paths are unstable".

I struggle to come up with a good rule to explain what apps can
/ can't rely on wrt QOM paths, other than stay far away from
anything with '/unattached'.

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 v4 2/4] hw/i386: Attach CPUs to machine

[Igor Mammedov]

On Mon, 7 Feb 2022 11:48:27 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Mon, Feb 07, 2022 at 12:22:22PM +0100, Igor Mammedov wrote:
> > On Mon, 7 Feb 2022 10:36:42 +0100
> > Peter Krempa <pkrempa@redhat.com> wrote:
> >   
> > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote:  
> > > > On Mon, 7 Feb 2022 09:14:37 +0100
> > > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > >     
> > > > > On Sat,  5 Feb 2022 13:45:24 +0100
> > > > > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.      
> > > > > 
> > > > > Could you add to commit message the reason behind the change?    
> > > > 
> > > > regardless, it looks like unwarranted movement to me
> > > > prompted by livirt accessing/expecting a QOM patch which is
> > > > not stable ABI. I'd rather get it fixed on libvirt side.
> > > > 
> > > > If libvirt needs for some reason access a CPU instance,
> > > > it should use @query-hotpluggable-cpus to get a list of CPUs
> > > > (which includes QOM path of already present CPUs) instead of
> > > > hard-codding some 'well-known' path as there is no any guarantee 
> > > > that it will stay stable whatsoever.    
> > > 
> > > I don't disagree with you about the use of hardcoded path, but the way
> > > of using @query-hotpluggable-cpus is not really aligning well for how
> > > it's being used.
> > >
> > > To shed a bit more light, libvirt uses the following hardcoded path
> > > 
> > > #define QOM_CPU_PATH  "/machine/unattached/device[0]"
> > > 
> > > in code which is used to query CPU flags. That code doesn't care at all
> > > which cpus are present but wants to get any of them. So yes, calling
> > > query-hotpluggable-cpus is possible but a bit pointless.  
> > 
> > Even though query-hotpluggable-cpus is cumbersome
> > it still lets you avoid hardcodding QOM path and let you
> > get away with keeping "_400 QMP calls" probing while
> > something better comes along.
> > 
> >   
> > > In general the code probing cpu flags via qom-get is very cumbersome as
> > > it ends up doing ~400 QMP calls at startup of a VM in cases when we deem
> > > it necessary to probe the cpu fully.
> > > 
> > > It would be much better (And would sidestep the issue altoghether) if we
> > > had a more sane interface to probe all cpu flags in one go, and ideally
> > > the argument specifying the cpu being optional.
> > > 
> > > Libvirt can do the adjustment, but for now IMO the path to the first cpu
> > > (/machine/unattached/device[0]) became de-facto ABI by the virtue that
> > > it was used by libvirt and if I remember correctly it was suggested by
> > > the folks dealing with the CPU when the code was added originally.  
> > I would've argued against that back then as well,
> > there weren't any guarantee and I wouldn't like precedent of
> > QOM abuse becoming de-facto ABI.
> > Note: this patch breaks this so called ABI as well and introduces
> > yet another harcodded path without any stability guarantee whatsoever.  
> 
> AFAIK, we've never defined anything about QOM paths wrt ABI one way
> or the other ? In the absence of guidelines then it comes down to

not written in docs anyways (all I have is vague recollection that
we really didn't want to make of QOM path/tree an ABI).
For more on this topic see the comment at the end.

> what are reasonable expectations of the mgmt app. These expectations
> will be influenced by what it is actually possible to acheive given
> our API as exposed.
> 
> I think it is unreasonable to expect /machine/unattached to be
> stable because by its very nature it is just a dumping ground
> for anything where the dev hasn't put in any thought to the path
> placement.  IOW, it was/is definitely a bad idea for libvirt to
> rely on /machine/unattached in any way. That we're liable to be
> broken is not nice, but its really our own fault - we should 
> have asked for a better solution from day one here.
> 
> 
> I think it is somewhat reasonable to expect other QOM paths to
> be stable as there's been some degree of thought put into their
> placement. If we don't want apps to be considering other
> paths to be stable, then we need to explain exactly what they
> can and can't rely on, and most importantly actually provide
> a way for them to do what they need
> 
> 
> For example, libvirt needs a QOM path to query memory balloon
> stats. All libvirt knows is that it set 'id=balloon0' when
> creating it on the CLI. To find the balloon device in QOM it
> then looks for all paths under '/machine/peripheral', and
> tries to find one called 'child<$ID>' where $ID is the id=xxx
> value from the CLI.
> 
> We do the same dance when we need to find out where thue
> default VGA device we created lives.
> 
> This all feels kinda silly as we're going through a dance to
> dynamically find the device, but in practice it is no better
> than just hardcoding it.
> 
> The problem we face in these examp\les is that as an input we
> are giving QMEU a device 'id' but at runtime we're needing to
> then use a QOM path instead of the 'id'. So we need a way to
> translate an 'id' to a QOM path. What is the right way to do
> this in a supported manner without making any assumptions
> about QOM paths ?

Maybe we should have a QMP command that could find object
instance by ID?
 
> For the CPUs cases, we don't have any 'id' on the CLI since
> CPUs aren't configured that way, so we just hardcoded the
> full path. You've pointed out query-hotpluggable-cpus which
> is a possible solution.
> 
> In another case we're assuming that '/machine' has a property
> called 'rtc-time'. IMHO it is reasonable to assume that
> '/machine' as a QOM path is stable.

/machine probably will stay stable, but I wouldn't bet any money
on that either. But, don't we have a QMP interface to query machine
properties?
(if there were a need with a good for QEMU justification to change
/machine, it most likely would've been merged as long as it doesn't
break QEMU itself, same goes for other QOM paths)

> It isn't as simple as just saying "all QOM paths are unstable".
> 
> I struggle to come up with a good rule to explain what apps can
> / can't rely on wrt QOM paths, other than stay far away from
> anything with '/unattached'.

If I recall right, the main reason against declaring QOM path
as stable ABI, was maintenance headache QEMU side would get
with that as we wanted to have an ability to change tree
freely without breaking external users. My fear is that
making QOM path a stable ABI, will eventually dig us
trap similar to CLI interface, only more deeply embedded into
the code.

What this series does is trying to introduce an alternative
management API on top of exiting QMP one (willfully changing
child parent relations), that's fine if that's a way to go
forward, but it should be thought over and documented before
we go this route.

I always assumed that if a stable interface was necessary
for external users, the QMP was the way to go.

(it's easy to (ab)use direct QMP access, as usually it gives
one the access to the feature since the day it was merged,
but then it backfires, when QEMU changes unexpectedly,
since no one has been any paying attention to path stability
nor adds any compat knobs when it happens)
 
Anyways, we should discuss QOM path ABI topic again and makeup
our mind on this (and preferably documenting it somewhere)
to avoid wildly different assumptions about it.

> Regards,
> Daniel

Re: [PATCH v4 2/4] hw/i386: Attach CPUs to machine

[Peter Maydell]

On Mon, 7 Feb 2022 at 11:56, Daniel P. Berrangé <berrange@redhat.com> wrote:
> AFAIK, we've never defined anything about QOM paths wrt ABI one way
> or the other ? In the absence of guidelines then it comes down to
> what are reasonable expectations of the mgmt app. These expectations
> will be influenced by what it is actually possible to acheive given
> our API as exposed.
>
> I think it is unreasonable to expect /machine/unattached to be
> stable because by its very nature it is just a dumping ground
> for anything where the dev hasn't put in any thought to the path
> placement.  IOW, it was/is definitely a bad idea for libvirt to
> rely on /machine/unattached in any way. That we're liable to be
> broken is not nice, but its really our own fault - we should
> have asked for a better solution from day one here.
>
>
> I think it is somewhat reasonable to expect other QOM paths to
> be stable as there's been some degree of thought put into their
> placement. If we don't want apps to be considering other
> paths to be stable, then we need to explain exactly what they
> can and can't rely on, and most importantly actually provide
> a way for them to do what they need

I wouldn't personally expect other QOM paths to be stable
except in the sense of "probably don't change very often".
There are internal-to-QEMU code refactorings and rearrangements
that will change QOM paths, and there is no clear "warning,
don't change this stuff" to point people away from making
that kind of code change, nor are there tests in the test suite
that will catch accidental changes.

-- PMM

Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Yang Zhong]

On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> On Sat,  5 Feb 2022 13:45:26 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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.
> 
> yet again, why it's necessary?


  Igor, Sorry for delay feedback because of Chinese New Year holiday.

  This series patches are to fix below issues I reported before,
  https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html

  Since the /machine/unattached/device[0] is used by vcpu and Libvirt
  use this interface to get unavailable-features list. But in the SGX
  VM, the device[0] will be occupied by virtual sgx epc device, Libvirt
  can't get unavailable-features from this device[0].

  Although patch 2 in this series already fixed "unavailable-features" issue,
  this patch can move sgx virtual device from /machine/unattached/device[nn]
  to /machine/sgx-epc[nn], which seems more clear. Thanks!

  Yang
  

> 
> > 
> > Reported-by: Yang Zhong <yang.zhong@intel.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@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);

Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Yang Zhong]

On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> On Sat,  5 Feb 2022 13:45:26 +0100
> Philippe Mathieu-Daudé <f4bug@amsat.org> 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.
> 
> yet again, why it's necessary?
> 
> > 
> > Reported-by: Yang Zhong <yang.zhong@intel.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Reviewed-by: Daniel P. Berrangé <berrange@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);

   Philippe, I verified this patch, which work well. Thanks a lot!

   (qemu) qom-list /machine
   ......
   sgx-epc[2] (child<sgx-epc>)
   ......
   sgx-epc[0] (child<memory-region>)
   acpi-device (link<hotplug-handler>)
   sgx-epc[1] (child<sgx-epc>)
   ......

   Yang
  

Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Igor Mammedov]

On Mon, 14 Feb 2022 14:58:57 +0800
Yang Zhong <yang.zhong@intel.com> wrote:

> On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> > On Sat,  5 Feb 2022 13:45:26 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.  
> > 
> > yet again, why it's necessary?  
> 
> 
>   Igor, Sorry for delay feedback because of Chinese New Year holiday.
> 
>   This series patches are to fix below issues I reported before,
>   https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html
> 
>   Since the /machine/unattached/device[0] is used by vcpu and Libvirt
>   use this interface to get unavailable-features list. But in the SGX
>   VM, the device[0] will be occupied by virtual sgx epc device, Libvirt
>   can't get unavailable-features from this device[0].
> 
>   Although patch 2 in this series already fixed "unavailable-features" issue,

I've seen patches on libvirt fixing "unavailable-features" in another way
without dependence on  /machine/unattached/device[0].
see:
 https://www.mail-archive.com/libvir-list@redhat.com/msg226244.html

>   this patch can move sgx virtual device from /machine/unattached/device[nn]
>   to /machine/sgx-epc[nn], which seems more clear. Thanks!

with those patches device[0] becomes non issue, and this patch also becomes
unnecessary.
I don't mind putting sgx-epc under machine, but that shall be justified
somehow. A drawback I noticed in this case is an extra manual
plumbing/wiring without apparent need for it.

PS:
general note on submitting patches.
Commit message shall
 1 describe problem (+error message/way to reproduce the issue)
 2 what patch does
 3 and why patch fixes the issue in a certain way

commit message in this patch only does #2,  without any clue
to what the problem was nor why it tries to fix it this way.

> 
>   Yang
>   
> 
> >   
> > > 
> > > Reported-by: Yang Zhong <yang.zhong@intel.com>
> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Reviewed-by: Daniel P. Berrangé <berrange@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);  
> 

Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Daniel P. Berrangé]

On Mon, Feb 14, 2022 at 09:21:07AM +0100, Igor Mammedov wrote:
> On Mon, 14 Feb 2022 14:58:57 +0800
> Yang Zhong <yang.zhong@intel.com> wrote:
> 
> > On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> > > On Sat,  5 Feb 2022 13:45:26 +0100
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.  
> > > 
> > > yet again, why it's necessary?  
> > 
> > 
> >   Igor, Sorry for delay feedback because of Chinese New Year holiday.
> > 
> >   This series patches are to fix below issues I reported before,
> >   https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html
> > 
> >   Since the /machine/unattached/device[0] is used by vcpu and Libvirt
> >   use this interface to get unavailable-features list. But in the SGX
> >   VM, the device[0] will be occupied by virtual sgx epc device, Libvirt
> >   can't get unavailable-features from this device[0].
> > 
> >   Although patch 2 in this series already fixed "unavailable-features" issue,
> 
> I've seen patches on libvirt fixing "unavailable-features" in another way
> without dependence on  /machine/unattached/device[0].
> see:
>  https://www.mail-archive.com/libvir-list@redhat.com/msg226244.html
> 
> >   this patch can move sgx virtual device from /machine/unattached/device[nn]
> >   to /machine/sgx-epc[nn], which seems more clear. Thanks!
> 
> with those patches device[0] becomes non issue, and this patch also becomes
> unnecessary.
> I don't mind putting sgx-epc under machine, but that shall be justified
> somehow. A drawback I noticed in this case is an extra manual
> plumbing/wiring without apparent need for it.

This is effectively questioning why we have a QOM hierarchy with
named devices at all. IMHO we don't need to justify giving explicitly
named nodes under QOM beyond  "this is normal QOM modelling", and
anything under '/unattached' is subject to being fixed in this way.

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 v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Igor Mammedov]

On Mon, 14 Feb 2022 10:30:18 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Mon, Feb 14, 2022 at 09:21:07AM +0100, Igor Mammedov wrote:
> > On Mon, 14 Feb 2022 14:58:57 +0800
> > Yang Zhong <yang.zhong@intel.com> wrote:
> >   
> > > On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:  
> > > > On Sat,  5 Feb 2022 13:45:26 +0100
> > > > Philippe Mathieu-Daudé <f4bug@amsat.org> 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.    
> > > > 
> > > > yet again, why it's necessary?    
> > > 
> > > 
> > >   Igor, Sorry for delay feedback because of Chinese New Year holiday.
> > > 
> > >   This series patches are to fix below issues I reported before,
> > >   https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html
> > > 
> > >   Since the /machine/unattached/device[0] is used by vcpu and Libvirt
> > >   use this interface to get unavailable-features list. But in the SGX
> > >   VM, the device[0] will be occupied by virtual sgx epc device, Libvirt
> > >   can't get unavailable-features from this device[0].
> > > 
> > >   Although patch 2 in this series already fixed "unavailable-features" issue,  
> > 
> > I've seen patches on libvirt fixing "unavailable-features" in another way
> > without dependence on  /machine/unattached/device[0].
> > see:
> >  https://www.mail-archive.com/libvir-list@redhat.com/msg226244.html
> >   
> > >   this patch can move sgx virtual device from /machine/unattached/device[nn]
> > >   to /machine/sgx-epc[nn], which seems more clear. Thanks!  
> > 
> > with those patches device[0] becomes non issue, and this patch also becomes
> > unnecessary.
> > I don't mind putting sgx-epc under machine, but that shall be justified
> > somehow. A drawback I noticed in this case is an extra manual
> > plumbing/wiring without apparent need for it.  
> 
> This is effectively questioning why we have a QOM hierarchy with
> named devices at all. IMHO we don't need to justify giving explicitly
> named nodes under QOM beyond  "this is normal QOM modelling", and
> anything under '/unattached' is subject to being fixed in this way.

I agree that we should fix '/unattached', however blindly naming and
moving it wherever just because we can is not the fixing I've have had
in the mind.

With QOM device models, I'd try to compose parent/child relationships
like it's done in real hardware (ex: apic is a part of x86 CPU, so we
made cpu its parent, there are many ARM device models that follow
the same suit.)

In commit message, there must be a reason/explanation as to why
proposed parent has been chosen.
The current reason (lets get it out of the way just because some
userspace abused direct access to QOM) in commit message in not
a valid (I'd even say wasn't valid to begin with).
All I'm asking for is for sane commit message explaining why
something is moved to where it's proposed so that others can
understand it when looking at it.

With this patch I'm not sure if SGX should be a part of machine
or a part of CPU device model. (it seem SGX is a CPU feature
after all)
 
> Regards,
> Daniel