Re: [PATCH] iommu/amd: flush IOTLB for specific domains only

2017-05-08 Thread Daniel Drake 
On Wed, Apr 5, 2017 at 9:01 AM, Nath, Arindam  wrote:
>
> >-Original Message-
> >From: Daniel Drake [mailto:dr...@endlessm.com]
> >Sent: Thursday, March 30, 2017 7:15 PM
> >To: Nath, Arindam
> >Cc: j...@8bytes.org; Deucher, Alexander; Bridgman, John; amd-
> >g...@lists.freedesktop.org; iommu@lists.linux-foundation.org; Suthikulpanit,
> >Suravee; Linux Upstreaming Team
> >Subject: Re: [PATCH] iommu/amd: flush IOTLB for specific domains only
> >
> >On Thu, Mar 30, 2017 at 12:23 AM, Nath, Arindam 
> >wrote:
> >> Daniel, did you get chance to test this patch?
> >
> >Not yet. Should we test it alone or alongside "PCI: Blacklist AMD
> >Stoney GPU devices for ATS"?
>
> Daniel, any luck with this patch?

Sorry for the delay. The patch appears to be working fine.

Daniel
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 0/7] Cavium ThunderX2 SMMUv3 errata workarounds

2017-05-08 Thread Linu Cherian 
On Sat May 06, 2017 at 12:22:50AM +0200, Robert Richter wrote:
> On 05.05.17 17:38:04, Geetha sowjanya wrote:
> > From: Linu Cherian 
> > 
> > Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas.
> > 1. Errata ID #74
> >SMMU register alias Page 1 is not implemented
> > 2. Errata ID #126
> >SMMU doesnt support unique IRQ lines and also MSI for gerror, 
> >eventq and cmdq-sync
> > 
> > The following patchset does software workaround for these two erratas.
> > 
> > This series is based on patchset. 
> > https://www.spinics.net/lists/arm-kernel/msg578443.html
> > 
> > Changes from v1:
> >  Since the use of MIDR register is rejected and SMMU_IIDR is broken on this 
> >  silicon, as suggested by Will Deacon modified the patches to use ThunderX2 
> >  SMMUv3 IORT model number to enable errata workaround.
> > 
> > Changes from v2:
> >  Updated "Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt" document 
> > with 
> >  new SMMU option used to enable errata workaround.
> >  
> > Geetha Sowjanya (1):
> >   iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126
> > 
> > Linu Cherian (6):
> >   iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2
> > errata#74.
> >   iommu/arm-smmu-v3: Do resource size checks based on SMMU option
> > PAGE0_REGS_ONLY
> >   ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
> >   iommu/arm-smmu-v3: For ACPI based device probing, set PAGE0_REGS_ONLY
> > option for ThunderX2 SMMUv3 implementations.
> >   ACPI/IORT: Fixup SMMUv3 resource size for Cavium ThunderX2 SMMUv3
> > model
> >   arm64: Documentation: Add Cavium ThunderX2 SMMUv3 erratas
> 
> This split into patches does not look reasonable to me. 1 patch only
> for each workaround should be sufficient.
> 

* Should we not atleast keep the changes in drivers/acpi/iort.c and
 include/acpi/actbl2.h seperate, since they are outside smmuv3 driver ? 

* Probably i can merge the below patches, 
  1. iommu/arm-smmu-v3: Introduce smmu option PAGE0_REGS_ONLY for ThunderX2 
errata#74.
  2. iommu/arm-smmu-v3: Do resource size checks based on SMMU option
 PAGE0_REGS_O

Is that fine ?

Thanks.
-- 
Linu cherian
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH V11 00/11] IOMMU probe deferral support

2017-05-08 Thread Sricharan R 
Hi,

On 5/8/2017 4:53 PM, Marek Szyprowski wrote:
> Hi Sricharan,
> 
> On 2017-04-10 13:20, Sricharan R wrote:
>> This series calls the dma ops configuration for the devices
>> at a generic place so that it works for all busses.
>> The dma_configure_ops for a device is now called during
>> the device_attach callback just before the probe of the
>> bus/driver is called. Similarly dma_deconfigure is called during
>> device/driver_detach path.
>>
>> pci_bus_add_devices(platform/amba)(_device_create/driver_register)
>> | |
>> pci_bus_add_device (device_add/driver_register)
>> | |
>> device_attach   device_initial_probe
>> | |
>> __device_attach_driver__device_attach_driver
>> |
>> driver_probe_device
>> |
>> really_probe
>> |
>> dma_configure
>>
>> Similarly on the device/driver_unregister path __device_release_driver is
>> called which inturn calls dma_deconfigure.
>>
>> Rebased the series against mainline 4.11-rc5. Applies and builds cleanly
>> against iommu-next and with 3-way merge applies on top of linux-next
>> as well (patch #8), because of "ACPI platform MSI support" from
>> Hanjun being merged.
>>* Tested with platform and pci devices for probe deferral
>>and reprobe on arm64 based platform.
> 
> I just noticed that in the meantime the following patch: "arm: dma-mapping:
> Reset the device's dma_ops" (https://patchwork.kernel.org/patch/9434105/ )
> has been dropped from this series.
> 
> Lack of it causes serious issues with deferred probe of devices, which are
> under IOMMU on ARM architecture. Please check where it got lost and push it
> to Russell's patch tracking system ASAP to get it during v4.12-rcX cycle.

ha, bad on me. I posted this patch independently from the series, but was not
added to patch system. Will add it now.

Regards,
 Sricharan

> 
>>
>> Previous post of this series [8].
>>
>> Please note that, i have kept the tested/acked tags intact from V8
>> because V9/10/11 were for more fixes that was added, so the original
>> tags that was given for the functional testing remains the same.
>>
>>   [V11]
>>   * No functional changes.
>>
>>   * Rebased on top of 4.11-rc6.
>>
>>   * Dropped patch#3 from V10, as a result have to make
>> a change in patch#7 to return a 'non-void' to fix a
>> build warning.
>>
>>   * Added Robin's and Rob's tags.
>>
>>   [V10]
>>   * Rebased on top of 4.11-rc5.
>> * Fixed coherent_dma_mask 64bit overflow issue [8]
>> for OF. The fix for OF was added as a separate
>> patch#6, since the issue is true even without probe deferral,
>> but gets reproduced with the probe deferral series.
>> Added Lorenzo's ACPI fix for coherent_dma_mask overflow
>> and the fix for dma_configure getting called more than
>> once for the same device.
>>
>>   * Also fixed an build issue caught by kbuild robot for
>> m68k arch. The issue was dma_(de)configure was not
>> getting defined for !CONFIG_HAS_DMA, so fixed that as well.
>>
>>   [V9]
>>   * Rebased on top of 4.11-rc1.
>>
>>   * Merged Robin's fixes for legacy binding issue,
>> pci devices with no iommu-map property and deferencing
>> of_iommu_table after init.
>> [V8]
>>   * Picked up all the acks and tested tags from Marek and
>> Hanjun for DT and ACPI patches respectively, since
>> no functional changes was done.
>>
>>   * Addressed Minor comments Sinan and Bjorn.
>>
>>   * Added Robin's fix for fixing the deferencing NULL for
>> of_iommu_table after init in patch #2.
>>
>>   * Rebased it on top of linux-next
>>
>>   [V7]
>>   * Updated the subject and commit log for patch #6 as per
>> comments from Lorenzo. No functional changes.
>>
>>   [V6]
>>   * Fixed a bug in dma_configure function pointed out by
>> Robin.
>>   * Reordered the patches as per comments from Robin and
>> Lorenzo.
>>   * Added Tags.
>>
>>   [V5]
>>   * Reworked the pci configuration code hanging outside and
>> pushed it to dma_configure as in PATCH#5,6,7.
>> Also added a couple of patches that Lorenzo provided for
>> correcting the Probe deferring mechanism in case of
>> ACPI devices from here [5].
>>
>>   [V4]
>>   * Took the reworked patches [2] from Robin's branch and
>> rebased on top of Lorenzo's ACPI IORT ARM support series [3].
>>
>>   * Added the patches for moving the dma ops configuration of
>> acpi based devices to probe time as well.
>>   [V3]
>>   * Removed the patch to split dma_masks/dma_ops configuration
>> separately based on review comments that both masks and ops are
>> required only during the device probe time.
>>
>>   * Reworked the series based on Generic DT bindings series.
>>
>>   *

Re: [PATCH v5 15/32] efi: Update efi_mem_type() to return an error rather than 0

2017-05-08 Thread Tom Lendacky 

On 5/7/2017 12:18 PM, Borislav Petkov wrote:

On Tue, Apr 18, 2017 at 04:19:00PM -0500, Tom Lendacky wrote:

The efi_mem_type() function currently returns a 0, which maps to
EFI_RESERVED_TYPE, if the function is unable to find a memmap entry for
the supplied physical address. Returning EFI_RESERVED_TYPE implies that
a memmap entry exists, when it doesn't.  Instead of returning 0, change
the function to return a negative error value when no memmap entry is
found.

Signed-off-by: Tom Lendacky 
---


...


diff --git a/include/linux/efi.h b/include/linux/efi.h
index cd768a1..a27bb3f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -973,7 +973,7 @@ static inline void efi_esrt_init(void) { }
 extern int efi_config_parse_tables(void *config_tables, int count, int sz,
   efi_config_table_type_t *arch_tables);
 extern u64 efi_get_iobase (void);
-extern u32 efi_mem_type (unsigned long phys_addr);
+extern int efi_mem_type (unsigned long phys_addr);


WARNING: space prohibited between function name and open parenthesis '('
#101: FILE: include/linux/efi.h:976:
+extern int efi_mem_type (unsigned long phys_addr);

Please integrate scripts/checkpatch.pl in your patch creation workflow.
Some of the warnings/errors *actually* make sense.


I do/did run scripts/checkpatch.pl against all my patches. In this case
I chose to keep the space in order to stay consistent with some of the
surrounding functions.  No problem though, I can remove the space.

Thanks,
Tom



I know, the other function prototypes have a space too but that's not
our coding style. Looks like this trickled in from ia64, from looking at
arch/ia64/kernel/efi.c.


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

2017-05-08 Thread Robert Richter 
On 08.05.17 16:20:49, Linu Cherian wrote:
> 
> On Mon May 08, 2017 at 12:09:32PM +0200, Robert Richter wrote:
> > On 08.05.17 15:14:37, Linu Cherian wrote:
> > > On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> > > > On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > > > > From: Linu Cherian 
> > > > > 
> > > > > With implementations supporting only page 0 register space,
> > > > > resource size can be 64k as well and hence perform size checks
> > > > > based on SMMU option PAGE0_REGS_ONLY.
> > > > > 
> > > > > For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> > > > > platform_get_resource call, so that SMMU options are set beforehand.
> > > > > 
> > > > > Signed-off-by:  Linu Cherian 
> > > > > Signed-off-by:  Geetha Sowjanya 
> > > > > ---
> > > > >  drivers/iommu/arm-smmu-v3.c | 26 +-
> > > > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > > > index 107b4a6..f027676 100644
> > > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > > @@ -2672,6 +2672,14 @@ static int arm_smmu_device_dt_probe(struct 
> > > > > platform_device *pdev,
> > > > >   return ret;
> > > > >  }
> > > > >  
> > > > > +static unsigned long arm_smmu_resource_size(struct arm_smmu_device 
> > > > > *smmu)
> > > > > +{
> > > > > + if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> > > > > + return SZ_64K;
> > > > > + else
> > > > > + return SZ_128K;
> > > > > +}
> > > > > +
> > > > 
> > > > I think this can be dropped. See below.
> > > > 
> > > > >  static int arm_smmu_device_probe(struct platform_device *pdev)
> > > > >  {
> > > > >   int irq, ret;
> > > > > @@ -2688,9 +2696,17 @@ static int arm_smmu_device_probe(struct 
> > > > > platform_device *pdev)
> > > > >   }
> > > > >   smmu->dev = dev;
> > > > >  
> > > > > + if (dev->of_node) {
> > > > > + ret = arm_smmu_device_dt_probe(pdev, smmu);
> > > > > + } else {
> > > > > + ret = arm_smmu_device_acpi_probe(pdev, smmu);
> > > > > + if (ret == -ENODEV)
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > >   /* Base address */
> > > > >   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > > - if (resource_size(res) + 1 < SZ_128K) {
> > > > > + if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > > > >   dev_err(dev, "MMIO region too small (%pr)\n", res);
> > > > >   return -EINVAL;
> > > > >   }
> > > > 
> > > > Why not just do the follwoing here:
> > > > 
> > > > /* Base address */
> > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > > > dev_err(dev, "MMIO region too small (%pr)\n", res);
> > > > return -EINVAL;
> > > > }
> > > > ioaddr = res->start;
> > > > 
> > > > +   /*
> > > > +* Override the size, for Cavium ThunderX2 implementation
> > > > +* which doesn't support the page 1 SMMU register space.
> > > > +*/
> > > > +   if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> > > > +   res->end = res->size + SZ_64K -1;
> > > > +
> > > > smmu->base = devm_ioremap_resource(dev, res);
> > > > if (IS_ERR(smmu->base))
> > > > return PTR_ERR(smmu->base);
> > > 
> > > 
> > > This might not work, since platform_device_add is being called from
> > > iort.c before the res->end gets fixed up here. 
> > 
> > It should. You added it with 128k and you get it back with
> > platform_get_resource(), but before ioremap you shrink the size to
> > 64k.
> > 
> 
> The smmu devices are located at 64k offsets and not at 128k
> offsets and hence this would be result in resource conflict during
> platform_add_device ?

Right, we have overlapping io spaces then. So we need to change also
iort.c for the fix.

-Robert
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 6/7] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

