Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
On Wed, Mar 20, 2019 at 01:13:41PM -0300, Thiago Jung Bauermann wrote: > >> Another way of looking at this issue which also explains our reluctance > >> is that the only difference between a secure guest and a regular guest > >> (at least regarding virtio) is that the former uses swiotlb while the > >> latter doens't. > > > > But swiotlb is just one implementation. It's a guest internal thing. The > > issue is that memory isn't host accessible. > > >From what I understand of the ACCESS_PLATFORM definition, the host will > only ever try to access memory addresses that are supplied to it by the > guest, so all of the secure guest memory that the host cares about is > accessible: > > If this feature bit is set to 0, then the device has same access to > memory addresses supplied to it as the driver has. In particular, > the device will always use physical addresses matching addresses > used by the driver (typically meaning physical addresses used by the > CPU) and not translated further, and can access any address supplied > to it by the driver. When clear, this overrides any > platform-specific description of whether device access is limited or > translated in any way, e.g. whether an IOMMU may be present. > > All of the above is true for POWER guests, whether they are secure > guests or not. > > Or are you saying that a virtio device may want to access memory > addresses that weren't supplied to it by the driver? Your logic would apply to IOMMUs as well. For your mode, there are specific encrypted memory regions that driver has access to but device does not. that seems to violate the constraint. > >> And from the device's point of view they're > >> indistinguishable. It can't tell one guest that is using swiotlb from > >> one that isn't. And that implies that secure guest vs regular guest > >> isn't a virtio interface issue, it's "guest internal affairs". So > >> there's no reason to reflect that in the feature flags. > > > > So don't. The way not to reflect that in the feature flags is > > to set ACCESS_PLATFORM. Then you say *I don't care let platform device*. > > > > > > Without ACCESS_PLATFORM > > virtio has a very specific opinion about the security of the > > device, and that opinion is that device is part of the guest > > supervisor security domain. > > Sorry for being a bit dense, but not sure what "the device is part of > the guest supervisor security domain" means. In powerpc-speak, > "supervisor" is the operating system so perhaps that explains my > confusion. Are you saying that without ACCESS_PLATFORM, the guest > considers the host to be part of the guest operating system's security > domain? I think so. The spec says "device has same access as driver". > If so, does that have any other implication besides "the host > can access any address supplied to it by the driver"? If that is the > case, perhaps the definition of ACCESS_PLATFORM needs to be amended to > include that information because it's not part of the current > definition. > > >> That said, we still would like to arrive at a proper design for this > >> rather than add yet another hack if we can avoid it. So here's another > >> proposal: considering that the dma-direct code (in kernel/dma/direct.c) > >> automatically uses swiotlb when necessary (thanks to Christoph's recent > >> DMA work), would it be ok to replace virtio's own direct-memory code > >> that is used in the !ACCESS_PLATFORM case with the dma-direct code? That > >> way we'll get swiotlb even with !ACCESS_PLATFORM, and virtio will get a > >> code cleanup (replace open-coded stuff with calls to existing > >> infrastructure). > > > > Let's say I have some doubts that there's an API that > > matches what virtio with its bag of legacy compatibility exactly. > > Ok. > > >> > But the name "sev_active" makes me scared because at least AMD guys who > >> > were doing the sensible thing and setting ACCESS_PLATFORM > >> > >> My understanding is, AMD guest-platform knows in advance that their > >> guest will run in secure mode and hence sets the flag at the time of VM > >> instantiation. Unfortunately we dont have that luxury on our platforms. > > > > Well you do have that luxury. It looks like that there are existing > > guests that already acknowledge ACCESS_PLATFORM and you are not happy > > with how that path is slow. So you are trying to optimize for > > them by clearing ACCESS_PLATFORM and then you have lost ability > > to invoke DMA API. > > > > For example if there was another flag just like ACCESS_PLATFORM > > just not yet used by anyone, you would be all fine using that right? > > Yes, a new flag sounds like a great idea. What about the definition > below? > > VIRTIO_F_ACCESS_PLATFORM_NO_IOMMU This feature has the same meaning as > VIRTIO_F_ACCESS_PLATFORM both when set and when not set, with the > exception that the IOMMU is explicitly defined to be off or bypassed > when accessing memory addresses supplied to
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 01/03/2019 20:20, Douglas Anderson wrote: > If you're bisecting why your peripherals stopped working, it's > probably this CL. Specifically if you see this in your dmesg: > Unexpected global fault, this could be serious > ...then it's almost certainly this CL. > > Running your IOMMU-enabled peripherals with the IOMMU in bypass mode > is insecure and effectively disables the protection they provide. > There are few reasons to allow unmatched stream bypass, and even fewer > good ones. > > This patch starts the transition over to make it much harder to run > your system insecurely. Expected steps: > > 1. By default disable bypass (so anyone insecure will notice) but make >it easy for someone to re-enable bypass with just a KConfig change. >That's this patch. > > 2. After people have had a little time to come to grips with the fact >that they need to set their IOMMUs properly and have had time to >dig into how to do this, the KConfig will be eliminated and bypass >will simply be disabled. Folks who are truly upset and still >haven't fixed their system can either figure out how to add >'arm-smmu.disable_bypass=n' to their command line or revert the >patch in their own private kernel. Of course these folks will be >less secure. > > Suggested-by: Robin Murphy > Signed-off-by: Douglas Anderson > --- > > Changes in v2: > - Flipped default to 'yes' and changed comments a lot. > > drivers/iommu/Kconfig| 25 + > drivers/iommu/arm-smmu.c | 3 ++- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 1ca1fa107b21..a4210672804a 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -359,6 +359,31 @@ config ARM_SMMU > Say Y here if your SoC includes an IOMMU device implementing > the ARM SMMU architecture. > > +config ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT > + bool "Default to disabling bypass on ARM SMMU v1 and v2" > + depends on ARM_SMMU > + default y > + help > + Say Y here to (by default) disable bypass streams such that > + incoming transactions from devices that are not attached to > + an iommu domain will report an abort back to the device and > + will not be allowed to pass through the SMMU. > + > + Any old kernels that existed before this KConfig was > + introduced would default to _allowing_ bypass (AKA the > + equivalent of NO for this config). However the default for > + this option is YES because the old behavior is insecure. > + > + There are few reasons to allow unmatched stream bypass, and > + even fewer good ones. If saying YES here breaks your board > + you should work on fixing your board. This KConfig option > + is expected to be removed in the future and we'll simply > + hardcode the bypass disable in the code. > + > + NOTE: the kernel command line parameter > + 'arm-smmu.disable_bypass' will continue to override this > + config. > + > config ARM_SMMU_V3 > bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" > depends on ARM64 > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 045d93884164..930c07635956 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -110,7 +110,8 @@ static int force_stage; > module_param(force_stage, int, S_IRUGO); > MODULE_PARM_DESC(force_stage, > "Force SMMU mappings to be installed at a particular stage of > translation. A value of '1' or '2' forces the corresponding stage. All other > values are ignored (i.e. no stage is forced). Note that selecting a specific > stage will disable support for nested translation."); > -static bool disable_bypass; > +static bool disable_bypass = > + IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT); > module_param(disable_bypass, bool, S_IRUGO); > MODULE_PARM_DESC(disable_bypass, > "Disable bypass streams such that incoming transactions from devices > that are not attached to an iommu domain will report an abort back to the > device and will not be allowed to pass through the SMMU."); I'm hoping someone can clear my confusion: drivers/iommu/arm-smmu.c defines a boolean module_param: disable_bypass It is used to select the s2cr_init_val, and whether sCR0_USFCFG is set. drivers/iommu/iommu.c defines iommu_def_domain_type differently, based on CONFIG_IOMMU_DEFAULT_PASSTHROUGH How do these two similar concepts interact? (bypass vs passthrough) Regards. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2] iommu/iova: Fix tracking of recently failed iova address
On 18.03.19 15:19:23, Robin Murphy wrote:
> On 15/03/2019 15:56, Robert Richter wrote:
> > We track the smallest size that failed for a 32 bit allocation. The
> > Size decreases only and if we actually walked the tree and noticed an
> > allocation failure. Current code is broken and wrongly updates the
> > size value even if we did not try an allocation. This leads to
> > increased size values and we might go the slow path again even if we
> > have seen a failure before for the same or a smaller size.
>
> That description wasn't too clear (since it rather contradicts itself by
> starting off with "XYZ happens" when the whole point is that XYZ doesn't
> actually happen properly), but having gone and looked at the code in context
> I think I understand it now - specifically, it's that the early-exit path
> for detecting that a 32-bit allocation request is too big to possibly
> succeed should never have gone via the route which assigns to
> max32_alloc_size.
>
> In that respect, the diff looks correct, so modulo possibly tweaking the
> commit message,
>
> Reviewed-by: Robin Murphy
Robin, thanks for your review.
I hope the following description is better now.
Thanks,
-Robert
-- >8 --
From: Robert Richter
Subject: [PATCH v2] iommu/iova: Fix tracking of recently failed iova address
size
If a 32 bit allocation request is too big to possibly succeed, it
early exits with a failure and then should never update max32_alloc_
size. This patch fixes current code, now the size is only updated if
the slow path failed while walking the tree. Without the fix the
allocation may enter the slow path again even if there was a failure
before of a request with the same or a smaller size.
Cc: # 4.20+
Fixes: bee60e94a1e2 ("iommu/iova: Optimise attempts to allocate iova from 32bit
address range")
Signed-off-by: Robert Richter
Reviewed-by: Robin Murphy
Signed-off-by: Robert Richter
---
drivers/iommu/iova.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index f8d3ba247523..2de8122e218f 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -207,8 +207,10 @@ static int __alloc_and_insert_iova_range(struct
iova_domain *iovad,
curr_iova = rb_entry(curr, struct iova, node);
} while (curr && new_pfn <= curr_iova->pfn_hi);
- if (limit_pfn < size || new_pfn < iovad->start_pfn)
+ if (limit_pfn < size || new_pfn < iovad->start_pfn) {
+ iovad->max32_alloc_size = size;
goto iova32_full;
+ }
/* pfn_lo will point to size aligned address if size_aligned is set */
new->pfn_lo = new_pfn;
@@ -222,7 +224,6 @@ static int __alloc_and_insert_iova_range(struct iova_domain
*iovad,
return 0;
iova32_full:
- iovad->max32_alloc_size = size;
spin_unlock_irqrestore(>iova_rbtree_lock, flags);
return -ENOMEM;
}
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu/arm-smmu: Break insecure users by disabling bypass by default
On 20/03/2019 15:48, Marc Gonzalez wrote: On 01/03/2019 20:20, Douglas Anderson wrote: If you're bisecting why your peripherals stopped working, it's probably this CL. Specifically if you see this in your dmesg: Unexpected global fault, this could be serious ...then it's almost certainly this CL. Running your IOMMU-enabled peripherals with the IOMMU in bypass mode is insecure and effectively disables the protection they provide. There are few reasons to allow unmatched stream bypass, and even fewer good ones. This patch starts the transition over to make it much harder to run your system insecurely. Expected steps: 1. By default disable bypass (so anyone insecure will notice) but make it easy for someone to re-enable bypass with just a KConfig change. That's this patch. 2. After people have had a little time to come to grips with the fact that they need to set their IOMMUs properly and have had time to dig into how to do this, the KConfig will be eliminated and bypass will simply be disabled. Folks who are truly upset and still haven't fixed their system can either figure out how to add 'arm-smmu.disable_bypass=n' to their command line or revert the patch in their own private kernel. Of course these folks will be less secure. Suggested-by: Robin Murphy Signed-off-by: Douglas Anderson --- Changes in v2: - Flipped default to 'yes' and changed comments a lot. drivers/iommu/Kconfig| 25 + drivers/iommu/arm-smmu.c | 3 ++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 1ca1fa107b21..a4210672804a 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -359,6 +359,31 @@ config ARM_SMMU Say Y here if your SoC includes an IOMMU device implementing the ARM SMMU architecture. +config ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT + bool "Default to disabling bypass on ARM SMMU v1 and v2" + depends on ARM_SMMU + default y + help + Say Y here to (by default) disable bypass streams such that + incoming transactions from devices that are not attached to + an iommu domain will report an abort back to the device and + will not be allowed to pass through the SMMU. + + Any old kernels that existed before this KConfig was + introduced would default to _allowing_ bypass (AKA the + equivalent of NO for this config). However the default for + this option is YES because the old behavior is insecure. + + There are few reasons to allow unmatched stream bypass, and + even fewer good ones. If saying YES here breaks your board + you should work on fixing your board. This KConfig option + is expected to be removed in the future and we'll simply + hardcode the bypass disable in the code. + + NOTE: the kernel command line parameter + 'arm-smmu.disable_bypass' will continue to override this + config. + config ARM_SMMU_V3 bool "ARM Ltd. System MMU Version 3 (SMMUv3) Support" depends on ARM64 diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 045d93884164..930c07635956 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -110,7 +110,8 @@ static int force_stage; module_param(force_stage, int, S_IRUGO); MODULE_PARM_DESC(force_stage, "Force SMMU mappings to be installed at a particular stage of translation. A value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no stage is forced). Note that selecting a specific stage will disable support for nested translation."); -static bool disable_bypass; +static bool disable_bypass = + IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT); module_param(disable_bypass, bool, S_IRUGO); MODULE_PARM_DESC(disable_bypass, "Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU."); I'm hoping someone can clear my confusion: drivers/iommu/arm-smmu.c defines a boolean module_param: disable_bypass It is used to select the s2cr_init_val, and whether sCR0_USFCFG is set. drivers/iommu/iommu.c defines iommu_def_domain_type differently, based on CONFIG_IOMMU_DEFAULT_PASSTHROUGH How do these two similar concepts interact? (bypass vs passthrough) Bypass in this case mostly refers to unmatched stream bypass, i.e. whether Stream IDs that the kernel doesn't know about are blocked or not once the SMMU is enabled. The s2cr_init_val thing is less important since default domain support got finished, as the window in which streams can be known but not assigned to anything is now pretty minimal. The only real reasons for allowing bypass are bringup situations where you'd rather focus on more important things than SMMU
[PATCH 4/4] iommu/arm-smmu-v3: Disable tagged pointers
The ARM architecture has a "Top Byte Ignore" (TBI) option that makes the MMU mask out bits [63:56] of an address, allowing a userspace application to store data in its pointers. This option is incompatible with PCI ATS. If TBI is enabled in the SMMU and userspace triggers DMA transactions on tagged pointers, the endpoint might create ATC entries for addresses that include a tag. Software would then have to send ATC invalidation packets for each 255 possible alias of an address, or just wipe the whole address space. This is not a viable option, so disable TBI. The impact of this change is unclear, since there are very few users of tagged pointers, much less SVA. But the requirement introduced by this patch doesn't seem excessive: a userspace application using both tagged pointers and SVA should now sanitize addresses (clear the tag) before using them for device DMA. Signed-off-by: Jean-Philippe Brucker --- drivers/iommu/arm-smmu-v3.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c index c605d6f1b2df..69afffdaf907 100644 --- a/drivers/iommu/arm-smmu-v3.c +++ b/drivers/iommu/arm-smmu-v3.c @@ -1076,7 +1076,6 @@ static u64 arm_smmu_cpu_tcr_to_cd(u64 tcr) val |= ARM_SMMU_TCR2CD(tcr, EPD0); val |= ARM_SMMU_TCR2CD(tcr, EPD1); val |= ARM_SMMU_TCR2CD(tcr, IPS); - val |= ARM_SMMU_TCR2CD(tcr, TBI0); return val; } -- 2.21.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/4] ACPI/IORT: Check ATS capability in root complex nodes
Root complex node in IORT has a bit telling whether it supports ATS or
not. Store this bit in the IOMMU fwspec when setting up a device, so it
can be accessed later by an IOMMU driver.
Use the negative version (NO_ATS) at the moment because it's not clear
if/how the bit needs to be integrated in other firmware descriptions. The
SMMU has a feature bit telling if it supports ATS, which might be
sufficient in most systems for deciding whether or not we should enable
the ATS capability in endpoints.
Signed-off-by: Jean-Philippe Brucker
---
drivers/acpi/arm64/iort.c | 11 +++
include/linux/iommu.h | 4
2 files changed, 15 insertions(+)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index e48894e002ba..7f2c1c9c6b38 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1028,6 +1028,14 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr,
u64 *dma_size)
dev_dbg(dev, "dma_pfn_offset(%#08llx)\n", offset);
}
+static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
+{
+ struct acpi_iort_root_complex *pci_rc;
+
+ pci_rc = (struct acpi_iort_root_complex *)node->node_data;
+ return pci_rc->ats_attribute & ACPI_IORT_ATS_SUPPORTED;
+}
+
/**
* iort_iommu_configure - Set-up IOMMU configuration for a device.
*
@@ -1063,6 +1071,9 @@ const struct iommu_ops *iort_iommu_configure(struct
device *dev)
info.node = node;
err = pci_for_each_dma_alias(to_pci_dev(dev),
iort_pci_iommu_init, );
+
+ if (!err && !iort_pci_rc_supports_ats(node))
+ dev->iommu_fwspec->flags |= IOMMU_FWSPEC_PCI_NO_ATS;
} else {
int i = 0;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 3dbeb457fb16..ed6738c358ca 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -509,10 +509,14 @@ struct iommu_fwspec {
const struct iommu_ops *ops;
struct fwnode_handle*iommu_fwnode;
void*iommu_priv;
+ u32 flags;
unsigned intnum_ids;
u32 ids[1];
};
+/* Firmware disabled ATS in the root complex */
+#define IOMMU_FWSPEC_PCI_NO_ATS(1 << 0)
+
int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
const struct iommu_ops *ops);
void iommu_fwspec_free(struct device *dev);
--
2.21.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 2/4] iommu/arm-smmu-v3: Link domains and devices
When removing a mapping from a domain, we need to send an invalidation to
all devices that might have stored it in their Address Translation Cache
(ATC). In addition when updating the context descriptor of a live domain,
we'll need to send invalidations for all devices attached to it.
Maintain a list of devices in each domain, protected by a spinlock. It is
updated every time we attach or detach devices to and from domains.
It needs to be a spinlock because we'll invalidate ATC entries from
within hardirq-safe contexts, but it may be possible to relax the read
side with RCU later.
Signed-off-by: Jean-Philippe Brucker
---
drivers/iommu/arm-smmu-v3.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index d3880010c6cf..66a29c113dbc 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -594,6 +594,11 @@ struct arm_smmu_device {
struct arm_smmu_master_data {
struct arm_smmu_device *smmu;
struct arm_smmu_strtab_ent ste;
+
+ struct arm_smmu_domain *domain;
+ struct list_headdomain_head;
+
+ struct device *dev;
};
/* SMMU private data for an IOMMU domain */
@@ -618,6 +623,9 @@ struct arm_smmu_domain {
};
struct iommu_domain domain;
+
+ struct list_headdevices;
+ spinlock_t devices_lock;
};
struct arm_smmu_option_prop {
@@ -1493,6 +1501,9 @@ static struct iommu_domain
*arm_smmu_domain_alloc(unsigned type)
}
mutex_init(_domain->init_mutex);
+ INIT_LIST_HEAD(_domain->devices);
+ spin_lock_init(_domain->devices_lock);
+
return _domain->domain;
}
@@ -1711,8 +1722,18 @@ static void arm_smmu_install_ste_for_dev(struct
iommu_fwspec *fwspec)
static void arm_smmu_detach_dev(struct device *dev)
{
+ unsigned long flags;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct arm_smmu_master_data *master = fwspec->iommu_priv;
+ struct arm_smmu_domain *smmu_domain = master->domain;
+
+ if (smmu_domain) {
+ spin_lock_irqsave(_domain->devices_lock, flags);
+ list_del(>domain_head);
+ spin_unlock_irqrestore(_domain->devices_lock, flags);
+
+ master->domain = NULL;
+ }
master->ste.assigned = false;
arm_smmu_install_ste_for_dev(fwspec);
@@ -1721,6 +1742,7 @@ static void arm_smmu_detach_dev(struct device *dev)
static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
{
int ret = 0;
+ unsigned long flags;
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct arm_smmu_device *smmu;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
@@ -1757,6 +1779,11 @@ static int arm_smmu_attach_dev(struct iommu_domain
*domain, struct device *dev)
}
ste->assigned = true;
+ master->domain = smmu_domain;
+
+ spin_lock_irqsave(_domain->devices_lock, flags);
+ list_add(>domain_head, _domain->devices);
+ spin_unlock_irqrestore(_domain->devices_lock, flags);
if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS) {
ste->s1_cfg = NULL;
@@ -1883,6 +1910,7 @@ static int arm_smmu_add_device(struct device *dev)
return -ENOMEM;
master->smmu = smmu;
+ master->dev = dev;
fwspec->iommu_priv = master;
}
--
2.21.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 05/22] iommu: Introduce cache_invalidate API
On 20/03/2019 16:37, Jacob Pan wrote:
[...]
>> +struct iommu_inv_addr_info {
>> +#define IOMMU_INV_ADDR_FLAGS_PASID (1 << 0)
>> +#define IOMMU_INV_ADDR_FLAGS_ARCHID (1 << 1)
>> +#define IOMMU_INV_ADDR_FLAGS_LEAF (1 << 2)
>> +__u32 flags;
>> +__u32 archid;
>> +__u64 pasid;
>> +__u64 addr;
>> +__u64 granule_size;
>> +__u64 nb_granules;
>> +};
>> +
>> +/**
>> + * First level/stage invalidation information
>> + * @cache: bitfield that allows to select which caches to invalidate
>> + * @granularity: defines the lowest granularity used for the
>> invalidation:
>> + * domain > pasid > addr
>> + *
>> + * Not all the combinations of cache/granularity make sense:
>> + *
>> + * type | DEV_IOTLB | IOTLB | PASID|
>> + * granularity | | |
>> cache|
>> + * -+---+---+---+
>> + * DOMAIN | N/A | Y |
>> Y|
>> + * PASID| Y | Y |
>> Y|
>> + * ADDR | Y | Y |
>> N/A |
>> + */
>> +struct iommu_cache_invalidate_info {
>> +#define IOMMU_CACHE_INVALIDATE_INFO_VERSION_1 1
>> +__u32 version;
>> +/* IOMMU paging structure cache */
>> +#define IOMMU_CACHE_INV_TYPE_IOTLB (1 << 0) /* IOMMU IOTLB */
>> +#define IOMMU_CACHE_INV_TYPE_DEV_IOTLB (1 << 1) /* Device
>> IOTLB */ +#define IOMMU_CACHE_INV_TYPE_PASID (1 << 2) /* PASID
>> cache */
> Just a clarification, this used to be an enum. You do intend to issue a
> single invalidation request on multiple cache types? Perhaps for
> virtio-IOMMU? I only see a single cache type in your patch #14. For VT-d
> we plan to issue one cache type at a time for now. So this format works
> for us.
Yes for virtio-iommu I'd like as little overhead as possible, which
means a single invalidation message to hit both IOTLB and ATC at once,
and the ability to specify multiple pages with @nb_granules.
> However, if multiple cache types are issued in a single invalidation.
> They must share a single granularity, not all combinations are valid.
> e.g. dev IOTLB does not support domain granularity. Just a reminder,
> not an issue. Driver could filter out invalid combinations.
Agreed. Even the core could filter out invalid combinations based on the
table above: IOTLB and domain granularity are N/A.
Thanks,
Jean
>
>> +__u8cache;
>> +__u8granularity;
>> +__u8padding[2];
>> +union {
>> +__u64 pasid;
>> +struct iommu_inv_addr_info addr_info;
>> +};
>> +};
>> +
>> +
>> #endif /* _UAPI_IOMMU_H */
>
> [Jacob Pan]
> ___
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
Hello Michael, Sorry for the delay in responding. We had some internal discussions on this. Michael S. Tsirkin writes: > On Mon, Feb 04, 2019 at 04:14:20PM -0200, Thiago Jung Bauermann wrote: >> >> Hello Michael, >> >> Michael S. Tsirkin writes: >> >> > On Tue, Jan 29, 2019 at 03:42:44PM -0200, Thiago Jung Bauermann wrote: >> So while ACCESS_PLATFORM solves our problems for secure guests, we can't >> turn it on by default because we can't affect legacy systems. Doing so >> would penalize existing systems that can access all memory. They would >> all have to unnecessarily go through address translations, and take a >> performance hit. > > So as step one, you just give hypervisor admin an option to run legacy > systems faster by blocking secure mode. I don't see why that is > so terrible. There are a few reasons why: 1. It's bad user experience to require people to fiddle with knobs for obscure reasons if it's possible to design things such that they Just Work. 2. "User" in this case can be a human directly calling QEMU, but could also be libvirt or one of its users, or some other framework. This means having to adjust and/or educate an open-ended number of people and software. It's best avoided if possible. 3. The hypervisor admin and the admin of the guest system don't necessarily belong to the same organization (e.g., cloud provider and cloud customer), so there may be some friction when they need to coordinate to get this right. 4. A feature of our design is that the guest may or may not decide to "go secure" at boot time, so it's best not to depend on flags that may or may not have been set at the time QEMU was started. >> The semantics of ACCESS_PLATFORM assume that the hypervisor/QEMU knows >> in advance - right when the VM is instantiated - that it will not have >> access to all guest memory. > > Not quite. It just means that hypervisor can live with not having > access to all memory. If platform wants to give it access > to all memory that is quite all right. Except that on powerpc it also means "there's an IOMMU present" and there's no way to say "bypass IOMMU translation". :-/ >> Another way of looking at this issue which also explains our reluctance >> is that the only difference between a secure guest and a regular guest >> (at least regarding virtio) is that the former uses swiotlb while the >> latter doens't. > > But swiotlb is just one implementation. It's a guest internal thing. The > issue is that memory isn't host accessible. >From what I understand of the ACCESS_PLATFORM definition, the host will only ever try to access memory addresses that are supplied to it by the guest, so all of the secure guest memory that the host cares about is accessible: If this feature bit is set to 0, then the device has same access to memory addresses supplied to it as the driver has. In particular, the device will always use physical addresses matching addresses used by the driver (typically meaning physical addresses used by the CPU) and not translated further, and can access any address supplied to it by the driver. When clear, this overrides any platform-specific description of whether device access is limited or translated in any way, e.g. whether an IOMMU may be present. All of the above is true for POWER guests, whether they are secure guests or not. Or are you saying that a virtio device may want to access memory addresses that weren't supplied to it by the driver? >> And from the device's point of view they're >> indistinguishable. It can't tell one guest that is using swiotlb from >> one that isn't. And that implies that secure guest vs regular guest >> isn't a virtio interface issue, it's "guest internal affairs". So >> there's no reason to reflect that in the feature flags. > > So don't. The way not to reflect that in the feature flags is > to set ACCESS_PLATFORM. Then you say *I don't care let platform device*. > > > Without ACCESS_PLATFORM > virtio has a very specific opinion about the security of the > device, and that opinion is that device is part of the guest > supervisor security domain. Sorry for being a bit dense, but not sure what "the device is part of the guest supervisor security domain" means. In powerpc-speak, "supervisor" is the operating system so perhaps that explains my confusion. Are you saying that without ACCESS_PLATFORM, the guest considers the host to be part of the guest operating system's security domain? If so, does that have any other implication besides "the host can access any address supplied to it by the driver"? If that is the case, perhaps the definition of ACCESS_PLATFORM needs to be amended to include that information because it's not part of the current definition. >> That said, we still would like to arrive at a proper design for this >> rather than add yet another hack if we can avoid it. So here's another >> proposal: considering that the dma-direct code (in kernel/dma/direct.c) >> automatically uses
[PATCH v2 0/1] IOMMU SVA device driver interface
This is the device driver API for SVA (Shared Virtual Addressing). Changes since v1 [1]: * Following comments, return a handle rather than a PASID. I agree that it makes things easier for device drivers as well, because they don't need to worry about the unbind()/mm_exit() race as much. Previously they couldn't issue an unbind() if the mm exited since the PASID could have been reallocated. * If a handle already exists, reuse it and take a reference instead of returning EEXIST. As noted by Zhangfei and myself during development, it can make things a bit easier for device drivers that need to handle multiple threads in a process issuing bind() for the same dev and mm. The four dev_feature functions are implemented by Lu Baolu's IOMMU-aware mdev series [2]. iommu_dev_has_feature(dev, IOMMU_DEV_FEAT_SVA) -> true/false iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) -> 0/err iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA) -> 0/err iommu_dev_feature_enabled(dev, IOMMU_DEV_FEAT_SVA) -> true/false Patch 1/1 adds the next four functions, once the SVA feature is enabled: iommu_sva_bind(dev, mm, drvdata) -> handle /* * Only exit_mm() for now, but later callbacks to deal with * unhandled PRI faults and possible others: */ iommu_sva_set_ops(handle, iommu_sva_ops) -> 0/err iommu_sva_get_pasid(handle) -> pasid/invalid iommu_sva_unbind(handle) Full support for the SMMUv3 can be found at [3] [1] https://lore.kernel.org/lkml/20190220142759.33308-1-jean-philippe.bruc...@arm.com/ [2] https://lore.kernel.org/lkml/20190213040301.23021-10-baolu...@linux.intel.com/T/ [3] git://linux-arm.org/linux-jpb.git sva/current http://www.linux-arm.org/git?p=linux-jpb.git;a=shortlog;h=refs/heads/sva/current Jean-Philippe Brucker (1): iommu: Bind process address spaces to devices drivers/iommu/iommu.c | 105 ++ include/linux/iommu.h | 71 2 files changed, 176 insertions(+) -- 2.21.0 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/1] iommu: Bind process address spaces to devices
Add bind() and unbind() operations to the IOMMU API.
iommu_sva_bind_device() binds a device to an mm, and returns a handle to
the bond, which is released by calling iommu_sva_unbind_device().
Each mm bound to devices gets a PASID (by convention, a 20-bit system-wide
ID representing the address space), which can be retrieved with
iommu_sva_get_pasid(). When programming DMA addresses, device drivers
include this PASID in a device-specific manner, to let the device access
the given address space. Since the process memory may be paged out, device
and IOMMU must support I/O page faults (e.g. PCI PRI).
Using iommu_sva_set_ops(), device drivers provide an mm_exit() callback
that is called by the IOMMU driver if the process exits before the device
driver called unbind(). In mm_exit(), device driver should disable DMA
from the given context, so that the core IOMMU can reallocate the PASID.
Whether the process exited or nor, the device driver should always release
the handle with unbind().
To use these functions, device driver must first enable the
IOMMU_DEV_FEAT_SVA device feature with iommu_dev_enable_feature().
Signed-off-by: Jean-Philippe Brucker
---
drivers/iommu/iommu.c | 105 ++
include/linux/iommu.h | 71
2 files changed, 176 insertions(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 5aba05fa4964..0ab163f17912 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2355,3 +2355,108 @@ int iommu_aux_get_pasid(struct iommu_domain *domain,
struct device *dev)
return ret;
}
EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
+
+/**
+ * iommu_sva_bind_device() - Bind a process address space to a device
+ * @dev: the device
+ * @mm: the mm to bind, caller must hold a reference to it
+ *
+ * Create a bond between device and address space, allowing the device to
access
+ * the mm using the returned PASID. If a bond already exists between @device
and
+ * @mm, it is returned and an additional reference is taken. Caller must call
+ * iommu_sva_unbind_device() to release each reference.
+ *
+ * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
+ * initialize the required SVA features.
+ *
+ * On error, returns an ERR_PTR value.
+ */
+struct iommu_sva *
+iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, void *drvdata)
+{
+ struct iommu_group *group;
+ struct iommu_sva *handle = ERR_PTR(-EINVAL);
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+ if (!ops || !ops->sva_bind)
+ return ERR_PTR(-ENODEV);
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return ERR_PTR(-ENODEV);
+
+ /* Ensure device count and domain don't change while we're binding */
+ mutex_lock(>mutex);
+
+ /*
+* To keep things simple, SVA currently doesn't support IOMMU groups
+* with more than one device. Existing SVA-capable systems are not
+* affected by the problems that required IOMMU groups (lack of ACS
+* isolation, device ID aliasing and other hardware issues).
+*/
+ if (iommu_group_device_count(group) != 1)
+ goto out_unlock;
+
+ handle = ops->sva_bind(dev, mm, drvdata);
+
+out_unlock:
+ mutex_unlock(>mutex);
+ iommu_group_put(group);
+
+ return handle;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_bind_device);
+
+/**
+ * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device
+ * @handle: the handle returned by iommu_sva_bind_device()
+ *
+ * Put reference to a bond between device and address space. The device should
+ * not be issuing any more transaction for this PASID. All outstanding page
+ * requests for this PASID must have been flushed to the IOMMU.
+ *
+ * Returns 0 on success, or an error value
+ */
+void iommu_sva_unbind_device(struct iommu_sva *handle)
+{
+ struct iommu_group *group;
+ struct device *dev = handle->dev;
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+ if (!ops || !ops->sva_unbind)
+ return;
+
+ group = iommu_group_get(dev);
+ if (!group)
+ return;
+
+ mutex_lock(>mutex);
+ ops->sva_unbind(handle);
+ mutex_unlock(>mutex);
+
+ iommu_group_put(group);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_unbind_device);
+
+int iommu_sva_set_ops(struct iommu_sva *handle,
+ const struct iommu_sva_ops *sva_ops)
+{
+ const struct iommu_ops *ops = handle->dev->bus->iommu_ops;
+
+ if (!ops || !ops->sva_set_ops)
+ return -ENODEV;
+
+ return ops->sva_set_ops(handle, sva_ops);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_set_ops);
+
+int iommu_sva_get_pasid(struct iommu_sva *handle)
+{
+ const struct iommu_ops *ops = handle->dev->bus->iommu_ops;
+
+ if (!ops || !ops->sva_get_pasid)
+ return IOMMU_PASID_INVALID;
+
+ return ops->sva_get_pasid(handle);
+}
[PATCH] svm/avic: iommu/amd: Flush IOMMU IRT after update all entries
When AVIC is enabled and the VM has discrete device assignment,
the interrupt remapping table (IRT) is used to keep track of which
destination APIC ID the IOMMU will inject the device interrput to.
This means every time a vcpu is blocked or context-switched (i.e.
vcpu_blocking/unblocking() and vcpu_load/put()), the information
in IRT must be updated and the IOMMU IRT flush command must be
issued.
The current implementation flushes IOMMU IRT every time an entry
is modified. If the assigned device has large number of interrupts
(hence large number of entries), this would add large amount of
overhead to vcpu context-switch. Instead, this can be optmized by
only flush IRT once per vcpu context-switch per device after all
IRT entries are modified.
The function amd_iommu_update_ga() is refactored to only update
IRT entry, while the amd_iommu_sync_ga() is introduced to allow
IRT flushing to be done separately.
Cc: Joerg Roedel
Cc: Radim Krčmář
Cc: Paolo Bonzini
Signed-off-by: Suravee Suthikulpanit
---
arch/x86/kvm/svm.c| 35 ++-
drivers/iommu/amd_iommu.c | 20 +---
include/linux/amd-iommu.h | 13 ++---
3 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 47c4993448c7..a5c7ca5d70d3 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -118,6 +118,8 @@ MODULE_DEVICE_TABLE(x86cpu, svm_cpu_id);
#define AVIC_GATAG_TO_VMID(x) ((x >> AVIC_VCPU_ID_BITS) &
AVIC_VM_ID_MASK)
#define AVIC_GATAG_TO_VCPUID(x)(x & AVIC_VCPU_ID_MASK)
+#define AVIC_DEVID_BITMAP_SIZE (1 << 16)
+
static bool erratum_383_found __read_mostly;
static const u32 host_save_user_msrs[] = {
@@ -251,6 +253,12 @@ struct vcpu_svm {
/* which host CPU was used for running this vcpu */
unsigned int last_cpu;
+
+ /*
+* Bitmap used to store PCI devid to sync
+* AMD IOMMU interrupt remapping table
+*/
+ unsigned long *avic_devid_sync_bitmap;
};
/*
@@ -1984,6 +1992,7 @@ static inline int
avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
{
int ret = 0;
+ int devid = 0;
unsigned long flags;
struct amd_svm_iommu_ir *ir;
struct vcpu_svm *svm = to_svm(vcpu);
@@ -2001,9 +2010,21 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu,
int cpu, bool r)
goto out;
list_for_each_entry(ir, >ir_list, node) {
- ret = amd_iommu_update_ga(cpu, r, ir->data);
+ ret = amd_iommu_update_ga(cpu, r, ir->data, );
if (ret)
break;
+ set_bit(devid, svm->avic_devid_sync_bitmap);
+ }
+
+ /* Sync AMD IOMMU interrupt remapping table changes for each device. */
+ devid = find_next_bit(svm->avic_devid_sync_bitmap,
+ AVIC_DEVID_BITMAP_SIZE, 0);
+
+ while (devid < AVIC_DEVID_BITMAP_SIZE) {
+ clear_bit(devid, svm->avic_devid_sync_bitmap);
+ ret = amd_iommu_sync_ga(devid);
+ devid = find_next_bit(svm->avic_devid_sync_bitmap,
+ AVIC_DEVID_BITMAP_SIZE, devid+1);
}
out:
spin_unlock_irqrestore(>ir_list_lock, flags);
@@ -2107,6 +2128,13 @@ static int avic_init_vcpu(struct vcpu_svm *svm)
INIT_LIST_HEAD(>ir_list);
spin_lock_init(>ir_list_lock);
+ svm->avic_devid_sync_bitmap = (void *)__get_free_pages(
+ GFP_KERNEL | __GFP_ZERO,
+ get_order(AVIC_DEVID_BITMAP_SIZE/8));
+ if (svm->avic_devid_sync_bitmap == NULL)
+ ret = -ENOMEM;
+ memset(svm->avic_devid_sync_bitmap, 0, AVIC_DEVID_BITMAP_SIZE/8);
+
return ret;
}
@@ -2221,6 +2249,11 @@ static void svm_free_vcpu(struct kvm_vcpu *vcpu)
__free_pages(virt_to_page(svm->msrpm), MSRPM_ALLOC_ORDER);
__free_page(virt_to_page(svm->nested.hsave));
__free_pages(virt_to_page(svm->nested.msrpm), MSRPM_ALLOC_ORDER);
+
+ free_pages((unsigned long)svm->avic_devid_sync_bitmap,
+ get_order(AVIC_DEVID_BITMAP_SIZE/8));
+ svm->avic_devid_sync_bitmap = NULL;
+
kvm_vcpu_uninit(vcpu);
kmem_cache_free(x86_fpu_cache, svm->vcpu.arch.guest_fpu);
kmem_cache_free(kvm_vcpu_cache, svm);
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2a7b78bb98b4..637bcc9192e5 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -4499,7 +4499,20 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
return 0;
}
-int amd_iommu_update_ga(int cpu, bool is_run, void *data)
+int amd_iommu_sync_ga(int devid)
+{
+ struct amd_iommu *iommu = amd_iommu_rlookup_table[devid];
+
+ if (!iommu)
+ return -ENODEV;
+
+ iommu_flush_irt(iommu, devid);
+
Re: [PATCH v7 9/9] vfio/type1: Handle different mdev isolation type
Hi Neo, On 3/9/19 2:03 AM, Neo Jia wrote: On Thu, Mar 07, 2019 at 04:56:23PM -0700, Alex Williamson wrote: On Thu, 7 Mar 2019 00:44:54 -0800 Neo Jia wrote: On Fri, Feb 22, 2019 at 10:19:27AM +0800, Lu Baolu wrote: This adds the support to determine the isolation type of a mediated device group by checking whether it has an iommu device. If an iommu device exists, an iommu domain will be allocated and then attached to the iommu device. Otherwise, keep the same behavior as it is. Cc: Ashok Raj Cc: Jacob Pan Cc: Kevin Tian Signed-off-by: Sanjay Kumar Signed-off-by: Liu Yi L Signed-off-by: Lu Baolu Reviewed-by: Jean-Philippe Brucker --- drivers/vfio/vfio_iommu_type1.c | 48 - 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index ccc4165474aa..f1392c582a3c 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1368,13 +1368,40 @@ static void vfio_iommu_detach_group(struct vfio_domain *domain, iommu_detach_group(domain->domain, group->iommu_group); } Hi Baolu, To allow IOMMU-awared mdev, I think you need to modify the vfio_iommu_type1_pin_pages and vfio_iommu_type1_unpin_pages to remove the iommu->external_domain check. Could you please include that in your patch? If not, I can send out a separate patch to address that issue. I figured it was intentional that an IOMMU backed mdev would not use the pin/unpin interface and therefore the exiting -EINVAL returns would be correct. Can you elaborate on the use case for still requiring the mdev pin/unpin interface for these devices? Thanks, Sure. We are using this api to fetch a pfn of a guest physical address so we can access it for PV. Okay, I will remove these two checks in the next version. Best regards, Lu Baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
