Re: [RFC PATCH v1 2/7] iommu/arm: ipmmu-vmsa: Add Xen changes for main driver

[Julien Grall <julien.grall@arm.com>]

Hi,

On 10/08/17 15:27, Oleksandr Tyshchenko wrote:
> On Tue, Aug 8, 2017 at 2:34 PM, Julien Grall <julien.grall@arm.com> wrote:
>> On 26/07/17 16:09, Oleksandr Tyshchenko wrote:
>>> @@ -355,6 +557,10 @@ static struct hw_register
>>> *root_pgtable[IPMMU_CTX_MAX] = {
>>>
>>>  static bool ipmmu_is_root(struct ipmmu_vmsa_device *mmu)
>>>  {
>>> +       /* Xen: Fix */
>>
>>
>> Hmmm. Can we get a bit more details?
>
> These is a case when ipmmu_is_root is called with "mmu" being NULL.
> https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L2330
>
> In ipmmu_vmsa_alloc_page_table() we need to find "root mmu", but we
> doesn't have argument to pass.
> So, I had two options:
>
> 1. Add code searching for it.
> ...
> spin_lock(&ipmmu_devices_lock);
> list_for_each_entry(mmu, &ipmmu_devices, list) {
>    if (ipmmu_is_root(mmu))
>       break;
> }
> spin_unlock(&ipmmu_devices_lock);
> ...
>
> 2. Use existing ipmmu_find_root() with adding this check for a valid value.
> So, if we call ipmmu_find_root() with argument being NULL we will
> actually get searching the list.
>
> I decided to use 2 option.

Can you please expand the comment then?

>
>>
>>> +       if (!mmu)
>>> +               return false;
>>> +
>>>         if (mmu->features->has_cache_leaf_nodes)
>>>                 return mmu->is_leaf ? false : true;
>>>         else
>>> @@ -405,14 +611,28 @@ static void ipmmu_ctx_write(struct ipmmu_vmsa_domain
>>> *domain, unsigned int reg,
>>>         ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg,
>>> data);
>>>  }
>>>
>>> -static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned
>>> int reg,
>>> +/* Xen: Write the context for cache IPMMU only. */
>>
>>
>> Same here. Why does it need to be different with Xen?
>
> Well, let me elaborate a bit more about this.
>
> I feel that I need to explain in a few words about IPMMU itself:
> Generally speaking,
> The IPMMU hardware (R-Car Gen3) has 8 context banks and consists of next parts:
> - root IPMMU
> - a number of cache IPMMUs
>
> Each cache IPMMU is connected to root IPMMU and has uTLB ports the
> master devices can be tied to.
> Something, like this:
>
> master device1 ---> cache IPMMU1 [8 ctx] ---> root IPMMU [8 ctx] -> memory
>                            |                                          |
> master device2 --                                          |
>                                                                       |
> master device3 ---> cache IPMMU2 [8 ctx] --
>
> Each context bank has registers.
> Some registers exist for both root IPMMU and cache IPMMUs -> IMCTR
> Some registers exist only for root IPMMU -> IMTTLBRx/IMTTUBRx, IMMAIR0, etc
>
> So, original driver has two helpers:
> 1. ipmmu_ctx_write() - is intended to write a register in context bank
> N* for root IPMMU only.
> 2. ipmmu_ctx_write2() - is intended to write a register in context
> bank N for both root IPMMU and cache IPMMU.
> *where N=0-7
>
> AFAIU, original Linux driver provides each IOMMU domain with a
> separate IPMMU context:
> master device1 + master device2 are in IOMMU domain1 and use IPMMU context 0
> master device3 is in IOMMU domain2 and uses IPMMU context 1
>
> So, when attaching device to new IOMMU domain in Linux we have to
> initialize context for root IPMMU and enable context (IMCTR register)
> for both root and cache IPMMUs.
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/drivers/iommu/ipmmu-vmsa.c?h=v4.9/rcar-3.5.3#n620
>
> In Xen we need additional helper "ipmmu_ctx_write1" for writing a
> register in context bank N for cache IPMMU only.
> The reason is that we need a way to control cache IPMMU separately
> since we have a little bit another model.
>
> All IOMMU domains within a single Xen domain (dom_iommu(d)->arch.priv)
> use the same IPMMU context N
> which was initialized and enabled at the domain creation time. This
> means that all master devices
> that are assigned to the guest domain "d" use only this IPMMU context
> N which actually contains P2M mapping for domain "d":
> master device1 + master device2 are in IOMMU domain1 and use IPMMU context 0
> master device3 is in IOMMU domain2 and also uses IPMMU context 0
>
> So, when attaching device to new IOMMU domain in Xen we don't have to
> initialize and enable context,
> because it has been already done at domain initialization time:
> https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L2380
> we just have to enable context for corresponding cache IPMMU only:
> https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L1083
>
> This is the main difference between drivers in Linux and Xen.
>
> So, as you can see there is a need to manipulate context registers for
> cache IPMMU without touching root IPMMU,
> that's why I added this helper.
>
> Does this make sense?

I think it does.

>
>>
>>
>>> +static void ipmmu_ctx_write1(struct ipmmu_vmsa_domain *domain, unsigned
>>> int reg,
>>>                              u32 data)
>>>  {
>>>         if (domain->mmu != domain->root)
>>> -               ipmmu_write(domain->mmu,
>>> -                           domain->context_id * IM_CTX_SIZE + reg, data);
>>> +               ipmmu_write(domain->mmu, domain->context_id * IM_CTX_SIZE
>>> + reg, data);
>>> +}
>>>
>>> -       ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg,
>>> data);
>>> +/*
>>> + * Xen: Write the context for both root IPMMU and all cache IPMMUs
>>> + * that assigned to this Xen domain.
>>> + */
>>> +static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned
>>> int reg,
>>> +                            u32 data)
>>> +{
>>> +       struct ipmmu_vmsa_xen_domain *xen_domain =
>>> dom_iommu(domain->d)->arch.priv;
>>> +       struct iommu_domain *io_domain;
>>> +
>>> +       list_for_each_entry(io_domain, &xen_domain->contexts, list)
>>> +               ipmmu_ctx_write1(to_vmsa_domain(io_domain), reg, data);
>>> +
>>> +       ipmmu_ctx_write(domain, reg, data);
>>>  }
>>>
>>>  /*
>>> -----------------------------------------------------------------------------
>>> @@ -488,6 +708,10 @@ static void ipmmu_tlb_flush_all(void *cookie)
>>>  {
>>>         struct ipmmu_vmsa_domain *domain = cookie;
>>>
>>> +       /* Xen: Just return if context_id has non-existent value */
>>
>>
>> Same here.
>
> I think that there is a possible race.
> In ipmmu_domain_init_context() we are trying to allocate context and
> if allocation fails we will call free_io_pgtable_ops(),
> but "domain->context_id" hasn't been initialized yet (likely it is 0).
> https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L799
>
> And having following call stack:
> free_io_pgtable_ops() -> io_pgtable_tlb_flush_all() ->
> ipmmu_tlb_flush_all() -> ipmmu_tlb_invalidate()
> we will get a mistaken cache flush for a context pointed by
> uninitialized "domain->context_id".
>
> That's why I initialized context_id with non-existent value before
> allocating context
> https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L792
> and checked it for a valid value here
> https://github.com/otyshchenko1/xen/blob/fc231a0f2edb3d01d178fb5c27dd6c1065807c81/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L712
> and everywhere it is need to checked.