2017-05-08 Thread Geetha Akula 
On Mon, May 8, 2017 at 4:51 PM, Robin Murphy  wrote:
> On 05/05/17 13:08, Geetha sowjanya wrote:
>> From: Geetha Sowjanya 
>>
>> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
>> lines for gerror, eventq and cmdq-sync.
>>
>> This patch addresses the issue by checking if any interrupt sources are
>> using same irq number, then they are registered as shared irqs.
>>
>> Signed-off-by: Geetha Sowjanya 
>> ---
>>  drivers/iommu/arm-smmu-v3.c | 32 
>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 016b702..46428e7 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -2236,10 +2236,30 @@ static void arm_smmu_setup_msis(struct 
>> arm_smmu_device *smmu)
>>   devm_add_action(dev, arm_smmu_free_msis, dev);
>>  }
>>
>> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq)
>> +{
>> + int match_count = 0;
>> +
>> + if (irq == smmu->evtq.q.irq)
>> + match_count++;
>> + if (irq == smmu->cmdq.q.irq)
>> + match_count++;
>> + if (irq == smmu->gerr_irq)
>> + match_count++;
>> + if (irq == smmu->priq.q.irq)
>> + match_count++;
>> +
>> + if (match_count > 1)
>> + return IRQF_SHARED | IRQF_ONESHOT;
>> +
>> + return 0;
>
> I'd say just have this return IRQF_ONESHOT in the non-shared case...
>
>> +}
>> +
>>  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>  {
>>   int ret, irq;
>>   u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>> + u32 irqflags = 0;
>>
>>   /* Disable IRQs first */
>>   ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
>> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
>> *smmu)
>>   /* Request interrupt lines */
>>   irq = smmu->evtq.q.irq;
>>   if (irq) {
>> + irqflags = get_irq_flags(smmu, irq);
>>   ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>>   arm_smmu_evtq_thread,
>> - IRQF_ONESHOT,
>> + IRQF_ONESHOT | irqflags,
>
> ...and pass get_irq_flags(smmu, irq) directly as the argument here.
>
> The local variable and intermediate logic only seem to add unnecessary
> complexity, given that the two cases we actually end up with are:
>
> IRQF_ONESHOT | 0
>
> vs.
>
> IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT
>
> neither of which looks particularly sensible.
>
> Robin.
>
I will resubmit the patch with suggested changes.

Thank you,
Geetha.



>>   "arm-smmu-v3-evtq", smmu);
>>   if (ret < 0)
>>   dev_warn(smmu->dev, "failed to enable evtq irq\n");
>> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
>> *smmu)
>>
>>   irq = smmu->cmdq.q.irq;
>>   if (irq) {
>> + irqflags = get_irq_flags(smmu, irq);
>>   ret = devm_request_irq(smmu->dev, irq,
>> -arm_smmu_cmdq_sync_handler, 0,
>> +arm_smmu_cmdq_sync_handler, irqflags,
>>  "arm-smmu-v3-cmdq-sync", smmu);
>>   if (ret < 0)
>>   dev_warn(smmu->dev, "failed to enable cmdq-sync 
>> irq\n");
>> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
>> *smmu)
>>
>>   irq = smmu->gerr_irq;
>>   if (irq) {
>> + irqflags = get_irq_flags(smmu, irq);
>>   ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
>> -0, "arm-smmu-v3-gerror", smmu);
>> +irqflags, "arm-smmu-v3-gerror", smmu);
>>   if (ret < 0)
>>   dev_warn(smmu->dev, "failed to enable gerror irq\n");
>>   }
>> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
>> *smmu)
>>   if (smmu->features & ARM_SMMU_FEAT_PRI) {
>>   irq = smmu->priq.q.irq;
>>   if (irq) {
>> + irqflags = get_irq_flags(smmu, irq);
>>   ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>>   arm_smmu_priq_thread,
>> - IRQF_ONESHOT,
>> + IRQF_ONESHOT | 
>> irqflags,
>>   "arm-smmu-v3-priq",
>>   smmu);
>>   if (ret < 0)
>>
>
___
iommu mailing list

Re: [PATCH V11 00/11] IOMMU probe deferral support

2017-05-08 Thread Marek Szyprowski 

Hi Sricharan,

On 2017-04-10 13:20, Sricharan R wrote:

This series calls the dma ops configuration for the devices
at a generic place so that it works for all busses.
The dma_configure_ops for a device is now called during
the device_attach callback just before the probe of the
bus/driver is called. Similarly dma_deconfigure is called during
device/driver_detach path.

pci_bus_add_devices(platform/amba)(_device_create/driver_register)
| |
pci_bus_add_device (device_add/driver_register)
| |
device_attach   device_initial_probe
| |
__device_attach_driver__device_attach_driver
|
driver_probe_device
|
really_probe
|
dma_configure

Similarly on the device/driver_unregister path __device_release_driver is
called which inturn calls dma_deconfigure.

Rebased the series against mainline 4.11-rc5. Applies and builds cleanly
against iommu-next and with 3-way merge applies on top of linux-next
as well (patch #8), because of "ACPI platform MSI support" from
Hanjun being merged.
   
* Tested with platform and pci devices for probe deferral

   and reprobe on arm64 based platform.


I just noticed that in the meantime the following patch: "arm: dma-mapping:
Reset the device's dma_ops" (https://patchwork.kernel.org/patch/9434105/ )
has been dropped from this series.

Lack of it causes serious issues with deferred probe of devices, which are
under IOMMU on ARM architecture. Please check where it got lost and push it
to Russell's patch tracking system ASAP to get it during v4.12-rcX cycle.



Previous post of this series [8].

Please note that, i have kept the tested/acked tags intact from V8
because V9/10/11 were for more fixes that was added, so the original
tags that was given for the functional testing remains the same.

  [V11]
  * No functional changes.

  * Rebased on top of 4.11-rc6.

  * Dropped patch#3 from V10, as a result have to make
a change in patch#7 to return a 'non-void' to fix a
build warning.

  * Added Robin's and Rob's tags.

  [V10]
  * Rebased on top of 4.11-rc5.
  
  * Fixed coherent_dma_mask 64bit overflow issue [8]

for OF. The fix for OF was added as a separate
patch#6, since the issue is true even without probe deferral,
but gets reproduced with the probe deferral series.
Added Lorenzo's ACPI fix for coherent_dma_mask overflow
and the fix for dma_configure getting called more than
once for the same device.

  * Also fixed an build issue caught by kbuild robot for
m68k arch. The issue was dma_(de)configure was not
getting defined for !CONFIG_HAS_DMA, so fixed that as well.

  [V9]
  * Rebased on top of 4.11-rc1.

  * Merged Robin's fixes for legacy binding issue,
pci devices with no iommu-map property and deferencing
of_iommu_table after init.
  
  [V8]

  * Picked up all the acks and tested tags from Marek and
Hanjun for DT and ACPI patches respectively, since
no functional changes was done.

  * Addressed Minor comments Sinan and Bjorn.

  * Added Robin's fix for fixing the deferencing NULL for
of_iommu_table after init in patch #2.

  * Rebased it on top of linux-next

  [V7]
  * Updated the subject and commit log for patch #6 as per
comments from Lorenzo. No functional changes.

  [V6]
  * Fixed a bug in dma_configure function pointed out by
Robin.
  * Reordered the patches as per comments from Robin and
Lorenzo.
  * Added Tags.

  [V5]
  * Reworked the pci configuration code hanging outside and
pushed it to dma_configure as in PATCH#5,6,7.
Also added a couple of patches that Lorenzo provided for
correcting the Probe deferring mechanism in case of
ACPI devices from here [5].

  [V4]
  * Took the reworked patches [2] from Robin's branch and
rebased on top of Lorenzo's ACPI IORT ARM support series [3].

  * Added the patches for moving the dma ops configuration of
acpi based devices to probe time as well.
  [V3]
  * Removed the patch to split dma_masks/dma_ops configuration
separately based on review comments that both masks and ops are
required only during the device probe time.

  * Reworked the series based on Generic DT bindings series.

  * Added call to iommu's remove_device in the cleanup path for arm and
arm64.

  * Removed the notifier trick in arm64 to handle early device
registration.

  * Added reset of dma_ops in cleanup path for arm based on comments.

  * Fixed the pci_iommu_configure path and tested with PCI device as
well.
  
  * Fixed a bug to return the correct iommu_ops from patch 7 [4] in

last post.

  * Fixed few other cosmetic comments.
   
  [V2]

  * Updated the Initial post

Re: [PATCH v3 6/7] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126

2017-05-08 Thread Robin Murphy 
On 05/05/17 13:08, Geetha sowjanya wrote:
> From: Geetha Sowjanya 
> 
> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
> lines for gerror, eventq and cmdq-sync.
> 
> This patch addresses the issue by checking if any interrupt sources are
> using same irq number, then they are registered as shared irqs.
> 
> Signed-off-by: Geetha Sowjanya 
> ---
>  drivers/iommu/arm-smmu-v3.c | 32 
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 016b702..46428e7 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2236,10 +2236,30 @@ static void arm_smmu_setup_msis(struct 
> arm_smmu_device *smmu)
>   devm_add_action(dev, arm_smmu_free_msis, dev);
>  }
>  
> +static int get_irq_flags(struct arm_smmu_device *smmu, int irq)
> +{
> + int match_count = 0;
> +
> + if (irq == smmu->evtq.q.irq)
> + match_count++;
> + if (irq == smmu->cmdq.q.irq)
> + match_count++;
> + if (irq == smmu->gerr_irq)
> + match_count++;
> + if (irq == smmu->priq.q.irq)
> + match_count++;
> +
> + if (match_count > 1)
> + return IRQF_SHARED | IRQF_ONESHOT;
> +
> + return 0;

I'd say just have this return IRQF_ONESHOT in the non-shared case...

> +}
> +
>  static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  {
>   int ret, irq;
>   u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> + u32 irqflags = 0;
>  
>   /* Disable IRQs first */
>   ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
> @@ -2254,9 +2274,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
> *smmu)
>   /* Request interrupt lines */
>   irq = smmu->evtq.q.irq;
>   if (irq) {
> + irqflags = get_irq_flags(smmu, irq);
>   ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>   arm_smmu_evtq_thread,
> - IRQF_ONESHOT,
> + IRQF_ONESHOT | irqflags,

...and pass get_irq_flags(smmu, irq) directly as the argument here.

The local variable and intermediate logic only seem to add unnecessary
complexity, given that the two cases we actually end up with are:

IRQF_ONESHOT | 0

vs.

IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT

neither of which looks particularly sensible.

Robin.

>   "arm-smmu-v3-evtq", smmu);
>   if (ret < 0)
>   dev_warn(smmu->dev, "failed to enable evtq irq\n");
> @@ -2264,8 +2285,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
> *smmu)
>  
>   irq = smmu->cmdq.q.irq;
>   if (irq) {
> + irqflags = get_irq_flags(smmu, irq);
>   ret = devm_request_irq(smmu->dev, irq,
> -arm_smmu_cmdq_sync_handler, 0,
> +arm_smmu_cmdq_sync_handler, irqflags,
>  "arm-smmu-v3-cmdq-sync", smmu);
>   if (ret < 0)
>   dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n");
> @@ -2273,8 +2295,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
> *smmu)
>  
>   irq = smmu->gerr_irq;
>   if (irq) {
> + irqflags = get_irq_flags(smmu, irq);
>   ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler,
> -0, "arm-smmu-v3-gerror", smmu);
> +irqflags, "arm-smmu-v3-gerror", smmu);
>   if (ret < 0)
>   dev_warn(smmu->dev, "failed to enable gerror irq\n");
>   }
> @@ -2282,9 +2305,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device 
> *smmu)
>   if (smmu->features & ARM_SMMU_FEAT_PRI) {
>   irq = smmu->priq.q.irq;
>   if (irq) {
> + irqflags = get_irq_flags(smmu, irq);
>   ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>   arm_smmu_priq_thread,
> - IRQF_ONESHOT,
> + IRQF_ONESHOT | irqflags,
>   "arm-smmu-v3-priq",
>   smmu);
>   if (ret < 0)
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 03/20] intel_iommu: add "svm" option

