[PATCH v2 0/4] xen/arm: Make PCI passthrough code non-x86 specific

[PATCH v2 0/4] xen/arm: Make PCI passthrough code non-x86 specific

[Rahul Singh]

This patch series is v2 of preparatory work to make PCI passthrough code
non-x86 specific.

Rahul Singh (4):
  xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI
    enabled.
  xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.
  xen/pci: Move x86 specific code to x86 directory.
  xen/pci: solve compilation error on ARM with HAS_PCI enabled.

 xen/drivers/char/Kconfig             |  7 +++
 xen/drivers/char/ns16550.c           | 32 +++++-----
 xen/drivers/passthrough/ats.h        | 26 ++++++++
 xen/drivers/passthrough/pci.c        | 84 +------------------------
 xen/drivers/passthrough/x86/Makefile |  3 +-
 xen/drivers/passthrough/x86/iommu.c  | 20 ++++++
 xen/drivers/passthrough/x86/pci.c    | 91 ++++++++++++++++++++++++++++
 xen/drivers/pci/Kconfig              |  9 +++
 xen/include/xen/iommu.h              |  2 +
 xen/include/xen/pci.h                |  2 +
 10 files changed, 177 insertions(+), 99 deletions(-)
 create mode 100644 xen/drivers/passthrough/x86/pci.c

-- 
2.17.1

[PATCH v2 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.

[Rahul Singh]

ARM platforms do not have PCI support available. When CONFIG_HAS_PCI
is enabled for ARM a compilation error is observed for ns16550 driver.

Fixed compilation error after introducing new kconfig option
CONFIG_HAS_NS16550_PCI to support ns16550 PCI for X86.

For X86 platforms it is enabled by default. For ARM platforms it is
disabled by default, once we have proper support for NS16550 PCI for
ARM we can enable it.

No functional change.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---

Changes in v2:
 - Silently enable the HAS_NS16550_PCI for x86 by default. 

---
 xen/drivers/char/Kconfig   |  7 +++++++
 xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index b572305657..12a53607d1 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -4,6 +4,13 @@ config HAS_NS16550
 	help
 	  This selects the 16550-series UART support. For most systems, say Y.
 
+config HAS_NS16550_PCI
+	def_bool y
+	depends on X86 && HAS_NS16550 && HAS_PCI
+	help
+	  This selects the 16550-series UART PCI support.For most systems,
+	  say Y.
+
 config HAS_CADENCE_UART
 	bool "Xilinx Cadence UART driver"
 	default y
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index d8b52eb813..bd1c2af956 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -16,7 +16,7 @@
 #include <xen/timer.h>
 #include <xen/serial.h>
 #include <xen/iocap.h>
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <xen/pci_ids.h>
@@ -54,7 +54,7 @@ enum serial_param_type {
     reg_shift,
     reg_width,
     stop_bits,
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     bridge_bdf,
     device,
     port_bdf,
@@ -83,7 +83,7 @@ static struct ns16550 {
     unsigned int timeout_ms;
     bool_t intr_works;
     bool_t dw_usr_bsy;
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     /* PCI card parameters. */
     bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
     bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
@@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = {
     {"reg-shift", reg_shift},
     {"reg-width", reg_width},
     {"stop-bits", stop_bits},
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     {"bridge", bridge_bdf},
     {"dev", device},
     {"port", port_bdf},
 #endif
 };
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
 struct ns16550_config {
     u16 vendor_id;
     u16 dev_id;
@@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
 
 static void pci_serial_early_init(struct ns16550 *uart)
 {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
         return;
 
@@ -719,7 +719,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
 
 static void __init ns16550_init_irq(struct serial_port *port)
 {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     struct ns16550 *uart = port->uart;
 
     if ( uart->msi )
@@ -761,7 +761,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
     uart->timeout_ms = max_t(
         unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     if ( uart->bar || uart->ps_bdf_enable )
     {
         if ( uart->param && uart->param->mmio &&
@@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port)
 
     stop_timer(&uart->timer);
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     if ( uart->bar )
        uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
                                   uart->ps_bdf[2]), PCI_COMMAND);
@@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port)
 
 static void _ns16550_resume(struct serial_port *port)
 {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     struct ns16550 *uart = port->uart;
 
     if ( uart->bar )
@@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart)
     return 1; /* Everything is MMIO */
 #endif
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     pci_serial_early_init(uart);
 #endif
 
@@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart)
     return (status == 0x90);
 }
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
 static int __init
 pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
 {
@@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
 
     if ( *conf == ',' && *++conf != ',' )
     {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
         if ( strncmp(conf, "pci", 3) == 0 )
         {
             if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
@@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
 
     if ( *conf == ',' && *++conf != ',' )
     {
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
         if ( strncmp(conf, "msi", 3) == 0 )
         {
             conf += 3;
@@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
             uart->irq = simple_strtol(conf, &conf, 10);
     }
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
     if ( *conf == ',' && *++conf != ',' )
     {
         conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
@@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
             uart->reg_width = simple_strtoul(param_value, NULL, 0);
             break;
 
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
         case bridge_bdf:
             if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
                             &uart->ps_bdf[1], &uart->ps_bdf[2]) )
-- 
2.17.1

Re: [PATCH v2 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.

[Bertrand Marquis]

> On 3 Nov 2020, at 15:59, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> ARM platforms do not have PCI support available. When CONFIG_HAS_PCI
> is enabled for ARM a compilation error is observed for ns16550 driver.
> 
> Fixed compilation error after introducing new kconfig option
> CONFIG_HAS_NS16550_PCI to support ns16550 PCI for X86.
> 
> For X86 platforms it is enabled by default. For ARM platforms it is
> disabled by default, once we have proper support for NS16550 PCI for
> ARM we can enable it.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 
> Changes in v2:
> - Silently enable the HAS_NS16550_PCI for x86 by default. 
> 
> ---
> xen/drivers/char/Kconfig   |  7 +++++++
> xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
> 2 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b572305657..12a53607d1 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -4,6 +4,13 @@ config HAS_NS16550
> 	help
> 	  This selects the 16550-series UART support. For most systems, say Y.
> 
> +config HAS_NS16550_PCI
> +	def_bool y
> +	depends on X86 && HAS_NS16550 && HAS_PCI
> +	help
> +	  This selects the 16550-series UART PCI support.For most systems,
> +	  say Y.
> +
> config HAS_CADENCE_UART
> 	bool "Xilinx Cadence UART driver"
> 	default y
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index d8b52eb813..bd1c2af956 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -16,7 +16,7 @@
> #include <xen/timer.h>
> #include <xen/serial.h>
> #include <xen/iocap.h>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> #include <xen/pci.h>
> #include <xen/pci_regs.h>
> #include <xen/pci_ids.h>
> @@ -54,7 +54,7 @@ enum serial_param_type {
>     reg_shift,
>     reg_width,
>     stop_bits,
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     bridge_bdf,
>     device,
>     port_bdf,
> @@ -83,7 +83,7 @@ static struct ns16550 {
>     unsigned int timeout_ms;
>     bool_t intr_works;
>     bool_t dw_usr_bsy;
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     /* PCI card parameters. */
>     bool_t pb_bdf_enable;   /* if =1, pb-bdf effective, port behind bridge */
>     bool_t ps_bdf_enable;   /* if =1, ps_bdf effective, port on pci card */
> @@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = {
>     {"reg-shift", reg_shift},
>     {"reg-width", reg_width},
>     {"stop-bits", stop_bits},
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     {"bridge", bridge_bdf},
>     {"dev", device},
>     {"port", port_bdf},
> #endif
> };
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> struct ns16550_config {
>     u16 vendor_id;
>     u16 dev_id;
> @@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
> 
> static void pci_serial_early_init(struct ns16550 *uart)
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
>         return;
> 
> @@ -719,7 +719,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
> 
> static void __init ns16550_init_irq(struct serial_port *port)
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     struct ns16550 *uart = port->uart;
> 
>     if ( uart->msi )
> @@ -761,7 +761,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
>     uart->timeout_ms = max_t(
>         unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     if ( uart->bar || uart->ps_bdf_enable )
>     {
>         if ( uart->param && uart->param->mmio &&
> @@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port)
> 
>     stop_timer(&uart->timer);
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     if ( uart->bar )
>        uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>                                   uart->ps_bdf[2]), PCI_COMMAND);
> @@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port)
> 
> static void _ns16550_resume(struct serial_port *port)
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     struct ns16550 *uart = port->uart;
> 
>     if ( uart->bar )
> @@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart)
>     return 1; /* Everything is MMIO */
> #endif
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     pci_serial_early_init(uart);
> #endif
> 
> @@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart)
>     return (status == 0x90);
> }
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> static int __init
> pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
> {
> @@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> 
>     if ( *conf == ',' && *++conf != ',' )
>     {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>         if ( strncmp(conf, "pci", 3) == 0 )
>         {
>             if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> @@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> 
>     if ( *conf == ',' && *++conf != ',' )
>     {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>         if ( strncmp(conf, "msi", 3) == 0 )
>         {
>             conf += 3;
> @@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>             uart->irq = simple_strtol(conf, &conf, 10);
>     }
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>     if ( *conf == ',' && *++conf != ',' )
>     {
>         conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
> @@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>             uart->reg_width = simple_strtoul(param_value, NULL, 0);
>             break;
> 
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
>         case bridge_bdf:
>             if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
>                             &uart->ps_bdf[1], &uart->ps_bdf[2]) )
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.

[Jan Beulich]

On 03.11.2020 16:59, Rahul Singh wrote:
> ARM platforms do not have PCI support available. When CONFIG_HAS_PCI
> is enabled for ARM a compilation error is observed for ns16550 driver.

I still don't really agree to the approach taken together with
the wording. If Arm was to select HAS_PCI, my expectation would
be that this file compiled fine. You don't mention what
compilation error it is that results, so it's hard to judge if
I'm completely wrong. If, however, this is just a shortcoming
of the Arm implementation right now, then I'd like to ask that
the description here be worded to this effect. This will then
make it easier to understand that the change here can really
be reverted without much further consideration.

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -4,6 +4,13 @@ config HAS_NS16550
>  	help
>  	  This selects the 16550-series UART support. For most systems, say Y.
>  
> +config HAS_NS16550_PCI
> +	def_bool y
> +	depends on X86 && HAS_NS16550 && HAS_PCI
> +	help
> +	  This selects the 16550-series UART PCI support.For most systems,
> +	  say Y.

There's not much point for a prompt-less option to have help
text. There's definitely no point telling what to select when
really there's nothing to select, due to the lack of a prompt.

Jan

[PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Rahul Singh]

PCI ATS functionality is not enabled and tested for ARM architecture
but it is enabled for x86 and referenced in common passthrough/pci.c
code.

Therefore introducing the new flag to enable the ATS functionality for
x86 only to avoid issues for ARM architecture.

No functional change.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---

Changes in v2:
 - Fixed return value of PCI ATS related functions when PCI_ATS is not enabled.
 - Make PCI_ATS user selectable kconfig option.

---
 xen/drivers/passthrough/ats.h        | 26 ++++++++++++++++++++++++++
 xen/drivers/passthrough/x86/Makefile |  2 +-
 xen/drivers/pci/Kconfig              |  9 +++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
index 22ae209b37..3a71fedcb4 100644
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -17,6 +17,8 @@
 
 #include <xen/pci_regs.h>
 
+#ifdef CONFIG_PCI_ATS
+
 #define ATS_REG_CAP    4
 #define ATS_REG_CTL    6
 #define ATS_QUEUE_DEPTH_MASK     0x1f
@@ -48,5 +50,29 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
     return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
 }
 
+#else
+
+#define ats_enabled (false)
+
+static inline int enable_ats_device(struct pci_dev *pdev,
+                                    struct list_head *ats_list)
+{
+    return -EOPNOTSUPP;
+}
+
+static inline void disable_ats_device(struct pci_dev *pdev) { }
+
+static inline int pci_ats_enabled(int seg, int bus, int devfn)
+{
+    return 0;
+}
+
+static inline int pci_ats_device(int seg, int bus, int devfn)
+{
+    return 0;
+}
+
+#endif /* CONFIG_PCI_ATS */
+
 #endif /* _ATS_H_ */
 
diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
index a70cf9460d..aa515c680d 100644
--- a/xen/drivers/passthrough/x86/Makefile
+++ b/xen/drivers/passthrough/x86/Makefile
@@ -1,2 +1,2 @@
-obj-y += ats.o
+obj-$(CONFIG_PCI_ATS) += ats.o
 obj-y += iommu.o
diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
index 7da03fa13b..3cb79ea954 100644
--- a/xen/drivers/pci/Kconfig
+++ b/xen/drivers/pci/Kconfig
@@ -1,3 +1,12 @@
 
 config HAS_PCI
 	bool
+
+config PCI_ATS
+	bool "PCI ATS support"
+	default y
+	depends on X86 && HAS_PCI
+	---help---
+	 Enable PCI Address Translation Services.
+
+	 If unsure, say Y.
-- 
2.17.1

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Bertrand Marquis]

> On 3 Nov 2020, at 15:59, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> PCI ATS functionality is not enabled and tested for ARM architecture
> but it is enabled for x86 and referenced in common passthrough/pci.c
> code.
> 
> Therefore introducing the new flag to enable the ATS functionality for
> x86 only to avoid issues for ARM architecture.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 
> Changes in v2:
> - Fixed return value of PCI ATS related functions when PCI_ATS is not enabled.
> - Make PCI_ATS user selectable kconfig option.
> 
> ---
> xen/drivers/passthrough/ats.h        | 26 ++++++++++++++++++++++++++
> xen/drivers/passthrough/x86/Makefile |  2 +-
> xen/drivers/pci/Kconfig              |  9 +++++++++
> 3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
> index 22ae209b37..3a71fedcb4 100644
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -17,6 +17,8 @@
> 
> #include <xen/pci_regs.h>
> 
> +#ifdef CONFIG_PCI_ATS
> +
> #define ATS_REG_CAP    4
> #define ATS_REG_CTL    6
> #define ATS_QUEUE_DEPTH_MASK     0x1f
> @@ -48,5 +50,29 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
>     return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
> }
> 
> +#else
> +
> +#define ats_enabled (false)
> +
> +static inline int enable_ats_device(struct pci_dev *pdev,
> +                                    struct list_head *ats_list)
> +{
> +    return -EOPNOTSUPP;
> +}
> +
> +static inline void disable_ats_device(struct pci_dev *pdev) { }
> +
> +static inline int pci_ats_enabled(int seg, int bus, int devfn)
> +{
> +    return 0;
> +}
> +
> +static inline int pci_ats_device(int seg, int bus, int devfn)
> +{
> +    return 0;
> +}
> +
> +#endif /* CONFIG_PCI_ATS */
> +
> #endif /* _ATS_H_ */
> 
> diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
> index a70cf9460d..aa515c680d 100644
> --- a/xen/drivers/passthrough/x86/Makefile
> +++ b/xen/drivers/passthrough/x86/Makefile
> @@ -1,2 +1,2 @@
> -obj-y += ats.o
> +obj-$(CONFIG_PCI_ATS) += ats.o
> obj-y += iommu.o
> diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
> index 7da03fa13b..3cb79ea954 100644
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,12 @@
> 
> config HAS_PCI
> 	bool
> +
> +config PCI_ATS
> +	bool "PCI ATS support"
> +	default y
> +	depends on X86 && HAS_PCI
> +	---help---
> +	 Enable PCI Address Translation Services.
> +
> +	 If unsure, say Y.
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Jan Beulich]

On 03.11.2020 16:59, Rahul Singh wrote:
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,12 @@
>  
>  config HAS_PCI
>  	bool
> +
> +config PCI_ATS
> +	bool "PCI ATS support"
> +	default y
> +	depends on X86 && HAS_PCI
> +	---help---
> +	 Enable PCI Address Translation Services.
> +
> +	 If unsure, say Y.

Support for "---help---" having gone away in Linux, I think we'd
better not add new instances. Also indentation of help content
typically is by a tab and two spaces. With these two adjusted

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Jan Beulich]

On 04.11.2020 16:43, Jan Beulich wrote:
> On 03.11.2020 16:59, Rahul Singh wrote:
>> --- a/xen/drivers/pci/Kconfig
>> +++ b/xen/drivers/pci/Kconfig
>> @@ -1,3 +1,12 @@
>>  
>>  config HAS_PCI
>>  	bool
>> +
>> +config PCI_ATS
>> +	bool "PCI ATS support"
>> +	default y
>> +	depends on X86 && HAS_PCI
>> +	---help---
>> +	 Enable PCI Address Translation Services.
>> +
>> +	 If unsure, say Y.
> 
> Support for "---help---" having gone away in Linux, I think we'd
> better not add new instances. Also indentation of help content
> typically is by a tab and two spaces. With these two adjusted
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Initially I wanted to merely reply indicating I'd be fine making
these changes while committing, but there are two more things
(and I withdraw my R-b): For one, isn't strict pci_dev's ats
field now unused when !PCI_ATS? If so, if should get an #ifdef
added. And then, what exactly is it in ats.c that's x86-specific?
Shouldn't the whole file instead be moved one level up, and be
usable by Arm right away?

Jan

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Stefano Stabellini]

On Wed, 4 Nov 2020, Jan Beulich wrote:
> On 04.11.2020 16:43, Jan Beulich wrote:
> > On 03.11.2020 16:59, Rahul Singh wrote:
> >> --- a/xen/drivers/pci/Kconfig
> >> +++ b/xen/drivers/pci/Kconfig
> >> @@ -1,3 +1,12 @@
> >>  
> >>  config HAS_PCI
> >>  	bool
> >> +
> >> +config PCI_ATS
> >> +	bool "PCI ATS support"
> >> +	default y
> >> +	depends on X86 && HAS_PCI
> >> +	---help---
> >> +	 Enable PCI Address Translation Services.
> >> +
> >> +	 If unsure, say Y.
> > 
> > Support for "---help---" having gone away in Linux, I think we'd
> > better not add new instances. Also indentation of help content
> > typically is by a tab and two spaces. With these two adjusted
> > 
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Initially I wanted to merely reply indicating I'd be fine making
> these changes while committing, but there are two more things
> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
> field now unused when !PCI_ATS? If so, if should get an #ifdef
> added. And then, what exactly is it in ats.c that's x86-specific?
> Shouldn't the whole file instead be moved one level up, and be
> usable by Arm right away?

If the issue is that ATS wouldn't work on ARM straight away, then I
think it would be best to make this a silent option like we did in patch
#1: if x86 && HAS_PCI -> automatically enable, otherwise disable. I
wouldn't move the code just yet, that's better done when we can actually
test it on ARM.

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Jan Beulich]

On 05.11.2020 22:04, Stefano Stabellini wrote:
> On Wed, 4 Nov 2020, Jan Beulich wrote:
>> On 04.11.2020 16:43, Jan Beulich wrote:
>>> On 03.11.2020 16:59, Rahul Singh wrote:
>>>> --- a/xen/drivers/pci/Kconfig
>>>> +++ b/xen/drivers/pci/Kconfig
>>>> @@ -1,3 +1,12 @@
>>>>  
>>>>  config HAS_PCI
>>>>  	bool
>>>> +
>>>> +config PCI_ATS
>>>> +	bool "PCI ATS support"
>>>> +	default y
>>>> +	depends on X86 && HAS_PCI
>>>> +	---help---
>>>> +	 Enable PCI Address Translation Services.
>>>> +
>>>> +	 If unsure, say Y.
>>>
>>> Support for "---help---" having gone away in Linux, I think we'd
>>> better not add new instances. Also indentation of help content
>>> typically is by a tab and two spaces. With these two adjusted
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Initially I wanted to merely reply indicating I'd be fine making
>> these changes while committing, but there are two more things
>> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
>> field now unused when !PCI_ATS? If so, if should get an #ifdef
>> added. And then, what exactly is it in ats.c that's x86-specific?
>> Shouldn't the whole file instead be moved one level up, and be
>> usable by Arm right away?
> 
> If the issue is that ATS wouldn't work on ARM straight away, then I
> think it would be best to make this a silent option like we did in patch
> #1: if x86 && HAS_PCI -> automatically enable, otherwise disable.

Taking the opportunity to make this a non-silent option was actually
a request of mine. As long as the code builds and isn't obviously
broken for Arm, I think it shouldn't have an X86 dependency (and it
then should be moved up in the tree). Arguably it could then
default to off for Arm, but when asking for this option to gain a
prompt I also indicated that I wonder whether the default shouldn't
be off on x86 as well, seeing that the controlling command line
option also defaults to off.

Jan

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Rahul Singh]

Hello Jan,

> On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 04.11.2020 16:43, Jan Beulich wrote:
>> On 03.11.2020 16:59, Rahul Singh wrote:
>>> --- a/xen/drivers/pci/Kconfig
>>> +++ b/xen/drivers/pci/Kconfig
>>> @@ -1,3 +1,12 @@
>>> 
>>> config HAS_PCI
>>> 	bool
>>> +
>>> +config PCI_ATS
>>> +	bool "PCI ATS support"
>>> +	default y
>>> +	depends on X86 && HAS_PCI
>>> +	---help---
>>> +	 Enable PCI Address Translation Services.
>>> +
>>> +	 If unsure, say Y.
>> 
>> Support for "---help---" having gone away in Linux, I think we'd
>> better not add new instances. Also indentation of help content
>> typically is by a tab and two spaces. With these two adjusted
>> 
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Initially I wanted to merely reply indicating I'd be fine making
> these changes while committing, but there are two more things
> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
> field now unused when !PCI_ATS? If so, if should get an #ifdef
> added.

Yes, I tried to #ifdef ats field in struct pci_dev but after doing that I found that I have to modify the 
code related to x86  iotlb flush, as I have limited knowledge about how iotlb flush works for 
x86 so I decided not to modify that part of the code. 

I already compiled the x86 with !PCI_ATS and is having same behaviour as command line options "ats=false” with unused struct pci_dev ats field.

> And then, what exactly is it in ats.c that's x86-specific?
> Shouldn't the whole file instead be moved one level up, and be
> usable by Arm right away?

Yes, you are right ats.c file is not x86 specific, but not tested for ARM so I thought we will move the ats.c file to common code once ATS code is tested/implemented for ARM.

disable_ats_device() is referenced in common "passthrough/pci.c" file  and defined in "x86/ats.c" therefore I decided to introduce the PCI_ATS option for X86. 
As I am not sure on ARM how the ATS functionality is going to be implemented. 

There are three ways to fix the compilation error for ARM related to disable_ats_device() function.

1. Introduce the PCI_ATS config option for x86 and enabled it by default for X86 and having same behaviour as  command line options for !PCi_ATS  as "ats=false”

2. disable_ats_device() is called by iommu_dev_iotlb_flush_timeout () that is as per my understanding is x86 specific ( not sure please confirm).
Move iommu_dev_iotlb_flush_timeout () to "passthrough/x86/iommu.c” now and move the ats.c file to common code once ATS code is tested for ARM.

3. Move x86/ats.c file one level up , fixed compilation error now and if on ARM platforms going to implement the ATS functionality different from
x86 we have to move ats.c file back to x86 directory 

Please suggest us how we can proceed further on this.

> 
> Jan

Regards,
Rahul

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Jan Beulich]

On 06.11.2020 12:43, Rahul Singh wrote:
> Hello Jan,
> 
>> On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 04.11.2020 16:43, Jan Beulich wrote:
>>> On 03.11.2020 16:59, Rahul Singh wrote:
>>>> --- a/xen/drivers/pci/Kconfig
>>>> +++ b/xen/drivers/pci/Kconfig
>>>> @@ -1,3 +1,12 @@
>>>>
>>>> config HAS_PCI
>>>> 	bool
>>>> +
>>>> +config PCI_ATS
>>>> +	bool "PCI ATS support"
>>>> +	default y
>>>> +	depends on X86 && HAS_PCI
>>>> +	---help---
>>>> +	 Enable PCI Address Translation Services.
>>>> +
>>>> +	 If unsure, say Y.
>>>
>>> Support for "---help---" having gone away in Linux, I think we'd
>>> better not add new instances. Also indentation of help content
>>> typically is by a tab and two spaces. With these two adjusted
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Initially I wanted to merely reply indicating I'd be fine making
>> these changes while committing, but there are two more things
>> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
>> field now unused when !PCI_ATS? If so, if should get an #ifdef
>> added.
> 
> Yes, I tried to #ifdef ats field in struct pci_dev but after doing that I found that I have to modify the 
> code related to x86  iotlb flush, as I have limited knowledge about how iotlb flush works for 
> x86 so I decided not to modify that part of the code. 
> 
> I already compiled the x86 with !PCI_ATS and is having same behaviour as command line options "ats=false” with unused struct pci_dev ats field.
> 
>> And then, what exactly is it in ats.c that's x86-specific?
>> Shouldn't the whole file instead be moved one level up, and be
>> usable by Arm right away?
> 
> Yes, you are right ats.c file is not x86 specific, but not tested for ARM so I thought we will move the ats.c file to common code once ATS code is tested/implemented for ARM.
> 
> disable_ats_device() is referenced in common "passthrough/pci.c" file  and defined in "x86/ats.c" therefore I decided to introduce the PCI_ATS option for X86. 
> As I am not sure on ARM how the ATS functionality is going to be implemented. 
> 
> There are three ways to fix the compilation error for ARM related to disable_ats_device() function.
> 
> 1. Introduce the PCI_ATS config option for x86 and enabled it by default for X86 and having same behaviour as  command line options for !PCi_ATS  as "ats=false”

