Re: [PATCH v2 01/35] xen: introduce resource.h

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Dec 05, 2024 at 08:41:31PM -0800, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Move resource definitions to a new architecture-agnostic shared header file.
> 
> It will be used in follow on NS8250 emulator code to describe legacy
> PC COM resources.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
>  xen/common/device-tree/device-tree.c | 21 +------------------
>  xen/drivers/passthrough/arm/smmu.c   | 15 +-------------
>  xen/include/xen/resource.h           | 40 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 42 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/common/device-tree/device-tree.c b/xen/common/device-tree/device-tree.c
> index d0528c5825651f7cc9ebca0c949229c9083063c6..e8f810b2fe10890c033ed3a9d4ca627010ad019b 100644
> --- a/xen/common/device-tree/device-tree.c
> +++ b/xen/common/device-tree/device-tree.c
> @@ -24,6 +24,7 @@
>  #include <xen/ctype.h>
>  #include <asm/setup.h>
>  #include <xen/err.h>
> +#include <xen/resource.h>
>  
>  const void *device_tree_flattened;
>  dt_irq_xlate_func dt_irq_xlate;
> @@ -535,26 +536,6 @@ int dt_child_n_size_cells(const struct dt_device_node *parent)
>      return __dt_n_size_cells(parent, true);
>  }
>  
> -/*
> - * These are defined in Linux where much of this code comes from, but
> - * are currently unused outside this file in the context of Xen.
> - */
> -#define IORESOURCE_BITS         0x000000ff      /* Bus-specific bits */
> -
> -#define IORESOURCE_TYPE_BITS    0x00001f00      /* Resource type */
> -#define IORESOURCE_IO           0x00000100      /* PCI/ISA I/O ports */
> -#define IORESOURCE_MEM          0x00000200
> -#define IORESOURCE_REG          0x00000300      /* Register offsets */
> -#define IORESOURCE_IRQ          0x00000400
> -#define IORESOURCE_DMA          0x00000800
> -#define IORESOURCE_BUS          0x00001000
> -
> -#define IORESOURCE_PREFETCH     0x00002000      /* No side effects */
> -#define IORESOURCE_READONLY     0x00004000
> -#define IORESOURCE_CACHEABLE    0x00008000
> -#define IORESOURCE_RANGELENGTH  0x00010000
> -#define IORESOURCE_SHADOWABLE   0x00020000
> -
>  /*
>   * Default translator (generic bus)
>   */
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 03d22bce1e497e41834c273f9048b98dcbd48a54..aa6a968b574dce7cc753e8070fad3a6e585cd9e7 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -50,6 +50,7 @@
>  #include <xen/rbtree.h>
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
> +#include <xen/resource.h>
>  #include <asm/atomic.h>
>  #include <asm/device.h>
>  #include <asm/io.h>
> @@ -70,22 +71,8 @@
>  #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
> -{
> -	paddr_t addr;
> -	paddr_t size;
> -	unsigned int type;
> -};
> -
> -#define resource_size(res) (res)->size;
> -
>  #define platform_device dt_device_node
>  
> -#define IORESOURCE_MEM 0
> -#define IORESOURCE_IRQ 1
> -
>  static struct resource *platform_get_resource(struct platform_device *pdev,
>  					      unsigned int type,
>  					      unsigned int num)
> diff --git a/xen/include/xen/resource.h b/xen/include/xen/resource.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..4962e17da8387b7f324317482b19cc9fe71433fc
> --- /dev/null
> +++ b/xen/include/xen/resource.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * System resource description.
> + *
> + * Reference:
> + *   include/linux/ioport.h
> + */
> +#if !defined(XEN__RESOURCE_H)
> +#define XEN__RESOURCE_H
> +
> +#define IORESOURCE_BITS         0x000000FFU      /* Bus-specific bits */
> +
> +#define IORESOURCE_TYPE_BITS    0x00001F00U      /* Resource type */
> +#define IORESOURCE_IO           0x00000100U      /* PCI/ISA I/O ports */
> +#define IORESOURCE_MEM          0x00000200U
> +#define IORESOURCE_REG          0x00000300U      /* Register offsets */
> +#define IORESOURCE_IRQ          0x00000400U
> +#define IORESOURCE_DMA          0x00000800U
> +#define IORESOURCE_BUS          0x00001000U
> +
> +#define IORESOURCE_PREFETCH     0x00002000U      /* No side effects */
> +#define IORESOURCE_READONLY     0x00004000U
> +#define IORESOURCE_CACHEABLE    0x00008000U
> +#define IORESOURCE_RANGELENGTH  0x00010000U
> +#define IORESOURCE_SHADOWABLE   0x00020000U
> +
> +#define IORESOURCE_UNKNOWN      (~0U)
> +
> +struct resource {
> +    paddr_t addr;
> +    paddr_t size;
> +    unsigned int type;
> +};
> +
> +#define resource_size(res) (res)->size;
> +
> +#define foreach_resource(res) \

Nit: we usually name those for_each_foo instead of foreach_foo.

> +    for (; res && res->type != IORESOURCE_UNKNOWN; res++)

Missing spaces between parentheses:

for ( ; res && res->type != IORESOURCE_UNKNOWN; res++ )

Note that this macro will modify (advance) the res pointer, which is
maybe unexpected by the caller?

Also, the current logic forces the array of resources to always have a
trailing IORESOURCE_UNKNOWN element in order to break the loop, it
might be better to pass an explicit number of elements to iterate
against if possible?

As Jan said, it would be helpful to have an example usage of the
macro.

Thanks, Roger.