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 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.

[...]

> > +
> > +#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

> > diff --git a/xen/drivers/passthrough/arm/smmu.c 
> > b/xen/drivers/passthrough/arm/smmu.c
> > index ad956d5b8d..4c04391e21 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -41,6 +41,7 @@
> >   #include <xen/irq.h>
> >   #include <xen/lib.h>
> >   #include <xen/list.h>
> > +#include <xen/linux_compat.h>
> >   #include <xen/mm.h>
> >   #include <xen/vmap.h>
> >   #include <xen/rbtree.h>
> > @@ -51,36 +52,13 @@
> >   #include <asm/io.h>
> >   #include <asm/platform.h>
> >   
> > +#include "arm_smmu.h" /* Not a self contained header. So last in the list */
> >   /* Xen: The below defines are redefined within the file. Undef it */
> >   #undef SCTLR_AFE
> >   #undef SCTLR_TRE
> >   #undef SCTLR_M
> >   #undef TTBCR_EAE
> >   
> > -/* 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
> > -
> >   static struct resource *platform_get_resource(struct platform_device *pdev,
> >                                               unsigned int type,
> >                                               unsigned int num)
> > @@ -118,58 +96,6 @@ static struct resource *platform_get_resource(struct 
> > platform_device *pdev,
> >   
> >   /* Xen: Helpers for IRQ functions */
> >   #define request_irq(irq, func, flags, name, dev) request_irq(irq, flags, 
> > func, name, dev)
> > -#define free_irq release_irq
> > -
> > -enum irqreturn {
> > -       IRQ_NONE        = (0 << 0),
> > -       IRQ_HANDLED     = (1 << 0),
> > -};
> > -
> > -typedef enum irqreturn irqreturn_t;
> You remove the irqreturn enum without adding any replacement, is this
> really unused?
It is used, so looks like the SMMU driver has not been build test it. 
Sameer, please at least build test the changes you made in the SMMU driver.
Cheers,

--
Julien Grall

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