Re: [XEN][PATCH v9 14/19] common/device_tree: Add rwlock for dt_host

[Julien Grall <julien@xxxxxxx>]

Hi,

On 25/08/2023 08:52, Vikram Garhwal wrote:
> Hi,
> On Thu, Aug 24, 2023 at 11:22:00PM -0700, Vikram Garhwal wrote:
> > Hi Julien,
> > On Wed, Aug 23, 2023 at 11:06:59PM +0100, Julien Grall wrote:
> > > Hi Vikram,
> > >
> > > On 19/08/2023 01:28, Vikram Garhwal wrote:
> > > >    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
> > > Typo: I think you want to write 'functions'
> > >
> > > >    conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> > > >    writer will be added in the follow-up patch titled "xen/arm: Implement device
> > > >    tree node addition functionalities."
> > > I would prefer if we avoid mention the name of the follow-up commit. This
> > > will reduce the risk that the name of the commit is incorrect if we decide
> > > to commit this patch before the rest of the series is ready.
> > >
> > > Also, the commit message seems to be indented. Was it intended?
> > >
> > > >    Reason behind adding rwlock instead of spinlock:
> > > >       For now, dynamic programming is the sole modifier of dt_host in Xen during
> > > >           run time. All other access functions like iommu_release_dt_device() 
> > > > are
> > > >           just reading the dt_host during run-time. So, there is a need to 
> > > > protect
> > > >           others from browsing the dt_host while dynamic programming is 
> > > > modifying
> > > >           it. rwlock is better suitable for this task as spinlock won't be able 
> > > > to
> > > >           differentiate between read and write access.
> > > The indentation looks odd here as well.
> > Changed above comments in v10.
> > > > Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> > > >
> > > > ---
> > > > Changes from v7:
> > > >       Keep one lock for dt_host instead of lock for each node under dt_host.
> > > > ---
> > > > ---
> > > >    xen/common/device_tree.c              |  5 +++++
> > > >    xen/drivers/passthrough/device_tree.c | 15 +++++++++++++++
> > > >    xen/include/xen/device_tree.h         |  6 ++++++
> > > >    3 files changed, 26 insertions(+)
> > > I am not sue where to put the comment. I noticed that you didn't touch
> > > iommu_remove_dt_device() and iommu_add_dt_device(). Does this mean the
> > > caller is expected to held the lock? If so, then this should be documented
> > > and an ASSERT() should be added.
> > Added ASSERT in iommu_(add,remove,assign and deassign)_dt_device(),
> iommu_add_ and iommu_assign_ are called at boot time. Also, only other callers
> are handle_device via overlays and iommu_do_dt_domctl() which will hold the
> dt_host_lock. 
The goal of the ASSERT() is to confirm that this holds true today and in 
the future.
> Will look into it more but for now sending v10 with ASSER in these
> two functions.
You could have at least written a comment on top... Regarding the boot 
function, I would consider to take the lock there too.
Otherwise, you could use:

ASSERT(system_state <= SYS_STATE_active || check lock);

Cheers,

--
Julien Grall