| Message ID | 1465315583-1278-11-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 7 Jun 2016, Julien Grall wrote: > The CPU capabilities will be set depending on the value found in the CPU > registers. This patch provides a generic to go through a set of capabilities > and find which one should be enabled. > > The parameter "info" is used to display the kind of capability updated (e.g > workaround, feature...). > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v3: > - Patch added. The code was previously part of "Detect > silicon...". > --- > xen/arch/arm/cpufeature.c | 16 ++++++++++++++++ > xen/include/asm-arm/cpufeature.h | 9 +++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c > index 7a1b56b..088625b 100644 > --- a/xen/arch/arm/cpufeature.c > +++ b/xen/arch/arm/cpufeature.c > @@ -24,6 +24,22 @@ > > DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); > > +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > + const char *info) > +{ > + int i; > + > + for ( i = 0; caps[i].matches; i++ ) > + { > + if ( !caps[i].matches(&caps[i]) ) > + continue; > + > + if ( !cpus_have_cap(caps[i].capability) && caps[i].desc ) > + printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc); The patch is OK. I still have the same comment as before: if an info parameter is passed to this function with a string, then I think the string should contain some useful information. Not a generic message. I would just do: printk(XENLOG_INFO "enable workaround for: %s\n", caps[i].desc); If one day we need to change the message depending on the caller, then that day we'll add a parameter to update_cpu_capabilities. > + cpus_set_cap(caps[i].capability); > + } > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index 2bebad1..be2414c 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -62,6 +62,15 @@ static inline void cpus_set_cap(unsigned int num) > __set_bit(num, cpu_hwcaps); > } > > +struct arm_cpu_capabilities { > + const char *desc; > + u16 capability; > + bool_t (*matches)(const struct arm_cpu_capabilities *); > +}; > + > +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > + const char *info); > + > #endif /* __ASSEMBLY__ */ > > #endif > -- > 1.9.1 >
Hi Stefano, On 22/06/16 10:59, Stefano Stabellini wrote: > On Tue, 7 Jun 2016, Julien Grall wrote: >> The CPU capabilities will be set depending on the value found in the CPU >> registers. This patch provides a generic to go through a set of capabilities >> and find which one should be enabled. >> >> The parameter "info" is used to display the kind of capability updated (e.g >> workaround, feature...). >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in v3: >> - Patch added. The code was previously part of "Detect >> silicon...". >> --- >> xen/arch/arm/cpufeature.c | 16 ++++++++++++++++ >> xen/include/asm-arm/cpufeature.h | 9 +++++++++ >> 2 files changed, 25 insertions(+) >> >> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >> index 7a1b56b..088625b 100644 >> --- a/xen/arch/arm/cpufeature.c >> +++ b/xen/arch/arm/cpufeature.c >> @@ -24,6 +24,22 @@ >> >> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >> >> +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >> + const char *info) >> +{ >> + int i; >> + >> + for ( i = 0; caps[i].matches; i++ ) >> + { >> + if ( !caps[i].matches(&caps[i]) ) >> + continue; >> + >> + if ( !cpus_have_cap(caps[i].capability) && caps[i].desc ) >> + printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc); > > The patch is OK. I still have the same comment as before: if an info > parameter is passed to this function with a string, then I think the > string should contain some useful information. Not a generic message. It is useful. As mentioned in the commit message the parameter "info" is used to display the kind of capability updated (e.g workaround, feature, ...). > I would just do: > > printk(XENLOG_INFO "enable workaround for: %s\n", caps[i].desc); > > If one day we need to change the message depending on the caller, then > that day we'll add a parameter to update_cpu_capabilities. We both know that this function will be used for other purpose very soon. So I would rather avoid to modify again in the future. That is why I described the purpose in the commit message. Cheers,
On Wed, 22 Jun 2016, Julien Grall wrote: > Hi Stefano, > > On 22/06/16 10:59, Stefano Stabellini wrote: > > On Tue, 7 Jun 2016, Julien Grall wrote: > > > The CPU capabilities will be set depending on the value found in the CPU > > > registers. This patch provides a generic to go through a set of > > > capabilities > > > and find which one should be enabled. > > > > > > The parameter "info" is used to display the kind of capability updated > > > (e.g > > > workaround, feature...). > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > > --- > > > Changes in v3: > > > - Patch added. The code was previously part of "Detect > > > silicon...". > > > --- > > > xen/arch/arm/cpufeature.c | 16 ++++++++++++++++ > > > xen/include/asm-arm/cpufeature.h | 9 +++++++++ > > > 2 files changed, 25 insertions(+) > > > > > > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c > > > index 7a1b56b..088625b 100644 > > > --- a/xen/arch/arm/cpufeature.c > > > +++ b/xen/arch/arm/cpufeature.c > > > @@ -24,6 +24,22 @@ > > > > > > DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); > > > > > > +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > > > + const char *info) > > > +{ > > > + int i; > > > + > > > + for ( i = 0; caps[i].matches; i++ ) > > > + { > > > + if ( !caps[i].matches(&caps[i]) ) > > > + continue; > > > + > > > + if ( !cpus_have_cap(caps[i].capability) && caps[i].desc ) > > > + printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc); > > > > The patch is OK. I still have the same comment as before: if an info > > parameter is passed to this function with a string, then I think the > > string should contain some useful information. Not a generic message. > > It is useful. As mentioned in the commit message the parameter "info" is used > to display the kind of capability updated (e.g workaround, feature, ...). > > > I would just do: > > > > printk(XENLOG_INFO "enable workaround for: %s\n", caps[i].desc); > > > > If one day we need to change the message depending on the caller, then > > that day we'll add a parameter to update_cpu_capabilities. > > We both know that this function will be used for other purpose very soon. So I > would rather avoid to modify again in the future. That is why I described the > purpose in the commit message. I am unconvinced, but I'll leave it be. Acked-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c index 7a1b56b..088625b 100644 --- a/xen/arch/arm/cpufeature.c +++ b/xen/arch/arm/cpufeature.c @@ -24,6 +24,22 @@ DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, + const char *info) +{ + int i; + + for ( i = 0; caps[i].matches; i++ ) + { + if ( !caps[i].matches(&caps[i]) ) + continue; + + if ( !cpus_have_cap(caps[i].capability) && caps[i].desc ) + printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc); + cpus_set_cap(caps[i].capability); + } +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h index 2bebad1..be2414c 100644 --- a/xen/include/asm-arm/cpufeature.h +++ b/xen/include/asm-arm/cpufeature.h @@ -62,6 +62,15 @@ static inline void cpus_set_cap(unsigned int num) __set_bit(num, cpu_hwcaps); } +struct arm_cpu_capabilities { + const char *desc; + u16 capability; + bool_t (*matches)(const struct arm_cpu_capabilities *); +}; + +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, + const char *info); + #endif /* __ASSEMBLY__ */ #endif
The CPU capabilities will be set depending on the value found in the CPU registers. This patch provides a generic to go through a set of capabilities and find which one should be enabled. The parameter "info" is used to display the kind of capability updated (e.g workaround, feature...). Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v3: - Patch added. The code was previously part of "Detect silicon...". --- xen/arch/arm/cpufeature.c | 16 ++++++++++++++++ xen/include/asm-arm/cpufeature.h | 9 +++++++++ 2 files changed, 25 insertions(+)