Re: [PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support

[Magnus Damm]

Hi Robin,

Thanks for your feedback!

On Wed, Mar 8, 2017 at 9:21 PM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
> On 08/03/17 11:01, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>>
>> Add support for up to 8 contexts. Each context is mapped to one
>> domain. One domain is assigned one or more slave devices. Contexts
>> are allocated dynamically and slave devices are grouped together
>> based on which IPMMU device they are connected to. This makes slave
>> devices tied to the same IPMMU device share the same IOVA space.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@xxxxxxxxxxxxx>
>> ---
>>
>>  Changes since V2:
>>  - Updated patch description to reflect code included in:
>>    [PATCH v7 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V7
>>
>>  Changes since V1:
>>  - Support up to 8 contexts instead of 4
>>  - Use feature flag and runtime handling
>>  - Default to single context
>>
>>  drivers/iommu/ipmmu-vmsa.c |   38 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 30 insertions(+), 8 deletions(-)
>>
>> --- 0012/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2017-03-08 17:59:19.900607110 +0900
>> @@ -30,11 +30,12 @@
>>
>>  #include "io-pgtable.h"
>>
>> -#define IPMMU_CTX_MAX 1
>> +#define IPMMU_CTX_MAX 8
>>
>>  struct ipmmu_features {
>>       bool use_ns_alias_offset;
>>       bool has_cache_leaf_nodes;
>> +     bool has_eight_ctx;
>
> Wouldn't it be more sensible to just encode a number of contexts
> directly, if it isn't reported by the hardware itself? I'm just
> imagining future hardware generations... :P
>
> bool also_has_another_eight_ctx_on_top_of_that;
> bool wait_no_this_is_the_one_where_ctx_15_isnt_usable;

=)

Sure, I agree with you!

Please note that this is currently a mix of software and hardware
policy. On R-Car Gen2 (ARM32) the legacy code only uses a single
context for now but 4 contexts are supported by hardware according to
the data sheet. The remaining 3 contexts are untested at this point.
For R-Car Gen3 (ARM64) the hardware supports 8 contexts and this patch
enables all of them.

>>  };
>>
>>  struct ipmmu_vmsa_device {
>> @@ -44,6 +45,7 @@ struct ipmmu_vmsa_device {
>>       const struct ipmmu_features *features;
>>       bool is_leaf;
>>       unsigned int num_utlbs;
>> +     unsigned int num_ctx;
>>       spinlock_t lock;                        /* Protects ctx and domains[] */
>>       DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>>       struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
>> @@ -376,11 +378,12 @@ static int ipmmu_domain_allocate_context
>>
>>       spin_lock_irqsave(&mmu->lock, flags);
>>
>> -     ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
>> -     if (ret != IPMMU_CTX_MAX) {
>> +     ret = find_first_zero_bit(mmu->ctx, mmu->num_ctx);
>> +     if (ret != mmu->num_ctx) {
>>               mmu->domains[ret] = domain;
>>               set_bit(ret, mmu->ctx);
>
> Using test_and_set_bit() in a loop would avoid having to take a lock here.

So you mean that in case of test_and_set_bit() returns 1 then we try
find_first_zero_bit() again?

This is not really a performance sensitive part of the driver, so I'm
currently optimizing for code readability. I'm of course all for
dropping the lock, but I have a hard time figuring out how your
suggestion could result in semi-readable code. Any pointers? =)

>> @@ -1112,6 +1123,17 @@ static int ipmmu_probe(struct platform_d
>>       if (mmu->features->use_ns_alias_offset)
>>               mmu->base += IM_NS_ALIAS_OFFSET;
>>
>> +     /*
>> +      * The number of contexts varies with generation and instance.
>> +      * Newer SoCs get a total of 8 contexts enabled, older ones just one.
>> +      */
>> +     if (mmu->features->has_eight_ctx)
>> +             mmu->num_ctx = 8;
>> +     else
>> +             mmu->num_ctx = 1;
>> +
>> +     WARN_ON(mmu->num_ctx > IPMMU_CTX_MAX);
>
> The likelihood of that happening doesn't appear to warrant a runtime
> check. Especially one which probably isn't even generated because it's
> trivially resolvable to "if (false)..." at compile time.

Sure, I agree. Will drop.

Thanks,

/ magnus