Re: [Xen-devel] [PATCH 6/7] drivers/passthrough/arm: Refactor code for arm smmu drivers

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 02/09/2018 11:02 AM, Roger Pau Monné wrote:
> On Fri, Feb 09, 2018 at 10:51:01AM +0000, Julien Grall wrote:
> > Hi,
> >
> > On 02/09/2018 10:43 AM, Roger Pau Monné wrote:
> > > > +    unsigned int type;
> > > > +};
> > > > +
> > > > +#define resource_size(res) ((res)->size)
> > > > +
> > > > +#define platform_device device
> > > > +
> > > > +#define IORESOURCE_MEM 0
> > > > +#define IORESOURCE_IRQ 1
> > > > +
> > > > +/* Stub out DMA domain related functions */
> > > > +#define iommu_get_dma_cookie(dom) 0
> > > > +#define iommu_put_dma_cookie(dom)
> > > > +
> > > > +#define VA_BITS                0 /* Only used for configuring stage-1 input 
> > > > size */
> > > > +
> > > > +#define MODULE_DEVICE_TABLE(type, name)
> > > > +#define module_param_named(name, value, type, perm)
> > > > +#define MODULE_PARM_DESC(_parm, desc)
> > > > +
> > > > +#define dma_set_mask_and_coherent(d, b)        0
> > > > +#define of_dma_is_coherent(n)  0
> > > > +
> > > > +static void __iomem *devm_ioremap_resource(struct device *dev,
> > > > +                                          struct resource *res)
> > > Aligment, please use spaces.
> > >
> > > Also, is __iomem needed here at all?
> > On Arm, we tend to add keep __iomem on pointer dealing with MMIO.
> I understand that you keep it when directly importing code from Linux,
> but this is Xen code, so unless this is done merely for consistency it
> seems quite pointless (__iomem is defined to nothing AFAICT).
We do it quite consistenly in Xen Arm code :). Have a look at the return 
of ioremap_* or read*/write* parameters.
While it might be defined to nothing today, I'd like to keep it because 
it could easily be defined to check the address space used.
> > [...]
> >
> > > > +
> > > > +#endif /* __ARM_SMMU_H__ */
> > > > +
> > > > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > > > b/xen/drivers/passthrough/arm/smmu-v3.c
> > > > index f43485fe6e..f0a61521fb 100644
> > > > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > > > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > > > @@ -49,28 +49,7 @@
> > > >    #include <asm/io.h>
> > > >    #include <asm/platform.h>
> > > > -/* Alias to Xen device tree helpers */
> > > > -#define device_node dt_device_node
> > > > -#define of_phandle_args dt_phandle_args
> > > > -#define of_device_id dt_device_match
> > > > -#define of_match_node dt_match_node
> > > > -#define of_property_read_u32(np, pname, out) (!dt_property_read_u32(np, pname, 
> > > > out))
> > > > -#define of_property_read_bool dt_property_read_bool
> > > > -#define of_parse_phandle_with_args dt_parse_phandle_with_args
> > > > -
> > > > -/* Xen: Helpers to get device MMIO and IRQs */
> > > > -struct resource {
> > > > -       u64 addr;
> > > > -       u64 size;
> > > > -       unsigned int type;
> > > > -};
> > > > -
> > > > -#define resource_size(res) ((res)->size)
> > > > -
> > > > -#define platform_device device
> > > > -
> > > > -#define IORESOURCE_MEM 0
> > > > -#define IORESOURCE_IRQ 1
> > > You introduce the above code in patch 5, and remove it in patch 6, is
> > > this really needed?
> > >
> > > Ie: why not simply introduce this code directly in this patch?
> > See https://lists.xen.org/archives/html/xen-devel/2018-01/msg02066.html
> Hm, OK, I'm not sure I follow that.
>
> AFAICT the above code is added in patch 5 so that the driver can be
> hooked up into the build system. Could we just hold off hooking the
> driver to the build system until patch 6, in order to avoid such
> addition and removal of code?
What matters is to know what is common between SMMUv2 and SMMUv3 driver. 
So it can be pulled in a separate headers.
IHMO, the both yours and his way are valid. TBH, I would have done a 
third way and move that patch before #5. But at this stage, this is a 
matter of taste, hence way I didn't push to reshuffle the series.
Cheers,

--
Julien Grall

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