2017-05-08 Thread Peter Xu 
On Mon, May 08, 2017 at 10:38:09AM +, Liu, Yi L wrote:
> On Thu, 27 Apr 2017 18:53:17 +0800
> Peter Xu  wrote:
> 
> > On Wed, Apr 26, 2017 at 06:06:33PM +0800, Liu, Yi L wrote:
> > > Expose "Shared Virtual Memory" to guest by using "svm" option.
> > > Also use "svm" to expose SVM related capabilities to guest.
> > > e.g. "-device intel-iommu, svm=on"
> > >
> > > Signed-off-by: Liu, Yi L 
> > > ---
> > >  hw/i386/intel_iommu.c  | 10 ++
> > >  hw/i386/intel_iommu_internal.h |  5 +
> > > include/hw/i386/intel_iommu.h  |  1 +
> > >  3 files changed, 16 insertions(+)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > > bf98fa5..ba1e7eb 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -2453,6 +2453,7 @@ static Property vtd_properties[] = {
> > >  DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false),
> > >  DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,
> > FALSE),
> > >  DEFINE_PROP_BOOL("ecs", IntelIOMMUState, ecs, FALSE),
> > > +DEFINE_PROP_BOOL("svm", IntelIOMMUState, svm, FALSE),
> > >  DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >
> > > @@ -2973,6 +2974,15 @@ static void vtd_init(IntelIOMMUState *s)
> > >  s->ecap |= VTD_ECAP_ECS;
> > >  }
> > >
> > > +if (s->svm) {
> > > +if (!s->ecs || !x86_iommu->pt_supported || !s->caching_mode) {
> > > +error_report("Need to set ecs, pt, caching-mode for svm");
> > > +exit(1);
> > > +}
> > > +s->cap |= VTD_CAP_DWD | VTD_CAP_DRD;
> > > +s->ecap |= VTD_ECAP_PRS | VTD_ECAP_PTS | VTD_ECAP_PASID28;
> > > +}
> > > +
> > >  if (s->caching_mode) {
> > >  s->cap |= VTD_CAP_CM;
> > >  }
> > > diff --git a/hw/i386/intel_iommu_internal.h
> > > b/hw/i386/intel_iommu_internal.h index 71a1c1e..f2a7d12 100644
> > > --- a/hw/i386/intel_iommu_internal.h
> > > +++ b/hw/i386/intel_iommu_internal.h
> > > @@ -191,6 +191,9 @@
> > >  #define VTD_ECAP_PT (1ULL << 6)
> > >  #define VTD_ECAP_MHMV   (15ULL << 20)
> > >  #define VTD_ECAP_ECS(1ULL << 24)
> > > +#define VTD_ECAP_PASID28(1ULL << 28)
> > 
> > Could I ask what's this bit? On my spec, it says this bit is reserved and 
> > defunct (spec
> > version: June 2016).
> 
> As Ashok confirmed, yes it should be bit 40. would update it.

