Re: [Xen-devel] [PATCH V1] iommu/arm: Add Renesas IPMMU-VMSA support

[Oleksandr <olekstysh@xxxxxxxxx>]

On 23.07.19 16:36, Julien Grall wrote:
> Hi Oleksandr,
Hi, Julien.


> > > On 6/26/19 11:30 AM, Oleksandr Tyshchenko wrote:
> > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > >
> > > > The IPMMU-VMSA is VMSA-compatible I/O Memory Management Unit (IOMMU)
> > > > which provides address translation and access protection 
> > > > functionalities
> > > > to processing units and interconnect networks.
> > > Do you have a link to the specification?
> > All I have is a TRM. Unfortunately, I can't share it.
> Does anyone in the community has access to the spec?
Yes. I believe, there are persons from the Linux community who have 
access to the spec. I have already asked for review.

> > > What are the major differences compare the Linux driver?
> > Well, the major differences are:
> >
> > 1. Stage 1/Stage 2 translation. Linux driver supports Stage 1 
> > translation only (with Stage 1 translation table format). It manages 
> > page table by itself. But Xen driver supports Stage 2 translation 
> > (with Stage 2 translation table format) to be able to share the page 
> > table with the CPU. Stage 1 translation is always bypassed in Xen 
> > driver.
> > So, Xen driver is supposed to be used with newest Gen3 SoC revisions 
> > only (H3 ES3.0, M3 ES3.0, etc.) which IPMMU hardware does support 
> > stage 2 translation table format.
> > 2. AArch64 support. Linux driver uses VMSAv8-32 mode, while Xen 
> > driver enables Armv8 VMSAv8-64 mode to cover up to 40 bit input address.
> > 3. Context bank (sets of page table) usage. In Xen, each context bank 
> > is mapped to one Xen domain.  So, all devices being pass throughed to 
> > the same Xen domain share the same context bank.
> Can this be written in the commit message? This is helpful for anyone 
> reviewing the driver today and future developer.
Sure. Will update.


> > > > + * you can found at:
> > > > + *    url: 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git
> > > > + *    branch: v4.14.75-ltsi/rcar-3.9.2
> > > > + *    commit: a5266d298124874c2c06b8b13d073f6ecc2ee355
> > > Is there any reason to use the BSP driver and not the one provided 
> > > by Linux directly?
> > I was thinking the BSP driver is a *little bit* more updated than 
> > Linux one. Sometime it was a big difference between mainline and BSP 
> > driver. But now
> > the difference is not big and mostly in DDR_BACKUP and WHITELIST 
> > support. I looked at mainline driver as well when implementing Xen 
> > driver.
> What is the review process for patches to be merged in the BSP? Is it 
> the same as Linux upstream?
I don't know at the moment, I will try to clarify this question.


> > It can be >= 0.
> Ok, so please switch to unsigned int here please.
ok


