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

[PATCH 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 to the machine,
then the SGX-EPC to its memory backend.

Philippe Mathieu-Daudé (2):
  hw/i386/x86: Attach CPUs to machine
  hw/i386/sgx: Attach SGX-EPC to its memory backend

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

-- 
2.34.1

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

[Philippe Mathieu-Daudé via]

Avoid having CPUs objects dangling as unattached QOM ones,
directly attach them to the machine.

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

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

[Daniel P. Berrangé]

On Mon, Jan 17, 2022 at 12:53:30AM +0100, Philippe Mathieu-Daudé via wrote:
> Avoid having CPUs objects dangling as unattached QOM ones,
> directly attach them to the machine.

Lets be more explicit here

[quote]
  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
[/quote]

to  /machine/unattached/device[0->$SMP-COUNT]

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

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

[Philippe Mathieu-Daudé via]

On 1/17/22 14:48, Daniel P. Berrangé wrote:
> On Mon, Jan 17, 2022 at 12:53:30AM +0100, Philippe Mathieu-Daudé via wrote:
>> Avoid having CPUs objects dangling as unattached QOM ones,
>> directly attach them to the machine.
> 
> Lets be more explicit here
> 
> [quote]
>   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
> [/quote]

OK, thanks for the help here!

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

[Yang Zhong]

On Mon, Jan 17, 2022 at 01:48:46PM +0000, Daniel P. Berrangé wrote:
> On Mon, Jan 17, 2022 at 12:53:30AM +0100, Philippe Mathieu-Daudé via wrote:
> > Avoid having CPUs objects dangling as unattached QOM ones,
> > directly attach them to the machine.
> 
> Lets be more explicit here
> 
> [quote]
>   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
> [/quote]
> 
> to  /machine/unattached/device[0->$SMP-COUNT]
> 
> > 
> > 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
> > 
> > 


  Thanks Philippe, if we change /machine/unattached/device[nn] to /machine/cpu[nn],
  the related changes should also be done in the Libvirt side, which still check
  /machine/unattached/device[0] to get unvailable-features. thanks!

  Yang

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

[RFC PATCH 2/2] hw/i386/sgx: Attach SGX-EPC to its memory backend

[Philippe Mathieu-Daudé via]

We have one SGX-EPC address/size/node per memory backend,
make it child of the backend in the QOM composition tree.

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

diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index 5de5dd08936..6362e5e9d02 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -300,6 +300,9 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
         /* set the memdev link with memory backend */
         object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
                               &error_fatal);
+        object_property_add_child(OBJECT(list->value->memdev), "sgx-epc",
+                                  OBJECT(obj));
+
         /* set the numa node property for sgx epc object */
         object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
                              &error_fatal);
-- 
2.34.1

Re: [RFC PATCH 2/2] hw/i386/sgx: Attach SGX-EPC to its memory backend

[Paolo Bonzini]

On 1/17/22 00:53, Philippe Mathieu-Daudé via wrote:
> We have one SGX-EPC address/size/node per memory backend,
> make it child of the backend in the QOM composition tree.
> 
> Cc: Yang Zhong <yang.zhong@intel.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   hw/i386/sgx.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index 5de5dd08936..6362e5e9d02 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -300,6 +300,9 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>           /* set the memdev link with memory backend */
>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>                                 &error_fatal);
> +        object_property_add_child(OBJECT(list->value->memdev), "sgx-epc",
> +                                  OBJECT(obj));
> +
>           /* set the numa node property for sgx epc object */
>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>                                &error_fatal);

I don't think this is a good idea; only list->value->memdev should add 
something below itself in the tree.

However, I think obj can be added under the machine itself as 
/machine/sgx-epc-device[*].

Paolo

Re: [RFC PATCH 2/2] hw/i386/sgx: Attach SGX-EPC to its memory backend

[Philippe Mathieu-Daudé via]

