Bug 1972419

Summary: [AMD 9.0 Features] Support protection keys in an AMD EPYC-Milan VM
Product: Red Hat Enterprise Linux 9 Reporter: Terry Bowman (AMD) <tbowman>
Component: qemu-kvmAssignee: Dr. David Alan Gilbert <dgilbert>
qemu-kvm sub component: CPU Models QA Contact: liunana <nanliu>
Status: CLOSED ERRATA Docs Contact:
Severity: unspecified    
Priority: unspecified CC: babu.moger, bhu, ctatman, dgilbert, jinzhao, jon.grimm, juzhang, nilal, qzhang, virt-maint, wehuang
Version: 9.0Keywords: FutureFeature, TestOnly, Triaged
Target Milestone: beta   
Target Release: 9.0   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
: 1988104 (view as bug list) Environment:
Last Closed: 2022-05-17 12:23:27 UTC Type: Feature Request
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1878110, 1988104    

Description Terry Bowman (AMD) 2021-06-15 20:30:10 UTC 
Description of problem:

Subject: [RFC PATCH 0/7] Support protection keys in an AMD EPYC-Milan VM
Date: Thu, 20 May 2021 15:56:40 +0100
Message-ID: <20210520145647.3483809-1-david.edmondson> (raw)

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.

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(-)


Additional info:
Patchset is [RFC] at https://lore.kernel.org/qemu-devel/20210520145647.3483809-1-david.edmondson@oracle.com/
Comment 2 John Ferlan 2021-07-07 18:50:10 UTC 
Assigned to Amnon for initial triage per bz process and age of bug created or assigned to virt-maint without triage.
Comment 3 Dr. David Alan Gilbert 2022-02-10 13:23:51 UTC 
I believe this is already in 9.0 since it uses qemu 6.2 that has this set of patches.
Marking on QA;
Terry: Can you check this on 9.0 please?

(I've been backporting these patches to earlier versions as well, because Milan is hitting crashes even for guests not using protection keys)
Comment 5 liunana 2022-02-23 08:45:56 UTC 
Hi,


Would you please help to set the ITR and provide how to test this feature on QEMU side?
Thanks a lot!


Best regards
Liu Nana
Comment 7 Terry Bowman (AMD) 2022-03-04 18:21:58 UTC 
(In reply to liunana from comment #5)
> Hi,
> 
> 
> Would you please help to set the ITR and provide how to test this feature on
> QEMU side?
> Thanks a lot!
> 
> 
> Best regards
> Liu Nana

Hi Liu,

You can test this with running tools/testing/selftests/vm/protection_keys_64 in the guest. This is how I tested BZ1988104 (this is cloned from).

Regards,
Terry
Comment 8 liunana 2022-03-07 08:52:53 UTC 
(In reply to Terry Bowman from comment #7)
> (In reply to liunana from comment #5)
> > Hi,
> > 
> > 
> > Would you please help to set the ITR and provide how to test this feature on
> > QEMU side?
> > Thanks a lot!
> > 
> > 
> > Best regards
> > Liu Nana
> 
> Hi Liu,
> 
> You can test this with running tools/testing/selftests/vm/protection_keys_64
> in the guest. This is how I tested BZ1988104 (this is cloned from).
> 
Thank you, Terry.

But I met some issue while compling the source file, could you please help to check if the results are expected?

Test Env:
   5.14.0-69.el9.x86_64
  qemu-kvm-6.2.0-10.el9.x86_64


Test Steps:
1. download the source kernel file kernel-5.14.0-69.el9.src.rpm and uncompress it.


2. complie protection_keys.c FAILED at first.
# gcc      -o protection_keys    -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm

protection_keys.c: In function ‘arch_force_pkey_reg_init’:
protection_keys.c:1302:9: warning: implicit declaration of function ‘__builtin_ia32_xsave’; did you mean ‘__builtin_ia32_fxsave’? [-Wimplicit-function-declaration]
 1302 |         __builtin_ia32_xsave(buf, XSTATE_PKEY);
      |         ^~~~~~~~~~~~~~~~~~~~
      |         __builtin_ia32_fxsave
protection_keys.c:1306:9: warning: implicit declaration of function ‘__builtin_ia32_xrstor’; did you mean ‘__builtin_ia32_fxrstor’? [-Wimplicit-function-declaration]
 1306 |         __builtin_ia32_xrstor(buf, XSTATE_PKEY);
      |         ^~~~~~~~~~~~~~~~~~~~~
      |         __builtin_ia32_fxrstor
/usr/bin/ld: /tmp/ccQ11BDn.o: in function `arch_force_pkey_reg_init':
/home/rpm/linux-5.14.0-69.el9/tools/testing/selftests/vm/protection_keys.c:1302: undefined reference to `__builtin_ia32_xsave'
/usr/bin/ld: /home/rpm/linux-5.14.0-69.el9/tools/testing/selftests/vm/protection_keys.c:1306: undefined reference to `__builtin_ia32_xrstor'
collect2: error: ld returned 1 exit status


3.  Disable the Line 1302 and Line 1306

1301         /* XSAVE to build a valid buffer: */
1302         //__builtin_ia32_xsave(buf, XSTATE_PKEY);
1303         /* Clear XSTATE_BV[PKRU]: */
1304         buf[XSTATE_BV_OFFSET/sizeof(u64)] &= ~XSTATE_PKEY;
1305         /* XRSTOR will likely get PKRU back to the init state: */
1306         //__builtin_ia32_xrstor(buf, XSTATE_PKEY);


4. re-complie PASS.


5. execute test file and PASS.
# ./protection_keys
has pkeys: 1
startup pkey_reg: 0000000055555550
test  0 PASSED (iteration 1)
test  1 PASSED (iteration 1)
test  2 PASSED (iteration 1)
......
test 16 PASSED (iteration 22)
test 17 PASSED (iteration 22)
test 18 PASSED (iteration 22)
test 19 PASSED (iteration 22)
done (all tests OK)


Will upload the full test results later.


Besides,  in Documentation/core-api/protection-keys.rst, I only find one flag 'pku' which may can be tested on qemu side, the test steps may only check that this flag can expose to guest.
Could you please also help to check this?

Thanks a lot.



Best regards
Liu Nana
Comment 10 liunana 2022-03-07 08:57:06 UTC 
Hi,

Would you please help to check this is a Testonly bz? If so we can add the Testonly keywords.

If not please help to put the right Fixed Version, Thanks!



Best regards
Liu Nana
Comment 11 Dr. David Alan Gilbert 2022-03-07 11:57:29 UTC 
(In reply to liunana from comment #8)
> (In reply to Terry Bowman from comment #7)
> > (In reply to liunana from comment #5)
> > > Hi,
> > > 
> > > 
> > > Would you please help to set the ITR and provide how to test this feature on
> > > QEMU side?
> > > Thanks a lot!
> > > 
> > > 
> > > Best regards
> > > Liu Nana
> > 
> > Hi Liu,
> > 
> > You can test this with running tools/testing/selftests/vm/protection_keys_64
> > in the guest. This is how I tested BZ1988104 (this is cloned from).
> > 
> Thank you, Terry.
> 
> But I met some issue while compling the source file, could you please help
> to check if the results are expected?
> 
> Test Env:
>    5.14.0-69.el9.x86_64
>   qemu-kvm-6.2.0-10.el9.x86_64
> 
> 
> Test Steps:
> 1. download the source kernel file kernel-5.14.0-69.el9.src.rpm and
> uncompress it.
> 
> 
> 2. complie protection_keys.c FAILED at first.
> # gcc      -o protection_keys    -O2 -g -std=gnu99 -pthread -Wall
> protection_keys.c -lrt -ldl -lm
> 
> protection_keys.c: In function ‘arch_force_pkey_reg_init’:
> protection_keys.c:1302:9: warning: implicit declaration of function
> ‘__builtin_ia32_xsave’; did you mean ‘__builtin_ia32_fxsave’?
> [-Wimplicit-function-declaration]
>  1302 |         __builtin_ia32_xsave(buf, XSTATE_PKEY);
>       |         ^~~~~~~~~~~~~~~~~~~~
>       |         __builtin_ia32_fxsave
> protection_keys.c:1306:9: warning: implicit declaration of function
> ‘__builtin_ia32_xrstor’; did you mean ‘__builtin_ia32_fxrstor’?
> [-Wimplicit-function-declaration]
>  1306 |         __builtin_ia32_xrstor(buf, XSTATE_PKEY);
>       |         ^~~~~~~~~~~~~~~~~~~~~
>       |         __builtin_ia32_fxrstor
> /usr/bin/ld: /tmp/ccQ11BDn.o: in function `arch_force_pkey_reg_init':
> /home/rpm/linux-5.14.0-69.el9/tools/testing/selftests/vm/protection_keys.c:
> 1302: undefined reference to `__builtin_ia32_xsave'
> /usr/bin/ld:
> /home/rpm/linux-5.14.0-69.el9/tools/testing/selftests/vm/protection_keys.c:
> 1306: undefined reference to `__builtin_ia32_xrstor'
> collect2: error: ld returned 1 exit status
> 

If you add:
  -march=x86-64-v3 

to the gcc command line it should fix those without removing instructions.

Dave

> 3.  Disable the Line 1302 and Line 1306
> 
> 1301         /* XSAVE to build a valid buffer: */
> 1302         //__builtin_ia32_xsave(buf, XSTATE_PKEY);
> 1303         /* Clear XSTATE_BV[PKRU]: */
> 1304         buf[XSTATE_BV_OFFSET/sizeof(u64)] &= ~XSTATE_PKEY;
> 1305         /* XRSTOR will likely get PKRU back to the init state: */
> 1306         //__builtin_ia32_xrstor(buf, XSTATE_PKEY);
> 
> 
> 4. re-complie PASS.
> 
> 
> 5. execute test file and PASS.
> # ./protection_keys
> has pkeys: 1
> startup pkey_reg: 0000000055555550
> test  0 PASSED (iteration 1)
> test  1 PASSED (iteration 1)
> test  2 PASSED (iteration 1)
> ......
> test 16 PASSED (iteration 22)
> test 17 PASSED (iteration 22)
> test 18 PASSED (iteration 22)
> test 19 PASSED (iteration 22)
> done (all tests OK)
> 
> 
> Will upload the full test results later.
> 
> 
> Besides,  in Documentation/core-api/protection-keys.rst, I only find one
> flag 'pku' which may can be tested on qemu side, the test steps may only
> check that this flag can expose to guest.
> Could you please also help to check this?
> 
> Thanks a lot.
> 
> 
> 
> Best regards
> Liu Nana
Comment 12 liunana 2022-03-07 12:19:38 UTC 
(In reply to Dr. David Alan Gilbert from comment #11)
> (In reply to liunana from comment #8)
> > (In reply to Terry Bowman from comment #7)
> > > (In reply to liunana from comment #5)
> > > > Hi,
> > > > 
> > > > 
> > > > Would you please help to set the ITR and provide how to test this feature on
> > > > QEMU side?
> > > > Thanks a lot!
> > > > 
> > > > 
> > > > Best regards
> > > > Liu Nana
> > > 
> > > Hi Liu,
> > > 
> > > You can test this with running tools/testing/selftests/vm/protection_keys_64
> > > in the guest. This is how I tested BZ1988104 (this is cloned from).
> > > 
> > Thank you, Terry.
> > 
> > But I met some issue while compling the source file, could you please help
> > to check if the results are expected?
> > 
> > Test Env:
> >    5.14.0-69.el9.x86_64
> >   qemu-kvm-6.2.0-10.el9.x86_64
> > 
> > 
> > Test Steps:
> > 1. download the source kernel file kernel-5.14.0-69.el9.src.rpm and
> > uncompress it.
> > 
> > 
> > 2. complie protection_keys.c FAILED at first.
> > # gcc      -o protection_keys    -O2 -g -std=gnu99 -pthread -Wall
> > protection_keys.c -lrt -ldl -lm
> > 
> > protection_keys.c: In function ‘arch_force_pkey_reg_init’:
> > protection_keys.c:1302:9: warning: implicit declaration of function
> > ‘__builtin_ia32_xsave’; did you mean ‘__builtin_ia32_fxsave’?
> > [-Wimplicit-function-declaration]
> >  1302 |         __builtin_ia32_xsave(buf, XSTATE_PKEY);
> >       |         ^~~~~~~~~~~~~~~~~~~~
> >       |         __builtin_ia32_fxsave
> > protection_keys.c:1306:9: warning: implicit declaration of function
> > ‘__builtin_ia32_xrstor’; did you mean ‘__builtin_ia32_fxrstor’?
> > [-Wimplicit-function-declaration]
> >  1306 |         __builtin_ia32_xrstor(buf, XSTATE_PKEY);
> >       |         ^~~~~~~~~~~~~~~~~~~~~
> >       |         __builtin_ia32_fxrstor
> > /usr/bin/ld: /tmp/ccQ11BDn.o: in function `arch_force_pkey_reg_init':
> > /home/rpm/linux-5.14.0-69.el9/tools/testing/selftests/vm/protection_keys.c:
> > 1302: undefined reference to `__builtin_ia32_xsave'
> > /usr/bin/ld:
> > /home/rpm/linux-5.14.0-69.el9/tools/testing/selftests/vm/protection_keys.c:
> > 1306: undefined reference to `__builtin_ia32_xrstor'
> > collect2: error: ld returned 1 exit status
> > 
> 
> If you add:
>   -march=x86-64-v3 
> 
> to the gcc command line it should fix those without removing instructions.
> 

Yes, it fixes this complie issue, thanks.

And test protection_keys PASS.

#./protection_keys
has pkeys: 1
startup pkey_reg: 0000000055555550
test  0 PASSED (iteration 1)
test  1 PASSED (iteration 1)
......
test 17 PASSED (iteration 22)
test 18 PASSED (iteration 22)
test 19 PASSED (iteration 22)
done (all tests OK)


Full test results will be upload as 'protection_keys_result_final' later.


Best regards
Liu Nana
Comment 13 liunana 2022-03-07 12:20:36 UTC 
Created attachment 1864344 [details]
protection_keys_result_final
Comment 14 Nitesh Narayan Lal 2022-03-07 17:13:44 UTC 
As per comment 3, the patches came in as part of the qemu rebase.
Hence, marking this as a TestOnly BZ.
Comment 15 liunana 2022-03-08 02:54:40 UTC 
Move this bug to verified according to Comment 12.
Comment 19 errata-xmlrpc 2022-05-17 12:23:27 UTC 
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (new packages: qemu-kvm), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2022:2307
Comment 20 liunana 2022-09-07 04:17:34 UTC 
(In reply to Terry Bowman (AMD) from comment #7)
> (In reply to liunana from comment #5)
> > Hi,
> > 
> > 
> > Would you please help to set the ITR and provide how to test this feature on
> > QEMU side?
> > Thanks a lot!
> > 
> > 
> > Best regards
> > Liu Nana
> 
> Hi Liu,
> 
> You can test this with running tools/testing/selftests/vm/protection_keys_64
> in the guest. This is how I tested BZ1988104 (this is cloned from).
> 
> Regards,
> Terry

Hi Terry,


I met on warning issue while compile this tool with '-march=x86-64' inside rhel9 guest. I didn't hit this issue inside RHEL8.7 guest.

[root@vm-73-197 vm]# gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm -march=x86-64
gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall protection_keys.c -lrt -ldl -lm -march=x86-64
protection_keys.c: In function ‘arch_force_pkey_reg_init’:
protection_keys.c:1302:9: warning: implicit declaration of function ‘__builtin_ia32_xsave’; did you mean ‘__builtin_ia32_fxsave’? [-Wimplicit-function-declaration]
 1302 |         __builtin_ia32_xsave(buf, XSTATE_PKEY);
      |         ^~~~~~~~~~~~~~~~~~~~
      |         __builtin_ia32_fxsave
protection_keys.c:1306:9: warning: implicit declaration of function ‘__builtin_ia32_xrstor’; did you mean ‘__builtin_ia32_fxrstor’? [-Wimplicit-function-declaration]
 1306 |         __builtin_ia32_xrstor(buf, XSTATE_PKEY);
      |         ^~~~~~~~~~~~~~~~~~~~~
      |         __builtin_ia32_fxrstor
/usr/bin/ld: /tmp/cczIdRgy.o: in function `arch_force_pkey_reg_init':
/home/tmp/linux-5.14.0-161.el9/tools/testing/selftests/vm/protection_keys.c:1302: undefined reference to `__builtin_ia32_xsave'
/usr/bin/ld: /home/tmp/linux-5.14.0-161.el9/tools/testing/selftests/vm/protection_keys.c:1306: undefined reference to `__builtin_ia32_xrstor'
collect2: error: ld returned 1 exit status


Besides, tools work well with '-march=x86-64-v3'.
Would you please help to check if this is tools' issue?  Thanks.



Best regards
Nana Liu
Comment 21 Terry Bowman (AMD) 2022-09-12 14:42:21 UTC 
(In reply to liunana from comment #20)
> (In reply to Terry Bowman (AMD) from comment #7)
> > (In reply to liunana from comment #5)
> > > Hi,
> > > 
> > > 
> > > Would you please help to set the ITR and provide how to test this feature on
> > > QEMU side?
> > > Thanks a lot!
> > > 
> > > 
> > > Best regards
> > > Liu Nana
> > 
> > Hi Liu,
> > 
> > You can test this with running tools/testing/selftests/vm/protection_keys_64
> > in the guest. This is how I tested BZ1988104 (this is cloned from).
> > 
> > Regards,
> > Terry
> 
> Hi Terry,
> 
> 
> I met on warning issue while compile this tool with '-march=x86-64' inside
> rhel9 guest. I didn't hit this issue inside RHEL8.7 guest.
> 
> [root@vm-73-197 vm]# gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall
> protection_keys.c -lrt -ldl -lm -march=x86-64
> gcc -o protection_keys -O2 -g -std=gnu99 -pthread -Wall protection_keys.c
> -lrt -ldl -lm -march=x86-64
> protection_keys.c: In function ‘arch_force_pkey_reg_init’:
> protection_keys.c:1302:9: warning: implicit declaration of function
> ‘__builtin_ia32_xsave’; did you mean ‘__builtin_ia32_fxsave’?
> [-Wimplicit-function-declaration]
>  1302 |         __builtin_ia32_xsave(buf, XSTATE_PKEY);
>       |         ^~~~~~~~~~~~~~~~~~~~
>       |         __builtin_ia32_fxsave
> protection_keys.c:1306:9: warning: implicit declaration of function
> ‘__builtin_ia32_xrstor’; did you mean ‘__builtin_ia32_fxrstor’?
> [-Wimplicit-function-declaration]
>  1306 |         __builtin_ia32_xrstor(buf, XSTATE_PKEY);
>       |         ^~~~~~~~~~~~~~~~~~~~~
>       |         __builtin_ia32_fxrstor
> /usr/bin/ld: /tmp/cczIdRgy.o: in function `arch_force_pkey_reg_init':
> /home/tmp/linux-5.14.0-161.el9/tools/testing/selftests/vm/protection_keys.c:
> 1302: undefined reference to `__builtin_ia32_xsave'
> /usr/bin/ld:
> /home/tmp/linux-5.14.0-161.el9/tools/testing/selftests/vm/protection_keys.c:
> 1306: undefined reference to `__builtin_ia32_xrstor'
> collect2: error: ld returned 1 exit status
> 
> 
> Besides, tools work well with '-march=x86-64-v3'.
> Would you please help to check if this is tools' issue?  Thanks.
> 
> 
> 
> Best regards
> Nana Liu

Hi Nana,


I added a Redhat documentation link below that describes the micro-architecture changes introduced in RHEL9. This includes x86-64-v2, x86-64-v3, and x86-64-v4.


https://developers.redhat.com/blog/2021/01/05/building-red-hat-enterprise-linux-9-for-the-x86-64-v2-microarchitecture-level