* [PATCH] sgx: Move sgx object from /machine/unattached to /machine @ 2022-01-12 16:55 Yang Zhong 2022-01-12 10:11 ` Daniel P. Berrangé 0 siblings, 1 reply; 5+ messages in thread From: Yang Zhong @ 2022-01-12 16:55 UTC (permalink / raw) To: pbonzini, berrange, eduardo; +Cc: yang.zhong, qemu-devel 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]. 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); + } else if (!obj->parent) { gchar *name = g_strdup_printf("device[%d]", unattached_count++); object_property_add_child(container_get(qdev_get_machine(), ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] sgx: Move sgx object from /machine/unattached to /machine 2022-01-12 16:55 [PATCH] sgx: Move sgx object from /machine/unattached to /machine Yang Zhong @ 2022-01-12 10:11 ` Daniel P. Berrangé 2022-01-12 10:16 ` Peter Maydell 2022-01-13 7:05 ` Yang Zhong 0 siblings, 2 replies; 5+ messages in thread From: Daniel P. Berrangé @ 2022-01-12 10:11 UTC (permalink / raw) To: Yang Zhong; +Cc: eduardo, pbonzini, qemu-devel 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 ? > > 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. > + } 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 :| ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sgx: Move sgx object from /machine/unattached to /machine 2022-01-12 10:11 ` Daniel P. Berrangé @ 2022-01-12 10:16 ` Peter Maydell 2022-01-12 10:24 ` Daniel P. Berrangé 2022-01-13 7:05 ` Yang Zhong 1 sibling, 1 reply; 5+ messages in thread From: Peter Maydell @ 2022-01-12 10:16 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: Yang Zhong, eduardo, qemu-devel, pbonzini On Wed, 12 Jan 2022 at 10:14, Daniel P. Berrangé <berrange@redhat.com> 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 ? libvirt having to know it needs to look at /machine/unattached/device[n] for anything is a bit fragile, really... it's effectively encoding knowledge about what order things happen to get created inside QEMU. -- PMM ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sgx: Move sgx object from /machine/unattached to /machine 2022-01-12 10:16 ` Peter Maydell @ 2022-01-12 10:24 ` Daniel P. Berrangé 0 siblings, 0 replies; 5+ messages in thread From: Daniel P. Berrangé @ 2022-01-12 10:24 UTC (permalink / raw) To: Peter Maydell; +Cc: Yang Zhong, eduardo, qemu-devel, pbonzini On Wed, Jan 12, 2022 at 10:16:33AM +0000, Peter Maydell wrote: > On Wed, 12 Jan 2022 at 10:14, Daniel P. Berrangé <berrange@redhat.com> 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 ? > > libvirt having to know it needs to look at /machine/unattached/device[n] > for anything is a bit fragile, really... it's effectively encoding > knowledge about what order things happen to get created inside QEMU. So how do CPUs and other devices end up being under /unattached/ ? Can we ensure that *all* QEMU devices have a well defined attachment point ? 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 :| ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sgx: Move sgx object from /machine/unattached to /machine 2022-01-12 10:11 ` Daniel P. Berrangé 2022-01-12 10:16 ` Peter Maydell @ 2022-01-13 7:05 ` Yang Zhong 1 sibling, 0 replies; 5+ messages in thread From: Yang Zhong @ 2022-01-13 7:05 UTC (permalink / raw) To: Daniel P. Berrangé Cc: eduardo, yang.zhong, lin.a.yang, qemu-devel, pbonzini, haibin.huang 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 :| ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-13 7:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-12 16:55 [PATCH] sgx: Move sgx object from /machine/unattached to /machine Yang Zhong 2022-01-12 10:11 ` Daniel P. Berrangé 2022-01-12 10:16 ` Peter Maydell 2022-01-12 10:24 ` Daniel P. Berrangé 2022-01-13 7:05 ` Yang Zhong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.