On 1/17/22 12:48, Paolo Bonzini wrote:
> On 1/17/22 00:53, Philippe Mathieu-Daudé via wrote:
>> We have one SGX-EPC address/size/node per memory backend,
>> make it child of the backend in the QOM composition tree.
>>
>> Cc: Yang Zhong <yang.zhong@intel.com>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   hw/i386/sgx.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>> index 5de5dd08936..6362e5e9d02 100644
>> --- a/hw/i386/sgx.c
>> +++ b/hw/i386/sgx.c
>> @@ -300,6 +300,9 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>           /* set the memdev link with memory backend */
>>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP,
>> list->value->memdev,
>>                                 &error_fatal);
>> +        object_property_add_child(OBJECT(list->value->memdev),
>> "sgx-epc",
>> +                                  OBJECT(obj));
>> +
>>           /* set the numa node property for sgx epc object */
>>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP,
>> list->value->node,
>>                                &error_fatal);
> 
> I don't think this is a good idea; only list->value->memdev should add
> something below itself in the tree.

OK, I see.

> However, I think obj can be added under the machine itself as
> /machine/sgx-epc-device[*].

OK. It is hard to understand the difference between /unattached and
/machine.

Thanks for the review,

Phil.

Re: [RFC PATCH 2/2] hw/i386/sgx: Attach SGX-EPC to its memory backend

[Yang Zhong]

On Mon, Jan 17, 2022 at 12:48:10PM +0100, Paolo Bonzini wrote:
> On 1/17/22 00:53, Philippe Mathieu-Daudé via wrote:
> >We have one SGX-EPC address/size/node per memory backend,
> >make it child of the backend in the QOM composition tree.
> >
> >Cc: Yang Zhong <yang.zhong@intel.com>
> >Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >---
> >  hw/i386/sgx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> >index 5de5dd08936..6362e5e9d02 100644
> >--- a/hw/i386/sgx.c
> >+++ b/hw/i386/sgx.c
> >@@ -300,6 +300,9 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
> >          /* set the memdev link with memory backend */
> >          object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
> >                                &error_fatal);
> >+        object_property_add_child(OBJECT(list->value->memdev), "sgx-epc",
> >+                                  OBJECT(obj));
> >+
> >          /* set the numa node property for sgx epc object */
> >          object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
> >                               &error_fatal);
> 
> I don't think this is a good idea; only list->value->memdev should
> add something below itself in the tree.
> 
> However, I think obj can be added under the machine itself as
> /machine/sgx-epc-device[*].
> 

  Thanks Philippe, calling object_property_add_child() in the hw/i386/sgx.c is more
  reasonable than in device_set_realized(), thanks!

  Yang

> Paolo

Re: [RFC PATCH 2/2] hw/i386/sgx: Attach SGX-EPC to its memory backend

[Yang Zhong]

On Mon, Jan 17, 2022 at 12:48:10PM +0100, Paolo Bonzini wrote:
> On 1/17/22 00:53, Philippe Mathieu-Daudé via wrote:
> >We have one SGX-EPC address/size/node per memory backend,
> >make it child of the backend in the QOM composition tree.
> >
> >Cc: Yang Zhong <yang.zhong@intel.com>
> >Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >---
> >  hw/i386/sgx.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> >index 5de5dd08936..6362e5e9d02 100644
> >--- a/hw/i386/sgx.c
> >+++ b/hw/i386/sgx.c
> >@@ -300,6 +300,9 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
> >          /* set the memdev link with memory backend */
> >          object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
> >                                &error_fatal);
> >+        object_property_add_child(OBJECT(list->value->memdev), "sgx-epc",
> >+                                  OBJECT(obj));
> >+
> >          /* set the numa node property for sgx epc object */
> >          object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
> >                               &error_fatal);
> 
> I don't think this is a good idea; only list->value->memdev should
> add something below itself in the tree.
> 
> However, I think obj can be added under the machine itself as
> /machine/sgx-epc-device[*].
> 

  Philippe, Sorry I can't receive all Qemu mails from my mutt tool.

  https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg03535.html
  I verified this patch, and the issue was reported as below:

  Unexpected error in object_property_try_add() at ../qom/object.c:1224:
  qemu-system-x86_64: attempt to add duplicate property 'sgx-epc' to object (type 'pc-q35-7.0-machine')
  Aborted (core dumped)

  Even I changed it to another name, which still reported same kind of issue.

  I tried below patch as my previous patch, and it can work
  diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
  index d60485fc422..66444745b47 100644
  --- a/hw/i386/sgx.c
  +++ b/hw/i386/sgx.c
  @@ -281,6 +281,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
       SGXEPCState *sgx_epc = &pcms->sgx_epc;
       X86MachineState *x86ms = X86_MACHINE(pcms);
       SgxEPCList *list = NULL;
  +    int sgx_count = 0;
       Object *obj;

       memset(sgx_epc, 0, sizeof(SGXEPCState));
  @@ -297,7 +298,9 @@ 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));
  +        gchar *name = g_strdup_printf("device[%d]", sgx_count++);
  +        object_property_add_child(container_get(qdev_get_machine(), "/sgx-epc-device"),
  +                                      name, obj);
  
           /* set the memdev link with memory backend */
           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,


  From the monitor, 
  (qemu) qom-list /machine/sgx-epc-device
  type (string)
  device[0] (child<sgx-epc>)
  device[1] (child<sgx-epc>)
  
  This can normally show two sgx epc section devices.
  
  If you have new patch, I can help verify, thanks!

  Yang

> Paolo

Re: [RFC PATCH 2/2] hw/i386/sgx: Attach SGX-EPC to its memory backend

[Philippe Mathieu-Daudé via]

On 23/1/22 13:52, Yang Zhong wrote:
> On Mon, Jan 17, 2022 at 12:48:10PM +0100, Paolo Bonzini wrote:
>> On 1/17/22 00:53, Philippe Mathieu-Daudé via wrote:
>>> We have one SGX-EPC address/size/node per memory backend,
>>> make it child of the backend in the QOM composition tree.
>>>
>>> Cc: Yang Zhong <yang.zhong@intel.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>   hw/i386/sgx.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>>> index 5de5dd08936..6362e5e9d02 100644
>>> --- a/hw/i386/sgx.c
>>> +++ b/hw/i386/sgx.c
>>> @@ -300,6 +300,9 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>>>           /* set the memdev link with memory backend */
>>>           object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>>>                                 &error_fatal);
>>> +        object_property_add_child(OBJECT(list->value->memdev), "sgx-epc",
>>> +                                  OBJECT(obj));
>>> +
>>>           /* set the numa node property for sgx epc object */
>>>           object_property_set_uint(obj, SGX_EPC_NUMA_NODE_PROP, list->value->node,
>>>                                &error_fatal);
>>
>> I don't think this is a good idea; only list->value->memdev should
>> add something below itself in the tree.
>>
>> However, I think obj can be added under the machine itself as
>> /machine/sgx-epc-device[*].
>>
> 
>    Philippe, Sorry I can't receive all Qemu mails from my mutt tool.
> 
>    https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg03535.html
>    I verified this patch, and the issue was reported as below:
> 
>    Unexpected error in object_property_try_add() at ../qom/object.c:1224:
>    qemu-system-x86_64: attempt to add duplicate property 'sgx-epc' to object (type 'pc-q35-7.0-machine')
>    Aborted (core dumped)
> 
>    Even I changed it to another name, which still reported same kind of issue.
> 
>    I tried below patch as my previous patch, and it can work
>    diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
>    index d60485fc422..66444745b47 100644
>    --- a/hw/i386/sgx.c
>    +++ b/hw/i386/sgx.c
>    @@ -281,6 +281,7 @@ void pc_machine_init_sgx_epc(PCMachineState *pcms)
>         SGXEPCState *sgx_epc = &pcms->sgx_epc;
>         X86MachineState *x86ms = X86_MACHINE(pcms);
>         SgxEPCList *list = NULL;
>    +    int sgx_count = 0;
>         Object *obj;
> 
>         memset(sgx_epc, 0, sizeof(SGXEPCState));
>    @@ -297,7 +298,9 @@ 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));
>    +        gchar *name = g_strdup_printf("device[%d]", sgx_count++);

Oops yes you are right... Fixed in v3!

>    +        object_property_add_child(container_get(qdev_get_machine(), "/sgx-epc-device"),
>    +                                      name, obj);
>    
>             /* set the memdev link with memory backend */
>             object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
> 
> 
>    From the monitor,
>    (qemu) qom-list /machine/sgx-epc-device
>    type (string)
>    device[0] (child<sgx-epc>)
>    device[1] (child<sgx-epc>)
>    
>    This can normally show two sgx epc section devices.
>    
>    If you have new patch, I can help verify, thanks!

Here you go for v3:
https://lore.kernel.org/qemu-devel/20220131233507.334174-1-f4bug@amsat.org/

Regards,

Phil.