| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9ba29982-ae96-bf53-f021-21cb1b22643a@arm.com>
Date: Thu, 17 Aug 2023 17:24:51 +0100
From: Robin Murphy <robin.murphy@....com>
To: Jason Gunthorpe <jgg@...dia.com>,
Nicolin Chen <nicolinc@...dia.com>
Cc: will@...nel.org, joro@...tes.org, mshavit@...gle.com,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
iommu@...ts.linux.dev
Subject: Re: [PATCH v2] iommu/arm-smmu-v3: Allow default substream bypass with
a pasid support
On 2023-08-17 16:20, Jason Gunthorpe wrote:
> On Wed, Aug 16, 2023 at 09:21:35PM -0700, Nicolin Chen wrote:
>> When an iommu_domain is set to IOMMU_DOMAIN_IDENTITY, the driver sets the
>> arm_smmu_domain->stage to ARM_SMMU_DOMAIN_BYPASS and skips the allocation
>> of a CD table, and then sets STRTAB_STE_0_CFG_BYPASS to the CONFIG field
>> of the STE. This works well for devices that only have one substream, i.e.
>> pasid disabled.
>>
>> With a pasid-capable device, however, there could be a use case where it
>> allows an IDENTITY domain attachment without disabling its pasid feature.
>> This requires the driver to allocate a multi-entry CD table to attach the
>> IDENTITY domain to its default substream and to configure the S1DSS filed
>> of the STE to STRTAB_STE_1_S1DSS_BYPASS. So, there is a missing link here
>> between the STE setup and an IDENTITY domain attachment.
>>
>> Add a new stage ARM_SMMU_DOMAIN_BYPASS_S1DSS to tag this configuration by
>> overriding the ARM_SMMU_DOMAIN_BYPASS if the device has pasid capability.
>> This new tag will allow the driver allocating a CD table yet skipping an
>> CD insertion from the IDENTITY domain, and setting up the STE accordingly.
>>
>> In a use case of ARM_SMMU_DOMAIN_BYPASS_S1DSS, the SHCFG field of the STE
>> should be set to STRTAB_STE_1_SHCFG_INCOMING. In other cases of having a
>> CD table, the shareability comes from a CD, not the SHCFG field: according
>> to "13.5 Summary of attribute/permission configuration fields" in the spec
>> the SHCFG field value is irrelevant. So, always configure the SHCFG field
>> of the STE to STRTAB_STE_1_SHCFG_INCOMING when a CD table is present, for
>> simplification.
>>
>> Signed-off-by: Nicolin Chen <nicolinc@...dia.com>
>> ---
>>
>> Changelog
>> v2:
>> * Rebased on top of Michael's series reworking CD table ownership:
>> https://lore.kernel.org/all/20230816131925.2521220-1-mshavit@google.com/
>> * Added a new ARM_SMMU_DOMAIN_BYPASS_S1DSS stage to tag the use case
>> v1: https://lore.kernel.org/all/20230627033326.5236-1-nicolinc@nvidia.com/
>
> After rebasing there really shouldn't be a
> ARM_SMMU_DOMAIN_BYPASS_S1DSS. I want to get to a model where the
> identity domain is a global static, so it can't be changing depending
> on how it is attched.
>
> I continue to think that the right way to think about this is to have
> the CD table code generate the STE it wants and when doing so it will
> inspect what SSID0 is. If it is the IDENTITY domain then it fills
> s1dss / etc
Indeed, that's what I was getting at with "generalisation of
ARM_SMMU_DOMAIN_BYPASS based on s1cdmax" - just one type all the way
down to the bowels of arm_smmu_write_strtab_ent(), which then decides
whether it means touching S1DSS or Config in the given STE.
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index b27011b2bec9..860db4fbb995 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1271,6 +1271,7 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>> * 3. Update Config, sync
>> */
>> u64 val = le64_to_cpu(dst[0]);
>> + u8 s1dss = STRTAB_STE_1_S1DSS_SSID0;
>> bool ste_live = false;
>> struct arm_smmu_device *smmu = NULL;
>> struct arm_smmu_ctx_desc_cfg *cd_table = NULL;
>> @@ -1290,6 +1291,9 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
>>
>> if (smmu_domain) {
>> switch (smmu_domain->stage) {
>> + case ARM_SMMU_DOMAIN_BYPASS_S1DSS:
>> + s1dss = STRTAB_STE_1_S1DSS_BYPASS;
>> + fallthrough;
>> case ARM_SMMU_DOMAIN_S1:
>> cd_table = &master->cd_table;
>> break;
>
> Eg, I think the code looks much nicer if the logic here is more like:
>
> if (master->cd_table.cdtab)
> arm_smmu_cd_table_get_ste(master->cd_table, &ste)
> else if (master->domain)
> arm_smmu_domain_get_ste(master->domain, &ste);
> else
> ste = not attached
>
> And you'd check in arm_smmu_cd_table_get_ste() to learn the CD
> parameters and also what SSID=0 is. If SSID=0 is IDENTITY then
> arm_smmu_cd_table_get_ste would return with S1DSS set.
>
> arm_smmu_domain_get_ste() would multiplex based on the domain type.
>
>> @@ -2435,6 +2440,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>> } else if (smmu_domain->smmu != smmu)
>> ret = -EINVAL;
>>
>> + /*
>> + * When attaching an IDENTITY domain to a master with pasid capability,
>> + * the master can still enable SVA feature by allocating a multi-entry
>> + * CD table and attaching the IDENTITY domain to its default substream
>> + * that alone can be byassed using the S1DSS field of the STE.
>> + */
>> + if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS && master->ssid_bits &&
>> + smmu->features & ARM_SMMU_FEAT_TRANS_S1)
>> + smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS_S1DSS;
>
> Then you don't technically need to do this.
>
> Though if we can't atomically change the STE from IDENTITY to IDENTIY
> with a CD then you still have to do something here,
Strictly I think we are safe to do that - fill in all the S1* fields
while Config[0] is still 0 and they're ignored, sync, then set
Config[0]. Adding a CD table under a translation domain should be
achievable as well, since S1CDMax, S1ContextPtr and S1Fmt can all be
updated together atomically (although it's still the kind of switcheroo
where I'd be scared of a massive boulder suddenly rolling out of the
ceiling...)
> but really what we
> want is to force a CD table for all cases if PASID is enabled, and
> force DMA domains to be S2 domains as well.
Wut? No, DMA domains really want to be stage 1, for many reasons.
Implementing them with stage 2 when stage 1 isn't supported was always a
bit of a bodge, but thankfully I'm not aware of anyone ever building a
stage-2-only SMMUv3 anyway.
The most glaringly obvious one, though, is that I think people like
PASID support and SVA to actually work ;)
Thanks,
Robin.
>> mutex_unlock(&smmu_domain->init_mutex);
>> if (ret)
>> return ret;
>> @@ -2456,7 +2471,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>> list_add(&master->domain_head, &smmu_domain->devices);
>> spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>>
>> - if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
>> + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 ||
>> + smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS_S1DSS) {
>> if (!master->cd_table.cdtab) {
>> ret = arm_smmu_alloc_cd_tables(master);
>> if (ret) {
>
> So more like:
>
> if (smmu_domain == IDENTIY && arm_smmu_support_ssid(dev))
> arm_smmu_alloc_cd_tables()
>
> Jason
Powered by blists - more mailing lists