RE: [XEN][PATCH v5 12/17] common/device_tree: Add rwlock for dt_host

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Vikram,

> -----Original Message-----
> Subject: [XEN][PATCH v5 12/17] common/device_tree: Add rwlock for dt_host
> 
>  Dynamic programming ops will modify the dt_host and there might be other
>  function which are browsing the dt_host at the same time. To avoid the race
>  conditions, adding rwlock for browsing the dt_host during runtime.

For clarity, could you please add a little bit more details to explain why you 
chose
rwlock instead of normal spinlock?

> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> ---
>  xen/common/device_tree.c              |  3 +++
>  xen/drivers/passthrough/device_tree.c | 39 +++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h         |  6 +++++
>  3 files changed, 48 insertions(+)
> 
>          if ( ret )
> +        {
>              printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign
> \"%s\""
>                     " to dom%u failed (%d)\n",
>                     dt_node_full_name(dev), d->domain_id, ret);
> +        }

I am not sure if it is necessary to add "{" and "}" here.

> +
> +        read_unlock(&dt_host->lock);
>          break;
> 
>      case XEN_DOMCTL_deassign_device:
> @@ -322,25 +345,41 @@ int iommu_do_dt_domctl(struct xen_domctl
> *domctl, struct domain *d,
>          if ( domctl->u.assign_device.flags )
>              break;
> 
> +        read_lock(&dt_host->lock);
> +
>          ret = dt_find_node_by_gpath(domctl->u.assign_device.u.dt.path,
>                                      domctl->u.assign_device.u.dt.size,
>                                      &dev);
>          if ( ret )
> +        {
> +            read_unlock(&dt_host->lock);

I think instead of adding "read_unlock" in every break and return path,
you can...

>              break;
> +        }
> 
>          ret = xsm_deassign_dtdevice(XSM_HOOK, d, dt_node_full_name(dev));
> +
>          if ( ret )
> +        {
> +            read_unlock(&dt_host->lock);
>              break;
> +        }
> 
>          if ( d == dom_io )
> +        {
> +            read_unlock(&dt_host->lock);
>              return -EINVAL;

...do something like:

ret = -EINVAL;
break;

here, and then add one single "read_unlock" before the "return ret;"
in the bottom of the function?

> +        }
> 
>          ret = iommu_deassign_dt_device(d, dev);
> 
>          if ( ret )
> +        {
>              printk(XENLOG_G_ERR "XEN_DOMCTL_assign_dt_device: assign
> \"%s\""
>                     " to dom%u failed (%d)\n",
>                     dt_node_full_name(dev), d->domain_id, ret);
> +        }

Same here. I am not sure if it is necessary to add "{" and "}".

Kind regards,
Henry