diff mbox series

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

Message ID 20241205-vuart-ns8250-v1-1-e9aa923127eb@ford.com (mailing list archive)
State New
Headers show
Series Introduce NS8250 UART emulator | expand

Commit Message

Denis Mukhin via B4 Relay Dec. 6, 2024, 4:41 a.m. UTC 
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(-)

Comments

Jan Beulich Dec. 10, 2024, 1:13 p.m. UTC | #1 
On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
> --- /dev/null
> +++ b/xen/include/xen/resource.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

GPL-2.0-only

> +/*
> + * System resource description.
> + *
> + * Reference:
> + *   include/linux/ioport.h

I'm unsure of the usefulness of such a reference.

> + */
> +#if !defined(XEN__RESOURCE_H)

Nit: #ifdef / #ifndef please whenever possible (as long as not inconsistent
with adjacent code).

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

The semicolon surely was wrong before and is wrong now. Plus Misra
demands that such macro expansions be parenthesized, I think.

> +#define foreach_resource(res) \
> +    for (; res && res->type != IORESOURCE_UNKNOWN; res++)

This one isn't being moved, but is being added. It's not used here,
which makes it difficult to judge its correctness. Perhaps better to
introduce this when its first needed, and then right away with the
required parentheses around uses of the macro parameter.

> +#endif /* #if !defined(XEN__RESOURCE_H) */

Just the guard identifier in the comment please.

Jan
Roger Pau Monné Dec. 11, 2024, 11:01 a.m. UTC | #2 
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.
diff mbox series

Patch

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) \
+    for (; res && res->type != IORESOURCE_UNKNOWN; res++)
+
+#endif /* #if !defined(XEN__RESOURCE_H) */