Ok.

> 
> > > +#define VTD_ECAP_PRS(1ULL << 29)
> > > +#define VTD_ECAP_PTS(0xeULL << 35)
> > 
> > Would it better we avoid using 0xe here, or at least add some comment?
> 
> For this value, it must be no more than the bits host supports. So it may be
> better to have a default value and meanwhile expose an option to let user
> set it. how about your opinion?

I think a more important point is that we need to make sure this value
is no larger than hardware support? Since you are also working on the
vfio interface for virt-svm... would it be possible that we can talk
to kernel in some way so that we can know the supported pasid size in
host IOMMU? So that when guest specifies something bigger, we can stop
the user.

I don't know the practical value for this field, if it's static
enough, I think it's also okay we make it static here as well. But
again, I would prefer at least some comment, like:

  /* Value N indicates PASID field of N+1 bits, here 0xe stands for.. */

> 
> > 
> > >
> > >  /* CAP_REG */
> > >  /* (offset >> 4) << 24 */
> > > @@ -207,6 +210,8 @@
> > >  #define VTD_CAP_PSI (1ULL << 39)
> > >  #define VTD_CAP_SLLPS   ((1ULL << 34) | (1ULL << 35))
> > >  #define VTD_CAP_CM  (1ULL << 7)
> > > +#define VTD_CAP_DWD (1ULL << 54)
> > > +#define VTD_CAP_DRD (1ULL << 55)
> > 
> > Just to confirm: after this series, we should support drain read/write 
> > then, right?
> 
> I haven’t done special process against it in IOMMU emulator. It's set to keep
> consistence with VT-d spec since DWD and DRW is required capability when
> PASID it reported as Set. However, I think it should be fine if guest issue QI
> with drain read/write set in the descriptor. Host should be able to process 
> it.

I see. IIUC the point here is we need to deliver these requests to
host IOMMU, and I guess we need to be able to do this in a synchronous
way as well.

Thanks,

-- 
Peter Xu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

2017-05-08 Thread Geetha Akula 
On Mon, May 8, 2017 at 3:39 PM, Robert Richter
 wrote:
> On 08.05.17 15:14:37, Linu Cherian wrote:
>> On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
>> > On 05.05.17 17:38:06, Geetha sowjanya wrote:
>> > > From: Linu Cherian 
>> > >
>> > > With implementations supporting only page 0 register space,
>> > > resource size can be 64k as well and hence perform size checks
>> > > based on SMMU option PAGE0_REGS_ONLY.
>> > >
>> > > For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
>> > > platform_get_resource call, so that SMMU options are set beforehand.
>> > >
>> > > Signed-off-by:  Linu Cherian 
>> > > Signed-off-by:  Geetha Sowjanya 
>> > > ---
>> > >  drivers/iommu/arm-smmu-v3.c | 26 +-
>> > >  1 file changed, 17 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> > > index 107b4a6..f027676 100644
>> > > --- a/drivers/iommu/arm-smmu-v3.c
>> > > +++ b/drivers/iommu/arm-smmu-v3.c
>> > > @@ -2672,6 +2672,14 @@ static int arm_smmu_device_dt_probe(struct 
>> > > platform_device *pdev,
>> > >   return ret;
>> > >  }
>> > >
>> > > +static unsigned long arm_smmu_resource_size(struct arm_smmu_device 
>> > > *smmu)
>> > > +{
>> > > + if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
>> > > + return SZ_64K;
>> > > + else
>> > > + return SZ_128K;
>> > > +}
>> > > +
>> >
>> > I think this can be dropped. See below.
>> >
>> > >  static int arm_smmu_device_probe(struct platform_device *pdev)
>> > >  {
>> > >   int irq, ret;
>> > > @@ -2688,9 +2696,17 @@ static int arm_smmu_device_probe(struct 
>> > > platform_device *pdev)
>> > >   }
>> > >   smmu->dev = dev;
>> > >
>> > > + if (dev->of_node) {
>> > > + ret = arm_smmu_device_dt_probe(pdev, smmu);
>> > > + } else {
>> > > + ret = arm_smmu_device_acpi_probe(pdev, smmu);
>> > > + if (ret == -ENODEV)
>> > > + return ret;
>> > > + }
>> > > +
>> > >   /* Base address */
>> > >   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > > - if (resource_size(res) + 1 < SZ_128K) {
>> > > + if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
>> > >   dev_err(dev, "MMIO region too small (%pr)\n", res);
>> > >   return -EINVAL;
>> > >   }
>> >
>> > Why not just do the follwoing here:
>> >
>> > /* Base address */
>> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
>> > dev_err(dev, "MMIO region too small (%pr)\n", res);
>> > return -EINVAL;
>> > }
>> > ioaddr = res->start;
>> >
>> > +   /*
>> > +* Override the size, for Cavium ThunderX2 implementation
>> > +* which doesn't support the page 1 SMMU register space.
>> > +*/
>> > +   if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
>> > +   res->end = res->size + SZ_64K -1;
>> > +
>> > smmu->base = devm_ioremap_resource(dev, res);
>> > if (IS_ERR(smmu->base))
>> > return PTR_ERR(smmu->base);
>>
>>
>> This might not work, since platform_device_add is being called from
>> iort.c before the res->end gets fixed up here.
>
> It should. You added it with 128k and you get it back with
> platform_get_resource(), but before ioremap you shrink the size to
> 64k.
>
> -Robert

Hi Robert,

Linu is right. You are missing the sequence of event. If we skip the
changes in iort file, smmu initialization in acpi fails.


[3.721647] ACPI: IORT: iort_add_smmu_platform_device
[3.726826] platform arm-smmu-v3.1.auto: failed to claim resource
0: [mem 0x40231-0x40232]
[3.736052] arm-smmu-v3 arm-smmu-v3.0.auto: option mask 0x1
[3.741753] arm_smmu_device_probe


Thank you,
Geetha.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

2017-05-08 Thread Linu Cherian 

On Mon May 08, 2017 at 12:09:32PM +0200, Robert Richter wrote:
> On 08.05.17 15:14:37, Linu Cherian wrote:
> > On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> > > On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > > > From: Linu Cherian 
> > > > 
> > > > With implementations supporting only page 0 register space,
> > > > resource size can be 64k as well and hence perform size checks
> > > > based on SMMU option PAGE0_REGS_ONLY.
> > > > 
> > > > For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> > > > platform_get_resource call, so that SMMU options are set beforehand.
> > > > 
> > > > Signed-off-by:  Linu Cherian 
> > > > Signed-off-by:  Geetha Sowjanya 
> > > > ---
> > > >  drivers/iommu/arm-smmu-v3.c | 26 +-
> > > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > > index 107b4a6..f027676 100644
> > > > --- a/drivers/iommu/arm-smmu-v3.c
> > > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > > @@ -2672,6 +2672,14 @@ static int arm_smmu_device_dt_probe(struct 
> > > > platform_device *pdev,
> > > > return ret;
> > > >  }
> > > >  
> > > > +static unsigned long arm_smmu_resource_size(struct arm_smmu_device 
> > > > *smmu)
> > > > +{
> > > > +   if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> > > > +   return SZ_64K;
> > > > +   else
> > > > +   return SZ_128K;
> > > > +}
> > > > +
> > > 
> > > I think this can be dropped. See below.
> > > 
> > > >  static int arm_smmu_device_probe(struct platform_device *pdev)
> > > >  {
> > > > int irq, ret;
> > > > @@ -2688,9 +2696,17 @@ static int arm_smmu_device_probe(struct 
> > > > platform_device *pdev)
> > > > }
> > > > smmu->dev = dev;
> > > >  
> > > > +   if (dev->of_node) {
> > > > +   ret = arm_smmu_device_dt_probe(pdev, smmu);
> > > > +   } else {
> > > > +   ret = arm_smmu_device_acpi_probe(pdev, smmu);
> > > > +   if (ret == -ENODEV)
> > > > +   return ret;
> > > > +   }
> > > > +
> > > > /* Base address */
> > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > -   if (resource_size(res) + 1 < SZ_128K) {
> > > > +   if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > > > dev_err(dev, "MMIO region too small (%pr)\n", res);
> > > > return -EINVAL;
> > > > }
> > > 
> > > Why not just do the follwoing here:
> > > 
> > >   /* Base address */
> > >   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >   if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > >   dev_err(dev, "MMIO region too small (%pr)\n", res);
> > >   return -EINVAL;
> > >   }
> > >   ioaddr = res->start;
> > > 
> > > + /*
> > > +  * Override the size, for Cavium ThunderX2 implementation
> > > +  * which doesn't support the page 1 SMMU register space.
> > > +  */
> > > + if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> > > + res->end = res->size + SZ_64K -1;
> > > +
> > >   smmu->base = devm_ioremap_resource(dev, res);
> > >   if (IS_ERR(smmu->base))
> > >   return PTR_ERR(smmu->base);
> > 
> > 
> > This might not work, since platform_device_add is being called from
> > iort.c before the res->end gets fixed up here. 
> 
> It should. You added it with 128k and you get it back with
> platform_get_resource(), but before ioremap you shrink the size to
> 64k.
> 

