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

From: Brijesh Singh <brijesh.singh@amd.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>,
	Peter Maydell <peter.maydell@linaro.org>,
	brijesh.singh@amd.com, Eduardo Habkost <ehabkost@redhat.com>,
	kvm@vger.kernel.org, Tom Lendacky <Thomas.Lendacky@amd.com>,
	Stefan Hajnoczi <stefanha@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, Markus Armbruster <armbru@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v6 13/23] hmp: display memory encryption support in 'info kvm'
Date: Fri, 2 Feb 2018 07:46:44 -0600	[thread overview]
Message-ID: <3e6cf550-9bd8-daa7-e54a-d390354ffcb0@amd.com> (raw)
In-Reply-To: <20180202130817.GE15403@redhat.com>

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