Re: [PATCH v3 02/10] swiotlb: Factor out slot allocation and free

2019-05-15 Thread Lu Baolu

Hi,

On 5/13/19 3:05 PM, Christoph Hellwig wrote:

On Mon, May 06, 2019 at 09:54:30AM +0800, Lu Baolu wrote:

Agreed. I will prepare the next version simply without the optimization, so
the offset is not required.

For your changes in swiotlb, will you formalize them in patches or want
me to do this?


Please do it yourself given that you still need the offset and thus a
rework of the patches anyway.



Okay.

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


[PATCH] iommu: io-pgtable: Support non-coherent page tables

2019-05-15 Thread Bjorn Andersson
Describe the memory related to page table walks as non-cachable for iommu
instances that are not DMA coherent.

Signed-off-by: Bjorn Andersson 
---
 drivers/iommu/io-pgtable-arm.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index 4e21efbc4459..68ff22ffd2cb 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -803,9 +803,15 @@ arm_64_lpae_alloc_pgtable_s1(struct io_pgtable_cfg *cfg, 
void *cookie)
return NULL;
 
/* TCR */
-   reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
- (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) |
- (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
+   if (cfg->quirks & IO_PGTABLE_QUIRK_NO_DMA) {
+   reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
+ (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_IRGN0_SHIFT) |
+ (ARM_LPAE_TCR_RGN_WBWA << ARM_LPAE_TCR_ORGN0_SHIFT);
+   } else {
+   reg = (ARM_LPAE_TCR_SH_IS << ARM_LPAE_TCR_SH0_SHIFT) |
+ (ARM_LPAE_TCR_RGN_NC << ARM_LPAE_TCR_IRGN0_SHIFT) |
+ (ARM_LPAE_TCR_RGN_NC << ARM_LPAE_TCR_ORGN0_SHIFT);
+   }
 
switch (ARM_LPAE_GRANULE(data)) {
case SZ_4K:
-- 
2.18.0

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


Re: [PATCH v3 00/16] Shared virtual address IOMMU and VT-d support

2019-05-15 Thread Jacob Pan
Hi all,
Just wondering if you have any more feedbacks other than the cache
invalidate API change for archid?
I plan to do the next version on top of Jean's sva/api branch (common
iommu APIs) with minor tweak to support non-identity guest-host PASID
mapping. It would be great if I can address additional comments
together.

Thanks!

Jacob

On Fri,  3 May 2019 15:32:01 -0700
Jacob Pan  wrote:

> Shared virtual address (SVA), a.k.a, Shared virtual memory (SVM) on
> Intel platforms allow address space sharing between device DMA and
> applications. SVA can reduce programming complexity and enhance
> security. This series is intended to enable SVA virtualization, i.e.
> shared guest application address space and physical device DMA
> address. Only IOMMU portion of the changes are included in this
> series. Additional support is needed in VFIO and QEMU (will be
> submitted separately) to complete this functionality.
> 
> To make incremental changes and reduce the size of each patchset.
> This series does not inlcude support for page request services.
> 
> In VT-d implementation, PASID table is per device and maintained in
> the host. Guest PASID table is shadowed in VMM where virtual IOMMU is
> emulated.
> 
> .-.  .---.
> |   vIOMMU|  | Guest process CR3, FL only|
> | |  '---'
> ./
> | PASID Entry |--- PASID cache flush -
> '-'   |
> | |   V
> | |CR3 in GPA
> '-'
> Guest
> --| Shadow |--|
>   vv  v
> Host
> .-.  .--.
> |   pIOMMU|  | Bind FL for GVA-GPA  |
> | |  '--'
> ./  |
> | PASID Entry | V (Nested xlate)
> '\.--.
> | |   |SL for GPA-HPA, default domain|
> | |   '--'
> '-'
> Where:
>  - FL = First level/stage one page tables
>  - SL = Second level/stage two page tables
> 
> 
> This work is based on collaboration with other developers on the IOMMU
> mailing list. Notably,
> 
> [1] [PATCH v6 00/22] SMMUv3 Nested Stage Setup by Eric Auger
> https://lkml.org/lkml/2019/3/17/124
> 
> [2] [RFC PATCH 2/6] drivers core: Add I/O ASID allocator by
> Jean-Philippe Brucker
> https://www.spinics.net/lists/iommu/msg30639.html
> 
> [3] [RFC PATCH 0/5] iommu: APIs for paravirtual PASID allocation by
> Lu Baolu https://lkml.org/lkml/2018/11/12/1921
> 
> [4] [PATCH v5 00/23] IOMMU and VT-d driver support for Shared Virtual
> Address (SVA)
> https://lwn.net/Articles/754331/
> 
> There are roughly three parts:
> 1. Generic PASID allocator [1] with extension to support custom
> allocator 2. IOMMU cache invalidation passdown from guest to host
> 3. Guest PASID bind for nested translation
> 
> All generic IOMMU APIs are reused from [1], which has a v7 just
> published with no real impact to the patches used here. It is worth
> noting that unlike sMMU nested stage setup, where PASID table is
> owned by the guest, VT-d PASID table is owned by the host, individual
> PASIDs are bound instead of the PASID table.
> 
> This series is based on the new VT-d 3.0 Specification
> (https://software.intel.com/sites/default/files/managed/c5/15/vt-directed-io-spec.pdf).
> This is different than the older series in [4] which was based on the
> older specification that does not have scalable mode.
> 
> 
> ChangeLog:
>   - V3
> - Addressed thorough review comments from Eric Auger (Thank
> you!)
> - Moved IOASID allocator from driver core to IOMMU code per
>   suggestion by Christoph Hellwig
>   (https://lkml.org/lkml/2019/4/26/462)
> - Rebased on top of Jean's SVA API branch and Eric's v7[1]
>   (git://linux-arm.org/linux-jpb.git sva/api)
> - All IOMMU APIs are unmodified (except the new bind guest
> PASID call in patch 9/16)
> 
>   - V2
> - Rebased on Joerg's IOMMU x86/vt-d branch v5.1-rc4
> - Integrated with Eric Auger's new v7 series for common APIs
> (https://github.com/eauger/linux/tree/v5.1-rc3-2stage-v7)
> - Addressed review comments from Andy Shevchenko and Alex
> Williamson on IOASID custom allocator.
> - Support multiple custom IOASID allocators (vIOMMUs) and
> dynamic registration.
> 
> 
> Jacob Pan (13):
>   iommu: Introduce attach/detach_pasid_table API
>   ioasid: Add custom IOASID allocator
>   iommu/vt-d: Add custom allocator for IOASID
>   iommu/vtd: Optimize tlb invalidation for vIOMMU
>   iommu/vt-d: Replace Intel specific PASID allocator with IOASID
>   iommu: Introduce guest PASID bind function
>   iommu/vt-d: Move domain helper to header
>   iommu/vt-d: Avoid duplicated code for PASID setup
>  

Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API

2019-05-15 Thread Jacob Pan
On Wed, 15 May 2019 16:52:46 +0100
Jean-Philippe Brucker  wrote:

> On 14/05/2019 18:55, Jacob Pan wrote:
> > Yes, I agree to replace the standalone __64 pasid with this struct.
> > Looks more inline with address selective info., Just to double
> > confirm the new struct.
> > 
> > Jean, will you put this in your sva/api repo?  
> 
> Yes, I pushed it along with some documentation fixes (mainly getting
> rid of scripts/kernel-doc warnings and outputting valid rst)
> 
Just pulled, I am rebasing on top of this branch. If you could also
include our api for bind guest pasid, then we have a complete set of
common APIs in one place.
https://lkml.org/lkml/2019/5/3/775

I just need to add a small tweak for supporting non-identity guest-host
PASID mapping for the next version.

> Thanks,
> Jean

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


Re: [RFC PATCH v3 11/21] x86/watchdog/hardlockup: Add an HPET-based hardlockup detector

2019-05-15 Thread Ricardo Neri
On Tue, May 14, 2019 at 07:26:58AM -0700, Randy Dunlap wrote:
> On 5/14/19 7:02 AM, Ricardo Neri wrote:
> > diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> > index 15d0fbe27872..376a5db81aec 100644
> > --- a/arch/x86/Kconfig.debug
> > +++ b/arch/x86/Kconfig.debug
> > @@ -169,6 +169,17 @@ config IOMMU_LEAK
> >  config HAVE_MMIOTRACE_SUPPORT
> > def_bool y
> >  
> > +config X86_HARDLOCKUP_DETECTOR_HPET
> > +   bool "Use HPET Timer for Hard Lockup Detection"
> > +   select SOFTLOCKUP_DETECTOR
> > +   select HARDLOCKUP_DETECTOR
> > +   select HARDLOCKUP_DETECTOR_CORE
> > +   depends on HPET_TIMER && HPET && X86_64
> > +   help
> > + Say y to enable a hardlockup detector that is driven by an High-
> 
>  by a
> 
I'll correct.

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


Re: [PATCH v7 04/23] iommu: Introduce attach/detach_pasid_table API

2019-05-15 Thread Jean-Philippe Brucker
On 15/05/2019 14:06, Auger Eric wrote:
> Hi Jean-Philippe,
> 
> On 5/15/19 2:09 PM, Jean-Philippe Brucker wrote:
>> On 08/04/2019 13:18, Eric Auger wrote:
>>> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
>>> index edcc0dda7993..532a64075f23 100644
>>> --- a/include/uapi/linux/iommu.h
>>> +++ b/include/uapi/linux/iommu.h
>>> @@ -112,4 +112,51 @@ struct iommu_fault {
>>> struct iommu_fault_page_request prm;
>>> };
>>>  };
>>> +
>>> +/**
>>> + * SMMUv3 Stream Table Entry stage 1 related information
>>> + * The PASID table is referred to as the context descriptor (CD) table.
>>> + *
>>> + * @s1fmt: STE s1fmt (format of the CD table: single CD, linear table
>>> +   or 2-level table)
>>
>> Running "scripts/kernel-doc -v -none" on this header produces some
>> warnings. Not sure if we want to get rid of all of them, but we should
>> at least fix the coding style for this comment (line must start with
>> " * "). I'm fixing it up on my sva/api branch
> Thanks!
> 
> Let me know if you want me to do the job for additional fixes.

I fixed the others warnings as well, in case we ever want to include
this into the kernel doc

Thanks,
Jean

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


Re: [RFC PATCH v3 04/21] x86/hpet: Add hpet_set_comparator() for periodic and one-shot modes

2019-05-15 Thread Ricardo Neri
On Tue, May 14, 2019 at 07:24:38AM -0700, Randy Dunlap wrote:
> On 5/14/19 7:01 AM, Ricardo Neri wrote:
> > Instead of setting the timer period directly in hpet_set_periodic(), add a
> > new helper function hpet_set_comparator() that only sets the accumulator
> > and comparator. hpet_set_periodic() will only prepare the timer for
> > periodic mode and leave the expiration programming to
> > hpet_set_comparator().
> > 
> > This new function can also be used by other components (e.g., the HPET-
> > based hardlockup detector) which also need to configure HPET timers. Thus,
> > add its declaration into the hpet header file.
> > 
> > Cc: "H. Peter Anvin" 
> > Cc: Ashok Raj 
> > Cc: Andi Kleen 
> > Cc: Tony Luck 
> > Cc: Philippe Ombredanne 
> > Cc: Kate Stewart 
> > Cc: "Rafael J. Wysocki" 
> > Cc: Stephane Eranian 
> > Cc: Suravee Suthikulpanit 
> > Cc: "Ravi V. Shankar" 
> > Cc: x...@kernel.org
> > Originally-by: Suravee Suthikulpanit 
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/include/asm/hpet.h |  1 +
> >  arch/x86/kernel/hpet.c  | 57 -
> >  2 files changed, 45 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
> > index f132fbf984d4..e7098740f5ee 100644
> > --- a/arch/x86/include/asm/hpet.h
> > +++ b/arch/x86/include/asm/hpet.h
> > @@ -102,6 +102,7 @@ extern int hpet_rtc_timer_init(void);
> >  extern irqreturn_t hpet_rtc_interrupt(int irq, void *dev_id);
> >  extern int hpet_register_irq_handler(rtc_irq_handler handler);
> >  extern void hpet_unregister_irq_handler(rtc_irq_handler handler);
> > +extern void hpet_set_comparator(int num, unsigned int cmp, unsigned int 
> > period);
> >  
> >  #endif /* CONFIG_HPET_EMULATE_RTC */
> >  
> > diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> > index 560fc28e1d13..c5c5fc150193 100644
> > --- a/arch/x86/kernel/hpet.c
> > +++ b/arch/x86/kernel/hpet.c
> > @@ -289,6 +289,46 @@ static void hpet_legacy_clockevent_register(void)
> > printk(KERN_DEBUG "hpet clockevent registered\n");
> >  }
> >  
> > +/**
> > + * hpet_set_comparator() - Helper function for setting comparator register
> > + * @num:   The timer ID
> > + * @cmp:   The value to be written to the comparator/accumulator
> > + * @period:The value to be written to the period (0 = oneshot mode)
> > + *
> > + * Helper function for updating comparator, accumulator and period values.
> > + *
> > + * In periodic mode, HPET needs HPET_TN_SETVAL to be set before writing
> > + * to the Tn_CMP to update the accumulator. Then, HPET needs a second
> > + * write (with HPET_TN_SETVAL cleared) to Tn_CMP to set the period.
> > + * The HPET_TN_SETVAL bit is automatically cleared after the first write.
> > + *
> > + * For one-shot mode, HPET_TN_SETVAL does not need to be set.
> > + *
> > + * See the following documents:
> > + *   - Intel IA-PC HPET (High Precision Event Timers) Specification
> > + *   - AMD-8111 HyperTransport I/O Hub Data Sheet, Publication # 24674
> > + */
> > +void hpet_set_comparator(int num, unsigned int cmp, unsigned int period)
> > +{
> > +   if (period) {
> > +   unsigned int v = hpet_readl(HPET_Tn_CFG(num));
> > +
> > +   hpet_writel(v | HPET_TN_SETVAL, HPET_Tn_CFG(num));
> > +   }
> > +
> > +   hpet_writel(cmp, HPET_Tn_CMP(num));
> > +
> > +   if (!period)
> > +   return;
> > +
> > +   /* This delay is seldom used: never in one-shot mode and in periodic
> > +* only when reprogramming the timer.
> > +*/
> 
> comment style warning ;)
>

Uh! I'll correct this. Strangely, I reran checkpatch and it didn't catch
it.

Thanks and BR,
Ricardo


Re: [RFC PATCH v3 03/21] x86/hpet: Calculate ticks-per-second in a separate function

2019-05-15 Thread Ricardo Neri
On Tue, May 14, 2019 at 07:23:47AM -0700, Randy Dunlap wrote:
> On 5/14/19 7:01 AM, Ricardo Neri wrote:
> > It is easier to compute the expiration times of an HPET timer by using
> > its frequency (i.e., the number of times it ticks in a second) than its
> > period, as given in the capabilities register.
> > 
> > In addition to the HPET char driver, the HPET-based hardlockup detector
> > will also need to know the timer's frequency. Thus, create a common
> > function that both can use.
> > 
> > Cc: "H. Peter Anvin" 
> > Cc: Ashok Raj 
> > Cc: Andi Kleen 
> > Cc: Tony Luck 
> > Cc: Clemens Ladisch 
> > Cc: Arnd Bergmann 
> > Cc: Philippe Ombredanne 
> > Cc: Kate Stewart 
> > Cc: "Rafael J. Wysocki" 
> > Cc: Stephane Eranian 
> > Cc: Suravee Suthikulpanit 
> > Cc: "Ravi V. Shankar" 
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  drivers/char/hpet.c  | 31 ---
> >  include/linux/hpet.h |  1 +
> >  2 files changed, 25 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> > index d0ad85900b79..bdcbecfdb858 100644
> > --- a/drivers/char/hpet.c
> > +++ b/drivers/char/hpet.c
> > @@ -836,6 +836,29 @@ static unsigned long hpet_calibrate(struct hpets 
> > *hpetp)
> > return ret;
> >  }
> >  
> > +u64 hpet_get_ticks_per_sec(u64 hpet_caps)
> > +{
> > +   u64 ticks_per_sec, period;
> > +
> > +   period = (hpet_caps & HPET_COUNTER_CLK_PERIOD_MASK) >>
> > +HPET_COUNTER_CLK_PERIOD_SHIFT; /* fs, 10^-15 */
> > +
> > +   /*
> > +* The frequency is the reciprocal of the period. The period is given
> > +* femtoseconds per second. Thus, prepare a dividend to obtain the
> 
>* in femtoseconds per second.
> 

Thanks for your review Randy! I'll fix this grammar issue.
> > +* frequency in ticks per second.
> > +*/
> > +
> > +   /* 10^15 femtoseconds per second */
> > +   ticks_per_sec = 1000uLL;
> 
>   ULL is overwhelmingly used in the kernel.
> 

Sure, I'll update it.

BR,
Ricardo


Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API

2019-05-15 Thread Jean-Philippe Brucker
On 14/05/2019 18:55, Jacob Pan wrote:
> Yes, I agree to replace the standalone __64 pasid with this struct.
> Looks more inline with address selective info., Just to double confirm
> the new struct.
> 
> Jean, will you put this in your sva/api repo?

Yes, I pushed it along with some documentation fixes (mainly getting rid
of scripts/kernel-doc warnings and outputting valid rst)

Thanks,
Jean


Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API

2019-05-15 Thread Jean-Philippe Brucker
On 15/05/2019 15:47, Tian, Kevin wrote:
>> From: Jean-Philippe Brucker
>> Sent: Wednesday, May 15, 2019 7:04 PM
>>
>> On 14/05/2019 18:44, Jacob Pan wrote:
>>> Hi Thank you both for the explanation.
>>>
>>> On Tue, 14 May 2019 11:41:24 +0100
>>> Jean-Philippe Brucker  wrote:
>>>
 On 14/05/2019 08:36, Auger Eric wrote:
> Hi Jacob,
>
> On 5/14/19 12:16 AM, Jacob Pan wrote:
>> On Mon, 13 May 2019 18:09:48 +0100
>> Jean-Philippe Brucker  wrote:
>>
>>> On 13/05/2019 17:50, Auger Eric wrote:
> struct iommu_inv_pasid_info {
> #define IOMMU_INV_PASID_FLAGS_PASID   (1 << 0)
> #define IOMMU_INV_PASID_FLAGS_ARCHID  (1 << 1)
>   __u32   flags;
>   __u32   archid;
>   __u64   pasid;
> };
 I agree it does the job now. However it looks a bit strange to
 do a PASID based invalidation in my case - SMMUv3 nested stage -
 where I don't have any PASID involved.

 Couldn't we call it context based invalidation then? A context
 can be tagged by a PASID or/and an ARCHID.
>>>
>>> I think calling it "context" would be confusing as well (I
>>> shouldn't have used it earlier), since VT-d uses that name for
>>> device table entries (=STE on Arm SMMU). Maybe "addr_space"?
>>>
>> I am still struggling to understand what ARCHID is after scanning
>> through SMMUv3.1 spec. It seems to be a constant for a given SMMU.
>> Why do you need to pass it down every time? Could you point to me
>> the document or explain a little more on ARCHID use cases.
>> We have three fileds called pasid under this struct
>> iommu_cache_invalidate_info{}
>> Gets confusing :)
> archid is a generic term. That's why you did not find it in the
> spec ;-)
>
> On ARM SMMU the archid is called the ASID (Address Space ID, up to
> 16 bits. The ASID is stored in the Context Descriptor Entry (your
> PASID entry) and thus characterizes a given stage 1 translation
> "context"/"adress space".

 Yes, another way to look at it is, for a given address space:
 * PASID tags device-IOTLB (ATC) entries.
 * ASID (here called archid) tags IOTLB entries.

 They could have the same value, but it depends on the guest's
 allocation policy which isn't in our control. With my PASID patches
 for SMMUv3, they have different values. So we need both fields if we
 intend to invalidate both ATC and IOTLB with a single call.

>>> For ASID invalidation, there is also page/address selective within an
>>> ASID, right? I guess it is CMD_TLBI_NH_VA?
>>> So the single call to invalidate both ATC & IOTLB should share the same
>>> address information. i.e.
>>> struct iommu_inv_addr_info {}
>>>
>>> Just out of curiosity, what is the advantage of having guest tag its
>>> ATC with its own PASID? I thought you were planning to use custom
>>> ioasid allocator to get PASID from host.
>>
>> Hm, for the moment I mostly considered the custom ioasid allocator for
>> Intel platforms. On Arm platforms the SR-IOV model where each VM has its
>> own PASID space is still very much on the table. This would be the only
>> model supported by a vSMMU emulation for example, since the SMMU
>> doesn't
>> have PASID allocation commands.
>>
> 
> I didn't get how ATS works in such case, if device ATC PASID is different
> from IOTLB ASID. Who will be responsible for translation in-between?

ATS with the SMMU works like this:

* The PCI function sends a Translation Request with PASID.
* The SMMU walks the PASID table (which we call context descriptor
table), finds the context descriptor indexed by PASID. This context
descriptor has an ASID field, and a page directory pointer.
* After successfully walking the page tables, the SMMU may add an IOTLB
entry tagged by ASID and address, then returns a Translation Completion.
* The PCI function adds an ATC entry tagged by PASID and address.

I think the ASID on Arm CPUs is roughly equivalent to Intel PCID. One
reason we use ASIDs for IOTLBs is that with SVA, the ASID of an address
space is the same on the CPU side. And when the CPU executes a TLB
invalidation instructions, it also invalidates the corresponding IOTLB
entries. It's nice for vSVA because you don't need to context-switch to
the host to send an IOTLB invalidation. But only non-PCI devices that
implement SVA benefit from this at the moment, because ATC invalidations
still have to go through the SMMU command queue.

Thanks,
Jean


RE: [PATCH v3 02/16] iommu: Introduce cache_invalidate API

2019-05-15 Thread Tian, Kevin
> From: Jean-Philippe Brucker
> Sent: Wednesday, May 15, 2019 7:04 PM
> 
> On 14/05/2019 18:44, Jacob Pan wrote:
> > Hi Thank you both for the explanation.
> >
> > On Tue, 14 May 2019 11:41:24 +0100
> > Jean-Philippe Brucker  wrote:
> >
> >> On 14/05/2019 08:36, Auger Eric wrote:
> >>> Hi Jacob,
> >>>
> >>> On 5/14/19 12:16 AM, Jacob Pan wrote:
>  On Mon, 13 May 2019 18:09:48 +0100
>  Jean-Philippe Brucker  wrote:
> 
> > On 13/05/2019 17:50, Auger Eric wrote:
> >>> struct iommu_inv_pasid_info {
> >>> #define IOMMU_INV_PASID_FLAGS_PASID   (1 << 0)
> >>> #define IOMMU_INV_PASID_FLAGS_ARCHID  (1 << 1)
> >>>   __u32   flags;
> >>>   __u32   archid;
> >>>   __u64   pasid;
> >>> };
> >> I agree it does the job now. However it looks a bit strange to
> >> do a PASID based invalidation in my case - SMMUv3 nested stage -
> >> where I don't have any PASID involved.
> >>
> >> Couldn't we call it context based invalidation then? A context
> >> can be tagged by a PASID or/and an ARCHID.
> >
> > I think calling it "context" would be confusing as well (I
> > shouldn't have used it earlier), since VT-d uses that name for
> > device table entries (=STE on Arm SMMU). Maybe "addr_space"?
> >
>  I am still struggling to understand what ARCHID is after scanning
>  through SMMUv3.1 spec. It seems to be a constant for a given SMMU.
>  Why do you need to pass it down every time? Could you point to me
>  the document or explain a little more on ARCHID use cases.
>  We have three fileds called pasid under this struct
>  iommu_cache_invalidate_info{}
>  Gets confusing :)
> >>> archid is a generic term. That's why you did not find it in the
> >>> spec ;-)
> >>>
> >>> On ARM SMMU the archid is called the ASID (Address Space ID, up to
> >>> 16 bits. The ASID is stored in the Context Descriptor Entry (your
> >>> PASID entry) and thus characterizes a given stage 1 translation
> >>> "context"/"adress space".
> >>
> >> Yes, another way to look at it is, for a given address space:
> >> * PASID tags device-IOTLB (ATC) entries.
> >> * ASID (here called archid) tags IOTLB entries.
> >>
> >> They could have the same value, but it depends on the guest's
> >> allocation policy which isn't in our control. With my PASID patches
> >> for SMMUv3, they have different values. So we need both fields if we
> >> intend to invalidate both ATC and IOTLB with a single call.
> >>
> > For ASID invalidation, there is also page/address selective within an
> > ASID, right? I guess it is CMD_TLBI_NH_VA?
> > So the single call to invalidate both ATC & IOTLB should share the same
> > address information. i.e.
> > struct iommu_inv_addr_info {}
> >
> > Just out of curiosity, what is the advantage of having guest tag its
> > ATC with its own PASID? I thought you were planning to use custom
> > ioasid allocator to get PASID from host.
> 
> Hm, for the moment I mostly considered the custom ioasid allocator for
> Intel platforms. On Arm platforms the SR-IOV model where each VM has its
> own PASID space is still very much on the table. This would be the only
> model supported by a vSMMU emulation for example, since the SMMU
> doesn't
> have PASID allocation commands.
> 

I didn't get how ATS works in such case, if device ATC PASID is different
from IOTLB ASID. Who will be responsible for translation in-between?

Thanks
Kevin


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread starost...@gmail.com

Dne 15.5.2019 v 15:54 Oliver Neukum napsal(a):

1. Determine whether the bug depends on the use of an IOMMU

No, bug not depends on the use of an IOMMU. System crash on both cases.


2.Send a new report to the corresponding mailing list

Which mailing list is correct?

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


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread Oliver Neukum
On Mi, 2019-05-15 at 13:21 +0200,  starost...@gmail.com  wrote:
> Dne 15.5.2019 v 11:46 Oliver Neukum napsal(a):
> 
> 
> >   Most helpful. First, try to replicate this with the iommu
> > disabled.
> > 
> 
> I am trying this with "iommu disabled" in bios, but system crash
> too: https://paste.ee/p/wUgHl
> 
> 
> 
> 
> >   Secondly, make a proper bugreport mentioning the affected
> > kernel version (5.1)
> > 
> 
> How can I do this?

1. Determine whether the bug depends on the use of an IOMMU
2.Send a new report to the corresponding mailing list

> 
> 
> 
> >   Thirdly, if possible replicate this with the vanilla kernel
> > from kernel.org
> > 
> 
> I am afraid, that is not possible. My skills is not too good - is
> there some procedure to how do this?
> 
> I made test with mainline kernel 5.1.2 from
> https://kernel.ubuntu.com/~kernel-ppa/mainline/
> 
> but computer crash on boot (kernel panic: unable to mount root
> fs...).

Your initrd has most likely not included a driver for the filesystem on
your root device.

Regards
Oliver

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

Re: [PATCH v7 04/23] iommu: Introduce attach/detach_pasid_table API

2019-05-15 Thread Auger Eric
Hi Jean-Philippe,

On 5/15/19 2:09 PM, Jean-Philippe Brucker wrote:
> On 08/04/2019 13:18, Eric Auger wrote:
>> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
>> index edcc0dda7993..532a64075f23 100644
>> --- a/include/uapi/linux/iommu.h
>> +++ b/include/uapi/linux/iommu.h
>> @@ -112,4 +112,51 @@ struct iommu_fault {
>>  struct iommu_fault_page_request prm;
>>  };
>>  };
>> +
>> +/**
>> + * SMMUv3 Stream Table Entry stage 1 related information
>> + * The PASID table is referred to as the context descriptor (CD) table.
>> + *
>> + * @s1fmt: STE s1fmt (format of the CD table: single CD, linear table
>> +   or 2-level table)
> 
> Running "scripts/kernel-doc -v -none" on this header produces some
> warnings. Not sure if we want to get rid of all of them, but we should
> at least fix the coding style for this comment (line must start with
> " * "). I'm fixing it up on my sva/api branch
Thanks!

Let me know if you want me to do the job for additional fixes.

Eric


> 
> Thanks,
> Jean
> 
>> + * @s1dss: STE s1dss (specifies the behavior when pasid_bits != 0
>> +   and no pasid is passed along with the incoming transaction)
>> + * Please refer to the smmu 3.x spec (ARM IHI 0070A) for full details
>> + */
>> +struct iommu_pasid_smmuv3 {
>> +#define PASID_TABLE_SMMUV3_CFG_VERSION_1 1
>> +__u32   version;
>> +__u8 s1fmt;
>> +__u8 s1dss;
>> +__u8 padding[2];
>> +};
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH v3 2/4] iommu/dma-iommu: Handle deferred devices

2019-05-15 Thread Tom Murphy
like this?

In that case we need to add a call to iommu_dma_alloc_remap.

>From 862aeebb601008cf863e3aff4ff8ed7cefebeefa Mon Sep 17 00:00:00 2001
From: Tom Murphy 
Date: Wed, 15 May 2019 05:43:25 -0700
Subject: [PATCH] iommu/dma-iommu: Handle deferred devices

Handle devices which defer their attach to the iommu in the dma-iommu api

Signed-off-by: Tom Murphy 
---
 drivers/iommu/dma-iommu.c | 27 ++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7f313cfa9..a48ae906d 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 

 struct iommu_dma_msi_page {
 struct list_headlist;
@@ -323,6 +324,21 @@ static int iommu_dma_init_domain(struct
iommu_domain *domain, dma_addr_t base,
 return iova_reserve_iommu_regions(dev, domain);
 }

+static int handle_deferred_device(struct device *dev,
+struct iommu_domain *domain)
+{
+const struct iommu_ops *ops = domain->ops;
+
+if (!is_kdump_kernel())
+return 0;
+
+if (unlikely(ops->is_attach_deferred &&
+ops->is_attach_deferred(domain, dev)))
+return iommu_attach_device(domain, dev);
+
+return 0;
+}
+
 /**
  * dma_info_to_prot - Translate DMA API directions and attributes to IOMMU API
  *page flags.
@@ -432,6 +448,9 @@ static dma_addr_t __iommu_dma_map(struct device
*dev, phys_addr_t phys,
 size_t iova_off = 0;
 dma_addr_t iova;

+if (unlikely(handle_deferred_device(dev, domain)))
+return DMA_MAPPING_ERROR;
+
 if (cookie->type == IOMMU_DMA_IOVA_COOKIE) {
 iova_off = iova_offset(>iovad, phys);
 size = iova_align(>iovad, size + iova_off);
@@ -609,6 +628,9 @@ static void *iommu_dma_alloc_remap(struct device
*dev, size_t size,
 dma_addr_t iova;
 void *vaddr;

+if (unlikely(handle_deferred_device(dev, domain)))
+return DMA_MAPPING_ERROR;
+
 *dma_handle = DMA_MAPPING_ERROR;

 min_size = alloc_sizes & -alloc_sizes;
@@ -836,7 +858,7 @@ static dma_addr_t iommu_dma_map_page(struct device
*dev, struct page *page,
 bool coherent = dev_is_dma_coherent(dev);
 dma_addr_t dma_handle;

-dma_handle =__iommu_dma_map(dev, phys, size,
+dma_handle = __iommu_dma_map(dev, phys, size,
 dma_info_to_prot(dir, coherent, attrs),
 iommu_get_dma_domain(dev));
 if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
@@ -954,6 +976,9 @@ static int iommu_dma_map_sg(struct device *dev,
struct scatterlist *sg,
 unsigned long mask = dma_get_seg_boundary(dev);
 int i;

+if (unlikely(handle_deferred_device(dev, domain)))
+return 0;
+
 if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
 iommu_dma_sync_sg_for_device(dev, sg, nents, dir);

-- 
2.20.0

On Tue, May 7, 2019 at 7:40 AM Christoph Hellwig  wrote:
>
> On Mon, May 06, 2019 at 07:52:04PM +0100, Tom Murphy wrote:
> > +static int handle_deferred_device(struct device *dev)
> > +{
> > + struct iommu_domain *domain;
> > + const struct iommu_ops *ops;
> > +
> > + if (!is_kdump_kernel())
> > + return 0;
> > +
> > + domain = iommu_get_domain_for_dev(dev);
>
> > - dma_handle =__iommu_dma_map(dev, phys, size,
> > + if (unlikely(handle_deferred_device(dev)))
> > + return DMA_MAPPING_ERROR;
> > +
> > + dma_handle = __iommu_dma_map(dev, phys, size,
>
> __iommu_dma_map already looks up the domain, and as far as I can
> tell all callers need the handle_deferred_device call.  Should we
> just move it to there and pass the domain from the caller?
>
> Also shouldn't the iommu_attach_device call inside
> handle_deferred_device also get an unlikely marker?


Re: [PATCH v7 04/23] iommu: Introduce attach/detach_pasid_table API

2019-05-15 Thread Jean-Philippe Brucker
On 08/04/2019 13:18, Eric Auger wrote:
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index edcc0dda7993..532a64075f23 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -112,4 +112,51 @@ struct iommu_fault {
>   struct iommu_fault_page_request prm;
>   };
>  };
> +
> +/**
> + * SMMUv3 Stream Table Entry stage 1 related information
> + * The PASID table is referred to as the context descriptor (CD) table.
> + *
> + * @s1fmt: STE s1fmt (format of the CD table: single CD, linear table
> +   or 2-level table)

Running "scripts/kernel-doc -v -none" on this header produces some
warnings. Not sure if we want to get rid of all of them, but we should
at least fix the coding style for this comment (line must start with
" * "). I'm fixing it up on my sva/api branch

Thanks,
Jean

> + * @s1dss: STE s1dss (specifies the behavior when pasid_bits != 0
> +   and no pasid is passed along with the incoming transaction)
> + * Please refer to the smmu 3.x spec (ARM IHI 0070A) for full details
> + */
> +struct iommu_pasid_smmuv3 {
> +#define PASID_TABLE_SMMUV3_CFG_VERSION_1 1
> + __u32   version;
> + __u8 s1fmt;
> + __u8 s1dss;
> + __u8 padding[2];
> +};
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread starost...@gmail.com

Dne 15.5.2019 v 11:46 Oliver Neukum napsal(a):

Most helpful. First, try to replicate this with the iommu disabled.
I am trying this with "iommu disabled" in bios, but system crash too: 
https://paste.ee/p/wUgHl


Secondly, make a proper bugreport mentioning the affected kernel 
version (5.1)

How can I do this?

Thirdly, if possible replicate this with the vanilla kernel from 
kernel.org
I am afraid, that is not possible. My skills is not too good - is there 
some procedure to how do this?
I made test with mainline kernel 5.1.2 from 
https://kernel.ubuntu.com/~kernel-ppa/mainline/

but computer crash on boot (kernel panic: unable to mount root fs...).

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


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread starost...@gmail.com

  
  
Dne 15.5.2019 v 11:46 Oliver Neukum napsal(a):

  Most helpful. First, try to replicate this with the iommu
disabled.

I am trying this with "iommu disabled" in bios, but system crash
too: https://paste.ee/p/wUgHl


  Secondly, make a proper bugreport mentioning the affected
kernel version (5.1)

How can I do this?


  Thirdly, if possible replicate this with the vanilla kernel
from kernel.org

I am afraid, that is not possible. My skills is not too good - is
there some procedure to how do this?
I made test with mainline kernel 5.1.2 from
https://kernel.ubuntu.com/~kernel-ppa/mainline/
but computer crash on boot (kernel panic: unable to mount root
fs...).

starosta
  

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

Re: [PATCH v3 02/16] iommu: Introduce cache_invalidate API

2019-05-15 Thread Jean-Philippe Brucker
On 14/05/2019 18:44, Jacob Pan wrote:
> Hi Thank you both for the explanation.
> 
> On Tue, 14 May 2019 11:41:24 +0100
> Jean-Philippe Brucker  wrote:
> 
>> On 14/05/2019 08:36, Auger Eric wrote:
>>> Hi Jacob,
>>>
>>> On 5/14/19 12:16 AM, Jacob Pan wrote:  
 On Mon, 13 May 2019 18:09:48 +0100
 Jean-Philippe Brucker  wrote:
  
> On 13/05/2019 17:50, Auger Eric wrote:  
>>> struct iommu_inv_pasid_info {
>>> #define IOMMU_INV_PASID_FLAGS_PASID (1 << 0)
>>> #define IOMMU_INV_PASID_FLAGS_ARCHID(1 << 1)
>>> __u32   flags;
>>> __u32   archid;
>>> __u64   pasid;
>>> };
>> I agree it does the job now. However it looks a bit strange to
>> do a PASID based invalidation in my case - SMMUv3 nested stage -
>> where I don't have any PASID involved.
>>
>> Couldn't we call it context based invalidation then? A context
>> can be tagged by a PASID or/and an ARCHID.
>
> I think calling it "context" would be confusing as well (I
> shouldn't have used it earlier), since VT-d uses that name for
> device table entries (=STE on Arm SMMU). Maybe "addr_space"?
>  
 I am still struggling to understand what ARCHID is after scanning
 through SMMUv3.1 spec. It seems to be a constant for a given SMMU.
 Why do you need to pass it down every time? Could you point to me
 the document or explain a little more on ARCHID use cases.
 We have three fileds called pasid under this struct
 iommu_cache_invalidate_info{}
 Gets confusing :)  
>>> archid is a generic term. That's why you did not find it in the
>>> spec ;-)
>>>
>>> On ARM SMMU the archid is called the ASID (Address Space ID, up to
>>> 16 bits. The ASID is stored in the Context Descriptor Entry (your
>>> PASID entry) and thus characterizes a given stage 1 translation
>>> "context"/"adress space".  
>>
>> Yes, another way to look at it is, for a given address space:
>> * PASID tags device-IOTLB (ATC) entries.
>> * ASID (here called archid) tags IOTLB entries.
>>
>> They could have the same value, but it depends on the guest's
>> allocation policy which isn't in our control. With my PASID patches
>> for SMMUv3, they have different values. So we need both fields if we
>> intend to invalidate both ATC and IOTLB with a single call.
>>
> For ASID invalidation, there is also page/address selective within an
> ASID, right? I guess it is CMD_TLBI_NH_VA?
> So the single call to invalidate both ATC & IOTLB should share the same
> address information. i.e.
> struct iommu_inv_addr_info {}
> 
> Just out of curiosity, what is the advantage of having guest tag its
> ATC with its own PASID? I thought you were planning to use custom
> ioasid allocator to get PASID from host.

Hm, for the moment I mostly considered the custom ioasid allocator for
Intel platforms. On Arm platforms the SR-IOV model where each VM has its
own PASID space is still very much on the table. This would be the only
model supported by a vSMMU emulation for example, since the SMMU doesn't
have PASID allocation commands.

> Also ASID is 16 bit as Eric said and PASID (substreamID?) is 20 bit,
> right?

Yes. Some implementations have 8-bit ASIDs, but I think those would be
on embedded rather than server class platforms. And yes, if it wasn't
confusing enough, the Arm SMMU uses "SubstreamID" (SSID) for PASIDs :)

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


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread Oliver Neukum
On Mi, 2019-05-15 at 09:54 +0200,  starost...@gmail.com  wrote:
> Hello,
> can I still help with this problem? It's very important for us. Thank you.

Your first step would be t verify whether your kernel has the
fix coming in the patch Joerg mentioned.

Regards
Oliver

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


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread starost...@gmail.com

  
  
As I wrote, I made new test on kernel Linux version
5.1.0-050100-generic amd64:
https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.1/
Same problem, system crash after a few seconds. Full kern.log: https://paste.ee/p/EmLsw

Unfortunately, I can't judge if the patch is there, but
I found, that this patch was added to kernel 5.0.2:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu/amd_iommu.c?id=4e50ce03976fbc8ae995a000c4b10c737467beaa
  Can I somehow
verify that the patch is in this ubuntu kernel 5.1.0?
starosta
Dne 15.5.2019 v 11:17 Oliver Neukum
  napsal(a):


  On Mi, 2019-05-15 at 09:54 +0200,  starost...@gmail.com  wrote:

  
Hello,
can I still help with this problem? It's very important for us. Thank you.

  
  
Your first step would be t verify whether your kernel has the
fix coming in the patch Joerg mentioned.

	Regards
		Oliver




  

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

Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread starost...@gmail.com
As I wrote, I made new test on kernel Linux version 5.1.0-050100-generic 
amd64:

https://kernel.ubuntu.com/~kernel-ppa/mainline/v5.1/
Same problem, system crash after a few seconds. Full kern.log: 
https://paste.ee/p/EmLsw


Unfortunately, I can't judge if the patch is there, but I found, that 
this patch was added to kernel 5.0.2:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu/amd_iommu.c?id=4e50ce03976fbc8ae995a000c4b10c737467beaa
Can I somehow verify that the patch is in this ubuntu kernel 5.1.0?

starosta

Dne 15.5.2019 v 11:17 Oliver Neukum napsal(a):

On Mi, 2019-05-15 at 09:54 +0200,  starost...@gmail.com  wrote:

Hello,
can I still help with this problem? It's very important for us. Thank you.

Your first step would be t verify whether your kernel has the
fix coming in the patch Joerg mentioned.

Regards
Oliver



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


Re: Linux crash when using FTDI FT232R USB to serial converter on AMD boards.

2019-05-15 Thread starost...@gmail.com

Hello,
can I still help with this problem? It's very important for us. Thank you.

starosta

Dne 6.5.2019 v 9:10 starost...@gmail.com napsal(a):
New test on kernel Linux version 5.1.0-050100-generic. Same problem, 
system crash after a few seconds.

Full kern.log: https://paste.ee/p/EmLsw
I can do access to my pc through SSH if useful.

starosta

Dne 3.5.2019 v 17:37 Joerg Roedel napsal(a):

On Mon, Apr 29, 2019 at 11:48:47AM +0200, Johan Hovold wrote:

So this is a debian 4.18 kernel seemingly crashing due to a xhci or
iommu issue.

Can you reproduce this on a mainline kernel?

If so, please post the corresponding logs to the lists and CC the xhci
and iommu maintainers (added to CC).

Your kernel is probably missing this upstream fix:

4e50ce03976f iommu/amd: fix sg->dma_address for sg->offset bigger 
than PAGE_SIZE


Regards,

Joerg





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


Re: [PATCH v4 1/1] iommu/io-pgtable-arm: Add support to use system cache

2019-05-15 Thread Vivek Gautam
On Tue, May 14, 2019 at 12:26 PM Vivek Gautam
 wrote:
>
> Hi Robin,
>
>
> On Mon, May 13, 2019 at 5:02 PM Robin Murphy  wrote:
> >
> > On 13/05/2019 11:04, Vivek Gautam wrote:
> > > Few Qualcomm platforms such as, sdm845 have an additional outer
> > > cache called as System cache, aka. Last level cache (LLC) that
> > > allows non-coherent devices to upgrade to using caching.
> > > This cache sits right before the DDR, and is tightly coupled
> > > with the memory controller. The clients using this cache request
> > > their slices from this system cache, make it active, and can then
> > > start using it.
> > >
> > > There is a fundamental assumption that non-coherent devices can't
> > > access caches. This change adds an exception where they *can* use
> > > some level of cache despite still being non-coherent overall.
> > > The coherent devices that use cacheable memory, and CPU make use of
> > > this system cache by default.
> > >
> > > Looking at memory types, we have following -
> > > a) Normal uncached :- MAIR 0x44, inner non-cacheable,
> > >outer non-cacheable;
> > > b) Normal cached :-   MAIR 0xff, inner read write-back non-transient,
> > >outer read write-back non-transient;
> > >attribute setting for coherenet I/O devices.
> > > and, for non-coherent i/o devices that can allocate in system cache
> > > another type gets added -
> > > c) Normal sys-cached :- MAIR 0xf4, inner non-cacheable,
> > >  outer read write-back non-transient
> > >
> > > Coherent I/O devices use system cache by marking the memory as
> > > normal cached.
> > > Non-coherent I/O devices should mark the memory as normal
> > > sys-cached in page tables to use system cache.
> > >
> > > Signed-off-by: Vivek Gautam 
> > > ---
> > >
> > > V3 version of this patch and related series can be found at [1].
> > >
> > > This change is a realisation of following changes from downstream msm-4.9:
> > > iommu: io-pgtable-arm: Implement IOMMU_USE_UPSTREAM_HINT[2]
> > >
> > > Changes since v3:
> > >   - Dropping support to cache i/o page tables to system cache. Getting 
> > > support
> > > for data buffers is the first step.
> > > Removed io-pgtable quirk and related change to add domain attribute.
> > >
> > > Glmark2 numbers on SDM845 based cheza board:
> > >
> > > S.No.|with LLC support   |without LLC support
> > >   |   for data buffers   |
> > > ---
> > > 1|4480; 72.3fps  |4042; 65.2fps
> > > 2|4500; 72.6fps  |4039; 65.1fps
> > > 3|4523; 72.9fps  |4106; 66.2fps
> > > 4|4489; 72.4fps  |4104; 66.2fps
> > > 5|4518; 72.9fps  |4072; 65.7fps
> > >
> > > [1] https://patchwork.kernel.org/cover/10772629/
> > > [2] 
> > > https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?h=msm-4.9=d4c72c413ea27c43f60825193d4de9cb8ffd9602
> > >
> > >   drivers/iommu/io-pgtable-arm.c | 9 -
> > >   include/linux/iommu.h  | 1 +
> > >   2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iommu/io-pgtable-arm.c 
> > > b/drivers/iommu/io-pgtable-arm.c
> > > index d3700ec15cbd..2dbafe697531 100644
> > > --- a/drivers/iommu/io-pgtable-arm.c
> > > +++ b/drivers/iommu/io-pgtable-arm.c
> > > @@ -167,10 +167,12 @@
> > >   #define ARM_LPAE_MAIR_ATTR_MASK 0xff
> > >   #define ARM_LPAE_MAIR_ATTR_DEVICE   0x04
> > >   #define ARM_LPAE_MAIR_ATTR_NC   0x44
> > > +#define ARM_LPAE_MAIR_ATTR_QCOM_SYS_CACHE0xf4

