Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM

[Paolo Bonzini <pbonzini@redhat.com>]

First of all, sorry for the delayed review.

On 20/05/21 16:56, David Edmondson wrote:
> AMD EPYC-Milan CPUs introduced support for protection keys, previously
> available only with Intel CPUs.
> 
> AMD chose to place the XSAVE state component for the protection keys
> at a different offset in the XSAVE state area than that chosen by
> Intel.
> 
> To accommodate this, modify QEMU to behave appropriately on AMD
> systems, allowing a VM to properly take advantage of the new feature.

Uff, that sucks. :(

If I understand correctly, the problem is that the layout of 
KVM_GET_XSAVE/KVM_SET_XSAVE depends on the host CPUID, which in 
retrospect would be obvious.  Is that correct?  If so, it would make 
sense and might even be easier to drop all usage of X86XSaveArea:

* update ext_save_areas based on CPUID information in kvm_cpu_instance_init

* make x86_cpu_xsave_all_areas and x86_cpu_xrstor_all_areas use the 
ext_save_areas offsets to build pointers to XSaveAVX, XSaveBNDREG, etc.

What do you think?

Paolo

> Further, avoid manipulating XSAVE state components that are not
> present on AMD systems.
> 
> The code in patch 6 that changes the CPUID 0x0d leaf is mostly dumped
> somewhere that seemed to work - I'm not sure where it really belongs.
> 
> David Edmondson (7):
>    target/i386: Declare constants for XSAVE offsets
>    target/i386: Use constants for XSAVE offsets
>    target/i386: Clarify the padding requirements of X86XSaveArea
>    target/i386: Prepare for per-vendor X86XSaveArea layout
>    target/i386: Introduce AMD X86XSaveArea sub-union
>    target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
>    target/i386: Manipulate only AMD XSAVE state on AMD
> 
>   target/i386/cpu.c            | 19 +++++----
>   target/i386/cpu.h            | 80 ++++++++++++++++++++++++++++--------
>   target/i386/kvm/kvm.c        | 57 +++++++++----------------
>   target/i386/tcg/fpu_helper.c | 20 ++++++---
>   target/i386/xsave_helper.c   | 70 +++++++++++++++++++------------
>   5 files changed, 152 insertions(+), 94 deletions(-)
>