2021-02-24 00:15:41

by Eric Auger

[permalink] [raw]
Subject: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than one context descriptor

In preparation for vSVA, let's accept userspace provided configs
with more than one CD. We check the max CD against the host iommu
capability and also the format (linear versus 2 level).

Signed-off-by: Eric Auger <[email protected]>
Signed-off-by: Shameer Kolothum <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)

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 332d31c0680f..ab74a0289893 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3038,14 +3038,17 @@ static int arm_smmu_attach_pasid_table(struct iommu_domain *domain,
if (smmu_domain->s1_cfg.set)
goto out;

- /*
- * we currently support a single CD so s1fmt and s1dss
- * fields are also ignored
- */
- if (cfg->pasid_bits)
+ list_for_each_entry(master, &smmu_domain->devices, domain_head) {
+ if (cfg->pasid_bits > master->ssid_bits)
+ goto out;
+ }
+ if (cfg->vendor_data.smmuv3.s1fmt == STRTAB_STE_0_S1FMT_64K_L2 &&
+ !(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB))
goto out;

smmu_domain->s1_cfg.cdcfg.cdtab_dma = cfg->base_ptr;
+ smmu_domain->s1_cfg.s1cdmax = cfg->pasid_bits;
+ smmu_domain->s1_cfg.s1fmt = cfg->vendor_data.smmuv3.s1fmt;
smmu_domain->s1_cfg.set = true;
smmu_domain->abort = false;
break;
--
2.26.2


2021-03-30 09:27:48

by Zenghui Yu

[permalink] [raw]
Subject: Re: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than one context descriptor

Hi Eric,

On 2021/2/24 4:56, Eric Auger wrote:
> In preparation for vSVA, let's accept userspace provided configs
> with more than one CD. We check the max CD against the host iommu
> capability and also the format (linear versus 2 level).
>
> Signed-off-by: Eric Auger <[email protected]>
> Signed-off-by: Shameer Kolothum <[email protected]>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> 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 332d31c0680f..ab74a0289893 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3038,14 +3038,17 @@ static int arm_smmu_attach_pasid_table(struct iommu_domain *domain,
> if (smmu_domain->s1_cfg.set)
> goto out;
>
> - /*
> - * we currently support a single CD so s1fmt and s1dss
> - * fields are also ignored
> - */
> - if (cfg->pasid_bits)
> + list_for_each_entry(master, &smmu_domain->devices, domain_head) {
> + if (cfg->pasid_bits > master->ssid_bits)
> + goto out;
> + }
> + if (cfg->vendor_data.smmuv3.s1fmt == STRTAB_STE_0_S1FMT_64K_L2 &&
> + !(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB))
> goto out;
>
> smmu_domain->s1_cfg.cdcfg.cdtab_dma = cfg->base_ptr;
> + smmu_domain->s1_cfg.s1cdmax = cfg->pasid_bits;
> + smmu_domain->s1_cfg.s1fmt = cfg->vendor_data.smmuv3.s1fmt;

And what about the SIDSS field?

Subject: RE: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than one context descriptor



> -----Original Message-----
> From: Auger Eric [mailto:[email protected]]
> Sent: 01 April 2021 12:49
> To: yuzenghui <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; zhukeqian <[email protected]>;
> [email protected]; [email protected]; wangxingang
> <[email protected]>; jiangkunkun <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; Shameerali Kolothum Thodi
> <[email protected]>; [email protected];
> lushenming <[email protected]>; [email protected]; Wanghaibin (D)
> <[email protected]>
> Subject: Re: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than
> one context descriptor
>
> Hi Zenghui,
>
> On 3/30/21 11:23 AM, Zenghui Yu wrote:
> > Hi Eric,
> >
> > On 2021/2/24 4:56, Eric Auger wrote:
> >> In preparation for vSVA, let's accept userspace provided configs
> >> with more than one CD. We check the max CD against the host iommu
> >> capability and also the format (linear versus 2 level).
> >>
> >> Signed-off-by: Eric Auger <[email protected]>
> >> Signed-off-by: Shameer Kolothum
> <[email protected]>
> >> ---
> >>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
> >>   1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> 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 332d31c0680f..ab74a0289893 100644
> >> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >> @@ -3038,14 +3038,17 @@ static int
> arm_smmu_attach_pasid_table(struct
> >> iommu_domain *domain,
> >>           if (smmu_domain->s1_cfg.set)
> >>               goto out;
> >>   -        /*
> >> -         * we currently support a single CD so s1fmt and s1dss
> >> -         * fields are also ignored
> >> -         */
> >> -        if (cfg->pasid_bits)
> >> +        list_for_each_entry(master, &smmu_domain->devices,
> >> domain_head) {
> >> +            if (cfg->pasid_bits > master->ssid_bits)
> >> +                goto out;
> >> +        }
> >> +        if (cfg->vendor_data.smmuv3.s1fmt ==
> >> STRTAB_STE_0_S1FMT_64K_L2 &&
> >> +                !(smmu->features &
> ARM_SMMU_FEAT_2_LVL_CDTAB))
> >>               goto out;
> >>             smmu_domain->s1_cfg.cdcfg.cdtab_dma = cfg->base_ptr;
> >> +        smmu_domain->s1_cfg.s1cdmax = cfg->pasid_bits;
> >> +        smmu_domain->s1_cfg.s1fmt =
> cfg->vendor_data.smmuv3.s1fmt;
> >
> > And what about the SIDSS field?
> >
> I added this patch upon Shameer's request, to be more vSVA friendly.
> Hower this series does not really target multiple CD support. At the
> moment the driver only supports STRTAB_STE_1_S1DSS_SSID0 (0x2) I think.
> At this moment maybe I can only check the s1dss field is 0x2. Or simply
> removes this patch?
>
> Thoughts?

Right. This was useful for vSVA tests. But yes, to properly support multiple CDs
we need to pass the S1DSS from Qemu. And that requires further changes.
So I think it's better to remove this patch and reject S1CDMAX != 0 cases.

Thanks,
Shameer

>
> Eric

2021-04-01 18:38:45

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than one context descriptor

Hi Zenghui,

On 3/30/21 11:23 AM, Zenghui Yu wrote:
> Hi Eric,
>
> On 2021/2/24 4:56, Eric Auger wrote:
>> In preparation for vSVA, let's accept userspace provided configs
>> with more than one CD. We check the max CD against the host iommu
>> capability and also the format (linear versus 2 level).
>>
>> Signed-off-by: Eric Auger <[email protected]>
>> Signed-off-by: Shameer Kolothum <[email protected]>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> 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 332d31c0680f..ab74a0289893 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -3038,14 +3038,17 @@ static int arm_smmu_attach_pasid_table(struct
>> iommu_domain *domain,
>>           if (smmu_domain->s1_cfg.set)
>>               goto out;
>>   -        /*
>> -         * we currently support a single CD so s1fmt and s1dss
>> -         * fields are also ignored
>> -         */
>> -        if (cfg->pasid_bits)
>> +        list_for_each_entry(master, &smmu_domain->devices,
>> domain_head) {
>> +            if (cfg->pasid_bits > master->ssid_bits)
>> +                goto out;
>> +        }
>> +        if (cfg->vendor_data.smmuv3.s1fmt ==
>> STRTAB_STE_0_S1FMT_64K_L2 &&
>> +                !(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB))
>>               goto out;
>>             smmu_domain->s1_cfg.cdcfg.cdtab_dma = cfg->base_ptr;
>> +        smmu_domain->s1_cfg.s1cdmax = cfg->pasid_bits;
>> +        smmu_domain->s1_cfg.s1fmt = cfg->vendor_data.smmuv3.s1fmt;
>
> And what about the SIDSS field?
>
I added this patch upon Shameer's request, to be more vSVA friendly.
Hower this series does not really target multiple CD support. At the
moment the driver only supports STRTAB_STE_1_S1DSS_SSID0 (0x2) I think.
At this moment maybe I can only check the s1dss field is 0x2. Or simply
removes this patch?

Thoughts?

Eric

2021-04-01 18:46:11

by Eric Auger

[permalink] [raw]
Subject: Re: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than one context descriptor

Hi Shameer,
On 4/1/21 2:38 PM, Shameerali Kolothum Thodi wrote:
>
>
>> -----Original Message-----
>> From: Auger Eric [mailto:[email protected]]
>> Sent: 01 April 2021 12:49
>> To: yuzenghui <[email protected]>
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; zhukeqian <[email protected]>;
>> [email protected]; [email protected]; wangxingang
>> <[email protected]>; jiangkunkun <[email protected]>;
>> [email protected]; [email protected]; [email protected];
>> [email protected]; Shameerali Kolothum Thodi
>> <[email protected]>; [email protected];
>> lushenming <[email protected]>; [email protected]; Wanghaibin (D)
>> <[email protected]>
>> Subject: Re: [PATCH v14 13/13] iommu/smmuv3: Accept configs with more than
>> one context descriptor
>>
>> Hi Zenghui,
>>
>> On 3/30/21 11:23 AM, Zenghui Yu wrote:
>>> Hi Eric,
>>>
>>> On 2021/2/24 4:56, Eric Auger wrote:
>>>> In preparation for vSVA, let's accept userspace provided configs
>>>> with more than one CD. We check the max CD against the host iommu
>>>> capability and also the format (linear versus 2 level).
>>>>
>>>> Signed-off-by: Eric Auger <[email protected]>
>>>> Signed-off-by: Shameer Kolothum
>> <[email protected]>
>>>> ---
>>>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++-----
>>>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> 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 332d31c0680f..ab74a0289893 100644
>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> @@ -3038,14 +3038,17 @@ static int
>> arm_smmu_attach_pasid_table(struct
>>>> iommu_domain *domain,
>>>>           if (smmu_domain->s1_cfg.set)
>>>>               goto out;
>>>>   -        /*
>>>> -         * we currently support a single CD so s1fmt and s1dss
>>>> -         * fields are also ignored
>>>> -         */
>>>> -        if (cfg->pasid_bits)
>>>> +        list_for_each_entry(master, &smmu_domain->devices,
>>>> domain_head) {
>>>> +            if (cfg->pasid_bits > master->ssid_bits)
>>>> +                goto out;
>>>> +        }
>>>> +        if (cfg->vendor_data.smmuv3.s1fmt ==
>>>> STRTAB_STE_0_S1FMT_64K_L2 &&
>>>> +                !(smmu->features &
>> ARM_SMMU_FEAT_2_LVL_CDTAB))
>>>>               goto out;
>>>>             smmu_domain->s1_cfg.cdcfg.cdtab_dma = cfg->base_ptr;
>>>> +        smmu_domain->s1_cfg.s1cdmax = cfg->pasid_bits;
>>>> +        smmu_domain->s1_cfg.s1fmt =
>> cfg->vendor_data.smmuv3.s1fmt;
>>>
>>> And what about the SIDSS field?
>>>
>> I added this patch upon Shameer's request, to be more vSVA friendly.
>> Hower this series does not really target multiple CD support. At the
>> moment the driver only supports STRTAB_STE_1_S1DSS_SSID0 (0x2) I think.
>> At this moment maybe I can only check the s1dss field is 0x2. Or simply
>> removes this patch?
>>
>> Thoughts?
>
> Right. This was useful for vSVA tests. But yes, to properly support multiple CDs
> we need to pass the S1DSS from Qemu. And that requires further changes.
> So I think it's better to remove this patch and reject S1CDMAX != 0 cases.
OK I will remove it

Thanks

Eric
>
> Thanks,
> Shameer
>
>>
>> Eric
>