The smmu devices are located at 64k offsets and not at 128k
offsets and hence this would be result in resource conflict during
platform_add_device ?

Code snippet from platform_add_device:

for (i = 0; i < pdev->num_resources; i++) {
struct resource *p, *r = >resource[i];

if (r->name == NULL)
r->name = dev_name(>dev);

p = r->parent;
if (!p) {
if (resource_type(r) == IORESOURCE_MEM)
p = _resource;
else if (resource_type(r) == IORESOURCE_IO)
p = _resource;
}

if (p && insert_resource(p, r)) {
dev_err(>dev, "failed to claim resource %d: 
%pR\n", i, r);
ret = -EBUSY;
goto failed;
}
}







-- 
Linu cherian
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [RFC PATCH 03/20] intel_iommu: add "svm" option

2017-05-08 Thread Liu, Yi L 
On Thu, 27 Apr 2017 18:53:17 +0800
Peter Xu  wrote:

> On Wed, Apr 26, 2017 at 06:06:33PM +0800, Liu, Yi L wrote:
> > Expose "Shared Virtual Memory" to guest by using "svm" option.
> > Also use "svm" to expose SVM related capabilities to guest.
> > e.g. "-device intel-iommu, svm=on"
> >
> > Signed-off-by: Liu, Yi L 
> > ---
> >  hw/i386/intel_iommu.c  | 10 ++
> >  hw/i386/intel_iommu_internal.h |  5 +
> > include/hw/i386/intel_iommu.h  |  1 +
> >  3 files changed, 16 insertions(+)
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > bf98fa5..ba1e7eb 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2453,6 +2453,7 @@ static Property vtd_properties[] = {
> >  DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false),
> >  DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,
> FALSE),
> >  DEFINE_PROP_BOOL("ecs", IntelIOMMUState, ecs, FALSE),
> > +DEFINE_PROP_BOOL("svm", IntelIOMMUState, svm, FALSE),
> >  DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > @@ -2973,6 +2974,15 @@ static void vtd_init(IntelIOMMUState *s)
> >  s->ecap |= VTD_ECAP_ECS;
> >  }
> >
> > +if (s->svm) {
> > +if (!s->ecs || !x86_iommu->pt_supported || !s->caching_mode) {
> > +error_report("Need to set ecs, pt, caching-mode for svm");
> > +exit(1);
> > +}
> > +s->cap |= VTD_CAP_DWD | VTD_CAP_DRD;
> > +s->ecap |= VTD_ECAP_PRS | VTD_ECAP_PTS | VTD_ECAP_PASID28;
> > +}
> > +
> >  if (s->caching_mode) {
> >  s->cap |= VTD_CAP_CM;
> >  }
> > diff --git a/hw/i386/intel_iommu_internal.h
> > b/hw/i386/intel_iommu_internal.h index 71a1c1e..f2a7d12 100644
> > --- a/hw/i386/intel_iommu_internal.h
> > +++ b/hw/i386/intel_iommu_internal.h
> > @@ -191,6 +191,9 @@
> >  #define VTD_ECAP_PT (1ULL << 6)
> >  #define VTD_ECAP_MHMV   (15ULL << 20)
> >  #define VTD_ECAP_ECS(1ULL << 24)
> > +#define VTD_ECAP_PASID28(1ULL << 28)
> 
> Could I ask what's this bit? On my spec, it says this bit is reserved and 
> defunct (spec
> version: June 2016).

As Ashok confirmed, yes it should be bit 40. would update it.

> > +#define VTD_ECAP_PRS(1ULL << 29)
> > +#define VTD_ECAP_PTS(0xeULL << 35)
> 
> Would it better we avoid using 0xe here, or at least add some comment?

For this value, it must be no more than the bits host supports. So it may be
better to have a default value and meanwhile expose an option to let user
set it. how about your opinion?

> 
> >
> >  /* CAP_REG */
> >  /* (offset >> 4) << 24 */
> > @@ -207,6 +210,8 @@
> >  #define VTD_CAP_PSI (1ULL << 39)
> >  #define VTD_CAP_SLLPS   ((1ULL << 34) | (1ULL << 35))
> >  #define VTD_CAP_CM  (1ULL << 7)
> > +#define VTD_CAP_DWD (1ULL << 54)
> > +#define VTD_CAP_DRD (1ULL << 55)
> 
> Just to confirm: after this series, we should support drain read/write then, 
> right?

I haven’t done special process against it in IOMMU emulator. It's set to keep
consistence with VT-d spec since DWD and DRW is required capability when
PASID it reported as Set. However, I think it should be fine if guest issue QI
with drain read/write set in the descriptor. Host should be able to process it.

Thanks,
Yi L
> >
> >  /* Supported Adjusted Guest Address Widths */
> >  #define VTD_CAP_SAGAW_SHIFT 8
> > diff --git a/include/hw/i386/intel_iommu.h
> > b/include/hw/i386/intel_iommu.h index ae21fe5..8981615 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -267,6 +267,7 @@ struct IntelIOMMUState {
> >
> >  bool caching_mode;  /* RO - is cap CM enabled? */
> >  bool ecs;   /* Extended Context Support */
> > +bool svm;   /* Shared Virtual Memory */
> >
> >  dma_addr_t root;/* Current root table pointer */
> >  bool root_extended; /* Type of root table (extended or 
> > not) */
> > --
> > 1.9.1
> >
> 
> --
> Peter Xu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

2017-05-08 Thread Robert Richter 
On 08.05.17 15:14:37, Linu Cherian wrote:
> On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> > On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > > From: Linu Cherian 
> > > 
> > > With implementations supporting only page 0 register space,
> > > resource size can be 64k as well and hence perform size checks
> > > based on SMMU option PAGE0_REGS_ONLY.
> > > 
> > > For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> > > platform_get_resource call, so that SMMU options are set beforehand.
> > > 
> > > Signed-off-by:  Linu Cherian 
> > > Signed-off-by:  Geetha Sowjanya 
> > > ---
> > >  drivers/iommu/arm-smmu-v3.c | 26 +-
> > >  1 file changed, 17 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > > index 107b4a6..f027676 100644
> > > --- a/drivers/iommu/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm-smmu-v3.c
> > > @@ -2672,6 +2672,14 @@ static int arm_smmu_device_dt_probe(struct 
> > > platform_device *pdev,
> > >   return ret;
> > >  }
> > >  
> > > +static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)
> > > +{
> > > + if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> > > + return SZ_64K;
> > > + else
> > > + return SZ_128K;
> > > +}
> > > +
> > 
> > I think this can be dropped. See below.
> > 
> > >  static int arm_smmu_device_probe(struct platform_device *pdev)
> > >  {
> > >   int irq, ret;
> > > @@ -2688,9 +2696,17 @@ static int arm_smmu_device_probe(struct 
> > > platform_device *pdev)
> > >   }
> > >   smmu->dev = dev;
> > >  
> > > + if (dev->of_node) {
> > > + ret = arm_smmu_device_dt_probe(pdev, smmu);
> > > + } else {
> > > + ret = arm_smmu_device_acpi_probe(pdev, smmu);
> > > + if (ret == -ENODEV)
> > > + return ret;
> > > + }
> > > +
> > >   /* Base address */
> > >   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > - if (resource_size(res) + 1 < SZ_128K) {
> > > + if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > >   dev_err(dev, "MMIO region too small (%pr)\n", res);
> > >   return -EINVAL;
> > >   }
> > 
> > Why not just do the follwoing here:
> > 
> > /* Base address */
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > dev_err(dev, "MMIO region too small (%pr)\n", res);
> > return -EINVAL;
> > }
> > ioaddr = res->start;
> > 
> > +   /*
> > +* Override the size, for Cavium ThunderX2 implementation
> > +* which doesn't support the page 1 SMMU register space.
> > +*/
> > +   if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> > +   res->end = res->size + SZ_64K -1;
> > +
> > smmu->base = devm_ioremap_resource(dev, res);
> > if (IS_ERR(smmu->base))
> > return PTR_ERR(smmu->base);
> 
> 
> This might not work, since platform_device_add is being called from
> iort.c before the res->end gets fixed up here. 

