Re: [PATCH] sgx: Move sgx object from /machine/unattached to /machine

[Yang Zhong <yang.zhong@intel.com>]

Hi Daniel,

On Wed, Jan 12, 2022 at 10:11:35AM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 12, 2022 at 11:55:17AM -0500, Yang Zhong wrote:
> > When Libvirt start, it get the vcpu's unavailable-features from
> > /machine/unattached/device[0] path by qom-get command, but in SGX
> > guest, since the sgx-epc virtual device is initialized before VCPU
> > creation(virtual sgx need set the virtual EPC info in the cpuid). This
> > /machine/unattached/device[0] is occupied by sgx-epc device, which
> > fail to get the unvailable-features from /machine/unattached/device[0].
> 
> If libvirt decides to enable SGX in a VM, then surely it knows
> that it should just query /machine/unattached/device[1] to get
> the CPU features instead. Why do we need to do anything in QEMU ?
> 

  I listed two solutions in the Qemu or Libvirt before:
  https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html

  This time, I posted this patch and hope to have a talk for this issue.

  If Libvirt side should handle this, I will drop this patch and inform
  them to do this. Thanks!


> > 
> > This patch make one new /machine/sgx object to avoid this issue.
> > (qemu) qom-list /machine/unattached/
> > device[0] (child<host-x86_64-cpu>)
> > 
> > (qemu) qom-list /machine/sgx
> > device[0] (child<sgx-epc>)
> > 
> > Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> > ---
> >  hw/core/qdev.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 84f3019440..4154eef0d8 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -497,7 +497,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >      NamedClockList *ncl;
> >      Error *local_err = NULL;
> >      bool unattached_parent = false;
> > -    static int unattached_count;
> > +    static int unattached_count, sgx_count;
> >  
> >      if (dev->hotplugged && !dc->hotpluggable) {
> >          error_setg(errp, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
> > @@ -509,7 +509,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >              goto fail;
> >          }
> >  
> > -        if (!obj->parent) {
> > +        if (!obj->parent && !strcmp(object_get_typename(obj), "sgx-epc")) {
> > +            gchar *name = g_strdup_printf("device[%d]", sgx_count++);
> > +
> > +            object_property_add_child(container_get(qdev_get_machine(),
> > +                                                    "/sgx"),
> > +                                      name, obj);
> > +            unattached_parent = true;
> > +            g_free(name);
> 
> The qdev.c file is part of our generic object code. It should not
> contain any code that is tied to very specific object types like
> this.

  Okay, thanks!

  Yang 


> 
> > +        } else if (!obj->parent) {
> >              gchar *name = g_strdup_printf("device[%d]", unattached_count++);
> >  
> >              object_property_add_child(container_get(qdev_get_machine(),
> 
> 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 :|