Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-02 Thread Daniel P . Berrangé 
On Thu, Feb 01, 2018 at 08:04:43PM +, Dr. David Alan Gilbert wrote:
> * Brijesh Singh (brijesh.si...@amd.com) wrote:
> > 
> > 
> > On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote:
> > > * Brijesh Singh (brijesh.si...@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
-- 
|: 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 v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-02 Thread Dr. David Alan Gilbert 
* Brijesh Singh (brijesh.si...@amd.com) wrote:
> 
> 
> On 2/2/18 7:08 AM, Daniel P. Berrangé wrote:
> > On Thu, Feb 01, 2018 at 08:04:43PM +, Dr. David Alan Gilbert wrote:
> >> * Brijesh Singh (brijesh.si...@amd.com) wrote:
> >>>
> >>> On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote:
>  * Brijesh Singh (brijesh.si...@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 / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-02 Thread Brijesh Singh 


On 2/2/18 7:08 AM, Daniel P. Berrangé wrote:
> On Thu, Feb 01, 2018 at 08:04:43PM +, Dr. David Alan Gilbert wrote:
>> * Brijesh Singh (brijesh.si...@amd.com) wrote:
>>>
>>> On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote:
 * Brijesh Singh (brijesh.si...@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

Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-01 Thread Dr. David Alan Gilbert 
* Brijesh Singh (brijesh.si...@amd.com) wrote:
> 
> 
> On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote:
> > * Brijesh Singh (brijesh.si...@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" 
> >> Cc: Eric Blake 
> >> Cc: Markus Armbruster 
> >> Cc: Paolo Bonzini 
> >> Signed-off-by: Brijesh Singh 
> >> ---
> >>  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 / dgilb...@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-01 Thread Brijesh Singh 


On 2/1/18 11:58 AM, Dr. David Alan Gilbert wrote:
> * Brijesh Singh (brijesh.si...@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" 
>> Cc: Eric Blake 
>> Cc: Markus Armbruster 
>> Cc: Paolo Bonzini 
>> Signed-off-by: Brijesh Singh 
>> ---
>>  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 / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-01 Thread Dr. David Alan Gilbert 
* Brijesh Singh (brijesh.si...@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" 
> Cc: Eric Blake 
> Cc: Markus Armbruster 
> Cc: Paolo Bonzini 
> Signed-off-by: Brijesh Singh 
> ---
>  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 / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-02-01 Thread Brijesh Singh 



On 01/31/2018 11:43 AM, Markus Armbruster wrote:

Brijesh Singh  writes:


update 'info kvm' to display the memory encryption support.

(qemu) info kvm
kvm support: enabled
memory encryption: disabled

Cc: "Dr. David Alan Gilbert" 
Cc: Eric Blake 
Cc: Markus Armbruster 
Cc: Paolo Bonzini 
Signed-off-by: Brijesh Singh 
---
  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.

Re: [Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-01-31 Thread Markus Armbruster 
Brijesh Singh  writes:

> update 'info kvm' to display the memory encryption support.
>
> (qemu) info kvm
> kvm support: enabled
> memory encryption: disabled
>
> Cc: "Dr. David Alan Gilbert" 
> Cc: Eric Blake 
> Cc: Markus Armbruster 
> Cc: Paolo Bonzini 
> Signed-off-by: Brijesh Singh 
> ---
>  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;
>  }

[Qemu-devel] [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'

2018-01-29 Thread Brijesh Singh 
update 'info kvm' to display the memory encryption support.

(qemu) info kvm
kvm support: enabled
memory encryption: disabled

Cc: "Dr. David Alan Gilbert" 
Cc: Eric Blake 
Cc: Markus Armbruster 
Cc: Paolo Bonzini 
Signed-off-by: Brijesh Singh 
---
 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