Re: [PATCH v4 4/4] hw/i386/sgx: Attach SGX-EPC objects to machine

[Igor Mammedov <imammedo@redhat.com>]

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