date:20200312

Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()

2020-03-12 Thread Jean-Philippe Brucker

Hi Baolu,

On Thu, Mar 12, 2020 at 09:44:16AM +0800, Lu Baolu wrote:
> Hi Jean,
> 
> On 2020/3/11 20:45, Jean-Philippe Brucker wrote:
> > The pci_ats_supported() function checks if a device supports ATS and is
> > allowed to use it.
> > 
> > Signed-off-by: Jean-Philippe Brucker 
> > ---
> >   drivers/iommu/intel-iommu.c | 9 +++--
> >   1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 6fa6de2b6ad5..17208280ef5c 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct 
> > device_domain_info *info)
> > !pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
> > info->pri_enabled = 1;
> >   #endif
> > -   if (!pdev->untrusted && info->ats_supported &&
> > -   pci_ats_page_aligned(pdev) &&
> > +   if (info->ats_supported && pci_ats_page_aligned(pdev) &&
> > !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
> > info->ats_enabled = 1;
> > domain_update_iotlb(info->domain);
> > @@ -2611,10 +2610,8 @@ static struct dmar_domain 
> > *dmar_insert_one_dev_info(struct intel_iommu *iommu,
> > if (dev && dev_is_pci(dev)) {
> > struct pci_dev *pdev = to_pci_dev(info->dev);
> > -   if (!pdev->untrusted &&
> > -   !pci_ats_disabled() &&
> 
> The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even
> pci_ats_supported() returns true, user still can disable it. Or move
> ats_disabled into pci_ats_supported()?

It is already there, but hidden behind the "if (!dev->ats_cap)":
pci_ats_init() only sets dev->ats_cap after checking that
pci_ats_disabled() returns false.

Thanks,
Jean

> 
> Best regards,
> baolu
> 
> > -   ecap_dev_iotlb_support(iommu->ecap) &&
> > -   pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ATS) &&
> > +   if (ecap_dev_iotlb_support(iommu->ecap) &&
> > +   pci_ats_supported(pdev) &&
> > dmar_find_matched_atsr_unit(pdev))
> > info->ats_supported = 1;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()

2020-03-12 Thread Lu Baolu


On 2020/3/12 15:54, Jean-Philippe Brucker wrote:

Hi Baolu,

On Thu, Mar 12, 2020 at 09:44:16AM +0800, Lu Baolu wrote:

Hi Jean,

On 2020/3/11 20:45, Jean-Philippe Brucker wrote:

The pci_ats_supported() function checks if a device supports ATS and is
allowed to use it.

Signed-off-by: Jean-Philippe Brucker 
---
   drivers/iommu/intel-iommu.c | 9 +++--
   1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6fa6de2b6ad5..17208280ef5c 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1454,8 +1454,7 @@ static void iommu_enable_dev_iotlb(struct 
device_domain_info *info)
!pci_reset_pri(pdev) && !pci_enable_pri(pdev, 32))
info->pri_enabled = 1;
   #endif
-   if (!pdev->untrusted && info->ats_supported &&
-   pci_ats_page_aligned(pdev) &&
+   if (info->ats_supported && pci_ats_page_aligned(pdev) &&
!pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
info->ats_enabled = 1;
domain_update_iotlb(info->domain);
@@ -2611,10 +2610,8 @@ static struct dmar_domain 
*dmar_insert_one_dev_info(struct intel_iommu *iommu,
if (dev && dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(info->dev);
-   if (!pdev->untrusted &&
-   !pci_ats_disabled() &&


The pci_ats_disabled() couldn't be replaced by pci_ats_supported(). Even
pci_ats_supported() returns true, user still can disable it. Or move
ats_disabled into pci_ats_supported()?


It is already there, but hidden behind the "if (!dev->ats_cap)":
pci_ats_init() only sets dev->ats_cap after checking that
pci_ats_disabled() returns false.



Ah! Yes.

Acked-by: Lu Baolu 


Thanks,
Jean


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

[PATCH] iommu/amd: Fix IOMMU AVIC not properly update the is_run bit in IRTE

2020-03-12 Thread Suravee Suthikulpanit

Commit b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC
(de-)activation code") accidentally left out the ir_data pointer when
calling modity_irte_ga(), which causes the function amd_iommu_update_ga()
to return prematurely due to struct amd_ir_data.ref is NULL and
the "is_run" bit of IRTE does not get updated properly.

This results in bad I/O performance since IOMMU AVIC always generate GA Log
entry and notify IOMMU driver and KVM when it receives interrupt from the
PCI pass-through device instead of directly inject interrupt to the vCPU.

Fixes by passing ir_data when calling modify_irte_ga() as done previously.

Fixes: b9c6ff94e43a ("iommu/amd: Re-factor guest virtual APIC (de-)activation 
code")
Signed-off-by: Suravee Suthikulpanit 
---
 drivers/iommu/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index aac132b..20cce36 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3826,7 +3826,7 @@ int amd_iommu_activate_guest_mode(void *data)
entry->lo.fields_vapic.ga_tag  = ir_data->ga_tag;
 
return modify_irte_ga(ir_data->irq_2_irte.devid,
- ir_data->irq_2_irte.index, entry, NULL);
+ ir_data->irq_2_irte.index, entry, ir_data);
 }
 EXPORT_SYMBOL(amd_iommu_activate_guest_mode);
 
@@ -3852,7 +3852,7 @@ int amd_iommu_deactivate_guest_mode(void *data)
APICID_TO_IRTE_DEST_HI(cfg->dest_apicid);
 
return modify_irte_ga(ir_data->irq_2_irte.devid,
- ir_data->irq_2_irte.index, entry, NULL);
+ ir_data->irq_2_irte.index, entry, ir_data);
 }
 EXPORT_SYMBOL(amd_iommu_deactivate_guest_mode);
 
-- 
1.8.3.1

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

[PATCH] iommu/vt-d: Unlock on error paths

2020-03-12 Thread Dan Carpenter

There were a couple places where we need to unlock before returning.

Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected")
Signed-off-by: Dan Carpenter 
---
 drivers/iommu/intel-iommu-debugfs.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel-iommu-debugfs.c 
b/drivers/iommu/intel-iommu-debugfs.c
index 8d24c4d85cc2..6a495b103972 100644
--- a/drivers/iommu/intel-iommu-debugfs.c
+++ b/drivers/iommu/intel-iommu-debugfs.c
@@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct seq_file 
*m, void *unused)
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_TES)) {
seq_puts(m, "DMA Remapping is not enabled\n");
-   return 0;
+   goto unlock;
}
root_tbl_walk(m, iommu);
seq_putc(m, '\n');
}
+unlock:
rcu_read_unlock();
 
return 0;
@@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file *m, 
void *unused)
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_IRES)) {
seq_puts(m, "Interrupt Remapping is not enabled\n");
-   return 0;
+   goto unlock;
}
 
