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

Singh, Brijesh posted 1 patch 5 years ago
Failed in applying to current master (apply log)
There is a newer version of this series
qapi/target.json  | 6 ++++--
target/i386/sev.c | 6 ++++--
2 files changed, 8 insertions(+), 4 deletions(-)
[Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Singh, Brijesh 5 years ago
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'},
   'if': 'defined(TARGET_I386)' }
 
 ##
@@ -200,7 +201,8 @@
 #
 # -> { "execute": "query-sev-capabilities" }
 # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
-#                  "cbitpos": 47, "reduced-phys-bits": 5}}
+#                  "cbitpos": 47, "reduced-phys-bits": 5,
+#                  "sev-max-guests" : 15}}
 #
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
diff --git a/target/i386/sev.c b/target/i386/sev.c
index cd77f6b5d4..bb0cd79acd 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -488,7 +488,7 @@ sev_get_capabilities(void)
     guchar *pdh_data = NULL;
     guchar *cert_chain_data = NULL;
     size_t pdh_len = 0, cert_chain_len = 0;
-    uint32_t ebx;
+    uint32_t ebx, ecx, edx;
     int fd;
 
     fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
@@ -507,7 +507,7 @@ sev_get_capabilities(void)
     cap->pdh = g_base64_encode(pdh_data, pdh_len);
     cap->cert_chain = g_base64_encode(cert_chain_data, cert_chain_len);
 
-    host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
+    host_cpuid(0x8000001F, 0, NULL, &ebx, &ecx, &edx);
     cap->cbitpos = ebx & 0x3f;
 
     /*
@@ -516,6 +516,8 @@ sev_get_capabilities(void)
      */
     cap->reduced_phys_bits = 1;
 
+    /* the maximum number of SEV guests that can run simultaneously */
+    cap->sev_max_guests = ecx - edx + 1;
 out:
     g_free(pdh_data);
     g_free(cert_chain_data);
-- 
2.17.1

Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Daniel P. Berrangé 5 years ago
On Thu, Apr 11, 2019 at 05:59:50PM +0000, 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 @@

A few lines above here you need to document the new field
with (since 4.1) annotation.

>    'data': { 'pdh': 'str',
>              'cert-chain': 'str',
>              'cbitpos': 'int',
> -            'reduced-phys-bits': 'int'},
> +            'reduced-phys-bits': 'int',
> +            'sev-max-guests': 'int'},
>    'if': 'defined(TARGET_I386)' }
>  
>  ##
> @@ -200,7 +201,8 @@
>  #
>  # -> { "execute": "query-sev-capabilities" }
>  # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
> -#                  "cbitpos": 47, "reduced-phys-bits": 5}}
> +#                  "cbitpos": 47, "reduced-phys-bits": 5,
> +#                  "sev-max-guests" : 15}}
>  #
>  ##
>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index cd77f6b5d4..bb0cd79acd 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -488,7 +488,7 @@ sev_get_capabilities(void)
>      guchar *pdh_data = NULL;
>      guchar *cert_chain_data = NULL;
>      size_t pdh_len = 0, cert_chain_len = 0;
> -    uint32_t ebx;
> +    uint32_t ebx, ecx, edx;
>      int fd;
>  
>      fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
> @@ -507,7 +507,7 @@ sev_get_capabilities(void)
>      cap->pdh = g_base64_encode(pdh_data, pdh_len);
>      cap->cert_chain = g_base64_encode(cert_chain_data, cert_chain_len);
>  
> -    host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
> +    host_cpuid(0x8000001F, 0, NULL, &ebx, &ecx, &edx);
>      cap->cbitpos = ebx & 0x3f;
>  
>      /*
> @@ -516,6 +516,8 @@ sev_get_capabilities(void)
>       */
>      cap->reduced_phys_bits = 1;
>  
> +    /* the maximum number of SEV guests that can run simultaneously */
> +    cap->sev_max_guests = ecx - edx + 1;
>  out:
>      g_free(pdh_data);
>      g_free(cert_chain_data);

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: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Singh, Brijesh 5 years ago

On 4/11/19 1:05 PM, Daniel P. Berrangé wrote:
> On Thu, Apr 11, 2019 at 05:59:50PM +0000, 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 @@
> 
> A few lines above here you need to document the new field
> with (since 4.1) annotation.
> 

noted. thanks
Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Laszlo Ersek 5 years ago
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".

Again, not sure if that's useful, but it's not hard to introduce the
field as optional now. Removing mandatory fields later is impossible.

Thanks
Laszlo

>    'if': 'defined(TARGET_I386)' }
>  
>  ##
> @@ -200,7 +201,8 @@
>  #
>  # -> { "execute": "query-sev-capabilities" }
>  # <- { "return": { "pdh": "8CCDD8DDD", "cert-chain": "888CCCDDDEE",
> -#                  "cbitpos": 47, "reduced-phys-bits": 5}}
> +#                  "cbitpos": 47, "reduced-phys-bits": 5,
> +#                  "sev-max-guests" : 15}}
>  #
>  ##
>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index cd77f6b5d4..bb0cd79acd 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -488,7 +488,7 @@ sev_get_capabilities(void)
>      guchar *pdh_data = NULL;
>      guchar *cert_chain_data = NULL;
>      size_t pdh_len = 0, cert_chain_len = 0;
> -    uint32_t ebx;
> +    uint32_t ebx, ecx, edx;
>      int fd;
>  
>      fd = open(DEFAULT_SEV_DEVICE, O_RDWR);
> @@ -507,7 +507,7 @@ sev_get_capabilities(void)
>      cap->pdh = g_base64_encode(pdh_data, pdh_len);
>      cap->cert_chain = g_base64_encode(cert_chain_data, cert_chain_len);
>  
> -    host_cpuid(0x8000001F, 0, NULL, &ebx, NULL, NULL);
> +    host_cpuid(0x8000001F, 0, NULL, &ebx, &ecx, &edx);
>      cap->cbitpos = ebx & 0x3f;
>  
>      /*
> @@ -516,6 +516,8 @@ sev_get_capabilities(void)
>       */
>      cap->reduced_phys_bits = 1;
>  
> +    /* the maximum number of SEV guests that can run simultaneously */
> +    cap->sev_max_guests = ecx - edx + 1;
>  out:
>      g_free(pdh_data);
>      g_free(cert_chain_data);
> 


Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Laszlo Ersek 5 years ago
On 04/11/19 20:10, 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".
> 
> Again, not sure if that's useful, but it's not hard to introduce the
> field as optional now. Removing mandatory fields later is impossible.

On second thought, if we're sure the hardware / encryption engine will
always have this kind of limitation, then mandatory looks fine.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Singh, Brijesh 5 years ago

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.

> Again, not sure if that's useful, but it's not hard to introduce the
> field as optional now. Removing mandatory fields later is impossible.
> 

Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Paolo Bonzini 5 years ago
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.

Paolo


Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Erik Skultety 5 years ago
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

Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Paolo Bonzini 5 years ago
On 12/04/19 10:19, Erik Skultety wrote:
>> 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.

Sort of, they are required to start a guest, aren't they?  But the
number of guests is irrelevant.

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

For Libvirt, sure.  But I think this doesn't belong in QEMU at all.
Libvirt should just use CPUID.

Paolo

Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Erik Skultety 5 years ago
On Fri, Apr 12, 2019 at 10:26:45AM +0200, Paolo Bonzini wrote:
> On 12/04/19 10:19, Erik Skultety wrote:
> >> 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.
>
> Sort of, they are required to start a guest, aren't they?  But the

Unless you're interested in the measurement, aka attestation, I don't think
those are required in any way.

Erik

> number of guests is irrelevant.
>
> > 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.
>
> For Libvirt, sure.  But I think this doesn't belong in QEMU at all.
> Libvirt should just use CPUID.
>
> Paolo

Re: [Qemu-devel] [PATCH] target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities'
Posted by Laszlo Ersek 5 years ago
On 04/11/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.

Yup, sounds reasonable. Please resubmit with Daniel's request addressed
and I'll be happy to R-b.

Erik: can you please ACK too?

Thanks!
Laszlo

>> Again, not sure if that's useful, but it's not hard to introduce the
>> field as optional now. Removing mandatory fields later is impossible.
>>
>