Re: [PATCH] ARM: Qualify enabling of swiotlb_init()
On Mon, Mar 29, 2021 at 12:30:42PM -0700, Florian Fainelli wrote: > Should I toss this in Russell's patch tracker or do you need me to make > some changes to the patch? Due to all the other changes in this area I don't think anything but the swiotlb tree makes much sense here. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
> From: Tian, Kevin > Sent: Tuesday, March 30, 2021 10:24 AM > > > From: Jason Gunthorpe > > Sent: Tuesday, March 30, 2021 12:32 AM > > > In terms of usage for guest SVA, an ioasid_set is mostly tied to a host > > > mm, > > > the use case is as the following: > > > > From that doc: > > > > It is imperative to enforce > > VM-IOASID ownership such that a malicious guest cannot target DMA > > traffic outside its own IOASIDs, or free an active IOASID that belongs > > to another VM. > > > > Huh? > > > > Security in a PASID world comes from the IOMMU blocking access to the > > PASID except from approved PCI-ID's. If a VF/PF is assigned to a guest > > then that guest can cause the device to issue any PASID by having > > complete control and the vIOMMU is supposed to tell the real IOMMU > > what PASID's the device is alowed to access. > > > > If a device is sharing a single PCI function with different security > > contexts (eg vfio mdev) then the device itself is responsible to > > ensure that only the secure interface can program a PASID and a less > > secure context can never self-enroll. > > > > Here the mdev driver would have to consule with the vIOMMU to ensure > > the mdev device is allowed to access the PASID - is that what this > > set stuff is about? > > > > If yes, it is backwards. The MDEV is the thing doing the security, the > > MDEV should have the list of allowed PASID's and a single PASID > > created under /dev/ioasid should be loaded into MDEV with some 'Ok you > > can use PASID xyz from FD abc' command. > > > > The 'set' is per-VM. Once the mdev is assigned to a VM, all valid PASID's > in the set of that VM are considered legitimate on this mdev. The mdev > driver will mediate guest operations which program PASID to the backend > context and load the PASID only if it is within the 'set' (i.e. already > allocated through /dev/ioasid). This prevents a malicious VM from attacking > others. Though it's not mdev which directly maintaining the list of allowed > PASID's, the effect is the same in concept. > One correction. The mdev should still construct the list of allowed PASID's as you said (by listening to IOASID_BIND/UNBIND event), in addition to the ioasid set maintained per VM (updated when a PASID is allocated/freed). The per-VM set is required for inter-VM isolation (verified when a pgtable is bound to the mdev/PASID), while the mdev's own list is necessary for intra-VM isolation when multiple mdevs are assigned to the same VM (verified before loading a PASID to the mdev). This series just handles the general part i.e. per-VM ioasid set and leaves the mdev's own list to be managed by specific mdev driver which listens to various IOASID events). Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
> From: Jason Gunthorpe > Sent: Tuesday, March 30, 2021 12:32 AM > > In terms of usage for guest SVA, an ioasid_set is mostly tied to a host mm, > > the use case is as the following: > > From that doc: > > It is imperative to enforce > VM-IOASID ownership such that a malicious guest cannot target DMA > traffic outside its own IOASIDs, or free an active IOASID that belongs > to another VM. > > Huh? > > Security in a PASID world comes from the IOMMU blocking access to the > PASID except from approved PCI-ID's. If a VF/PF is assigned to a guest > then that guest can cause the device to issue any PASID by having > complete control and the vIOMMU is supposed to tell the real IOMMU > what PASID's the device is alowed to access. > > If a device is sharing a single PCI function with different security > contexts (eg vfio mdev) then the device itself is responsible to > ensure that only the secure interface can program a PASID and a less > secure context can never self-enroll. > > Here the mdev driver would have to consule with the vIOMMU to ensure > the mdev device is allowed to access the PASID - is that what this > set stuff is about? > > If yes, it is backwards. The MDEV is the thing doing the security, the > MDEV should have the list of allowed PASID's and a single PASID > created under /dev/ioasid should be loaded into MDEV with some 'Ok you > can use PASID xyz from FD abc' command. > The 'set' is per-VM. Once the mdev is assigned to a VM, all valid PASID's in the set of that VM are considered legitimate on this mdev. The mdev driver will mediate guest operations which program PASID to the backend context and load the PASID only if it is within the 'set' (i.e. already allocated through /dev/ioasid). This prevents a malicious VM from attacking others. Though it's not mdev which directly maintaining the list of allowed PASID's, the effect is the same in concept. Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/1] iommu/vt-d: Report right snoop capability when using FL for IOVA
The Intel VT-d driver checks wrong register to report snoop capablility
when using first level page table for GPA to HPA translation. This might
lead the IOMMU driver to say that it supports snooping control, but in
reality, it does not. Fix this by always setting PASID-table-entry.PGSNP
whenever a pasid entry is setting up for GPA to HPA translation so that
the IOMMU driver could report snoop capability as long as it runs in the
scalable mode.
Fixes: b802d070a52a1 ("iommu/vt-d: Use iova over first level")
Suggested-by: Rajesh Sankaran
Suggested-by: Kevin Tian
Suggested-by: Ashok Raj
Signed-off-by: Lu Baolu
---
drivers/iommu/intel/pasid.h | 1 +
drivers/iommu/intel/iommu.c | 11 ++-
drivers/iommu/intel/pasid.c | 16
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 079534fcf55d..5ff61c3d401f 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -48,6 +48,7 @@
*/
#define PASID_FLAG_SUPERVISOR_MODE BIT(0)
#define PASID_FLAG_NESTED BIT(1)
+#define PASID_FLAG_PAGE_SNOOP BIT(2)
/*
* The PASID_FLAG_FL5LP flag Indicates using 5-level paging for first-
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7354f9ce47d8..deaa87ad4e5f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -657,7 +657,14 @@ static int domain_update_iommu_snooping(struct intel_iommu
*skip)
rcu_read_lock();
for_each_active_iommu(iommu, drhd) {
if (iommu != skip) {
- if (!ecap_sc_support(iommu->ecap)) {
+ /*
+* If the hardware is operating in the scalable mode,
+* the snooping control is always supported since we
+* always set PASID-table-entry.PGSNP bit if the domain
+* is managed outside (UNMANAGED).
+*/
+ if (!sm_supported(iommu) &&
+ !ecap_sc_support(iommu->ecap)) {
ret = 0;
break;
}
@@ -2516,6 +2523,8 @@ static int domain_setup_first_level(struct intel_iommu
*iommu,
flags |= PASID_FLAG_SUPERVISOR_MODE;
if (level == 5)
flags |= PASID_FLAG_FL5LP;
+ if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
+ flags |= PASID_FLAG_PAGE_SNOOP;
return intel_pasid_setup_first_level(iommu, dev, (pgd_t *)pgd, pasid,
domain->iommu_did[iommu->seq_id],
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c896aef7db40..b901909da79e 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -425,6 +425,16 @@ static inline void pasid_set_page_snoop(struct pasid_entry
*pe, bool value)
pasid_set_bits(>val[1], 1 << 23, value << 23);
}
+/*
+ * Setup the Page Snoop (PGSNP) field (Bit 88) of a scalable mode
+ * PASID entry.
+ */
+static inline void
+pasid_set_pgsnp(struct pasid_entry *pe)
+{
+ pasid_set_bits(>val[1], 1ULL << 24, 1ULL << 24);
+}
+
/*
* Setup the First Level Page table Pointer field (Bit 140~191)
* of a scalable mode PASID entry.
@@ -599,6 +609,9 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
}
}
+ if (flags & PASID_FLAG_PAGE_SNOOP)
+ pasid_set_pgsnp(pte);
+
pasid_set_domain_id(pte, did);
pasid_set_address_width(pte, iommu->agaw);
pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
@@ -677,6 +690,9 @@ int intel_pasid_setup_second_level(struct intel_iommu
*iommu,
pasid_set_fault_enable(pte);
pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+ if (domain->domain.type == IOMMU_DOMAIN_UNMANAGED)
+ pasid_set_pgsnp(pte);
+
/*
* Since it is a second level only translation setup, we should
* set SRE bit as well (addresses are expected to be GPAs).
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V2 13/13] genirq/msi: Provide helpers to return Linux IRQ/dev_msi hw IRQ number
Hi Marc,
On 3/26/2021 6:28 PM, Marc Zyngier wrote:
On Fri, 26 Mar 2021 01:02:43 +,
"Dey, Megha" wrote:
Hi Marc,
On 3/25/2021 10:53 AM, Marc Zyngier wrote:
On Fri, 26 Feb 2021 20:11:17 +,
Megha Dey wrote:
From: Dave Jiang
Add new helpers to get the Linux IRQ number and device specific index
for given device-relative vector so that the drivers don't need to
allocate their own arrays to keep track of the vectors and hwirq for
the multi vector device MSI case.
Reviewed-by: Tony Luck
Signed-off-by: Dave Jiang
Signed-off-by: Megha Dey
---
include/linux/msi.h | 2 ++
kernel/irq/msi.c| 44
2 files changed, 46 insertions(+)
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 24abec0..d60a6ba 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -451,6 +451,8 @@ struct irq_domain *platform_msi_create_irq_domain(struct
fwnode_handle *fwnode,
int platform_msi_domain_alloc_irqs(struct device *dev, unsigned int nvec,
irq_write_msi_msg_t write_msi_msg);
void platform_msi_domain_free_irqs(struct device *dev);
+int msi_irq_vector(struct device *dev, unsigned int nr);
+int dev_msi_hwirq(struct device *dev, unsigned int nr);
/* When an MSI domain is used as an intermediate domain */
int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev,
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 047b59d..f2a8f55 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -581,4 +581,48 @@ struct msi_domain_info *msi_get_domain_info(struct
irq_domain *domain)
return (struct msi_domain_info *)domain->host_data;
}
+/**
+ * msi_irq_vector - Get the Linux IRQ number of a device vector
+ * @dev: device to operate on
+ * @nr: device-relative interrupt vector index (0-based).
+ *
+ * Returns the Linux IRQ number of a device vector.
+ */
+int msi_irq_vector(struct device *dev, unsigned int nr)
+{
+ struct msi_desc *entry;
+ int i = 0;
+
+ for_each_msi_entry(entry, dev) {
+ if (i == nr)
+ return entry->irq;
+ i++;
This obviously doesn't work with Multi-MSI, does it?
This API is only for devices that support device MSI interrupts. They
follow MSI-x format and don't support multi MSI (part of MSI).
Not sure if I am missing something here, can you please let me know?
Nothing in the prototype of the function indicates this limitation,
nor does the documentation. And I'm not sure why you should exclude
part of the MSI functionality here. It can't be for performance
reason, so you might as well make sure this works for all the MSI
variants:
int msi_irq_vector(struct device *dev, unsigned int nr)
{
struct msi_desc *entry;
int irq, index = 0;
for_each_msi_vector(entry, irq, dev) {
if (index == nr}
return irq;
index++;
}
return WARN_ON_ONCE(-EINVAL);
}
Ok, got it!
+ }
+ WARN_ON_ONCE(1);
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(msi_irq_vector);
+
+/**
+ * dev_msi_hwirq - Get the device MSI hw IRQ number of a device vector
+ * @dev: device to operate on
+ * @nr: device-relative interrupt vector index (0-based).
+ *
+ * Return the dev_msi hw IRQ number of a device vector.
+ */
+int dev_msi_hwirq(struct device *dev, unsigned int nr)
+{
+ struct msi_desc *entry;
+ int i = 0;
+
+ for_each_msi_entry(entry, dev) {
+ if (i == nr)
+ return entry->device_msi.hwirq;
+ i++;
+ }
+ WARN_ON_ONCE(1);
+ return -EINVAL;
+}
And this helper would be more generally useful if it returned the n-th
msi_desc entry rather than some obscure field in a substructure.
struct msi_desc *msi_get_nth_desc(struct device *dev, unsigned int nth)
{
struct msi_desc *entry = NULL;
unsigned int i = 0;
for_each_msi_entry(entry, dev) {
if (i == nth)
return entry;
i++;
}
WARN_ON_ONCE(!entry);
return entry;
}
You can always wrap it for your particular use case.
Yeah, makes sense.
+EXPORT_SYMBOL_GPL(dev_msi_hwirq);
+
#endif /* CONFIG_GENERIC_MSI_IRQ_DOMAIN */
And what uses these helpers?]
These helpers are to be used by a driver series(Intel's IDXD driver)
which is currently stuck due to VFIO refactoring.
Then I's suggest you keep the helpers together with the actual user,
unless this can generally be useful to existing users (exported
symbols without in-tree users is always a bit odd).
Yeah in the next submission, we will submit this patch series along with
the user driver patch series.
Thanks,
M.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
> From: Jason Gunthorpe > Sent: Tuesday, March 30, 2021 12:32 AM > > On Wed, Mar 24, 2021 at 12:05:28PM -0700, Jacob Pan wrote: > > > > IMHO a use created PASID is either bound to a mm (current) at creation > > > time, or it will never be bound to a mm and its page table is under > > > user control via /dev/ioasid. > > > > > True for PASID used in native SVA bind. But for binding with a guest mm, > > PASID is allocated first (VT-d virtual cmd interface Spec 10.4.44), the > > bind with the host IOMMU when vIOMMU PASID cache is invalidated. > > > > Our intention is to have two separate interfaces: > > 1. /dev/ioasid (allocation/free only) > > 2. /dev/sva (handles all SVA related activities including page tables) > > I'm not sure I understand why you'd want to have two things. Doesn't > that just complicate everything? > > Manipulating the ioasid, including filling it with page tables, seems > an integral inseperable part of the whole interface. Why have two ? Hi, Jason, Actually above is a major open while we are refactoring vSVA uAPI toward this direction. We have two concerns about merging /dev/ioasid with /dev/sva, and would like to hear your thought whether they are valid. First, userspace may use ioasid in a non-SVA scenario where ioasid is bound to specific security context (e.g. a control vq in vDPA) instead of tying to mm. In this case there is no pgtable binding initiated from user space. Instead, ioasid is allocated from /dev/ioasid and then programmed to the intended security context through specific passthrough framework which manages that context. Second, ioasid is managed per process/VM while pgtable binding is a device-wise operation. The userspace flow looks like below for an integral /dev/ioasid interface: ---initialization-- - ioctl(container->fd, VFIO_SET_IOMMU, VFIO_TYPE1_NESTING_IOMMU) - ioasid_fd = open(/dev/ioasid) - ioctl(ioasid_fd, IOASID_GET_USVA_FD, _fd) //an empty context - ioctl(device->fd, VFIO_DEVICE_SET_SVA, _fd); //sva_fd ties to device - ioctl(sva_fd, USVA_GET_INFO, _info); ---runtime - ioctl(ioasid_fd, IOMMU_ALLOC_IOASID, ); - ioctl(sva_fd, USVA_BIND_PGTBL, _data); - ioctl(sva_fd, USVA_FLUSH_CACHE, _info); - ioctl(sva_fd, USVA_UNBIND_PGTBL, _data); ---destroy - ioctl(device->fd, VFIO_DEVICE_UNSET_SVA, _fd); - close(sva_fd) - close(ioasid_fd) Our hesitation here is based on one of your earlier comments that you are not a fan of constructing fd's through ioctl. Are you OK with above flow or have a better idea of handling it? With separate interfaces then userspace just opens /dev/sva instead of getting it through ioasid_fd: - ioasid_fd = open(/dev/ioasid) - sva_fd = open(/dev/sva) Thanks Kevin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
Hi Will,
在 2021/3/29 22:45, Will Deacon 写道:
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
From: Xiang Chen
Currently it will send a iotlb sync at end of iommu unmap even if
iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
necessary, so add a check to avoid invalid iotlb sync.
Signed-off-by: Xiang Chen
---
include/linux/iommu.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ca6e6b..6afa61b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct
iommu_domain *domain)
static inline void iommu_iotlb_sync(struct iommu_domain *domain,
struct iommu_iotlb_gather *iotlb_gather)
{
+ if (!iotlb_gather->pgsize)
+ return;
In which circumstances does this occur?
Will
When iommu_unmap, it initializes iotlb_gather (then iotlb_gather->pgsize
= 0).
If the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && iopte_leaf(), it will
fill the iotlb_gather (set iotlb_gather->pgsize = granule);
but if the unmap size = ARM_LPAE_BLOCK_SIZE(lvl) && !iopte_leaf() (tlb
flush walk situation), iotlb_gather is not filled (iotlb_gather->pgsize
= 0),
it will still send iommu_iotlb_sync(domain, _gather) which is
actually invalid iotlb sync at last.
.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
Hi Jason, On Mon, 29 Mar 2021 13:31:47 -0300, Jason Gunthorpe wrote: > On Wed, Mar 24, 2021 at 12:05:28PM -0700, Jacob Pan wrote: > > > > IMHO a use created PASID is either bound to a mm (current) at creation > > > time, or it will never be bound to a mm and its page table is under > > > user control via /dev/ioasid. > > > > > True for PASID used in native SVA bind. But for binding with a guest mm, > > PASID is allocated first (VT-d virtual cmd interface Spec 10.4.44), the > > bind with the host IOMMU when vIOMMU PASID cache is invalidated. > > > > Our intention is to have two separate interfaces: > > 1. /dev/ioasid (allocation/free only) > > 2. /dev/sva (handles all SVA related activities including page tables) > > I'm not sure I understand why you'd want to have two things. Doesn't > that just complicate everything? > > Manipulating the ioasid, including filling it with page tables, seems > an integral inseperable part of the whole interface. Why have two ? > In one of the earlier discussions, I was made aware of some use cases (by AMD, iirc) where PASID can be used w/o IOMMU. That is why I tried to keep ioasid a separate subsystem. Other than that, I don't see an issue combining the two. > > > I thought the whole point of something like a /dev/ioasid was to get > > > away from each and every device creating its own PASID interface? > > > > > yes, but only for the use cases that need to expose PASID to the > > userspace. > > Why "but only"? This thing should reach for a higher generality, not > just be contained to solve some problem within qemu. > I totally agree in terms of generality. I was just trying to point out existing framework or drivers such as uacce and idxd driver does not have a need to use /dev/ioasid. > > > It maybe somewhat reasonable that some devices could have some easy > > > 'make a SVA PASID on current' interface built in, > > I agree, this is the case PASID is hidden from the userspace, right? > > e.g. uacce. > > "hidden", I guess, but does it matter so much? > it matters when it comes to which interface to choose. Use /dev/ioasid to allocate if PASID value cannot be hidden. Use some other interface for bind current and allocate if a PASID is not visible to the user. > The PASID would still consume a cgroup credit > yes, credit still consumed. Just the PASID value is hidden. > > > but anything more > > > complicated should use /dev/ioasid, and anything consuming PASID > > > should also have an API to import and attach a PASID from /dev/ioasid. > > > > > Would the above two use cases constitute the "complicated" criteria? Or > > we should say anything that need the explicit PASID value has to through > > /dev/ioasid? > > Anything that needs more that creating a hidden PASID link'd to > current should use the full interface. > Yes, I think we are on the same page. For example, today's uacce or idxd driver creates a hidden PASID when user does open(), where a new WQ is provisioned and bound to current mm. This is the case where /dev/ioasid is not needed. > > In terms of usage for guest SVA, an ioasid_set is mostly tied to a host > > mm, the use case is as the following: > > From that doc: > > It is imperative to enforce > VM-IOASID ownership such that a malicious guest cannot target DMA > traffic outside its own IOASIDs, or free an active IOASID that belongs > to another VM. > > Huh? > Sorry, I am not following. In the doc, I have an example to show the ioasid_set to VM/mm mapping. We use mm as the ioasid_set token to identify who the owner of an IOASID is. i.e. who allocated the IOASID. Non-owner cannot perform bind page table or free operations. Section: IOASID Set Private ID (SPID) .--..--. | VM 1 || VM 2 | | || | |--||--| | GPASID/SPID 101 || GPASID/SPID 101 | '--'---' Guest __|__| | | Host v v .--..--. | Host IOASID 201 || Host IOASID 202 | '--''--' | IOASID set 1 || IOASID set 2 | '--''--' > Security in a PASID world comes from the IOMMU blocking access to the > PASID except from approved PCI-ID's. If a VF/PF is assigned to a guest > then that guest can cause the device to issue any PASID by having > complete control and the vIOMMU is supposed to tell the real IOMMU > what PASID's the device is alowed to access. > Yes, each PF/VF has its own PASID table. The device can do whatever it wants as long as the PASID is present in the table. Programming of the pIOMMU PASID table entry, however, is controlled by the host. IMHO, there are two levels of security here: 1. A PASID can only be
Re: [PATCH] ARM: Qualify enabling of swiotlb_init()
On 3/19/21 5:22 PM, Stefano Stabellini wrote: > On Fri, 19 Mar 2021, Konrad Rzeszutek Wilk wrote: >> On Fri, Mar 19, 2021 at 02:07:31PM +0100, Christoph Hellwig wrote: >>> On Thu, Mar 18, 2021 at 09:03:33PM -0700, Florian Fainelli wrote: #ifdef CONFIG_ARM_LPAE + if (swiotlb_force == SWIOTLB_FORCE || + max_pfn > arm_dma_pfn_limit) >>> >>> Does arm_dma_pfn_limit do the right thing even with the weirdest >>> remapping ranges? Maybe a commen here would be useful. >>> + swiotlb_init(1); + else + swiotlb_force = SWIOTLB_NO_FORCE; >>> >>> Konrad: what do you think of setting swiotlb_force to SWIOTLB_NO_FORCE >>> and only switching it to SWIOTLB_NORMAL when swiotlb_init* is called? >>> That kind makes more sense than forcing the callers to do it. >>> >>> While we're at it, I think swiotlb_force should probably be renamed to >>> swiotlb_mode or somethng like that. >> >> swiotlb_mode sounds good. >> >> Also it got me thinking - ARM on Xen at some point was a bit strange, so not >> sure how >> the logic works here, Stefano? > > There is nothing strange in regards to swiotlb_force. swiotlb_force is only > used > in swiotlb-xen map_page to figure out whether: > > - we actually have to use the swiotlb bounce buffer (this is the > swiotlb_xen == SWIOTLB_FORCE case) > - or we can use the provided page directly for dma if other conditions > are met (dma_capable, !range_straddles_page_boundary, ...) > > > I don't think that switching to "swiotlb_mode" would cause any issues. > Should I toss this in Russell's patch tracker or do you need me to make some changes to the patch? -- Florian ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 05/18] iommu/ioasid: Redefine IOASID set and allocation APIs
On Wed, Mar 24, 2021 at 12:05:28PM -0700, Jacob Pan wrote: > > IMHO a use created PASID is either bound to a mm (current) at creation > > time, or it will never be bound to a mm and its page table is under > > user control via /dev/ioasid. > > > True for PASID used in native SVA bind. But for binding with a guest mm, > PASID is allocated first (VT-d virtual cmd interface Spec 10.4.44), the > bind with the host IOMMU when vIOMMU PASID cache is invalidated. > > Our intention is to have two separate interfaces: > 1. /dev/ioasid (allocation/free only) > 2. /dev/sva (handles all SVA related activities including page tables) I'm not sure I understand why you'd want to have two things. Doesn't that just complicate everything? Manipulating the ioasid, including filling it with page tables, seems an integral inseperable part of the whole interface. Why have two ? > > I thought the whole point of something like a /dev/ioasid was to get > > away from each and every device creating its own PASID interface? > > > yes, but only for the use cases that need to expose PASID to the > userspace. Why "but only"? This thing should reach for a higher generality, not just be contained to solve some problem within qemu. > > It maybe somewhat reasonable that some devices could have some easy > > 'make a SVA PASID on current' interface built in, > I agree, this is the case PASID is hidden from the userspace, right? e.g. > uacce. "hidden", I guess, but does it matter so much? The PASID would still consume a cgroup credit > > but anything more > > complicated should use /dev/ioasid, and anything consuming PASID > > should also have an API to import and attach a PASID from /dev/ioasid. > > > Would the above two use cases constitute the "complicated" criteria? Or we > should say anything that need the explicit PASID value has to through > /dev/ioasid? Anything that needs more that creating a hidden PASID link'd to current should use the full interface. > In terms of usage for guest SVA, an ioasid_set is mostly tied to a host mm, > the use case is as the following: >From that doc: It is imperative to enforce VM-IOASID ownership such that a malicious guest cannot target DMA traffic outside its own IOASIDs, or free an active IOASID that belongs to another VM. Huh? Security in a PASID world comes from the IOMMU blocking access to the PASID except from approved PCI-ID's. If a VF/PF is assigned to a guest then that guest can cause the device to issue any PASID by having complete control and the vIOMMU is supposed to tell the real IOMMU what PASID's the device is alowed to access. If a device is sharing a single PCI function with different security contexts (eg vfio mdev) then the device itself is responsible to ensure that only the secure interface can program a PASID and a less secure context can never self-enroll. Here the mdev driver would have to consule with the vIOMMU to ensure the mdev device is allowed to access the PASID - is that what this set stuff is about? If yes, it is backwards. The MDEV is the thing doing the security, the MDEV should have the list of allowed PASID's and a single PASID created under /dev/ioasid should be loaded into MDEV with some 'Ok you can use PASID xyz from FD abc' command. Because you absolutely don't want to have a generic 'set' that all the mdevs are sharing as that violates the basic security principle at the start - each and every device must have a unique list of what PASID's it can talk to. > 1. Identify a pool of PASIDs for permission checking (below to the same VM), > e.g. only allow SVA binding for PASIDs allocated from the same set. > > 2. Allow different PASID-aware kernel subsystems to associate, e.g. KVM, > device drivers, and IOMMU driver. i.e. each KVM instance only cares about > the ioasid_set associated with the VM. Events notifications are also within > the ioasid_set to synchronize PASID states. > > 3. Guest-Host PASID look up (each set has its own XArray to store the > mapping) > > 4. Quota control (going away once we have cgroup) It sounds worrysome things have gone this way. I'd say you shoul have a single /dev/ioasid per VM and KVM should attach to that - it should get all the global events/etc that are not device specific. permission checking *must* be done on a per-device level, either inside the mdev driver, or inside the IOMMU at a per-PCI device level. Not sure what guest-host PASID means, these have to be 1:1 for device assignment to work - why would use something else for mdev? Jason ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v1 02/15] iommu: Add a simple PASID table library
On Fri, Mar 12, 2021 at 06:17:55PM +0530, Vivek Kumar Gautam wrote: > > Regarding the overall design, I was initially assigning page directories > > instead of whole PASID tables, which would simplify the driver and host > > implementation. A major complication, however, is SMMUv3 accesses PASID > > tables using a guest-physical address, so there is a messy negotiation > > needed between host and guest when the host needs to allocate PASID > > tables. Plus vSMMU needs PASID table assignment, so that's what the host > > driver will implement. > > By assigning the page directories, you mean setting up just the stage-1 page > table ops, and passing that information to the host using ATTACH_TABLE? Yes. And we can support nested translation with SMMUv2 that way. But with SMMUv3 the guest has to manage the whole PASID table. > Right now when using kvmtool, the struct iommu_pasid_table_config is > populated with the correct information, and this whole memory is mapped > between host and guest by creating a mem bank using > kvm__for_each_mem_bank(). > Did I get you or did I fail terribly in understanding the point you are > making here? Makes sense Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v1 15/15] iommu/virtio: Update fault type and reason info for viommu fault
On Fri, Mar 12, 2021 at 06:39:05PM +0530, Vivek Kumar Gautam wrote: > To complete the page request we would also need to send the response back to > the host from virtio backend when handling page request. So the virtio > command should also be accompanied with a vfio api to send the page request > response back to the host. Isn't it? > This is where the host smmuv3 can send PRI_RESP command to the device to > complete the page fault. It looks like Eric already has this in the VFIO series: https://lore.kernel.org/linux-iommu/20210223210625.604517-14-eric.au...@redhat.com/ Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH RFC v1 13/15] iommu/virtio: Attach Arm PASID tables when available
On Fri, Mar 12, 2021 at 06:59:17PM +0530, Vivek Kumar Gautam wrote: > > > + /* XXX HACK: set feature bit ARM_SMMU_FEAT_2_LVL_CDTAB */ > > > + pst_cfg->vendor.cfg.feat_flag |= (1 << 1); > > > > Oh right, this flag is missing. I'll add > > > >#define VIRTIO_IOMMU_PST_ARM_SMMU3_F_CD2L (1ULL << 1) > > > > to the spec. > > Regarding this Eric pointed out [1] in my other patch about the scalability > of the approach where we keep adding flags in 'iommu_nesting_info' > corresponding to the arm-smmu-v3 capabilities. I guess the same goes to > these flags in virtio. > May be the 'iommu_nesting_info' can have a bitmap with the caps for vendor > specific features, and here we can add the related flags? Something like that, but I'd keep separate arch-specific structs. Vt-d reports the capability registers directly through iommu_nesting_info [2]. We could do the same for Arm, copy sanitized values of IDR0..5 into struct iommu_nesting_info_arm_smmuv3. I've avoided doing that for virtio-iommu because every field needs a description in the spec. So where possible I used generic properties that apply to any architecture, such as page, PASID and address size. What's left is the minimum arch-specific information to get nested translation going, leaving out a lot of properties such as big-endian and 32-bit, which can be added later if needed. The Arm specific properties are split into page table and pasid table information. Page table info should work for both SMMUv2 and v3 (where they correspond to an SMMU_IDRx field that constrains a context descriptor field.) I should move BTM in there since it's supported by SMMUv2. Thanks, Jean [2] https://lore.kernel.org/linux-iommu/20210302203545.436623-11-yi.l@intel.com/ ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Add a check to avoid invalid iotlb sync
On Sat, Mar 27, 2021 at 02:23:10PM +0800, chenxiang wrote:
> From: Xiang Chen
>
> Currently it will send a iotlb sync at end of iommu unmap even if
> iotlb_gather is not valid (iotlb_gather->pgsize = 0). Actually it is not
> necessary, so add a check to avoid invalid iotlb sync.
>
> Signed-off-by: Xiang Chen
> ---
> include/linux/iommu.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9ca6e6b..6afa61b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -529,6 +529,9 @@ static inline void iommu_flush_iotlb_all(struct
> iommu_domain *domain)
> static inline void iommu_iotlb_sync(struct iommu_domain *domain,
> struct iommu_iotlb_gather *iotlb_gather)
> {
> + if (!iotlb_gather->pgsize)
> + return;
In which circumstances does this occur?
Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/30] DMA: Mundane typo fixes
On 13:48 Mon 29 Mar 2021, Greg KH wrote: On Mon, Mar 29, 2021 at 11:25:11AM +0530, Bhaskar Chowdhury wrote: On 07:29 Mon 29 Mar 2021, Christoph Hellwig wrote: > I really don't think these typo patchbomb are that useful. I'm all > for fixing typos when working with a subsystem, but I'm not sure these > patchbombs help anything. > I am sure you are holding the wrong end of the wand and grossly failing to understand. Please stop statements like this, it is not helpful and is doing nothing but ensure that your patches will not be looked at in the future. Greg, don't you think you are bit harsh and have an one sided view? People can say in better way if they don't like some work. I Have always try to get along. Anyway, I hope I give a heads up ...find "your way" to fix those damn thing...it's glaring There is no requirement that anyone accept patches that are sent to them. When you complain when receiving comments on them, that shows you do not wish to work with others. Unfortunate you are only seeing my complains...I don't know why you are so blindfolded. Sorry, but you are now on my local blacklist for a while, and I encourage other maintainers to just ignore these patches as well. I can not overrule that ...I know my pathes are trivial ..but it seems some other problems are looming large. NOT good Gregnot good seriously. thanks, greg k-h signature.asc Description: PGP signature ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 00/30] DMA: Mundane typo fixes
On Mon, Mar 29, 2021 at 11:25:11AM +0530, Bhaskar Chowdhury wrote: > On 07:29 Mon 29 Mar 2021, Christoph Hellwig wrote: > > I really don't think these typo patchbomb are that useful. I'm all > > for fixing typos when working with a subsystem, but I'm not sure these > > patchbombs help anything. > > > I am sure you are holding the wrong end of the wand and grossly failing to > understand. Please stop statements like this, it is not helpful and is doing nothing but ensure that your patches will not be looked at in the future. > Anyway, I hope I give a heads up ...find "your way" to fix those damn > thing...it's glaring There is no requirement that anyone accept patches that are sent to them. When you complain when receiving comments on them, that shows you do not wish to work with others. Sorry, but you are now on my local blacklist for a while, and I encourage other maintainers to just ignore these patches as well. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
