| Message ID | 33f39e7f521e6f73a0dba57a8be9fb50656e1807.1607524536.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Emulate ID registers | expand |
On Wed, 9 Dec 2020, Bertrand Marquis wrote: > Create a cpuinfo structure for guest and mask into it the features that > we do not support in Xen or that we do not want to publish to guests. > > Modify some values in the cpuinfo structure for guests to mask some > features which we do not want to allow to guests (like AMU) or we do not > support (like SVE). > > The code is trying to group together registers modifications for the > same feature to be able in the long term to easily enable/disable a > feature depending on user parameters or add other registers modification > in the same place (like enabling/disabling HCR bits). > > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Changes in V2: Rebase > Changes in V3: > Use current_cpu_data info instead of recalling identify_cpu > > --- > xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ > xen/include/asm-arm/cpufeature.h | 2 ++ > 2 files changed, 53 insertions(+) > > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c > index bc7ee5ac95..7255383504 100644 > --- a/xen/arch/arm/cpufeature.c > +++ b/xen/arch/arm/cpufeature.c > @@ -24,6 +24,8 @@ > > DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); > > +struct cpuinfo_arm __read_mostly guest_cpuinfo; > + > void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > const char *info) > { > @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) > #endif > } > > +/* > + * This function is creating a cpuinfo structure with values modified to mask > + * all cpu features that should not be published to guest. > + * The created structure is then used to provide ID registers values to guests. > + */ > +static int __init create_guest_cpuinfo(void) > +{ > + /* > + * TODO: The code is currently using only the features detected on the boot > + * core. In the long term we should try to compute values containing only > + * features supported by all cores. > + */ > + guest_cpuinfo = current_cpu_data; > + > +#ifdef CONFIG_ARM_64 > + /* Disable MPAM as xen does not support it */ > + guest_cpuinfo.pfr64.mpam = 0; > + guest_cpuinfo.pfr64.mpam_frac = 0; > + > + /* Disable SVE as Xen does not support it */ > + guest_cpuinfo.pfr64.sve = 0; > + guest_cpuinfo.zfr64.bits[0] = 0; > + > + /* Disable MTE as Xen does not support it */ > + guest_cpuinfo.pfr64.mte = 0; > +#endif > + > + /* Disable AMU */ > +#ifdef CONFIG_ARM_64 > + guest_cpuinfo.pfr64.amu = 0; > +#endif > + guest_cpuinfo.pfr32.amu = 0; > + > + /* Disable RAS as Xen does not support it */ > +#ifdef CONFIG_ARM_64 > + guest_cpuinfo.pfr64.ras = 0; > + guest_cpuinfo.pfr64.ras_frac = 0; > +#endif > + guest_cpuinfo.pfr32.ras = 0; > + guest_cpuinfo.pfr32.ras_frac = 0; > + > + return 0; > +} > +/* > + * This function needs to be run after all smp are started to have > + * cpuinfo structures for all cores. > + */ > +__initcall(create_guest_cpuinfo); > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index 6cf83d775b..10b62bd324 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -283,6 +283,8 @@ extern void identify_cpu(struct cpuinfo_arm *); > extern struct cpuinfo_arm cpu_data[]; > #define current_cpu_data cpu_data[smp_processor_id()] > > +extern struct cpuinfo_arm guest_cpuinfo; > + > #endif /* __ASSEMBLY__ */ > > #endif > -- > 2.17.1 >
Hi Bertand, On 09/12/2020 16:30, Bertrand Marquis wrote: > Create a cpuinfo structure for guest and mask into it the features that > we do not support in Xen or that we do not want to publish to guests. > > Modify some values in the cpuinfo structure for guests to mask some > features which we do not want to allow to guests (like AMU) or we do not > support (like SVE). > > The code is trying to group together registers modifications for the > same feature to be able in the long term to easily enable/disable a > feature depending on user parameters or add other registers modification > in the same place (like enabling/disabling HCR bits). > > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > Changes in V2: Rebase > Changes in V3: > Use current_cpu_data info instead of recalling identify_cpu > > --- > xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ > xen/include/asm-arm/cpufeature.h | 2 ++ > 2 files changed, 53 insertions(+) > > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c > index bc7ee5ac95..7255383504 100644 > --- a/xen/arch/arm/cpufeature.c > +++ b/xen/arch/arm/cpufeature.c > @@ -24,6 +24,8 @@ > > DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); > > +struct cpuinfo_arm __read_mostly guest_cpuinfo; > + > void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > const char *info) > { > @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) > #endif > } > > +/* > + * This function is creating a cpuinfo structure with values modified to mask > + * all cpu features that should not be published to guest. > + * The created structure is then used to provide ID registers values to guests. > + */ > +static int __init create_guest_cpuinfo(void) > +{ > + /* > + * TODO: The code is currently using only the features detected on the boot > + * core. In the long term we should try to compute values containing only > + * features supported by all cores. > + */ > + guest_cpuinfo = current_cpu_data; It would be more logical to use boot_cpu_data as this would be easier to match with your comment. > + > +#ifdef CONFIG_ARM_64 > + /* Disable MPAM as xen does not support it */ > + guest_cpuinfo.pfr64.mpam = 0; > + guest_cpuinfo.pfr64.mpam_frac = 0; > + > + /* Disable SVE as Xen does not support it */ > + guest_cpuinfo.pfr64.sve = 0; > + guest_cpuinfo.zfr64.bits[0] = 0; > + > + /* Disable MTE as Xen does not support it */ > + guest_cpuinfo.pfr64.mte = 0; > +#endif > + > + /* Disable AMU */ > +#ifdef CONFIG_ARM_64 > + guest_cpuinfo.pfr64.amu = 0; > +#endif > + guest_cpuinfo.pfr32.amu = 0; > + > + /* Disable RAS as Xen does not support it */ > +#ifdef CONFIG_ARM_64 > + guest_cpuinfo.pfr64.ras = 0; > + guest_cpuinfo.pfr64.ras_frac = 0; > +#endif > + guest_cpuinfo.pfr32.ras = 0; > + guest_cpuinfo.pfr32.ras_frac = 0; How about all the fields that are currently marked as RES0/RES1? Shouldn't we make sure they will stay like that even if newer architecture use them? > + > + return 0; > +} > +/* > + * This function needs to be run after all smp are started to have > + * cpuinfo structures for all cores. > + */ > +__initcall(create_guest_cpuinfo); > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index 6cf83d775b..10b62bd324 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -283,6 +283,8 @@ extern void identify_cpu(struct cpuinfo_arm *); > extern struct cpuinfo_arm cpu_data[]; > #define current_cpu_data cpu_data[smp_processor_id()] > > +extern struct cpuinfo_arm guest_cpuinfo; > + > #endif /* __ASSEMBLY__ */ > > #endif >
Hi,
On 09/12/2020 16:30, Bertrand Marquis wrote:
> + /* Disable MPAM as xen does not support it */
I am going to be picky :). I think we want to say "hide" rather than
"disable" because the latter is done differently via the HCR_EL2.
Cheers,
Hi Julien, > On 9 Dec 2020, at 23:09, Julien Grall <julien@xen.org> wrote: > > Hi Bertand, > > On 09/12/2020 16:30, Bertrand Marquis wrote: >> Create a cpuinfo structure for guest and mask into it the features that >> we do not support in Xen or that we do not want to publish to guests. >> Modify some values in the cpuinfo structure for guests to mask some >> features which we do not want to allow to guests (like AMU) or we do not >> support (like SVE). >> The code is trying to group together registers modifications for the >> same feature to be able in the long term to easily enable/disable a >> feature depending on user parameters or add other registers modification >> in the same place (like enabling/disabling HCR bits). >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> --- >> Changes in V2: Rebase >> Changes in V3: >> Use current_cpu_data info instead of recalling identify_cpu >> --- >> xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/cpufeature.h | 2 ++ >> 2 files changed, 53 insertions(+) >> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >> index bc7ee5ac95..7255383504 100644 >> --- a/xen/arch/arm/cpufeature.c >> +++ b/xen/arch/arm/cpufeature.c >> @@ -24,6 +24,8 @@ >> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >> +struct cpuinfo_arm __read_mostly guest_cpuinfo; >> + >> void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >> const char *info) >> { >> @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) >> #endif >> } >> +/* >> + * This function is creating a cpuinfo structure with values modified to mask >> + * all cpu features that should not be published to guest. >> + * The created structure is then used to provide ID registers values to guests. >> + */ >> +static int __init create_guest_cpuinfo(void) >> +{ >> + /* >> + * TODO: The code is currently using only the features detected on the boot >> + * core. In the long term we should try to compute values containing only >> + * features supported by all cores. >> + */ >> + guest_cpuinfo = current_cpu_data; > > It would be more logical to use boot_cpu_data as this would be easier to match with your comment. Agree, I will fix that in V4. > >> + >> +#ifdef CONFIG_ARM_64 >> + /* Disable MPAM as xen does not support it */ >> + guest_cpuinfo.pfr64.mpam = 0; >> + guest_cpuinfo.pfr64.mpam_frac = 0; >> + >> + /* Disable SVE as Xen does not support it */ >> + guest_cpuinfo.pfr64.sve = 0; >> + guest_cpuinfo.zfr64.bits[0] = 0; >> + >> + /* Disable MTE as Xen does not support it */ >> + guest_cpuinfo.pfr64.mte = 0; >> +#endif >> + >> + /* Disable AMU */ >> +#ifdef CONFIG_ARM_64 >> + guest_cpuinfo.pfr64.amu = 0; >> +#endif >> + guest_cpuinfo.pfr32.amu = 0; >> + >> + /* Disable RAS as Xen does not support it */ >> +#ifdef CONFIG_ARM_64 >> + guest_cpuinfo.pfr64.ras = 0; >> + guest_cpuinfo.pfr64.ras_frac = 0; >> +#endif >> + guest_cpuinfo.pfr32.ras = 0; >> + guest_cpuinfo.pfr32.ras_frac = 0; > > How about all the fields that are currently marked as RES0/RES1? Shouldn't we make sure they will stay like that even if newer architecture use them? Definitely we can do more then this here (including allowing to enable some things for dom0 or for test reasons). This is a first try to solve current issues with MPAM and SVE and I plan to continue to enhance this in the future to enable more customisation here. I do think we could do a bit more here to have some features controlled by the user but this will need a bit of discussion to agree on a strategy. Could we agree to keep the current scope for this serie (to have this in next release) and work then on future enhancements like this ? Cheers Bertrand > >> + >> + return 0; >> +} >> +/* >> + * This function needs to be run after all smp are started to have >> + * cpuinfo structures for all cores. >> + */ >> +__initcall(create_guest_cpuinfo); >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h >> index 6cf83d775b..10b62bd324 100644 >> --- a/xen/include/asm-arm/cpufeature.h >> +++ b/xen/include/asm-arm/cpufeature.h >> @@ -283,6 +283,8 @@ extern void identify_cpu(struct cpuinfo_arm *); >> extern struct cpuinfo_arm cpu_data[]; >> #define current_cpu_data cpu_data[smp_processor_id()] >> +extern struct cpuinfo_arm guest_cpuinfo; >> + >> #endif /* __ASSEMBLY__ */ >> #endif > > -- > Julien Grall
Hi, > On 9 Dec 2020, at 23:22, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 09/12/2020 16:30, Bertrand Marquis wrote: >> + /* Disable MPAM as xen does not support it */ > > I am going to be picky :). I think we want to say "hide" rather than "disable" because the latter is done differently via the HCR_EL2. That does make sense as we are not really disabling but hiding you are right. I will fix that in V4. Cheers Bertrand > > Cheers, > > -- > Julien Grall
On 10/12/2020 15:48, Bertrand Marquis wrote: > Hi Julien, > >> On 9 Dec 2020, at 23:09, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertand, >> >> On 09/12/2020 16:30, Bertrand Marquis wrote: >>> Create a cpuinfo structure for guest and mask into it the features that >>> we do not support in Xen or that we do not want to publish to guests. >>> Modify some values in the cpuinfo structure for guests to mask some >>> features which we do not want to allow to guests (like AMU) or we do not >>> support (like SVE). >>> The code is trying to group together registers modifications for the >>> same feature to be able in the long term to easily enable/disable a >>> feature depending on user parameters or add other registers modification >>> in the same place (like enabling/disabling HCR bits). >>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>> --- >>> Changes in V2: Rebase >>> Changes in V3: >>> Use current_cpu_data info instead of recalling identify_cpu >>> --- >>> xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ >>> xen/include/asm-arm/cpufeature.h | 2 ++ >>> 2 files changed, 53 insertions(+) >>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >>> index bc7ee5ac95..7255383504 100644 >>> --- a/xen/arch/arm/cpufeature.c >>> +++ b/xen/arch/arm/cpufeature.c >>> @@ -24,6 +24,8 @@ >>> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >>> +struct cpuinfo_arm __read_mostly guest_cpuinfo; >>> + >>> void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >>> const char *info) >>> { >>> @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) >>> #endif >>> } >>> +/* >>> + * This function is creating a cpuinfo structure with values modified to mask >>> + * all cpu features that should not be published to guest. >>> + * The created structure is then used to provide ID registers values to guests. >>> + */ >>> +static int __init create_guest_cpuinfo(void) >>> +{ >>> + /* >>> + * TODO: The code is currently using only the features detected on the boot >>> + * core. In the long term we should try to compute values containing only >>> + * features supported by all cores. >>> + */ >>> + guest_cpuinfo = current_cpu_data; >> >> It would be more logical to use boot_cpu_data as this would be easier to match with your comment. > > Agree, I will fix that in V4. > >> >>> + >>> +#ifdef CONFIG_ARM_64 >>> + /* Disable MPAM as xen does not support it */ >>> + guest_cpuinfo.pfr64.mpam = 0; >>> + guest_cpuinfo.pfr64.mpam_frac = 0; >>> + >>> + /* Disable SVE as Xen does not support it */ >>> + guest_cpuinfo.pfr64.sve = 0; >>> + guest_cpuinfo.zfr64.bits[0] = 0; >>> + >>> + /* Disable MTE as Xen does not support it */ >>> + guest_cpuinfo.pfr64.mte = 0; >>> +#endif >>> + >>> + /* Disable AMU */ >>> +#ifdef CONFIG_ARM_64 >>> + guest_cpuinfo.pfr64.amu = 0; >>> +#endif >>> + guest_cpuinfo.pfr32.amu = 0; >>> + >>> + /* Disable RAS as Xen does not support it */ >>> +#ifdef CONFIG_ARM_64 >>> + guest_cpuinfo.pfr64.ras = 0; >>> + guest_cpuinfo.pfr64.ras_frac = 0; >>> +#endif >>> + guest_cpuinfo.pfr32.ras = 0; >>> + guest_cpuinfo.pfr32.ras_frac = 0; >> >> How about all the fields that are currently marked as RES0/RES1? Shouldn't we make sure they will stay like that even if newer architecture use them? > > Definitely we can do more then this here (including allowing to enable some things for dom0 or for test reasons). > This is a first try to solve current issues with MPAM and SVE and I plan to continue to enhance this in the future > to enable more customisation here. > I do think we could do a bit more here to have some features controlled by the user but this will need a bit of > discussion to agree on a strategy. I think you misunderstood my comment. I am not asking whether we want to customize the value per domain. Instead, I am raising questions for the strategy taken in this patch. I am going to leave the safety aside, because I think this is orthogonal to this patch. This patch is introducing a deny list. This means that all the features will be exposed to a domain unless someone determine that this is not supported by Xen. This means we will always try to catch up with what Arm decided to invent and attempt to fix it before the silicon is out. Instead, I think it would be better to use an allow list. We should only expose features to the guest we know works (this could possibly be just the Armv8.0 one). Cheers,
Hi Julien, > On 10 Dec 2020, at 16:05, Julien Grall <julien@xen.org> wrote: > > > > On 10/12/2020 15:48, Bertrand Marquis wrote: >> Hi Julien, >>> On 9 Dec 2020, at 23:09, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertand, >>> >>> On 09/12/2020 16:30, Bertrand Marquis wrote: >>>> Create a cpuinfo structure for guest and mask into it the features that >>>> we do not support in Xen or that we do not want to publish to guests. >>>> Modify some values in the cpuinfo structure for guests to mask some >>>> features which we do not want to allow to guests (like AMU) or we do not >>>> support (like SVE). >>>> The code is trying to group together registers modifications for the >>>> same feature to be able in the long term to easily enable/disable a >>>> feature depending on user parameters or add other registers modification >>>> in the same place (like enabling/disabling HCR bits). >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>> --- >>>> Changes in V2: Rebase >>>> Changes in V3: >>>> Use current_cpu_data info instead of recalling identify_cpu >>>> --- >>>> xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ >>>> xen/include/asm-arm/cpufeature.h | 2 ++ >>>> 2 files changed, 53 insertions(+) >>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >>>> index bc7ee5ac95..7255383504 100644 >>>> --- a/xen/arch/arm/cpufeature.c >>>> +++ b/xen/arch/arm/cpufeature.c >>>> @@ -24,6 +24,8 @@ >>>> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >>>> +struct cpuinfo_arm __read_mostly guest_cpuinfo; >>>> + >>>> void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >>>> const char *info) >>>> { >>>> @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) >>>> #endif >>>> } >>>> +/* >>>> + * This function is creating a cpuinfo structure with values modified to mask >>>> + * all cpu features that should not be published to guest. >>>> + * The created structure is then used to provide ID registers values to guests. >>>> + */ >>>> +static int __init create_guest_cpuinfo(void) >>>> +{ >>>> + /* >>>> + * TODO: The code is currently using only the features detected on the boot >>>> + * core. In the long term we should try to compute values containing only >>>> + * features supported by all cores. >>>> + */ >>>> + guest_cpuinfo = current_cpu_data; >>> >>> It would be more logical to use boot_cpu_data as this would be easier to match with your comment. >> Agree, I will fix that in V4. >>> >>>> + >>>> +#ifdef CONFIG_ARM_64 >>>> + /* Disable MPAM as xen does not support it */ >>>> + guest_cpuinfo.pfr64.mpam = 0; >>>> + guest_cpuinfo.pfr64.mpam_frac = 0; >>>> + >>>> + /* Disable SVE as Xen does not support it */ >>>> + guest_cpuinfo.pfr64.sve = 0; >>>> + guest_cpuinfo.zfr64.bits[0] = 0; >>>> + >>>> + /* Disable MTE as Xen does not support it */ >>>> + guest_cpuinfo.pfr64.mte = 0; >>>> +#endif >>>> + >>>> + /* Disable AMU */ >>>> +#ifdef CONFIG_ARM_64 >>>> + guest_cpuinfo.pfr64.amu = 0; >>>> +#endif >>>> + guest_cpuinfo.pfr32.amu = 0; >>>> + >>>> + /* Disable RAS as Xen does not support it */ >>>> +#ifdef CONFIG_ARM_64 >>>> + guest_cpuinfo.pfr64.ras = 0; >>>> + guest_cpuinfo.pfr64.ras_frac = 0; >>>> +#endif >>>> + guest_cpuinfo.pfr32.ras = 0; >>>> + guest_cpuinfo.pfr32.ras_frac = 0; >>> >>> How about all the fields that are currently marked as RES0/RES1? Shouldn't we make sure they will stay like that even if newer architecture use them? >> Definitely we can do more then this here (including allowing to enable some things for dom0 or for test reasons). >> This is a first try to solve current issues with MPAM and SVE and I plan to continue to enhance this in the future >> to enable more customisation here. >> I do think we could do a bit more here to have some features controlled by the user but this will need a bit of >> discussion to agree on a strategy. > > I think you misunderstood my comment. I am not asking whether we want to customize the value per domain. Instead, I am raising questions for the strategy taken in this patch. > > I am going to leave the safety aside, because I think this is orthogonal to this patch. > > This patch is introducing a deny list. This means that all the features will be exposed to a domain unless someone determine that this is not > supported by Xen. > > This means we will always try to catch up with what Arm decided to invent and attempt to fix it before the silicon is out. > > Instead, I think it would be better to use an allow list. We should only expose features to the guest we know works (this could possibly be just the Armv8.0 one). I understood that and I fully agree that this is what we should do: only expose what we support and know and let everything else as “disabled”. And I definitely want to do that and I think with this serie we have the required support to do that, we will need to rework how we initialise the guest_cpuinfo structure. I just want to leave this discussion for after so that we can at least right now have a current linux booting without the need to modify the linux kernel binary to remove things like SVE. Regards Bertrand > > Cheers, > > -- > Julien Grall
On 10/12/2020 16:17, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, >> On 10 Dec 2020, at 16:05, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 10/12/2020 15:48, Bertrand Marquis wrote: >>> Hi Julien, >>>> On 9 Dec 2020, at 23:09, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Bertand, >>>> >>>> On 09/12/2020 16:30, Bertrand Marquis wrote: >>>>> Create a cpuinfo structure for guest and mask into it the features that >>>>> we do not support in Xen or that we do not want to publish to guests. >>>>> Modify some values in the cpuinfo structure for guests to mask some >>>>> features which we do not want to allow to guests (like AMU) or we do not >>>>> support (like SVE). >>>>> The code is trying to group together registers modifications for the >>>>> same feature to be able in the long term to easily enable/disable a >>>>> feature depending on user parameters or add other registers modification >>>>> in the same place (like enabling/disabling HCR bits). >>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>>> --- >>>>> Changes in V2: Rebase >>>>> Changes in V3: >>>>> Use current_cpu_data info instead of recalling identify_cpu >>>>> --- >>>>> xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ >>>>> xen/include/asm-arm/cpufeature.h | 2 ++ >>>>> 2 files changed, 53 insertions(+) >>>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >>>>> index bc7ee5ac95..7255383504 100644 >>>>> --- a/xen/arch/arm/cpufeature.c >>>>> +++ b/xen/arch/arm/cpufeature.c >>>>> @@ -24,6 +24,8 @@ >>>>> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >>>>> +struct cpuinfo_arm __read_mostly guest_cpuinfo; >>>>> + >>>>> void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >>>>> const char *info) >>>>> { >>>>> @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) >>>>> #endif >>>>> } >>>>> +/* >>>>> + * This function is creating a cpuinfo structure with values modified to mask >>>>> + * all cpu features that should not be published to guest. >>>>> + * The created structure is then used to provide ID registers values to guests. >>>>> + */ >>>>> +static int __init create_guest_cpuinfo(void) >>>>> +{ >>>>> + /* >>>>> + * TODO: The code is currently using only the features detected on the boot >>>>> + * core. In the long term we should try to compute values containing only >>>>> + * features supported by all cores. >>>>> + */ >>>>> + guest_cpuinfo = current_cpu_data; >>>> >>>> It would be more logical to use boot_cpu_data as this would be easier to match with your comment. >>> Agree, I will fix that in V4. >>>> >>>>> + >>>>> +#ifdef CONFIG_ARM_64 >>>>> + /* Disable MPAM as xen does not support it */ >>>>> + guest_cpuinfo.pfr64.mpam = 0; >>>>> + guest_cpuinfo.pfr64.mpam_frac = 0; >>>>> + >>>>> + /* Disable SVE as Xen does not support it */ >>>>> + guest_cpuinfo.pfr64.sve = 0; >>>>> + guest_cpuinfo.zfr64.bits[0] = 0; >>>>> + >>>>> + /* Disable MTE as Xen does not support it */ >>>>> + guest_cpuinfo.pfr64.mte = 0; >>>>> +#endif >>>>> + >>>>> + /* Disable AMU */ >>>>> +#ifdef CONFIG_ARM_64 >>>>> + guest_cpuinfo.pfr64.amu = 0; >>>>> +#endif >>>>> + guest_cpuinfo.pfr32.amu = 0; >>>>> + >>>>> + /* Disable RAS as Xen does not support it */ >>>>> +#ifdef CONFIG_ARM_64 >>>>> + guest_cpuinfo.pfr64.ras = 0; >>>>> + guest_cpuinfo.pfr64.ras_frac = 0; >>>>> +#endif >>>>> + guest_cpuinfo.pfr32.ras = 0; >>>>> + guest_cpuinfo.pfr32.ras_frac = 0; >>>> >>>> How about all the fields that are currently marked as RES0/RES1? Shouldn't we make sure they will stay like that even if newer architecture use them? >>> Definitely we can do more then this here (including allowing to enable some things for dom0 or for test reasons). >>> This is a first try to solve current issues with MPAM and SVE and I plan to continue to enhance this in the future >>> to enable more customisation here. >>> I do think we could do a bit more here to have some features controlled by the user but this will need a bit of >>> discussion to agree on a strategy. >> >> I think you misunderstood my comment. I am not asking whether we want to customize the value per domain. Instead, I am raising questions for the strategy taken in this patch. >> >> I am going to leave the safety aside, because I think this is orthogonal to this patch. >> >> This patch is introducing a deny list. This means that all the features will be exposed to a domain unless someone determine that this is not >> supported by Xen. >> >> This means we will always try to catch up with what Arm decided to invent and attempt to fix it before the silicon is out. >> >> Instead, I think it would be better to use an allow list. We should only expose features to the guest we know works (this could possibly be just the Armv8.0 one). > > I understood that and I fully agree that this is what we should do: only expose what we support and know and let everything else as “disabled”. > And I definitely want to do that and I think with this serie we have the required support to do that, we will need to rework how we initialise the > guest_cpuinfo structure. > > I just want to leave this discussion for after so that we can at least right now have a current linux booting without the need to modify the linux > kernel binary to remove things like SVE. Ok. So this patch is more to fill the gap rather than the final solution. This should be clarified in the commit message. Although, it is still unclear to me why this can't be an allowlist from the start. As you said, the infrastructure is already there. So it would be a matter of copying bits we know work with Xen (rather than cloberring). Cheers,
Hi Julien, > On 10 Dec 2020, at 16:30, Julien Grall <julien@xen.org> wrote: > > > > On 10/12/2020 16:17, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 10 Dec 2020, at 16:05, Julien Grall <julien@xen.org> wrote: >>> >>> >>> >>> On 10/12/2020 15:48, Bertrand Marquis wrote: >>>> Hi Julien, >>>>> On 9 Dec 2020, at 23:09, Julien Grall <julien@xen.org> wrote: >>>>> >>>>> Hi Bertand, >>>>> >>>>> On 09/12/2020 16:30, Bertrand Marquis wrote: >>>>>> Create a cpuinfo structure for guest and mask into it the features that >>>>>> we do not support in Xen or that we do not want to publish to guests. >>>>>> Modify some values in the cpuinfo structure for guests to mask some >>>>>> features which we do not want to allow to guests (like AMU) or we do not >>>>>> support (like SVE). >>>>>> The code is trying to group together registers modifications for the >>>>>> same feature to be able in the long term to easily enable/disable a >>>>>> feature depending on user parameters or add other registers modification >>>>>> in the same place (like enabling/disabling HCR bits). >>>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>>>> --- >>>>>> Changes in V2: Rebase >>>>>> Changes in V3: >>>>>> Use current_cpu_data info instead of recalling identify_cpu >>>>>> --- >>>>>> xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ >>>>>> xen/include/asm-arm/cpufeature.h | 2 ++ >>>>>> 2 files changed, 53 insertions(+) >>>>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >>>>>> index bc7ee5ac95..7255383504 100644 >>>>>> --- a/xen/arch/arm/cpufeature.c >>>>>> +++ b/xen/arch/arm/cpufeature.c >>>>>> @@ -24,6 +24,8 @@ >>>>>> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >>>>>> +struct cpuinfo_arm __read_mostly guest_cpuinfo; >>>>>> + >>>>>> void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >>>>>> const char *info) >>>>>> { >>>>>> @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) >>>>>> #endif >>>>>> } >>>>>> +/* >>>>>> + * This function is creating a cpuinfo structure with values modified to mask >>>>>> + * all cpu features that should not be published to guest. >>>>>> + * The created structure is then used to provide ID registers values to guests. >>>>>> + */ >>>>>> +static int __init create_guest_cpuinfo(void) >>>>>> +{ >>>>>> + /* >>>>>> + * TODO: The code is currently using only the features detected on the boot >>>>>> + * core. In the long term we should try to compute values containing only >>>>>> + * features supported by all cores. >>>>>> + */ >>>>>> + guest_cpuinfo = current_cpu_data; >>>>> >>>>> It would be more logical to use boot_cpu_data as this would be easier to match with your comment. >>>> Agree, I will fix that in V4. >>>>> >>>>>> + >>>>>> +#ifdef CONFIG_ARM_64 >>>>>> + /* Disable MPAM as xen does not support it */ >>>>>> + guest_cpuinfo.pfr64.mpam = 0; >>>>>> + guest_cpuinfo.pfr64.mpam_frac = 0; >>>>>> + >>>>>> + /* Disable SVE as Xen does not support it */ >>>>>> + guest_cpuinfo.pfr64.sve = 0; >>>>>> + guest_cpuinfo.zfr64.bits[0] = 0; >>>>>> + >>>>>> + /* Disable MTE as Xen does not support it */ >>>>>> + guest_cpuinfo.pfr64.mte = 0; >>>>>> +#endif >>>>>> + >>>>>> + /* Disable AMU */ >>>>>> +#ifdef CONFIG_ARM_64 >>>>>> + guest_cpuinfo.pfr64.amu = 0; >>>>>> +#endif >>>>>> + guest_cpuinfo.pfr32.amu = 0; >>>>>> + >>>>>> + /* Disable RAS as Xen does not support it */ >>>>>> +#ifdef CONFIG_ARM_64 >>>>>> + guest_cpuinfo.pfr64.ras = 0; >>>>>> + guest_cpuinfo.pfr64.ras_frac = 0; >>>>>> +#endif >>>>>> + guest_cpuinfo.pfr32.ras = 0; >>>>>> + guest_cpuinfo.pfr32.ras_frac = 0; >>>>> >>>>> How about all the fields that are currently marked as RES0/RES1? Shouldn't we make sure they will stay like that even if newer architecture use them? >>>> Definitely we can do more then this here (including allowing to enable some things for dom0 or for test reasons). >>>> This is a first try to solve current issues with MPAM and SVE and I plan to continue to enhance this in the future >>>> to enable more customisation here. >>>> I do think we could do a bit more here to have some features controlled by the user but this will need a bit of >>>> discussion to agree on a strategy. >>> >>> I think you misunderstood my comment. I am not asking whether we want to customize the value per domain. Instead, I am raising questions for the strategy taken in this patch. >>> >>> I am going to leave the safety aside, because I think this is orthogonal to this patch. >>> >>> This patch is introducing a deny list. This means that all the features will be exposed to a domain unless someone determine that this is not >>> supported by Xen. >>> >>> This means we will always try to catch up with what Arm decided to invent and attempt to fix it before the silicon is out. >>> >>> Instead, I think it would be better to use an allow list. We should only expose features to the guest we know works (this could possibly be just the Armv8.0 one). >> I understood that and I fully agree that this is what we should do: only expose what we support and know and let everything else as “disabled”. >> And I definitely want to do that and I think with this serie we have the required support to do that, we will need to rework how we initialise the >> guest_cpuinfo structure. >> I just want to leave this discussion for after so that we can at least right now have a current linux booting without the need to modify the linux >> kernel binary to remove things like SVE. > > Ok. So this patch is more to fill the gap rather than the final solution. This should be clarified in the commit message. I can add that in the commit message. > > Although, it is still unclear to me why this can't be an allowlist from the start. As you said, the infrastructure is already there. So it would be a matter of copying bits we know work with Xen (rather than cloberring). The analysis to do that properly might require more work as we should start from everything off and then going one by one and making sure we not only do that here but also in HCR register bits. Here we are just explicitely “hiding” features which is somehow easy to review and check. I will not have time to investigate deeper then what I did already before the next xen release. So best i can do is change this patch to not modify anything in the guest_cpuinfo so that someone else can do that work on top of this serie. I have planned some time to work on continuing this work, but not before march. Regards Bertrand > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c index bc7ee5ac95..7255383504 100644 --- a/xen/arch/arm/cpufeature.c +++ b/xen/arch/arm/cpufeature.c @@ -24,6 +24,8 @@ DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); +struct cpuinfo_arm __read_mostly guest_cpuinfo; + void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, const char *info) { @@ -157,6 +159,55 @@ void identify_cpu(struct cpuinfo_arm *c) #endif } +/* + * This function is creating a cpuinfo structure with values modified to mask + * all cpu features that should not be published to guest. + * The created structure is then used to provide ID registers values to guests. + */ +static int __init create_guest_cpuinfo(void) +{ + /* + * TODO: The code is currently using only the features detected on the boot + * core. In the long term we should try to compute values containing only + * features supported by all cores. + */ + guest_cpuinfo = current_cpu_data; + +#ifdef CONFIG_ARM_64 + /* Disable MPAM as xen does not support it */ + guest_cpuinfo.pfr64.mpam = 0; + guest_cpuinfo.pfr64.mpam_frac = 0; + + /* Disable SVE as Xen does not support it */ + guest_cpuinfo.pfr64.sve = 0; + guest_cpuinfo.zfr64.bits[0] = 0; + + /* Disable MTE as Xen does not support it */ + guest_cpuinfo.pfr64.mte = 0; +#endif + + /* Disable AMU */ +#ifdef CONFIG_ARM_64 + guest_cpuinfo.pfr64.amu = 0; +#endif + guest_cpuinfo.pfr32.amu = 0; + + /* Disable RAS as Xen does not support it */ +#ifdef CONFIG_ARM_64 + guest_cpuinfo.pfr64.ras = 0; + guest_cpuinfo.pfr64.ras_frac = 0; +#endif + guest_cpuinfo.pfr32.ras = 0; + guest_cpuinfo.pfr32.ras_frac = 0; + + return 0; +} +/* + * This function needs to be run after all smp are started to have + * cpuinfo structures for all cores. + */ +__initcall(create_guest_cpuinfo); + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h index 6cf83d775b..10b62bd324 100644 --- a/xen/include/asm-arm/cpufeature.h +++ b/xen/include/asm-arm/cpufeature.h @@ -283,6 +283,8 @@ extern void identify_cpu(struct cpuinfo_arm *); extern struct cpuinfo_arm cpu_data[]; #define current_cpu_data cpu_data[smp_processor_id()] +extern struct cpuinfo_arm guest_cpuinfo; + #endif /* __ASSEMBLY__ */ #endif
Create a cpuinfo structure for guest and mask into it the features that we do not support in Xen or that we do not want to publish to guests. Modify some values in the cpuinfo structure for guests to mask some features which we do not want to allow to guests (like AMU) or we do not support (like SVE). The code is trying to group together registers modifications for the same feature to be able in the long term to easily enable/disable a feature depending on user parameters or add other registers modification in the same place (like enabling/disabling HCR bits). Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- Changes in V2: Rebase Changes in V3: Use current_cpu_data info instead of recalling identify_cpu --- xen/arch/arm/cpufeature.c | 51 ++++++++++++++++++++++++++++++++ xen/include/asm-arm/cpufeature.h | 2 ++ 2 files changed, 53 insertions(+)