[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN][PATCH v8 09/19] xen/iommu: Move spin_lock from iommu_dt_device_is_assigned to caller



Hi,

On 21/08/2023 20:46, Vikram Garhwal wrote:
Hi Julien
On Fri, Aug 18, 2023 at 09:35:02PM +0100, Julien Grall wrote:
Hi Vikram,

On 18/08/2023 20:52, Vikram Garhwal wrote:
Hi Jan
On Thu, Aug 17, 2023 at 09:05:44AM +0200, Jan Beulich wrote:
On 17.08.2023 02:39, Vikram Garhwal wrote:
--- /dev/null
+++ b/xen/include/xen/iommu-private.h

I don't think private headers should live in include/xen/. Judging from only
the patches I was Cc-ed on, ...
Thank you for suggestion. Do you where can i place it then?
Please see another comment down regarding who might be using this function.

@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * xen/iommu-private.h
+ */
+#ifndef __XEN_IOMMU_PRIVATE_H__
+#define __XEN_IOMMU_PRIVATE_H__
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+#include <xen/device_tree.h>
+
+/*
+ * Checks if dt_device_node is assigned to a domain or not. This function
+ * expects to be called with dtdevs_lock acquired by caller.
+ */
+bool_t iommu_dt_device_is_assigned_locked(const struct dt_device_node *dev);
+#endif

... I don't even see the need for the declaration, as the function is used
only from the file also defining it. But of course if there is a use
elsewhere (in Arm-only code, as is suggested by the description here), then
the header (under a suitable name) wants to live under drivers/passthrough/
(and of course be included only from anywhere in that sub-tree).

This is also use in smmu.c:arm_smmu_dt_remove_device_legacy(). This is added in
12/19 patch(xen/smmu: Add remove_device callback for smmu_iommu ops).

AFAICT, the caller of this function (iommu_remove_dt_device()) will already
check if the device was assigned and bail out if that's the case.


So why do we need to check it again in the SMMU driver?

This was comment from you in v2:
"Even if the IOMMU subsystem check it, it would be good that the SMMU
driver also check the device is not currently used before removing it.

If it is, then we should return -EBUSY."
Link:https://patchew.org/Xen/1636441347-133850-1-git-send-email-fnu.vikram@xxxxxxxxxx/1636441347-133850-7-git-send-email-fnu.vikram@xxxxxxxxxx/

A lot of water flown under the bridge since that discussion. If you want to check the device is not attached in the SMMU driver, then you should use the internal SMMU state rather than the generic state.

You can look at arm_smmu_attach_dev() for an example how to do it.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.