if (iommu->ir_table) {
@@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file *m, 
void *unused)
}
seq_putc(m, '\n');
}
+unlock:
rcu_read_unlock();
 
return 0;
-- 
2.20.1

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

Re: [PATCH] iommu/vt-d: Unlock on error paths

2020-03-12 Thread Lu Baolu


On 2020/3/12 19:37, Dan Carpenter wrote:

There were a couple places where we need to unlock before returning.

Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected")
Signed-off-by: Dan Carpenter 
---
  drivers/iommu/intel-iommu-debugfs.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel-iommu-debugfs.c 
b/drivers/iommu/intel-iommu-debugfs.c
index 8d24c4d85cc2..6a495b103972 100644
--- a/drivers/iommu/intel-iommu-debugfs.c
+++ b/drivers/iommu/intel-iommu-debugfs.c
@@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct seq_file 
*m, void *unused)
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_TES)) {
seq_puts(m, "DMA Remapping is not enabled\n");
-   return 0;
+   goto unlock;
}
root_tbl_walk(m, iommu);
seq_putc(m, '\n');
}
+unlock:
rcu_read_unlock();
  
  	return 0;

@@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file *m, 
void *unused)
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_IRES)) {
seq_puts(m, "Interrupt Remapping is not enabled\n");
-   return 0;
+   goto unlock;
}
  
  		if (iommu->ir_table) {

@@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file *m, 
void *unused)
}
seq_putc(m, '\n');
}
+unlock:
rcu_read_unlock();
  
  	return 0;




Thanks a lot for the catch. I think it could be further cleanup. How
about below changes?

