Re: [XEN][PATCH v11 14/20] common/device_tree: Add rwlock for dt_host

[Michal Orzel <michal.orzel@xxxxxxx>]

On 01/09/2023 06:59, Vikram Garhwal wrote:
> Dynamic programming ops will modify the dt_host and there might be other
> functions which are browsing the dt_host at the same time. To avoid the race
> conditions, adding rwlock for browsing the dt_host during runtime. dt_host
> writer will be added in the follow-up patch for device tree overlay
> functionalities.
> 
> 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.
> 
> Signed-off-by: Vikram Garhwal <vikram.garhwal@xxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
> Changes from v10:
>     Add ASSERT for iommu_assign_dt_device() and iommu_add_dt_device().
> Changes from v9:
>     Update commit message and fix indentation.
>     Add ASSERT() for iommu_deassign_dt_device() and iommu_remove_dt_device().
>     Fix code styles.
>     Remove rwlock_init in unflatten_device_tree() and do DEFINE_RWLOCK in
>         device-tree.c
> Changes from v7:
>     Keep one lock for dt_host instead of lock for each node under dt_host.
> ---
> ---
>  xen/common/device_tree.c              |  1 +
>  xen/drivers/passthrough/device_tree.c | 28 +++++++++++++++++++++++++--
>  xen/include/xen/device_tree.h         |  7 +++++++
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index f38f51ec0b..b1c2952951 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -31,6 +31,7 @@ dt_irq_xlate_func dt_irq_xlate;
>  struct dt_device_node *dt_host;
>  /* Interrupt controller node*/
>  const struct dt_device_node *dt_interrupt_controller;
> +DEFINE_RWLOCK(dt_host_lock);
>  
>  /**
>   * struct dt_alias_prop - Alias property in 'aliases' node
> diff --git a/xen/drivers/passthrough/device_tree.c 
> b/xen/drivers/passthrough/device_tree.c
> index 80f6efc606..1f9cfccf95 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -31,6 +31,8 @@ int iommu_assign_dt_device(struct domain *d, struct 
> dt_device_node *dev)
>      int rc = -EBUSY;
>      struct domain_iommu *hd = dom_iommu(d);
>  
> +    ASSERT(system_state <= SYS_STATE_active || rw_is_locked(&dt_host_lock));
This looks not right (I know Julien suggested this). The second part will be 
checked only if state > active i.e. suspend/resume.
I think this wants to be:
ASSERT(system_state < SYS_STATE_active || rw_is_locked(&dt_host_lock));
so that once the state is >= active, we require dt_host_lock to be locked.

~Michal