Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On Mon, 14 Feb 2022 10:30:18 + Daniel P. Berrangé wrote: > On Mon, Feb 14, 2022 at 09:21:07AM +0100, Igor Mammedov wrote: > > On Mon, 14 Feb 2022 14:58:57 +0800 > > Yang Zhong wrote: > > > > > On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote: > > > > On Sat, 5 Feb 2022 13:45:26 +0100 > > > > Philippe Mathieu-Daudé wrote: > > > > > > > > > Previously SGX-EPC objects were exposed in the QOM tree at a path > > > > > > > > > > /machine/unattached/device[nn] > > > > > > > > > > where the 'nn' varies depending on what devices were already created. > > > > > > > > > > With this change the SGX-EPC objects are now at > > > > > > > > > > /machine/sgx-epc[nn] > > > > > > > > > > where the 'nn' of the first SGX-EPC object is always zero. > > > > > > > > yet again, why it's necessary? > > > > > > > > > Igor, Sorry for delay feedback because of Chinese New Year holiday. > > > > > > This series patches are to fix below issues I reported before, > > > https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html > > > > > > Since the /machine/unattached/device[0] is used by vcpu and Libvirt > > > use this interface to get unavailable-features list. But in the SGX > > > VM, the device[0] will be occupied by virtual sgx epc device, Libvirt > > > can't get unavailable-features from this device[0]. > > > > > > Although patch 2 in this series already fixed "unavailable-features" > > > issue, > > > > I've seen patches on libvirt fixing "unavailable-features" in another way > > without dependence on /machine/unattached/device[0]. > > see: > > https://www.mail-archive.com/libvir-list@redhat.com/msg226244.html > > > > > this patch can move sgx virtual device from > > > /machine/unattached/device[nn] > > > to /machine/sgx-epc[nn], which seems more clear. Thanks! > > > > with those patches device[0] becomes non issue, and this patch also becomes > > unnecessary. > > I don't mind putting sgx-epc under machine, but that shall be justified > > somehow. A drawback I noticed in this case is an extra manual > > plumbing/wiring without apparent need for it. > > This is effectively questioning why we have a QOM hierarchy with > named devices at all. IMHO we don't need to justify giving explicitly > named nodes under QOM beyond "this is normal QOM modelling", and > anything under '/unattached' is subject to being fixed in this way. I agree that we should fix '/unattached', however blindly naming and moving it wherever just because we can is not the fixing I've have had in the mind. With QOM device models, I'd try to compose parent/child relationships like it's done in real hardware (ex: apic is a part of x86 CPU, so we made cpu its parent, there are many ARM device models that follow the same suit.) In commit message, there must be a reason/explanation as to why proposed parent has been chosen. The current reason (lets get it out of the way just because some userspace abused direct access to QOM) in commit message in not a valid (I'd even say wasn't valid to begin with). All I'm asking for is for sane commit message explaining why something is moved to where it's proposed so that others can understand it when looking at it. With this patch I'm not sure if SGX should be a part of machine or a part of CPU device model. (it seem SGX is a CPU feature after all) > Regards, > Daniel
Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On Mon, Feb 14, 2022 at 09:21:07AM +0100, Igor Mammedov wrote: > On Mon, 14 Feb 2022 14:58:57 +0800 > Yang Zhong wrote: > > > On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote: > > > On Sat, 5 Feb 2022 13:45:26 +0100 > > > Philippe Mathieu-Daudé wrote: > > > > > > > Previously SGX-EPC objects were exposed in the QOM tree at a path > > > > > > > > /machine/unattached/device[nn] > > > > > > > > where the 'nn' varies depending on what devices were already created. > > > > > > > > With this change the SGX-EPC objects are now at > > > > > > > > /machine/sgx-epc[nn] > > > > > > > > where the 'nn' of the first SGX-EPC object is always zero. > > > > > > yet again, why it's necessary? > > > > > > Igor, Sorry for delay feedback because of Chinese New Year holiday. > > > > This series patches are to fix below issues I reported before, > > https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html > > > > Since the /machine/unattached/device[0] is used by vcpu and Libvirt > > use this interface to get unavailable-features list. But in the SGX > > VM, the device[0] will be occupied by virtual sgx epc device, Libvirt > > can't get unavailable-features from this device[0]. > > > > Although patch 2 in this series already fixed "unavailable-features" > > issue, > > I've seen patches on libvirt fixing "unavailable-features" in another way > without dependence on /machine/unattached/device[0]. > see: > https://www.mail-archive.com/libvir-list@redhat.com/msg226244.html > > > this patch can move sgx virtual device from /machine/unattached/device[nn] > > to /machine/sgx-epc[nn], which seems more clear. Thanks! > > with those patches device[0] becomes non issue, and this patch also becomes > unnecessary. > I don't mind putting sgx-epc under machine, but that shall be justified > somehow. A drawback I noticed in this case is an extra manual > plumbing/wiring without apparent need for it. This is effectively questioning why we have a QOM hierarchy with named devices at all. IMHO we don't need to justify giving explicitly named nodes under QOM beyond "this is normal QOM modelling", and anything under '/unattached' is subject to being fixed in this way. 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 v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On Mon, 14 Feb 2022 14:58:57 +0800
Yang Zhong wrote:
> On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> > On Sat, 5 Feb 2022 13:45:26 +0100
> > Philippe Mathieu-Daudé wrote:
> >
> > > Previously SGX-EPC objects were exposed in the QOM tree at a path
> > >
> > > /machine/unattached/device[nn]
> > >
> > > where the 'nn' varies depending on what devices were already created.
> > >
> > > With this change the SGX-EPC objects are now at
> > >
> > > /machine/sgx-epc[nn]
> > >
> > > where the 'nn' of the first SGX-EPC object is always zero.
> >
> > yet again, why it's necessary?
>
>
> Igor, Sorry for delay feedback because of Chinese New Year holiday.
>
> This series patches are to fix below issues I reported before,
> https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html
>
> Since the /machine/unattached/device[0] is used by vcpu and Libvirt
> use this interface to get unavailable-features list. But in the SGX
> VM, the device[0] will be occupied by virtual sgx epc device, Libvirt
> can't get unavailable-features from this device[0].
>
> Although patch 2 in this series already fixed "unavailable-features" issue,
I've seen patches on libvirt fixing "unavailable-features" in another way
without dependence on /machine/unattached/device[0].
see:
https://www.mail-archive.com/libvir-list@redhat.com/msg226244.html
> this patch can move sgx virtual device from /machine/unattached/device[nn]
> to /machine/sgx-epc[nn], which seems more clear. Thanks!
with those patches device[0] becomes non issue, and this patch also becomes
unnecessary.
I don't mind putting sgx-epc under machine, but that shall be justified
somehow. A drawback I noticed in this case is an extra manual
plumbing/wiring without apparent need for it.
PS:
general note on submitting patches.
Commit message shall
1 describe problem (+error message/way to reproduce the issue)
2 what patch does
3 and why patch fixes the issue in a certain way
commit message in this patch only does #2, without any clue
to what the problem was nor why it tries to fix it this way.
>
> Yang
>
>
> >
> > >
> > > Reported-by: Yang Zhong
> > > Suggested-by: Paolo Bonzini
> > > Reviewed-by: Daniel P. Berrangé
> > > Signed-off-by: Philippe Mathieu-Daudé
> > > ---
> > > hw/i386/sgx.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> > > index a2b318dd938..3ab2217ca43 100644
> > > --- a/hw/i386/sgx.c
> > > +++ b/hw/i386/sgx.c
> > > @@ -304,6 +304,8 @@ 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));
> > > +
> > > /* set the memdev link with memory backend */
> > > object_property_parse(obj, SGX_EPC_MEMDEV_PROP,
> > > list->value->memdev,
> > >_fatal);
>
Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> On Sat, 5 Feb 2022 13:45:26 +0100
> Philippe Mathieu-Daudé wrote:
>
> > Previously SGX-EPC objects were exposed in the QOM tree at a path
> >
> > /machine/unattached/device[nn]
> >
> > where the 'nn' varies depending on what devices were already created.
> >
> > With this change the SGX-EPC objects are now at
> >
> > /machine/sgx-epc[nn]
> >
> > where the 'nn' of the first SGX-EPC object is always zero.
>
> yet again, why it's necessary?
>
> >
> > Reported-by: Yang Zhong
> > Suggested-by: Paolo Bonzini
> > Reviewed-by: Daniel P. Berrangé
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > hw/i386/sgx.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> > index a2b318dd938..3ab2217ca43 100644
> > --- a/hw/i386/sgx.c
> > +++ b/hw/i386/sgx.c
> > @@ -304,6 +304,8 @@ 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));
> > +
> > /* set the memdev link with memory backend */
> > object_property_parse(obj, SGX_EPC_MEMDEV_PROP,
> > list->value->memdev,
> >_fatal);
Philippe, I verified this patch, which work well. Thanks a lot!
(qemu) qom-list /machine
..
sgx-epc[2] (child)
..
sgx-epc[0] (child)
acpi-device (link)
sgx-epc[1] (child)
..
Yang
Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On Mon, Feb 07, 2022 at 09:37:52AM +0100, Igor Mammedov wrote:
> On Sat, 5 Feb 2022 13:45:26 +0100
> Philippe Mathieu-Daudé wrote:
>
> > Previously SGX-EPC objects were exposed in the QOM tree at a path
> >
> > /machine/unattached/device[nn]
> >
> > where the 'nn' varies depending on what devices were already created.
> >
> > With this change the SGX-EPC objects are now at
> >
> > /machine/sgx-epc[nn]
> >
> > where the 'nn' of the first SGX-EPC object is always zero.
>
> yet again, why it's necessary?
Igor, Sorry for delay feedback because of Chinese New Year holiday.
This series patches are to fix below issues I reported before,
https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg05670.html
Since the /machine/unattached/device[0] is used by vcpu and Libvirt
use this interface to get unavailable-features list. But in the SGX
VM, the device[0] will be occupied by virtual sgx epc device, Libvirt
can't get unavailable-features from this device[0].
Although patch 2 in this series already fixed "unavailable-features" issue,
this patch can move sgx virtual device from /machine/unattached/device[nn]
to /machine/sgx-epc[nn], which seems more clear. Thanks!
Yang
>
> >
> > Reported-by: Yang Zhong
> > Suggested-by: Paolo Bonzini
> > Reviewed-by: Daniel P. Berrangé
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > hw/i386/sgx.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> > index a2b318dd938..3ab2217ca43 100644
> > --- a/hw/i386/sgx.c
> > +++ b/hw/i386/sgx.c
> > @@ -304,6 +304,8 @@ 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));
> > +
> > /* set the memdev link with memory backend */
> > object_property_parse(obj, SGX_EPC_MEMDEV_PROP,
> > list->value->memdev,
> >_fatal);
Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On 7/2/22 09:37, Igor Mammedov wrote:
On Sat, 5 Feb 2022 13:45:26 +0100
Philippe Mathieu-Daudé wrote:
Previously SGX-EPC objects were exposed in the QOM tree at a path
/machine/unattached/device[nn]
where the 'nn' varies depending on what devices were already created.
With this change the SGX-EPC objects are now at
/machine/sgx-epc[nn]
where the 'nn' of the first SGX-EPC object is always zero.
yet again, why it's necessary?
I'll defer that question to Yang & Daniel.
Reported-by: Yang Zhong
Suggested-by: Paolo Bonzini
Reviewed-by: Daniel P. Berrangé
Signed-off-by: Philippe Mathieu-Daudé
---
hw/i386/sgx.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
index a2b318dd938..3ab2217ca43 100644
--- a/hw/i386/sgx.c
+++ b/hw/i386/sgx.c
@@ -304,6 +304,8 @@ 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));
+
/* set the memdev link with memory backend */
object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
_fatal);
Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine
On Sat, 5 Feb 2022 13:45:26 +0100
Philippe Mathieu-Daudé wrote:
> Previously SGX-EPC objects were exposed in the QOM tree at a path
>
> /machine/unattached/device[nn]
>
> where the 'nn' varies depending on what devices were already created.
>
> With this change the SGX-EPC objects are now at
>
> /machine/sgx-epc[nn]
>
> where the 'nn' of the first SGX-EPC object is always zero.
yet again, why it's necessary?
>
> Reported-by: Yang Zhong
> Suggested-by: Paolo Bonzini
> Reviewed-by: Daniel P. Berrangé
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/i386/sgx.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/i386/sgx.c b/hw/i386/sgx.c
> index a2b318dd938..3ab2217ca43 100644
> --- a/hw/i386/sgx.c
> +++ b/hw/i386/sgx.c
> @@ -304,6 +304,8 @@ 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));
> +
> /* set the memdev link with memory backend */
> object_property_parse(obj, SGX_EPC_MEMDEV_PROP, list->value->memdev,
>_fatal);
