| Message ID | 20210520145647.3483809-1-david.edmondson@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support protection keys in an AMD EPYC-Milan VM | expand |
Patchew URL: https://patchew.org/QEMU/20210520145647.3483809-1-david.edmondson@oracle.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20210520145647.3483809-1-david.edmondson@oracle.com
Subject: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/20210520145647.3483809-1-david.edmondson@oracle.com -> patchew/20210520145647.3483809-1-david.edmondson@oracle.com
Switched to a new branch 'test'
9761ad4 target/i386: Manipulate only AMD XSAVE state on AMD
fcba7d5 target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd
0493da2 target/i386: Introduce AMD X86XSaveArea sub-union
e8d400c target/i386: Prepare for per-vendor X86XSaveArea layout
5a78b06 target/i386: Clarify the padding requirements of X86XSaveArea
ba3c3af target/i386: Use constants for XSAVE offsets
844afa9 target/i386: Declare constants for XSAVE offsets
=== OUTPUT BEGIN ===
1/7 Checking commit 844afa9929e3 (target/i386: Declare constants for XSAVE offsets)
WARNING: line over 80 characters
#60: FILE: target/i386/cpu.h:1352:
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, zmm_hi256_state) != XSAVE_ZMM_HI256_OFFSET);
WARNING: line over 80 characters
#63: FILE: target/i386/cpu.h:1354:
+QEMU_BUILD_BUG_ON(offsetof(X86XSaveArea, hi16_zmm_state) != XSAVE_HI16_ZMM_OFFSET);
total: 0 errors, 2 warnings, 48 lines checked
Patch 1/7 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/7 Checking commit ba3c3afe9795 (target/i386: Use constants for XSAVE offsets)
3/7 Checking commit 5a78b069f6d4 (target/i386: Clarify the padding requirements of X86XSaveArea)
4/7 Checking commit e8d400c3e40e (target/i386: Prepare for per-vendor X86XSaveArea layout)
5/7 Checking commit 0493da29bcd9 (target/i386: Introduce AMD X86XSaveArea sub-union)
6/7 Checking commit fcba7d58090d (target/i386: Adjust AMD XSAVE PKRU area offset in CPUID leaf 0xd)
7/7 Checking commit 9761ad41cd68 (target/i386: Manipulate only AMD XSAVE state on AMD)
ERROR: spaces required around that '+' (ctx:VxV)
#90: FILE: target/i386/xsave_helper.c:53:
+ stq_p(zmmh+8, env->xmm_regs[i].ZMM_Q(5));
^
ERROR: spaces required around that '+' (ctx:VxV)
#91: FILE: target/i386/xsave_helper.c:54:
+ stq_p(zmmh+16, env->xmm_regs[i].ZMM_Q(6));
^
ERROR: spaces required around that '+' (ctx:VxV)
#92: FILE: target/i386/xsave_helper.c:55:
+ stq_p(zmmh+24, env->xmm_regs[i].ZMM_Q(7));
^
ERROR: spaces required around that '+' (ctx:VxV)
#150: FILE: target/i386/xsave_helper.c:114:
+ env->xmm_regs[i].ZMM_Q(5) = ldq_p(zmmh+8);
^
ERROR: spaces required around that '+' (ctx:VxV)
#151: FILE: target/i386/xsave_helper.c:115:
+ env->xmm_regs[i].ZMM_Q(6) = ldq_p(zmmh+16);
^
ERROR: spaces required around that '+' (ctx:VxV)
#152: FILE: target/i386/xsave_helper.c:116:
+ env->xmm_regs[i].ZMM_Q(7) = ldq_p(zmmh+24);
^
total: 6 errors, 0 warnings, 140 lines checked
Patch 7/7 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20210520145647.3483809-1-david.edmondson@oracle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thursday, 2021-05-20 at 15:56:40 +01, 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. > > 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. Ping - any thoughts about this approach? > 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(-) > > -- > 2.30.2 dme.
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(-) >
On Friday, 2021-06-11 at 18:01:55 +02, Paolo Bonzini wrote: > 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? Yes. > 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? I will produce a patch and send it out. > 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(-) >> dme.
David, Are you still working on v2 of these series? I was going to test and review. Thanks > -----Original Message----- > From: David Edmondson <dme@dme.org> > Sent: Tuesday, June 8, 2021 3:25 AM > To: qemu-devel@nongnu.org > Cc: kvm@vger.kernel.org; Eduardo Habkost <ehabkost@redhat.com>; Paolo > Bonzini <pbonzini@redhat.com>; Marcelo Tosatti <mtosatti@redhat.com>; > Richard Henderson <richard.henderson@linaro.org>; Moger, Babu > <Babu.Moger@amd.com> > Subject: Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan > VM > > On Thursday, 2021-05-20 at 15:56:40 +01, 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. > > > > 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. > > Ping - any thoughts about this approach? > > > 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(-) > > > > -- > > 2.30.2 > > dme. > -- > You know your green from your red.
On Thursday, 2021-07-01 at 16:24:51 -05, Babu Moger wrote: > David, Are you still working on v2 of these series? I was going to test > and review. Thanks Yes. I have something that works, but it's messy in places. I hope to get it out in a couple of days. >> -----Original Message----- >> From: David Edmondson <dme@dme.org> >> Sent: Tuesday, June 8, 2021 3:25 AM >> To: qemu-devel@nongnu.org >> Cc: kvm@vger.kernel.org; Eduardo Habkost <ehabkost@redhat.com>; Paolo >> Bonzini <pbonzini@redhat.com>; Marcelo Tosatti <mtosatti@redhat.com>; >> Richard Henderson <richard.henderson@linaro.org>; Moger, Babu >> <Babu.Moger@amd.com> >> Subject: Re: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan >> VM >> >> On Thursday, 2021-05-20 at 15:56:40 +01, 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. >> > >> > 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. >> >> Ping - any thoughts about this approach? >> >> > 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(-) >> > >> > -- >> > 2.30.2 >> >> dme. >> -- >> You know your green from your red. dme.