Re: [PATCH] xen/arm: smmuv1: Fixed stream matching register allocation

[Julien Grall <julien@xxxxxxx>]

On 02/07/2021 10:54, Rahul Singh wrote:
> Hi Julien,
Hi Rahul,

> > On 30 Jun 2021, at 7:00 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> >
> >
> > On 30/06/2021 18:49, Rahul Singh wrote:
> > > Hi Julien,
> > Hi,
> >
> > > > On 30 Jun 2021, at 10:09 am, Julien Grall <julien@xxxxxxx> wrote:
> > > >
> > > > Hi Rahul,
> > > >
> > > > On 25/06/2021 17:37, Rahul Singh wrote:
> > > > > SMR allocation should be based on the number of supported stream
> > > > > matching register for each SMMU device.
> > > > > Issue introduced by commit 5e08586afbb90b2e2d56c175c07db77a4afa873c
> > > > > when backported the patches from Linux to XEN to fix the stream match
> > > > > conflict issue when two devices have the same stream-id.
> > > > > Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > Tested-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > > > ---
> > > > >   xen/drivers/passthrough/arm/smmu.c | 3 ++-
> > > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > > > > b/xen/drivers/passthrough/arm/smmu.c
> > > > > index d9a3a0cbf6..da2cd457d7 100644
> > > > > --- a/xen/drivers/passthrough/arm/smmu.c
> > > > > +++ b/xen/drivers/passthrough/arm/smmu.c
> > > > > @@ -149,6 +149,7 @@ typedef enum irqreturn irqreturn_t;
> > > > >   #define kzalloc(size, flags)          _xzalloc(size, sizeof(void *))
> > > > >   #define devm_kzalloc(dev, size, flags)        _xzalloc(size, sizeof(void *))
> > > > >   #define kmalloc_array(size, n, flags) _xmalloc_array(size, sizeof(void *), n)
> > > > > +#define kzalloc_array(size, n, flags)  _xzalloc_array(size, sizeof(void *), n)
> > > > >     static void __iomem *devm_ioremap_resource(struct device *dev,
> > > > >                                            struct resource *res)
> > > > > @@ -2221,7 +2222,7 @@ static int arm_smmu_device_cfg_probe(struct 
> > > > > arm_smmu_device *smmu)
> > > > >                 smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
> > > > >                 /* Zero-initialised to mark as invalid */
> > > > > -               smmu->smrs = devm_kzalloc(smmu->dev, sizeof(*smmu->smrs), 
> > > > > GFP_KERNEL);
> > > > > +               smmu->smrs = kzalloc_array(sizeof(*smmu->smrs), size, 
> > > > > GFP_KERNEL);
> > > > I noticed this is already in... However, I am a bit puzzled into why this was 
> > > > switched devm_kzalloc() to kzalloc_array(). This doesn't matter for Xen as they 
> > > > are just wrappers to x*alloc() but a mention in the commit message would have 
> > > > been useful.
> > > Yes we can use the devm_kzalloc(..) but then we have to pass 
> > > (sizeof(*smmu->smrs) * size ) as size argument to devm_kzalloc(..)
> > > I thought for better code readability I will use kzalloc_array() as the 
> > > function name suggests we are allocating memory for an array.
> > My point is devm_k*alloc() and k*alloc() are quite different on the paper. One 
> > will allocate memory for a given device while the other is unknown memory.
> >
> > It would have been better to call the function devm_kzalloc_array() to keep to 
> > keep the code coherent. Can you please send a patch to make the switch?
> Ok. I will modify the code as per your request as below . I will use 
> devm_kcalloc(..) as this will be more coherent.
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index da2cd457d7..658c40433c 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -149,7 +149,8 @@ typedef enum irqreturn irqreturn_t;
>   #define kzalloc(size, flags)           _xzalloc(size, sizeof(void *))
>   #define devm_kzalloc(dev, size, flags) _xzalloc(size, sizeof(void *))
>   #define kmalloc_array(size, n, flags)  _xmalloc_array(size, sizeof(void *), n)
> -#define kzalloc_array(size, n, flags)  _xzalloc_array(size, sizeof(void *), n)
> +#define devm_kcalloc(dev, n, size, flags)                      \
> +       _xzalloc_array(size, sizeof(void *), n)
>   
>   static void __iomem *devm_ioremap_resource(struct device *dev,
>                                             struct resource *res)
> @@ -2222,7 +2223,8 @@ static int arm_smmu_device_cfg_probe(struct 
> arm_smmu_device *smmu)
>                  smmu->smr_mask_mask = smr >> SMR_MASK_SHIFT;
>   
>                  /* Zero-initialised to mark as invalid */
> -               smmu->smrs = kzalloc_array(sizeof(*smmu->smrs), size, 
> GFP_KERNEL);
> +               smmu->smrs = devm_kcalloc(smmu->dev, size, sizeof(*smmu->smrs),
> +                                                               GFP_KERNEL);
>                  if (!smmu->smrs)
>                          return -ENOMEM;
This sounds good to me.

Cheers,

--
Julien Grall