It should. You added it with 128k and you get it back with
platform_get_resource(), but before ioremap you shrink the size to
64k.

-Robert
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

2017-05-08 Thread Robert Richter 
On 08.05.17 10:59:46, Robin Murphy wrote:
> On 08/05/17 10:17, Linu Cherian wrote:
> > This actually results in more lines of changes. If you think the below
> > approach is still better, will post a V4 of this series with this change.
> 
> Why not just do this?:
> 
> static inline unsigned long page1_offset_adjust(
>   unsigned long off, struct arm_smmu_device *smmu)
> {
>   if (off > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
>   return (off - SZ_64K);
> 
>   return off;
> }
> 
> AFAICS that should be the least disruptive way to go about it.

Yeah, let's go with this.

Thanks Robin,

-Robert
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

2017-05-08 Thread Robin Murphy 
On 08/05/17 10:17, Linu Cherian wrote:
> On Sat May 06, 2017 at 01:03:28AM +0200, Robert Richter wrote:
>> On 05.05.17 17:38:05, Geetha sowjanya wrote:
>>> From: Linu Cherian 
>>>
>>> Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
>>> and PAGE0_REGS_ONLY option will be enabled as an errata workaround.
>>>
>>> This option when turned on, replaces all page 1 offsets used for
>>> EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
>>>
>>> Signed-off-by: Linu Cherian 
>>> Signed-off-by: Geetha Sowjanya 
>>> ---
>>>  .../devicetree/bindings/iommu/arm,smmu-v3.txt  |  6 +++
>>>  drivers/iommu/arm-smmu-v3.c| 44 
>>> --
>>>  2 files changed, 38 insertions(+), 12 deletions(-)
>>
>>> @@ -1995,8 +2011,10 @@ static int arm_smmu_init_queues(struct 
>>> arm_smmu_device *smmu)
>>> if (!(smmu->features & ARM_SMMU_FEAT_PRI))
>>> return 0;
>>>  
>>> -   return arm_smmu_init_one_queue(smmu, >priq.q, ARM_SMMU_PRIQ_PROD,
>>> -  ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
>>> +   return arm_smmu_init_one_queue(smmu, >priq.q,
>>> +  ARM_SMMU_PRIQ_PROD(smmu),
>>> +  ARM_SMMU_PRIQ_CONS(smmu),
>>> +  PRIQ_ENT_DWORDS);
>>
>> I would also suggest Robin's idea from the v1 review here. This works
>> if we rework arm_smmu_init_one_queue() to pass addresses instead of
>> offsets.
>>
>> This would make these widespread offset calculations obsolete.
>>
> 
> Have pasted here the relevant changes for doing fixups on smmu base instead
> of offset to get feedback. 
> 
> This actually results in more lines of changes. If you think the below
> approach is still better, will post a V4 of this series with this change.

Why not just do this?:

static inline unsigned long page1_offset_adjust(
unsigned long off, struct arm_smmu_device *smmu)
{
if (off > SZ_64K && ARM_SMMU_PAGE0_REGS_ONLY(smmu))
return (off - SZ_64K);

return off;
}

AFAICS that should be the least disruptive way to go about it.

Robin.

> 
> 
> +static inline unsigned long arm_smmu_page1_base(
> + struct arm_smmu_device *smmu)
> +{
> + if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> + return smmu->base;
> + else
> + return smmu->base + SZ_64K;
> +}
> +
> 
> @@ -1948,8 +1962,8 @@ static void arm_smmu_put_resv_regions(struct device 
> *dev,
>  /* Probing and initialisation functions */
>  static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  struct arm_smmu_queue *q,
> -unsigned long prod_off,
> -unsigned long cons_off,
> +unsigned long prod_addr,
> +unsigned long cons_addr,
>  size_t dwords)
>  {
>   size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
> @@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct 
> arm_smmu_device *smmu,
>   return -ENOMEM;
>   }
>  
> - q->prod_reg = smmu->base + prod_off;
> - q->cons_reg = smmu->base + cons_off;
> + q->prod_reg = prod_addr;
> + q->cons_reg = cons_addr;
>   q->ent_dwords   = dwords;
>  
>   q->q_base  = Q_BASE_RWA;
> @@ -1977,17 +1991,25 @@ static int arm_smmu_init_one_queue(struct 
> arm_smmu_device *smmu,
>  static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  {
>   int ret;
> + unsigned long page1_base, page0_base;
> +
> + page0_base = smmu->base;
> + page1_base = arm_smmu_page1_base(smmu);
>  
>   /* cmdq */
>   spin_lock_init(>cmdq.lock);
> - ret = arm_smmu_init_one_queue(smmu, >cmdq.q, ARM_SMMU_CMDQ_PROD,
> -   ARM_SMMU_CMDQ_CONS, CMDQ_ENT_DWORDS);
> + ret = arm_smmu_init_one_queue(smmu, >cmdq.q, 
> +   page0_base + ARM_SMMU_CMDQ_PROD,
> +   page0_base + ARM_SMMU_CMDQ_CONS, 
> +   CMDQ_ENT_DWORDS);
>   if (ret)
>   return ret;
>  
>   /* evtq */
> - ret = arm_smmu_init_one_queue(smmu, >evtq.q, ARM_SMMU_EVTQ_PROD,
> -   ARM_SMMU_EVTQ_CONS, EVTQ_ENT_DWORDS);
> + ret = arm_smmu_init_one_queue(smmu, >evtq.q,
> +   page1_base + ARM_SMMU_EVTQ_PROD,
> +   page1_base + ARM_SMMU_EVTQ_CONS,
> +   EVTQ_ENT_DWORDS);
>   if (ret)
>   return ret;
>  
> @@ -1995,8 +2017,10 @@ static int arm_smmu_init_queues(struct arm_smmu_device 
> *smmu)
>   if (!(smmu->features & ARM_SMMU_FEAT_PRI))
>   return 0;
>  
> - return arm_smmu_init_one_queue(smmu,

Re: [PATCH v3 2/7] iommu/arm-smmu-v3: Do resource size checks based on SMMU

2017-05-08 Thread Linu Cherian 
On Sat May 06, 2017 at 12:18:44AM +0200, Robert Richter wrote:
> On 05.05.17 17:38:06, Geetha sowjanya wrote:
> > From: Linu Cherian 
> > 
> > With implementations supporting only page 0 register space,
> > resource size can be 64k as well and hence perform size checks
> > based on SMMU option PAGE0_REGS_ONLY.
> > 
> > For this, arm_smmu_device_dt_probe/acpi_probe has been moved before
> > platform_get_resource call, so that SMMU options are set beforehand.
> > 
> > Signed-off-by:  Linu Cherian 
> > Signed-off-by:  Geetha Sowjanya 
> > ---
> >  drivers/iommu/arm-smmu-v3.c | 26 +-
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> > index 107b4a6..f027676 100644
> > --- a/drivers/iommu/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -2672,6 +2672,14 @@ static int arm_smmu_device_dt_probe(struct 
> > platform_device *pdev,
> > return ret;
> >  }
> >  
> > +static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)
> > +{
> > +   if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> > +   return SZ_64K;
> > +   else
> > +   return SZ_128K;
> > +}
> > +
> 
> I think this can be dropped. See below.
> 
> >  static int arm_smmu_device_probe(struct platform_device *pdev)
> >  {
> > int irq, ret;
> > @@ -2688,9 +2696,17 @@ static int arm_smmu_device_probe(struct 
> > platform_device *pdev)
> > }
> > smmu->dev = dev;
> >  
> > +   if (dev->of_node) {
> > +   ret = arm_smmu_device_dt_probe(pdev, smmu);
> > +   } else {
> > +   ret = arm_smmu_device_acpi_probe(pdev, smmu);
> > +   if (ret == -ENODEV)
> > +   return ret;
> > +   }
> > +
> > /* Base address */
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -   if (resource_size(res) + 1 < SZ_128K) {
> > +   if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
> > dev_err(dev, "MMIO region too small (%pr)\n", res);
> > return -EINVAL;
> > }
> 
> Why not just do the follwoing here:
> 
>   /* Base address */
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   if (resource_size(res) + 1 < arm_smmu_resource_size(smmu)) {
>   dev_err(dev, "MMIO region too small (%pr)\n", res);
>   return -EINVAL;
>   }
>   ioaddr = res->start;
> 
> + /*
> +  * Override the size, for Cavium ThunderX2 implementation
> +  * which doesn't support the page 1 SMMU register space.
> +  */
> + if (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY)
> + res->end = res->size + SZ_64K -1;
> +
>   smmu->base = devm_ioremap_resource(dev, res);
>   if (IS_ERR(smmu->base))
>   return PTR_ERR(smmu->base);


This might not work, since platform_device_add is being called from
iort.c before the res->end gets fixed up here. 


-- 
Linu cherian
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/2] Fix incorrect warning from dma-debug

2017-05-08 Thread Robin Murphy 
On 07/05/17 01:06, Jon Masters wrote:
> On 05/09/2016 06:00 AM, Robin Murphy wrote:
>> On 09/05/16 10:37, Robin Murphy wrote:
>>> Hi Niklas,
>>>
>>> On 08/05/16 11:59, Niklas Söderlund wrote:
 Hi,

 While using CONFIG_DMA_API_DEBUG i came across this warning which I
 think is a false positive. As shown dma_sync_single_for_device() are
 called from the dma_map_single() call path. This triggers the warning
 since the dma-debug code have not yet been made aware of the mapping.
>>>
>>> Almost right ;) The thing being mapped (the SPI device's buffer) and the
>>> thing being synced (the IOMMU's PTE) are entirely unrelated. Due to the
>>> current of_iommu_init() setup, the IOMMU is probed long before
>>> dma_debug_init() gets called, therefore DMA debug is missing entries for
>>> some of the initial page table mappings and gets confused when we update
>>> them later.
> 
> What was the eventual followup/fix for this? We're seeing similar traces
> to this during internal testing of 4.11 kernels.

The IOMMU probe-deferral patches queued for the current merge window get
rid of the need for early device creation bodges that cause the problem
(of IOMMU pagetables being mapped for DMA before dma-debug is up and
running). In the meantime, I'd suggest either adjusting the initcall
level per 256ff1cf6b44, or turning off the IOMMU driver and/or using
dma-debug's driver-filter option if you're doing more targeted debugging.

Robin.

> 
> Jon.
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

2017-05-08 Thread Robert Richter 
On 08.05.17 14:47:39, Linu Cherian wrote:
> Have pasted here the relevant changes for doing fixups on smmu base instead
> of offset to get feedback. 

To me this looks better than the ARM_SMMU_EVTQ_*() macros. It still
needs some more shaping (e.g. maybe remove page1_base var and call
arm_smmu_page1_base() directly).

But let's see what others say first.

Thanks,

-Robert

> 
> This actually results in more lines of changes. If you think the below
> approach is still better, will post a V4 of this series with this change.
> 
> 
> +static inline unsigned long arm_smmu_page1_base(
> + struct arm_smmu_device *smmu)
> +{
> + if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
> + return smmu->base;
> + else
> + return smmu->base + SZ_64K;
> +}
> +
> 
> @@ -1948,8 +1962,8 @@ static void arm_smmu_put_resv_regions(struct device 
> *dev,
>  /* Probing and initialisation functions */
>  static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
>  struct arm_smmu_queue *q,
> -unsigned long prod_off,
> -unsigned long cons_off,
> +unsigned long prod_addr,
> +unsigned long cons_addr,
>  size_t dwords)
>  {
>   size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
> @@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct 
> arm_smmu_device *smmu,
>   return -ENOMEM;
>   }
>  
> - q->prod_reg = smmu->base + prod_off;
> - q->cons_reg = smmu->base + cons_off;
> + q->prod_reg = prod_addr;
> + q->cons_reg = cons_addr;
>   q->ent_dwords   = dwords;
>  
>   q->q_base  = Q_BASE_RWA;
> @@ -1977,17 +1991,25 @@ static int arm_smmu_init_one_queue(struct 
> arm_smmu_device *smmu,
>  static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  {
>   int ret;
> + unsigned long page1_base, page0_base;
> +
> + page0_base = smmu->base;
> + page1_base = arm_smmu_page1_base(smmu);
>  
>   /* cmdq */
>   spin_lock_init(>cmdq.lock);
> - ret = arm_smmu_init_one_queue(smmu, >cmdq.q, ARM_SMMU_CMDQ_PROD,
> -   ARM_SMMU_CMDQ_CONS, CMDQ_ENT_DWORDS);
> + ret = arm_smmu_init_one_queue(smmu, >cmdq.q, 
> +   page0_base + ARM_SMMU_CMDQ_PROD,
> +   page0_base + ARM_SMMU_CMDQ_CONS, 
> +   CMDQ_ENT_DWORDS);
>   if (ret)
>   return ret;
>  
>   /* evtq */
> - ret = arm_smmu_init_one_queue(smmu, >evtq.q, ARM_SMMU_EVTQ_PROD,
> -   ARM_SMMU_EVTQ_CONS, EVTQ_ENT_DWORDS);
> + ret = arm_smmu_init_one_queue(smmu, >evtq.q,
> +   page1_base + ARM_SMMU_EVTQ_PROD,
> +   page1_base + ARM_SMMU_EVTQ_CONS,
> +   EVTQ_ENT_DWORDS);
>   if (ret)
>   return ret;
>  
> @@ -1995,8 +2017,10 @@ static int arm_smmu_init_queues(struct arm_smmu_device 
> *smmu)
>   if (!(smmu->features & ARM_SMMU_FEAT_PRI))
>   return 0;
>  
> - return arm_smmu_init_one_queue(smmu, >priq.q, ARM_SMMU_PRIQ_PROD,
> -ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
> + return arm_smmu_init_one_queue(smmu, >priq.q,
> +page1_base + ARM_SMMU_PRIQ_PROD,
> +page1_base + ARM_SMMU_PRIQ_CONS,
> +PRIQ_ENT_DWORDS);
>  }
> 
> 
> 
> @@ -2301,8 +2349,11 @@ static int arm_smmu_device_reset(struct 
> arm_smmu_device *smmu, bool bypass)
>  {
>   int ret;
>   u32 reg, enables;
> + unsigned long page1_base;
>   struct arm_smmu_cmdq_ent cmd;
>  
> + page1_base = arm_smmu_page1_base(smmu);
> +
>   /* Clear CR0 and sync (disables SMMU and queue processing) */
>   reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
>   if (reg & CR0_SMMUEN)
> @@ -2363,8 +2414,8 @@ static int arm_smmu_device_reset(struct arm_smmu_device 
> *smmu, bool bypass)
>  
>   /* Event queue */
>   writeq_relaxed(smmu->evtq.q.q_base, smmu->base + ARM_SMMU_EVTQ_BASE);
> - writel_relaxed(smmu->evtq.q.prod, smmu->base + ARM_SMMU_EVTQ_PROD);
> - writel_relaxed(smmu->evtq.q.cons, smmu->base + ARM_SMMU_EVTQ_CONS);
> + writel_relaxed(smmu->evtq.q.prod, page1_base + ARM_SMMU_EVTQ_PROD);
> + writel_relaxed(smmu->evtq.q.cons, page1_base + ARM_SMMU_EVTQ_CONS);
>  
>   enables |= CR0_EVTQEN;
>   ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
> @@ -2379,9 +2430,9 @@ static int arm_smmu_device_reset(struct arm_smmu_device 
> *smmu, bool bypass)
>   writeq_relaxed(smmu->priq.q.q_base,
>  smmu->base + ARM_SMMU_PRIQ_qBASE);
>   writel_relaxed(smmu->priq.q.prod,
> -

Re: [PATCH v3 1/7] iommu/arm-smmu-v3: Introduce SMMU option PAGE0_REGS_ONLY for ThunderX2 errata #74

2017-05-08 Thread Linu Cherian 
On Sat May 06, 2017 at 01:03:28AM +0200, Robert Richter wrote:
> On 05.05.17 17:38:05, Geetha sowjanya wrote:
> > From: Linu Cherian 
> > 
> > Cavium ThunderX2 SMMU implementation doesn't support page 1 register space
> > and PAGE0_REGS_ONLY option will be enabled as an errata workaround.
> > 
> > This option when turned on, replaces all page 1 offsets used for
> > EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets.
> > 
> > Signed-off-by: Linu Cherian 
> > Signed-off-by: Geetha Sowjanya 
> > ---
> >  .../devicetree/bindings/iommu/arm,smmu-v3.txt  |  6 +++
> >  drivers/iommu/arm-smmu-v3.c| 44 
> > --
> >  2 files changed, 38 insertions(+), 12 deletions(-)
> 
> > @@ -1995,8 +2011,10 @@ static int arm_smmu_init_queues(struct 
> > arm_smmu_device *smmu)
> > if (!(smmu->features & ARM_SMMU_FEAT_PRI))
> > return 0;
> >  
> > -   return arm_smmu_init_one_queue(smmu, >priq.q, ARM_SMMU_PRIQ_PROD,
> > -  ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
> > +   return arm_smmu_init_one_queue(smmu, >priq.q,
> > +  ARM_SMMU_PRIQ_PROD(smmu),
> > +  ARM_SMMU_PRIQ_CONS(smmu),
> > +  PRIQ_ENT_DWORDS);
> 
> I would also suggest Robin's idea from the v1 review here. This works
> if we rework arm_smmu_init_one_queue() to pass addresses instead of
> offsets.
> 
> This would make these widespread offset calculations obsolete.
>

Have pasted here the relevant changes for doing fixups on smmu base instead
of offset to get feedback. 

This actually results in more lines of changes. If you think the below
approach is still better, will post a V4 of this series with this change.


+static inline unsigned long arm_smmu_page1_base(
+   struct arm_smmu_device *smmu)
+{
+   if (ARM_SMMU_PAGE0_REGS_ONLY(smmu))
+   return smmu->base;
+   else
+   return smmu->base + SZ_64K;
+}
+

@@ -1948,8 +1962,8 @@ static void arm_smmu_put_resv_regions(struct device *dev,
 /* Probing and initialisation functions */
 static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,
   struct arm_smmu_queue *q,
-  unsigned long prod_off,
-  unsigned long cons_off,
+  unsigned long prod_addr,
+  unsigned long cons_addr,
   size_t dwords)
 {
size_t qsz = ((1 << q->max_n_shift) * dwords) << 3;
@@ -1961,8 +1975,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device 
*smmu,
return -ENOMEM;
}
 
-   q->prod_reg = smmu->base + prod_off;
-   q->cons_reg = smmu->base + cons_off;
+   q->prod_reg = prod_addr;
+   q->cons_reg = cons_addr;
q->ent_dwords   = dwords;
 
q->q_base  = Q_BASE_RWA;
@@ -1977,17 +1991,25 @@ static int arm_smmu_init_one_queue(struct 
arm_smmu_device *smmu,
 static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
 {
int ret;
+   unsigned long page1_base, page0_base;
+
+   page0_base = smmu->base;
+   page1_base = arm_smmu_page1_base(smmu);
 
/* cmdq */
spin_lock_init(>cmdq.lock);
-   ret = arm_smmu_init_one_queue(smmu, >cmdq.q, ARM_SMMU_CMDQ_PROD,
- ARM_SMMU_CMDQ_CONS, CMDQ_ENT_DWORDS);
+   ret = arm_smmu_init_one_queue(smmu, >cmdq.q, 
+ page0_base + ARM_SMMU_CMDQ_PROD,
+ page0_base + ARM_SMMU_CMDQ_CONS, 
+ CMDQ_ENT_DWORDS);
if (ret)
return ret;
 
/* evtq */
-   ret = arm_smmu_init_one_queue(smmu, >evtq.q, ARM_SMMU_EVTQ_PROD,
- ARM_SMMU_EVTQ_CONS, EVTQ_ENT_DWORDS);
+   ret = arm_smmu_init_one_queue(smmu, >evtq.q,
+ page1_base + ARM_SMMU_EVTQ_PROD,
+ page1_base + ARM_SMMU_EVTQ_CONS,
+ EVTQ_ENT_DWORDS);
if (ret)
return ret;
 
@@ -1995,8 +2017,10 @@ static int arm_smmu_init_queues(struct arm_smmu_device 
*smmu)
if (!(smmu->features & ARM_SMMU_FEAT_PRI))
return 0;
 
-   return arm_smmu_init_one_queue(smmu, >priq.q, ARM_SMMU_PRIQ_PROD,
-  ARM_SMMU_PRIQ_CONS, PRIQ_ENT_DWORDS);
+   return arm_smmu_init_one_queue(smmu, >priq.q,
+  page1_base + ARM_SMMU_PRIQ_PROD,
+  page1_base + ARM_SMMU_PRIQ_CONS,
+  PRIQ_ENT_DWORDS);
 }



@@ -2301,8 +2349,11 @@ static int arm_smmu_device_reset(struct

Re: [PATCH 0/2] Fix incorrect warning from dma-debug

2017-05-08 Thread Geert Uytterhoeven 
Hi Jon,

On Sun, May 7, 2017 at 2:06 AM, Jon Masters  wrote:
> On 05/09/2016 06:00 AM, Robin Murphy wrote:
>> On 09/05/16 10:37, Robin Murphy wrote:
>>> On 08/05/16 11:59, Niklas Söderlund wrote:
 While using CONFIG_DMA_API_DEBUG i came across this warning which I
 think is a false positive. As shown dma_sync_single_for_device() are
 called from the dma_map_single() call path. This triggers the warning
 since the dma-debug code have not yet been made aware of the mapping.
>>>
>>> Almost right ;) The thing being mapped (the SPI device's buffer) and the
>>> thing being synced (the IOMMU's PTE) are entirely unrelated. Due to the
>>> current of_iommu_init() setup, the IOMMU is probed long before
>>> dma_debug_init() gets called, therefore DMA debug is missing entries for
>>> some of the initial page table mappings and gets confused when we update
>>> them later.
>
> What was the eventual followup/fix for this? We're seeing similar traces
> to this during internal testing of 4.11 kernels.

A quick hack is to change arch/arm64/mm/dma-mapping.c:dma_debug_do_init()
from fs_initcall() to arch_initcall().

However, then you loose the debugfs features. A proper fix would involve
splitting dma_debug_init() in two phases: an early one doing the real work,
and a late one registering the debugfs interface.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [Qemu-devel] [RFC PATCH 0/8] Shared Virtual Memory virtualization for VT-d

2017-05-08 Thread Liu, Yi L 
On Mon, May 08, 2017 at 12:09:42PM +0800, Xiao Guangrong wrote:
> 
> Hi Liu Yi,
> 
> I haven't started to read the code yet, however, could you
> detail more please? It emulates a SVM capable iommu device in
> a VM? Or It speeds up device's DMA access in a VM? Or it is a
> new facility introduced for a VM? Could you please add a bit
> more for its usage?

Hi Guangrong,

Nice to hear from you.

This patchset is part of the whole SVM virtualization work. The whole
patchset wants to expose a SVM capable Intel IOMMU to guest. And yes,
it is an emulated iommu.

For the detail introduction for SVM and SVM virtualization, I think
you may get more from the link below.

http://www.spinics.net/lists/kvm/msg148798.html

For the usage, I can give an example with IGD. Latest IGD is SVM capable
device. On bare metal(Intel IOMMU is also SVM capable), application could
request to share its virtual address(an allocated buffer) with IGD device
through the IOCTL cmd provided by IGD driver. e.g. OpenCL application. When
IGD is assigned to a guest, it is expected to support such usage in guest.
With the SVM virtualization patchset, the application in guest would also
be able to share its virtual address with IGD device. Different from bare
metal, it's sharing GVA with IGD. The hardware IOMMU needs to help translate
the GVA to HPA. So hardware IOMMU needs to know the GVA->HPA mapping. This
patchset would make sure the GVA->HPA mapping is built and maintain the TLB.

Be free to let me know if you want more detail.

Thanks,
Yi L

> 
> Thanks!
> 
> On 04/26/2017 06:11 PM, Liu, Yi L wrote:
> >Hi,
> >
> >This patchset introduces SVM virtualization for intel_iommu in
> >IOMMU/VFIO. The total SVM virtualization for intel_iommu touched
> >Qemu/IOMMU/VFIO.
> >
> >Another patchset would change the Qemu. It is "[RFC PATCH 0/20] Qemu:
> >Extend intel_iommu emulator to support Shared Virtual Memory"
> >
> >In this patchset, it adds two new IOMMU APIs and their implementation
> >in intel_iommu driver. In VFIO, it adds two IOCTL cmd attached on
> >container->fd to propagate data from QEMU to kernel space.
> >
> >[Patch Overview]
> >* 1 adds iommu API definition for binding guest PASID table
> >* 2 adds binding PASID table API implementation in VT-d iommu driver
> >* 3 adds iommu API definition to do IOMMU TLB invalidation from guest
> >* 4 adds IOMMU TLB invalidation implementation in VT-d iommu driver
> >* 5 adds VFIO IOCTL for propagating PASID table binding from guest
> >* 6 adds processing of pasid table binding in vfio_iommu_type1
> >* 7 adds VFIO IOCTL for propagating IOMMU TLB invalidation from guest
> >* 8 adds processing of IOMMU TLB invalidation in vfio_iommu_type1
> >
> >Best Wishes,
> >Yi L
> >
> >
> >Jacob Pan (3):
> >   iommu: Introduce bind_pasid_table API function
> >   iommu/vt-d: add bind_pasid_table function
> >   iommu/vt-d: Add iommu do invalidate function
> >
> >Liu, Yi L (5):
> >   iommu: Introduce iommu do invalidate API function
> >   VFIO: Add new IOTCL for PASID Table bind propagation
> >   VFIO: do pasid table binding
> >   VFIO: Add new IOCTL for IOMMU TLB invalidate propagation
> >   VFIO: do IOMMU TLB invalidation from guest
> >
> >  drivers/iommu/intel-iommu.c | 146 
> > 
> >  drivers/iommu/iommu.c   |  32 +
> >  drivers/vfio/vfio_iommu_type1.c |  98 +++
> >  include/linux/dma_remapping.h   |   1 +
> >  include/linux/intel-iommu.h |  11 +++
> >  include/linux/iommu.h   |  47 +
> >  include/uapi/linux/vfio.h   |  26 +++
> >  7 files changed, 361 insertions(+)
> >
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu