Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1595668

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

Show all headers | View raw

Hi Robin,

Thanks for your feedback!

On Wed, Mar 8, 2017 at 9:21 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 08/03/17 11:01, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> 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@opensource.se>
>> ---
>>
>>  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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 00/09] iommu/ipmmu-vmsa: r8a7795 support V3 Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:10 +0100
  [PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:10 +0100
    Re: [PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out  alias Robin Murphy <robin.murphy@arm.com> - 2017-03-08 13:30 +0100
      Re: [PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias Magnus Damm <magnus.damm@gmail.com> - 2017-03-09 05:30 +0100
  [PATCH v3 08/09] iommu/ipmmu-vmsa: Allow two bit SL0 Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:20 +0100
  [PATCH v3 07/09] iommu/ipmmu-vmsa: Make IMBUSCTR setup optional Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:20 +0100
  [PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:20 +0100
    Re: [PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support Robin Murphy <robin.murphy@arm.com> - 2017-03-08 14:10 +0100
      Re: [PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support Magnus Damm <magnus.damm@gmail.com> - 2017-03-09 05:20 +0100
  [PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:20 +0100
    Re: [PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice Robin Murphy <robin.murphy@arm.com> - 2017-03-08 13:40 +0100
      Re: [PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice Magnus Damm <magnus.damm@gmail.com> - 2017-03-09 05:00 +0100
  [PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:20 +0100
    Re: [PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-08 15:10 +0100
      Re: [PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code Magnus Damm <magnus.damm@gmail.com> - 2017-03-09 05:30 +0100
  [PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE() Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 12:40 +0100
    Re: [PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE() Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-08 15:00 +0100
      Re: [PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE() Magnus Damm <magnus.damm@gmail.com> - 2017-03-09 05:00 +0100
  [PATCH v3 05/09] iommu/ipmmu-vmsa: IPMMU device is 40-bit bus master Magnus Damm <magnus.damm@gmail.com> - 2017-03-08 13:20 +0100

csiph-web