s/QCOM_SYS_CACHE/INC_OWBRWA/ looks more appropriate here.

> > >   #define ARM_LPAE_MAIR_ATTR_WBRWA0xff
> > >   #define ARM_LPAE_MAIR_ATTR_IDX_NC   0
> > >   #define ARM_LPAE_MAIR_ATTR_IDX_CACHE1
> > >   #define ARM_LPAE_MAIR_ATTR_IDX_DEV  2
> > > +#define ARM_LPAE_MAIR_ATTR_IDX_QCOM_SYS_CACHE3
> >
> > Here at the implementation level, I'd rather just call these what they
> > are, i.e. s/QCOM_SYS_CACHE/INC_OWBRWA/.

Allow me to change this to - s/QCOM_SYS_CACHE/INC_OC, or
s/QCOM_SYS_CACHE/INC_OCACHE to go inline with IDX_NC and IDX_CACHE.
Sounds okay?

> >
>
> Thanks for the review.
> Sure, will change this as suggested.
>
> > >
> > >   /* IOPTE accessors */
> > >   #define iopte_deref(pte,d) __va(iopte_to_paddr(pte, d))
> > > @@ -442,6 +444,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
> > > arm_lpae_io_pgtable *data,
> > >   else if (prot & IOMMU_CACHE)
> > >   pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
> > >   << ARM_LPAE_PTE_ATTRINDX_SHIFT);
> > > + else if (prot & IOMMU_QCOM_SYS_CACHE)
> >
> > Where in the call stack is this going to be decided? (I don't recall the
> > previous discussions ever really reaching a solid conclusion on how to
> > separate responsibilities).
> >
>
> Based on the last discussion [1], I understood that we may