[PATCH] target/i386: Remove MPX support on Snowridge CPU model

[PATCH] target/i386: Remove MPX support on Snowridge CPU model

[Xiaoyao Li]

MPX support is being phased out by Intel. Following other CPU models
like Skylake, Icelake and Cascadelake, do not enable it by default.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
I'm not sure is there anyone already use Snowridge CPU model and whether to
add it in pc_compat_4_1, since Snowridge has not been released yet.

---
 hw/i386/pc.c      | 4 +++-
 target/i386/cpu.c | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index bcda50efcc23..97eb7ac32588 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -105,7 +105,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 /* Physical Address of PVH entry point read from kernel ELF NOTE */
 static size_t pvh_start_addr;
 
-GlobalProperty pc_compat_4_1[] = {};
+GlobalProperty pc_compat_4_1[] = {
+    { "Snowridge" "-" TYPE_X86_CPU, "mpx", "on" },
+};
 const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
 
 GlobalProperty pc_compat_4_0[] = {};
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 44f1bbdcac76..885bea76205d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -2762,7 +2762,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
             CPUID_7_0_EBX_FSGSBASE |
             CPUID_7_0_EBX_SMEP |
             CPUID_7_0_EBX_ERMS |
-            CPUID_7_0_EBX_MPX |  /* missing bits 13, 15 */
             CPUID_7_0_EBX_RDSEED |
             CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT |
             CPUID_7_0_EBX_CLWB |
-- 
2.19.1

Re: [PATCH] target/i386: Remove MPX support on Snowridge CPU model

[Eduardo Habkost]

On Fri, Oct 11, 2019 at 03:18:44PM +0800, Xiaoyao Li wrote:
> MPX support is being phased out by Intel. Following other CPU models
> like Skylake, Icelake and Cascadelake, do not enable it by default.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> I'm not sure is there anyone already use Snowridge CPU model and whether to
> add it in pc_compat_4_1, since Snowridge has not been released yet.

We have a CPU model versioning mechanism now, please implement this using the
new system.  e.g.:

        .versions = (X86CPUVersionDefinition[]) {
            { .version = 1 },
            {
                .version = 2,
                .props = (PropValue[]) {
                    { "mpx", "off" },
                    { /* end of list */ }
                }
            },
            { /* end of list */ }
        }

Then machine-type compat properties won't be needed anymore.

> 
> ---
>  hw/i386/pc.c      | 4 +++-
>  target/i386/cpu.c | 1 -
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index bcda50efcc23..97eb7ac32588 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -105,7 +105,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  /* Physical Address of PVH entry point read from kernel ELF NOTE */
>  static size_t pvh_start_addr;
>  
> -GlobalProperty pc_compat_4_1[] = {};
> +GlobalProperty pc_compat_4_1[] = {
> +    { "Snowridge" "-" TYPE_X86_CPU, "mpx", "on" },
> +};
>  const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
>  
>  GlobalProperty pc_compat_4_0[] = {};
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 44f1bbdcac76..885bea76205d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -2762,7 +2762,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
>              CPUID_7_0_EBX_FSGSBASE |
>              CPUID_7_0_EBX_SMEP |
>              CPUID_7_0_EBX_ERMS |
> -            CPUID_7_0_EBX_MPX |  /* missing bits 13, 15 */
>              CPUID_7_0_EBX_RDSEED |
>              CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT |
>              CPUID_7_0_EBX_CLWB |
> -- 
> 2.19.1
> 

-- 
Eduardo

Re: [PATCH] target/i386: Remove MPX support on Snowridge CPU model

[Xiaoyao Li]

On Fri, 2019-10-11 at 09:57 -0300, Eduardo Habkost wrote:
> On Fri, Oct 11, 2019 at 03:18:44PM +0800, Xiaoyao Li wrote:
> > MPX support is being phased out by Intel. Following other CPU models
> > like Skylake, Icelake and Cascadelake, do not enable it by default.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> > I'm not sure is there anyone already use Snowridge CPU model and whether to
> > add it in pc_compat_4_1, since Snowridge has not been released yet.
> 
> We have a CPU model versioning mechanism now, please implement this using the
> new system.  e.g.:
> 
>         .versions = (X86CPUVersionDefinition[]) {
>             { .version = 1 },
>             {
>                 .version = 2,
>                 .props = (PropValue[]) {
>                     { "mpx", "off" },
>                     { /* end of list */ }
>                 }
>             },
>             { /* end of list */ }
>         }
> 
> Then machine-type compat properties won't be needed anymore.

Since Snowridge CPU hasn't been released yet and I guess there might be no real
user use this CPU model. So do we really need to add v2? Can we just remove the
mpx feature in the original version?

> > 
> > ---
> >  hw/i386/pc.c      | 4 +++-
> >  target/i386/cpu.c | 1 -
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index bcda50efcc23..97eb7ac32588 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -105,7 +105,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> >  /* Physical Address of PVH entry point read from kernel ELF NOTE */
> >  static size_t pvh_start_addr;
> >  
> > -GlobalProperty pc_compat_4_1[] = {};
> > +GlobalProperty pc_compat_4_1[] = {
> > +    { "Snowridge" "-" TYPE_X86_CPU, "mpx", "on" },
> > +};
> >  const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
> >  
> >  GlobalProperty pc_compat_4_0[] = {};
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index 44f1bbdcac76..885bea76205d 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -2762,7 +2762,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
> >              CPUID_7_0_EBX_FSGSBASE |
> >              CPUID_7_0_EBX_SMEP |
> >              CPUID_7_0_EBX_ERMS |
> > -            CPUID_7_0_EBX_MPX |  /* missing bits 13, 15 */
> >              CPUID_7_0_EBX_RDSEED |
> >              CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT |
> >              CPUID_7_0_EBX_CLWB |
> > -- 
> > 2.19.1
> > 
> 
> 

Re: [PATCH] target/i386: Remove MPX support on Snowridge CPU model

[Eduardo Habkost]

On Fri, Oct 11, 2019 at 09:16:28PM +0800, Xiaoyao Li wrote:
> On Fri, 2019-10-11 at 09:57 -0300, Eduardo Habkost wrote:
> > On Fri, Oct 11, 2019 at 03:18:44PM +0800, Xiaoyao Li wrote:
> > > MPX support is being phased out by Intel. Following other CPU models
> > > like Skylake, Icelake and Cascadelake, do not enable it by default.
> > > 
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > ---
> > > I'm not sure is there anyone already use Snowridge CPU model and whether to
> > > add it in pc_compat_4_1, since Snowridge has not been released yet.
> > 
> > We have a CPU model versioning mechanism now, please implement this using the
> > new system.  e.g.:
> > 
> >         .versions = (X86CPUVersionDefinition[]) {
> >             { .version = 1 },
> >             {
> >                 .version = 2,
> >                 .props = (PropValue[]) {
> >                     { "mpx", "off" },
> >                     { /* end of list */ }
> >                 }
> >             },
> >             { /* end of list */ }
> >         }
> > 
> > Then machine-type compat properties won't be needed anymore.
> 
> Since Snowridge CPU hasn't been released yet and I guess there might be no real
> user use this CPU model. So do we really need to add v2? Can we just remove the
> mpx feature in the original version?

Fair question.  My main concern is that the inconsistency would
make automated validation of CPUID compatibility more difficult,
and also trigger bugs or spurious warnings in management software
that validates the CPU configuration.  It doesn't hurt to keep
the CPU model versions consistent with released QEMU versions.

> 
> > > 
> > > ---
> > >  hw/i386/pc.c      | 4 +++-
> > >  target/i386/cpu.c | 1 -
> > >  2 files changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index bcda50efcc23..97eb7ac32588 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -105,7 +105,9 @@ struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> > >  /* Physical Address of PVH entry point read from kernel ELF NOTE */
> > >  static size_t pvh_start_addr;
> > >  
> > > -GlobalProperty pc_compat_4_1[] = {};
> > > +GlobalProperty pc_compat_4_1[] = {
> > > +    { "Snowridge" "-" TYPE_X86_CPU, "mpx", "on" },
> > > +};
> > >  const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
> > >  
> > >  GlobalProperty pc_compat_4_0[] = {};
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index 44f1bbdcac76..885bea76205d 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -2762,7 +2762,6 @@ static X86CPUDefinition builtin_x86_defs[] = {
> > >              CPUID_7_0_EBX_FSGSBASE |
> > >              CPUID_7_0_EBX_SMEP |
> > >              CPUID_7_0_EBX_ERMS |
> > > -            CPUID_7_0_EBX_MPX |  /* missing bits 13, 15 */
> > >              CPUID_7_0_EBX_RDSEED |
> > >              CPUID_7_0_EBX_SMAP | CPUID_7_0_EBX_CLFLUSHOPT |
> > >              CPUID_7_0_EBX_CLWB |
> > > -- 
> > > 2.19.1
> > > 
> > 
> > 
> 

-- 
Eduardo