> > > > +{
> > > > +    return readl(mmu->base + offset);
> > > > +}
> > > > +
> > > > +static void ipmmu_write(struct ipmmu_vmsa_device *mmu, unsigned 
> > > > int offset,
> > > > +                        u32 data)
> > > > +{
> > > > +    writel(data, mmu->base + offset);
> > > > +}
> > > > +
> > > > +static u32 ipmmu_ctx_read_root(struct ipmmu_vmsa_domain *domain,
> > > > +                               unsigned int reg)
> > > > +{
> > > > +    return ipmmu_read(domain->mmu->root,
> > > > +                      domain->context_id * IM_CTX_SIZE + reg);
> > > > +}
> > > > +
> > > > +static void ipmmu_ctx_write_root(struct ipmmu_vmsa_domain *domain,
> > > > +                                 unsigned int reg, u32 data)
> > > > +{
> > > > +    ipmmu_write(domain->mmu->root,
> > > > +                domain->context_id * IM_CTX_SIZE + reg, data);
> > > > +}
> > > > +
> > > > +static void ipmmu_ctx_write_cache(struct ipmmu_vmsa_domain *domain,
> > > > +                                  unsigned int reg, u32 data)
> > > > +{
> > > > +    ASSERT(reg == IMCTR);
> > > What's the rationale of passing reg in parameter if it can only be 
> > > equal to IMCTR?
> > Good question. I tried to retain the same interface as for 
> > ipmmu_ctx_write_root(_all) for visibility.
> > Cache IPMMU device has other than IMCTR context registers, but they 
> > are not used by this driver.
> Could the function be able to deal with those other registers without 
> any change?
No. "data & IMCTR_COMMON_MASK" should be moved out of the function at 
least.

> > The worst case scenario would be when these devices are assigned to 
> > different Xen domains. So, I think, the same utlb *can't* be shared 
> > between multiple Xen domains, since it points to the context bank to 
> > use for the page walk.
> Thank you for the explanation. What can actually happen? Could it lead 
> to a security issue (e.g the IPMMU is bypassed)?
Yes, using context bank for IPA to PA translation from Domain A for a 
device running in Domain B could lead to something very bad. The best 
case scenario would be when IPA supplied by a device in Domain B is not 
effectively mapped in a context for Domain A. In this case we would just 
get a page fault...
> Also, the question is whether this is worth to try to implement it. Do 
> we have cases where devices use the same micro-TLB but assigned to 
> different domains?
No. At least I am not aware of. What is more that devices (which share 
the same utlb) can't be easily separated from each other the first in 
order to be assigned to different Xen domains then, I think.
Such DMA devices as AVB, SATA, eMMC/SD, USB, DU, GPU, etc (which really 
could be located in different Xen domains according to the particular 
use case) don't share utlbs.

> If not, then maybe you could just add check in the driver to prevent 
> that use cases. The work around the iommu_group done by Paul [1] might 
> be useful.
> Anyway, from upstream perspective this is not a massive concern for 
> now as platform device-passthrough is not security supported. So I 
> would be happy if the TODO is addressed in a follow-up series.

Agree.

So, the following actions:

1. TODO remains for this driver series.

2. TODO will be addressed in a follow-up series by *preventing* the use 
cases where the same utlb could be shared between multiple Xen domains.

> [...]
>
> > > > +/* Master devices management */
> > > > +static int ipmmu_attach_device(struct ipmmu_vmsa_domain *domain,
> > > > +                               struct device *dev)
> > > > +{
> > > > +    struct ipmmu_vmsa_master_cfg *cfg = dev_archdata(dev)->cfg;
> > > > +    struct ipmmu_vmsa_device *mmu = cfg->mmu;
> > > > +    unsigned int i;
> > > > +
> > > > +    if ( !mmu )
> > > > +    {
> > > > +        dev_err(dev, "Cannot attach to IPMMU\n");
> > > > +        return -ENXIO;
> > > > +    }
> > > > +
> > > > +    if ( !domain->mmu )
> > > So you read domain->mmu here and ...
> > >
> > > > +    {
> > > > +        /* The domain hasn't been used yet, initialize it. */
> > > > +        domain->mmu = mmu;
> > > > +
> > > > +        /*
> > > > +         * We have already enabled context for Root IPMMU assigned 
> > > > to this
> > > > +         * Xen domain in ipmmu_domain_init_context().
> > > > +         * Enable the context for Cache IPMMU only. Flush the TLB 
> > > > as required
> > > > +         * when modifying the context registers.
> > > > +         */
> > > > +        ipmmu_ctx_write_cache(domain, IMCTR,
> > > > +                              ipmmu_ctx_read_root(domain, IMCTR) | 
> > > > IMCTR_FLUSH);
> > > > +
> > > > +        dev_info(dev, "Using IPMMU context %u\n", 
> > > > domain->context_id);
> > > > +    }
> > > > +    else if ( domain->mmu != mmu )
> > > ... here. What actually promise that domain->mmu can't change in 
> > > parallel?
> > ipmmu_attach_device is protected by xen_domain->lock
> I find confusing to rely on xen_domain->lock to serialize access to a 
> field from a different structure. It would be good if this is written 
> in the document of the structure.
> Also is this always read behind the same lock?
I think, yes. I couldn't find the case when not.

Will clarify documentation.


> > > The IOMMU interface in Xen has not been designed with the new IOMMU 
> > > bindings in mind. I would prefer if we look for extending add_device 
> > > callback to support platform device.
> > > This would allow to probe the device later on and therefore avoid to 
> > > go through the device-tree multiple.
> >
> > I completely agree with you that current implementation is not 
> > optimal and should be reworked in order not to scan the whole DT many 
> > times, but I am not completely understand what we should do and how 
> > exactly.
> > Could you, please, add more details?
> It would be good to have an abstract way to add new device to IOMMU 
> based on the generic IOMMU DT binding. I am quite keen to seen 
> something similar to iommu_fwspec in Xen so this can be used for both 
> DT and ACPI.
> From an high level perspective, we would have some code add a new 
> device to the IOMMU. The generic code would:
>    1) Parse the binding and prepare iommu_fwspec with the correct 
> information
>    2) Call the IOMMU driver to register the new device
>
> The new function would be either called from handle_device or a new 
> loop over the DT nodes.
In the whole, I understand your point. I will come up with questions if any.


> > > Can we look at handling -EDEFER in Xen instead?
> > I am not sure this is something we should implement at this stage 
> > (while only IPMMU driver would be a user). I have already resolved 
> > that possible issue by trying to locate a Root IPMMU device and probe 
> > it the first
> > to avoid the case described above. So now, we don't depend on how 
> > IPMMU devices are located in DT. Please, see ipmmu_init(). So, I tend 
> > to live with it some time.
> The reason I asked the question is the current solution feels like 
> papering over an API that does not fit for the new driver. So it would 
> be worth investigating whether a -EDEFER like could be easily used in Xen.
Well, I will definitely investigate the possibility.


> > ----------
> >
> > Julien, what we should do with the fact that IPMMU supports only 
> > 3-level page table?
> > I left a TODO regarding that, but we need to work out some usable 
> > solution if possible.
> >          /*
> >           * As 4-level translation table is not supported in IPMMU, 
> > we need
> >           * to check IPA size used for P2M table beforehand to be 
> > sure it is
> >           * 3-level and the IPMMU will be able to use it.
> >           *
> >           * In case of using 4KB page granule we should use two 
> > concatenated
> >           * translation tables at level 1 in order to support 40 bit IPA
> >           * with 3-level translation table.
> >           *
> >           * TODO: Probably, when determing the "pa_range" in 
> > setup_virt_paging()
> >           * we should take into the account the IPMMU ability as well.
> >           */
> >          if ( IPMMU_MAX_P2M_IPA_BITS < p2m_ipa_bits )
> >          {
> >              dev_err(&node->dev, "P2M IPA size is not supported 
> > (P2M=%u IPMMU=%u)!\n",
> >                      p2m_ipa_bits, IPMMU_MAX_P2M_IPA_BITS);
> >              return -EOPNOTSUPP;
> >          }
> We have similar problem with the SMMU. The only strict requirement for 
> the IOMMU is to have a valid p2m_ipa_bits at the time a domain is 
> built. Note that the SMMU will store the value when probing the SMMU, 
> but that could be reworked.
> So rather than initializing the P2M first and then the IOMMU, I would 
> first initialize the IOMMU so we can gather the requirements and then 
> initialize the P2M.
> In the P2M code, you can take into account the IOMMU requirements and 
> further restrict if necessary. What do you think?
I think, this sounds reasonable and worth trying. Could this TODO be 
addressed in a follow-up series?

> Cheers,
>
> [1] <20190716101657.23327-1-paul.durrant@xxxxxxxxxx>
--
Regards,

Oleksandr Tyshchenko


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel