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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

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.

Cheers,

--
Julien Grall

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