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

[Vikram Garhwal <vikram.garhwal@xxxxxxx>]

Hi Henry,

On 4/13/23 7:09 PM, Henry Wang wrote:
> 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?
rwlock is just to protect someone reading the dt_host while dynamic 
programming is modifying the dt_host.
Initial suggestion was from Julien about adding rwlock here.
For now, dynamic programming is the dt_host writer in Xen during run 
time. All other iommu passthrough function was just readers during 
run-time. So, it was better to go for r/w locks here as spinlock won't 
be able to difference between read and r/w access.
For next version, I will add a comment in commit message.
> > 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.
You are right. Will remove it in next version.
> > +
> > +        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?
Will do.
> > +        }
> >
> >           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