| Message ID | 20190411175931.29235-1-brijesh.singh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386: sev: add 'sev-max-guests' field to 'query-sev-capabilities' | expand |
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
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); >
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
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
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. >
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. >> >
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
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
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
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
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);
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(-)