diff mbox series

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

Message ID 1545227081-213696-2-git-send-email-robert.hu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series Revert exposure of PCONFIG to guest | expand

Commit Message

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

Comments

Daniel P. Berrangé Dec. 19, 2018, 2:01 p.m. UTC | #1 
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
Robert Hoo Dec. 20, 2018, 12:18 a.m. UTC | #2 
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
Paolo Bonzini Dec. 20, 2018, 12:38 p.m. UTC | #3 
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
Robert Hoo Dec. 20, 2018, 12:50 p.m. UTC | #4 
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
>
Paolo Bonzini Dec. 20, 2018, 1:06 p.m. UTC | #5 
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.
>
Paolo Bonzini Dec. 21, 2018, 6:27 a.m. UTC | #6 
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
Robert Hoo Dec. 21, 2018, 2:04 p.m. UTC | #7 
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
Paolo Bonzini Dec. 21, 2018, 3:03 p.m. UTC | #8 
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
Robert Hoo Dec. 22, 2018, 12:58 a.m. UTC | #9 
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
>
diff mbox series

Patch

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,