index 8d24c4d85cc2..2583a6743dd0 100644
--- a/drivers/iommu/intel-iommu-debugfs.c
+++ b/drivers/iommu/intel-iommu-debugfs.c
@@ -288,8 +288,9 @@ static int dmar_translation_struct_show(struct 
seq_file *m, void *unused)

for_each_active_iommu(iommu, drhd) {
sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
if (!(sts & DMA_GSTS_TES)) {
-   seq_puts(m, "DMA Remapping is not enabled\n");
-   return 0;
+   seq_printf(m, "DMA Remapping is not enabled on 
%s\n",

+  iommu->name);
+   continue;
}
root_tbl_walk(m, iommu);
seq_putc(m, '\n');
@@ -431,7 +432,6 @@ static int ir_translation_struct_show(struct 
seq_file *m, void *unused)

struct dmar_drhd_unit *drhd;
struct intel_iommu *iommu;
u64 irta;
-   u32 sts;

rcu_read_lock();
for_each_active_iommu(iommu, drhd) {
@@ -441,12 +441,6 @@ static int ir_translation_struct_show(struct 
seq_file *m, void *unused)
seq_printf(m, "Remapped Interrupt supported on IOMMU: 
%s\n",

   iommu->name);

-   sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
-   if (!(sts & DMA_GSTS_IRES)) {
-   seq_puts(m, "Interrupt Remapping is not enabled\n");
-   return 0;
-   }
-
if (iommu->ir_table) {
irta = virt_to_phys(iommu->ir_table->base);
seq_printf(m, " IR table address:%llx\n", irta);

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

Re: [PATCH 0/3] Request direct mapping for modem firmware subdevice

2020-03-12 Thread Robin Murphy


On 2020-03-12 6:28 am, Sai Prakash Ranjan wrote:

Hi Robin,

On 2020-03-10 22:14, Robin Murphy wrote:

On 10/03/2020 4:23 pm, Joerg Roedel wrote:

On Tue, Mar 10, 2020 at 07:30:50PM +0530, Sibi Sankar wrote:

The accesses are initiated by the firmware
and they access modem reserved regions.
However as explained in ^^ any accesses
outside the region will result in a violation
and is controlled through XPUs (protection units).


Okay, this sounds like a case for arm_smmu_get_resv_region(). It should
return an entry for the reserved memory region the firmware needs to
access, so that generic iommu can setup this mapping.

Note that it should return that entry only for your device, not for all
devices. Maybe there is a property in DT or IORT you can set to
transport this information into the arm-smmu driver.

This is pretty similar to RMRR mapping on the Intel VT-d IOMMU or
Unity-mapped ranges in the AMD-Vi IOMMU.


Yup, a way to describe boot-time memory regions in IORT is in the
process of being specced out; the first attempt at an equivalent for
DT is here:

https://lore.kernel.org/linux-iommu/20191209150748.2471814-1-thierry.red...@gmail.com/ 



If that's not enough and the SMMU still needs to treat certain Stream
IDs specially because they may be untranslatable (due to having direct
access to memory as a side-channel), then that should be handled in
the SoC-specific corner of the SMMU driver, not delegated to
individual endpoint drivers.



Are you talking about this one for SoC specific change - 
https://lore.kernel.org/patchwork/patch/1183530/


Exactly - this particular wheel needs no reinventing at all.

[ I guess I should go review those patches properly... :) ]

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

Re: [PATCH] iommu/vt-d: Unlock on error paths

2020-03-12 Thread Dan Carpenter

On Thu, Mar 12, 2020 at 08:02:41PM +0800, Lu Baolu wrote:
> On 2020/3/12 19:37, Dan Carpenter wrote:
> > There were a couple places where we need to unlock before returning.
> > 
> > Fixes: 91391b919e19 ("iommu/vt-d: Populate debugfs if IOMMUs are detected")
> > Signed-off-by: Dan Carpenter 
> > ---
> >   drivers/iommu/intel-iommu-debugfs.c | 6 --
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel-iommu-debugfs.c 
> > b/drivers/iommu/intel-iommu-debugfs.c
> > index 8d24c4d85cc2..6a495b103972 100644
> > --- a/drivers/iommu/intel-iommu-debugfs.c
> > +++ b/drivers/iommu/intel-iommu-debugfs.c
> > @@ -289,11 +289,12 @@ static int dmar_translation_struct_show(struct 
> > seq_file *m, void *unused)
> > sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
> > if (!(sts & DMA_GSTS_TES)) {
> > seq_puts(m, "DMA Remapping is not enabled\n");
> > -   return 0;
> > +   goto unlock;
> > }
> > root_tbl_walk(m, iommu);
> > seq_putc(m, '\n');
> > }
> > +unlock:
> > rcu_read_unlock();
> > return 0;
> > @@ -444,7 +445,7 @@ static int ir_translation_struct_show(struct seq_file 
> > *m, void *unused)
> > sts = dmar_readl(iommu->reg + DMAR_GSTS_REG);
> > if (!(sts & DMA_GSTS_IRES)) {
> > seq_puts(m, "Interrupt Remapping is not enabled\n");
> > -   return 0;
> > +   goto unlock;
> > }
> > if (iommu->ir_table) {
> > @@ -475,6 +476,7 @@ static int ir_translation_struct_show(struct seq_file 
> > *m, void *unused)
> > }
> > seq_putc(m, '\n');
> > }
> > +unlock:
> > rcu_read_unlock();
> > return 0;
> > 
> 
> Thanks a lot for the catch. I think it could be further cleanup. How
> about below changes?

Obviously that solves the issues with forgetting to drop the lock but
I'm not qualified to comment on the rest.  (And I can't really review
it anyway because the patch was damaged in sending the email).

regards,
dan carepnter

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

[PATCH v3] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

2020-03-12 Thread Kevin Grandemange

pageno is an int and the PAGE_SHIFT shift is done on an int,
overflowing if the memory is bigger than 2G

This can be reproduced using for example a reserved-memory of 4G

reserved-memory {
#address-cells = <2>;
#size-cells = <2>;
ranges;

reserved_dma: buffer@0 {
compatible = "shared-dma-pool";
no-map;
reg = <0x5 0x 0x1 0x0>;
};
};

Signed-off-by: Kevin Grandemange 
---

Changes v1 -> v2:
  - removed mem_offset tmp variable
  - use dma_addr_t instead of ssize_t
  - Fix reserved-memory size in the dts example

Changes v2 -> v3:
  - Fix several other site where PAGE_SHIFT shifts are done on ints.

 kernel/dma/coherent.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
index 551b0eb7028a..d322cb786e7e 100644
--- a/kernel/dma/coherent.c
+++ b/kernel/dma/coherent.c
@@ -134,7 +134,7 @@ static void *__dma_alloc_from_coherent(struct device *dev,
 
spin_lock_irqsave(&mem->spinlock, flags);
 
-   if (unlikely(size > (mem->size << PAGE_SHIFT)))
+   if (unlikely(size > ((dma_addr_t)mem->size << PAGE_SHIFT)))
goto err;
 
pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
@@ -144,8 +144,8 @@ static void *__dma_alloc_from_coherent(struct device *dev,
/*
 * Memory was found in the coherent area.
 */
-   *dma_handle = dma_get_device_base(dev, mem) + (pageno << PAGE_SHIFT);
-   ret = mem->virt_base + (pageno << PAGE_SHIFT);
+   *dma_handle = dma_get_device_base(dev, mem) + ((dma_addr_t)pageno << 
PAGE_SHIFT);
+   ret = mem->virt_base + ((dma_addr_t)pageno << PAGE_SHIFT);
spin_unlock_irqrestore(&mem->spinlock, flags);
memset(ret, 0, size);
return ret;
@@ -194,7 +194,7 @@ static int __dma_release_from_coherent(struct 
dma_coherent_mem *mem,
   int order, void *vaddr)
 {
if (mem && vaddr >= mem->virt_base && vaddr <
-  (mem->virt_base + (mem->size << PAGE_SHIFT))) {
+  (mem->virt_base + ((dma_addr_t)mem->size << PAGE_SHIFT))) {
int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
unsigned long flags;
 
@@ -238,7 +238,7 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem 
*mem,
struct vm_area_struct *vma, void *vaddr, size_t size, int *ret)
 {
if (mem && vaddr >= mem->virt_base && vaddr + size <=
-  (mem->virt_base + (mem->size << PAGE_SHIFT))) {
+  (mem->virt_base + ((dma_addr_t)mem->size << PAGE_SHIFT))) {
unsigned long off = vma->vm_pgoff;
int start = (vaddr - mem->virt_base) >> PAGE_SHIFT;
int user_count = vma_pages(vma);
@@ -248,7 +248,7 @@ static int __dma_mmap_from_coherent(struct dma_coherent_mem 
*mem,
if (off < count && user_count <= count - off) {
unsigned long pfn = mem->pfn_base + start + off;
*ret = remap_pfn_range(vma, vma->vm_start, pfn,
-  user_count << PAGE_SHIFT,
+  (unsigned long)user_count << 
PAGE_SHIFT,
   vma->vm_page_prot);
}
return 1;
-- 
2.20.1

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

[RFC PATCH] vfio: Ignore -ENODEV when getting MSI cookie

2020-03-12 Thread Andre Przywara

When we try to get an MSI cookie for a VFIO device, that can fail if
CONFIG_IOMMU_DMA is not set. In this case iommu_get_msi_cookie() returns
-ENODEV, and that should not be fatal.

Ignore that case and proceed with the initialisation.

This fixes VFIO with a platform device on the Calxeda Midway (no MSIs).

Signed-off-by: Andre Przywara 
---
Hi,

not sure this is the right fix, or we should rather check if the
platform doesn't support MSIs at all (which doesn't seem to be easy
to do).
Or is this because arm-smmu.c always reserves an IOMMU_RESV_SW_MSI
region?

Also this seems to be long broken, actually since Eric introduced MSI
support in 4.10-rc3, but at least since the initialisation order was
fixed with f6810c15cf9.

Grateful for any insight.

Cheers,
Andre

 drivers/vfio/vfio_iommu_type1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a177bf2c6683..467e217ef09a 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1786,7 +1786,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 
if (resv_msi) {
ret = iommu_get_msi_cookie(domain->domain, resv_msi_base);
-   if (ret)
+   if (ret && ret != -ENODEV)
goto out_detach;
}
 
-- 
2.17.1

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

Re: [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS

2020-03-12 Thread Bjorn Helgaas

On Wed, Mar 11, 2020 at 01:44:58PM +0100, Jean-Philippe Brucker wrote:
> When setting up a generic host on a device-tree based system, copy the
> ats-supported flag into the pci_host_bridge structure.
> 
> Signed-off-by: Jean-Philippe Brucker 
> ---
> v1->v2: keep the helper in pci-host-common.c
> ---
>  drivers/pci/controller/pci-host-common.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pci/controller/pci-host-common.c 
> b/drivers/pci/controller/pci-host-common.c
> index 250a3fc80ec6..2e800bc6ae7a 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -54,6 +54,16 @@ static struct pci_config_window *gen_pci_init(struct 
> device *dev,
>   return ERR_PTR(err);
>  }
>  
> +static void of_pci_host_check_ats(struct pci_host_bridge *bridge)
> +{
> + struct device_node *np = bridge->bus->dev.of_node;
> +
> + if (!np)
> + return;
> +
> + bridge->ats_supported = of_property_read_bool(np, "ats-supported");
> +}
> +
>  int pci_host_common_probe(struct platform_device *pdev,
> struct pci_ecam_ops *ops)
>  {
> @@ -92,6 +102,7 @@ int pci_host_common_probe(struct platform_device *pdev,
>   return ret;
>   }
>  
> + of_pci_host_check_ats(bridge);

I would prefer to write this as a predicate instead of having the
assignment be a side-effect, e.g.,

  bridge->ats_supported = of_pci_host_ats_supported(bridge);

If that works for you,

Acked-by: Bjorn Helgaas 

>   platform_set_drvdata(pdev, bridge->bus);
>   return 0;
>  }
> -- 
> 2.25.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node

2020-03-12 Thread Bjorn Helgaas

On Wed, Mar 11, 2020 at 01:44:59PM +0100, Jean-Philippe Brucker wrote:
> When initializing a PCI root bridge, copy its "ATS supported" attribute
> into the root bridge.
> 
> Acked-by: Hanjun Guo 
> Signed-off-by: Jean-Philippe Brucker 
> ---
>  drivers/acpi/arm64/iort.c | 27 +++
>  drivers/acpi/pci_root.c   |  3 +++
>  include/linux/acpi_iort.h |  8 
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index ed3d2d1a7ae9..d99d7f5b51e1 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1633,6 +1633,33 @@ static void __init iort_enable_acs(struct 
> acpi_iort_node *iort_node)
>   }
>   }
>  }
> +
> +static acpi_status iort_match_host_bridge_callback(struct acpi_iort_node 
> *node,
> +void *context)
> +{
> + struct acpi_iort_root_complex *pci_rc;
> + struct pci_host_bridge *host_bridge = context;
> +
> + pci_rc = (struct acpi_iort_root_complex *)node->node_data;
> +
> + return pci_domain_nr(host_bridge->bus) == pci_rc->pci_segment_number ?
> + AE_OK : AE_NOT_FOUND;
> +}
> +
> +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge)
> +{
> + struct acpi_iort_node *node;
> + struct acpi_iort_root_complex *pci_rc;
> +
> + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> +   iort_match_host_bridge_callback, host_bridge);
> + if (!node)
> + return;
> +
> + pci_rc = (struct acpi_iort_root_complex *)node->node_data;
> + host_bridge->ats_supported = !!(pci_rc->ats_attribute &
> + ACPI_IORT_ATS_SUPPORTED);
> +}
>  #else
>  static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
>  #endif
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d1e666ef3fcc..eb2fb8f17c0b 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -6,6 +6,7 @@
>   *  Copyright (C) 2001, 2002 Paul Diefenbaugh 
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -917,6 +918,8 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root 
> *root,
>   if (!(root->osc_control_set & OSC_PCI_EXPRESS_LTR_CONTROL))
>   host_bridge->native_ltr = 0;
>  
> + iort_pci_host_bridge_setup(host_bridge);

Similar comment as on the OF side.

You mentioned at [1] that "it's important that we only enable ATS if
the host bridge supports it".  That should be captured in a commit log
and comment somewhere here.

That suggests to me that we should not set

  bridge->ats_supported = 1;

by default in pci_init_host_bridge(), but rather leave it zero as it
is by default, and then do things like:

  if (iort_pci_host_bridge_ats_supported(bridge))
bridge->ats_supported = 1;

  if (of_pci_host_bridge_ats_supported(bridge))
bridge->ats_supported = 1;

I don't know what you do about IVRS and DMAR, which don't appear in
this series except in the comment.

[1] https://lore.kernel.org/r/20200213165049.508908-1-jean-phili...@linaro.org

>   /*
>* Evaluate the "PCI Boot Configuration" _DSM Function.  If it
>* exists and returns 0, we must preserve any PCI resource
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index 8e7e2ec37f1b..7b06871cc3aa 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define IORT_IRQ_MASK(irq)   (irq & 0xULL)
>  #define IORT_IRQ_TRIGGER_MASK(irq)   ((irq >> 32) & 0xULL)
> @@ -55,4 +56,11 @@ int iort_iommu_msi_get_resv_regions(struct device *dev, 
> struct list_head *head)
>  { return 0; }
>  #endif
>  
> +#if defined(CONFIG_ACPI_IORT) && defined(CONFIG_PCI)
> +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge);
> +#else
> +static inline
> +void iort_pci_host_bridge_setup(struct pci_host_bridge *host_bridge) { }
> +#endif
> +
>  #endif /* __ACPI_IORT_H__ */
> -- 
> 2.25.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported()

2020-03-12 Thread Bjorn Helgaas

On Wed, Mar 11, 2020 at 01:45:00PM +0100, Jean-Philippe Brucker wrote:
> IOMMU drivers need to perform several tests when checking if a device
> supports ATS.  Move them all into a new function that returns true when
> a device and its host bridge support ATS.
> 
> Since pci_enable_ats() now calls pci_ats_supported(), the following
> new checks are now common:
> * whether a device is trusted.  Devices plugged into external-facing
>   ports such as thunderbolt are untrusted.
> * whether the host bridge supports ATS, which defaults to true unless
>   the firmware description states that ATS isn't supported by the host
>   bridge.
> 
> Signed-off-by: Jean-Philippe Brucker 

Acked-by: Bjorn Helgaas 

> ---
>  drivers/pci/ats.c   | 30 +-
>  include/linux/pci-ats.h |  3 +++
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 390e92f2d8d1..bbfd0d42b8b9 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -30,6 +30,34 @@ void pci_ats_init(struct pci_dev *dev)
>   dev->ats_cap = pos;
>  }
>  
> +/**
> + * pci_ats_supported - check if the device can use ATS
> + * @dev: the PCI device
> + *
> + * Returns true if the device supports ATS and is allowed to use it, false
> + * otherwise.
> + */
> +bool pci_ats_supported(struct pci_dev *dev)
> +{
> + struct pci_host_bridge *bridge;
> +
> + if (!dev->ats_cap)
> + return false;
> +
> + if (dev->untrusted)
> + return false;
> +
> + bridge = pci_find_host_bridge(dev->bus);
> + if (!bridge)
> + return false;
> +
> + if (!bridge->ats_supported)
> + return false;
> +
> + return true;

I assume this is the same as

  return bridge->ats_supported;

Only "assuming" because I'm not a C language lawyer, but I assume it
does the obvious conversion from unsigned:1 to bool.

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

Re: [PATCH v2 02/11] PCI: Add ats_supported host bridge flag

2020-03-12 Thread Bjorn Helgaas

On Wed, Mar 11, 2020 at 01:44:57PM +0100, Jean-Philippe Brucker wrote:
> Each vendor has their own way of describing whether a host bridge
> supports ATS.  The Intel and AMD ACPI tables selectively enable or
> disable ATS per device or sub-tree, while Arm has a single bit for each
> host bridge.  For those that need it, add an ats_supported bit to the
> host bridge structure.

Can you mention the specific ACPI tables here in the commit log?

Maybe elaborate on the "for those that need it" bit?  I'm not sure if
you need it for the cases where DT or ACPI tells us directly for the
host bridge, or if you need it for the more selective cases?

I guess in one sense you *always* need it since you check the cached
bit later.

I don't understand the implications of this, especially the selective
situation.  Given your comment from the first posting, I thought this
was a property of the host bridge, so I don't know what it means to
say some devices support ATS but others don't.

> Signed-off-by: Jean-Philippe Brucker 
> ---
> v1->v2: try to improve the comment
> ---
>  drivers/pci/probe.c | 8 
>  include/linux/pci.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 512cb4312ddd..b5e36f06b40a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -598,6 +598,14 @@ static void pci_init_host_bridge(struct pci_host_bridge 
> *bridge)
>   bridge->native_shpc_hotplug = 1;
>   bridge->native_pme = 1;
>   bridge->native_ltr = 1;
> +
> + /*
> +  * Some systems (ACPI IORT, device-tree) declare ATS support at the host
> +  * bridge, and clear this bit when ATS isn't supported. Others (ACPI
> +  * DMAR and IVRS) declare ATS support with a smaller granularity, and
> +  * need this bit set.
> +  */
> + bridge->ats_supported = 1;
>  }
>  
>  struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 3840a541a9de..9fe2e84d74d7 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -511,6 +511,7 @@ struct pci_host_bridge {
>   unsigned intnative_pme:1;   /* OS may use PCIe PME */
>   unsigned intnative_ltr:1;   /* OS may use PCIe LTR */
>   unsigned intpreserve_config:1;  /* Preserve FW resource setup */
> + unsigned intats_supported:1;
>  
>   /* Resource alignment requirements */
>   resource_size_t (*align_resource)(struct pci_dev *dev,
> -- 
> 2.25.1
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()

Re: [PATCH v2 08/11] iommu/vt-d: Use pci_ats_supported()

[PATCH] iommu/amd: Fix IOMMU AVIC not properly update the is_run bit in IRTE

[PATCH] iommu/vt-d: Unlock on error paths

Re: [PATCH] iommu/vt-d: Unlock on error paths

Re: [PATCH 0/3] Request direct mapping for modem firmware subdevice

Re: [PATCH] iommu/vt-d: Unlock on error paths

[PATCH v3] [dma-coherent] Fix integer overflow in the reserved-memory dma allocation

[RFC PATCH] vfio: Ignore -ENODEV when getting MSI cookie

Re: [PATCH v2 03/11] PCI: OF: Check whether the host bridge supports ATS

Re: [PATCH v2 04/11] ACPI/IORT: Check ATS capability in root complex node

Re: [PATCH v2 05/11] PCI/ATS: Gather checks into pci_ats_supported()

Re: [PATCH v2 02/11] PCI: Add ats_supported host bridge flag

13 matches

Site Navigation

Mail list logo

Footer information