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

[Philippe Mathieu-Daudé]

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:
20220131233507.334174-1-f4bug@amsat.org/">https://lore.kernel.org/qemu-devel/20220131233507.334174-1-f4bug@amsat.org/

Regards,

Phil.