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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? 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.

Of course, with the VMSAVE gone from svm_ctxt_switch_to() the
only one left is in _svm_cpu_up(), i.e. long before the first
VM exit, but then the same consideration could apply the other
way around (VM exit writing full cache lines, assuming the
other parts of the HSA are unused).

And then of course, if in fact this isn't spelled out anywhere,
there's the forward compatibility question.

> --- 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. This would in particular mean
not starting with (only) "Host State Area" (unless here you really
mean to use a term different from the PM, to express the superset,
but then one less easy to mix up with Host Save Area would likely
be better), and not solely referring to non-root mode. Albeit
maybe you mean root mode by saying "contains Xen's"? If so,
perhaps "..., and also contains"?

Jan