* 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
* [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 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.