| Message ID | 20220205124526.500158-5-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386: QOM-attach CPUs/SGX-EPC objects to their parents | expand |
On Sat, 5 Feb 2022 13:45:26 +0100 Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <yang.zhong@intel.com> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > 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, > &error_fatal);
On 7/2/22 09:37, Igor Mammedov wrote: > On Sat, 5 Feb 2022 13:45:26 +0100 > Philippe Mathieu-Daudé <f4bug@amsat.org> 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 <yang.zhong@intel.com> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> 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, >> &error_fatal); >
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é <f4bug@amsat.org> 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 <yang.zhong@intel.com> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > 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, > > &error_fatal);
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é <f4bug@amsat.org> 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 <yang.zhong@intel.com> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > 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, > > &error_fatal); Philippe, I verified this patch, which work well. Thanks a lot! (qemu) qom-list /machine ...... sgx-epc[2] (child<sgx-epc>) ...... sgx-epc[0] (child<memory-region>) acpi-device (link<hotplug-handler>) sgx-epc[1] (child<sgx-epc>) ...... Yang
On Mon, 14 Feb 2022 14:58:57 +0800 Yang Zhong <yang.zhong@intel.com> 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é <f4bug@amsat.org> 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 <yang.zhong@intel.com> > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > --- > > > 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, > > > &error_fatal); >
On Mon, Feb 14, 2022 at 09:21:07AM +0100, Igor Mammedov wrote: > On Mon, 14 Feb 2022 14:58:57 +0800 > Yang Zhong <yang.zhong@intel.com> 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é <f4bug@amsat.org> 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
On Mon, 14 Feb 2022 10:30:18 +0000 Daniel P. Berrangé <berrange@redhat.com> 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 <yang.zhong@intel.com> 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é <f4bug@amsat.org> 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
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, &error_fatal);