I'll be happy to see the config option retained, but that's
orthogonal to the goals here.

> 2. disable_ats_device() is called by iommu_dev_iotlb_flush_timeout () that is as per my understanding is x86 specific ( not sure please confirm).
> Move iommu_dev_iotlb_flush_timeout () to "passthrough/x86/iommu.c” now and move the ats.c file to common code once ATS code is tested for ARM.

While the function is currently used by VT-d code only, I again
don't see what's x86-specific about it. Hence ...

> 3. Move x86/ats.c file one level up , fixed compilation error now and if on ARM platforms going to implement the ATS functionality different from
> x86 we have to move ats.c file back to x86 directory 

... I view this as the only "option" among the ones you name.

Jan

Re: [PATCH v2 2/4] xen/pci: Introduce new CONFIG_PCI_ATS flag for PCI ATS functionality.

[Julien Grall]

Hi,

On 06/11/2020 12:48, Jan Beulich wrote:
> On 06.11.2020 12:43, Rahul Singh wrote:
>> Hello Jan,
>>
>>> On 4 Nov 2020, at 3:49 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 04.11.2020 16:43, Jan Beulich wrote:
>>>> On 03.11.2020 16:59, Rahul Singh wrote:
>>>>> --- a/xen/drivers/pci/Kconfig
>>>>> +++ b/xen/drivers/pci/Kconfig
>>>>> @@ -1,3 +1,12 @@
>>>>>
>>>>> config HAS_PCI
>>>>> 	bool
>>>>> +
>>>>> +config PCI_ATS
>>>>> +	bool "PCI ATS support"
>>>>> +	default y
>>>>> +	depends on X86 && HAS_PCI
>>>>> +	---help---
>>>>> +	 Enable PCI Address Translation Services.
>>>>> +
>>>>> +	 If unsure, say Y.
>>>>
>>>> Support for "---help---" having gone away in Linux, I think we'd
>>>> better not add new instances. Also indentation of help content
>>>> typically is by a tab and two spaces. With these two adjusted
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Initially I wanted to merely reply indicating I'd be fine making
>>> these changes while committing, but there are two more things
>>> (and I withdraw my R-b): For one, isn't strict pci_dev's ats
>>> field now unused when !PCI_ATS? If so, if should get an #ifdef
>>> added.
>>
>> Yes, I tried to #ifdef ats field in struct pci_dev but after doing that I found that I have to modify the
>> code related to x86  iotlb flush, as I have limited knowledge about how iotlb flush works for
>> x86 so I decided not to modify that part of the code.
>>
>> I already compiled the x86 with !PCI_ATS and is having same behaviour as command line options "ats=false” with unused struct pci_dev ats field.
>>
>>> And then, what exactly is it in ats.c that's x86-specific?
>>> Shouldn't the whole file instead be moved one level up, and be
>>> usable by Arm right away?
>>
>> Yes, you are right ats.c file is not x86 specific, but not tested for ARM so I thought we will move the ats.c file to common code once ATS code is tested/implemented for ARM.
>>
>> disable_ats_device() is referenced in common "passthrough/pci.c" file  and defined in "x86/ats.c" therefore I decided to introduce the PCI_ATS option for X86.
>> As I am not sure on ARM how the ATS functionality is going to be implemented.
>>
>> There are three ways to fix the compilation error for ARM related to disable_ats_device() function.
>>
>> 1. Introduce the PCI_ATS config option for x86 and enabled it by default for X86 and having same behaviour as  command line options for !PCi_ATS  as "ats=false”
> 
> I'll be happy to see the config option retained, but that's
> orthogonal to the goals here.
> 
>> 2. disable_ats_device() is called by iommu_dev_iotlb_flush_timeout () that is as per my understanding is x86 specific ( not sure please confirm).
>> Move iommu_dev_iotlb_flush_timeout () to "passthrough/x86/iommu.c” now and move the ats.c file to common code once ATS code is tested for ARM.
> 
> While the function is currently used by VT-d code only, I again
> don't see what's x86-specific about it. Hence ...
The ATS code looks arch-agnostic. So I agree with this statement.

> 
>> 3. Move x86/ats.c file one level up , fixed compilation error now and if on ARM platforms going to implement the ATS functionality different from
>> x86 we have to move ats.c file back to x86 directory
> 
> ... I view this as the only "option" among the ones you name.

+1.

Cheers,

-- 
Julien Grall

[PATCH v2 3/4] xen/pci: Move x86 specific code to x86 directory.

[Rahul Singh]

passthrough/pci.c file is common for all architecture, but there is x86
sepcific code in this file.

Move x86 specific code to the x86 directory to avoid compilation error
for other architecture.

No functional change.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---

Changes is v2:
 - fixed comments.
 - rename pci_clean_dpci_irqs() to arch_pci_clean_pirqs().

---
 xen/drivers/passthrough/pci.c        | 76 +----------------------
 xen/drivers/passthrough/x86/Makefile |  1 +
 xen/drivers/passthrough/x86/iommu.c  |  7 +++
 xen/drivers/passthrough/x86/pci.c    | 91 ++++++++++++++++++++++++++++
 xen/include/xen/pci.h                |  2 +
 5 files changed, 102 insertions(+), 75 deletions(-)
 create mode 100644 xen/drivers/passthrough/x86/pci.c

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 2a3bce1462..04d3e2c0f9 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -14,7 +14,6 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/sched.h>
 #include <xen/pci.h>
 #include <xen/pci_regs.h>
 #include <xen/pci_ids.h>
@@ -24,7 +23,6 @@
 #include <xen/irq.h>
 #include <xen/param.h>
 #include <xen/vm_event.h>
-#include <asm/hvm/irq.h>
 #include <xen/delay.h>
 #include <xen/keyhandler.h>
 #include <xen/event.h>
@@ -847,71 +845,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
     return ret;
 }
 
-static int pci_clean_dpci_irq(struct domain *d,
-                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
-{
-    struct dev_intx_gsi_link *digl, *tmp;
-
-    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
-
-    if ( pt_irq_need_timer(pirq_dpci->flags) )
-        kill_timer(&pirq_dpci->timer);
-
-    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
-    {
-        list_del(&digl->list);
-        xfree(digl);
-    }
-
-    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
-
-    if ( !pt_pirq_softirq_active(pirq_dpci) )
-        return 0;
-
-    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
-
-    return -ERESTART;
-}
-
-static int pci_clean_dpci_irqs(struct domain *d)
-{
-    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
-
-    if ( !is_iommu_enabled(d) )
-        return 0;
-
-    if ( !is_hvm_domain(d) )
-        return 0;
-
-    spin_lock(&d->event_lock);
-    hvm_irq_dpci = domain_get_irq_dpci(d);
-    if ( hvm_irq_dpci != NULL )
-    {
-        int ret = 0;
-
-        if ( hvm_irq_dpci->pending_pirq_dpci )
-        {
-            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
-                 ret = -ERESTART;
-            else
-                 hvm_irq_dpci->pending_pirq_dpci = NULL;
-        }
-
-        if ( !ret )
-            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
-        if ( ret )
-        {
-            spin_unlock(&d->event_lock);
-            return ret;
-        }
-
-        hvm_domain_irq(d)->dpci = NULL;
-        free_hvm_irq_dpci(hvm_irq_dpci);
-    }
-    spin_unlock(&d->event_lock);
-    return 0;
-}
-
 /* Caller should hold the pcidevs_lock */
 static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
                            uint8_t devfn)
@@ -971,7 +904,7 @@ int pci_release_devices(struct domain *d)
     int ret;
 
     pcidevs_lock();
-    ret = pci_clean_dpci_irqs(d);
+    ret = arch_pci_clean_pirqs(d);
     if ( ret )
     {
         pcidevs_unlock();
@@ -1375,13 +1308,6 @@ static int __init setup_dump_pcidevs(void)
 }
 __initcall(setup_dump_pcidevs);
 
-int iommu_update_ire_from_msi(
-    struct msi_desc *msi_desc, struct msi_msg *msg)
-{
-    return iommu_intremap
-           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
-}
-
 static int iommu_add_device(struct pci_dev *pdev)
 {
     const struct domain_iommu *hd;
diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
index aa515c680d..d02ff75de5 100644
--- a/xen/drivers/passthrough/x86/Makefile
+++ b/xen/drivers/passthrough/x86/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_PCI_ATS) += ats.o
 obj-y += iommu.o
+obj-y += pci.o
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index f17b1820f4..875e67b53b 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -308,6 +308,13 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
     return pg;
 }
 
+int iommu_update_ire_from_msi(
+    struct msi_desc *msi_desc, struct msi_msg *msg)
+{
+    return iommu_intremap
+           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/drivers/passthrough/x86/pci.c b/xen/drivers/passthrough/x86/pci.c
new file mode 100644
index 0000000000..59588aa8d4
--- /dev/null
+++ b/xen/drivers/passthrough/x86/pci.c
@@ -0,0 +1,91 @@
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/sched.h>
+#include <xen/pci.h>
+
+static int pci_clean_dpci_irq(struct domain *d,
+                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
+{
+    struct dev_intx_gsi_link *digl, *tmp;
+
+    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
+
+    if ( pt_irq_need_timer(pirq_dpci->flags) )
+        kill_timer(&pirq_dpci->timer);
+
+    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
+    {
+        list_del(&digl->list);
+        xfree(digl);
+    }
+
+    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
+
+    if ( !pt_pirq_softirq_active(pirq_dpci) )
+        return 0;
+
+    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
+
+    return -ERESTART;
+}
+
+int arch_pci_clean_pirqs(struct domain *d)
+{
+    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+
+    if ( !is_iommu_enabled(d) )
+        return 0;
+
+    if ( !is_hvm_domain(d) )
+        return 0;
+
+    spin_lock(&d->event_lock);
+    hvm_irq_dpci = domain_get_irq_dpci(d);
+    if ( hvm_irq_dpci != NULL )
+    {
+        int ret = 0;
+
+        if ( hvm_irq_dpci->pending_pirq_dpci )
+        {
+            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
+                 ret = -ERESTART;
+            else
+                 hvm_irq_dpci->pending_pirq_dpci = NULL;
+        }
+
+        if ( !ret )
+            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
+        if ( ret )
+        {
+            spin_unlock(&d->event_lock);
+            return ret;
+        }
+
+        hvm_domain_irq(d)->dpci = NULL;
+        free_hvm_irq_dpci(hvm_irq_dpci);
+    }
+    spin_unlock(&d->event_lock);
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index c4d3879761..fd28d11f6e 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -209,4 +209,6 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
 void msixtbl_pt_unregister(struct domain *, struct pirq *);
 void msixtbl_pt_cleanup(struct domain *d);
 
+int arch_pci_clean_pirqs(struct domain *d);
+
 #endif /* __XEN_PCI_H__ */
-- 
2.17.1

Re: [PATCH v2 3/4] xen/pci: Move x86 specific code to x86 directory.

[Bertrand Marquis]

> On 3 Nov 2020, at 15:59, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> passthrough/pci.c file is common for all architecture, but there is x86
> sepcific code in this file.
> 
> Move x86 specific code to the x86 directory to avoid compilation error
> for other architecture.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 
> Changes is v2:
> - fixed comments.
> - rename pci_clean_dpci_irqs() to arch_pci_clean_pirqs().
> 
> ---
> xen/drivers/passthrough/pci.c        | 76 +----------------------
> xen/drivers/passthrough/x86/Makefile |  1 +
> xen/drivers/passthrough/x86/iommu.c  |  7 +++
> xen/drivers/passthrough/x86/pci.c    | 91 ++++++++++++++++++++++++++++
> xen/include/xen/pci.h                |  2 +
> 5 files changed, 102 insertions(+), 75 deletions(-)
> create mode 100644 xen/drivers/passthrough/x86/pci.c
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 2a3bce1462..04d3e2c0f9 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -14,7 +14,6 @@
>  * this program; If not, see <http://www.gnu.org/licenses/>.
>  */
> 
> -#include <xen/sched.h>
> #include <xen/pci.h>
> #include <xen/pci_regs.h>
> #include <xen/pci_ids.h>
> @@ -24,7 +23,6 @@
> #include <xen/irq.h>
> #include <xen/param.h>
> #include <xen/vm_event.h>
> -#include <asm/hvm/irq.h>
> #include <xen/delay.h>
> #include <xen/keyhandler.h>
> #include <xen/event.h>
> @@ -847,71 +845,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>     return ret;
> }
> 
> -static int pci_clean_dpci_irq(struct domain *d,
> -                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
> -{
> -    struct dev_intx_gsi_link *digl, *tmp;
> -
> -    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> -
> -    if ( pt_irq_need_timer(pirq_dpci->flags) )
> -        kill_timer(&pirq_dpci->timer);
> -
> -    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> -    {
> -        list_del(&digl->list);
> -        xfree(digl);
> -    }
> -
> -    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> -
> -    if ( !pt_pirq_softirq_active(pirq_dpci) )
> -        return 0;
> -
> -    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> -
> -    return -ERESTART;
> -}
> -
> -static int pci_clean_dpci_irqs(struct domain *d)
> -{
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> -
> -    if ( !is_iommu_enabled(d) )
> -        return 0;
> -
> -    if ( !is_hvm_domain(d) )
> -        return 0;
> -
> -    spin_lock(&d->event_lock);
> -    hvm_irq_dpci = domain_get_irq_dpci(d);
> -    if ( hvm_irq_dpci != NULL )
> -    {
> -        int ret = 0;
> -
> -        if ( hvm_irq_dpci->pending_pirq_dpci )
> -        {
> -            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> -                 ret = -ERESTART;
> -            else
> -                 hvm_irq_dpci->pending_pirq_dpci = NULL;
> -        }
> -
> -        if ( !ret )
> -            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> -        if ( ret )
> -        {
> -            spin_unlock(&d->event_lock);
> -            return ret;
> -        }
> -
> -        hvm_domain_irq(d)->dpci = NULL;
> -        free_hvm_irq_dpci(hvm_irq_dpci);
> -    }
> -    spin_unlock(&d->event_lock);
> -    return 0;
> -}
> -
> /* Caller should hold the pcidevs_lock */
> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>                            uint8_t devfn)
> @@ -971,7 +904,7 @@ int pci_release_devices(struct domain *d)
>     int ret;
> 
>     pcidevs_lock();
> -    ret = pci_clean_dpci_irqs(d);
> +    ret = arch_pci_clean_pirqs(d);
>     if ( ret )
>     {
>         pcidevs_unlock();
> @@ -1375,13 +1308,6 @@ static int __init setup_dump_pcidevs(void)
> }
> __initcall(setup_dump_pcidevs);
> 
> -int iommu_update_ire_from_msi(
> -    struct msi_desc *msi_desc, struct msi_msg *msg)
> -{
> -    return iommu_intremap
> -           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> -}
> -
> static int iommu_add_device(struct pci_dev *pdev)
> {
>     const struct domain_iommu *hd;
> diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
> index aa515c680d..d02ff75de5 100644
> --- a/xen/drivers/passthrough/x86/Makefile
> +++ b/xen/drivers/passthrough/x86/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_PCI_ATS) += ats.o
> obj-y += iommu.o
> +obj-y += pci.o
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index f17b1820f4..875e67b53b 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -308,6 +308,13 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
>     return pg;
> }
> 
> +int iommu_update_ire_from_msi(
> +    struct msi_desc *msi_desc, struct msi_msg *msg)
> +{
> +    return iommu_intremap
> +           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> +}
> +
> /*
>  * Local variables:
>  * mode: C
> diff --git a/xen/drivers/passthrough/x86/pci.c b/xen/drivers/passthrough/x86/pci.c
> new file mode 100644
> index 0000000000..59588aa8d4
> --- /dev/null
> +++ b/xen/drivers/passthrough/x86/pci.c
> @@ -0,0 +1,91 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/pci.h>
> +
> +static int pci_clean_dpci_irq(struct domain *d,
> +                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
> +{
> +    struct dev_intx_gsi_link *digl, *tmp;
> +
> +    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> +
> +    if ( pt_irq_need_timer(pirq_dpci->flags) )
> +        kill_timer(&pirq_dpci->timer);
> +
> +    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> +    {
> +        list_del(&digl->list);
> +        xfree(digl);
> +    }
> +
> +    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> +
> +    if ( !pt_pirq_softirq_active(pirq_dpci) )
> +        return 0;
> +
> +    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> +
> +    return -ERESTART;
> +}
> +
> +int arch_pci_clean_pirqs(struct domain *d)
> +{
> +    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +
> +    if ( !is_iommu_enabled(d) )
> +        return 0;
> +
> +    if ( !is_hvm_domain(d) )
> +        return 0;
> +
> +    spin_lock(&d->event_lock);
> +    hvm_irq_dpci = domain_get_irq_dpci(d);
> +    if ( hvm_irq_dpci != NULL )
> +    {
> +        int ret = 0;
> +
> +        if ( hvm_irq_dpci->pending_pirq_dpci )
> +        {
> +            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> +                 ret = -ERESTART;
> +            else
> +                 hvm_irq_dpci->pending_pirq_dpci = NULL;
> +        }
> +
> +        if ( !ret )
> +            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> +        if ( ret )
> +        {
> +            spin_unlock(&d->event_lock);
> +            return ret;
> +        }
> +
> +        hvm_domain_irq(d)->dpci = NULL;
> +        free_hvm_irq_dpci(hvm_irq_dpci);
> +    }
> +    spin_unlock(&d->event_lock);
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index c4d3879761..fd28d11f6e 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -209,4 +209,6 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
> void msixtbl_pt_unregister(struct domain *, struct pirq *);
> void msixtbl_pt_cleanup(struct domain *d);
> 
> +int arch_pci_clean_pirqs(struct domain *d);
> +
> #endif /* __XEN_PCI_H__ */
> -- 
> 2.17.1
> 

Re: [PATCH v2 3/4] xen/pci: Move x86 specific code to x86 directory.

[Stefano Stabellini]

On Tue, 3 Nov 2020, Rahul Singh wrote:
> passthrough/pci.c file is common for all architecture, but there is x86
> sepcific code in this file.
   ^ specific

Aside from that:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> Move x86 specific code to the x86 directory to avoid compilation error
> for other architecture.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> 
> Changes is v2:
>  - fixed comments.
>  - rename pci_clean_dpci_irqs() to arch_pci_clean_pirqs().
> 
> ---
>  xen/drivers/passthrough/pci.c        | 76 +----------------------
>  xen/drivers/passthrough/x86/Makefile |  1 +
>  xen/drivers/passthrough/x86/iommu.c  |  7 +++
>  xen/drivers/passthrough/x86/pci.c    | 91 ++++++++++++++++++++++++++++
>  xen/include/xen/pci.h                |  2 +
>  5 files changed, 102 insertions(+), 75 deletions(-)
>  create mode 100644 xen/drivers/passthrough/x86/pci.c
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 2a3bce1462..04d3e2c0f9 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -14,7 +14,6 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <xen/sched.h>
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
>  #include <xen/pci_ids.h>
> @@ -24,7 +23,6 @@
>  #include <xen/irq.h>
>  #include <xen/param.h>
>  #include <xen/vm_event.h>
> -#include <asm/hvm/irq.h>
>  #include <xen/delay.h>
>  #include <xen/keyhandler.h>
>  #include <xen/event.h>
> @@ -847,71 +845,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>      return ret;
>  }
>  
> -static int pci_clean_dpci_irq(struct domain *d,
> -                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
> -{
> -    struct dev_intx_gsi_link *digl, *tmp;
> -
> -    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> -
> -    if ( pt_irq_need_timer(pirq_dpci->flags) )
> -        kill_timer(&pirq_dpci->timer);
> -
> -    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> -    {
> -        list_del(&digl->list);
> -        xfree(digl);
> -    }
> -
> -    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> -
> -    if ( !pt_pirq_softirq_active(pirq_dpci) )
> -        return 0;
> -
> -    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> -
> -    return -ERESTART;
> -}
> -
> -static int pci_clean_dpci_irqs(struct domain *d)
> -{
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> -
> -    if ( !is_iommu_enabled(d) )
> -        return 0;
> -
> -    if ( !is_hvm_domain(d) )
> -        return 0;
> -
> -    spin_lock(&d->event_lock);
> -    hvm_irq_dpci = domain_get_irq_dpci(d);
> -    if ( hvm_irq_dpci != NULL )
> -    {
> -        int ret = 0;
> -
> -        if ( hvm_irq_dpci->pending_pirq_dpci )
> -        {
> -            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> -                 ret = -ERESTART;
> -            else
> -                 hvm_irq_dpci->pending_pirq_dpci = NULL;
> -        }
> -
> -        if ( !ret )
> -            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> -        if ( ret )
> -        {
> -            spin_unlock(&d->event_lock);
> -            return ret;
> -        }
> -
> -        hvm_domain_irq(d)->dpci = NULL;
> -        free_hvm_irq_dpci(hvm_irq_dpci);
> -    }
> -    spin_unlock(&d->event_lock);
> -    return 0;
> -}
> -
>  /* Caller should hold the pcidevs_lock */
>  static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>                             uint8_t devfn)
> @@ -971,7 +904,7 @@ int pci_release_devices(struct domain *d)
>      int ret;
>  
>      pcidevs_lock();
> -    ret = pci_clean_dpci_irqs(d);
> +    ret = arch_pci_clean_pirqs(d);
>      if ( ret )
>      {
>          pcidevs_unlock();
> @@ -1375,13 +1308,6 @@ static int __init setup_dump_pcidevs(void)
>  }
>  __initcall(setup_dump_pcidevs);
>  
> -int iommu_update_ire_from_msi(
> -    struct msi_desc *msi_desc, struct msi_msg *msg)
> -{
> -    return iommu_intremap
> -           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> -}
> -
>  static int iommu_add_device(struct pci_dev *pdev)
>  {
>      const struct domain_iommu *hd;
> diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
> index aa515c680d..d02ff75de5 100644
> --- a/xen/drivers/passthrough/x86/Makefile
> +++ b/xen/drivers/passthrough/x86/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_PCI_ATS) += ats.o
>  obj-y += iommu.o
> +obj-y += pci.o
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index f17b1820f4..875e67b53b 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -308,6 +308,13 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
>      return pg;
>  }
>  
> +int iommu_update_ire_from_msi(
> +    struct msi_desc *msi_desc, struct msi_msg *msg)
> +{
> +    return iommu_intremap
> +           ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/drivers/passthrough/x86/pci.c b/xen/drivers/passthrough/x86/pci.c
> new file mode 100644
> index 0000000000..59588aa8d4
> --- /dev/null
> +++ b/xen/drivers/passthrough/x86/pci.c
> @@ -0,0 +1,91 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/pci.h>
> +
> +static int pci_clean_dpci_irq(struct domain *d,
> +                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
> +{
> +    struct dev_intx_gsi_link *digl, *tmp;
> +
> +    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> +
> +    if ( pt_irq_need_timer(pirq_dpci->flags) )
> +        kill_timer(&pirq_dpci->timer);
> +
> +    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> +    {
> +        list_del(&digl->list);
> +        xfree(digl);
> +    }
> +
> +    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> +
> +    if ( !pt_pirq_softirq_active(pirq_dpci) )
> +        return 0;
> +
> +    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> +
> +    return -ERESTART;
> +}
> +
> +int arch_pci_clean_pirqs(struct domain *d)
> +{
> +    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +
> +    if ( !is_iommu_enabled(d) )
> +        return 0;
> +
> +    if ( !is_hvm_domain(d) )
> +        return 0;
> +
> +    spin_lock(&d->event_lock);
> +    hvm_irq_dpci = domain_get_irq_dpci(d);
> +    if ( hvm_irq_dpci != NULL )
> +    {
> +        int ret = 0;
> +
> +        if ( hvm_irq_dpci->pending_pirq_dpci )
> +        {
> +            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> +                 ret = -ERESTART;
> +            else
> +                 hvm_irq_dpci->pending_pirq_dpci = NULL;
> +        }
> +
> +        if ( !ret )
> +            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> +        if ( ret )
> +        {
> +            spin_unlock(&d->event_lock);
> +            return ret;
> +        }
> +
> +        hvm_domain_irq(d)->dpci = NULL;
> +        free_hvm_irq_dpci(hvm_irq_dpci);
> +    }
> +    spin_unlock(&d->event_lock);
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index c4d3879761..fd28d11f6e 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -209,4 +209,6 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
>  void msixtbl_pt_unregister(struct domain *, struct pirq *);
>  void msixtbl_pt_cleanup(struct domain *d);
>  
> +int arch_pci_clean_pirqs(struct domain *d);
> +
>  #endif /* __XEN_PCI_H__ */
> -- 
> 2.17.1
> 

Re: [PATCH v2 3/4] xen/pci: Move x86 specific code to x86 directory.

[Jan Beulich]

On 03.11.2020 16:59, Rahul Singh wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -14,7 +14,6 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <xen/sched.h>
>  #include <xen/pci.h>
>  #include <xen/pci_regs.h>
>  #include <xen/pci_ids.h>

I think this hunk wants dropping - struct domain continues to be used
in this file, for example.

> @@ -847,71 +845,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>      return ret;
>  }
>  
> -static int pci_clean_dpci_irq(struct domain *d,
> -                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
> -{
> -    struct dev_intx_gsi_link *digl, *tmp;
> -
> -    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> -
> -    if ( pt_irq_need_timer(pirq_dpci->flags) )
> -        kill_timer(&pirq_dpci->timer);
> -
> -    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> -    {
> -        list_del(&digl->list);
> -        xfree(digl);
> -    }
> -
> -    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> -
> -    if ( !pt_pirq_softirq_active(pirq_dpci) )
> -        return 0;
> -
> -    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> -
> -    return -ERESTART;
> -}
> -
> -static int pci_clean_dpci_irqs(struct domain *d)
> -{
> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> -
> -    if ( !is_iommu_enabled(d) )
> -        return 0;
> -
> -    if ( !is_hvm_domain(d) )
> -        return 0;
> -
> -    spin_lock(&d->event_lock);
> -    hvm_irq_dpci = domain_get_irq_dpci(d);
> -    if ( hvm_irq_dpci != NULL )
> -    {
> -        int ret = 0;
> -
> -        if ( hvm_irq_dpci->pending_pirq_dpci )
> -        {
> -            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> -                 ret = -ERESTART;
> -            else
> -                 hvm_irq_dpci->pending_pirq_dpci = NULL;
> -        }
> -
> -        if ( !ret )
> -            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> -        if ( ret )
> -        {
> -            spin_unlock(&d->event_lock);
> -            return ret;
> -        }
> -
> -        hvm_domain_irq(d)->dpci = NULL;
> -        free_hvm_irq_dpci(hvm_irq_dpci);
> -    }
> -    spin_unlock(&d->event_lock);
> -    return 0;
> -}

If this code gets moved, I think it ought to move into
xen/drivers/passthrough/io.c, as that's where all the companion code
sits. (The file as a whole, getting built for x86/HVM only, may want
moving to xen/drivers/passthrough/x86/ if the underlying model isn't
suitable for Arm. Then it probably also would want to be named hvm.c,
to express its limited purpose.)

Jan

Re: [PATCH v2 3/4] xen/pci: Move x86 specific code to x86 directory.

[Rahul Singh]

Hello Jan,

> On 6 Nov 2020, at 9:09 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 03.11.2020 16:59, Rahul Singh wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -14,7 +14,6 @@
>>  * this program; If not, see <http://www.gnu.org/licenses/>.
>>  */
>> 
>> -#include <xen/sched.h>

Removed in this patch series 3/4
>> #include <xen/pci.h>
>> #include <xen/pci_regs.h>

I will remove in next version.

>> #include <xen/pci_ids.h>

It is required for PCI_VENDOR_ID_INTEL that is referenced in apply_quirks function.

> 
> I think this hunk wants dropping - struct domain continues to be used
> in this file, for example.
> 
>> @@ -847,71 +845,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>     return ret;
>> }
>> 
>> -static int pci_clean_dpci_irq(struct domain *d,
>> -                              struct hvm_pirq_dpci *pirq_dpci, void *arg)
>> -{
>> -    struct dev_intx_gsi_link *digl, *tmp;
>> -
>> -    pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
>> -
>> -    if ( pt_irq_need_timer(pirq_dpci->flags) )
>> -        kill_timer(&pirq_dpci->timer);
>> -
>> -    list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
>> -    {
>> -        list_del(&digl->list);
>> -        xfree(digl);
>> -    }
>> -
>> -    radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
>> -
>> -    if ( !pt_pirq_softirq_active(pirq_dpci) )
>> -        return 0;
>> -
>> -    domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
>> -
>> -    return -ERESTART;
>> -}
>> -
>> -static int pci_clean_dpci_irqs(struct domain *d)
>> -{
>> -    struct hvm_irq_dpci *hvm_irq_dpci = NULL;
>> -
>> -    if ( !is_iommu_enabled(d) )
>> -        return 0;
>> -
>> -    if ( !is_hvm_domain(d) )
>> -        return 0;
>> -
>> -    spin_lock(&d->event_lock);
>> -    hvm_irq_dpci = domain_get_irq_dpci(d);
>> -    if ( hvm_irq_dpci != NULL )
>> -    {
>> -        int ret = 0;
>> -
>> -        if ( hvm_irq_dpci->pending_pirq_dpci )
>> -        {
>> -            if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
>> -                 ret = -ERESTART;
>> -            else
>> -                 hvm_irq_dpci->pending_pirq_dpci = NULL;
>> -        }
>> -
>> -        if ( !ret )
>> -            ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
>> -        if ( ret )
>> -        {
>> -            spin_unlock(&d->event_lock);
>> -            return ret;
>> -        }
>> -
>> -        hvm_domain_irq(d)->dpci = NULL;
>> -        free_hvm_irq_dpci(hvm_irq_dpci);
>> -    }
>> -    spin_unlock(&d->event_lock);
>> -    return 0;
>> -}
> 
> If this code gets moved, I think it ought to move into
> xen/drivers/passthrough/io.c, as that's where all the companion code
> sits. (The file as a whole, getting built for x86/HVM only, may want
> moving to xen/drivers/passthrough/x86/ if the underlying model isn't
> suitable for Arm. Then it probably also would want to be named hvm.c,
> to express its limited purpose.)


Ok I will move the code to the file io.c and move that file to x86 directory and rename it hvm.c
> 
> Jan

Regards,
Rahul

[PATCH v2 4/4] xen/pci: solve compilation error on ARM with HAS_PCI enabled.

[Rahul Singh]

If mem-sharing, mem-paging and log-dirty functionality is not enabled
for architecture when HAS_PCI is enabled, compiler will throw an error.

Move code to x86 specific directory to fix compilation error.

No functional change.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---

Changes in v2:
 - Move mem-sharing , men-paging and log-dirty specific code to x86 directory. 

---
 xen/drivers/passthrough/pci.c       |  8 +-------
 xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++
 xen/include/xen/iommu.h             |  2 ++
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 04d3e2c0f9..433989e654 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -22,7 +22,6 @@
 #include <xen/iommu.h>
 #include <xen/irq.h>
 #include <xen/param.h>
-#include <xen/vm_event.h>
 #include <xen/delay.h>
 #include <xen/keyhandler.h>
 #include <xen/event.h>
@@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
     if ( !is_iommu_enabled(d) )
         return 0;
 
-    /* Prevent device assign if mem paging or mem sharing have been 
-     * enabled for this domain */
-    if ( d != dom_io &&
-         unlikely(mem_sharing_enabled(d) ||
-                  vm_event_check_ring(d->vm_event_paging) ||
-                  p2m_get_hostp2m(d)->global_logdirty) )
+    if( !arch_iommu_usable(d) )
         return -EXDEV;
 
     /* device_assigned() should already have cleared the device for assignment */
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index 875e67b53b..b3d151a14c 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -23,6 +23,7 @@
 #include <asm/hvm/io.h>
 #include <asm/io_apic.h>
 #include <asm/setup.h>
+#include <xen/vm_event.h>
 
 const struct iommu_init_ops *__initdata iommu_init_ops;
 struct iommu_ops __read_mostly iommu_ops;
@@ -315,6 +316,18 @@ int iommu_update_ire_from_msi(
            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
 }
 
+bool_t arch_iommu_usable(struct domain *d)
+{
+
+    /* Prevent device assign if mem paging or mem sharing have been
+     * enabled for this domain */
+    if ( d != dom_io && unlikely(mem_sharing_enabled(d) ||
+                        vm_event_check_ring(d->vm_event_paging) ||
+                        p2m_get_hostp2m(d)->global_logdirty) )
+        return false;
+    else
+        return true;
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 191021870f..493528cee3 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -381,6 +381,8 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
 extern struct spinlock iommu_pt_cleanup_lock;
 extern struct page_list_head iommu_pt_cleanup_list;
 
+bool_t arch_iommu_usable(struct domain *d);
+
 #endif /* _IOMMU_H_ */
 
 /*
-- 
2.17.1

Re: [PATCH v2 4/4] xen/pci: solve compilation error on ARM with HAS_PCI enabled.

[Bertrand Marquis]

> On 3 Nov 2020, at 15:59, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> If mem-sharing, mem-paging and log-dirty functionality is not enabled
> for architecture when HAS_PCI is enabled, compiler will throw an error.
> 
> Move code to x86 specific directory to fix compilation error.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 
> Changes in v2:
> - Move mem-sharing , men-paging and log-dirty specific code to x86 directory. 
> 
> ---
> xen/drivers/passthrough/pci.c       |  8 +-------
> xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++
> xen/include/xen/iommu.h             |  2 ++
> 3 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 04d3e2c0f9..433989e654 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -22,7 +22,6 @@
> #include <xen/iommu.h>
> #include <xen/irq.h>
> #include <xen/param.h>
> -#include <xen/vm_event.h>
> #include <xen/delay.h>
> #include <xen/keyhandler.h>
> #include <xen/event.h>
> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>     if ( !is_iommu_enabled(d) )
>         return 0;
> 
> -    /* Prevent device assign if mem paging or mem sharing have been 
> -     * enabled for this domain */
> -    if ( d != dom_io &&
> -         unlikely(mem_sharing_enabled(d) ||
> -                  vm_event_check_ring(d->vm_event_paging) ||
> -                  p2m_get_hostp2m(d)->global_logdirty) )
> +    if( !arch_iommu_usable(d) )
>         return -EXDEV;
> 
>     /* device_assigned() should already have cleared the device for assignment */
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index 875e67b53b..b3d151a14c 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -23,6 +23,7 @@
> #include <asm/hvm/io.h>
> #include <asm/io_apic.h>
> #include <asm/setup.h>
> +#include <xen/vm_event.h>
> 
> const struct iommu_init_ops *__initdata iommu_init_ops;
> struct iommu_ops __read_mostly iommu_ops;
> @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi(
>            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> }
> 
> +bool_t arch_iommu_usable(struct domain *d)
> +{
> +
> +    /* Prevent device assign if mem paging or mem sharing have been
> +     * enabled for this domain */
> +    if ( d != dom_io && unlikely(mem_sharing_enabled(d) ||
> +                        vm_event_check_ring(d->vm_event_paging) ||
> +                        p2m_get_hostp2m(d)->global_logdirty) )
> +        return false;
> +    else
> +        return true;
> +}
> /*
>  * Local variables:
>  * mode: C
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 191021870f..493528cee3 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -381,6 +381,8 @@ DECLARE_PER_CPU(bool_t, iommu_dont_flush_iotlb);
> extern struct spinlock iommu_pt_cleanup_lock;
> extern struct page_list_head iommu_pt_cleanup_list;
> 
> +bool_t arch_iommu_usable(struct domain *d);
> +
> #endif /* _IOMMU_H_ */
> 
> /*
> -- 
> 2.17.1
> 

Re: [PATCH v2 4/4] xen/pci: solve compilation error on ARM with HAS_PCI enabled.

[Jan Beulich]

On 03.11.2020 16:59, Rahul Singh wrote:
> If mem-sharing, mem-paging and log-dirty functionality is not enabled
> for architecture when HAS_PCI is enabled, compiler will throw an error.

Nit: Is it really "and", not "or"?

> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>      if ( !is_iommu_enabled(d) )
>          return 0;
>  
> -    /* Prevent device assign if mem paging or mem sharing have been 
> -     * enabled for this domain */
> -    if ( d != dom_io &&
> -         unlikely(mem_sharing_enabled(d) ||
> -                  vm_event_check_ring(d->vm_event_paging) ||
> -                  p2m_get_hostp2m(d)->global_logdirty) )
> +    if( !arch_iommu_usable(d) )
>          return -EXDEV;

While iirc I did suggest this name, seeing it used here leaves me
somewhat unhappy with the name, albeit I also can't think of any
better alternative right now. Maybe arch_iommu_use_permitted()?

> @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi(
>             ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>  }
>  
> +bool_t arch_iommu_usable(struct domain *d)

Just bool please and I very much hope the parameter can be const.

> +{
> +
> +    /* Prevent device assign if mem paging or mem sharing have been
> +     * enabled for this domain */

Please correct comment style as you move it.

> +    if ( d != dom_io && unlikely(mem_sharing_enabled(d) ||
> +                        vm_event_check_ring(d->vm_event_paging) ||
> +                        p2m_get_hostp2m(d)->global_logdirty) )

You've screwed up indentation, and I don't see why ...

> +        return false;
> +    else
> +        return true;
> +}

... this can't be a simple single return statement anyway:

    return d == dom_io ||
           likely(!mem_sharing_enabled(d) &&
                  !vm_event_check_ring(d->vm_event_paging) &&
                  !p2m_get_hostp2m(d)->global_logdirty);

In the course of moving I'd also suggest dropping the use of
likely() here: The way it's used (on an && expression) isn't
normally having much effect anyway. If anything it should imo
be

    return d == dom_io ||
           (likely(!mem_sharing_enabled(d)) &&
            likely(!vm_event_check_ring(d->vm_event_paging)) &&
            likely(!p2m_get_hostp2m(d)->global_logdirty));

Any transformation to this effect wants mentioning in the
description, though.

Jan

Re: [PATCH v2 4/4] xen/pci: solve compilation error on ARM with HAS_PCI enabled.

[Rahul Singh]

Hello Jan,

> On 6 Nov 2020, at 9:21 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 03.11.2020 16:59, Rahul Singh wrote:
>> If mem-sharing, mem-paging and log-dirty functionality is not enabled
>> for architecture when HAS_PCI is enabled, compiler will throw an error.
> 
> Nit: Is it really "and", not "or”?

Ok yes I will fix in next version.
> 
>> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>     if ( !is_iommu_enabled(d) )
>>         return 0;
>> 
>> -    /* Prevent device assign if mem paging or mem sharing have been 
>> -     * enabled for this domain */
>> -    if ( d != dom_io &&
>> -         unlikely(mem_sharing_enabled(d) ||
>> -                  vm_event_check_ring(d->vm_event_paging) ||
>> -                  p2m_get_hostp2m(d)->global_logdirty) )
>> +    if( !arch_iommu_usable(d) )
>>         return -EXDEV;
> 
> While iirc I did suggest this name, seeing it used here leaves me
> somewhat unhappy with the name, albeit I also can't think of any
> better alternative right now. Maybe arch_iommu_use_permitted()?

Ok I will modify as per your suggestion.
> 
>> @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi(
>>            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> 
>> +bool_t arch_iommu_usable(struct domain *d)
> 
> Just bool please and I very much hope the parameter can be const.

Ok I will fix in next version.
> 
>> +{
>> +
>> +    /* Prevent device assign if mem paging or mem sharing have been
>> +     * enabled for this domain */
> 
> Please correct comment style as you move it.

Ok. 
> 
>> +    if ( d != dom_io && unlikely(mem_sharing_enabled(d) ||
>> +                        vm_event_check_ring(d->vm_event_paging) ||
>> +                        p2m_get_hostp2m(d)->global_logdirty) )
> 
> You've screwed up indentation, and I don't see why ...

I will fix in next version.

> 
>> +        return false;
>> +    else
>> +        return true;
>> +}
> 
> ... this can't be a simple single return statement anyway:
> 
>    return d == dom_io ||
>           likely(!mem_sharing_enabled(d) &&
>                  !vm_event_check_ring(d->vm_event_paging) &&
>                  !p2m_get_hostp2m(d)->global_logdirty);
> 
> In the course of moving I'd also suggest dropping the use of
> likely() here: The way it's used (on an && expression) isn't
> normally having much effect anyway. If anything it should imo
> be
> 
>    return d == dom_io ||
>           (likely(!mem_sharing_enabled(d)) &&
>            likely(!vm_event_check_ring(d->vm_event_paging)) &&
>            likely(!p2m_get_hostp2m(d)->global_logdirty));
> 
> Any transformation to this effect wants mentioning in the
> description, though.

Ok I will modify as per your suggestion.
> 
> Jan

Regards,
Rahul