Re: [PULL 11/13] numa: Support SGX numa in the monitor and Libvirt interfaces

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Yang Zhong <yang.zhong@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Peter Krempa <pkrempa@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PULL 11/13] numa: Support SGX numa in the monitor and Libvirt interfaces
Date: Wed, 19 Jan 2022 09:19:49 +0000	[thread overview]
Message-ID: <YefXtfvTUCClK8Bj@redhat.com> (raw)
In-Reply-To: <20220117103731.GB20805@yangzhon-Virtual>

On Mon, Jan 17, 2022 at 06:37:31PM +0800, Yang Zhong wrote:
> On Thu, Jan 13, 2022 at 04:15:10PM +0000, Daniel P. Berrangé wrote:
> > On Wed, Dec 15, 2021 at 09:25:13PM +0100, Paolo Bonzini wrote:
> > > From: Yang Zhong <yang.zhong@intel.com>
> > > 
> > > Add the SGXEPCSection list into SGXInfo to show the multiple
> > > SGX EPC sections detailed info, not the total size like before.
> > > This patch can enable numa support for 'info sgx' command and
> > > QMP interfaces. The new interfaces show each EPC section info
> > > in one numa node. Libvirt can use QMP interface to get the
> > > detailed host SGX EPC capabilities to decide how to allocate
> > > host EPC sections to guest.
> > 
> > 
> > 
> > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json
> > > index 5aa2b95b7d..1022aa0184 100644
> > > --- a/qapi/misc-target.json
> > > +++ b/qapi/misc-target.json
> > > @@ -337,6 +337,21 @@
> > >    'if': 'TARGET_ARM' }
> > >  
> > >  
> > > +##
> > > +# @SGXEPCSection:
> > > +#
> > > +# Information about intel SGX EPC section info
> > > +#
> > > +# @node: the numa node
> > > +#
> > > +# @size: the size of epc section
> > > +#
> > > +# Since: 6.2
> > 
> > This is wrong because it was merged for 7.0 not 6.2
> > 
> > > +##
> > > +{ 'struct': 'SGXEPCSection',
> > > +  'data': { 'node': 'int',
> > > +            'size': 'uint64'}}
> > > +
> > >  ##
> > >  # @SGXInfo:
> > >  #
> > > @@ -350,7 +365,7 @@
> > >  #
> > >  # @flc: true if FLC is supported
> > >  #
> > > -# @section-size: The EPC section size for guest
> > > +# @sections: The EPC sections info for guest
> > 
> > This is a non-backwards compatible schema change.
> > 
> > "@section-size" must not be removed without going
> > through a deprecation period, so this needs to be
> > re-instated.
> > 
> > The "@sections" addition needs a "Since 7.0" annotation too.
> > 
> > 
> > Yong, can you submit a followup patch to correct these mistakes
> > 
> 
>   Thanks, I will submit one patch to fix this version issue. This series
>   support SGX NUMA, the background is SGX EPC section number is not fixed(<=8)
>   I added this "@sections" to include numa node and EPC section size, which can
>   be shown how to allocate EPC sections to different NUMA nodes in the VM.
>   The older "@section-size" is only suitable for one EPC section on one NUMA node
>   in one VM, so I moved this size into "@sections" here for NUMA support.

I understand the motivation for introducing '@sections' for NUMA and have
no objection to doing that.

The problem is that you also removed "@section-size" and this is an
incompatible API breakage. The QMP command schema promises backwards
compatibility as standard. If any field needs to be removed it is
*mandatory* to mark the field as deprecated for a minimum of two
QEMU releases, beforef it is permitted to remove it.

IOW, we need to have *both*  @sections and @section-size reported
for at least QEMU 7.0.0 and 7.1.0 releases. If you deprecate @section-size
in 7.0.0, the soonest you can delete it is in the 7.2.0 release.

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 :|