[PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Add support for ARM architected SMMUv3 implementation. It is based on the Linux SMMUv3 driver. Driver is currently supported as Tech Preview. Major differences with regard to Linux driver are as follows: 2. Only Stage-2 translation is supported as compared to the Linux driver that supports both Stage-1 and Stage-2 translations. 3. Use P2M page table instead of creating one as SMMUv3 has the capability to share the page tables with the CPU. 4. Tasklets are used in place of threaded IRQ's in Linux for event queue and priority queue IRQ handling. 5. Latest version of the Linux SMMUv3 code implements the commands queue access functions based on atomic operations implemented in Linux. Atomic functions used by the commands queue access functions are not implemented in XEN therefore we decided to port the earlier version of the code. Atomic operations are introduced to fix the bottleneck of the SMMU command queue insertion operation. A new algorithm for inserting commands into the queue is introduced, which is lock-free on the fast-path. Consequence of reverting the patch is that the command queue insertion will be slow for large systems as spinlock will be used to serializes accesses from all CPUs to the single queue supported by the hardware. Once the proper atomic operations will be available in XEN the driver can be updated. 6. Spin lock is used in place of mutex when attaching a device to the SMMU, as there is no blocking locks implementation available in XEN. This might introduce latency in XEN. Need to investigate before driver is out for tech preview. 7. PCI ATS functionality is not supported, as there is no support available in XEN to test the functionality. Code is not tested and compiled. Code is guarded by the flag CONFIG_PCI_ATS. 8. MSI interrupts are not supported as there is no support available in XEN to request MSI interrupts. Code is not tested and compiled. Code is guarded by the flag CONFIG_MSI. Signed-off-by: Rahul Singh --- Changes since v2: - added return statement for readx_poll_timeout function. - remove iommu_get_dma_cookie and iommu_put_dma_cookie. - remove struct arm_smmu_xen_device as not required. - move dt_property_match_string to device_tree.c file. - replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion. - use ARM_SMMU_REG_SZ as size when map memory to XEN. - remove bypass keyword to make sure when device-tree probe is failed we are reporting error and not continuing to configure SMMU in bypass mode. - fixed minor comments. Changes since v3: - Fixed typo for CONFIG_MSI - Added back the mutex code - Rebase the patch on top of newly added WARN_ON(). - Remove the direct read of register VTCR_EL2. - Fixed minor comments. Changes since v4: - Replace the ffsll() with ffs64() function. - Add code to free resources when probe failed. --- --- MAINTAINERS | 6 + SUPPORT.md| 1 + xen/drivers/passthrough/Kconfig | 11 + xen/drivers/passthrough/arm/Makefile | 1 + xen/drivers/passthrough/arm/smmu-v3.c | 939 ++ 5 files changed, 830 insertions(+), 128 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 5079b834c2..14240e8e1e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -249,6 +249,12 @@ F: xen/include/asm-arm/ F: xen/include/public/arch-arm/ F: xen/include/public/arch-arm.h +ARM SMMUv3 +M: Bertrand Marquis +M: Rahul Singh +S: Supported +F: xen/drivers/passthrough/arm/smmu-v3.c + Change Log M: Paul Durrant R: Community Manager diff --git a/SUPPORT.md b/SUPPORT.md index ab02aca5f4..5ee3c8651a 100644 --- a/SUPPORT.md +++ b/SUPPORT.md @@ -67,6 +67,7 @@ For the Cortex A57 r0p0 - r1p1, see Errata 832075. Status, Intel VT-d: Supported Status, ARM SMMUv1: Supported, not security supported Status, ARM SMMUv2: Supported, not security supported +Status, ARM SMMUv3: Tech Preview Status, Renesas IPMMU-VMSA: Supported, not security supported ### ARM/GICv3 ITS diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig index 0036007ec4..341ba92b30 100644 --- a/xen/drivers/passthrough/Kconfig +++ b/xen/drivers/passthrough/Kconfig @@ -13,6 +13,17 @@ config ARM_SMMU Say Y here if your SoC includes an IOMMU device implementing the ARM SMMU architecture. +config ARM_SMMU_V3 + bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT + depends on ARM_64 + ---help--- +Support for implementations of the ARM System MMU architecture +version 3. Driver is in experimental stage and should not be used in +production. + +Say Y here if your system includes an IOMMU device implementing +the ARM SMMUv3 architecture. + config IPMMU_VMSA bool "Renesas IPMMU-VMSA found in R-Car Gen3 SoCs" depends on ARM_64 diff --git a/xen/drivers/passthrough/arm/Makefile
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Hi Rahul, > On 20 Jan 2021, at 14:52, Rahul Singh wrote: > > Add support for ARM architected SMMUv3 implementation. It is based on > the Linux SMMUv3 driver. > > Driver is currently supported as Tech Preview. > > Major differences with regard to Linux driver are as follows: > 2. Only Stage-2 translation is supported as compared to the Linux driver > that supports both Stage-1 and Stage-2 translations. > 3. Use P2M page table instead of creating one as SMMUv3 has the > capability to share the page tables with the CPU. > 4. Tasklets are used in place of threaded IRQ's in Linux for event queue > and priority queue IRQ handling. > 5. Latest version of the Linux SMMUv3 code implements the commands queue > access functions based on atomic operations implemented in Linux. > Atomic functions used by the commands queue access functions are not > implemented in XEN therefore we decided to port the earlier version > of the code. Atomic operations are introduced to fix the bottleneck > of the SMMU command queue insertion operation. A new algorithm for > inserting commands into the queue is introduced, which is lock-free > on the fast-path. > Consequence of reverting the patch is that the command queue > insertion will be slow for large systems as spinlock will be used to > serializes accesses from all CPUs to the single queue supported by > the hardware. Once the proper atomic operations will be available in > XEN the driver can be updated. > 6. Spin lock is used in place of mutex when attaching a device to the > SMMU, as there is no blocking locks implementation available in XEN. > This might introduce latency in XEN. Need to investigate before > driver is out for tech preview. > 7. PCI ATS functionality is not supported, as there is no support > available in XEN to test the functionality. Code is not tested and > compiled. Code is guarded by the flag CONFIG_PCI_ATS. > 8. MSI interrupts are not supported as there is no support available in > XEN to request MSI interrupts. Code is not tested and compiled. Code > is guarded by the flag CONFIG_MSI. > > Signed-off-by: Rahul Singh Reviewed-by: Bertrand Marquis Cheers Bertrand > --- > Changes since v2: > - added return statement for readx_poll_timeout function. > - remove iommu_get_dma_cookie and iommu_put_dma_cookie. > - remove struct arm_smmu_xen_device as not required. > - move dt_property_match_string to device_tree.c file. > - replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion. > - use ARM_SMMU_REG_SZ as size when map memory to XEN. > - remove bypass keyword to make sure when device-tree probe is failed we > are reporting error and not continuing to configure SMMU in bypass > mode. > - fixed minor comments. > Changes since v3: > - Fixed typo for CONFIG_MSI > - Added back the mutex code > - Rebase the patch on top of newly added WARN_ON(). > - Remove the direct read of register VTCR_EL2. > - Fixed minor comments. > Changes since v4: > - Replace the ffsll() with ffs64() function. > - Add code to free resources when probe failed. > --- > --- > MAINTAINERS | 6 + > SUPPORT.md| 1 + > xen/drivers/passthrough/Kconfig | 11 + > xen/drivers/passthrough/arm/Makefile | 1 + > xen/drivers/passthrough/arm/smmu-v3.c | 939 ++ > 5 files changed, 830 insertions(+), 128 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5079b834c2..14240e8e1e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -249,6 +249,12 @@ F: xen/include/asm-arm/ > F:xen/include/public/arch-arm/ > F:xen/include/public/arch-arm.h > > +ARM SMMUv3 > +M: Bertrand Marquis > +M: Rahul Singh > +S: Supported > +F: xen/drivers/passthrough/arm/smmu-v3.c > + > Change Log > M:Paul Durrant > R:Community Manager > diff --git a/SUPPORT.md b/SUPPORT.md > index ab02aca5f4..5ee3c8651a 100644 > --- a/SUPPORT.md > +++ b/SUPPORT.md > @@ -67,6 +67,7 @@ For the Cortex A57 r0p0 - r1p1, see Errata 832075. > Status, Intel VT-d: Supported > Status, ARM SMMUv1: Supported, not security supported > Status, ARM SMMUv2: Supported, not security supported > +Status, ARM SMMUv3: Tech Preview > Status, Renesas IPMMU-VMSA: Supported, not security supported > > ### ARM/GICv3 ITS > diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig > index 0036007ec4..341ba92b30 100644 > --- a/xen/drivers/passthrough/Kconfig > +++ b/xen/drivers/passthrough/Kconfig > @@ -13,6 +13,17 @@ config ARM_SMMU > Say Y here if your SoC includes an IOMMU device implementing the > ARM SMMU architecture. > > +config ARM_SMMU_V3 > + bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT > + depends on ARM_64 > + ---help--- > + Support for implementations of the ARM System MMU architecture > + version 3. Driver is in experimental stage and should not be used in > + production.
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
On Wed, 20 Jan 2021, Rahul Singh wrote: > Add support for ARM architected SMMUv3 implementation. It is based on > the Linux SMMUv3 driver. > > Driver is currently supported as Tech Preview. > > Major differences with regard to Linux driver are as follows: > 2. Only Stage-2 translation is supported as compared to the Linux driver >that supports both Stage-1 and Stage-2 translations. > 3. Use P2M page table instead of creating one as SMMUv3 has the >capability to share the page tables with the CPU. > 4. Tasklets are used in place of threaded IRQ's in Linux for event queue >and priority queue IRQ handling. > 5. Latest version of the Linux SMMUv3 code implements the commands queue >access functions based on atomic operations implemented in Linux. >Atomic functions used by the commands queue access functions are not >implemented in XEN therefore we decided to port the earlier version >of the code. Atomic operations are introduced to fix the bottleneck >of the SMMU command queue insertion operation. A new algorithm for >inserting commands into the queue is introduced, which is lock-free >on the fast-path. >Consequence of reverting the patch is that the command queue >insertion will be slow for large systems as spinlock will be used to >serializes accesses from all CPUs to the single queue supported by >the hardware. Once the proper atomic operations will be available in >XEN the driver can be updated. > 6. Spin lock is used in place of mutex when attaching a device to the >SMMU, as there is no blocking locks implementation available in XEN. >This might introduce latency in XEN. Need to investigate before >driver is out for tech preview. > 7. PCI ATS functionality is not supported, as there is no support >available in XEN to test the functionality. Code is not tested and >compiled. Code is guarded by the flag CONFIG_PCI_ATS. > 8. MSI interrupts are not supported as there is no support available in >XEN to request MSI interrupts. Code is not tested and compiled. Code >is guarded by the flag CONFIG_MSI. > > Signed-off-by: Rahul Singh > --- > Changes since v2: > - added return statement for readx_poll_timeout function. > - remove iommu_get_dma_cookie and iommu_put_dma_cookie. > - remove struct arm_smmu_xen_device as not required. > - move dt_property_match_string to device_tree.c file. > - replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion. > - use ARM_SMMU_REG_SZ as size when map memory to XEN. > - remove bypass keyword to make sure when device-tree probe is failed we > are reporting error and not continuing to configure SMMU in bypass > mode. > - fixed minor comments. > Changes since v3: > - Fixed typo for CONFIG_MSI > - Added back the mutex code > - Rebase the patch on top of newly added WARN_ON(). > - Remove the direct read of register VTCR_EL2. > - Fixed minor comments. > Changes since v4: > - Replace the ffsll() with ffs64() function. > - Add code to free resources when probe failed. > --- > --- > MAINTAINERS | 6 + > SUPPORT.md| 1 + > xen/drivers/passthrough/Kconfig | 11 + > xen/drivers/passthrough/arm/Makefile | 1 + > xen/drivers/passthrough/arm/smmu-v3.c | 939 ++ > 5 files changed, 830 insertions(+), 128 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 5079b834c2..14240e8e1e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -249,6 +249,12 @@ F: xen/include/asm-arm/ > F: xen/include/public/arch-arm/ > F: xen/include/public/arch-arm.h > > +ARM SMMUv3 > +M: Bertrand Marquis > +M: Rahul Singh > +S: Supported > +F: xen/drivers/passthrough/arm/smmu-v3.c > + > Change Log > M: Paul Durrant > R: Community Manager > diff --git a/SUPPORT.md b/SUPPORT.md > index ab02aca5f4..5ee3c8651a 100644 > --- a/SUPPORT.md > +++ b/SUPPORT.md > @@ -67,6 +67,7 @@ For the Cortex A57 r0p0 - r1p1, see Errata 832075. > Status, Intel VT-d: Supported > Status, ARM SMMUv1: Supported, not security supported > Status, ARM SMMUv2: Supported, not security supported > +Status, ARM SMMUv3: Tech Preview > Status, Renesas IPMMU-VMSA: Supported, not security supported > > ### ARM/GICv3 ITS > diff --git a/xen/drivers/passthrough/Kconfig b/xen/drivers/passthrough/Kconfig > index 0036007ec4..341ba92b30 100644 > --- a/xen/drivers/passthrough/Kconfig > +++ b/xen/drivers/passthrough/Kconfig > @@ -13,6 +13,17 @@ config ARM_SMMU > Say Y here if your SoC includes an IOMMU device implementing the > ARM SMMU architecture. > > +config ARM_SMMU_V3 > + bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT > + depends on ARM_64 > + ---help--- > + Support for implementations of the ARM System MMU architecture > + version 3. Driver is in experimental stage and should not be used in > + production. > + > + Say Y here if your
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
On 20.01.21 16:52, Rahul Singh wrote:
Hi Rahul
Add support for ARM architected SMMUv3 implementation. It is based on
the Linux SMMUv3 driver.
Driver is currently supported as Tech Preview.
Major differences with regard to Linux driver are as follows:
2. Only Stage-2 translation is supported as compared to the Linux driver
that supports both Stage-1 and Stage-2 translations.
3. Use P2M page table instead of creating one as SMMUv3 has the
capability to share the page tables with the CPU.
4. Tasklets are used in place of threaded IRQ's in Linux for event queue
and priority queue IRQ handling.
5. Latest version of the Linux SMMUv3 code implements the commands queue
access functions based on atomic operations implemented in Linux.
Atomic functions used by the commands queue access functions are not
implemented in XEN therefore we decided to port the earlier version
of the code. Atomic operations are introduced to fix the bottleneck
of the SMMU command queue insertion operation. A new algorithm for
inserting commands into the queue is introduced, which is lock-free
on the fast-path.
Consequence of reverting the patch is that the command queue
insertion will be slow for large systems as spinlock will be used to
serializes accesses from all CPUs to the single queue supported by
the hardware. Once the proper atomic operations will be available in
XEN the driver can be updated.
6. Spin lock is used in place of mutex when attaching a device to the
SMMU, as there is no blocking locks implementation available in XEN.
This might introduce latency in XEN. Need to investigate before
driver is out for tech preview.
7. PCI ATS functionality is not supported, as there is no support
available in XEN to test the functionality. Code is not tested and
compiled. Code is guarded by the flag CONFIG_PCI_ATS.
8. MSI interrupts are not supported as there is no support available in
XEN to request MSI interrupts. Code is not tested and compiled. Code
is guarded by the flag CONFIG_MSI.
Signed-off-by: Rahul Singh
---
Changes since v2:
- added return statement for readx_poll_timeout function.
- remove iommu_get_dma_cookie and iommu_put_dma_cookie.
- remove struct arm_smmu_xen_device as not required.
- move dt_property_match_string to device_tree.c file.
- replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion.
- use ARM_SMMU_REG_SZ as size when map memory to XEN.
- remove bypass keyword to make sure when device-tree probe is failed we
are reporting error and not continuing to configure SMMU in bypass
mode.
- fixed minor comments.
Changes since v3:
- Fixed typo for CONFIG_MSI
- Added back the mutex code
- Rebase the patch on top of newly added WARN_ON().
- Remove the direct read of register VTCR_EL2.
- Fixed minor comments.
Changes since v4:
- Replace the ffsll() with ffs64() function.
- Add code to free resources when probe failed.
Thank you for addressing, patch looks ok to me, just one small remark below:
+
+static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
+{
+}
We discussed in V4 about adding some code here which all IOMMUs on Arm
already have, copy it below for the convenience:
/* Set to false options not supported on ARM. */
if ( iommu_hwdom_inclusive )
printk(XENLOG_WARNING
"map-inclusive dom0-iommu option is not supported on ARM\n");
iommu_hwdom_inclusive = false;
if ( iommu_hwdom_reserved == 1 )
printk(XENLOG_WARNING
"map-reserved dom0-iommu option is not supported on ARM\n");
iommu_hwdom_reserved = 0;
arch_iommu_hwdom_init(d);
Also we discussed about possibility to fold the part of code (which
disables unsupported options) in arch_iommu_hwdom_init() to avoid
duplication as a follow-up.
At least, I expected to see arch_iommu_hwdom_init() to be called by
arm_smmu_iommu_hwdom_init() it current patch... Please note, this is
*not* a request for change immediately,
I think, driver is functional even without this code (hopefully
arch_iommu_hwdom_init is empty now, etc). But, if you happen to do V6
or probably it could be done on commit ...
+
+static void arm_smmu_iommu_xen_domain_teardown(struct domain *d)
+{
+ struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
+
+ ASSERT(list_empty(&xen_domain->contexts));
+ xfree(xen_domain);
+}
+
+static const struct iommu_ops arm_smmu_iommu_ops = {
+ .init = arm_smmu_iommu_xen_domain_init,
+ .hwdom_init = arm_smmu_iommu_hwdom_init,
+ .teardown = arm_smmu_iommu_xen_domain_teardown,
+ .iotlb_flush= arm_smmu_iotlb_flush,
+ .iotlb_flush_all= arm_smmu_iotlb_flush_all,
+ .assign_device = arm_smmu_assign_dev,
+ .reassign_device= arm_smmu_reassign_dev,
+ .map_page = arm_iommu_map_page,
+ .unmap_page =
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Hello Stefano, > On 20 Jan 2021, at 8:31 pm, Stefano Stabellini wrote: > > On Wed, 20 Jan 2021, Rahul Singh wrote: >> Add support for ARM architected SMMUv3 implementation. It is based on >> the Linux SMMUv3 driver. >> >> Driver is currently supported as Tech Preview. >> >> Major differences with regard to Linux driver are as follows: >> 2. Only Stage-2 translation is supported as compared to the Linux driver >> that supports both Stage-1 and Stage-2 translations. >> 3. Use P2M page table instead of creating one as SMMUv3 has the >> capability to share the page tables with the CPU. >> 4. Tasklets are used in place of threaded IRQ's in Linux for event queue >> and priority queue IRQ handling. >> 5. Latest version of the Linux SMMUv3 code implements the commands queue >> access functions based on atomic operations implemented in Linux. >> Atomic functions used by the commands queue access functions are not >> implemented in XEN therefore we decided to port the earlier version >> of the code. Atomic operations are introduced to fix the bottleneck >> of the SMMU command queue insertion operation. A new algorithm for >> inserting commands into the queue is introduced, which is lock-free >> on the fast-path. >> Consequence of reverting the patch is that the command queue >> insertion will be slow for large systems as spinlock will be used to >> serializes accesses from all CPUs to the single queue supported by >> the hardware. Once the proper atomic operations will be available in >> XEN the driver can be updated. >> 6. Spin lock is used in place of mutex when attaching a device to the >> SMMU, as there is no blocking locks implementation available in XEN. >> This might introduce latency in XEN. Need to investigate before >> driver is out for tech preview. >> 7. PCI ATS functionality is not supported, as there is no support >> available in XEN to test the functionality. Code is not tested and >> compiled. Code is guarded by the flag CONFIG_PCI_ATS. >> 8. MSI interrupts are not supported as there is no support available in >> XEN to request MSI interrupts. Code is not tested and compiled. Code >> is guarded by the flag CONFIG_MSI. >> >> Signed-off-by: Rahul Singh >> --- >> Changes since v2: >> - added return statement for readx_poll_timeout function. >> - remove iommu_get_dma_cookie and iommu_put_dma_cookie. >> - remove struct arm_smmu_xen_device as not required. >> - move dt_property_match_string to device_tree.c file. >> - replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion. >> - use ARM_SMMU_REG_SZ as size when map memory to XEN. >> - remove bypass keyword to make sure when device-tree probe is failed we >> are reporting error and not continuing to configure SMMU in bypass >> mode. >> - fixed minor comments. >> Changes since v3: >> - Fixed typo for CONFIG_MSI >> - Added back the mutex code >> - Rebase the patch on top of newly added WARN_ON(). >> - Remove the direct read of register VTCR_EL2. >> - Fixed minor comments. >> Changes since v4: >> - Replace the ffsll() with ffs64() function. >> - Add code to free resources when probe failed. >> --- >> --- >> MAINTAINERS | 6 + >> SUPPORT.md| 1 + >> xen/drivers/passthrough/Kconfig | 11 + >> xen/drivers/passthrough/arm/Makefile | 1 + >> xen/drivers/passthrough/arm/smmu-v3.c | 939 ++ >> 5 files changed, 830 insertions(+), 128 deletions(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 5079b834c2..14240e8e1e 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -249,6 +249,12 @@ F: xen/include/asm-arm/ >> F: xen/include/public/arch-arm/ >> F: xen/include/public/arch-arm.h >> >> +ARM SMMUv3 >> +M: Bertrand Marquis >> +M: Rahul Singh >> +S: Supported >> +F: xen/drivers/passthrough/arm/smmu-v3.c >> + >> Change Log >> M: Paul Durrant >> R: Community Manager >> diff --git a/SUPPORT.md b/SUPPORT.md >> index ab02aca5f4..5ee3c8651a 100644 >> --- a/SUPPORT.md >> +++ b/SUPPORT.md >> @@ -67,6 +67,7 @@ For the Cortex A57 r0p0 - r1p1, see Errata 832075. >> Status, Intel VT-d: Supported >> Status, ARM SMMUv1: Supported, not security supported >> Status, ARM SMMUv2: Supported, not security supported >> +Status, ARM SMMUv3: Tech Preview >> Status, Renesas IPMMU-VMSA: Supported, not security supported >> >> ### ARM/GICv3 ITS >> diff --git a/xen/drivers/passthrough/Kconfig >> b/xen/drivers/passthrough/Kconfig >> index 0036007ec4..341ba92b30 100644 >> --- a/xen/drivers/passthrough/Kconfig >> +++ b/xen/drivers/passthrough/Kconfig >> @@ -13,6 +13,17 @@ config ARM_SMMU >>Say Y here if your SoC includes an IOMMU device implementing the >>ARM SMMU architecture. >> >> +config ARM_SMMU_V3 >> +bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" if EXPERT >> +depends on ARM_64 >> +---help--- >> + Support for implementations of the ARM System MMU archite
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Hello Oleksandr,
> On 20 Jan 2021, at 9:33 pm, Oleksandr wrote:
>
>
> On 20.01.21 16:52, Rahul Singh wrote:
>
> Hi Rahul
>
>> Add support for ARM architected SMMUv3 implementation. It is based on
>> the Linux SMMUv3 driver.
>>
>> Driver is currently supported as Tech Preview.
>>
>> Major differences with regard to Linux driver are as follows:
>> 2. Only Stage-2 translation is supported as compared to the Linux driver
>>that supports both Stage-1 and Stage-2 translations.
>> 3. Use P2M page table instead of creating one as SMMUv3 has the
>>capability to share the page tables with the CPU.
>> 4. Tasklets are used in place of threaded IRQ's in Linux for event queue
>>and priority queue IRQ handling.
>> 5. Latest version of the Linux SMMUv3 code implements the commands queue
>>access functions based on atomic operations implemented in Linux.
>>Atomic functions used by the commands queue access functions are not
>>implemented in XEN therefore we decided to port the earlier version
>>of the code. Atomic operations are introduced to fix the bottleneck
>>of the SMMU command queue insertion operation. A new algorithm for
>>inserting commands into the queue is introduced, which is lock-free
>>on the fast-path.
>>Consequence of reverting the patch is that the command queue
>>insertion will be slow for large systems as spinlock will be used to
>>serializes accesses from all CPUs to the single queue supported by
>>the hardware. Once the proper atomic operations will be available in
>>XEN the driver can be updated.
>> 6. Spin lock is used in place of mutex when attaching a device to the
>>SMMU, as there is no blocking locks implementation available in XEN.
>>This might introduce latency in XEN. Need to investigate before
>>driver is out for tech preview.
>> 7. PCI ATS functionality is not supported, as there is no support
>>available in XEN to test the functionality. Code is not tested and
>>compiled. Code is guarded by the flag CONFIG_PCI_ATS.
>> 8. MSI interrupts are not supported as there is no support available in
>>XEN to request MSI interrupts. Code is not tested and compiled. Code
>>is guarded by the flag CONFIG_MSI.
>>
>> Signed-off-by: Rahul Singh
>> ---
>> Changes since v2:
>> - added return statement for readx_poll_timeout function.
>> - remove iommu_get_dma_cookie and iommu_put_dma_cookie.
>> - remove struct arm_smmu_xen_device as not required.
>> - move dt_property_match_string to device_tree.c file.
>> - replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion.
>> - use ARM_SMMU_REG_SZ as size when map memory to XEN.
>> - remove bypass keyword to make sure when device-tree probe is failed we
>> are reporting error and not continuing to configure SMMU in bypass
>> mode.
>> - fixed minor comments.
>> Changes since v3:
>> - Fixed typo for CONFIG_MSI
>> - Added back the mutex code
>> - Rebase the patch on top of newly added WARN_ON().
>> - Remove the direct read of register VTCR_EL2.
>> - Fixed minor comments.
>> Changes since v4:
>> - Replace the ffsll() with ffs64() function.
>> - Add code to free resources when probe failed.
>
> Thank you for addressing, patch looks ok to me, just one small remark below:
>
>
>> +
>> +static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
>> +{
>> +}
>
> We discussed in V4 about adding some code here which all IOMMUs on Arm
> already have, copy it below for the convenience:
>
>
> /* Set to false options not supported on ARM. */
> if ( iommu_hwdom_inclusive )
> printk(XENLOG_WARNING
> "map-inclusive dom0-iommu option is not supported on ARM\n");
> iommu_hwdom_inclusive = false;
> if ( iommu_hwdom_reserved == 1 )
> printk(XENLOG_WARNING
> "map-reserved dom0-iommu option is not supported on ARM\n");
> iommu_hwdom_reserved = 0;
>
> arch_iommu_hwdom_init(d);
>
>
> Also we discussed about possibility to fold the part of code (which disables
> unsupported options) in arch_iommu_hwdom_init() to avoid duplication as a
> follow-up.
> At least, I expected to see arch_iommu_hwdom_init() to be called by
> arm_smmu_iommu_hwdom_init() it current patch... Please note, this is *not* a
> request for change immediately,
> I think, driver is functional even without this code (hopefully
> arch_iommu_hwdom_init is empty now, etc). But, if you happen to do V6 or
> probably it could be done on commit ...
>
Yes I will send the patch to move the code to arch_iommu_hwdom_init() to avoid
duplication once SMMUv3 driver will be merged.
I thought anyway I have to remove the code from SMMUv1 and IPMMU I will take
care of all the IOMMU driver at the same time because of that I didn’t modify
the SMMUv3 driver. Yes if there is a need for v6 I will add the
arch_iommu_hwdom_init(d) in arm_smmu_iommu_hwdom_init().
Regards,
Rahul
>
>> +
>> +static void arm_smmu_iommu_xen_domain_teardown(struct doma
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
On 21/01/2021 17:18, Rahul Singh wrote:
Hello Oleksandr,
Hi,
On 20 Jan 2021, at 9:33 pm, Oleksandr wrote:
On 20.01.21 16:52, Rahul Singh wrote:
Hi Rahul
Add support for ARM architected SMMUv3 implementation. It is based on
the Linux SMMUv3 driver.
Driver is currently supported as Tech Preview.
Major differences with regard to Linux driver are as follows:
2. Only Stage-2 translation is supported as compared to the Linux driver
that supports both Stage-1 and Stage-2 translations.
3. Use P2M page table instead of creating one as SMMUv3 has the
capability to share the page tables with the CPU.
4. Tasklets are used in place of threaded IRQ's in Linux for event queue
and priority queue IRQ handling.
5. Latest version of the Linux SMMUv3 code implements the commands queue
access functions based on atomic operations implemented in Linux.
Atomic functions used by the commands queue access functions are not
implemented in XEN therefore we decided to port the earlier version
of the code. Atomic operations are introduced to fix the bottleneck
of the SMMU command queue insertion operation. A new algorithm for
inserting commands into the queue is introduced, which is lock-free
on the fast-path.
Consequence of reverting the patch is that the command queue
insertion will be slow for large systems as spinlock will be used to
serializes accesses from all CPUs to the single queue supported by
the hardware. Once the proper atomic operations will be available in
XEN the driver can be updated.
6. Spin lock is used in place of mutex when attaching a device to the
SMMU, as there is no blocking locks implementation available in XEN.
This might introduce latency in XEN. Need to investigate before
driver is out for tech preview.
7. PCI ATS functionality is not supported, as there is no support
available in XEN to test the functionality. Code is not tested and
compiled. Code is guarded by the flag CONFIG_PCI_ATS.
8. MSI interrupts are not supported as there is no support available in
XEN to request MSI interrupts. Code is not tested and compiled. Code
is guarded by the flag CONFIG_MSI.
Signed-off-by: Rahul Singh
---
Changes since v2:
- added return statement for readx_poll_timeout function.
- remove iommu_get_dma_cookie and iommu_put_dma_cookie.
- remove struct arm_smmu_xen_device as not required.
- move dt_property_match_string to device_tree.c file.
- replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion.
- use ARM_SMMU_REG_SZ as size when map memory to XEN.
- remove bypass keyword to make sure when device-tree probe is failed we
are reporting error and not continuing to configure SMMU in bypass
mode.
- fixed minor comments.
Changes since v3:
- Fixed typo for CONFIG_MSI
- Added back the mutex code
- Rebase the patch on top of newly added WARN_ON().
- Remove the direct read of register VTCR_EL2.
- Fixed minor comments.
Changes since v4:
- Replace the ffsll() with ffs64() function.
- Add code to free resources when probe failed.
Thank you for addressing, patch looks ok to me, just one small remark below:
+
+static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
+{
+}
We discussed in V4 about adding some code here which all IOMMUs on Arm already
have, copy it below for the convenience:
/* Set to false options not supported on ARM. */
if ( iommu_hwdom_inclusive )
printk(XENLOG_WARNING
"map-inclusive dom0-iommu option is not supported on ARM\n");
iommu_hwdom_inclusive = false;
if ( iommu_hwdom_reserved == 1 )
printk(XENLOG_WARNING
"map-reserved dom0-iommu option is not supported on ARM\n");
iommu_hwdom_reserved = 0;
arch_iommu_hwdom_init(d);
Also we discussed about possibility to fold the part of code (which disables
unsupported options) in arch_iommu_hwdom_init() to avoid duplication as a
follow-up.
At least, I expected to see arch_iommu_hwdom_init() to be called by
arm_smmu_iommu_hwdom_init() it current patch... Please note, this is *not* a
request for change immediately,
I think, driver is functional even without this code (hopefully
arch_iommu_hwdom_init is empty now, etc). But, if you happen to do V6 or
probably it could be done on commit ...
Yes I will send the patch to move the code to arch_iommu_hwdom_init() to avoid
duplication once SMMUv3 driver will be merged.
I thought anyway I have to remove the code from SMMUv1 and IPMMU I will take
care of all the IOMMU driver at the same time because of that I didn’t modify
the SMMUv3 driver.
There are two part in the problem here:
1) Code duplication
2) The SMMUv3 not checking the command line and calling
arch_iommu_hwdom_init(d)
I agree that 1) can be deferred because it is a clean-up. However, I
consider 2) a (latent) bug because it means that we risk unintentionally
breaking the SMMUv3 driver is we need to add code in
arch_iommu_hwdom_i
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Hi Rahul,
Please try to trim the e-mail when quoting, otherwise it is quite
difficult to find the only couple of answer you wrote.
On 21/01/2021 17:10, Rahul Singh wrote:
On 20 Jan 2021, at 8:31 pm, Stefano Stabellini wrote:
-static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t
start,
- resource_size_t size)
+
+static void arm_smmu_free_structures(struct arm_smmu_device *smmu)
{
- struct resource res = {
- .flags = IORESOURCE_MEM,
- .start = start,
- .end = start + size - 1,
- };
+ if (smmu->cmdq.q.base)
+ xfree(smmu->cmdq.q.base);
+
+ if (smmu->evtq.q.base)
+ xfree(smmu->evtq.q.base);
- return devm_ioremap_resource(dev, &res);
+ if (smmu->priq.q.base)
+ xfree(smmu->priq.q.base);
+
+ if (smmu->strtab_cfg.strtab)
+ xfree(smmu->strtab_cfg.strtab);
+
+ if (smmu->strtab_cfg.l1_desc)
+ xfree(smmu->strtab_cfg.l1_desc);
From what I can tell we also need to free somewhere
smmu->strtab_cfg->l1_desc->l2ptr allocated by arm_smmu_init_l2_strtab
"l1_desc->l2ptr" is a pointer to the Level 1 Stream Table Descriptor if 2-level
Stream Table supported.
If the device is protected by IOMMU, SMMUv3 driver will allocate the
"l1_desc->l2ptr” when the device is added to the IOMMU via
arm_smmu_add_device() function and device will be configured in bypass/abort mode.
Once we assign the device to the domain(arm_smmu_assign_dev() ) smmuv3 hw will
be configured correctly to match the StreamID. If there is a failure in
assigning the device, that case also XEN will not remove the device and master
device still be in bypass/abort mode.
I am a bit confused with this answer. Wouldn't this mean that we are
"leaking" memory if we fail to assign the device?
As in XEN, there is no function to remove the master device from the IOMMU, because of
that I feel there is no need to free the "l1_desc->l2ptr” in case of failure
also.
Hmmm... Xen is able to remove device from the IOMMU. The reason this is
not implemented yet on Arm is because you can't hot-unplug "platform"
device.
I expect the removal function to be useful for PCI.
Cheers,
--
Julien Grall
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Hello Julien,
> On 21 Jan 2021, at 6:31 pm, Julien Grall wrote:
>
> On 21/01/2021 17:18, Rahul Singh wrote:
>> Hello Oleksandr,
>
> Hi,
>
>>> On 20 Jan 2021, at 9:33 pm, Oleksandr wrote:
>>>
>>>
>>> On 20.01.21 16:52, Rahul Singh wrote:
>>>
>>> Hi Rahul
>>>
Add support for ARM architected SMMUv3 implementation. It is based on
the Linux SMMUv3 driver.
Driver is currently supported as Tech Preview.
Major differences with regard to Linux driver are as follows:
2. Only Stage-2 translation is supported as compared to the Linux driver
that supports both Stage-1 and Stage-2 translations.
3. Use P2M page table instead of creating one as SMMUv3 has the
capability to share the page tables with the CPU.
4. Tasklets are used in place of threaded IRQ's in Linux for event queue
and priority queue IRQ handling.
5. Latest version of the Linux SMMUv3 code implements the commands queue
access functions based on atomic operations implemented in Linux.
Atomic functions used by the commands queue access functions are not
implemented in XEN therefore we decided to port the earlier version
of the code. Atomic operations are introduced to fix the bottleneck
of the SMMU command queue insertion operation. A new algorithm for
inserting commands into the queue is introduced, which is lock-free
on the fast-path.
Consequence of reverting the patch is that the command queue
insertion will be slow for large systems as spinlock will be used to
serializes accesses from all CPUs to the single queue supported by
the hardware. Once the proper atomic operations will be available in
XEN the driver can be updated.
6. Spin lock is used in place of mutex when attaching a device to the
SMMU, as there is no blocking locks implementation available in XEN.
This might introduce latency in XEN. Need to investigate before
driver is out for tech preview.
7. PCI ATS functionality is not supported, as there is no support
available in XEN to test the functionality. Code is not tested and
compiled. Code is guarded by the flag CONFIG_PCI_ATS.
8. MSI interrupts are not supported as there is no support available in
XEN to request MSI interrupts. Code is not tested and compiled. Code
is guarded by the flag CONFIG_MSI.
Signed-off-by: Rahul Singh
---
Changes since v2:
- added return statement for readx_poll_timeout function.
- remove iommu_get_dma_cookie and iommu_put_dma_cookie.
- remove struct arm_smmu_xen_device as not required.
- move dt_property_match_string to device_tree.c file.
- replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion.
- use ARM_SMMU_REG_SZ as size when map memory to XEN.
- remove bypass keyword to make sure when device-tree probe is failed we
are reporting error and not continuing to configure SMMU in bypass
mode.
- fixed minor comments.
Changes since v3:
- Fixed typo for CONFIG_MSI
- Added back the mutex code
- Rebase the patch on top of newly added WARN_ON().
- Remove the direct read of register VTCR_EL2.
- Fixed minor comments.
Changes since v4:
- Replace the ffsll() with ffs64() function.
- Add code to free resources when probe failed.
>>>
>>> Thank you for addressing, patch looks ok to me, just one small remark below:
>>>
>>>
+
+static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
+{
+}
>>>
>>> We discussed in V4 about adding some code here which all IOMMUs on Arm
>>> already have, copy it below for the convenience:
>>>
>>>
>>> /* Set to false options not supported on ARM. */
>>> if ( iommu_hwdom_inclusive )
>>> printk(XENLOG_WARNING
>>> "map-inclusive dom0-iommu option is not supported on ARM\n");
>>> iommu_hwdom_inclusive = false;
>>> if ( iommu_hwdom_reserved == 1 )
>>> printk(XENLOG_WARNING
>>> "map-reserved dom0-iommu option is not supported on ARM\n");
>>> iommu_hwdom_reserved = 0;
>>>
>>> arch_iommu_hwdom_init(d);
>>>
>>>
>>> Also we discussed about possibility to fold the part of code (which
>>> disables unsupported options) in arch_iommu_hwdom_init() to avoid
>>> duplication as a follow-up.
>>> At least, I expected to see arch_iommu_hwdom_init() to be called by
>>> arm_smmu_iommu_hwdom_init() it current patch... Please note, this is *not*
>>> a request for change immediately,
>>> I think, driver is functional even without this code (hopefully
>>> arch_iommu_hwdom_init is empty now, etc). But, if you happen to do V6 or
>>> probably it could be done on commit ...
>>>
>> Yes I will send the patch to move the code to arch_iommu_hwdom_init() to
>> avoid duplication once SMMUv3 driver will be merged.
>> I thought anyway I have to remo
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
On 21/01/2021 18:50, Rahul Singh wrote:
Hello Julien,
On 21 Jan 2021, at 6:31 pm, Julien Grall wrote:
On 21/01/2021 17:18, Rahul Singh wrote:
Hello Oleksandr,
Hi,
On 20 Jan 2021, at 9:33 pm, Oleksandr wrote:
On 20.01.21 16:52, Rahul Singh wrote:
Hi Rahul
Add support for ARM architected SMMUv3 implementation. It is based on
the Linux SMMUv3 driver.
Driver is currently supported as Tech Preview.
Major differences with regard to Linux driver are as follows:
2. Only Stage-2 translation is supported as compared to the Linux driver
that supports both Stage-1 and Stage-2 translations.
3. Use P2M page table instead of creating one as SMMUv3 has the
capability to share the page tables with the CPU.
4. Tasklets are used in place of threaded IRQ's in Linux for event queue
and priority queue IRQ handling.
5. Latest version of the Linux SMMUv3 code implements the commands queue
access functions based on atomic operations implemented in Linux.
Atomic functions used by the commands queue access functions are not
implemented in XEN therefore we decided to port the earlier version
of the code. Atomic operations are introduced to fix the bottleneck
of the SMMU command queue insertion operation. A new algorithm for
inserting commands into the queue is introduced, which is lock-free
on the fast-path.
Consequence of reverting the patch is that the command queue
insertion will be slow for large systems as spinlock will be used to
serializes accesses from all CPUs to the single queue supported by
the hardware. Once the proper atomic operations will be available in
XEN the driver can be updated.
6. Spin lock is used in place of mutex when attaching a device to the
SMMU, as there is no blocking locks implementation available in XEN.
This might introduce latency in XEN. Need to investigate before
driver is out for tech preview.
7. PCI ATS functionality is not supported, as there is no support
available in XEN to test the functionality. Code is not tested and
compiled. Code is guarded by the flag CONFIG_PCI_ATS.
8. MSI interrupts are not supported as there is no support available in
XEN to request MSI interrupts. Code is not tested and compiled. Code
is guarded by the flag CONFIG_MSI.
Signed-off-by: Rahul Singh
---
Changes since v2:
- added return statement for readx_poll_timeout function.
- remove iommu_get_dma_cookie and iommu_put_dma_cookie.
- remove struct arm_smmu_xen_device as not required.
- move dt_property_match_string to device_tree.c file.
- replace arm_smmu_*_thread to arm_smmu_*_tasklet to avoid confusion.
- use ARM_SMMU_REG_SZ as size when map memory to XEN.
- remove bypass keyword to make sure when device-tree probe is failed we
are reporting error and not continuing to configure SMMU in bypass
mode.
- fixed minor comments.
Changes since v3:
- Fixed typo for CONFIG_MSI
- Added back the mutex code
- Rebase the patch on top of newly added WARN_ON().
- Remove the direct read of register VTCR_EL2.
- Fixed minor comments.
Changes since v4:
- Replace the ffsll() with ffs64() function.
- Add code to free resources when probe failed.
Thank you for addressing, patch looks ok to me, just one small remark below:
+
+static void __hwdom_init arm_smmu_iommu_hwdom_init(struct domain *d)
+{
+}
We discussed in V4 about adding some code here which all IOMMUs on Arm already
have, copy it below for the convenience:
/* Set to false options not supported on ARM. */
if ( iommu_hwdom_inclusive )
printk(XENLOG_WARNING
"map-inclusive dom0-iommu option is not supported on ARM\n");
iommu_hwdom_inclusive = false;
if ( iommu_hwdom_reserved == 1 )
printk(XENLOG_WARNING
"map-reserved dom0-iommu option is not supported on ARM\n");
iommu_hwdom_reserved = 0;
arch_iommu_hwdom_init(d);
Also we discussed about possibility to fold the part of code (which disables
unsupported options) in arch_iommu_hwdom_init() to avoid duplication as a
follow-up.
At least, I expected to see arch_iommu_hwdom_init() to be called by
arm_smmu_iommu_hwdom_init() it current patch... Please note, this is *not* a
request for change immediately,
I think, driver is functional even without this code (hopefully
arch_iommu_hwdom_init is empty now, etc). But, if you happen to do V6 or
probably it could be done on commit ...
Yes I will send the patch to move the code to arch_iommu_hwdom_init() to avoid
duplication once SMMUv3 driver will be merged.
I thought anyway I have to remove the code from SMMUv1 and IPMMU I will take
care of all the IOMMU driver at the same time because of that I didn’t modify
the SMMUv3 driver.
There are two part in the problem here:
1) Code duplication
2) The SMMUv3 not checking the command line and calling
arch_iommu_hwdom_init(d)
I agree that 1) can be deferred because it is a clean-up. However, I consider
2) a (latent) bug because it
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
Hello Julien,
> On 21 Jan 2021, at 6:43 pm, Julien Grall wrote:
>
> Hi Rahul,
>
> Please try to trim the e-mail when quoting, otherwise it is quite difficult
> to find the only couple of answer you wrote.
>
> On 21/01/2021 17:10, Rahul Singh wrote:
>>> On 20 Jan 2021, at 8:31 pm, Stefano Stabellini
>>> wrote:
-static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t
start,
-resource_size_t size)
+
+static void arm_smmu_free_structures(struct arm_smmu_device *smmu)
{
- struct resource res = {
- .flags = IORESOURCE_MEM,
- .start = start,
- .end = start + size - 1,
- };
+ if (smmu->cmdq.q.base)
+ xfree(smmu->cmdq.q.base);
+
+ if (smmu->evtq.q.base)
+ xfree(smmu->evtq.q.base);
- return devm_ioremap_resource(dev, &res);
+ if (smmu->priq.q.base)
+ xfree(smmu->priq.q.base);
+
+ if (smmu->strtab_cfg.strtab)
+ xfree(smmu->strtab_cfg.strtab);
+
+ if (smmu->strtab_cfg.l1_desc)
+ xfree(smmu->strtab_cfg.l1_desc);
>>>
>>> From what I can tell we also need to free somewhere
>>> smmu->strtab_cfg->l1_desc->l2ptr allocated by arm_smmu_init_l2_strtab
>> "l1_desc->l2ptr" is a pointer to the Level 1 Stream Table Descriptor if
>> 2-level Stream Table supported.
>> If the device is protected by IOMMU, SMMUv3 driver will allocate the
>> "l1_desc->l2ptr” when the device is added to the IOMMU via
>> arm_smmu_add_device() function and device will be configured in bypass/abort
>> mode.
>> Once we assign the device to the domain(arm_smmu_assign_dev() ) smmuv3 hw
>> will be configured correctly to match the StreamID. If there is a failure in
>> assigning the device, that case also XEN will not remove the device and
>> master device still be in bypass/abort mode.
>
> I am a bit confused with this answer. Wouldn't this mean that we are
> "leaking" memory if we fail to assign the device?
No we are not leaking memory as "l1_desc->l2ptr” is still be valid and is
required for correct SMMUv3 opeation if we fail to assign the device, as in
that case device will operate in bypass or abort mode.
For example if by any chance there is faulty hw behind SMMUv3 then if we fail
to assign the device we can configure the device in abort mode in that case
SMMUv3 hw will silently report abort to device, without any event recorded.
We can also configure the device in bypass mode if we fail to assign. Thats why
I thought not to free the "l1_desc->l2ptr” if we fail to assign the device.
>
>> As in XEN, there is no function to remove the master device from the IOMMU,
>> because of that I feel there is no need to free the "l1_desc->l2ptr” in case
>> of failure also.
>
> Hmmm... Xen is able to remove device from the IOMMU. The reason this is not
> implemented yet on Arm is because you can't hot-unplug "platform" device.
>
> I expect the removal function to be useful for PCI.
Yes I agree for PCI hot plug devices we have to implement the IOMMU remove
function. If we implement the remove function for PCI hot-plug we can free the
the "l1_desc->l2ptr” in the remove function for that device.
Regards,
Rahul
>
> Cheers,
>
> --
> Julien Grall
Re: [PATCH v5 10/10] xen/arm: smmuv3: Add support for SMMUv3 driver
On Thu, 21 Jan 2021, Rahul Singh wrote:
> > On 21 Jan 2021, at 6:43 pm, Julien Grall wrote:
> > On 21/01/2021 17:10, Rahul Singh wrote:
> >>> On 20 Jan 2021, at 8:31 pm, Stefano Stabellini
> >>> wrote:
> -static void __iomem *arm_smmu_ioremap(struct device *dev,
> resource_size_t start,
> - resource_size_t size)
> +
> +static void arm_smmu_free_structures(struct arm_smmu_device *smmu)
> {
> -struct resource res = {
> -.flags = IORESOURCE_MEM,
> -.start = start,
> -.end = start + size - 1,
> -};
> +if (smmu->cmdq.q.base)
> +xfree(smmu->cmdq.q.base);
> +
> +if (smmu->evtq.q.base)
> +xfree(smmu->evtq.q.base);
>
> -return devm_ioremap_resource(dev, &res);
> +if (smmu->priq.q.base)
> +xfree(smmu->priq.q.base);
> +
> +if (smmu->strtab_cfg.strtab)
> +xfree(smmu->strtab_cfg.strtab);
> +
> +if (smmu->strtab_cfg.l1_desc)
> +xfree(smmu->strtab_cfg.l1_desc);
> >>>
> >>> From what I can tell we also need to free somewhere
> >>> smmu->strtab_cfg->l1_desc->l2ptr allocated by arm_smmu_init_l2_strtab
> >> "l1_desc->l2ptr" is a pointer to the Level 1 Stream Table Descriptor if
> >> 2-level Stream Table supported.
> >> If the device is protected by IOMMU, SMMUv3 driver will allocate the
> >> "l1_desc->l2ptr” when the device is added to the IOMMU via
> >> arm_smmu_add_device() function and device will be configured in
> >> bypass/abort mode.
> >> Once we assign the device to the domain(arm_smmu_assign_dev() ) smmuv3 hw
> >> will be configured correctly to match the StreamID. If there is a failure
> >> in assigning the device, that case also XEN will not remove the device and
> >> master device still be in bypass/abort mode.
> >
> > I am a bit confused with this answer. Wouldn't this mean that we are
> > "leaking" memory if we fail to assign the device?
>
> No we are not leaking memory as "l1_desc->l2ptr” is still be valid and is
> required for correct SMMUv3 opeation if we fail to assign the device, as in
> that case device will operate in bypass or abort mode.
>
> For example if by any chance there is faulty hw behind SMMUv3 then if we fail
> to assign the device we can configure the device in abort mode in that case
> SMMUv3 hw will silently report abort to device, without any event recorded.
>
> We can also configure the device in bypass mode if we fail to assign. Thats
> why I thought not to free the "l1_desc->l2ptr” if we fail to assign the
> device.
Basically there are two places where l2ptr should be freed:
1) the error out path (err_free_master) in arm_smmu_add_device
2) arm_smmu_remove_device
However, arm_smmu_remove_device is not actually implementedi at all. In
regards to 1), it would be redundant because the memory allocation of
l2ptr is the last operation that could return an error in
arm_smmu_add_device. In other words, if it fails then the memory is not
allocated.
To avoid future memory-leaks in case we add function calls that could
fail after the arm_smmu_init_l2_strtab() call in arm_smmu_add_device, I
think it would be good to have an xfree(l2ptr) under err_free_master.
But given that it is not necessary today I am OK either way.
