Re: [PATCH 2/2] iommu/s390: Add support for iommu_device handling
On Tue, 27 Jun 2017 17:28:06 +0200 Joerg Roedel wrote: > On Mon, Jun 19, 2017 at 05:02:19PM +0200, Gerald Schaefer wrote: > > On Thu, 15 Jun 2017 15:11:52 +0200 > > Joerg Roedel wrote: > > > + rc = zpci_init_iommu(zdev); > > > + if (rc) > > > + goto out_free; > > > + > > > > After this point, there are two options for failure (zpci_enable_device + > > zpci_scan_bus), but missing error handling with an appropriate call to > > zpci_destroy_iommu(). > > Right, I'll fix that in the next version. > > > I must admit that I do not understand what these sysfs attributes are > > used for, and by whom and when. Is it really necessary/correct to register > > them (including the s390_iommu_ops) _before_ the zpci_dev is registered > > to the bus (zpci_scan_bus -> pci_scan_root_bus/pci_bus_add_devices)? > > > > What is the expected life cycle for the attributes, and are they already > > needed when a device is still under DMA API access, or even before it is > > added to the PCI bus? > > The sysfs attributes are used by user-space for topology detection. A > user-space program using VFIO needs to know which PCI-devices it needs > to assign to the VFIO driver in order to gain access. > > On s390 this probably doesn't matter much, as the real PCI topology is > not exposed, but it matters on other architectures. The purpose of this > patch is not so much the sysfs attributes. Its a step to convert all > IOMMU drivers to use the iommu_device_register() interface. Goal is that > the iommu core code knows about individual hardware iommus and the > devices behind it. > > When this is implemented in all iommu drivers, we can start moving more > code out of the drivers into the iommu core. This also probably doesn't > matter much for s390, but all iommu drivers need to use this interface > before we can move on. The sysfs-stuff is just a by-product of that. > > So to move on with that, it would be good to get an Acked-by on this > patch from you guys once you think I fixed everything :) Ok, thanks for the explanation. So it should not be a problem if the attributes are registered in zpci_create_device() before the device is registered to the bus, especially since they do not provide any manipulation function but only topology information. BTW, I get the following new attributes with this patch: /sys/devices/pci:00/:00:00.0/iommu /sys/devices/virtual/iommu /sys/devices/virtual/iommu/s390-iommu.0012 /sys/class/iommu/s390-iommu.0012 Regards, Gerald ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/arm-smmu-v3: Implement shutdown method
The shutdown method disables the SMMU and its interrupts to avoid
potentially corrupting a new kernel started with kexec.
Signed-off-by: Nate Watterson
---
drivers/iommu/arm-smmu-v3.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 380969a..907d576 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
arm_smmu_device_disable(smmu);
+
+ /* Disable IRQs */
+ arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
+ ARM_SMMU_IRQ_CTRLACK);
+
return 0;
}
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+ arm_smmu_device_remove(pdev);
+}
+
static struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v3", },
{ },
@@ -2781,6 +2791,7 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
+ .shutdown = arm_smmu_device_shutdown,
};
module_platform_driver(arm_smmu_driver);
--
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Implement shutdown method
On Thu, Jun 29, 2017 at 01:40:15PM -0400, Nate Watterson wrote: > The shutdown method disables the SMMU and its interrupts to avoid > potentially corrupting a new kernel started with kexec. > > Signed-off-by: Nate Watterson > --- > drivers/iommu/arm-smmu-v3.c | 11 +++ > 1 file changed, 11 insertions(+) We should update arm-smmu.c as well. > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c > index 380969a..907d576 100644 > --- a/drivers/iommu/arm-smmu-v3.c > +++ b/drivers/iommu/arm-smmu-v3.c > @@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct > platform_device *pdev) > struct arm_smmu_device *smmu = platform_get_drvdata(pdev); > > arm_smmu_device_disable(smmu); > + > + /* Disable IRQs */ > + arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, > + ARM_SMMU_IRQ_CTRLACK); > + Can you justify the need for this? If we actually need to disable interrupts, then I'd like to understand why so that we can make sure we get the ordering right with respect to disabling the device. Also, do we need to clear the MSI registers too? My understanding is that kexec will mask irqs at the GIC, so there's not actually an issue here. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Implement shutdown method
On 6/29/2017 2:34 PM, Will Deacon wrote: On Thu, Jun 29, 2017 at 01:40:15PM -0400, Nate Watterson wrote: The shutdown method disables the SMMU and its interrupts to avoid potentially corrupting a new kernel started with kexec. Signed-off-by: Nate Watterson --- drivers/iommu/arm-smmu-v3.c | 11 +++ 1 file changed, 11 insertions(+) We should update arm-smmu.c as well. diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 380969a..907d576 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -2765,9 +2765,19 @@ static int arm_smmu_device_remove(struct platform_device *pdev) struct arm_smmu_device *smmu = platform_get_drvdata(pdev); arm_smmu_device_disable(smmu); + + /* Disable IRQs */ + arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, + ARM_SMMU_IRQ_CTRLACK); + Can you justify the need for this? If we actually need to disable interrupts, then I'd like to understand why so that we can make sure we get the ordering right with respect to disabling the device. Also, do we need to clear the MSI registers too? I have no justification. Based on the number of drivers that take care to prevent their HW from generating an interrupt, I thought it would be required, but I can't find any such requirement explicitly laid out in the documentation. When you mention the MSI registers do you mean, for instance, SMMU_GERROR_IRQ_CFG0? It looks like those are always cleared while initializing the SMMU so the case where an SMMU transitions from using MSIs to using wired interrupts between kernels will be handled properly. My understanding is that kexec will mask irqs at the GIC, so there's not actually an issue here. Will -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/arm-smmu-v3: Implement shutdown method
The shutdown method disables the SMMU to avoid corrupting a new kernel
started with kexec.
Signed-off-by: Nate Watterson
---
drivers/iommu/arm-smmu-v3.c | 7 +++
drivers/iommu/arm-smmu.c| 6 ++
2 files changed, 13 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 380969a..3d8ac29 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2765,9 +2765,15 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
arm_smmu_device_disable(smmu);
+
return 0;
}
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+ arm_smmu_device_remove(pdev);
+}
+
static struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v3", },
{ },
@@ -2781,6 +2787,7 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
+ .shutdown = arm_smmu_device_shutdown,
};
module_platform_driver(arm_smmu_driver);
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 7ec30b0..af50bab 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2319,6 +2319,11 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
return 0;
}
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+ arm_smmu_device_remove(pdev);
+}
+
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu",
@@ -2326,6 +2331,7 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
+ .shutdown = arm_smmu_device_shutdown,
};
module_platform_driver(arm_smmu_driver);
--
Qualcomm Datacenter Technologies, Inc. on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu-v3: Implement shutdown method
I should have removed the '-v3' since this revision of the patch adds
shutdown to arm-smmu.c as well. I'll fix that in a subsequent version
after waiting to see if there are additional changes that need to be
made.
On 6/29/2017 6:18 PM, Nate Watterson wrote:
The shutdown method disables the SMMU to avoid corrupting a new kernel
started with kexec.
Signed-off-by: Nate Watterson
---
drivers/iommu/arm-smmu-v3.c | 7 +++
drivers/iommu/arm-smmu.c| 6 ++
2 files changed, 13 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 380969a..3d8ac29 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2765,9 +2765,15 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
arm_smmu_device_disable(smmu);
+
return 0;
}
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+ arm_smmu_device_remove(pdev);
+}
+
static struct of_device_id arm_smmu_of_match[] = {
{ .compatible = "arm,smmu-v3", },
{ },
@@ -2781,6 +2787,7 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
+ .shutdown = arm_smmu_device_shutdown,
};
module_platform_driver(arm_smmu_driver);
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 7ec30b0..af50bab 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2319,6 +2319,11 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
return 0;
}
+static void arm_smmu_device_shutdown(struct platform_device *pdev)
+{
+ arm_smmu_device_remove(pdev);
+}
+
static struct platform_driver arm_smmu_driver = {
.driver = {
.name = "arm-smmu",
@@ -2326,6 +2331,7 @@ static int arm_smmu_device_remove(struct platform_device
*pdev)
},
.probe = arm_smmu_device_probe,
.remove = arm_smmu_device_remove,
+ .shutdown = arm_smmu_device_shutdown,
};
module_platform_driver(arm_smmu_driver);
--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
