Re: [Xen-devel] [PATCH 3/3] Free allocated resource on error

[Julien Grall <julien.grall@xxxxxxx>]

Hi Artem,

On 09/10/2019 15:20, Artem Mygaiev wrote:
> Also do not set mark device as SMMU protected when there are no more
> SMMU resources left
This is a sanity check on the content of the node, not a lack of resource in
this case.

TBH, the current placement is probably not correct. But I am not convinced the 
new placement is better.
Xen 4.12 and earlier will ignore any failure and continue, so we cannot use this 
device at all. Indeed, Xen will configure the SMMU to deny any transaction. If 
you fail to initialize the device, then it will be in an unusable state. In this 
case, we don't want a domain (including Dom0) to use it at all. This could be 
achieved by calling dt_device_set_protected.
Xen 4.13 will stop booting if any of the SMMU fails to configure (this include 
Master Device cannot be assigned). So there are no difference there.
My preference is to cater for all Xen versions so we can consider the patch for 
a backport and potentially revert of the Xen 4.13 behavior (i.e. crashing when 
one IOMMU is not correctly setup). So we would need to move the call at the 
beginning of the function.
> Coverity-ID: 1381862
> Signed-off-by: Artem Mygaiev <artem_mygaiev@xxxxxxxx>
> ---
>   xen/drivers/passthrough/arm/smmu.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index f151b9f5b5..cf42335eed 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -804,9 +804,6 @@ static int register_smmu_master(struct arm_smmu_device 
> *smmu,
>         master->of_node                      = masterspec->np;
>         master->cfg.num_streamids    = masterspec->args_count;
>   
> -	/* Xen: Let Xen know that the device is protected by an SMMU */
> -       dt_device_set_protected(masterspec->np);
> -
>         for (i = 0; i < master->cfg.num_streamids; ++i) {
>                 u16 streamid = masterspec->args[i];
>   
> @@ -815,10 +812,15 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>                         dev_err(dev,
>                                 "stream ID for master device %s greater than maximum 
> allowed (%d)\n",
>                                 masterspec->np->name, smmu->num_mapping_groups);
> +                       devm_free(master);
>                         return -ERANGE;
>                 }
>                 master->cfg.streamids[i] = streamid;
>         }
> +
> +        /* Xen: Let Xen know that the device is protected by an SMMU */
> +        dt_device_set_protected(masterspec->np);
This code is using Linux coding style, so it should be hard tab here.

> +
>         return insert_smmu_master(smmu, master);
>   }
>   
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel