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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 19.09.19 15:05, Julien Grall wrote:
> Hi,
Hi Julien.


> On 19/09/2019 12:58, Oleksandr wrote:
> > On 19.09.19 14:45, Julien Grall wrote:
> > > Hi Oleksandr,
> >
> > Hi Julien
> >
> >
> > > On 13/09/2019 16:35, 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.
> > > >
> > > > Please note, current driver is supposed to work only with newest
> > > > R-Car Gen3 SoCs revisions which IPMMU hardware supports stage 2 
> > > > translation
> > > > table format and is able to use CPU's P2M table as is if one is
> > > > 3-level page table (up to 40 bit IPA).
> > > >
> > > > The major differences compare to the Linux driver 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 P2M
> > > > with the CPU. Stage 1 translation is always bypassed in Xen driver.
> > > >
> > > > So, Xen driver is supposed to be used with newest R-Car Gen3 SoC 
> > > > revisions
> > > > only (H3 ES3.0, M3-W+, etc.) which IPMMU H/W supports 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.
> > > >
> > > > 4. IPMMU device tracking. In Xen, all IOMMU devices are managed
> > > > by single driver instance. So, driver uses global list to keep track
> > > > of registered IPMMU devices.
> > > >
> > > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> > > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > > CC: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > The Xen specific bits looks good now. Assuming Yoshihiro will give 
> > > his reviewed-by/acked-by:
> > > Acked-by: Julien Grall <julien.grall@xxxxxxx>
> > Thank you!
> >
> >
> > One note, it turned out [1] that "args" parameter in "dt_xlate" 
> > callback needs "const" added (it is not assumed to modify it inside 
> > the callback).
> > This leads to additional changes to the IPMMU driver. If you happy 
> > with the changes below, I will retain your A-b.
> >
> > diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
> > b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > index b5c18c2..43e4a84 100644
> > --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> > @@ -696,7 +696,7 @@ static void ipmmu_detach_device(struct 
> > ipmmu_vmsa_domain *domain,
> >   }
> >
> >   static int ipmmu_init_platform_device(struct device *dev,
> > -                                      struct dt_phandle_args *args)
> > +                                      const struct dt_phandle_args 
> > *args)
> >   {
> >       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >       struct ipmmu_vmsa_device *mmu;
> > @@ -1174,7 +1174,8 @@ static int ipmmu_reassign_device(struct domain 
> > *s, struct domain *t,
> >       return 0;
> >   }
> >
> > -static int ipmmu_dt_xlate(struct device *dev, struct dt_phandle_args 
> > *spec)
> > +static int ipmmu_dt_xlate(struct device *dev,
> > +                          const struct dt_phandle_args *spec)
> >   {
> >       int ret;
> >
> > @@ -1187,7 +1188,7 @@ static int ipmmu_dt_xlate(struct device *dev, 
> > struct dt_phandle_args *spec)
> >       if ( spec->args_count != 1 || spec->args[0] >= IPMMU_UTLB_MAX )
> >           return -EINVAL;
> >
> > -    ret = iommu_fwspec_add_ids(dev, spec->args, 1);
> > +    ret = iommu_fwspec_add_ids(dev, (uint32_t *)spec->args, 1);
> NAck, you should never use a cast to remove a const. Looking at the 
> code, iommu_fwspec_add_ids() does not need to modify the ids so the 
> const should be propagated.
> With that in place, my ack can stand.
Fair enough, will propagate.


--
Regards,

Oleksandr Tyshchenko


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