[kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid

[kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid

[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

Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid

[Sean Christopherson]

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
> 

Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid

[Sean Christopherson]

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

RE: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid

[Duan, Zhenzhong]

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

Re: [kvm-unit-tests PATCH] x86: Assign a canonical address before execute invpcid

[Sean Christopherson]

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.