Re: [PATCH v3 10/16] xen/arm: cpufeature: Provide an helper to check if a capability is supported

[Stefano Stabellini <sstabellini@kernel.org>]

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>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel