Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function

[Jacob Pan]

On Fri, 20 Apr 2018 19:25:34 +0100
Jean-Philippe Brucker  wrote:

> On Tue, Apr 17, 2018 at 08:10:47PM +0100, Alex Williamson wrote:
> [...]
> > > + /* Assign guest PASID table pointer and size order */
> > > + ctx_lo = (pasidt_binfo->base_ptr & VTD_PAGE_MASK) |
> > > + (pasidt_binfo->pasid_bits - MIN_NR_PASID_BITS);  
> > 
> > Where does this IOMMU API interface define that base_ptr is 4K
> > aligned or the format of the PASID table?  Are these all
> > standardized or do they vary by host IOMMU?  If they're standards,
> > maybe we could note that and the spec which defines them when we
> > declare base_ptr.  If they're IOMMU specific then I don't
> > understand how we'll match a user provided PASID table to the
> > requirements and format of the host IOMMU. Thanks,  
> 
> On SMMUv3 the minimum alignment for base_ptr is 64 bytes, so a guest
> under a vSMMU might pass a pointer that's not aligned on 4k.
> 
PASID table pointer for VT-d is 4K aligned. 
> Maybe this information could be part of the data passed to userspace
> about IOMMU table formats and features? They're not part of this
> series, but I think we wanted to communicate IOMMU-specific features
> via sysfs.
> 
Agreed, I believe Yi Liu is working on a sysfs interface such that QEMU
can match IOMMU model and features.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function

[Jacob Pan]

On Tue, 17 Apr 2018 13:10:47 -0600
Alex Williamson  wrote:

> On Mon, 16 Apr 2018 14:48:53 -0700
> Jacob Pan  wrote:
> 
> > Add Intel VT-d ops to the generic iommu_bind_pasid_table API
> > functions.
> > 
> > The primary use case is for direct assignment of SVM capable
> > device. Originated from emulated IOMMU in the guest, the request
> > goes through many layers (e.g. VFIO). Upon calling host IOMMU
> > driver, caller passes guest PASID table pointer (GPA) and size.
> > 
> > Device context table entry is modified by Intel IOMMU specific
> > bind_pasid_table function. This will turn on nesting mode and
> > matching translation type.
> > 
> > The unbind operation restores default context mapping.
> > 
> > Signed-off-by: Jacob Pan 
> > Signed-off-by: Liu, Yi L 
> > Signed-off-by: Ashok Raj 
> > ---
> >  drivers/iommu/intel-iommu.c   | 119
> > ++
> > include/linux/dma_remapping.h |   1 + 2 files changed, 120
> > insertions(+)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index a0f81a4..d8058be 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -2409,6 +2409,7 @@ static struct dmar_domain
> > *dmar_insert_one_dev_info(struct intel_iommu *iommu,
> > info->ats_supported = info->pasid_supported = info->pri_supported =
> > 0; info->ats_enabled = info->pasid_enabled = info->pri_enabled = 0;
> > info->ats_qdep = 0;
> > +   info->pasid_table_bound = 0;
> > info->dev = dev;
> > info->domain = domain;
> > info->iommu = iommu;
> > @@ -5132,6 +5133,7 @@ static void
> > intel_iommu_put_resv_regions(struct device *dev, 
> >  #ifdef CONFIG_INTEL_IOMMU_SVM
> >  #define MAX_NR_PASID_BITS (20)
> > +#define MIN_NR_PASID_BITS (5)
> >  static inline unsigned long intel_iommu_get_pts(struct intel_iommu
> > *iommu) {
> > /*
> > @@ -5258,6 +5260,119 @@ struct intel_iommu
> > *intel_svm_device_to_iommu(struct device *dev) 
> > return iommu;
> >  }
> > +
> > +static int intel_iommu_bind_pasid_table(struct iommu_domain
> > *domain,
> > +   struct device *dev, struct pasid_table_config
> > *pasidt_binfo) +{  
> 
> Never validates pasidt_binfo->{version,bytes}
> 
good catch, will do.
> > +   struct intel_iommu *iommu;
> > +   struct context_entry *context;
> > +   struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +   struct device_domain_info *info;
> > +   struct pci_dev *pdev;
> > +   u8 bus, devfn, host_table_pasid_bits;
> > +   u16 did, sid;
> > +   int ret = 0;
> > +   unsigned long flags;
> > +   u64 ctx_lo;
> > +
> > +   iommu = device_to_iommu(dev, , );
> > +   if (!iommu)
> > +   return -ENODEV;
> > +   /* VT-d spec section 9.4 says pasid table size is encoded
> > as 2^(x+5) */
> > +   host_table_pasid_bits = intel_iommu_get_pts(iommu) +
> > MIN_NR_PASID_BITS;
> > +   if (!pasidt_binfo || pasidt_binfo->pasid_bits >
> > host_table_pasid_bits ||
> > +   pasidt_binfo->pasid_bits < MIN_NR_PASID_BITS) {
> > +   pr_err("Invalid gPASID bits %d, host range %d -
> > %d\n",
> > +   pasidt_binfo->pasid_bits,
> > +   MIN_NR_PASID_BITS, host_table_pasid_bits);
> > +   return -ERANGE;
> > +   }
> > +   if (!ecap_nest(iommu->ecap)) {
> > +   dev_err(dev, "Cannot bind PASID table, no nested
> > translation\n");
> > +   ret = -EINVAL;
> > +   goto out;
> > +   }  
> 
> Gratuitous use of pr_err, some of these look user reachable, for
> instance can vfio know in advance the supported widths or can the user
> trigger that pr_err at will?
Yes, the current IOMMU sysfs for vt-d does show the content of
capability registers so user could know in advance whether the nested
mode is supported. But I think we are in need of some generic interface
to enumerate IOMMU features. Here I am trying to prepare for the worst.
Are you concerned about security if user can trigger that error at
will? Sorry I didn't get the point.

>  Some of these errno values are also
> maybe not as descriptive as they could be.  For instance if the iommu
> doesn't support nesting, that's not a calling argument error, that's
> an unsupported device error, right?
> 
your are right, that is not invalid argument. You mean use ENODEV?

> > +   pdev = to_pci_dev(dev);
> > +   sid = PCI_DEVID(bus, devfn);
> > +   info = dev->archdata.iommu;
> > +
> > +   if (!info) {
> > +   dev_err(dev, "Invalid device domain info\n");
> > +   ret = -EINVAL;
> > +   goto out;
> > +   }
> > +   if (info->pasid_table_bound) {
> > +   dev_err(dev, "Device PASID table already bound\n");
> > +   ret = -EBUSY;
> > +   goto out;
> > +   }
> > +   if (!info->pasid_enabled) {
> > +   ret = pci_enable_pasid(pdev, info->pasid_supported
> > & ~1);
> > +   if (ret) {
> > +   

Re: [PATCH v4 09/22] iommu/vt-d: add svm/sva invalidate function

[Jacob Pan]

On Tue, 17 Apr 2018 13:10:45 -0600
Alex Williamson  wrote:

> On Mon, 16 Apr 2018 14:48:58 -0700
> Jacob Pan  wrote:
> 
> > When Shared Virtual Address (SVA) is enabled for a guest OS via
> > vIOMMU, we need to provide invalidation support at IOMMU API and
> > driver level. This patch adds Intel VT-d specific function to
> > implement iommu passdown invalidate API for shared virtual address.
> > 
> > The use case is for supporting caching structure invalidation
> > of assigned SVM capable devices. Emulated IOMMU exposes queue
> > invalidation capability and passes down all descriptors from the
> > guest to the physical IOMMU.
> > 
> > The assumption is that guest to host device ID mapping should be
> > resolved prior to calling IOMMU driver. Based on the device handle,
> > host IOMMU driver can replace certain fields before submit to the
> > invalidation queue.
> > 
> > Signed-off-by: Liu, Yi L 
> > Signed-off-by: Ashok Raj 
> > Signed-off-by: Jacob Pan 
> > ---
> >  drivers/iommu/intel-iommu.c | 170
> >  1 file changed, 170
> > insertions(+)
> > 
> > diff --git a/drivers/iommu/intel-iommu.c
> > b/drivers/iommu/intel-iommu.c index cae4042..c765448 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -4973,6 +4973,175 @@ static void
> > intel_iommu_detach_device(struct iommu_domain *domain,
> > dmar_remove_one_dev_info(to_dmar_domain(domain), dev); }
> >  
> > +/*
> > + * 3D array for converting IOMMU generic type-granularity to VT-d
> > granularity
> > + * X indexed by enum iommu_inv_type
> > + * Y indicates request without and with PASID
> > + * Z indexed by enum iommu_inv_granularity
> > + *
> > + * For an example, if we want to find the VT-d granularity
> > encoding for IOTLB
> > + * type, DMA request with PASID, and page selective. The look up
> > indices are:
> > + * [1][1][8], where
> > + * 1: IOMMU_INV_TYPE_TLB
> > + * 1: with PASID
> > + * 8: IOMMU_INV_GRANU_PAGE_PASID
> > + *
> > + * Granu_map array indicates validity of the table. 1: valid, 0:
> > invalid
> > + *
> > + */
> > +const static int
> > inv_type_granu_map[IOMMU_INV_NR_TYPE][2][IOMMU_INV_NR_GRANU] = {
> > +   /* extended dev IOTLBs, for dev-IOTLB, only global is
> > valid,
> > +  for dev-EXIOTLB, two valid granu */
> > +   {
> > +   {1},
> > +   {0, 0, 0, 0, 1, 1, 0, 0, 0}
> > +   },
> > +   /* IOTLB and EIOTLB */
> > +   {
> > +   {1, 1, 0, 1, 0, 0, 0, 0, 0},
> > +   {0, 0, 0, 0, 1, 0, 1, 1, 1}
> > +   },
> > +   /* PASID cache */
> > +   {
> > +   {0},
> > +   {0, 0, 0, 0, 1, 1, 0, 0, 0}
> > +   },
> > +   /* context cache */
> > +   {
> > +   {1, 1, 1}
> > +   }
> > +};
> > +
> > +const static u64
> > inv_type_granu_table[IOMMU_INV_NR_TYPE][2][IOMMU_INV_NR_GRANU] = {
> > +   /* extended dev IOTLBs, only global is valid */
> > +   {
> > +   {QI_DEV_IOTLB_GRAN_ALL},
> > +   {0, 0, 0, 0, QI_DEV_IOTLB_GRAN_ALL,
> > QI_DEV_IOTLB_GRAN_PASID_SEL, 0, 0, 0}
> > +   },
> > +   /* IOTLB and EIOTLB */
> > +   {
> > +   {DMA_TLB_GLOBAL_FLUSH, DMA_TLB_DSI_FLUSH, 0,
> > DMA_TLB_PSI_FLUSH},
> > +   {0, 0, 0, 0, QI_GRAN_ALL_ALL, 0, QI_GRAN_NONG_ALL,
> > QI_GRAN_NONG_PASID, QI_GRAN_PSI_PASID}
> > +   },
> > +   /* PASID cache */
> > +   {
> > +   {0},
> > +   {0, 0, 0, 0, QI_PC_ALL_PASIDS, QI_PC_PASID_SEL}
> > +   },
> > +   /* context cache */
> > +   {
> > +   {DMA_CCMD_GLOBAL_INVL, DMA_CCMD_DOMAIN_INVL,
> > DMA_CCMD_DEVICE_INVL}
> > +   }
> > +};
> > +
> > +static inline int to_vtd_granularity(int type, int granu, int
> > with_pasid, u64 *vtd_granu) +{
> > +   if (type >= IOMMU_INV_NR_TYPE || granu >=
> > IOMMU_INV_NR_GRANU || with_pasid > 1)
> > +   return -EINVAL;
> > +
> > +   if (inv_type_granu_map[type][with_pasid][granu] == 0)
> > +   return -EINVAL;
> > +
> > +   *vtd_granu = inv_type_granu_table[type][with_pasid][granu];
> > +
> > +   return 0;
> > +}
> > +
> > +static int intel_iommu_sva_invalidate(struct iommu_domain *domain,
> > +   struct device *dev, struct tlb_invalidate_info
> > *inv_info)  
> 
> inv_info->hdr.version is never checked, why do we have these if
> they're not used?
> 
the version was added to leave room for future extension. you are
right, it should be checked.
> > +{
> > +   struct intel_iommu *iommu;
> > +   struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +   struct device_domain_info *info;
> > +   u16 did, sid;
> > +   u8 bus, devfn;
> > +   int ret = 0;
> > +   u64 granu;
> > +   unsigned long flags;
> > +
> > +   if (!inv_info || !dmar_domain)
> > +   return -EINVAL;
> > +
> > +   iommu = device_to_iommu(dev, , );
> > +   if (!iommu)
> > +   return -ENODEV;
> > +
> > +   if (!dev || !dev_is_pci(dev))
> > +   return -ENODEV;

Re: [PATCH 09/12] PCI: remove CONFIG_PCI_BUS_ADDR_T_64BIT

[Bjorn Helgaas]

On Sun, Apr 15, 2018 at 04:59:44PM +0200, Christoph Hellwig wrote:
> This symbol is now always identical to CONFIG_ARCH_DMA_ADDR_T_64BIT, so
> remove it.
> 
> Signed-off-by: Christoph Hellwig 

Acked-by: Bjorn Helgaas 

Please merge this along with the rest of the series; let me know if you
need anything more from me.

> ---
>  drivers/pci/Kconfig | 4 
>  drivers/pci/bus.c   | 4 ++--
>  include/linux/pci.h | 2 +-
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 34b56a8f8480..29a487f31dae 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -5,10 +5,6 @@
>  
>  source "drivers/pci/pcie/Kconfig"
>  
> -config PCI_BUS_ADDR_T_64BIT
> - def_bool y if (ARCH_DMA_ADDR_T_64BIT || 64BIT)
> - depends on PCI
> -
>  config PCI_MSI
>   bool "Message Signaled Interrupts (MSI and MSI-X)"
>   depends on PCI
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index bc2ded4c451f..35b7fc87eac5 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -120,7 +120,7 @@ int devm_request_pci_bus_resources(struct device *dev,
>  EXPORT_SYMBOL_GPL(devm_request_pci_bus_resources);
>  
>  static struct pci_bus_region pci_32_bit = {0, 0xULL};
> -#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  static struct pci_bus_region pci_64_bit = {0,
>   (pci_bus_addr_t) 0xULL};
>  static struct pci_bus_region pci_high = {(pci_bus_addr_t) 0x1ULL,
> @@ -230,7 +230,7 @@ int pci_bus_alloc_resource(struct pci_bus *bus, struct 
> resource *res,
> resource_size_t),
>   void *alignf_data)
>  {
> -#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>   int rc;
>  
>   if (res->flags & IORESOURCE_MEM_64) {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 73178a2fcee0..55371cb827ad 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -670,7 +670,7 @@ int raw_pci_read(unsigned int domain, unsigned int bus, 
> unsigned int devfn,
>  int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
> int reg, int len, u32 val);
>  
> -#ifdef CONFIG_PCI_BUS_ADDR_T_64BIT
> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
>  typedef u64 pci_bus_addr_t;
>  #else
>  typedef u32 pci_bus_addr_t;
> -- 
> 2.17.0
> 
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 04/22] iommu/vt-d: add bind_pasid_table function

[Jean-Philippe Brucker]

On Tue, Apr 17, 2018 at 08:10:47PM +0100, Alex Williamson wrote:
[...]
> > +   /* Assign guest PASID table pointer and size order */
> > +   ctx_lo = (pasidt_binfo->base_ptr & VTD_PAGE_MASK) |
> > +   (pasidt_binfo->pasid_bits - MIN_NR_PASID_BITS);
> 
> Where does this IOMMU API interface define that base_ptr is 4K
> aligned or the format of the PASID table?  Are these all standardized
> or do they vary by host IOMMU?  If they're standards, maybe we could
> note that and the spec which defines them when we declare base_ptr.  If
> they're IOMMU specific then I don't understand how we'll match a user
> provided PASID table to the requirements and format of the host IOMMU.
> Thanks,

On SMMUv3 the minimum alignment for base_ptr is 64 bytes, so a guest
under a vSMMU might pass a pointer that's not aligned on 4k.

Maybe this information could be part of the data passed to userspace
about IOMMU table formats and features? They're not part of this series,
but I think we wanted to communicate IOMMU-specific features via sysfs.

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

Re: [PATCH v4 05/22] iommu: introduce iommu invalidate API function

[Jean-Philippe Brucker]

Hi Jacob,

On Mon, Apr 16, 2018 at 10:48:54PM +0100, Jacob Pan wrote:
[...]
> +/**
> + * enum iommu_inv_granularity - Generic invalidation granularity
> + *
> + * When an invalidation request is sent to IOMMU to flush translation caches,
> + * it may carry different granularity. These granularity levels are not 
> specific
> + * to a type of translation cache. For an example, PASID selective 
> granularity
> + * is only applicable to PASID cache invalidation.

I'm still confused by this, I think we should add more definitions
because architectures tend to use different names. What you call
"Translations caches" encompasses all caches that can be invalidated
with this request, right? So all of:

* "TLB" and "DTLB" that cache IOVA->GPA and GPA->PA (TLB is in the
  IOMMU, DTLB is an ATC in an endpoint),
* "PASID cache" that cache PASID->Translation Table,
* "Context cache" that cache RID->PASID table

Does this match the model you're using?

The last name is a bit unfortunate. Since the Arm architecture uses the
name "context" for what a PASID points to, "Device cache" would suit us
better but it's not important.

I don't understand what you mean by "PASID selective granularity is only
applicable to PASID cache invalidation", it seems to contradict the
preceding sentence. What if user sends an invalidation with
IOMMU_INV_TYPE_TLB and IOMMU_INV_GRANU_ALL_PASID? Doesn't this remove
from the TLBs all entries with the given PASID?

> + * This enum is a collection of granularities for all types of translation
> + * caches. The idea is to make it easy for IOMMU model specific driver do
> + * conversion from generic to model specific value.
> + */
> +enum iommu_inv_granularity {

In patch 9, inv_type_granu_map has some valid fields with granularity ==
0. Does it mean "invalidate all caches"?

I don't think user should ever be allowed to invalidate caches entries
of devices and domains it doesn't own.

> + IOMMU_INV_GRANU_DOMAIN = 1, /* all TLBs associated with a domain */
> + IOMMU_INV_GRANU_DEVICE, /* caching structure associated with a
> +  * device ID
> +  */
> + IOMMU_INV_GRANU_DOMAIN_PAGE,/* address range with a domain */

> + IOMMU_INV_GRANU_ALL_PASID,  /* cache of a given PASID */

If this corresponds to QI_GRAN_ALL_ALL in patch 9, the comment should be
"Cache of all PASIDs"? Or maybe "all entries for all PASIDs"? Is it
different from GRANU_DOMAIN then?

> + IOMMU_INV_GRANU_PASID_SEL,  /* only invalidate specified PASID */
> +
> + IOMMU_INV_GRANU_NG_ALL_PASID,   /* non-global within all PASIDs */
> + IOMMU_INV_GRANU_NG_PASID,   /* non-global within a PASIDs */

Are the "NG" variant needed since there is a IOMMU_INVALIDATE_GLOBAL_PAGE
below? We should drop either flag or granule.

FWIW I'm starting to think more granule options is actually better than
flags, because it flattens the combinations and keeps them to two
dimensions, that we can understand and explain with a table.

> + IOMMU_INV_GRANU_PAGE_PASID, /* page-selective within a PASID */

Maybe this should be called "NG_PAGE_PASID", and "DOMAIN_PAGE" should
instead be "PAGE_PASID". If I understood their meaning correctly, it
would be more consistent with the rest.

> + IOMMU_INV_NR_GRANU,
> +};
> +
> +/** enum iommu_inv_type - Generic translation cache types for invalidation
> + *
> + * Invalidation requests sent to IOMMU may indicate which translation cache
> + * to be operated on.
> + * Combined with enum iommu_inv_granularity, model specific driver can do a
> + * simple lookup to convert generic type to model specific value.
> + */
> +enum iommu_inv_type {

These should be flags (1 << 0), (1 << 1) etc, since IOMMUs will want to
invalidate multiple caches at once (at least DTLB and TLB). You could
then do for_each_set_bit in the driver

> + IOMMU_INV_TYPE_DTLB,/* device IOTLB */
> + IOMMU_INV_TYPE_TLB, /* IOMMU paging structure cache */
> + IOMMU_INV_TYPE_PASID,   /* PASID cache */
> + IOMMU_INV_TYPE_CONTEXT, /* device context entry cache */
> + IOMMU_INV_NR_TYPE
> +};

We need to summarize and explain valid combinations, because reading
inv_type_granu_map and inv_type_granu_table is a bit tedious. I tried to
reproduce inv_type_granu_map here (Cell format is PASID_TAGGED /
!PASID_TAGGED). Could you check if this matches your model?

type |   DTLB|TLB|   PASID   |  CONTEXT
 granule |   |   |   |
-+---+---+---+---
   - | / Y   | / Y   |   | / Y
 DOMAIN  |   | / Y   |   | / Y
 DEVICE  |   |   |   | / Y
 DOMAIN_PAGE |   | / Y   |   |
 ALL_PASID   |   Y   |   Y   |   |
 PASID_SEL   |   Y   |   |   Y   |
 NG_ALL_PASID|

Re: [PATCH RFC] dma-direct: Try reallocation with GFP_DMA32 if possible

[Christoph Hellwig]

On Fri, Apr 20, 2018 at 11:58:58AM +0200, Takashi Iwai wrote:
> It's because it's written on the tree with another fix patch I sent
> beforehand ("[PATCH 1/2] dma-direct: Don't repeat allocation for no-op
> GFP_DMA").
> 
> Could you check that one at first?  I'm fine to rebase and resubmit
> this one, if still preferred, though.

That one looks fine, too.  I'll add it to my patch queue.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH RFC] dma-direct: Try reallocation with GFP_DMA32 if possible

[Takashi Iwai]

On Fri, 20 Apr 2018 11:47:02 +0200,
Christoph Hellwig wrote:
> 
> On Mon, Apr 16, 2018 at 05:18:19PM +0200, Takashi Iwai wrote:
> > As the recent swiotlb bug revealed, we seem to have given up the
> > direct DMA allocation too early and felt back to swiotlb allocation.
> > The reason is that swiotlb allocator expected that dma_direct_alloc()
> > would try harder to get pages even below 64bit DMA mask with
> > GFP_DMA32, but the function doesn't do that but only deals with
> > GFP_DMA case.
> > 
> > This patch adds a similar fallback reallocation with GFP_DMA32 as
> > we've done with GFP_DMA.  The condition is that the coherent mask is
> > smaller than 64bit (i.e. some address limitation), and neither GFP_DMA
> > nor GFP_DMA32 is set beforehand.
> > 
> > Signed-off-by: Takashi Iwai 
> > 
> > ---
> > 
> > This is a resend of a test patch included in the previous thread
> > ("swiotlb: Fix unexpected swiotlb_alloc_coherent() failures").
> 
> I like the patch, but as-is it doesn't apply.  Can you resend it against
> latest Linus' tree?

It's because it's written on the tree with another fix patch I sent
beforehand ("[PATCH 1/2] dma-direct: Don't repeat allocation for no-op
GFP_DMA").

Could you check that one at first?  I'm fine to rebase and resubmit
this one, if still preferred, though.


thanks,

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

[PATCH] iommu/amd: fix unused-variable warning

[Tobias Regnery]

The iommu_table_lock is only used by code inside an ifdef CONFIG_IRQ_REMAP
block. This leads to the following warning with CONFIG_IRQ_REMAP=n:

amd_iommu.c:86:24: warning: 'iommu_table_lock' defined but not used 
[-Wunused-variable]

Guard the spinlock definition with the same ifdef.

Fixes: ea6166f4b83e9 ("iommu/amd: Split irq_lookup_table out of the 
amd_iommu_devtable_lock")
Signed-off-by: Tobias Regnery 
---
 drivers/iommu/amd_iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2a99f0f14795..5ba141768000 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -83,7 +83,9 @@
 
 static DEFINE_SPINLOCK(amd_iommu_devtable_lock);
 static DEFINE_SPINLOCK(pd_bitmap_lock);
+#ifdef CONFIG_IRQ_REMAP
 static DEFINE_SPINLOCK(iommu_table_lock);
+#endif
 
 /* List of all available dev_data structures */
 static LLIST_HEAD(dev_data_list);
-- 
2.17.0

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

Re: [PATCH] iommu/amd: fix unused-variable warning

[Sebastian Andrzej Siewior]

On 2018-04-20 11:28:36 [+0200], Tobias Regnery wrote:
> The iommu_table_lock is only used by code inside an ifdef CONFIG_IRQ_REMAP
> block. This leads to the following warning with CONFIG_IRQ_REMAP=n:
> 
> amd_iommu.c:86:24: warning: 'iommu_table_lock' defined but not used 
> [-Wunused-variable]
> 
> Guard the spinlock definition with the same ifdef.
> 
> Fixes: ea6166f4b83e9 ("iommu/amd: Split irq_lookup_table out of the 
> amd_iommu_devtable_lock")
> Signed-off-by: Tobias Regnery 

Thank you for spotting this, I am waiting for Jörg to pick up this
instead:
  http://lkml.kernel.org/r/20180404105713.633983-1-a...@arndb.de

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

Re: [PATCH RFC] dma-direct: Try reallocation with GFP_DMA32 if possible

[Christoph Hellwig]

On Mon, Apr 16, 2018 at 05:18:19PM +0200, Takashi Iwai wrote:
> As the recent swiotlb bug revealed, we seem to have given up the
> direct DMA allocation too early and felt back to swiotlb allocation.
> The reason is that swiotlb allocator expected that dma_direct_alloc()
> would try harder to get pages even below 64bit DMA mask with
> GFP_DMA32, but the function doesn't do that but only deals with
> GFP_DMA case.
> 
> This patch adds a similar fallback reallocation with GFP_DMA32 as
> we've done with GFP_DMA.  The condition is that the coherent mask is
> smaller than 64bit (i.e. some address limitation), and neither GFP_DMA
> nor GFP_DMA32 is set beforehand.
> 
> Signed-off-by: Takashi Iwai 
> 
> ---
> 
> This is a resend of a test patch included in the previous thread
> ("swiotlb: Fix unexpected swiotlb_alloc_coherent() failures").

I like the patch, but as-is it doesn't apply.  Can you resend it against
latest Linus' tree?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] iommu/vt-d: fix shift-out-of-bounds in bug checking

[changbin . du]

From: Changbin Du 

It allows to flush more than 4GB of device TLBs. So the mask should be
64bit wide. UBSAN captured this fault as below.

[3.760024] 

[3.768440] UBSAN: Undefined behaviour in drivers/iommu/dmar.c:1348:3
[3.774864] shift exponent 64 is too large for 32-bit type 'int'
[3.780853] CPU: 2 PID: 0 Comm: swapper/2 Tainted: G U
4.17.0-rc1+ #89
[3.788661] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.2.8 
01/26/2016
[3.796034] Call Trace:
[3.798472]  
[3.800479]  dump_stack+0x90/0xfb
[3.803787]  ubsan_epilogue+0x9/0x40
[3.807353]  __ubsan_handle_shift_out_of_bounds+0x10e/0x170
[3.812916]  ? qi_flush_dev_iotlb+0x124/0x180
[3.817261]  qi_flush_dev_iotlb+0x124/0x180
[3.821437]  iommu_flush_dev_iotlb+0x94/0xf0
[3.825698]  iommu_flush_iova+0x10b/0x1c0
[3.829699]  ? fq_ring_free+0x1d0/0x1d0
[3.833527]  iova_domain_flush+0x25/0x40
[3.837448]  fq_flush_timeout+0x55/0x160
[3.841368]  ? fq_ring_free+0x1d0/0x1d0
[3.845200]  ? fq_ring_free+0x1d0/0x1d0
[3.849034]  call_timer_fn+0xbe/0x310
[3.852696]  ? fq_ring_free+0x1d0/0x1d0
[3.856530]  run_timer_softirq+0x223/0x6e0
[3.860625]  ? sched_clock+0x5/0x10
[3.864108]  ? sched_clock+0x5/0x10
[3.867594]  __do_softirq+0x1b5/0x6f5
[3.871250]  irq_exit+0xd4/0x130
[3.874470]  smp_apic_timer_interrupt+0xb8/0x2f0
[3.879075]  apic_timer_interrupt+0xf/0x20
[3.883159]  
[3.885255] RIP: 0010:poll_idle+0x60/0xe7
[3.889252] RSP: 0018:b1b201943e30 EFLAGS: 0246 ORIG_RAX: 
ff13
[3.896802] RAX: 8020 RBX: 008e RCX: 001f
[3.903918] RDX:  RSI: 2819aa06 RDI: 
[3.911031] RBP: 9e93c6b33280 R08: 0010f717d567 R09: 0010d205
[3.918146] R10: b1b201943df8 R11: 0001 R12: e01b169d
[3.925260] R13:  R14: b12aa400 R15: 
[3.932382]  cpuidle_enter_state+0xb4/0x470
[3.936558]  do_idle+0x222/0x310
[3.939779]  cpu_startup_entry+0x78/0x90
[3.943693]  start_secondary+0x205/0x2e0
[3.947607]  secondary_startup_64+0xa5/0xb0
[3.951783] 


Signed-off-by: Changbin Du 
---
 drivers/iommu/dmar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index accf5838..e4ae600 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1345,7 +1345,7 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 
sid, u16 qdep,
struct qi_desc desc;
 
if (mask) {
-   BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
+   BUG_ON(addr & ((1ULL << (VTD_PAGE_SHIFT + mask)) - 1));
addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;
desc.high = QI_DEV_IOTLB_ADDR(addr) | QI_DEV_IOTLB_SIZE;
} else
-- 
2.7.4

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