* [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid @ 2021-12-30 10:14 Zhenzhong Duan 2021-12-30 15:52 ` Sean Christopherson 0 siblings, 1 reply; 5+ messages in thread From: Zhenzhong Duan @ 2021-12-30 10:14 UTC (permalink / raw) To: kvm; +Cc: pbonzini, Zhenzhong Duan 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; + /* try executing invpcid when CR4.PCIDE=0, desc.pcid=0 and type=0..3 * no exception expected */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid 2021-12-30 10:14 [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid Zhenzhong Duan @ 2021-12-30 15:52 ` Sean Christopherson 2021-12-30 17:50 ` Sean Christopherson 0 siblings, 1 reply; 5+ messages in thread From: Sean Christopherson @ 2021-12-30 15:52 UTC (permalink / raw) To: Zhenzhong Duan; +Cc: kvm, pbonzini 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid 2021-12-30 15:52 ` Sean Christopherson @ 2021-12-30 17:50 ` Sean Christopherson 2022-01-11 8:22 ` Duan, Zhenzhong 0 siblings, 1 reply; 5+ messages in thread From: Sean Christopherson @ 2021-12-30 17:50 UTC (permalink / raw) To: Zhenzhong Duan; +Cc: kvm, pbonzini 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid 2021-12-30 17:50 ` Sean Christopherson @ 2022-01-11 8:22 ` Duan, Zhenzhong 2022-01-11 16:23 ` Sean Christopherson 0 siblings, 1 reply; 5+ messages in thread From: Duan, Zhenzhong @ 2022-01-11 8:22 UTC (permalink / raw) To: Christopherson,, Sean; +Cc: kvm, pbonzini >-----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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid 2022-01-11 8:22 ` Duan, Zhenzhong @ 2022-01-11 16:23 ` Sean Christopherson 0 siblings, 0 replies; 5+ messages in thread From: Sean Christopherson @ 2022-01-11 16:23 UTC (permalink / raw) To: Duan, Zhenzhong; +Cc: kvm, pbonzini 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-11 16:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-30 10:14 [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid Zhenzhong Duan 2021-12-30 15:52 ` Sean Christopherson 2021-12-30 17:50 ` Sean Christopherson 2022-01-11 8:22 ` Duan, Zhenzhong 2022-01-11 16:23 ` Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).