The race is in the code added or the one from Linux? If the latter, then 
you should have an action to fix it there. If the former, the I'd like 
to understand how come we introduced a race compare to Linux.

[...]

>>
>>> +
>>>         /*
>>>          * Find an unused context.
>>>          */
>>> @@ -578,6 +807,11 @@ static int ipmmu_domain_init_context(struct
>>> ipmmu_vmsa_domain *domain)
>>>
>>>         /* TTBR0 */
>>>         ttbr = domain->cfg.arm_lpae_s1_cfg.ttbr[0];
>>> +
>>> +       /* Xen: */
>>> +       dev_notice(domain->root->dev, "d%d: Set IPMMU context %u (pgd
>>> 0x%"PRIx64")\n",
>>> +                       domain->d->domain_id, domain->context_id, ttbr);
>>
>>
>> If you want to keep driver close to Linux, then you need to avoid unecessary
>> change.
> Shall I drop it?

Depends. How useful is it? If it is, then may you want to upstream that?

[...]

>>>  static int ipmmu_attach_device(struct iommu_domain *io_domain,
>>>                                struct device *dev)
>>> @@ -787,7 +1042,20 @@ static int ipmmu_attach_device(struct iommu_domain
>>> *io_domain,
>>>                 /* The domain hasn't been used yet, initialize it. */
>>>                 domain->mmu = mmu;
>>>                 domain->root = root;
>>> +
>>> +/*
>>> + * Xen: We have already initialized and enabled context for root IPMMU
>>> + * for this Xen domain. Enable context for given cache IPMMU only.
>>> + * Flush the TLB as required when modifying the context registers.
>>
>>
>> Why?
>
> Original Linux driver provides each IOMMU domain with a separate IPMMU context.
> So, when attaching device to IOMMU domain which hasn't been
> initialized yet we have to
> call ipmmu_domain_init_context() for initializing (root only) and
> enabling (root + cache * ) context for this IOMMU domain.
>
> * You can see at the end of the "original" ipmmu_domain_init_context()
> implementation, that context is enabled for both cache and root IPMMUs
> because of "ipmmu_ctx_write2".
> https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/tree/drivers/iommu/ipmmu-vmsa.c?h=v4.9/rcar-3.5.3#n620
>
> From my point of view, we don't have to do the same when we are
> attaching device in Xen, as we keep only one IPMMU context (P2M
> mappings) per Xen domain
> for using by all assigned to this guest devices.
> What is more a number of context banks is limited (8), and if we
> followed Linux way here, we would be quickly run out of available
> contexts.
> But having one IPMMU context per Xen domain allow us to passthrough
> devices to 8 guest domain.

The way you describe it give an impression that the driver is 
fundamentally different in Xen compare to Linux. Am I right?

>
> Taking into the account described above, we initialize (root only) and
> enable (root only ** ) context at the domain creation time
> if IOMMU is expected to be used for this guest.
> https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L2380
>
> ** You can see at the end of the "modified"
> ipmmu_domain_init_context() implementation, that context is enabled
> for root IPMMU only
> because of "ipmmu_ctx_write".
> https://github.com/otyshchenko1/xen/blob/ipmmu_v2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L882
>
> That's why, here, in ipmmu_attach_device() we don't have to call
> ipmmu_domain_init_context() anymore, because
> the context has been already initialized and enabled. All what we need
> here is to enable this context for cache IPMMU the device
> is physically connected to.
>
> Does this make sense?
>
>>
>>
>>> + */
>>> +#if 0
>>>                 ret = ipmmu_domain_init_context(domain);
>>> +#endif
>>> +               ipmmu_ctx_write1(domain, IMCTR,
>>> +                               ipmmu_ctx_read(domain, IMCTR) |
>>> IMCTR_FLUSH);
>>> +
>>> +               dev_info(dev, "Using IPMMU context %u\n",
>>> domain->context_id);
>>> +#if 0 /* Xen: Not needed */
>>>                 if (ret < 0) {
>>>                         dev_err(dev, "Unable to initialize IPMMU
>>> context\n");
>>>                         domain->mmu = NULL;
>>> @@ -795,6 +1063,7 @@ static int ipmmu_attach_device(struct iommu_domain
>>> *io_domain,
>>>                         dev_info(dev, "Using IPMMU context %u\n",
>>>                                  domain->context_id);
>>>                 }
>>> +#endif
>>>         } else if (domain->mmu != mmu) {
>>>                 /*
>>>                  * Something is wrong, we can't attach two devices using
>>> @@ -834,6 +1103,14 @@ static void ipmmu_detach_device(struct iommu_domain
>>> *io_domain,
>>>          */
>>>  }
>>>
>>> +/*
>>> + * Xen: The current implementation of these callbacks is insufficient for
>>> us
>>> + * since they are intended to be called from Linux IOMMU core that
>>> + * has already done all required actions such as doing various checks,
>>> + * splitting into memory block the hardware supports and so on.
>>
>>
>> Can you expand it here? Why can't our IOMMU framework could do that?
>
> If add all required support to IOMMU framework and modify all existing
> IOMMU drivers
> to follow this support, then yes, it will avoid IOMMU drivers such as
> IPMMU-VMSA from having these stuff in.
>
> To be honest, I was trying to touch IOMMU common code and other IOMMU
> drivers as little as possible,
> but I had to introduce a few changes ("non-shared IOMMU").

What I am looking is something we can easily maintain in the future. If 
it requires change in the common code then we should do it. If it 
happens to be too complex, then maybe we should not take it from Linux.

>
>>
>> IHMO, if we want to get driver from Linux, we need to get an interface very
>> close to it. Otherwise it is not worth it because you would have to
>> implement for each IOMMU.
> You are right.
>
>>
>> My overall feeling at the moment is Xen is not ready to welcome this driver
>> directly from Linux. This is also a BSP driver, so no thorough review done
>> by the community.
>
> As I said in a cover letter the BSP driver had more complete support
> than the mainline one.

I know. But this means we are going to bring code in Xen that was not 
fully reviewed and don't know the quality of the code.

> I would like to clarify what need to be done from my side.
> Should I wait for the missing things reach upsteam and then rebase on
> the mainline driver?
> Or should I rewrite this driver without following Linux?

I don't have a clear answer here. As I said, we need to weight pros and 
cons to use Linux driver over our own.

At the moment, you are using a BSP driver which has more features but 
modified quite a lot. We don't even know when this is going to be merged 
in Linux.

Keeping code close to Linux requires some hacks that are acceptable if 
you can benefits from the community (bug fix, review...). As the driver 
is taken from the BSP, we don't know if the code will stay in the 
current form nor be able to get bug fix.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel