| Message ID | 20211230101452.380581-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Assign a canonical address before execute invpcid | expand |
On Thu, Dec 30, 2021, Zhenzhong Duan wrote: > Occasionally we see pcid test fail as INVPCID_DESC[127:64] is > uninitialized before execute invpcid. > > According to Intel spec: "#GP If INVPCID_TYPE is 0 and the linear > address in INVPCID_DESC[127:64] is not canonical." > > Assign desc's address which is guaranteed to be a real memory > address and canonical. > > Fixes: b44d84dae10c ("Add PCID/INVPCID test") > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > x86/pcid.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/x86/pcid.c b/x86/pcid.c > index 527a4a9..4828bbc 100644 > --- a/x86/pcid.c > +++ b/x86/pcid.c > @@ -75,6 +75,9 @@ static void test_invpcid_enabled(int pcid_enabled) > struct invpcid_desc desc; > desc.rsv = 0; > > + /* Initialize INVPCID_DESC[127:64] with a canonical address */ > + desc.addr = (u64)&desc; Casting to a u64 is arguably wrong since the address is an unsigned long. It doesn't cause problems because the test is 64-bit only, but it's a bit odd. What about just replacing "desc.rsv = 0" with "memset(&desc, 0, sizeof(desc))"? > + > /* try executing invpcid when CR4.PCIDE=0, desc.pcid=0 and type=0..3 > * no exception expected > */ > -- > 2.25.1 >
On Thu, Dec 30, 2021, Sean Christopherson wrote: > On Thu, Dec 30, 2021, Zhenzhong Duan wrote: > > Occasionally we see pcid test fail as INVPCID_DESC[127:64] is > > uninitialized before execute invpcid. > > > > According to Intel spec: "#GP If INVPCID_TYPE is 0 and the linear > > address in INVPCID_DESC[127:64] is not canonical." > > > > Assign desc's address which is guaranteed to be a real memory > > address and canonical. > > > > Fixes: b44d84dae10c ("Add PCID/INVPCID test") > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > > --- > > x86/pcid.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/x86/pcid.c b/x86/pcid.c > > index 527a4a9..4828bbc 100644 > > --- a/x86/pcid.c > > +++ b/x86/pcid.c > > @@ -75,6 +75,9 @@ static void test_invpcid_enabled(int pcid_enabled) > > struct invpcid_desc desc; > > desc.rsv = 0; > > > > + /* Initialize INVPCID_DESC[127:64] with a canonical address */ > > + desc.addr = (u64)&desc; > > Casting to a u64 is arguably wrong since the address is an unsigned long. It > doesn't cause problems because the test is 64-bit only, but it's a bit odd. I take that back, "struct invpcid_desc" is the one that's "wrong". Again, doesn't truly matter as attempting to build on 32-bit would fail due to the bitfield values exceeding the storage capacity of an unsigned long. But to be pedantic, maybe this? diff --git a/x86/pcid.c b/x86/pcid.c index 527a4a9..fd218dd 100644 --- a/x86/pcid.c +++ b/x86/pcid.c @@ -5,9 +5,9 @@ #include "desc.h" struct invpcid_desc { - unsigned long pcid : 12; - unsigned long rsv : 52; - unsigned long addr : 64; + u64 pcid : 12; + u64 rsv : 52; + u64 addr : 64; }; static int write_cr0_checking(unsigned long val) @@ -73,7 +73,8 @@ static void test_invpcid_enabled(int pcid_enabled) int passed = 0, i; ulong cr4 = read_cr4(); struct invpcid_desc desc; - desc.rsv = 0; + + memset(&desc, 0, sizeof(desc)); /* try executing invpcid when CR4.PCIDE=0, desc.pcid=0 and type=0..3 * no exception expected
>-----Original Message----- >From: Sean Christopherson <seanjc@google.com> >Sent: Friday, December 31, 2021 1:50 AM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: kvm@vger.kernel.org; pbonzini@redhat.com >Subject: Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before >execute invpcid > >On Thu, Dec 30, 2021, Sean Christopherson wrote: >> On Thu, Dec 30, 2021, Zhenzhong Duan wrote: >> > Occasionally we see pcid test fail as INVPCID_DESC[127:64] is >> > uninitialized before execute invpcid. >> > >> > According to Intel spec: "#GP If INVPCID_TYPE is 0 and the linear >> > address in INVPCID_DESC[127:64] is not canonical." >> > >> > Assign desc's address which is guaranteed to be a real memory >> > address and canonical. >> > >> > Fixes: b44d84dae10c ("Add PCID/INVPCID test") >> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> > --- >> > x86/pcid.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/x86/pcid.c b/x86/pcid.c index 527a4a9..4828bbc 100644 >> > --- a/x86/pcid.c >> > +++ b/x86/pcid.c >> > @@ -75,6 +75,9 @@ static void test_invpcid_enabled(int pcid_enabled) >> > struct invpcid_desc desc; >> > desc.rsv = 0; >> > >> > + /* Initialize INVPCID_DESC[127:64] with a canonical address */ >> > + desc.addr = (u64)&desc; >> >> Casting to a u64 is arguably wrong since the address is an unsigned >> long. It doesn't cause problems because the test is 64-bit only, but it's a bit >odd. > >I take that back, "struct invpcid_desc" is the one that's "wrong". Again, >doesn't truly matter as attempting to build on 32-bit would fail due to the >bitfield values exceeding the storage capacity of an unsigned long. But to be >pedantic, maybe this? Sorry for late response. Not clear why the mail went into junk box automatically. Yea, I think your change is better. Will you send formal patch with your change or you want me to do that? Thanks Zhenzhong > >diff --git a/x86/pcid.c b/x86/pcid.c >index 527a4a9..fd218dd 100644 >--- a/x86/pcid.c >+++ b/x86/pcid.c >@@ -5,9 +5,9 @@ > #include "desc.h" > > struct invpcid_desc { >- unsigned long pcid : 12; >- unsigned long rsv : 52; >- unsigned long addr : 64; >+ u64 pcid : 12; >+ u64 rsv : 52; >+ u64 addr : 64; > }; > > static int write_cr0_checking(unsigned long val) @@ -73,7 +73,8 @@ static >void test_invpcid_enabled(int pcid_enabled) > int passed = 0, i; > ulong cr4 = read_cr4(); > struct invpcid_desc desc; >- desc.rsv = 0; >+ >+ memset(&desc, 0, sizeof(desc)); > > /* try executing invpcid when CR4.PCIDE=0, desc.pcid=0 and type=0..3 > * no exception expected
On Tue, Jan 11, 2022, Duan, Zhenzhong wrote: > >From: Sean Christopherson <seanjc@google.com> > >I take that back, "struct invpcid_desc" is the one that's "wrong". Again, > >doesn't truly matter as attempting to build on 32-bit would fail due to the > >bitfield values exceeding the storage capacity of an unsigned long. But to be > >pedantic, maybe this? > > Sorry for late response. Not clear why the mail went into junk box automatically. No worries, I know that pain all too well. > Yea, I think your change is better. Will you send formal patch with your change > or you want me to do that? No preference. If you get to it, great, if not I'll send a patch in a day or two.
diff --git a/x86/pcid.c b/x86/pcid.c index 527a4a9..4828bbc 100644 --- a/x86/pcid.c +++ b/x86/pcid.c @@ -75,6 +75,9 @@ static void test_invpcid_enabled(int pcid_enabled) struct invpcid_desc desc; desc.rsv = 0; + /* Initialize INVPCID_DESC[127:64] with a canonical address */ + desc.addr = (u64)&desc; + /* try executing invpcid when CR4.PCIDE=0, desc.pcid=0 and type=0..3 * no exception expected */
Occasionally we see pcid test fail as INVPCID_DESC[127:64] is uninitialized before execute invpcid. According to Intel spec: "#GP If INVPCID_TYPE is 0 and the linear address in INVPCID_DESC[127:64] is not canonical." Assign desc's address which is guaranteed to be a real memory address and canonical. Fixes: b44d84dae10c ("Add PCID/INVPCID test") Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- x86/pcid.c | 3 +++ 1 file changed, 3 insertions(+)