[Qemu-devel] [PATCH 0/2] Revert exposure of PCONFIG to guest

[Qemu-devel] [PATCH 0/2] Revert exposure of PCONFIG to guest

[Robert Hoo]

PCONFIG is not supposed to be exposed to guest. These 2 patches fix this.

Robert Hoo (2):
  i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model
  Revert "i386: Add CPUID bit for PCONFIG"

 target/i386/cpu.c | 5 ++---
 target/i386/cpu.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Robert Hoo]

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 677a3bd..b6113d0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2613,8 +2613,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
             CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57,
         .features[FEAT_7_0_EDX] =
-            CPUID_7_0_EDX_PCONFIG | CPUID_7_0_EDX_SPEC_CTRL |
-            CPUID_7_0_EDX_SPEC_CTRL_SSBD,
+            CPUID_7_0_EDX_SPEC_CTRL | CPUID_7_0_EDX_SPEC_CTRL_SSBD,
         /* Missing: XSAVES (not supported by some Linux versions,
                 * including v4.1 to v4.12).
                 * KVM doesn't yet expose any XSAVES state save component,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Daniel P. Berrangé]

On Wed, Dec 19, 2018 at 09:44:40PM +0800, Robert Hoo wrote:
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> ---
>  target/i386/cpu.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 677a3bd..b6113d0 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2613,8 +2613,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG |
>              CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57,
>          .features[FEAT_7_0_EDX] =
> -            CPUID_7_0_EDX_PCONFIG | CPUID_7_0_EDX_SPEC_CTRL |
> -            CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> +            CPUID_7_0_EDX_SPEC_CTRL | CPUID_7_0_EDX_SPEC_CTRL_SSBD,
>          /* Missing: XSAVES (not supported by some Linux versions,
>                  * including v4.1 to v4.12).
>                  * KVM doesn't yet expose any XSAVES state save component,

This was shipped in QEMU 3.1.0, so I don't think we can unconditionally
remove it like this without breaking CPU model migration compat. 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Robert Hoo]

On Wed, 2018-12-19 at 14:01 +0000, Daniel P. Berrangé wrote:
> On Wed, Dec 19, 2018 at 09:44:40PM +0800, Robert Hoo wrote:
> > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > ---
> >  target/i386/cpu.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 677a3bd..b6113d0 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -2613,8 +2613,7 @@ static X86CPUDefinition builtin_x86_defs[] =
> > {
> >              CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG
> > |
> >              CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57,
> >          .features[FEAT_7_0_EDX] =
> > -            CPUID_7_0_EDX_PCONFIG | CPUID_7_0_EDX_SPEC_CTRL |
> > -            CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> > +            CPUID_7_0_EDX_SPEC_CTRL |
> > CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> >          /* Missing: XSAVES (not supported by some Linux versions,
> >                  * including v4.1 to v4.12).
> >                  * KVM doesn't yet expose any XSAVES state save
> > component,
> 
> This was shipped in QEMU 3.1.0, so I don't think we can
> unconditionally
> remove it like this without breaking CPU model migration compat. 
> 
I think the sooner, the better. Take the time window that Icelake CPU
model has just shipped with QEMU 3.1.0 and is not publicly/widely used
yet.
> 
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Paolo Bonzini]

On 20/12/18 01:18, Robert Hoo wrote:
> On Wed, 2018-12-19 at 14:01 +0000, Daniel P. Berrangé wrote:
>> On Wed, Dec 19, 2018 at 09:44:40PM +0800, Robert Hoo wrote:
>>> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
>>> ---
>>>  target/i386/cpu.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 677a3bd..b6113d0 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -2613,8 +2613,7 @@ static X86CPUDefinition builtin_x86_defs[] =
>>> {
>>>              CPUID_7_0_ECX_AVX512VNNI | CPUID_7_0_ECX_AVX512BITALG
>>> |
>>>              CPUID_7_0_ECX_AVX512_VPOPCNTDQ | CPUID_7_0_ECX_LA57,
>>>          .features[FEAT_7_0_EDX] =
>>> -            CPUID_7_0_EDX_PCONFIG | CPUID_7_0_EDX_SPEC_CTRL |
>>> -            CPUID_7_0_EDX_SPEC_CTRL_SSBD,
>>> +            CPUID_7_0_EDX_SPEC_CTRL |
>>> CPUID_7_0_EDX_SPEC_CTRL_SSBD,
>>>          /* Missing: XSAVES (not supported by some Linux versions,
>>>                  * including v4.1 to v4.12).
>>>                  * KVM doesn't yet expose any XSAVES state save
>>> component,
>>
>> This was shipped in QEMU 3.1.0, so I don't think we can
>> unconditionally
>> remove it like this without breaking CPU model migration compat. 
>>
> I think the sooner, the better. Take the time window that Icelake CPU
> model has just shipped with QEMU 3.1.0 and is not publicly/widely used
> yet.

We should still leave it in the 3.1 machine types.  I've just sent a
patch to do the same with MPX.

Paolo

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Robert Hoo]

On Thu, 2018-12-20 at 13:38 +0100, Paolo Bonzini wrote:
> On 20/12/18 01:18, Robert Hoo wrote:
> > On Wed, 2018-12-19 at 14:01 +0000, Daniel P. Berrangé wrote:
> > > On Wed, Dec 19, 2018 at 09:44:40PM +0800, Robert Hoo wrote:
> > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > > ---
> > > >  target/i386/cpu.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > > index 677a3bd..b6113d0 100644
> > > > --- a/target/i386/cpu.c
> > > > +++ b/target/i386/cpu.c
> > > > @@ -2613,8 +2613,7 @@ static X86CPUDefinition
> > > > builtin_x86_defs[] =
> > > > {
> > > >              CPUID_7_0_ECX_AVX512VNNI |
> > > > CPUID_7_0_ECX_AVX512BITALG
> > > > > 
> > > > 
> > > >              CPUID_7_0_ECX_AVX512_VPOPCNTDQ |
> > > > CPUID_7_0_ECX_LA57,
> > > >          .features[FEAT_7_0_EDX] =
> > > > -            CPUID_7_0_EDX_PCONFIG | CPUID_7_0_EDX_SPEC_CTRL |
> > > > -            CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> > > > +            CPUID_7_0_EDX_SPEC_CTRL |
> > > > CPUID_7_0_EDX_SPEC_CTRL_SSBD,
> > > >          /* Missing: XSAVES (not supported by some Linux
> > > > versions,
> > > >                  * including v4.1 to v4.12).
> > > >                  * KVM doesn't yet expose any XSAVES state save
> > > > component,
> > > 
> > > This was shipped in QEMU 3.1.0, so I don't think we can
> > > unconditionally
> > > remove it like this without breaking CPU model migration compat. 
> > > 
> > 
> > I think the sooner, the better. Take the time window that Icelake
> > CPU
> > model has just shipped with QEMU 3.1.0 and is not publicly/widely
> > used
> > yet.
> 
> We should still leave it in the 3.1 machine types.  I've just sent a
> patch to do the same with MPX.
> 
I took a look your patch of "Disable MPX support on named CPU models".
Seems you do the same as I do to PCONFIG. So you agree with my above
patch?:-)

I won't object that keep it in 3.1 machine type as you do to MPX.

> Paolo
> 

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Paolo Bonzini]

On 20/12/18 13:50, Robert Hoo wrote:
>> We should still leave it in the 3.1 machine types.  I've just sent a
>> patch to do the same with MPX.
>>
> I took a look your patch of "Disable MPX support on named CPU models".
> Seems you do the same as I do to PCONFIG. So you agree with my above
> patch?:-)

If you add send a version that keeps it in the 3.1 machine type, I do.

Paolo

> I won't object that keep it in 3.1 machine type as you do to MPX.
> 

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Paolo Bonzini]

On 20/12/18 13:50, Robert Hoo wrote:
> On Thu, 2018-12-20 at 13:38 +0100, Paolo Bonzini wrote:
>> On 20/12/18 01:18, Robert Hoo wrote:
>>> I think the sooner, the better. Take the time window that Icelake
>>> CPU
>>> model has just shipped with QEMU 3.1.0 and is not publicly/widely
>>> used
>>> yet.
>>
>> We should still leave it in the 3.1 machine types.  I've just sent a
>> patch to do the same with MPX.
>>
> I took a look your patch of "Disable MPX support on named CPU models".
> Seems you do the same as I do to PCONFIG. So you agree with my above
> patch?:-)
> 
> I won't object that keep it in 3.1 machine type as you do to MPX.

Sorry Robert, I changed my mind.  If no hypervisor exists that enables
PCONFIG for guests (using the PCONFIG_ENABLE processor control),
effectively no one can ever have used it.  We should disable it in all
machine types and Cc qemu-stable.

In fact, the same is true for INTEL_PT, which is not supported by any
released kernel version and, even is going to be available only with a
module parameter when it will be.

This is not the same as MPX, which did work even though nobody was
probably using it.

So this series is correct and I will follow up with one for INTEL_PT;
however, this begs the question of how the patches are being tested.

Paolo

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Robert Hoo]

On Fri, 2018-12-21 at 07:27 +0100, Paolo Bonzini wrote:
> On 20/12/18 13:50, Robert Hoo wrote:
> > On Thu, 2018-12-20 at 13:38 +0100, Paolo Bonzini wrote:
> > > On 20/12/18 01:18, Robert Hoo wrote:
> > > > I think the sooner, the better. Take the time window that
> > > > Icelake
> > > > CPU
> > > > model has just shipped with QEMU 3.1.0 and is not
> > > > publicly/widely
> > > > used
> > > > yet.
> > > 
> > > We should still leave it in the 3.1 machine types.  I've just
> > > sent a
> > > patch to do the same with MPX.
> > > 
> > 
> > I took a look your patch of "Disable MPX support on named CPU
> > models".
> > Seems you do the same as I do to PCONFIG. So you agree with my
> > above
> > patch?:-)
> > 
> > I won't object that keep it in 3.1 machine type as you do to MPX.
> 
> Sorry Robert, I changed my mind.  If no hypervisor exists that
> enables
> PCONFIG for guests (using the PCONFIG_ENABLE processor control),
> effectively no one can ever have used it.  We should disable it in
> all
> machine types and Cc qemu-stable.

Thanks Paolo.
> 
> In fact, the same is true for INTEL_PT, which is not supported by any
> released kernel version and, even is going to be available only with
> a
> module parameter when it will be.

Add Luwei in judging this.
> 
> This is not the same as MPX, which did work even though nobody was
> probably using it.
> 
> So this series is correct and I will follow up with one for INTEL_PT;
> however, this begs the question of how the patches are being tested.
> 
My apologies for carelessness.

I've seen you patch for INTEL_PT. So am I going to resend these 2
patches and Cc qemu-stable? or simply reply these 2 patches adding
qemu-stable in Cc list?

> Paolo

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Paolo Bonzini]

On 21/12/18 15:04, Robert Hoo wrote:
>> So this series is correct and I will follow up with one for INTEL_PT;
>> however, this begs the question of how the patches are being tested.
>
> My apologies for carelessness.

No problem.  In the future please check that "-cpu Icelake-Client"
doesn't have warnings such as

qemu-system-x86_64: warning: host doesn't support requested feature:
CPUID.07H:EBX.intel-pt [bit 25]

when run on an Icelake-Client host.

> I've seen you patch for INTEL_PT. So am I going to resend these 2
> patches and Cc qemu-stable? or simply reply these 2 patches adding
> qemu-stable in Cc list?

I can take care of that, thanks.

Paolo

Re: [Qemu-devel] [PATCH 1/2] i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model

[Robert Hoo]

On Fri, 2018-12-21 at 16:03 +0100, Paolo Bonzini wrote:
> On 21/12/18 15:04, Robert Hoo wrote:
> > > So this series is correct and I will follow up with one for
> > > INTEL_PT;
> > > however, this begs the question of how the patches are being
> > > tested.
> > 
> > My apologies for carelessness.
> 
> No problem.  In the future please check that "-cpu Icelake-Client"
> doesn't have warnings such as
> 
> qemu-system-x86_64: warning: host doesn't support requested feature:
> CPUID.07H:EBX.intel-pt [bit 25]
> 
> when run on an Icelake-Client host.

Sure. I'm going to ask our validation team to add these in test
criteria.
> 
> > I've seen you patch for INTEL_PT. So am I going to resend these 2
> > patches and Cc qemu-stable? or simply reply these 2 patches adding
> > qemu-stable in Cc list?
> 
> I can take care of that, thanks.
> 
> Paolo
> 

[Qemu-devel] [PATCH 2/2] Revert "i386: Add CPUID bit for PCONFIG"

[Robert Hoo]

This reverts commit 5131dc433df54b37e8e918d8fba7fe10344e7a7b.
For new instruction 'PCONFIG' will not be exposed to guest.

Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
---
 target/i386/cpu.c | 2 +-
 target/i386/cpu.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b6113d0..08d4307 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1040,7 +1040,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
-            NULL, NULL, "pconfig", NULL,
+            NULL, NULL, NULL, NULL,
             NULL, NULL, NULL, NULL,
             NULL, NULL, "spec-ctrl", "stibp",
             NULL, "arch-capabilities", NULL, "ssbd",
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index ef41a03..66707d7 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -692,7 +692,6 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
 
 #define CPUID_7_0_EDX_AVX512_4VNNIW (1U << 2) /* AVX512 Neural Network Instructions */
 #define CPUID_7_0_EDX_AVX512_4FMAPS (1U << 3) /* AVX512 Multiply Accumulation Single Precision */
-#define CPUID_7_0_EDX_PCONFIG (1U << 18)       /* Platform Configuration */
 #define CPUID_7_0_EDX_SPEC_CTRL     (1U << 26) /* Speculation Control */
 #define CPUID_7_0_EDX_ARCH_CAPABILITIES (1U << 29)  /*Arch Capabilities*/
 #define CPUID_7_0_EDX_SPEC_CTRL_SSBD  (1U << 31) /* Speculative Store Bypass Disable */
-- 
1.8.3.1