Re: [PATCH] x86/svm: Merge hsa and host_vmcb to reduce memory overhead

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/10/2020 15:24, Jan Beulich wrote:
> On 26.10.2020 14:50, Andrew Cooper wrote:
>> The format of the Host State Area is, and has always been, a VMCB.  It is
>> explicitly safe to put the host VMSAVE data in.
> Nit: The PM calls this "Host Save Area" or "Host State Save Area"
> afaics.
>
> I recall us discussing this option in the past, and not right
> away pursuing it because of it not having been explicit at the
> time. What place in the doc has made this explicit?

Sadly still not yet, but the pestering has happened.

> The main
> uncertainty (without any explicit statement) on my part would be
> the risk of VMSAVE writing (for performance reasons) e.g. full
> cache lines, i.e. more than exactly the bits holding the state
> to be saved, without first bringing old contents in from memory.

SEV-ES now requires the hypervisor to program desired exit state in in
the VMCB, due to differences in how the VMRUN instruction works.  See
Vol3 15.35.8.  (And yes - this does contradict the earlier statement in
that a the hypervisor must not write directly into the host state area).

I have had it confirmed by AMD that it is safe to use in this fashion,
but if you want more evidence, KVM has had this behaviour on AMD for its
entire lifetime.

>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -72,11 +72,10 @@ static void svm_update_guest_efer(struct vcpu *);
>>  static struct hvm_function_table svm_function_table;
>>  
>>  /*
>> - * Physical addresses of the Host State Area (for hardware) and vmcb (for 
>> Xen)
>> - * which contains Xen's fs/gs/tr/ldtr and GSBASE/STAR/SYSENTER state when in
>> - * guest vcpu context.
>> + * Host State Area.  This area is used by the processor in non-root mode, 
>> and
>> + * contains Xen's fs/gs/tr/ldtr and GSBASE/STAR/SYSENTER state required to
>> + * leave guest vcpu context.
>>   */
>> -static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, hsa);
>>  static DEFINE_PER_CPU_READ_MOSTLY(paddr_t, host_vmcb);
> The comment now applies to host_vmcb, so making the dual purpose
> more obvious would imo be helpful.

But it isn't dual purpose.  It *is* host state, both the half which
VMRUN deals with, and the half which VMLOAD/SAVE deals with (separately,
to optimise VMRUN).

I suppose technically it gets a little complicated with whose state
fs/gs/ldtr/gsbase actually is, given the PV VMLOAD-optimised path, but
the state relevant to Xen's security is tr/syscall/sysenter, which will
remain correct from the start of day.

~Andrew