Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'

[Erik Skultety <eskultet@redhat.com>]

On Fri, Apr 12, 2019 at 09:45:02AM +0200, Paolo Bonzini wrote:
> On 11/04/19 21:02, Singh, Brijesh wrote:
> >
> >
> > On 4/11/19 1:10 PM, Laszlo Ersek wrote:
> >> On 04/11/19 19:59, Singh, Brijesh wrote:
> >>> There are limited numbers of the SEV guests that can be run concurrently.
> >>> A management applications may need to know this limit so that it can place
> >>> SEV VMs on hosts which have suitable resources available.
> >>>
> >>> Currently, this limit is not exposed to the application. Add a new
> >>> 'sev-max-guest' field in the query-sev-capabilities to provide this
> >>> information.
> >>>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Markus Armbruster <armbru@redhat.com>
> >>> Cc: Eric Blake <eblake@redhat.com>
> >>> Cc: Daniel P. Berrangé <berrange@redhat.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Cc: Erik Skultety <eskultet@redhat.com>
> >>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> >>> ---
> >>>   qapi/target.json  | 6 ++++--
> >>>   target/i386/sev.c | 6 ++++--
> >>>   2 files changed, 8 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/qapi/target.json b/qapi/target.json
> >>> index 1d4d54b600..b45121d30b 100644
> >>> --- a/qapi/target.json
> >>> +++ b/qapi/target.json
> >>> @@ -183,7 +183,8 @@
> >>>     'data': { 'pdh': 'str',
> >>>               'cert-chain': 'str',
> >>>               'cbitpos': 'int',
> >>> -            'reduced-phys-bits': 'int'},
> >>> +            'reduced-phys-bits': 'int',
> >>> +            'sev-max-guests': 'int'},
> >>
> >> Would it be useful to make this new field optional? E.g. if it was
> >> missing, libvirtd could assume "no limit".
> >>
> >
> > I am not sure if we need to make this field optional - mainly because
> > in SEV context hardware will always have some limits (at least in
> > foreseeable future). The architecture provides us a CPUID to query
> > this capabilities so I am assuming that future CPUs will populate
> > some values in it.
>
> Since this field is not specific to guest configuration, I don't think
> it belongs in query-sev-capabilities; QEMU does not care about >1 guest.

Neither pdh nor cert-chain are specific to the guest config. I see why this
should be better suited for query-sev, the same goes for libvirt - I think we
shouldn't have gone with reporting the SEV platform caps in domain capabilities,
we should have IMHO report it both in the host capabilities (platform specific
stuff) and in domain capabilities to indicate that both libvirt and QEMU
support the SEV feature. Having said that, we have a precedent which I think
we might be better off with following rather than splitting the information
among multiple commands.

Regards,
Erik