| Message ID | 1545227081-213696-2-git-send-email-robert.hu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert exposure of PCONFIG to guest | expand |
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
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
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
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 >
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. >
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
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
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
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 --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,
Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- target/i386/cpu.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)