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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Oleksandr,

On 7/24/19 11:54 AM, Oleksandr wrote:
> On 23.07.19 16:36, Julien Grall wrote:
> > > > On 6/26/19 11:30 AM, Oleksandr Tyshchenko wrote:
> > > > > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > > +{
> > > > > +    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.
I will leave up to you whether you want to drop the parameter. However, 
I'd like a comment explaining why the ASSERT() if you decide to keep it.

[...]

> > 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.
The 2 actions looks good to me.

[...]

> I think, this sounds reasonable and worth trying. Could this TODO be 
> addressed in a follow-up series?
I am fine with that.

Cheers,

--
Julien Grall

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