Cc: brijesh.singh@amd.com, linux-kernel@vger.kernel.org, x86@kernel.org,
        kvm@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Joerg Roedel <joro@8bytes.org>, "Michael S . Tsirkin" <mst@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [RFC Part2 PATCH v3 01/26] Documentation/virtual/kvm: Add AMD
 Secure Encrypted Virtualization (SEV)
To: Borislav Petkov <bp@suse.de>
References: <20170724200303.12197-1-brijesh.singh@amd.com>
 <20170724200303.12197-2-brijesh.singh@amd.com>
 <20170905172130.24fgl6xsrfovsbsp@pd.tnic>
From: Brijesh Singh <brijesh.singh@amd.com>
Message-ID: <ebc5e242-6ef5-f4bd-87db-a1ae551d9945@amd.com>
Date: Tue, 5 Sep 2017 16:39:14 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <20170905172130.24fgl6xsrfovsbsp@pd.tnic>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Sep 2017 21:39:19.3805 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR12MB0146
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4163
Lines: 142

Hi Boris,

Thanks for detail review, I have incorporate the spell check
in my work flow and will be fixing all those spell check errors
innext rev.


On 09/05/2017 12:21 PM, Borislav Petkov wrote:

[...]


>> +3. KVM_SEV_LAUNCH_MEASURE
>> +
>> +Parameters (in): struct  kvm_sev_launch_measure
>> +Returns: 0 on success, -negative on error
>> +
>> +LAUNCH_MEASURE returns the measurement of the memory region encrypted with
>> +LAUNCH_UPDATE_DATA. The measurement is keyed with the TIK so that the guest
>> +owner can use the measurement to verify the guest was properly launched without
>> +tempering.
> 
> So this could use a bit more text as it is such an important aspect of
> the whole verification of the guest.
> 
>> +
>> +struct kvm_sev_launch_measure {
>> +	/* where to copy the measurement blob */
>> +	__u64 address;
>> +
>> +	/* length of memory region containing measurement */
>> +	__u32 length;
>> +};
>> +
>> +If measurement length is too small, the required length is returned in the
>> +length field.
>> +
>> +On success, the measurement is copied to the address.
> 
> And how is success signalled to the caller?
> 

The measurement verification is performed outside the KVM/Qemu.

 From driver point of view, all we have to do is issues LAUNCH_MEASURE
command when userspace asks for the measurement. I can see that command
name is confusing - I am thinking of renaming it to
"KVM_SEV_GET_LAUNCH_MEASUREMENT"

The complete flow is listed in Appendix A of SEV firmware spec [1].

I will update the doc to give SEV spec section references for the details.

Not sure if we need to document the complete measurement flow in the
driver doc.

[...]

>> +
>> +4. KVM_SEV_LAUNCH_FINISH
>> +
>> +Returns: 0 on success, -negative on error
>> +
>> +LAUNCH_FINISH command finalize the SEV guest launch process.
> 
> "The KVM_SEV_LAUNCH_FINISH command..."
> 
>> +
>> +5. KVM_SEV_GUEST_STATUS
>> +
>> +Parameters (out): struct kvm_sev_guest_status
> 
> This is an "out" command, so it should be called
> KVM_SEV_GET_GUEST_STATUS. Or is it too late for that?


I was trying map with SEV firmware spec command names but I see your
point and will call it "KVM_SEV_GET_GUEST_STATUS".


>> +
>> +enum {
>> +	/* guest state is not known */
>> +	SEV_STATE_INVALID = 0;
> 
> not known or invalid?


Again, was trying to follow the spec naming convention but I can go
with UNKNOWN ..

> 
> Btw, side-comments will make this much more readable:
> 
> enum {
>          SEV_STATE_INVALID = 0,
>          SEV_STATE_LAUNCHING,
>          SEV_STATE_SECRET,       /* guest is being launched and ready to accept the ciphertext data */
>          SEV_STATE_RUNNING,      /* guest is fully launched and running */
>          SEV_STATE_RECEIVING,    /* guest is being migrated in from another SEV machine */
>          SEV_STATE_SENDING,      /* guest is getting migrated out to another SEV machine */
> };
> 


I was trying to keep everything to 80 column limit but if that is
not an issue for documentation then I like your recommendation.


[...]

>> +8. KVM_SEV_SEND_START
>> +
>> +Parameters (in): struct kvm_sev_send_start
>> +Returns: 0 on success, -negative on error
>> +
>> +SEND_START command is used to export a SEV guest from one platform to another.
> 
> Export or migrate?
> 
>> +It can be used for saving a guest to disk to be resumed later, or it can be
>> +used to migrate a guest across the network to a receiving platform.
> 
> And how do I specify which of those actions needs to happen?
> 

The command does not require explicit parameter to differentiate between
live migration vs snapshot. All it needs is a destination platform
PDH key. If its live migration case then VM management stack will probably
communicate with remote platform and get its PDH keys before calling us.
The KVM driver simply acts upon the request from the userspace. SEV firmware
spec Appendix A [1] provides complete flow diagram which need to be implemented
in userspace. The driver simply act upon when it asked to create SEND_START
context.
  
[1] http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf

> 
> Phew, that took long.
> 

Thank you for detail review.