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.garh...@amd.com>
---
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
