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 <[email protected]>
> Reviewed-by: Michal Orzel <[email protected]>
> ---
> 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
