| Message ID | 20180129174132.108925-14-brijesh.singh@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Secure Encrypted Virtualization (AMD) | expand |
Brijesh Singh <brijesh.singh@amd.com> writes: > update 'info kvm' to display the memory encryption support. > > (qemu) info kvm > kvm support: enabled > memory encryption: disabled > > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > hmp.c | 2 ++ > qapi-schema.json | 5 ++++- > qmp.c | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hmp.c b/hmp.c > index 056bf70cf1e2..6ceb6b30af75 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -88,6 +88,8 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "kvm support: "); > if (info->present) { > monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled"); > + monitor_printf(mon, "memory encryption: %s\n", > + info->mem_encryption ? "enabled" : "disabled"); > } else { > monitor_printf(mon, "not compiled\n"); > } > diff --git a/qapi-schema.json b/qapi-schema.json > index 5c06745c7927..2046c96669bf 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -314,9 +314,12 @@ > # > # @present: true if KVM acceleration is built into this executable > # > +# @mem-encryption: true if Memory Encryption is active (since 2.12) > +# > # Since: 0.14.0 > ## > -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} } > +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool', > + 'mem-encryption' : 'bool'} } This extends QMP's query-kvm. Your commit message claims the patch affects only HMP. Needs fixing. The cleanest way is to split the patch into its QMP part (hunks #2 and #3) and its HMP part (hunk #1). > > ## > # @query-kvm: > diff --git a/qmp.c b/qmp.c > index 52cfd2d81c0f..3a527bc8c39c 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -69,6 +69,7 @@ KvmInfo *qmp_query_kvm(Error **errp) > > info->enabled = kvm_enabled(); > info->present = kvm_available(); > + info->mem_encryption = kvm_memcrypt_enabled(); > > return info; > }
On 01/31/2018 11:43 AM, Markus Armbruster wrote: > Brijesh Singh <brijesh.singh@amd.com> writes: > >> update 'info kvm' to display the memory encryption support. >> >> (qemu) info kvm >> kvm support: enabled >> memory encryption: disabled >> >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> hmp.c | 2 ++ >> qapi-schema.json | 5 ++++- >> qmp.c | 1 + >> 3 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hmp.c b/hmp.c >> index 056bf70cf1e2..6ceb6b30af75 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -88,6 +88,8 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict) >> monitor_printf(mon, "kvm support: "); >> if (info->present) { >> monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled"); >> + monitor_printf(mon, "memory encryption: %s\n", >> + info->mem_encryption ? "enabled" : "disabled"); >> } else { >> monitor_printf(mon, "not compiled\n"); >> } >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 5c06745c7927..2046c96669bf 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -314,9 +314,12 @@ >> # >> # @present: true if KVM acceleration is built into this executable >> # >> +# @mem-encryption: true if Memory Encryption is active (since 2.12) >> +# >> # Since: 0.14.0 >> ## >> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} } >> +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool', >> + 'mem-encryption' : 'bool'} } > > This extends QMP's query-kvm. Your commit message claims the patch > affects only HMP. Needs fixing. The cleanest way is to split the patch > into its QMP part (hunks #2 and #3) and its HMP part (hunk #1). > Noted, I will break this into two patches in next series. Thanks for feedback.
* Brijesh Singh (brijesh.singh@amd.com) wrote: > update 'info kvm' to display the memory encryption support. > > (qemu) info kvm > kvm support: enabled > memory encryption: disabled As Markus said, this should be split qmp/hmp; but something else to think about is whether this is a boolean or needs to be an enum; do you have one version of encryption or are we going to need to flag up versions or the features of the encryption? Dave > Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > hmp.c | 2 ++ > qapi-schema.json | 5 ++++- > qmp.c | 1 + > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hmp.c b/hmp.c > index 056bf70cf1e2..6ceb6b30af75 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -88,6 +88,8 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict) > monitor_printf(mon, "kvm support: "); > if (info->present) { > monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled"); > + monitor_printf(mon, "memory encryption: %s\n", > + info->mem_encryption ? "enabled" : "disabled"); > } else { > monitor_printf(mon, "not compiled\n"); > } > diff --git a/qapi-schema.json b/qapi-schema.json > index 5c06745c7927..2046c96669bf 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -314,9 +314,12 @@ > # > # @present: true if KVM acceleration is built into this executable > # > +# @mem-encryption: true if Memory Encryption is active (since 2.12) > +# > # Since: 0.14.0 > ## > -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} } > +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool', > + 'mem-encryption' : 'bool'} } > > ## > # @query-kvm: > diff --git a/qmp.c b/qmp.c > index 52cfd2d81c0f..3a527bc8c39c 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -69,6 +69,7 @@ KvmInfo *qmp_query_kvm(Error **errp) > > info->enabled = kvm_enabled(); > info->present = kvm_available(); > + info->mem_encryption = kvm_memcrypt_enabled(); > > return info; > } > -- > 2.9.5 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote: > * Brijesh Singh (brijesh.singh@amd.com) wrote: >> update 'info kvm' to display the memory encryption support. >> >> (qemu) info kvm >> kvm support: enabled >> memory encryption: disabled > As Markus said, this should be split qmp/hmp; but something else to > think about is whether this is a boolean or needs to be an enum; do > you have one version of encryption or are we going to need to flag up > versions or the features of the encryption? In future I could see us providing encrypted state status when we implement SEV-ES support, something like (qemu) info kvm kvm support: enabled memory encryption: enabled cpu register state: encrypted but so far I do not see need to provide the version string. If user wants to know the SEV version then it can open /dev/sev device to get platform status and more. > Dave > >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> hmp.c | 2 ++ >> qapi-schema.json | 5 ++++- >> qmp.c | 1 + >> 3 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hmp.c b/hmp.c >> index 056bf70cf1e2..6ceb6b30af75 100644 >> --- a/hmp.c >> +++ b/hmp.c >> @@ -88,6 +88,8 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict) >> monitor_printf(mon, "kvm support: "); >> if (info->present) { >> monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled"); >> + monitor_printf(mon, "memory encryption: %s\n", >> + info->mem_encryption ? "enabled" : "disabled"); >> } else { >> monitor_printf(mon, "not compiled\n"); >> } >> diff --git a/qapi-schema.json b/qapi-schema.json >> index 5c06745c7927..2046c96669bf 100644 >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -314,9 +314,12 @@ >> # >> # @present: true if KVM acceleration is built into this executable >> # >> +# @mem-encryption: true if Memory Encryption is active (since 2.12) >> +# >> # Since: 0.14.0 >> ## >> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} } >> +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool', >> + 'mem-encryption' : 'bool'} } >> >> ## >> # @query-kvm: >> diff --git a/qmp.c b/qmp.c >> index 52cfd2d81c0f..3a527bc8c39c 100644 >> --- a/qmp.c >> +++ b/qmp.c >> @@ -69,6 +69,7 @@ KvmInfo *qmp_query_kvm(Error **errp) >> >> info->enabled = kvm_enabled(); >> info->present = kvm_available(); >> + info->mem_encryption = kvm_memcrypt_enabled(); >> >> return info; >> } >> -- >> 2.9.5 >> > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
* Brijesh Singh (brijesh.singh@amd.com) wrote: > > > On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote: > > * Brijesh Singh (brijesh.singh@amd.com) wrote: > >> update 'info kvm' to display the memory encryption support. > >> > >> (qemu) info kvm > >> kvm support: enabled > >> memory encryption: disabled > > As Markus said, this should be split qmp/hmp; but something else to > > think about is whether this is a boolean or needs to be an enum; do > > you have one version of encryption or are we going to need to flag up > > versions or the features of the encryption? > > In future I could see us providing encrypted state status when we > implement SEV-ES support, something like > > (qemu) info kvm > kvm support: enabled > memory encryption: enabled > cpu register state: encrypted > > but so far I do not see need to provide the version string. If user > wants to know the SEV version then it can open /dev/sev device to get > platform status and more. Yes, I was worried a bit more about how general that was going to be or whether we're collecting a lot of architecture specific fields here. So I wondered, if it was an enum, whether that would be come: memory encryption: none memory encryption: SEV memory encryption: SEV-ES (I'm not too sure whether that's better or not, just a suggestion) Dave > > > Dave > > > >> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > >> Cc: Eric Blake <eblake@redhat.com> > >> Cc: Markus Armbruster <armbru@redhat.com> > >> Cc: Paolo Bonzini <pbonzini@redhat.com> > >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > >> --- > >> hmp.c | 2 ++ > >> qapi-schema.json | 5 ++++- > >> qmp.c | 1 + > >> 3 files changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/hmp.c b/hmp.c > >> index 056bf70cf1e2..6ceb6b30af75 100644 > >> --- a/hmp.c > >> +++ b/hmp.c > >> @@ -88,6 +88,8 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict) > >> monitor_printf(mon, "kvm support: "); > >> if (info->present) { > >> monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled"); > >> + monitor_printf(mon, "memory encryption: %s\n", > >> + info->mem_encryption ? "enabled" : "disabled"); > >> } else { > >> monitor_printf(mon, "not compiled\n"); > >> } > >> diff --git a/qapi-schema.json b/qapi-schema.json > >> index 5c06745c7927..2046c96669bf 100644 > >> --- a/qapi-schema.json > >> +++ b/qapi-schema.json > >> @@ -314,9 +314,12 @@ > >> # > >> # @present: true if KVM acceleration is built into this executable > >> # > >> +# @mem-encryption: true if Memory Encryption is active (since 2.12) > >> +# > >> # Since: 0.14.0 > >> ## > >> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} } > >> +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool', > >> + 'mem-encryption' : 'bool'} } > >> > >> ## > >> # @query-kvm: > >> diff --git a/qmp.c b/qmp.c > >> index 52cfd2d81c0f..3a527bc8c39c 100644 > >> --- a/qmp.c > >> +++ b/qmp.c > >> @@ -69,6 +69,7 @@ KvmInfo *qmp_query_kvm(Error **errp) > >> > >> info->enabled = kvm_enabled(); > >> info->present = kvm_available(); > >> + info->mem_encryption = kvm_memcrypt_enabled(); > >> > >> return info; > >> } > >> -- > >> 2.9.5 > >> > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, Feb 01, 2018 at 08:04:43PM +0000, Dr. David Alan Gilbert wrote: > * Brijesh Singh (brijesh.singh@amd.com) wrote: > > > > > > On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote: > > > * Brijesh Singh (brijesh.singh@amd.com) wrote: > > >> update 'info kvm' to display the memory encryption support. > > >> > > >> (qemu) info kvm > > >> kvm support: enabled > > >> memory encryption: disabled > > > As Markus said, this should be split qmp/hmp; but something else to > > > think about is whether this is a boolean or needs to be an enum; do > > > you have one version of encryption or are we going to need to flag up > > > versions or the features of the encryption? > > > > In future I could see us providing encrypted state status when we > > implement SEV-ES support, something like > > > > (qemu) info kvm > > kvm support: enabled > > memory encryption: enabled > > cpu register state: encrypted > > > > but so far I do not see need to provide the version string. If user > > wants to know the SEV version then it can open /dev/sev device to get > > platform status and more. > > Yes, I was worried a bit more about how general that was going to be > or whether we're collecting a lot of architecture specific fields here. > So I wondered, if it was an enum, whether that would be come: > > memory encryption: none > > memory encryption: SEV > > memory encryption: SEV-ES > > (I'm not too sure whether that's better or not, just a suggestion) I wonder if it is is even appropriate to have under 'info kvm', since 'info kvm' is architecture independant and SEV is specific to AMD x86_64 only. It might suggest an 'info sev' command is better ? Regards, Daniel
On 2/2/18 7:08 AM, Daniel P. Berrangé wrote: > On Thu, Feb 01, 2018 at 08:04:43PM +0000, Dr. David Alan Gilbert wrote: >> * Brijesh Singh (brijesh.singh@amd.com) wrote: >>> >>> On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote: >>>> * Brijesh Singh (brijesh.singh@amd.com) wrote: >>>>> update 'info kvm' to display the memory encryption support. >>>>> >>>>> (qemu) info kvm >>>>> kvm support: enabled >>>>> memory encryption: disabled >>>> As Markus said, this should be split qmp/hmp; but something else to >>>> think about is whether this is a boolean or needs to be an enum; do >>>> you have one version of encryption or are we going to need to flag up >>>> versions or the features of the encryption? >>> In future I could see us providing encrypted state status when we >>> implement SEV-ES support, something like >>> >>> (qemu) info kvm >>> kvm support: enabled >>> memory encryption: enabled >>> cpu register state: encrypted >>> >>> but so far I do not see need to provide the version string. If user >>> wants to know the SEV version then it can open /dev/sev device to get >>> platform status and more. >> Yes, I was worried a bit more about how general that was going to be >> or whether we're collecting a lot of architecture specific fields here. >> So I wondered, if it was an enum, whether that would be come: >> >> memory encryption: none >> >> memory encryption: SEV >> >> memory encryption: SEV-ES >> >> (I'm not too sure whether that's better or not, just a suggestion) > I wonder if it is is even appropriate to have under 'info kvm', since > 'info kvm' is architecture independant and SEV is specific to AMD x86_64 > only. It might suggest an 'info sev' command is better ? The reason I kept under 'info kvm' is because now KVM has a ioctl for memory encryption operation, I like your suggestion for introducing 'info sev' -- the command can be used to provide additional SEV specific details (e.g SEV FW state, SEV FW version, SEV active policy etc). > > Regards, > Daniel
* Brijesh Singh (brijesh.singh@amd.com) wrote: > > > On 2/2/18 7:08 AM, Daniel P. Berrangé wrote: > > On Thu, Feb 01, 2018 at 08:04:43PM +0000, Dr. David Alan Gilbert wrote: > >> * Brijesh Singh (brijesh.singh@amd.com) wrote: > >>> > >>> On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote: > >>>> * Brijesh Singh (brijesh.singh@amd.com) wrote: > >>>>> update 'info kvm' to display the memory encryption support. > >>>>> > >>>>> (qemu) info kvm > >>>>> kvm support: enabled > >>>>> memory encryption: disabled > >>>> As Markus said, this should be split qmp/hmp; but something else to > >>>> think about is whether this is a boolean or needs to be an enum; do > >>>> you have one version of encryption or are we going to need to flag up > >>>> versions or the features of the encryption? > >>> In future I could see us providing encrypted state status when we > >>> implement SEV-ES support, something like > >>> > >>> (qemu) info kvm > >>> kvm support: enabled > >>> memory encryption: enabled > >>> cpu register state: encrypted > >>> > >>> but so far I do not see need to provide the version string. If user > >>> wants to know the SEV version then it can open /dev/sev device to get > >>> platform status and more. > >> Yes, I was worried a bit more about how general that was going to be > >> or whether we're collecting a lot of architecture specific fields here. > >> So I wondered, if it was an enum, whether that would be come: > >> > >> memory encryption: none > >> > >> memory encryption: SEV > >> > >> memory encryption: SEV-ES > >> > >> (I'm not too sure whether that's better or not, just a suggestion) > > I wonder if it is is even appropriate to have under 'info kvm', since > > 'info kvm' is architecture independant and SEV is specific to AMD x86_64 > > only. It might suggest an 'info sev' command is better ? > > The reason I kept under 'info kvm' is because now KVM has a ioctl for > memory encryption operation, I like your suggestion for introducing > 'info sev' -- the command can be used to provide additional SEV specific > details (e.g SEV FW state, SEV FW version, SEV active policy etc). Yes, that would be useful - I'm sure there's lots of information that will be useful to display for understanding the state of SEV, e.g. the policies etc. Dave > > > > Regards, > > Daniel > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/hmp.c b/hmp.c index 056bf70cf1e2..6ceb6b30af75 100644 --- a/hmp.c +++ b/hmp.c @@ -88,6 +88,8 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict) monitor_printf(mon, "kvm support: "); if (info->present) { monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled"); + monitor_printf(mon, "memory encryption: %s\n", + info->mem_encryption ? "enabled" : "disabled"); } else { monitor_printf(mon, "not compiled\n"); } diff --git a/qapi-schema.json b/qapi-schema.json index 5c06745c7927..2046c96669bf 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -314,9 +314,12 @@ # # @present: true if KVM acceleration is built into this executable # +# @mem-encryption: true if Memory Encryption is active (since 2.12) +# # Since: 0.14.0 ## -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} } +{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool', + 'mem-encryption' : 'bool'} } ## # @query-kvm: diff --git a/qmp.c b/qmp.c index 52cfd2d81c0f..3a527bc8c39c 100644 --- a/qmp.c +++ b/qmp.c @@ -69,6 +69,7 @@ KvmInfo *qmp_query_kvm(Error **errp) info->enabled = kvm_enabled(); info->present = kvm_available(); + info->mem_encryption = kvm_memcrypt_enabled(); return info; }
update 'info kvm' to display the memory encryption support. (qemu) info kvm kvm support: enabled memory encryption: disabled Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com> Cc: Eric Blake <eblake@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- hmp.c | 2 ++ qapi-schema.json | 5 ++++- qmp.c | 1 + 3 files changed, 7 insertions(+), 1 deletion(-)