Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption
On 04/21/17 at 02:55pm, Dave Hansen wrote: > On 04/18/2017 02:22 PM, Tom Lendacky wrote: > > Add sysfs support for SME so that user-space utilities (kdump, etc.) can > > determine if SME is active. > > > > A new directory will be created: > > /sys/kernel/mm/sme/ > > > > And two entries within the new directory: > > /sys/kernel/mm/sme/active > > /sys/kernel/mm/sme/encryption_mask > > Why do they care, and what will they be doing with this information? Since kdump will copy old memory but need this to know if the old memory was encrypted or not. With this sysfs file we can know the previous SME status and pass to kdump kernel as like a kernel param. Tom, have you got chance to try if it works or not? Thanks Dave ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 1/8] iommu: Introduce bind_pasid_table API function
On 27/04/17 07:36, Liu, Yi L wrote: > On Wed, Apr 26, 2017 at 05:56:45PM +0100, Jean-Philippe Brucker wrote: >> Hi Yi, Jacob, >> >> On 26/04/17 11:11, Liu, Yi L wrote: >>> From: Jacob Pan >>> >>> Virtual IOMMU was proposed to support Shared Virtual Memory (SVM) use >>> case in the guest: >>> https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg05311.html >>> >>> As part of the proposed architecture, when a SVM capable PCI >>> device is assigned to a guest, nested mode is turned on. Guest owns the >>> first level page tables (request with PASID) and performs GVA->GPA >>> translation. Second level page tables are owned by the host for GPA->HPA >>> translation for both request with and without PASID. >>> >>> A new IOMMU driver interface is therefore needed to perform tasks as >>> follows: >>> * Enable nested translation and appropriate translation type >>> * Assign guest PASID table pointer (in GPA) and size to host IOMMU >>> >>> This patch introduces new functions called iommu_(un)bind_pasid_table() >>> to IOMMU APIs. Architecture specific IOMMU function can be added later >>> to perform the specific steps for binding pasid table of assigned devices. >>> >>> This patch also adds model definition in iommu.h. It would be used to >>> check if the bind request is from a compatible entity. e.g. a bind >>> request from an intel_iommu emulator may not be supported by an ARM SMMU >>> driver. >>> >>> Signed-off-by: Jacob Pan >>> Signed-off-by: Liu, Yi L >>> --- >>> drivers/iommu/iommu.c | 19 +++ >>> include/linux/iommu.h | 31 +++ >>> 2 files changed, 50 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index dbe7f65..f2da636 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1134,6 +1134,25 @@ int iommu_attach_device(struct iommu_domain *domain, >>> struct device *dev) >>> } >>> EXPORT_SYMBOL_GPL(iommu_attach_device); >>> >>> +int iommu_bind_pasid_table(struct iommu_domain *domain, struct device *dev, >>> + struct pasid_table_info *pasidt_binfo) >> >> I guess that domain can always be deduced from dev using >> iommu_get_domain_for_dev, and doesn't need to be passed as argument? >> >> For the next version of my SVM series, I was thinking of passing group >> instead of device to iommu_bind. Since all devices in a group are expected >> to share the same mappings (whether they want it or not), users will have >> to do iommu_group_for_each_dev anyway (as you do in patch 6/8). So it >> might be simpler to let the IOMMU core take the group lock and do >> group->domain->ops->bind_task(dev...) for each device. The question also >> holds for iommu_do_invalidate in patch 3/8. >> >> This way the prototypes would be: >> int iommu_bind...(struct iommu_group *group, struct ... *info) >> int iommu_unbind...(struct iommu_group *group, struct ...*info) >> int iommu_invalidate...(struct iommu_group *group, struct ...*info) >> >> For PASID table binding it might not matter much, as VFIO will most likely >> be the only user. But task binding will be called by device drivers, which >> by now should be encouraged to do things at iommu_group granularity. >> Alternatively it could be done implicitly like in iommu_attach_device, >> with "iommu_bind_device_x" calling "iommu_bind_group_x". >> >> >> Extending this reasoning, since groups in a domain are also supposed to >> have the same mappings, then similarly to map/unmap, >> bind/unbind/invalidate should really be done with an iommu_domain (and >> nothing else) as target argument. However this requires the IOMMU core to >> keep a group list in each domain, which might complicate things a little >> too much. >> >> But "all devices in a domain share the same PASID table" is the paradigm >> I'm currently using in the guts of arm-smmu-v3. And I wonder if, as with >> iommu_group, it should be made more explicit to users, so they don't >> assume that devices within a domain are isolated from each others with >> regard to PASID DMA. >> >>> +{ >>> + if (unlikely(!domain->ops->bind_pasid_table)) >>> + return -EINVAL; >>> + >>> + return domain->ops->bind_pasid_table(domain, dev, pasidt_binfo); >>> +} >>> +EXPORT_SYMBOL_GPL(iommu_bind_pasid_table); >>> + >>> +int iommu_unbind_pasid_table(struct iommu_domain *domain, struct device >>> *dev) >>> +{ >>> + if (unlikely(!domain->ops->unbind_pasid_table)) >>> + return -EINVAL; >>> + >>> + return domain->ops->unbind_pasid_table(domain, dev); >>> +} >>> +EXPORT_SYMBOL_GPL(iommu_unbind_pasid_table); >>> + >>> static void __iommu_detach_device(struct iommu_domain *domain, >>> struct device *dev) >>> { >>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h >>> index 0ff5111..491a011 100644 >>> --- a/include/linux/iommu.h >>> +++ b/include/linux/iommu.h >>> @@ -131,6 +131,15 @@ struct iommu_dm_region { >>> int prot; >>> }; >>> >>> +s
Re: [Qemu-devel] [RFC PATCH 12/20] Memory: Add func to fire pasidt_bind notifier
On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > + void *data) > > > > +{ > > > > +IOMMUNotifier *iommu_notifier; > > > > +IOMMUNotifierFlag request_flags; > > > > + > > > > +assert(memory_region_is_iommu(mr)); > > > > + > > > > +/*TODO: support other bind requests with smaller gran, > > > > + * e.g. bind signle pasid entry > > > > + */ > > > > +request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > + > > > > +QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > +if (iommu_notifier->notifier_flags & request_flags) { > > > > +iommu_notifier->notify(iommu_notifier, data); > > > > +break; > > > > +} > > > > +} > > > > > > Peter, > > > > > > should this reuse ->notify, or should it be different function pointer > > > in IOMMUNotifier? > > > > Hi Paolo, > > > > Thx for your review. > > > > I think it should be “->notify” here. In this patchset, the new notifier > > is registered with the existing notifier registration API. So the all the > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > When the flag meets, trigger it by “->notify”. The diagram below shows > > my understanding , wish it helps to make me understood. > > > > VFIOContainer > >| > >giommu_list(VFIOGuestIOMMU) > > \ > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > > | | | > > mr->iommu_notify: IOMMUNotifier ->IOMMUNotifier -> IOMMUNotifier > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb > > invalidate) > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > > no start/end check, and there may be other types of bind notfier flag in > > future, so I added a separate fire func for SVM bind notifier. > > I agree with Paolo that this interface might not be the suitable place > for the SVM notifiers (just like what I worried about in previous > discussions). > > The biggest problem is that, if you see current notifier mechanism, > it's per-memory-region. However iiuc your messages should be > per-iommu, or say, per translation unit. While, for each iommu, there > can be more than one memory regions (ppc can be an example). When > there are more than one MRs binded to the same iommu unit, which > memory region should you register to? Any one of them, or all? > > So my conclusion is, it just has nothing to do with memory regions... > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > even move a step further, to isolate IOTLB notifications (targeted at > memory regions and with start/end ranges) out of SVM/other > notifications, since they are different in general. So we basically > need two notification mechanism: > > - one for memory regions, currently what I can see is IOTLB > notifications > > - one for translation units, currently I see all the rest of > notifications needed in virt-svm in this category > > Maybe some RFC patches would be good to show what I mean... I'll see > whether I can prepare some. Here it is (on qemu-devel): [RFC PATCH 0/8] IOMMU: introduce common IOMMUObject Thanks, -- Peter Xu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 02/20] intel_iommu: exposed extended-context mode to guest
On Wed, Apr 26, 2017 at 06:06:32PM +0800, Liu, Yi L wrote: > VT-d implementations reporting PASID or PRS fields as "Set", must also > report ecap.ECS as "Set". Extended-Context is required for SVM. > > When ECS is reported, intel iommu driver would initiate extended root entry > and extended context entry, and also PASID table if there is any SVM capable > device. > > Signed-off-by: Liu, Yi L > --- > hw/i386/intel_iommu.c | 131 > +++-- > hw/i386/intel_iommu_internal.h | 9 +++ > include/hw/i386/intel_iommu.h | 2 +- > 3 files changed, 97 insertions(+), 45 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 400d0d1..bf98fa5 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -497,6 +497,11 @@ static inline bool vtd_root_entry_present(VTDRootEntry > *root) > return root->val & VTD_ROOT_ENTRY_P; > } > > +static inline bool vtd_root_entry_upper_present(VTDRootEntry *root) > +{ > +return root->rsvd & VTD_ROOT_ENTRY_P; > +} > + > static int vtd_get_root_entry(IntelIOMMUState *s, uint8_t index, >VTDRootEntry *re) > { > @@ -509,6 +514,9 @@ static int vtd_get_root_entry(IntelIOMMUState *s, uint8_t > index, > return -VTD_FR_ROOT_TABLE_INV; > } > re->val = le64_to_cpu(re->val); > +if (s->ecs) { > +re->rsvd = le64_to_cpu(re->rsvd); > +} I feel it slightly hacky to play with re->rsvd. How about: union VTDRootEntry { struct { uint64_t val; uint64_t rsvd; } base; struct { uint64_t ext_lo; uint64_t ext_hi; } extended; }; (Or any better way that can get rid of rsvd...) Even: struct VTDRootEntry { union { struct { uint64_t val; uint64_t rsvd; } base; struct { uint64_t ext_lo; uint64_t ext_hi; } extended; } data; bool extended; }; Then we read the entry into data, and setup extended bit. A benefit of it is that we may avoid passing around IntelIOMMUState everywhere to know whether we are using extended context entries. > return 0; > } > > @@ -517,19 +525,30 @@ static inline bool > vtd_context_entry_present(VTDContextEntry *context) > return context->lo & VTD_CONTEXT_ENTRY_P; > } > > -static int vtd_get_context_entry_from_root(VTDRootEntry *root, uint8_t index, > - VTDContextEntry *ce) > +static int vtd_get_context_entry_from_root(IntelIOMMUState *s, > + VTDRootEntry *root, uint8_t index, VTDContextEntry *ce) > { > -dma_addr_t addr; > +dma_addr_t addr, ce_size; > > /* we have checked that root entry is present */ > -addr = (root->val & VTD_ROOT_ENTRY_CTP) + index * sizeof(*ce); > -if (dma_memory_read(&address_space_memory, addr, ce, sizeof(*ce))) { > +ce_size = (s->ecs) ? (2 * sizeof(*ce)) : (sizeof(*ce)); > +addr = (s->ecs && (index > 0x7f)) ? > + ((root->rsvd & VTD_ROOT_ENTRY_CTP) + (index - 0x80) * ce_size) : > + ((root->val & VTD_ROOT_ENTRY_CTP) + index * ce_size); > + > +if (dma_memory_read(&address_space_memory, addr, ce, ce_size)) { > trace_vtd_re_invalid(root->rsvd, root->val); > return -VTD_FR_CONTEXT_TABLE_INV; > } > -ce->lo = le64_to_cpu(ce->lo); > -ce->hi = le64_to_cpu(ce->hi); > + > +ce[0].lo = le64_to_cpu(ce[0].lo); > +ce[0].hi = le64_to_cpu(ce[0].hi); Again, I feel this even hackier. :) I would slightly prefer to play the same union trick to context entries, just like what I proposed to the root entries above... > + > +if (s->ecs) { > +ce[1].lo = le64_to_cpu(ce[1].lo); > +ce[1].hi = le64_to_cpu(ce[1].hi); > +} > + > return 0; > } > > @@ -595,9 +614,11 @@ static inline uint32_t > vtd_get_agaw_from_context_entry(VTDContextEntry *ce) > return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9; > } > > -static inline uint32_t vtd_ce_get_type(VTDContextEntry *ce) > +static inline uint32_t vtd_ce_get_type(IntelIOMMUState *s, > + VTDContextEntry *ce) > { > -return ce->lo & VTD_CONTEXT_ENTRY_TT; > +return s->ecs ? (ce->lo & VTD_CONTEXT_ENTRY_TT) : > +(ce->lo & VTD_EXT_CONTEXT_ENTRY_TT); > } > > static inline uint64_t vtd_iova_limit(VTDContextEntry *ce) > @@ -842,16 +863,20 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, > uint8_t bus_num, > return ret_fr; > } > > -if (!vtd_root_entry_present(&re)) { > +if (!vtd_root_entry_present(&re) || > +(s->ecs && (devfn > 0x7f) && (!vtd_root_entry_upper_present(&re { > /* Not error - it's okay we don't have root entry. */ > trace_vtd_re_not_present(bus_num); > return -VTD_FR_ROOT_ENTRY_P; > -} else if (re.rsvd || (re.val & VTD_ROOT_ENTRY_RSVD)) { > -trace_vtd_re_invalid(re.rsvd
Re: [Qemu-devel] [RFC PATCH 12/20] Memory: Add func to fire pasidt_bind notifier
On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > + void *data) > > > > +{ > > > > +IOMMUNotifier *iommu_notifier; > > > > +IOMMUNotifierFlag request_flags; > > > > + > > > > +assert(memory_region_is_iommu(mr)); > > > > + > > > > +/*TODO: support other bind requests with smaller gran, > > > > + * e.g. bind signle pasid entry > > > > + */ > > > > +request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > + > > > > +QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > +if (iommu_notifier->notifier_flags & request_flags) { > > > > +iommu_notifier->notify(iommu_notifier, data); > > > > +break; > > > > +} > > > > +} > > > > > > Peter, > > > > > > should this reuse ->notify, or should it be different function pointer > > > in IOMMUNotifier? > > > > Hi Paolo, > > > > Thx for your review. > > > > I think it should be “->notify” here. In this patchset, the new notifier > > is registered with the existing notifier registration API. So the all the > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > When the flag meets, trigger it by “->notify”. The diagram below shows > > my understanding , wish it helps to make me understood. > > > > VFIOContainer > >| > >giommu_list(VFIOGuestIOMMU) > > \ > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 ... > > | | | > > mr->iommu_notify: IOMMUNotifier ->IOMMUNotifier -> IOMMUNotifier > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb > > invalidate) > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier has > > no start/end check, and there may be other types of bind notfier flag in > > future, so I added a separate fire func for SVM bind notifier. > > I agree with Paolo that this interface might not be the suitable place > for the SVM notifiers (just like what I worried about in previous > discussions). > > The biggest problem is that, if you see current notifier mechanism, > it's per-memory-region. However iiuc your messages should be > per-iommu, or say, per translation unit. Hi Peter, yes, you're right. the newly added notifier is per-iommu. > While, for each iommu, there > can be more than one memory regions (ppc can be an example). When > there are more than one MRs binded to the same iommu unit, which > memory region should you register to? Any one of them, or all? Honestly, I'm not expert on ppc. According to the current code, I can only find one MR initialized with memory_region_init_iommu() in spapr_tce_table_realize(). So to better get your point, let me check. Do you mean there may be multiple of iommu MRs behind a iommu? I admit it must be considered if there are multiple iommu MRs. I may choose to register for one of them since the notifier is per-iommu as you've pointed. Then vIOMMU emulator need to trigger the notifier with the correct MR. Not sure if ppc vIOMMU is fine with it. > So my conclusion is, it just has nothing to do with memory regions... > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > even move a step further, to isolate IOTLB notifications (targeted at > memory regions and with start/end ranges) out of SVM/other > notifications, since they are different in general. So we basically > need two notification mechanism: > > - one for memory regions, currently what I can see is IOTLB > notifications > > - one for translation units, currently I see all the rest of > notifications needed in virt-svm in this category > > Maybe some RFC patches would be good to show what I mean... I'll see > whether I can prepare some. I agree that it would be helpful to split the two kinds of notifiers. I marked it as a FIXME in patch 0006 of this series. Just saw your RFC patch for common IOMMUObject. Thx for your work, would try to review it. Besides the notifier registration, pls also help to review the SVM virtualization itself. Would be glad to know your comments. Thanks, Yi L > Thanks, > > -- > Peter Xu > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Qemu-devel] [RFC PATCH 12/20] Memory: Add func to fire pasidt_bind notifier
On Thu, Apr 27, 2017 at 06:25:37PM +0800, Liu, Yi L wrote: > On Thu, Apr 27, 2017 at 02:14:27PM +0800, Peter Xu wrote: > > On Thu, Apr 27, 2017 at 10:37:19AM +0800, Liu, Yi L wrote: > > > On Wed, Apr 26, 2017 at 03:50:16PM +0200, Paolo Bonzini wrote: > > > > > > > > > > > > On 26/04/2017 12:06, Liu, Yi L wrote: > > > > > +void memory_region_notify_iommu_svm_bind(MemoryRegion *mr, > > > > > + void *data) > > > > > +{ > > > > > +IOMMUNotifier *iommu_notifier; > > > > > +IOMMUNotifierFlag request_flags; > > > > > + > > > > > +assert(memory_region_is_iommu(mr)); > > > > > + > > > > > +/*TODO: support other bind requests with smaller gran, > > > > > + * e.g. bind signle pasid entry > > > > > + */ > > > > > +request_flags = IOMMU_NOTIFIER_SVM_PASIDT_BIND; > > > > > + > > > > > +QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > > > > > +if (iommu_notifier->notifier_flags & request_flags) { > > > > > +iommu_notifier->notify(iommu_notifier, data); > > > > > +break; > > > > > +} > > > > > +} > > > > > > > > Peter, > > > > > > > > should this reuse ->notify, or should it be different function pointer > > > > in IOMMUNotifier? > > > > > > Hi Paolo, > > > > > > Thx for your review. > > > > > > I think it should be “->notify” here. In this patchset, the new notifier > > > is registered with the existing notifier registration API. So the all the > > > notifiers are in the mr->iommu_notify list. And notifiers are labeled > > > by notify flag, so it is able to differentiate the IOMMUNotifier nodes. > > > When the flag meets, trigger it by “->notify”. The diagram below shows > > > my understanding , wish it helps to make me understood. > > > > > > VFIOContainer > > >| > > >giommu_list(VFIOGuestIOMMU) > > > \ > > > VFIOGuestIOMMU1 -> VFIOGuestIOMMU2 -> VFIOGuestIOMMU3 > > > ... > > > | | | > > > mr->iommu_notify: IOMMUNotifier ->IOMMUNotifier -> IOMMUNotifier > > > (Flag:MAP/UNMAP) (Flag:SVM bind) (Flag:tlb > > > invalidate) > > > > > > > > > Actually, compared with the MAP/UNMAP notifier, the newly added notifier > > > has > > > no start/end check, and there may be other types of bind notfier flag in > > > future, so I added a separate fire func for SVM bind notifier. > > > > I agree with Paolo that this interface might not be the suitable place > > for the SVM notifiers (just like what I worried about in previous > > discussions). > > > > The biggest problem is that, if you see current notifier mechanism, > > it's per-memory-region. However iiuc your messages should be > > per-iommu, or say, per translation unit. > > Hi Peter, > > yes, you're right. the newly added notifier is per-iommu. > > > While, for each iommu, there > > can be more than one memory regions (ppc can be an example). When > > there are more than one MRs binded to the same iommu unit, which > > memory region should you register to? Any one of them, or all? > > Honestly, I'm not expert on ppc. According to the current code, > I can only find one MR initialized with memory_region_init_iommu() > in spapr_tce_table_realize(). So to better get your point, let me > check. Do you mean there may be multiple of iommu MRs behind a iommu? I am not either. :) But yes, that's what I mean. At least that's how I understand it. > > I admit it must be considered if there are multiple iommu MRs. I may > choose to register for one of them since the notifier is per-iommu as > you've pointed. Then vIOMMU emulator need to trigger the notifier with > the correct MR. Not sure if ppc vIOMMU is fine with it. > > > So my conclusion is, it just has nothing to do with memory regions... > > > > Instead of a different function pointer in IOMMUNotifer, IMHO we can > > even move a step further, to isolate IOTLB notifications (targeted at > > memory regions and with start/end ranges) out of SVM/other > > notifications, since they are different in general. So we basically > > need two notification mechanism: > > > > - one for memory regions, currently what I can see is IOTLB > > notifications > > > > - one for translation units, currently I see all the rest of > > notifications needed in virt-svm in this category > > > > Maybe some RFC patches would be good to show what I mean... I'll see > > whether I can prepare some. > > I agree that it would be helpful to split the two kinds of notifiers. I > marked it as a FIXME in patch 0006 of this series. Just saw your RFC patch > for common IOMMUObject. Thx for your work, would try to review it. Thanks, looking forward to your review comments. > > Besides the notifier registration, pls also help to review the SVM > virtualization itself. Would be glad to know your comments. Yes. It's on my list. Thanks, -- Peter Xu _
Re: [RFC PATCH 03/20] intel_iommu: add "svm" option
On Wed, Apr 26, 2017 at 06:06:33PM +0800, Liu, Yi L wrote: > Expose "Shared Virtual Memory" to guest by using "svm" option. > Also use "svm" to expose SVM related capabilities to guest. > e.g. "-device intel-iommu, svm=on" > > Signed-off-by: Liu, Yi L > --- > hw/i386/intel_iommu.c | 10 ++ > hw/i386/intel_iommu_internal.h | 5 + > include/hw/i386/intel_iommu.h | 1 + > 3 files changed, 16 insertions(+) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index bf98fa5..ba1e7eb 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -2453,6 +2453,7 @@ static Property vtd_properties[] = { > DEFINE_PROP_BOOL("x-buggy-eim", IntelIOMMUState, buggy_eim, false), > DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE), > DEFINE_PROP_BOOL("ecs", IntelIOMMUState, ecs, FALSE), > +DEFINE_PROP_BOOL("svm", IntelIOMMUState, svm, FALSE), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2973,6 +2974,15 @@ static void vtd_init(IntelIOMMUState *s) > s->ecap |= VTD_ECAP_ECS; > } > > +if (s->svm) { > +if (!s->ecs || !x86_iommu->pt_supported || !s->caching_mode) { > +error_report("Need to set ecs, pt, caching-mode for svm"); > +exit(1); > +} > +s->cap |= VTD_CAP_DWD | VTD_CAP_DRD; > +s->ecap |= VTD_ECAP_PRS | VTD_ECAP_PTS | VTD_ECAP_PASID28; > +} > + > if (s->caching_mode) { > s->cap |= VTD_CAP_CM; > } > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index 71a1c1e..f2a7d12 100644 > --- a/hw/i386/intel_iommu_internal.h > +++ b/hw/i386/intel_iommu_internal.h > @@ -191,6 +191,9 @@ > #define VTD_ECAP_PT (1ULL << 6) > #define VTD_ECAP_MHMV (15ULL << 20) > #define VTD_ECAP_ECS(1ULL << 24) > +#define VTD_ECAP_PASID28(1ULL << 28) Could I ask what's this bit? On my spec, it says this bit is reserved and defunct (spec version: June 2016). > +#define VTD_ECAP_PRS(1ULL << 29) > +#define VTD_ECAP_PTS(0xeULL << 35) Would it better we avoid using 0xe here, or at least add some comment? > > /* CAP_REG */ > /* (offset >> 4) << 24 */ > @@ -207,6 +210,8 @@ > #define VTD_CAP_PSI (1ULL << 39) > #define VTD_CAP_SLLPS ((1ULL << 34) | (1ULL << 35)) > #define VTD_CAP_CM (1ULL << 7) > +#define VTD_CAP_DWD (1ULL << 54) > +#define VTD_CAP_DRD (1ULL << 55) Just to confirm: after this series, we should support drain read/write then, right? Thanks, > > /* Supported Adjusted Guest Address Widths */ > #define VTD_CAP_SAGAW_SHIFT 8 > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index ae21fe5..8981615 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -267,6 +267,7 @@ struct IntelIOMMUState { > > bool caching_mode; /* RO - is cap CM enabled? */ > bool ecs; /* Extended Context Support */ > +bool svm; /* Shared Virtual Memory */ > > dma_addr_t root;/* Current root table pointer */ > bool root_extended; /* Type of root table (extended or not) > */ > -- > 1.9.1 > -- Peter Xu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/arm-smmu-v3: Poll for CMDQ drain completion more effectively
From: Sunil Goutham Modified polling on CMDQ consumer similar to how polling is done for TLB SYNC completion in SMMUv2 driver. Code changes are done with reference to 8513c8930069 iommu/arm-smmu: Poll for TLB sync completion more effectively Poll timeout has been increased which addresses issue of 100us timeout not sufficient, when command queue is full with TLB invalidation commands. Signed-off-by: Sunil Goutham Signed-off-by: Geetha --- drivers/iommu/arm-smmu-v3.c | 15 --- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index d412bdd..34599d4 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -379,6 +379,9 @@ #define CMDQ_SYNC_0_CS_NONE(0UL << CMDQ_SYNC_0_CS_SHIFT) #define CMDQ_SYNC_0_CS_SEV (2UL << CMDQ_SYNC_0_CS_SHIFT) +#define CMDQ_DRAIN_TIMEOUT_US 1000 +#define CMDQ_SPIN_COUNT10 + /* Event queue */ #define EVTQ_ENT_DWORDS4 #define EVTQ_MAX_SZ_SHIFT 7 @@ -737,7 +740,8 @@ static void queue_inc_prod(struct arm_smmu_queue *q) */ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe) { - ktime_t timeout = ktime_add_us(ktime_get(), ARM_SMMU_POLL_TIMEOUT_US); + ktime_t timeout = ktime_add_us(ktime_get(), CMDQ_DRAIN_TIMEOUT_US); + unsigned int spin_cnt, delay = 1; while (queue_sync_cons(q), (drain ? !queue_empty(q) : queue_full(q))) { if (ktime_compare(ktime_get(), timeout) > 0) @@ -746,8 +750,13 @@ static int queue_poll_cons(struct arm_smmu_queue *q, bool drain, bool wfe) if (wfe) { wfe(); } else { - cpu_relax(); - udelay(1); + for (spin_cnt = 0; +spin_cnt < CMDQ_SPIN_COUNT; spin_cnt++) { + cpu_relax(); + continue; + } + udelay(delay); + delay *= 2; } } -- 2.7.4 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 0/3] Cavium ThunderX2 SMMUv3 errata workarounds
From: Geetha Cavium CN99xx SMMUv3 implementation has two Silicon Erratas. 1. Errata ID #74 SMMU register alias Page 1 is not implemented 2. Errata ID #126 SMMU doesnt support unique IRQ lines for gerror, eventq and cmdq-sync The following patchset does software workaround for these two erratas. This series is based on RFC patch. https://www.spinics.net/lists/arm-kernel/msg575739.html As suggested by Will Deacon, code is modified to use silicon id to enable errata#74 workaround. Linu Cherian (1): iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74 Geetha (2): arm64: Add MIDR values for Cavium cn99xx SoCs iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 Documentation/arm64/silicon-errata.txt | 2 ++ arch/arm64/include/asm/cputype.h | 3 ++ drivers/acpi/arm64/iort.c | 14 +++- drivers/iommu/arm-smmu-v3.c| 64 +- 4 files changed, 73 insertions(+), 10 deletions(-) -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/3] arm64: Add MIDR values for Cavium cn99xx SoCs
From: Geetha Add MIDR values for Cavium cn99xx SoCs Signed-off-by: Geetha --- arch/arm64/include/asm/cputype.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h index fc50271..066fad0 100644 --- a/arch/arm64/include/asm/cputype.h +++ b/arch/arm64/include/asm/cputype.h @@ -85,6 +85,7 @@ #define CAVIUM_CPU_PART_THUNDERX 0x0A1 #define CAVIUM_CPU_PART_THUNDERX_81XX 0x0A2 +#define CAVIUM_CPU_PART_THUNDERX_99XX 0x0AF #define BRCM_CPU_PART_VULCAN 0x516 @@ -94,6 +95,8 @@ #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, ARM_CPU_PART_CORTEX_A57) #define MIDR_THUNDERX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX) #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_81XX) +#define MIDR_THUNDERX_99XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX_99XX) +#define MIDR_BRCM_VULCAN MIDR_CPU_MODEL(ARM_CPU_IMP_BRCM, BRCM_CPU_PART_VULCAN) #define MIDR_QCOM_FALKOR_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, QCOM_CPU_PART_FALKOR_V1) #ifndef __ASSEMBLY__ -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74
From: Linu Cherian Cavium 99xx SMMU implementation doesn't support page 1 register space. Based on silicon id, ARM_SMMU_PAGE0_REGS_ONLY macro is set as an errata workaround. This macro when set, replaces all page 1 offsets used for EVTQ_PROD/CONS, PRIQ_PROD/CONS register access with page 0 offsets. Signed-off-by: Linu Cherian Signed-off-by: Geetha --- Documentation/arm64/silicon-errata.txt | 1 + drivers/acpi/arm64/iort.c | 14 +- drivers/iommu/arm-smmu-v3.c| 32 +++- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt index 2f66683..629e2ce 100644 --- a/Documentation/arm64/silicon-errata.txt +++ b/Documentation/arm64/silicon-errata.txt @@ -61,6 +61,7 @@ stable kernels. | Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 | | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | | Cavium | ThunderX SMMUv2 | #27704 | N/A | +| Cavium | ThunderX2 SMMUv3| #74 | N/A | || | | | | Freescale/NXP | LS2080A/LS1043A | A-008585| FSL_ERRATUM_A008585 | || | | | diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 4a5bb96..a074ce9 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -25,6 +25,7 @@ #include #include #include +#include #define IORT_TYPE_MASK(type) (1 << (type)) #define IORT_MSI_TYPE (1 << ACPI_IORT_NODE_ITS_GROUP) @@ -669,12 +670,23 @@ static void __init arm_smmu_v3_init_resources(struct resource *res, { struct acpi_iort_smmu_v3 *smmu; int num_res = 0; + u32 cpu_model; + unsigned long size = SZ_128K; /* Retrieve SMMUv3 specific data */ smmu = (struct acpi_iort_smmu_v3 *)node->node_data; + /* +* Override the size, for Cavium CN99xx implementations +* which doesn't support the page 1 SMMU register space. +*/ + cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK; + if (cpu_model == MIDR_THUNDERX_99XX || + cpu_model == MIDR_BRCM_VULCAN) + size = SZ_64K; + res[num_res].start = smmu->base_address; - res[num_res].end = smmu->base_address + SZ_128K - 1; + res[num_res].end = smmu->base_address + size - 1; res[num_res].flags = IORESOURCE_MEM; num_res++; diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index 1dcd154..ee23ccd 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -38,6 +38,7 @@ #include #include +#include #include "io-pgtable.h" @@ -176,15 +177,15 @@ #define ARM_SMMU_CMDQ_CONS 0x9c #define ARM_SMMU_EVTQ_BASE 0xa0 -#define ARM_SMMU_EVTQ_PROD 0x100a8 -#define ARM_SMMU_EVTQ_CONS 0x100ac +#define ARM_SMMU_EVTQ_PROD (page1_offset_adjust(0x100a8)) +#define ARM_SMMU_EVTQ_CONS (page1_offset_adjust(0x100ac)) #define ARM_SMMU_EVTQ_IRQ_CFG0 0xb0 #define ARM_SMMU_EVTQ_IRQ_CFG1 0xb8 #define ARM_SMMU_EVTQ_IRQ_CFG2 0xbc #define ARM_SMMU_PRIQ_BASE 0xc0 -#define ARM_SMMU_PRIQ_PROD 0x100c8 -#define ARM_SMMU_PRIQ_CONS 0x100cc +#define ARM_SMMU_PRIQ_PROD (page1_offset_adjust(0x100c8)) +#define ARM_SMMU_PRIQ_CONS (page1_offset_adjust(0x100cc)) #define ARM_SMMU_PRIQ_IRQ_CFG0 0xd0 #define ARM_SMMU_PRIQ_IRQ_CFG1 0xd8 #define ARM_SMMU_PRIQ_IRQ_CFG2 0xdc @@ -412,6 +413,10 @@ #define MSI_IOVA_BASE 0x800 #define MSI_IOVA_LENGTH0x10 +#define ARM_SMMU_PAGE0_REGS_ONLY \ + (((read_cpuid_id() & MIDR_CPU_MODEL_MASK) == MIDR_THUNDERX_99XX) \ + || ((read_cpuid_id() & MIDR_CPU_MODEL_MASK) == MIDR_BRCM_VULCAN)) + static bool disable_bypass; module_param_named(disable_bypass, disable_bypass, bool, S_IRUGO); MODULE_PARM_DESC(disable_bypass, @@ -660,6 +665,15 @@ struct arm_smmu_option_prop { { 0, NULL}, }; +static inline unsigned long page1_offset_adjust( + unsigned long off) +{ + if (!ARM_SMMU_PAGE0_REGS_ONLY) + return off; + else + return (off - SZ_64K); +} + static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom) { return container_of(dom, struct arm_smmu_domain, domain); @@ -2631,6 +2645,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev, return ret; } +static unsigned long arm_smmu_resource_size(void) +{ + if (ARM_SMMU_PAGE0_REGS_ONLY) + return SZ_64K; + else + ret
[PATCH 3/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2
From: Geetha Cavium 99xx SMMU doesn't support MSI and also doesn't have unique irq lines for gerror, eventq and cmdq-sync. This patch addresses the issue by checking if any interrupt sources are using same irq number, then they are registered as shared irqs. Signed-off-by: Geetha --- Documentation/arm64/silicon-errata.txt | 1 + drivers/iommu/arm-smmu-v3.c| 32 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt index 629e2ce..cc15f25 100644 --- a/Documentation/arm64/silicon-errata.txt +++ b/Documentation/arm64/silicon-errata.txt @@ -62,6 +62,7 @@ stable kernels. | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 | | Cavium | ThunderX SMMUv2 | #27704 | N/A | | Cavium | ThunderX2 SMMUv3| #74 | N/A | +| Cavium | ThunderX2 SMMUv3| #126| N/A | || | | | | Freescale/NXP | LS2080A/LS1043A | A-008585| FSL_ERRATUM_A008585 | || | | | diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index ee23ccd..eb55d38 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -2211,10 +2211,30 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu) devm_add_action(dev, arm_smmu_free_msis, dev); } +static int get_irq_flags(struct arm_smmu_device *smmu, int irq) +{ + int match_count = 0; + + if (irq == smmu->evtq.q.irq) + match_count++; + if (irq == smmu->cmdq.q.irq) + match_count++; + if (irq == smmu->gerr_irq) + match_count++; + if (irq == smmu->priq.q.irq) + match_count++; + + if (match_count > 1) + return IRQF_SHARED | IRQF_ONESHOT; + + return 0; +} + static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) { int ret, irq; u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN; + u32 irqflags = 0; /* Disable IRQs first */ ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL, @@ -2229,9 +2249,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) /* Request interrupt lines */ irq = smmu->evtq.q.irq; if (irq) { + irqflags = get_irq_flags(smmu, irq); ret = devm_request_threaded_irq(smmu->dev, irq, NULL, arm_smmu_evtq_thread, - IRQF_ONESHOT, + IRQF_ONESHOT | irqflags, "arm-smmu-v3-evtq", smmu); if (ret < 0) dev_warn(smmu->dev, "failed to enable evtq irq\n"); @@ -2239,8 +2260,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) irq = smmu->cmdq.q.irq; if (irq) { + irqflags = get_irq_flags(smmu, irq); ret = devm_request_irq(smmu->dev, irq, - arm_smmu_cmdq_sync_handler, 0, + arm_smmu_cmdq_sync_handler, irqflags, "arm-smmu-v3-cmdq-sync", smmu); if (ret < 0) dev_warn(smmu->dev, "failed to enable cmdq-sync irq\n"); @@ -2248,8 +2270,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) irq = smmu->gerr_irq; if (irq) { + irqflags = get_irq_flags(smmu, irq); ret = devm_request_irq(smmu->dev, irq, arm_smmu_gerror_handler, - 0, "arm-smmu-v3-gerror", smmu); + irqflags, "arm-smmu-v3-gerror", smmu); if (ret < 0) dev_warn(smmu->dev, "failed to enable gerror irq\n"); } @@ -2257,9 +2280,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) if (smmu->features & ARM_SMMU_FEAT_PRI) { irq = smmu->priq.q.irq; if (irq) { + irqflags = get_irq_flags(smmu, irq); ret = devm_request_threaded_irq(smmu->dev, irq, NULL, arm_smmu_priq_thread, - IRQF_ONESHOT, + IRQF_ONESHOT | irqflags, "arm-smmu-v3-priq", smmu); if (ret < 0) -- 1.9.1 ___ iomm
Re: [PATCH 1/3] arm64: Add MIDR values for Cavium cn99xx SoCs
On Thu, Apr 27, 2017 at 5:16 PM, Geetha sowjanya wrote: > From: Geetha > > Add MIDR values for Cavium cn99xx SoCs > > Signed-off-by: Geetha > --- > arch/arm64/include/asm/cputype.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/include/asm/cputype.h > b/arch/arm64/include/asm/cputype.h > index fc50271..066fad0 100644 > --- a/arch/arm64/include/asm/cputype.h > +++ b/arch/arm64/include/asm/cputype.h > @@ -85,6 +85,7 @@ > > #define CAVIUM_CPU_PART_THUNDERX 0x0A1 > #define CAVIUM_CPU_PART_THUNDERX_81XX 0x0A2 > +#define CAVIUM_CPU_PART_THUNDERX_99XX 0x0AF Can you please use the name CAVIUM_CPU_PART_THUNDERX2? We have used ThunderX2 consistently for this platform, having THUNDERX here would be confusing. > #define BRCM_CPU_PART_VULCAN 0x516 > > @@ -94,6 +95,8 @@ > #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, > ARM_CPU_PART_CORTEX_A57) > #define MIDR_THUNDERX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, > CAVIUM_CPU_PART_THUNDERX) > #define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, > CAVIUM_CPU_PART_THUNDERX_81XX) > +#define MIDR_THUNDERX_99XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, > CAVIUM_CPU_PART_THUNDERX_99XX) > +#define MIDR_BRCM_VULCAN MIDR_CPU_MODEL(ARM_CPU_IMP_BRCM, > BRCM_CPU_PART_VULCAN) > #define MIDR_QCOM_FALKOR_V1 MIDR_CPU_MODEL(ARM_CPU_IMP_QCOM, > QCOM_CPU_PART_FALKOR_V1) > > #ifndef __ASSEMBLY__ Thanks, JC. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Cavium ThunderX2 SMMUv3 errata workarounds
On 27.04.17 17:16:21, Geetha sowjanya wrote: > From: Geetha > > Cavium CN99xx SMMUv3 implementation has two Silicon Erratas. > 1. Errata ID #74 >SMMU register alias Page 1 is not implemented > 2. Errata ID #126 >SMMU doesnt support unique IRQ lines for gerror, eventq and cmdq-sync > > The following patchset does software workaround for these two erratas. > > This series is based on RFC patch. > https://www.spinics.net/lists/arm-kernel/msg575739.html > > As suggested by Will Deacon, code is modified to use silicon id to > enable errata#74 workaround. Can we go with the previous series [1] and: * drop the iort model numbering part, * add an enablement function that enables flags (smmu->options) depending on midr values (which replaces the macro code)? E.g.: static void acpi_smmu_enable_cavium(struct arm_smmu_device *smmu) { u32 cpu_model; if (!IS_ENABLED(CONFIG_ARM64)) return; cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK; switch (cpu_model) { case ...: case ...: break; default: /* No Cavium CN99xx SMMU v3 */ return; } smmu->options |= (ARM_SMMU_OPT_PAGE0_REGS_ONLY | ARM_SMMU_OPT_USE_SHARED_IRQS); } -Robert [1] [RFC PATCH 0/7] Cavium CN99xx SMMUv3 Errata workarounds https://marc.info/?l=linux-acpi&m=149192179623708&w=2 > > Linu Cherian (1): > iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74 > > Geetha (2): > arm64: Add MIDR values for Cavium cn99xx SoCs > iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 > > Documentation/arm64/silicon-errata.txt | 2 ++ > arch/arm64/include/asm/cputype.h | 3 ++ > drivers/acpi/arm64/iort.c | 14 +++- > drivers/iommu/arm-smmu-v3.c| 64 > +- > 4 files changed, 73 insertions(+), 10 deletions(-) > > -- > 1.9.1 > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] iommu: Remove pci.h include from trace/events/iommu.h
Hi Joerg, [auto build test WARNING on linus/master] [also build test WARNING on v4.11-rc8 next-20170426] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Joerg-Roedel/iommu-Header-file-cleanups/20170427-160734 config: tile-allmodconfig (attached as .config) compiler: tilegx-linux-gcc (GCC) 4.6.2 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=tile All warnings (new ones prefixed by >>): In file included from drivers/infiniband//hw/qedr/main.c:39:0: >> include/linux/qed/qede_roce.h:57:12: warning: 'struct pci_dev' declared >> inside parameter list [enabled by default] >> include/linux/qed/qede_roce.h:57:12: warning: its scope is only this >> definition or declaration, which is probably not what you want [enabled by >> default] >> drivers/infiniband//hw/qedr/main.c:905:2: warning: initialization from >> incompatible pointer type [enabled by default] drivers/infiniband//hw/qedr/main.c:905:2: warning: (near initialization for 'qedr_drv.add') [enabled by default] -- In file included from drivers/infiniband/hw/qedr/main.c:39:0: >> include/linux/qed/qede_roce.h:57:12: warning: 'struct pci_dev' declared >> inside parameter list [enabled by default] >> include/linux/qed/qede_roce.h:57:12: warning: its scope is only this >> definition or declaration, which is probably not what you want [enabled by >> default] drivers/infiniband/hw/qedr/main.c:905:2: warning: initialization from incompatible pointer type [enabled by default] drivers/infiniband/hw/qedr/main.c:905:2: warning: (near initialization for 'qedr_drv.add') [enabled by default] vim +57 include/linux/qed/qede_roce.h cee9fbd8 Ram Amrani 2016-10-01 41 QEDE_DOWN, cee9fbd8 Ram Amrani 2016-10-01 42 QEDE_CHANGE_ADDR, cee9fbd8 Ram Amrani 2016-10-01 43 QEDE_CLOSE cee9fbd8 Ram Amrani 2016-10-01 44 }; cee9fbd8 Ram Amrani 2016-10-01 45 cee9fbd8 Ram Amrani 2016-10-01 46 struct qede_roce_event_work { cee9fbd8 Ram Amrani 2016-10-01 47 struct list_head list; cee9fbd8 Ram Amrani 2016-10-01 48 struct work_struct work; cee9fbd8 Ram Amrani 2016-10-01 49 void *ptr; cee9fbd8 Ram Amrani 2016-10-01 50 enum qede_roce_event event; cee9fbd8 Ram Amrani 2016-10-01 51 }; cee9fbd8 Ram Amrani 2016-10-01 52 cee9fbd8 Ram Amrani 2016-10-01 53 struct qedr_driver { cee9fbd8 Ram Amrani 2016-10-01 54 unsigned char name[32]; cee9fbd8 Ram Amrani 2016-10-01 55 cee9fbd8 Ram Amrani 2016-10-01 56 struct qedr_dev* (*add)(struct qed_dev *, struct pci_dev *, cee9fbd8 Ram Amrani 2016-10-01 @57 struct net_device *); cee9fbd8 Ram Amrani 2016-10-01 58 cee9fbd8 Ram Amrani 2016-10-01 59 void (*remove)(struct qedr_dev *); cee9fbd8 Ram Amrani 2016-10-01 60 void (*notify)(struct qedr_dev *, enum qede_roce_event); cee9fbd8 Ram Amrani 2016-10-01 61 }; cee9fbd8 Ram Amrani 2016-10-01 62 cee9fbd8 Ram Amrani 2016-10-01 63 /* APIs for RoCE driver to register callback handlers, cee9fbd8 Ram Amrani 2016-10-01 64 * which will be invoked when device is added, removed, ifup, ifdown cee9fbd8 Ram Amrani 2016-10-01 65 */ :: The code at line 57 was first introduced by commit :: cee9fbd8e2e9e713cd8bf227c6492fd8854de74b qede: Add qedr framework :: TO: Ram Amrani :: CC: David S. Miller --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/2] iommu/s390: Fix IOMMU groups
From: Joerg Roedel Currently the s390 iommu driver allocates an iommu-group for every device that is added. But that is wrong, as there is only one dma-table per pci-root-bus. Make all devices behind one dma-table share one iommu-group. Signed-off-by: Joerg Roedel --- arch/s390/include/asm/pci.h | 7 +++ arch/s390/pci/pci.c | 10 +- drivers/iommu/s390-iommu.c | 43 --- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index 4e31866..045665d 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -123,6 +124,8 @@ struct zpci_dev { unsigned long iommu_pages; unsigned intnext_bit; + struct iommu_group *group; /* IOMMU group for all devices behind this zdev */ + char res_name[16]; struct zpci_bar_struct bars[PCI_BAR_COUNT]; @@ -173,6 +176,10 @@ static inline bool zdev_enabled(struct zpci_dev *zdev) int clp_enable_fh(struct zpci_dev *, u8); int clp_disable_fh(struct zpci_dev *); +/* IOMMU Interface */ +int zpci_init_iommu(struct zpci_dev *zdev); +void zpci_destroy_iommu(struct zpci_dev *zdev); + #ifdef CONFIG_PCI /* Error handling and recovery */ void zpci_event_error(void *); diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c index 364b9d8..3178e4d 100644 --- a/arch/s390/pci/pci.c +++ b/arch/s390/pci/pci.c @@ -754,6 +754,7 @@ void pcibios_remove_bus(struct pci_bus *bus) zpci_exit_slot(zdev); zpci_cleanup_bus_resources(zdev); + zpci_destroy_iommu(zdev); zpci_free_domain(zdev); spin_lock(&zpci_list_lock); @@ -825,11 +826,15 @@ int zpci_create_device(struct zpci_dev *zdev) if (rc) goto out; + rc = zpci_init_iommu(zdev); + if (rc) + goto out_free; + mutex_init(&zdev->lock); if (zdev->state == ZPCI_FN_STATE_CONFIGURED) { rc = zpci_enable_device(zdev); if (rc) - goto out_free; + goto out_iommu; } rc = zpci_scan_bus(zdev); if (rc) @@ -846,6 +851,9 @@ int zpci_create_device(struct zpci_dev *zdev) out_disable: if (zdev->state == ZPCI_FN_STATE_ONLINE) zpci_disable_device(zdev); +out_iommu: + zpci_destroy_iommu(zdev); + out_free: zpci_free_domain(zdev); out: diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c index 179e636..cad3ad0 100644 --- a/drivers/iommu/s390-iommu.c +++ b/drivers/iommu/s390-iommu.c @@ -163,22 +163,22 @@ static void s390_iommu_detach_device(struct iommu_domain *domain, } } -static int s390_iommu_add_device(struct device *dev) +static struct iommu_group *s390_iommu_device_group(struct device *dev) { - struct iommu_group *group; - int rc; + struct zpci_dev *zdev = to_pci_dev(dev)->sysdata; - group = iommu_group_get(dev); - if (!group) { - group = iommu_group_alloc(); - if (IS_ERR(group)) - return PTR_ERR(group); - } + return zdev->group; +} + +static int s390_iommu_add_device(struct device *dev) +{ + struct iommu_group *group = iommu_group_get_for_dev(dev); + if (IS_ERR(group)) + return PTR_ERR(group); - rc = iommu_group_add_device(group, dev); iommu_group_put(group); - return rc; + return 0; } static void s390_iommu_remove_device(struct device *dev) @@ -333,6 +333,26 @@ static size_t s390_iommu_unmap(struct iommu_domain *domain, return size; } +int zpci_init_iommu(struct zpci_dev *zdev) +{ + int rc = 0; + + zdev->group = iommu_group_alloc(); + + if (IS_ERR(zdev->group)) { + rc = PTR_ERR(zdev->group); + zdev->group = NULL; + } + + return rc; +} + +void zpci_destroy_iommu(struct zpci_dev *zdev) +{ + iommu_group_put(zdev->group); + zdev->group = NULL; +} + static struct iommu_ops s390_iommu_ops = { .capable = s390_iommu_capable, .domain_alloc = s390_domain_alloc, @@ -344,6 +364,7 @@ static size_t s390_iommu_unmap(struct iommu_domain *domain, .iova_to_phys = s390_iommu_iova_to_phys, .add_device = s390_iommu_add_device, .remove_device = s390_iommu_remove_device, + .device_group = s390_iommu_device_group, .pgsize_bitmap = S390_IOMMU_PGSIZES, }; -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/2] iommu/s390: Add support for iommu_device handling
From: Joerg Roedel Add support for the iommu_device_register interface to make the s390 hardware iommus visible to the iommu core and in sysfs. Signed-off-by: Joerg Roedel --- arch/s390/include/asm/pci.h | 1 + drivers/iommu/s390-iommu.c | 30 ++ 2 files changed, 31 insertions(+) diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index 045665d..8c071af 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -124,6 +124,7 @@ struct zpci_dev { unsigned long iommu_pages; unsigned intnext_bit; + struct iommu_device iommu_dev; /* IOMMU core handle */ struct iommu_group *group; /* IOMMU group for all devices behind this zdev */ char res_name[16]; diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c index cad3ad0..ec7f5e4 100644 --- a/drivers/iommu/s390-iommu.c +++ b/drivers/iommu/s390-iommu.c @@ -18,6 +18,8 @@ */ #define S390_IOMMU_PGSIZES (~0xFFFUL) +static struct iommu_ops s390_iommu_ops; + struct s390_domain { struct iommu_domain domain; struct list_headdevices; @@ -173,10 +175,13 @@ static struct iommu_group *s390_iommu_device_group(struct device *dev) static int s390_iommu_add_device(struct device *dev) { struct iommu_group *group = iommu_group_get_for_dev(dev); + struct zpci_dev *zdev = to_pci_dev(dev)->sysdata; + if (IS_ERR(group)) return PTR_ERR(group); iommu_group_put(group); + iommu_device_link(&zdev->iommu_dev, dev); return 0; } @@ -203,6 +208,7 @@ static void s390_iommu_remove_device(struct device *dev) s390_iommu_detach_device(domain, dev); } + iommu_device_unlink(&zdev->iommu_dev, dev); iommu_group_remove_device(dev); } @@ -342,13 +348,37 @@ int zpci_init_iommu(struct zpci_dev *zdev) if (IS_ERR(zdev->group)) { rc = PTR_ERR(zdev->group); zdev->group = NULL; + goto out_err; } + rc = iommu_device_sysfs_add(&zdev->iommu_dev, NULL, NULL, + "s390-iommu.%08x", zdev->fid); + if (rc) + goto out_group; + + iommu_device_set_ops(&zdev->iommu_dev, &s390_iommu_ops); + + rc = iommu_device_register(&zdev->iommu_dev); + if (rc) + goto out_sysfs; + + return 0; + +out_sysfs: + iommu_device_sysfs_remove(&zdev->iommu_dev); + +out_group: + iommu_group_put(zdev->group); + zdev->group = NULL; + +out_err: return rc; } void zpci_destroy_iommu(struct zpci_dev *zdev) { + iommu_device_unregister(&zdev->iommu_dev); + iommu_device_sysfs_remove(&zdev->iommu_dev); iommu_group_put(zdev->group); zdev->group = NULL; } -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support
Hey, here are two patches for the s390 PCI and IOMMU code. It is based on the assumption that every pci_dev that points to the same zpci_dev shares a single dma-table (and thus a single address space). If this assupmtion is true (as it looks to me from reading the code) then the iommu-group setup code in the s390 iommu driver needs to be updated. These patches do this and also add support for the iommu_device_register interface to the s390 iommu driver. Any comments and testing appreciated. Thanks, Joerg Joerg Roedel (2): iommu/s390: Fix IOMMU groups iommu/s390: Add support for iommu_device handling arch/s390/include/asm/pci.h | 8 + arch/s390/pci/pci.c | 10 ++- drivers/iommu/s390-iommu.c | 71 ++--- 3 files changed, 78 insertions(+), 11 deletions(-) -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Does a new booting kernel by "kexec -l" need to copy IR table from previous kernel?
Hi Joerg Roedel, When we run below command, the kernel message showed below confusing failed messages: sudo kexec -l /boot/vmlinuz-4.11.0-rc7 --append=root=UUID=276659dd-77f0-47f9-967c-7643c260b746 --initrd=/boot/initrd.img-4.11.0-rc7 sudo kexec -e --- [ 0.203397] DMAR-IR: Failed to copy IR table for dmar2 from previous kernel [ 0.211568] DMAR-IR: Failed to copy IR table for dmar1 from previous kernel [ 0.219738] DMAR-IR: Failed to copy IR table for dmar0 from previous kernel [ 0.227906] DMAR-IR: Failed to copy IR table for dmar3 from previous kernel --- The driver "intel_irq_remapping.c" uses "is_kdump_kernel()" API to determine whether copy IR table from previous kernel. a) When booting a kdump kernel specified by "kexec -p" (loading the new kernel for use on panic), the "is_kdump_kernel()" API is true then the driver copy IR table from previous kernel. b) While when booting a new kernel specified by "kexec -l" (loading the new kernel into current kernel) the "is_kdump_kernel()" API is false then the driver prints error message "Failed to copy IR table from previous kernel" directly. It looks like the printk is misleading and it's nothing actually failed, but just it isn't copying if the new kernel is not a kdump kernel. Found that the commit id "af3b358e48115" said: --- iommu/vt-d: Copy IR table from old kernel when in kdump mode When we are booting into a kdump kernel and find IR enabled, copy over the contents of the previous IR table so that spurious interrupts will not be target aborted. --- May I ask some questions: What are spurious interrupts? For kdump kernel can we just reinitializing IR table(as like normal kernel boot from power on) to handle the "spurious interrupts" issue instead of copying IR table from previous kernel? For booting a new kernel by "kexec -l" (my test case), do we still need to copy IR table from previous kernel to handle the "spurious interrupts" issue? Thanks! qiuxu BR qiuxu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 06/32] x86/mm: Add Secure Memory Encryption (SME) support
On Tue, Apr 18, 2017 at 04:17:27PM -0500, Tom Lendacky wrote: > Add support for Secure Memory Encryption (SME). This initial support > provides a Kconfig entry to build the SME support into the kernel and > defines the memory encryption mask that will be used in subsequent > patches to mark pages as encrypted. ... > diff --git a/arch/x86/include/asm/mem_encrypt.h > b/arch/x86/include/asm/mem_encrypt.h > new file mode 100644 > index 000..d5c4a2b > --- /dev/null > +++ b/arch/x86/include/asm/mem_encrypt.h > @@ -0,0 +1,42 @@ > +/* > + * AMD Memory Encryption Support > + * > + * Copyright (C) 2016 Advanced Micro Devices, Inc. > + * > + * Author: Tom Lendacky > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + These ifdeffery closing #endif markers look strange: > +#ifndef __X86_MEM_ENCRYPT_H__ > +#define __X86_MEM_ENCRYPT_H__ > + > +#ifndef __ASSEMBLY__ > + > +#ifdef CONFIG_AMD_MEM_ENCRYPT > + > +extern unsigned long sme_me_mask; > + > +static inline bool sme_active(void) > +{ > + return !!sme_me_mask; > +} > + > +#else/* !CONFIG_AMD_MEM_ENCRYPT */ > + > +#ifndef sme_me_mask > +#define sme_me_mask 0UL > + > +static inline bool sme_active(void) > +{ > + return false; > +} > +#endif this endif is the sme_me_mask closing one and it has sme_active() in it. Shouldn't it be: #ifndef sme_me_mask #define sme_me_mask 0UL #endif and have sme_active below it, in the !CONFIG_AMD_MEM_ENCRYPT branch? The same thing is in include/linux/mem_encrypt.h -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption
On 04/27/2017 12:25 AM, Dave Young wrote: > On 04/21/17 at 02:55pm, Dave Hansen wrote: >> On 04/18/2017 02:22 PM, Tom Lendacky wrote: >>> Add sysfs support for SME so that user-space utilities (kdump, etc.) can >>> determine if SME is active. >>> >>> A new directory will be created: >>> /sys/kernel/mm/sme/ >>> >>> And two entries within the new directory: >>> /sys/kernel/mm/sme/active >>> /sys/kernel/mm/sme/encryption_mask >> >> Why do they care, and what will they be doing with this information? > > Since kdump will copy old memory but need this to know if the old memory > was encrypted or not. With this sysfs file we can know the previous SME > status and pass to kdump kernel as like a kernel param. > > Tom, have you got chance to try if it works or not? What will the kdump kernel do with it though? We kexec() into that kernel so the SME keys will all be the same, right? So, will the kdump kernel be just setting the encryption bit in the PTE so it can copy the old plaintext out? Why do we need both 'active' and 'encryption_mask'? How could it be that the hardware-enumerated 'encryption_mask' changes across a kexec()? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Does a new booting kernel by "kexec -l" need to copy IR table from previous kernel?
On Thu, Apr 27, 2017 at 03:34:06PM +, Zhuo, Qiuxu wrote: > It looks like the printk is misleading and it’s nothing actually > failed, but just it isn’t copying if the new kernel is not a kdump > kernel. Yes, that is true. But the messages are harmless and you are safe to ignore them in your usecase. We only care about the kdump case when copying the old IR and DMAR tables into the new kernel, because in the kdump case the old kernel crashed and left devices in an undefined state, so that they might still send DMA and IRQ requests to the new kernel, corrupting data or causing spurious/blocked irqs. Blocked IRQs or DMA requests might even break devices so that the new kernel can't initialize them anymore. That is why we want to make sure, that no spurious IRQ or DMA requests are blocked when the new kernel initializes the IOMMU. In the non-kdump case we can assume that the old kernel put the devices into a defined state where they are not sending spurious requests. >For kdump kernel can we just reinitializing IR table(as like normal > kernel boot from power on) to handle the “spurious interrupts” issue instead > of > copying IR table from previous kernel? > >For booting a new kernel by “kexec -l” (my test case), do we still need > to copy IR table from previous kernel to handle the “spurious interrupts” > issue? As I said, for your case there is no need to do the copying and you can ignore the messages. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 09/32] x86/mm: Provide general kernel support for memory encryption
On Tue, Apr 18, 2017 at 04:17:54PM -0500, Tom Lendacky wrote: > Changes to the existing page table macros will allow the SME support to > be enabled in a simple fashion with minimal changes to files that use these > macros. Since the memory encryption mask will now be part of the regular > pagetable macros, we introduce two new macros (_PAGE_TABLE_NOENC and > _KERNPG_TABLE_NOENC) to allow for early pagetable creation/initialization > without the encryption mask before SME becomes active. Two new pgprot() > macros are defined to allow setting or clearing the page encryption mask. ... > @@ -55,7 +57,7 @@ static inline void copy_user_page(void *to, void *from, > unsigned long vaddr, > __phys_addr_symbol(__phys_reloc_hide((unsigned long)(x))) > > #ifndef __va > -#define __va(x) ((void *)((unsigned > long)(x)+PAGE_OFFSET)) > +#define __va(x) ((void *)(__sme_clr(x) + PAGE_OFFSET)) > #endif > > #define __boot_va(x) __va(x) > diff --git a/arch/x86/include/asm/page_types.h > b/arch/x86/include/asm/page_types.h > index 7bd0099..fead0a5 100644 > --- a/arch/x86/include/asm/page_types.h > +++ b/arch/x86/include/asm/page_types.h > @@ -15,7 +15,7 @@ > #define PUD_PAGE_SIZE(_AC(1, UL) << PUD_SHIFT) > #define PUD_PAGE_MASK(~(PUD_PAGE_SIZE-1)) > > -#define __PHYSICAL_MASK ((phys_addr_t)((1ULL << > __PHYSICAL_MASK_SHIFT) - 1)) > +#define __PHYSICAL_MASK ((phys_addr_t)(__sme_clr((1ULL << > __PHYSICAL_MASK_SHIFT) - 1))) That looks strange: poking SME mask hole into a mask...? > #define __VIRTUAL_MASK ((1UL << __VIRTUAL_MASK_SHIFT) - 1) > > /* Cast *PAGE_MASK to a signed type so that it is sign-extended if -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Does a new booting kernel by "kexec -l" need to copy IR table from previous kernel?
Hi Joerg, On Thu, Apr 27, 2017 at 06:12:38PM +0200, j...@8bytes.org wrote: > On Thu, Apr 27, 2017 at 03:34:06PM +, Zhuo, Qiuxu wrote: > > It looks like the printk is misleading and it’s nothing actually > > failed, but just it isn’t copying if the new kernel is not a kdump > > kernel. > > Yes, that is true. But the messages are harmless and you are safe to > ignore them in your usecase. We only care about the kdump case when Looks like maybe sanitizing the message to make it less disturbing should be enough :-) > copying the old IR and DMAR tables into the new kernel, because in the > kdump case the old kernel crashed and left devices in an undefined > state, so that they might still send DMA and IRQ requests to the new > kernel, corrupting data or causing spurious/blocked irqs. Blocked IRQs > or DMA requests might even break devices so that the new kernel can't > initialize them anymore. Cheers, Ashok ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/3] Cavium ThunderX2 SMMUv3 errata workarounds
On Thu, Apr 27, 2017 at 7:09 PM, Robert Richter wrote: > On 27.04.17 17:16:21, Geetha sowjanya wrote: >> From: Geetha >> >> Cavium CN99xx SMMUv3 implementation has two Silicon Erratas. >> 1. Errata ID #74 >>SMMU register alias Page 1 is not implemented >> 2. Errata ID #126 >>SMMU doesnt support unique IRQ lines for gerror, eventq and cmdq-sync >> >> The following patchset does software workaround for these two erratas. >> >> This series is based on RFC patch. >> https://www.spinics.net/lists/arm-kernel/msg575739.html >> >> As suggested by Will Deacon, code is modified to use silicon id to >> enable errata#74 workaround. > > Can we go with the previous series [1] and: > > * drop the iort model numbering part, > > * add an enablement function that enables flags (smmu->options) >depending on midr values (which replaces the macro code)? I don't see how it is efficient and consistent, if we take data from DT for non-ACPI mode and read CPU ID from MIDR for ACPI mode. Thanks, Sunil. > > E.g.: > > static void acpi_smmu_enable_cavium(struct arm_smmu_device *smmu) > { > u32 cpu_model; > > if (!IS_ENABLED(CONFIG_ARM64)) > return; > > cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK; > switch (cpu_model) { > case ...: > case ...: > break; > default: > /* No Cavium CN99xx SMMU v3 */ > return; > } > > smmu->options |= (ARM_SMMU_OPT_PAGE0_REGS_ONLY | > ARM_SMMU_OPT_USE_SHARED_IRQS); > } > > -Robert > > > [1] [RFC PATCH 0/7] Cavium CN99xx SMMUv3 Errata workarounds > https://marc.info/?l=linux-acpi&m=149192179623708&w=2 > >> >> Linu Cherian (1): >> iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74 >> >> Geetha (2): >> arm64: Add MIDR values for Cavium cn99xx SoCs >> iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #126 >> >> Documentation/arm64/silicon-errata.txt | 2 ++ >> arch/arm64/include/asm/cputype.h | 3 ++ >> drivers/acpi/arm64/iort.c | 14 +++- >> drivers/iommu/arm-smmu-v3.c| 64 >> +- >> 4 files changed, 73 insertions(+), 10 deletions(-) >> >> -- >> 1.9.1 >> > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74
On Thu, Apr 27, 2017 at 05:16:23PM +0530, Geetha sowjanya wrote: > + /* > + * Override the size, for Cavium CN99xx implementations > + * which doesn't support the page 1 SMMU register space. > + */ > + cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK; > + if (cpu_model == MIDR_THUNDERX_99XX || > + cpu_model == MIDR_BRCM_VULCAN) > + size = SZ_64K; If you're trying to identify an SMMU erratum, identify the SMMU, not the CPU it happens to be paired with this time. There are ID registers in the SMMU you can use to do so. NAK to using the CPU ID here. Mark. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/3] iommu/arm-smmu-v3: Add workaround for Cavium ThunderX2 erratum #74
On Thu, Apr 27, 2017 at 05:42:37PM +0100, Mark Rutland wrote: > On Thu, Apr 27, 2017 at 05:16:23PM +0530, Geetha sowjanya wrote: > > + /* > > +* Override the size, for Cavium CN99xx implementations > > +* which doesn't support the page 1 SMMU register space. > > +*/ > > + cpu_model = read_cpuid_id() & MIDR_CPU_MODEL_MASK; > > + if (cpu_model == MIDR_THUNDERX_99XX || > > + cpu_model == MIDR_BRCM_VULCAN) > > + size = SZ_64K; > > If you're trying to identify an SMMU erratum, identify the SMMU, not the > CPU it happens to be paired with this time. > > There are ID registers in the SMMU you can use to do so. > > NAK to using the CPU ID here. Agreed. I had some off-list discussion with Geetha where we agreed to use the "silicon ID", which I assumed was the SMMU IIDR register. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support
On Thu, 27 Apr 2017 17:28:23 +0200 Joerg Roedel wrote: > Hey, > > here are two patches for the s390 PCI and IOMMU code. It is > based on the assumption that every pci_dev that points to > the same zpci_dev shares a single dma-table (and thus a > single address space). Well, there is a separate zpci_dev for each pci_dev on s390, and each of those has its own separate dma-table (thus not shared). > > If this assupmtion is true (as it looks to me from reading > the code) then the iommu-group setup code in the s390 iommu > driver needs to be updated. Given this "separate zpci_dev for each pci_dev" situation, I don't see what this update actually changes, compared to the previous code, see also my comments to that patch. > > These patches do this and also add support for the > iommu_device_register interface to the s390 iommu driver. > > Any comments and testing appreciated. > > Thanks, > > Joerg > > Joerg Roedel (2): > iommu/s390: Fix IOMMU groups > iommu/s390: Add support for iommu_device handling > > arch/s390/include/asm/pci.h | 8 + > arch/s390/pci/pci.c | 10 ++- > drivers/iommu/s390-iommu.c | 71 > ++--- > 3 files changed, 78 insertions(+), 11 deletions(-) > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
On Thu, 27 Apr 2017 17:28:24 +0200 Joerg Roedel wrote: > From: Joerg Roedel > > Currently the s390 iommu driver allocates an iommu-group for > every device that is added. But that is wrong, as there is > only one dma-table per pci-root-bus. Make all devices behind > one dma-table share one iommu-group. On s390, each PCI device has its own zpci_dev structure, and also its own DMA address space. Even with this patch, you'll still allocate an iommu_group for every device that is added, see below, so I do not really see the benefit of this (but my knowledge got a little rusty, so I may be missing something). The only reason why we allow the theoretical option to attach more than one device to the same IOMMU domain (and thus address space), is because it was a requirement by you at that time (IIRC). We have no use-case for this, and even in this theoretical scenario you would still have separate zpci_dev structures for the affected devices that share the same IOMMU domain (DMA address space), and thus also separate IOMMU groups, at least after this patch. Before this patch, you could in theory have different PCI devices in the same IOMMU group, by having iommu_group_get() in s390_iommu_add_device() return a group != NULL. With this patch, you will enforce that a new iommu_group is allocated for every device, which would be the contrary of what the description says. > > Signed-off-by: Joerg Roedel > --- > arch/s390/include/asm/pci.h | 7 +++ > arch/s390/pci/pci.c | 10 +- > drivers/iommu/s390-iommu.c | 43 --- > 3 files changed, 48 insertions(+), 12 deletions(-) > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h > index 4e31866..045665d 100644 > --- a/arch/s390/include/asm/pci.h > +++ b/arch/s390/include/asm/pci.h > @@ -8,6 +8,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -123,6 +124,8 @@ struct zpci_dev { > unsigned long iommu_pages; > unsigned intnext_bit; > > + struct iommu_group *group; /* IOMMU group for all devices behind > this zdev */ There is always only one device behind a zpci_dev, so this comment makes no sense. > + > char res_name[16]; > struct zpci_bar_struct bars[PCI_BAR_COUNT]; > > @@ -173,6 +176,10 @@ static inline bool zdev_enabled(struct zpci_dev *zdev) > int clp_enable_fh(struct zpci_dev *, u8); > int clp_disable_fh(struct zpci_dev *); > > +/* IOMMU Interface */ > +int zpci_init_iommu(struct zpci_dev *zdev); > +void zpci_destroy_iommu(struct zpci_dev *zdev); > + > #ifdef CONFIG_PCI > /* Error handling and recovery */ > void zpci_event_error(void *); > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c > index 364b9d8..3178e4d 100644 > --- a/arch/s390/pci/pci.c > +++ b/arch/s390/pci/pci.c > @@ -754,6 +754,7 @@ void pcibios_remove_bus(struct pci_bus *bus) > > zpci_exit_slot(zdev); > zpci_cleanup_bus_resources(zdev); > + zpci_destroy_iommu(zdev); > zpci_free_domain(zdev); > > spin_lock(&zpci_list_lock); > @@ -825,11 +826,15 @@ int zpci_create_device(struct zpci_dev *zdev) > if (rc) > goto out; > > + rc = zpci_init_iommu(zdev); > + if (rc) > + goto out_free; This will get called for every device that is added, and every device will get its own zpci_dev, so this will still result in allocating an IOMMU group for every device. > + > mutex_init(&zdev->lock); > if (zdev->state == ZPCI_FN_STATE_CONFIGURED) { > rc = zpci_enable_device(zdev); > if (rc) > - goto out_free; > + goto out_iommu; > } > rc = zpci_scan_bus(zdev); > if (rc) > @@ -846,6 +851,9 @@ int zpci_create_device(struct zpci_dev *zdev) > out_disable: > if (zdev->state == ZPCI_FN_STATE_ONLINE) > zpci_disable_device(zdev); > +out_iommu: > + zpci_destroy_iommu(zdev); > + > out_free: > zpci_free_domain(zdev); > out: > diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c > index 179e636..cad3ad0 100644 > --- a/drivers/iommu/s390-iommu.c > +++ b/drivers/iommu/s390-iommu.c > @@ -163,22 +163,22 @@ static void s390_iommu_detach_device(struct > iommu_domain *domain, > } > } > > -static int s390_iommu_add_device(struct device *dev) > +static struct iommu_group *s390_iommu_device_group(struct device *dev) > { > - struct iommu_group *group; > - int rc; > + struct zpci_dev *zdev = to_pci_dev(dev)->sysdata; > > - group = iommu_group_get(dev); > - if (!group) { > - group = iommu_group_alloc(); > - if (IS_ERR(group)) > - return PTR_ERR(group); > - } > + return zdev->group; > +} > + > +static int s390_iommu_add_device(struct device *dev) > +{ > + struct iommu_group *group = iommu_group_get_for_dev(dev); > + if (IS_ERR(group)) > + return PTR_ERR(gro
Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support
Hi Gerald, thanks for your reply. I have some more questions, please see below. On Thu, Apr 27, 2017 at 08:10:18PM +0200, Gerald Schaefer wrote: > Well, there is a separate zpci_dev for each pci_dev on s390, > and each of those has its own separate dma-table (thus not shared). Is that true for all functions of a PCIe card, so does every function of a device has its own zpci_dev structure and thus its own DMA-table? My assumption came from the fact that the zpci_dev is read from pci_dev->sysdata, which is propagated there from the pci_bridge through the pci_root_bus structures. > Given this "separate zpci_dev for each pci_dev" situation, I don't > see what this update actually changes, compared to the previous code, > see also my comments to that patch. The add_device call-back is invoked for every function of a pci-device, because each function gets its own pci_dev structure. Also we usually group all functions of a PCI-device together into one iommu-group, because we don't trust that the device isolates its functions from each other. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
On Thu, Apr 27, 2017 at 08:11:42PM +0200, Gerald Schaefer wrote: > > +void zpci_destroy_iommu(struct zpci_dev *zdev) > > +{ > > + iommu_group_put(zdev->group); > > + zdev->group = NULL; > > +} > > While the rest of this patch doesn't seem to make much of a difference to > the current behavior, I'm wondering where this extra iommu_group_put() > comes from. It either was erroneously missing before this patch, or it > is erroneously introduced by this patch. This is the way to free an iommu-group. It was missing before probably because it was unclear whether the add_device function allocated a group or not. So there was no way to know if it needs to be put again in the remove_device function. With this patch the iommu-group is explicitly allocated when the zpci_dev is created and destroyed again with it. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 31/32] x86: Add sysfs support for Secure Memory Encryption
On 04/27/17 at 08:52am, Dave Hansen wrote: > On 04/27/2017 12:25 AM, Dave Young wrote: > > On 04/21/17 at 02:55pm, Dave Hansen wrote: > >> On 04/18/2017 02:22 PM, Tom Lendacky wrote: > >>> Add sysfs support for SME so that user-space utilities (kdump, etc.) can > >>> determine if SME is active. > >>> > >>> A new directory will be created: > >>> /sys/kernel/mm/sme/ > >>> > >>> And two entries within the new directory: > >>> /sys/kernel/mm/sme/active > >>> /sys/kernel/mm/sme/encryption_mask > >> > >> Why do they care, and what will they be doing with this information? > > > > Since kdump will copy old memory but need this to know if the old memory > > was encrypted or not. With this sysfs file we can know the previous SME > > status and pass to kdump kernel as like a kernel param. > > > > Tom, have you got chance to try if it works or not? > > What will the kdump kernel do with it though? We kexec() into that > kernel so the SME keys will all be the same, right? So, will the kdump > kernel be just setting the encryption bit in the PTE so it can copy the > old plaintext out? I assume it is for active -> non active case, the new boot need to know the old memory is encrypted. But I think I did not read all the patches I may miss things. > > Why do we need both 'active' and 'encryption_mask'? How could it be > that the hardware-enumerated 'encryption_mask' changes across a kexec()? Leave this question to Tom.. Thanks Dave ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 02/20] intel_iommu: exposed extended-context mode to guest
On 2017年04月27日 18:32, Peter Xu wrote: > On Wed, Apr 26, 2017 at 06:06:32PM +0800, Liu, Yi L wrote: >> VT-d implementations reporting PASID or PRS fields as "Set", must also >> report ecap.ECS as "Set". Extended-Context is required for SVM. >> >> When ECS is reported, intel iommu driver would initiate extended root entry >> and extended context entry, and also PASID table if there is any SVM capable >> device. >> >> Signed-off-by: Liu, Yi L >> --- >> hw/i386/intel_iommu.c | 131 >> +++-- >> hw/i386/intel_iommu_internal.h | 9 +++ >> include/hw/i386/intel_iommu.h | 2 +- >> 3 files changed, 97 insertions(+), 45 deletions(-) >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 400d0d1..bf98fa5 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -497,6 +497,11 @@ static inline bool vtd_root_entry_present(VTDRootEntry >> *root) >> return root->val & VTD_ROOT_ENTRY_P; >> } >> >> +static inline bool vtd_root_entry_upper_present(VTDRootEntry *root) >> +{ >> +return root->rsvd & VTD_ROOT_ENTRY_P; >> +} >> + >> static int vtd_get_root_entry(IntelIOMMUState *s, uint8_t index, >>VTDRootEntry *re) >> { >> @@ -509,6 +514,9 @@ static int vtd_get_root_entry(IntelIOMMUState *s, >> uint8_t index, >> return -VTD_FR_ROOT_TABLE_INV; >> } >> re->val = le64_to_cpu(re->val); >> +if (s->ecs) { >> +re->rsvd = le64_to_cpu(re->rsvd); >> +} > > I feel it slightly hacky to play with re->rsvd. How about: > > union VTDRootEntry { > struct { > uint64_t val; > uint64_t rsvd; > } base; > struct { > uint64_t ext_lo; > uint64_t ext_hi; > } extended; > }; > > (Or any better way that can get rid of rsvd...) > > Even: > > struct VTDRootEntry { > union { > struct { > uint64_t val; > uint64_t rsvd; > } base; > struct { > uint64_t ext_lo; > uint64_t ext_hi; > } extended; > } data; > bool extended; > }; > > Then we read the entry into data, and setup extended bit. A benefit of > it is that we may avoid passing around IntelIOMMUState everywhere to > know whether we are using extended context entries. > >> return 0; >> } >> >> @@ -517,19 +525,30 @@ static inline bool >> vtd_context_entry_present(VTDContextEntry *context) >> return context->lo & VTD_CONTEXT_ENTRY_P; >> } >> >> -static int vtd_get_context_entry_from_root(VTDRootEntry *root, uint8_t >> index, >> - VTDContextEntry *ce) >> +static int vtd_get_context_entry_from_root(IntelIOMMUState *s, >> + VTDRootEntry *root, uint8_t index, VTDContextEntry *ce) >> { >> -dma_addr_t addr; >> +dma_addr_t addr, ce_size; >> >> /* we have checked that root entry is present */ >> -addr = (root->val & VTD_ROOT_ENTRY_CTP) + index * sizeof(*ce); >> -if (dma_memory_read(&address_space_memory, addr, ce, sizeof(*ce))) { >> +ce_size = (s->ecs) ? (2 * sizeof(*ce)) : (sizeof(*ce)); >> +addr = (s->ecs && (index > 0x7f)) ? >> + ((root->rsvd & VTD_ROOT_ENTRY_CTP) + (index - 0x80) * ce_size) : >> + ((root->val & VTD_ROOT_ENTRY_CTP) + index * ce_size); >> + >> +if (dma_memory_read(&address_space_memory, addr, ce, ce_size)) { >> trace_vtd_re_invalid(root->rsvd, root->val); >> return -VTD_FR_CONTEXT_TABLE_INV; >> } >> -ce->lo = le64_to_cpu(ce->lo); >> -ce->hi = le64_to_cpu(ce->hi); >> + >> +ce[0].lo = le64_to_cpu(ce[0].lo); >> +ce[0].hi = le64_to_cpu(ce[0].hi); > > Again, I feel this even hackier. :) > > I would slightly prefer to play the same union trick to context > entries, just like what I proposed to the root entries above... > >> + >> +if (s->ecs) { >> +ce[1].lo = le64_to_cpu(ce[1].lo); >> +ce[1].hi = le64_to_cpu(ce[1].hi); >> +} >> + >> return 0; >> } >> >> @@ -595,9 +614,11 @@ static inline uint32_t >> vtd_get_agaw_from_context_entry(VTDContextEntry *ce) >> return 30 + (ce->hi & VTD_CONTEXT_ENTRY_AW) * 9; >> } >> >> -static inline uint32_t vtd_ce_get_type(VTDContextEntry *ce) >> +static inline uint32_t vtd_ce_get_type(IntelIOMMUState *s, >> + VTDContextEntry *ce) >> { >> -return ce->lo & VTD_CONTEXT_ENTRY_TT; >> +return s->ecs ? (ce->lo & VTD_CONTEXT_ENTRY_TT) : >> +(ce->lo & VTD_EXT_CONTEXT_ENTRY_TT); >> } >> >> static inline uint64_t vtd_iova_limit(VTDContextEntry *ce) >> @@ -842,16 +863,20 @@ static int vtd_dev_to_context_entry(IntelIOMMUState >> *s, uint8_t bus_num, >> return ret_fr; >> } >> >> -if (!vtd_root_entry_present(&re)) { >> +if (!vtd_root_entry_present(&re) || >> +(s->ecs && (devfn > 0x7f) && (!vtd_root_entry_upper_present(&re >> { >> /* Not
Re: [RFC PATCH 09/20] Memory: introduce iommu_ops->record_device
On 2017年04月26日 18:06, Liu, Yi L wrote: > With vIOMMU exposed to guest, vIOMMU emulator needs to do translation > between host and guest. e.g. a device-selective TLB flush, vIOMMU > emulator needs to replace guest SID with host SID so that to limit > the invalidation. This patch introduces a new callback > iommu_ops->record_device() to notify vIOMMU emulator to record necessary > information about the assigned device. This patch is to prepare to translate guest sbdf to host sbdf. Alex: Could we add a new vfio API to do such translation? This will be more straight forward than storing host sbdf in the vIOMMU device model. > > Signed-off-by: Liu, Yi L > --- > include/exec/memory.h | 11 +++ > memory.c | 12 > 2 files changed, 23 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 7bd13ab..49087ef 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -203,6 +203,8 @@ struct MemoryRegionIOMMUOps { > IOMMUNotifierFlag new_flags); > /* Set this up to provide customized IOMMU replay function */ > void (*replay)(MemoryRegion *iommu, IOMMUNotifier *notifier); > +void (*record_device)(MemoryRegion *iommu, > + void *device_info); > }; > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -708,6 +710,15 @@ void memory_region_notify_iommu(MemoryRegion *mr, > void memory_region_notify_one(IOMMUNotifier *notifier, >IOMMUTLBEntry *entry); > > +/* > + * memory_region_notify_device_record: notify IOMMU to record assign > + * device. > + * @mr: the memory region to notify > + * @ device_info: device information > + */ > +void memory_region_notify_device_record(MemoryRegion *mr, > +void *info); > + > /** > * memory_region_register_iommu_notifier: register a notifier for changes to > * IOMMU translation entries. > diff --git a/memory.c b/memory.c > index 0728e62..45ef069 100644 > --- a/memory.c > +++ b/memory.c > @@ -1600,6 +1600,18 @@ static void > memory_region_update_iommu_notify_flags(MemoryRegion *mr) > mr->iommu_notify_flags = flags; > } > > +void memory_region_notify_device_record(MemoryRegion *mr, > +void *info) > +{ > +assert(memory_region_is_iommu(mr)); > + > +if (mr->iommu_ops->record_device) { > +mr->iommu_ops->record_device(mr, info); > +} > + > +return; > +} > + > void memory_region_register_iommu_notifier(MemoryRegion *mr, > IOMMUNotifier *n) > { > ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu