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

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

[Rahul Singh]

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

Rahul Singh (3):
  xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.
  xen/pci: Move x86 specific code to x86 directory.
  xen/pci: solve compilation error on ARM with HAS_PCI enabled.

 xen/drivers/char/Kconfig                    |  4 +
 xen/drivers/char/ns16550.c                  | 32 ++++----
 xen/drivers/passthrough/Makefile            |  3 -
 xen/drivers/passthrough/pci.c               | 86 +--------------------
 xen/drivers/passthrough/x86/Makefile        |  1 +
 xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 ++++++++++++++++
 xen/drivers/passthrough/x86/iommu.c         | 19 +++++
 xen/include/xen/iommu.h                     |  2 +
 xen/include/xen/pci.h                       |  2 +
 9 files changed, 112 insertions(+), 103 deletions(-)
 rename xen/drivers/passthrough/{io.c => x86/hvm.c} (95%)

-- 
2.17.1

[PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Rahul Singh]

NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
is enabled for ARM, compilation error is observed for ARM architecture
because ARM platforms do not have full PCI support available.

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 v3:
- remove help text from the Kconfig file because of prompt-less option.

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

diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index b572305657..abb59fdb0f 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -4,6 +4,10 @@ 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
+
 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

[PATCH v3 2/3] xen/pci: Move x86 specific code to x86 directory.

[Rahul Singh]

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

Move x86 specific code to the drivers/passthrough/io.c file to avoid
compilation error for other architecture.

As drivers/passthrough/io.c is compiled only for x86 move it to
x86 directory and rename it to hvm.c.

No functional change.

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

Changes in v3:
- fixed typo
- As per suggestion move the code to the file io.c and move that file to
  x86 directory and rename it hvm.c

---
 xen/drivers/passthrough/Makefile            |  3 -
 xen/drivers/passthrough/pci.c               | 78 +--------------------
 xen/drivers/passthrough/x86/Makefile        |  1 +
 xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 +++++++++++++++++
 xen/drivers/passthrough/x86/iommu.c         |  7 ++
 xen/include/xen/pci.h                       |  2 +
 6 files changed, 77 insertions(+), 80 deletions(-)
 rename xen/drivers/passthrough/{io.c => x86/hvm.c} (95%)

diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
index e973e16c74..cc646612c7 100644
--- a/xen/drivers/passthrough/Makefile
+++ b/xen/drivers/passthrough/Makefile
@@ -6,6 +6,3 @@ obj-$(CONFIG_ARM) += arm/
 obj-y += iommu.o
 obj-$(CONFIG_HAS_PCI) += pci.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
-
-x86-$(CONFIG_HVM) := io.o
-obj-$(CONFIG_X86) += $(x86-y)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 51e584127e..e8a28df126 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -14,9 +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>
 #include <xen/list.h>
 #include <xen/prefetch.h>
@@ -24,7 +21,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>
@@ -842,71 +838,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)
@@ -966,7 +897,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();
@@ -1370,13 +1301,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 a70cf9460d..69284a5d19 100644
--- a/xen/drivers/passthrough/x86/Makefile
+++ b/xen/drivers/passthrough/x86/Makefile
@@ -1,2 +1,3 @@
 obj-y += ats.o
 obj-y += iommu.o
+obj-$(CONFIG_HVM) += hvm.o
diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/x86/hvm.c
similarity index 95%
rename from xen/drivers/passthrough/io.c
rename to xen/drivers/passthrough/x86/hvm.c
index 6b1305a3e5..41cfa2e200 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -1036,6 +1036,72 @@ unlock:
     spin_unlock(&d->event_lock);
 }
 
+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;
+}
+
 /*
  * Note: 'pt_pirq_softirq_reset' can clear the STATE_SCHED before we get to
  * doing it. If that is the case we let 'pt_pirq_softirq_reset' do ref-counting.
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/include/xen/pci.h b/xen/include/xen/pci.h
index 20a54a5bb4..78d83afe64 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -208,4 +208,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

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

[Rahul Singh]

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

Move code to x86 specific directory to fix compilation error.

Also, modify the code to use likely() in place of unlikley() for each
condition to make code more optimized.

No functional change.

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

Changes in v3:
- rename arch_iommu_usable() to arch_iommu_use_permitted()
- fixed comments.

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

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index e8a28df126..804b24a0e0 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -20,7 +20,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>
@@ -1411,12 +1410,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_use_permitted(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..26f57b7e88 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,17 @@ int iommu_update_ire_from_msi(
            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
 }
 
+bool arch_iommu_use_permitted(const struct domain *d)
+{
+    /*
+     * Prevent device assign if mem paging, mem sharing or log-dirty
+     * have been enabled for this domain.
+     */
+    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));
+}
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 191021870f..056eaa09fc 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 arch_iommu_use_permitted(const struct domain *d);
+
 #endif /* _IOMMU_H_ */
 
 /*
-- 
2.17.1

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Stefano Stabellini]

On Mon, 16 Nov 2020, Rahul Singh wrote:
> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
> is enabled for ARM, compilation error is observed for ARM architecture
> because ARM platforms do not have full PCI support available.
> 
> 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: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Changes in v3:
> - remove help text from the Kconfig file because of prompt-less option.
> 
> ---
>  xen/drivers/char/Kconfig   |  4 ++++
>  xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
>  2 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b572305657..abb59fdb0f 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -4,6 +4,10 @@ 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
> +
>  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 v3 2/3] xen/pci: Move x86 specific code to x86 directory.

[Stefano Stabellini]

On Mon, 16 Nov 2020, Rahul Singh wrote:
> passthrough/pci.c file is common for all architecture, but there is x86
> specific code in this file.
> 
> Move x86 specific code to the drivers/passthrough/io.c file to avoid
> compilation error for other architecture.
> 
> As drivers/passthrough/io.c is compiled only for x86 move it to
> x86 directory and rename it to hvm.c.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

This patch breaks the x86 build if you disable CONFIG_HVM:

prelink-efi.o: In function `pci_release_devices':
/local/repos/xen-upstream/xen/drivers/passthrough/pci.c:900: undefined reference to `arch_pci_clean_pirqs'
Makefile:209: recipe for target '/local/repos/xen-upstream/xen/xen.efi' failed



> ---
> 
> Changes in v3:
> - fixed typo
> - As per suggestion move the code to the file io.c and move that file to
>   x86 directory and rename it hvm.c
> 
> ---
>  xen/drivers/passthrough/Makefile            |  3 -
>  xen/drivers/passthrough/pci.c               | 78 +--------------------
>  xen/drivers/passthrough/x86/Makefile        |  1 +
>  xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 +++++++++++++++++
>  xen/drivers/passthrough/x86/iommu.c         |  7 ++
>  xen/include/xen/pci.h                       |  2 +
>  6 files changed, 77 insertions(+), 80 deletions(-)
>  rename xen/drivers/passthrough/{io.c => x86/hvm.c} (95%)
> 
> diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
> index e973e16c74..cc646612c7 100644
> --- a/xen/drivers/passthrough/Makefile
> +++ b/xen/drivers/passthrough/Makefile
> @@ -6,6 +6,3 @@ obj-$(CONFIG_ARM) += arm/
>  obj-y += iommu.o
>  obj-$(CONFIG_HAS_PCI) += pci.o
>  obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> -
> -x86-$(CONFIG_HVM) := io.o
> -obj-$(CONFIG_X86) += $(x86-y)
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 51e584127e..e8a28df126 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -14,9 +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>
>  #include <xen/list.h>
>  #include <xen/prefetch.h>
> @@ -24,7 +21,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>
> @@ -842,71 +838,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)
> @@ -966,7 +897,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();
> @@ -1370,13 +1301,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 a70cf9460d..69284a5d19 100644
> --- a/xen/drivers/passthrough/x86/Makefile
> +++ b/xen/drivers/passthrough/x86/Makefile
> @@ -1,2 +1,3 @@
>  obj-y += ats.o
>  obj-y += iommu.o
> +obj-$(CONFIG_HVM) += hvm.o
> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/x86/hvm.c
> similarity index 95%
> rename from xen/drivers/passthrough/io.c
> rename to xen/drivers/passthrough/x86/hvm.c
> index 6b1305a3e5..41cfa2e200 100644
> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -1036,6 +1036,72 @@ unlock:
>      spin_unlock(&d->event_lock);
>  }
>  
> +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;
> +}
> +
>  /*
>   * Note: 'pt_pirq_softirq_reset' can clear the STATE_SCHED before we get to
>   * doing it. If that is the case we let 'pt_pirq_softirq_reset' do ref-counting.
> 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/include/xen/pci.h b/xen/include/xen/pci.h
> index 20a54a5bb4..78d83afe64 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -208,4 +208,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 v3 2/3] xen/pci: Move x86 specific code to x86 directory.

[Rahul Singh]

Hello Stefano,

> On 17 Nov 2020, at 1:20 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 16 Nov 2020, Rahul Singh wrote:
>> passthrough/pci.c file is common for all architecture, but there is x86
>> specific code in this file.
>> 
>> Move x86 specific code to the drivers/passthrough/io.c file to avoid
>> compilation error for other architecture.
>> 
>> As drivers/passthrough/io.c is compiled only for x86 move it to
>> x86 directory and rename it to hvm.c.
>> 
>> No functional change.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> This patch breaks the x86 build if you disable CONFIG_HVM:
> 
> prelink-efi.o: In function `pci_release_devices':
> /local/repos/xen-upstream/xen/drivers/passthrough/pci.c:900: undefined reference to `arch_pci_clean_pirqs'
> Makefile:209: recipe for target '/local/repos/xen-upstream/xen/xen.efi' failed
> 
> 

Thanks for reviewing the code.
I will fix the build and will send the v3 patch.

Regards,
Rahul

> 
>> ---
>> 
>> Changes in v3:
>> - fixed typo
>> - As per suggestion move the code to the file io.c and move that file to
>>  x86 directory and rename it hvm.c
>> 
>> ---
>> xen/drivers/passthrough/Makefile            |  3 -
>> xen/drivers/passthrough/pci.c               | 78 +--------------------
>> xen/drivers/passthrough/x86/Makefile        |  1 +
>> xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 +++++++++++++++++
>> xen/drivers/passthrough/x86/iommu.c         |  7 ++
>> xen/include/xen/pci.h                       |  2 +
>> 6 files changed, 77 insertions(+), 80 deletions(-)
>> rename xen/drivers/passthrough/{io.c => x86/hvm.c} (95%)
>> 
>> diff --git a/xen/drivers/passthrough/Makefile b/xen/drivers/passthrough/Makefile
>> index e973e16c74..cc646612c7 100644
>> --- a/xen/drivers/passthrough/Makefile
>> +++ b/xen/drivers/passthrough/Makefile
>> @@ -6,6 +6,3 @@ obj-$(CONFIG_ARM) += arm/
>> obj-y += iommu.o
>> obj-$(CONFIG_HAS_PCI) += pci.o
>> obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
>> -
>> -x86-$(CONFIG_HVM) := io.o
>> -obj-$(CONFIG_X86) += $(x86-y)
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index 51e584127e..e8a28df126 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -14,9 +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>
>> #include <xen/list.h>
>> #include <xen/prefetch.h>
>> @@ -24,7 +21,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>
>> @@ -842,71 +838,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)
>> @@ -966,7 +897,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();
>> @@ -1370,13 +1301,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 a70cf9460d..69284a5d19 100644
>> --- a/xen/drivers/passthrough/x86/Makefile
>> +++ b/xen/drivers/passthrough/x86/Makefile
>> @@ -1,2 +1,3 @@
>> obj-y += ats.o
>> obj-y += iommu.o
>> +obj-$(CONFIG_HVM) += hvm.o
>> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/x86/hvm.c
>> similarity index 95%
>> rename from xen/drivers/passthrough/io.c
>> rename to xen/drivers/passthrough/x86/hvm.c
>> index 6b1305a3e5..41cfa2e200 100644
>> --- a/xen/drivers/passthrough/io.c
>> +++ b/xen/drivers/passthrough/x86/hvm.c
>> @@ -1036,6 +1036,72 @@ unlock:
>>     spin_unlock(&d->event_lock);
>> }
>> 
>> +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;
>> +}
>> +
>> /*
>>  * Note: 'pt_pirq_softirq_reset' can clear the STATE_SCHED before we get to
>>  * doing it. If that is the case we let 'pt_pirq_softirq_reset' do ref-counting.
>> 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/include/xen/pci.h b/xen/include/xen/pci.h
>> index 20a54a5bb4..78d83afe64 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -208,4 +208,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 v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 16.11.2020 13:25, Rahul Singh wrote:
> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
> is enabled for ARM, compilation error is observed for ARM architecture
> because ARM platforms do not have full PCI support available.

While you've extended the sentence, it remains unclear to me what
compilation error it is that results here. I've requested such
clarification for v2 already.

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -4,6 +4,10 @@ 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

Looking at this again (in particular at all the #ifdef changes in
the actual source file), I wonder whether an approach with less
code churn and without such an extra Kconfig setting (with, as
said, a bogus dependency on x86) couldn't be found. For example,
how about ...

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

... #undef-ining CONFIG_HAS_PCI at a suitable position in this
file (e.g. after all #include-s, to make sure all structure
layouts remain correct)? This would then be far easier to revert
down the road, and would confine the oddity to a single file
(and there a single place) in the code base.

Jan

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

[Jan Beulich]

On 16.11.2020 13:25, Rahul Singh wrote:
> passthrough/pci.c file is common for all architecture, but there is x86
> specific code in this file.

In how far is ...

> @@ -1370,13 +1301,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;
> -}

... this code x86-specific? The hook being called lives in a
#ifdef CONFIG_HAS_PCI section, and MSI is a general PCI sub-
feature. IOW if this is another workaround, it should be
called so (if there's really no other way to address whatever
issue there is), which in turn likely means it wants to be in
a separate patch.

Jan

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

[Jan Beulich]

On 16.11.2020 13:25, Rahul Singh wrote:
> If mem-sharing, mem-paging, or log-dirty functionality is not enabled
> for non-x86 architecture when HAS_PCI is enabled, the compiler will
> throw an error.
> 
> Move code to x86 specific directory to fix compilation error.

Perhaps rather "file" than "directory"?

> Also, modify the code to use likely() in place of unlikley() for each
> condition to make code more optimized.
> 
> No functional change.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>

In principle I'm okay with this now, but there continue to be a few
nits:

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

Please insert this alongside the other "#include <xen/...>" higher up.

> @@ -315,6 +316,17 @@ int iommu_update_ire_from_msi(
>             ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>  }
>  
> +bool arch_iommu_use_permitted(const struct domain *d)
> +{
> +    /*
> +     * Prevent device assign if mem paging, mem sharing or log-dirty
> +     * have been enabled for this domain.
> +     */
> +    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));
> +}
>  /*
>   * Local variables:
>   * mode: C

Please don't alter stylistic aspects like this trailing comment
being preceded by a blank line.

> --- 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 arch_iommu_use_permitted(const struct domain *d);

Just FTR - this way you effectively preclude an arch from
making this a trivial static inline in one of its headers.

Jan

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

[Rahul Singh]

Hello Jan,

> On 17 Nov 2020, at 11:12 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.11.2020 13:25, Rahul Singh wrote:
>> If mem-sharing, mem-paging, or log-dirty functionality is not enabled
>> for non-x86 architecture when HAS_PCI is enabled, the compiler will
>> throw an error.
>> 
>> Move code to x86 specific directory to fix compilation error.
> 
> Perhaps rather "file" than "directory”?
Ok.
> 
>> Also, modify the code to use likely() in place of unlikley() for each
>> condition to make code more optimized.
>> 
>> No functional change.
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> In principle I'm okay with this now, but there continue to be a few
> nits:

Thanks for reviewing the code I will fix all comments and will share the next version.
> 
>> --- 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>
> 
> Please insert this alongside the other "#include <xen/...>" higher up.

Ok.
> 
>> @@ -315,6 +316,17 @@ int iommu_update_ire_from_msi(
>>            ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>> }
>> 
>> +bool arch_iommu_use_permitted(const struct domain *d)
>> +{
>> +    /*
>> +     * Prevent device assign if mem paging, mem sharing or log-dirty
>> +     * have been enabled for this domain.
>> +     */
>> +    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));
>> +}
>> /*
>>  * Local variables:
>>  * mode: C
> 
> Please don't alter stylistic aspects like this trailing comment
> being preceded by a blank line.

Ok.
> 
>> --- 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 arch_iommu_use_permitted(const struct domain *d);
> 
> Just FTR - this way you effectively preclude an arch from
> making this a trivial static inline in one of its headers.
> 
> Jan

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

[Rahul Singh]

Hello Jan,

> On 17 Nov 2020, at 11:03 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.11.2020 13:25, Rahul Singh wrote:
>> passthrough/pci.c file is common for all architecture, but there is x86
>> specific code in this file.
> 
> In how far is ...
> 
>> @@ -1370,13 +1301,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;
>> -}
> 
> ... this code x86-specific? The hook being called lives in a
> #ifdef CONFIG_HAS_PCI section, and MSI is a general PCI sub-
> feature. IOW if this is another workaround, it should be
> called so (if there's really no other way to address whatever
> issue there is), which in turn likely means it wants to be in
> a separate patch.
> 

As of now there is no implementation for ARM to remap the interrupt and interrupt remapping is enabled for x86 only so I thought I will move this code to x86 file as this function is called from the x86 specific code only.

I will remove this code form this patch and will fix this once once will have proper MSI implementation for ARM.

> Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Rahul Singh]

[-- Attachment #1: Type: text/plain, Size: 2669 bytes --]

Hello Jan,

> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 16.11.2020 13:25, Rahul Singh wrote:
>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>> is enabled for ARM, compilation error is observed for ARM architecture
>> because ARM platforms do not have full PCI support available.
> 
> While you've extended the sentence, it remains unclear to me what
> compilation error it is that results here. I've requested such
> clarification for v2 already.

Compilation error is related to the code that refer to x86  functions (create_irq()..) and MSI implementation related error. 
For more details please find the attached file for compilation error.

> 
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -4,6 +4,10 @@ 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
> 
> Looking at this again (in particular at all the #ifdef changes in
> the actual source file), I wonder whether an approach with less
> code churn and without such an extra Kconfig setting (with, as
> said, a bogus dependency on x86) couldn't be found. For example,
> how about ...
> 
>> --- 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>
> 
> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this
> file (e.g. after all #include-s, to make sure all structure
> layouts remain correct)? This would then be far easier to revert
> down the road, and would confine the oddity to a single file
> (and there a single place) in the code base.
> 

As for ARM platforms, PCI implementation is in the development process and I am not sure if after completion of PCI work, ns16500 PCI part of code will work out of the box. I think there is some effort required to test the ns16550 PCI part of the code on ARM.
As this code is tested on X86 only so I make the options depends on X86 and enable it by default for x86.  

I feel that adding a new Kconfig options is ok to enable/disable the PCI NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If in future other architecture wants to implement the PCI they will face the similar compilation error issues.

Please suggest how we can proceed on this.


> Jan

Regards,
Rahul

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: compilation error.rtf --]
[-- Type: text/rtf; name="compilation error.rtf", Size: 10164 bytes --]

{\rtf1\ansi\ansicpg1252\cocoartf2513
\cocoatextscaling0\cocoaplatform0{\fonttbl\f0\fnil\fcharset0 Menlo-Bold;\f1\fnil\fcharset0 Menlo-Regular;}
{\colortbl;\red255\green255\blue255;\red180\green36\blue25;\red0\green0\blue0;\red47\green180\blue29;
\red46\green174\blue187;}
{\*\expandedcolortbl;;\cssrgb\c76409\c21698\c12524;\csgray\c0;\cssrgb\c20238\c73898\c14947;
\cssrgb\c20196\c73240\c78250;}
\paperw11900\paperh16840\margl1440\margr1440\vieww28600\viewh18000\viewkind0
\pard\tx560\tx1120\tx1680\tx2240\tx2800\tx3360\tx3920\tx4480\tx5040\tx5600\tx6160\tx6720\pardirnatural\partightenfactor0

\f0\b\fs36 \cf2 \CocoaLigature0  \cf3 ns16550.c:
\f1\b0  In function \'91
\f0\b ns16550_init_irq
\f1\b0 \'92:\
\pard\tx560\tx1120\tx1680\tx2240\tx2800\tx3360\tx3920\tx4480\tx5040\tx5600\tx6160\tx6720\pardirnatural\partightenfactor0

\f0\b ns16550.c:726:21:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 implicit declaration of function \'91
\f0\b create_irq
\f1\b0 \'92; did you mean \'91
\f0\b release_irq
\f1\b0 \'92? [
\f0\b \cf2 -Werror=implicit-function-declaration
\f1\b0 \cf3 ]\
         uart->irq = 
\f0\b \cf2 create_irq
\f1\b0 \cf3 (0, false);\
                     
\f0\b \cf2 ^~~~~~~~~~
\f1\b0 \cf3 \
                     \cf4 release_irq\cf3 \

\f0\b ns16550.c:726:21:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 nested extern declaration of \'91
\f0\b create_irq
\f1\b0 \'92 [
\f0\b \cf2 -Werror=nested-externs
\f1\b0 \cf3 ]\

\f0\b ns16550.c:
\f1\b0  In function \'91
\f0\b ns16550_init_postirq
\f1\b0 \'92:\

\f0\b ns16550.c:768:33:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 \'91
\f0\b mmio_ro_ranges
\f1\b0 \'92 undeclared (first use in this function); did you mean \'91
\f0\b mmio_handler
\f1\b0 \'92?\
              rangeset_add_range(
\f0\b \cf2 mmio_ro_ranges
\f1\b0 \cf3 , uart->io_base,\
                                 
\f0\b \cf2 ^~~~~~~~~~~~~~
\f1\b0 \cf3 \
                                 \cf4 mmio_handler\cf3 \

\f0\b ns16550.c:768:33:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 each undeclared identifier is reported only once for each function it appears in\

\f0\b ns16550.c:780:20:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 variable \'91
\f0\b msi
\f1\b0 \'92 has initializer but incomplete type\
             struct 
\f0\b \cf2 msi_info
\f1\b0 \cf3  msi = \{\
                    
\f0\b \cf2 ^~~~~~~~
\f1\b0 \cf3 \

\f0\b ns16550.c:781:18:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 \'91
\f0\b struct msi_info
\f1\b0 \'92 has no member named \'91
\f0\b bus
\f1\b0 \'92\
                 .
\f0\b \cf2 bus
\f1\b0 \cf3  = uart->ps_bdf[0],\
                  
\f0\b \cf2 ^~~
\f1\b0 \cf3 \

\f0\b ns16550.c:781:24:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 excess elements in struct initializer [
\f0\b \cf2 -Werror
\f1\b0 \cf3 ]\
                 .bus = 
\f0\b \cf2 uart
\f1\b0 \cf3 ->ps_bdf[0],\
                        
\f0\b \cf2 ^~~~
\f1\b0 \cf3 \

\f0\b ns16550.c:781:24:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 (near initialization for \'91
\f0\b msi
\f1\b0 \'92)\

\f0\b ns16550.c:782:18:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 \'91
\f0\b struct msi_info
\f1\b0 \'92 has no member named \'91
\f0\b devfn
\f1\b0 \'92\
                 .
\f0\b \cf2 devfn
\f1\b0 \cf3  = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),\
                  
\f0\b \cf2 ^~~~~
\f1\b0 \cf3 \
In file included from 
\f0\b /home/rahsin01/work/xen-scm-git-code/fresh-test-code/xen/xen/include/xen/iommu.h:25:0
\f1\b0 ,\
                 from 
\f0\b /home/rahsin01/work/xen-scm-git-code/fresh-test-code/xen/xen/include/xen/sched.h:12
\f1\b0 ,\
                 from 
\f0\b ns16550.c:15
\f1\b0 :\

\f0\b /home/rahsin01/work/xen-scm-git-code/fresh-test-code/xen/xen/include/xen/pci.h:33:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 excess elements in struct initializer [
\f0\b \cf2 -Werror
\f1\b0 \cf3 ]\
 #define PCI_DEVFN(d,f)  
\f0\b \cf2 (
\f1\b0 \cf3 (((d) & 0x1f) << 3) | ((f) & 0x07))\
                         
\f0\b \cf2 ^
\f1\b0 \cf3 \

\f0\b ns16550.c:782:26:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 in expansion of macro \'91
\f0\b PCI_DEVFN
\f1\b0 \'92\
                 .devfn = 
\f0\b \cf5 PCI_DEVFN
\f1\b0 \cf3 (uart->ps_bdf[1], uart->ps_bdf[2]),\
                          
\f0\b \cf5 ^~~~~~~~~
\f1\b0 \cf3 \

\f0\b /home/rahsin01/work/xen-scm-git-code/fresh-test-code/xen/xen/include/xen/pci.h:33:25:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 (near initialization for \'91
\f0\b msi
\f1\b0 \'92)\
 #define PCI_DEVFN(d,f)  
\f0\b \cf5 (
\f1\b0 \cf3 (((d) & 0x1f) << 3) | ((f) & 0x07))\
                         
\f0\b \cf5 ^
\f1\b0 \cf3 \

\f0\b ns16550.c:782:26:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 in expansion of macro \'91
\f0\b PCI_DEVFN
\f1\b0 \'92\
                 .devfn = 
\f0\b \cf5 PCI_DEVFN
\f1\b0 \cf3 (uart->ps_bdf[1], uart->ps_bdf[2]),\
                          
\f0\b \cf5 ^~~~~~~~~
\f1\b0 \cf3 \

\f0\b ns16550.c:783:18:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 \'91
\f0\b struct msi_info
\f1\b0 \'92 has no member named \'91
\f0\b irq
\f1\b0 \'92\
                 .
\f0\b \cf2 irq
\f1\b0 \cf3  = rc = uart->irq,\
                  
\f0\b \cf2 ^~~
\f1\b0 \cf3 \

\f0\b ns16550.c:783:24:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 excess elements in struct initializer [
\f0\b \cf2 -Werror
\f1\b0 \cf3 ]\
                 .irq = 
\f0\b \cf2 rc
\f1\b0 \cf3  = uart->irq,\
                        
\f0\b \cf2 ^~
\f1\b0 \cf3 \

\f0\b ns16550.c:783:24:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 (near initialization for \'91
\f0\b msi
\f1\b0 \'92)\

\f0\b ns16550.c:784:18:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 \'91
\f0\b struct msi_info
\f1\b0 \'92 has no member named \'91
\f0\b entry_nr
\f1\b0 \'92\
                 .
\f0\b \cf2 entry_nr
\f1\b0 \cf3  = 1\
                  
\f0\b \cf2 ^~~~~~~~
\f1\b0 \cf3 \

\f0\b ns16550.c:784:29:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 excess elements in struct initializer [
\f0\b \cf2 -Werror
\f1\b0 \cf3 ]\
                 .entry_nr = 
\f0\b \cf2 1
\f1\b0 \cf3 \
                             
\f0\b \cf2 ^
\f1\b0 \cf3 \

\f0\b ns16550.c:784:29:
\f1\b0  
\f0\b \cf5 note: 
\f1\b0 \cf3 (near initialization for \'91
\f0\b msi
\f1\b0 \'92)\

\f0\b ns16550.c:780:29:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 storage size of \'91
\f0\b msi
\f1\b0 \'92 isn\'92t known\
             struct msi_info 
\f0\b \cf2 msi
\f1\b0 \cf3  = \{\
                             
\f0\b \cf2 ^~~
\f1\b0 \cf3 \

\f0\b ns16550.c:793:22:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 implicit declaration of function \'91
\f0\b pci_enable_msi
\f1\b0 \'92; did you mean \'91
\f0\b hap_enabled
\f1\b0 \'92? [
\f0\b \cf2 -Werror=implicit-function-declaration
\f1\b0 \cf3 ]\
                 rc = 
\f0\b \cf2 pci_enable_msi
\f1\b0 \cf3 (&msi, &msi_desc);\
                      
\f0\b \cf2 ^~~~~~~~~~~~~~
\f1\b0 \cf3 \
                      \cf4 hap_enabled\cf3 \

\f0\b ns16550.c:793:22:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 nested extern declaration of \'91
\f0\b pci_enable_msi
\f1\b0 \'92 [
\f0\b \cf2 -Werror=nested-externs
\f1\b0 \cf3 ]\

\f0\b ns16550.c:800:26:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 implicit declaration of function \'91
\f0\b setup_msi_irq
\f1\b0 \'92; did you mean \'91
\f0\b setup_irq
\f1\b0 \'92? [
\f0\b \cf2 -Werror=implicit-function-declaration
\f1\b0 \cf3 ]\
                     rc = 
\f0\b \cf2 setup_msi_irq
\f1\b0 \cf3 (desc, msi_desc);\
                          
\f0\b \cf2 ^~~~~~~~~~~~~
\f1\b0 \cf3 \
                          \cf4 setup_irq\cf3 \

\f0\b ns16550.c:800:26:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 nested extern declaration of \'91
\f0\b setup_msi_irq
\f1\b0 \'92 [
\f0\b \cf2 -Werror=nested-externs
\f1\b0 \cf3 ]\

\f0\b ns16550.c:803:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 implicit declaration of function \'91
\f0\b pci_disable_msi
\f1\b0 \'92; did you mean \'91
\f0\b gic_disable_cpu
\f1\b0 \'92? [
\f0\b \cf2 -Werror=implicit-function-declaration
\f1\b0 \cf3 ]\
                         
\f0\b \cf2 pci_disable_msi
\f1\b0 \cf3 (msi_desc);\
                         
\f0\b \cf2 ^~~~~~~~~~~~~~~
\f1\b0 \cf3 \
                         \cf4 gic_disable_cpu\cf3 \

\f0\b ns16550.c:803:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 nested extern declaration of \'91
\f0\b pci_disable_msi
\f1\b0 \'92 [
\f0\b \cf2 -Werror=nested-externs
\f1\b0 \cf3 ]\

\f0\b ns16550.c:812:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 implicit declaration of function \'91
\f0\b msi_free_irq
\f1\b0 \'92; did you mean \'91
\f0\b vgic_free_virq
\f1\b0 \'92? [
\f0\b \cf2 -Werror=implicit-function-declaration
\f1\b0 \cf3 ]\
                         
\f0\b \cf2 msi_free_irq
\f1\b0 \cf3 (msi_desc);\
                         
\f0\b \cf2 ^~~~~~~~~~~~
\f1\b0 \cf3 \
                         \cf4 vgic_free_virq\cf3 \

\f0\b ns16550.c:812:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 nested extern declaration of \'91
\f0\b msi_free_irq
\f1\b0 \'92 [
\f0\b \cf2 -Werror=nested-externs
\f1\b0 \cf3 ]\

\f0\b ns16550.c:814:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 implicit declaration of function \'91
\f0\b destroy_irq
\f1\b0 \'92; did you mean \'91
\f0\b setup_irq
\f1\b0 \'92? [
\f0\b \cf2 -Werror=implicit-function-declaration
\f1\b0 \cf3 ]\
                         
\f0\b \cf2 destroy_irq
\f1\b0 \cf3 (msi.irq);\
                         
\f0\b \cf2 ^~~~~~~~~~~
\f1\b0 \cf3 \
                         \cf4 setup_irq\cf3 \

\f0\b ns16550.c:814:25:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 nested extern declaration of \'91
\f0\b destroy_irq
\f1\b0 \'92 [
\f0\b \cf2 -Werror=nested-externs
\f1\b0 \cf3 ]\

\f0\b ns16550.c:780:29:
\f1\b0  
\f0\b \cf2 error: 
\f1\b0 \cf3 unused variable \'91
\f0\b msi
\f1\b0 \'92 [
\f0\b \cf2 -Werror=unused-variable
\f1\b0 \cf3 ]\
             struct msi_info 
\f0\b \cf2 msi
\f1\b0 \cf3  = \{\
                             
\f0\b \cf2 ^~~
\f1\b0 \cf3 \
}

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 18.11.2020 16:02, Rahul Singh wrote:
> Hello Jan,
> 
>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 16.11.2020 13:25, Rahul Singh wrote:
>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>> is enabled for ARM, compilation error is observed for ARM architecture
>>> because ARM platforms do not have full PCI support available.
>>
>> While you've extended the sentence, it remains unclear to me what
>> compilation error it is that results here. I've requested such
>> clarification for v2 already.
> 
> Compilation error is related to the code that refer to x86  functions (create_irq()..) and MSI implementation related error. 
> For more details please find the attached file for compilation error.

The use of mmio_ro_ranges is almost quite possibly going to remain
x86-specific, but then I guess this wants abstracting in a suitable
way.

The remaining look to all be MSI-related, so perhaps what you want
to avoid is just that part rather than everything PCI-ish?

>>> --- 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>
>>
>> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this
>> file (e.g. after all #include-s, to make sure all structure
>> layouts remain correct)? This would then be far easier to revert
>> down the road, and would confine the oddity to a single file
>> (and there a single place) in the code base.
>>
> 
> As for ARM platforms, PCI implementation is in the development process and I am not sure if after completion of PCI work, ns16500 PCI part of code will work out of the box. I think there is some effort required to test the ns16550 PCI part of the code on ARM.
> As this code is tested on X86 only so I make the options depends on X86 and enable it by default for x86.  
> 
> I feel that adding a new Kconfig options is ok to enable/disable the PCI NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If in future other architecture wants to implement the PCI they will face the similar compilation error issues.
> 
> Please suggest how we can proceed on this.

Introduce CONFIG_HAS_PCI_MSI (selected only by x86), if there's no
immediate plan to support it on Arm together with the rest of PCI?

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

Hi,

On 18/11/2020 15:16, Jan Beulich wrote:
> On 18.11.2020 16:02, Rahul Singh wrote:
>> Hello Jan,
>>
>>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 16.11.2020 13:25, Rahul Singh wrote:
>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>> because ARM platforms do not have full PCI support available.
>>>
>>> While you've extended the sentence, it remains unclear to me what
>>> compilation error it is that results here. I've requested such
>>> clarification for v2 already.
>>
>> Compilation error is related to the code that refer to x86  functions (create_irq()..) and MSI implementation related error.
>> For more details please find the attached file for compilation error.
> 
> The use of mmio_ro_ranges is almost quite possibly going to remain
> x86-specific, but then I guess this wants abstracting in a suitable
> way.
> 
> The remaining look to all be MSI-related, so perhaps what you want
> to avoid is just that part rather than everything PCI-ish?

Not really (see more above).

> 
>>>> --- 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>
>>>
>>> ... #undef-ining CONFIG_HAS_PCI at a suitable position in this
>>> file (e.g. after all #include-s, to make sure all structure
>>> layouts remain correct)? This would then be far easier to revert
>>> down the road, and would confine the oddity to a single file
>>> (and there a single place) in the code base.
>>>
>>
>> As for ARM platforms, PCI implementation is in the development process and I am not sure if after completion of PCI work, ns16500 PCI part of code will work out of the box. I think there is some effort required to test the ns16550 PCI part of the code on ARM.
>> As this code is tested on X86 only so I make the options depends on X86 and enable it by default for x86.
>>
>> I feel that adding a new Kconfig options is ok to enable/disable the PCI NS16550 support as compared to #undef CONFIG_HAS_PCI in the particular file. If in future other architecture wants to implement the PCI they will face the similar compilation error issues.
>>
>> Please suggest how we can proceed on this.
> 
> Introduce CONFIG_HAS_PCI_MSI (selected only by x86), if there's no
> immediate plan to support it on Arm together with the rest of PCI?

So even we are going to enable PCI on Arm and fix compilation issue, 
there are no way the NS16550 PCI would be usuable without effort for a 
few reasons:

   1) com1/com2 is x86 specific
   2) ns16550_init() is not used by Arm and the only way to use a PCI UART
   3) UART is discovered through the device-tree/ACPI tables on Arm

So I think CONFIG_HAS_NS16550_PCI is most suitable solution and we 
should probably guard more code (e.g. ns16550_init(), com1, com2...).

Note that's not a request for doing it in this patch :).

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

Hi Rahul,

On 16/11/2020 12:25, Rahul Singh wrote:
> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
> is enabled for ARM, compilation error is observed for ARM architecture
> because ARM platforms do not have full PCI support available.
 >
> 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.

NIT: I would say "No functional change intended" to make clear this is 
an expectation and hopefully will be correct :).

Regarding the commit message itself, I would suggest the following to 
address Jan's concern:

"
xen/char: ns16550: Gate all PCI code with a new Kconfig HAS_NS16550_PCI

The NS16550 driver is assuming that NS16550 PCI card are usable if the 
architecture supports PCI (i.e. CONFIG_HAS_PCI=y). However, the code is 
very x86 focus and will fail to build on Arm (/!\ it is not all the errors):

  ns16550.c: In function ‘ns16550_init_irq’:
ns16550.c:726:21: error: implicit declaration of function ‘create_irq’; 
did you mean ‘release_irq’? [-Werror=implicit-function-declaration]
          uart->irq = create_irq(0, false);
                      ^~~~~~~~~~
                      release_irq
ns16550.c:726:21: error: nested extern declaration of ‘create_irq’ 
[-Werror=nested-externs]
ns16550.c: In function ‘ns16550_init_postirq’:
ns16550.c:768:33: error: ‘mmio_ro_ranges’ undeclared (first use in this 
function); did you mean ‘mmio_handler’?
               rangeset_add_range(mmio_ro_ranges, uart->io_base,
                                  ^~~~~~~~~~~~~~
                                  mmio_handler
ns16550.c:768:33: note: each undeclared identifier is reported only once 
for each function it appears in
ns16550.c:780:20: error: variable ‘msi’ has initializer but incomplete type
              struct msi_info msi = {
                     ^~~~~~~~
ns16550.c:781:18: error: ‘struct msi_info’ has no member named ‘bus’
                  .bus = uart->ps_bdf[0],
                   ^~~
ns16550.c:781:24: error: excess elements in struct initializer [-Werror]
                  .bus = uart->ps_bdf[0],
                         ^~~~
ns16550.c:781:24: note: (near initialization for ‘msi’)
ns16550.c:782:18: error: ‘struct msi_info’ has no member named ‘devfn’
                  .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),

Enabling support for NS16550 PCI card on Arm would require more plumbing 
in addition to fixing the compilation error.

Arm systems tend to have platform UART available such as NS16550, PL011. 
So there are limited reasons to get NS16550 PCI support for now on Arm.

A new Kconfig option CONFIG_HAS_NS16550_PCI is introduced to gate all 
the PCI code.

This option will be select automically for x86 platform and left 
unselectable on Arm.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
[julieng: Commit message]
Signed-off-by: Julien Grall <jgrall@amazon.com>
"

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 18.11.2020 16:35, Julien Grall wrote:
> So even we are going to enable PCI on Arm and fix compilation issue, 
> there are no way the NS16550 PCI would be usuable without effort for a 
> few reasons:
> 
>    1) com1/com2 is x86 specific
>    2) ns16550_init() is not used by Arm and the only way to use a PCI UART

This is a good observation, and I wasn't aware of this. I'm sending
a patch which ...

>    3) UART is discovered through the device-tree/ACPI tables on Arm
> 
> So I think CONFIG_HAS_NS16550_PCI is most suitable solution and we 
> should probably guard more code (e.g. ns16550_init(), com1, com2...).

... hopefully fulfills this (to be considered a prereq to Rahul's
series then).

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 18.11.2020 16:35, Julien Grall wrote:
> On 18/11/2020 15:16, Jan Beulich wrote:
>> On 18.11.2020 16:02, Rahul Singh wrote:
>>> Hello Jan,
>>>
>>>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 16.11.2020 13:25, Rahul Singh wrote:
>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>>> because ARM platforms do not have full PCI support available.
>>>>
>>>> While you've extended the sentence, it remains unclear to me what
>>>> compilation error it is that results here. I've requested such
>>>> clarification for v2 already.
>>>
>>> Compilation error is related to the code that refer to x86  functions (create_irq()..) and MSI implementation related error.
>>> For more details please find the attached file for compilation error.
>>
>> The use of mmio_ro_ranges is almost quite possibly going to remain
>> x86-specific, but then I guess this wants abstracting in a suitable
>> way.
>>
>> The remaining look to all be MSI-related, so perhaps what you want
>> to avoid is just that part rather than everything PCI-ish?
> 
> Not really (see more above).

Did you really mean "above", not "below"? If so, I guess I need some
clarification. If not, I suppose I've addressed your concern by the
2-patch series I've just sent.

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 18.11.2020 16:50, Julien Grall wrote:
> On 16/11/2020 12:25, Rahul Singh wrote:
>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>> is enabled for ARM, compilation error is observed for ARM architecture
>> because ARM platforms do not have full PCI support available.
>  >
>> 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.
> 
> NIT: I would say "No functional change intended" to make clear this is 
> an expectation and hopefully will be correct :).
> 
> Regarding the commit message itself, I would suggest the following to 
> address Jan's concern:

While indeed this is a much better description, I continue to think
that the proposed Kconfig option is undesirable to have. Either,
following the patch I've just sent, truly x86-specific things (at
least as far as current state goes - if any of this was to be
re-used by a future port, suitable further abstraction may be
needed) should be guarded by CONFIG_X86 (or abstracted into arch
hooks), or the HAS_PCI_MSI proposal would at least want further
investigating as to its feasibility to address the issues at hand.

Jan

> "
> xen/char: ns16550: Gate all PCI code with a new Kconfig HAS_NS16550_PCI
> 
> The NS16550 driver is assuming that NS16550 PCI card are usable if the 
> architecture supports PCI (i.e. CONFIG_HAS_PCI=y). However, the code is 
> very x86 focus and will fail to build on Arm (/!\ it is not all the errors):
> 
>   ns16550.c: In function ‘ns16550_init_irq’:
> ns16550.c:726:21: error: implicit declaration of function ‘create_irq’; 
> did you mean ‘release_irq’? [-Werror=implicit-function-declaration]
>           uart->irq = create_irq(0, false);
>                       ^~~~~~~~~~
>                       release_irq
> ns16550.c:726:21: error: nested extern declaration of ‘create_irq’ 
> [-Werror=nested-externs]
> ns16550.c: In function ‘ns16550_init_postirq’:
> ns16550.c:768:33: error: ‘mmio_ro_ranges’ undeclared (first use in this 
> function); did you mean ‘mmio_handler’?
>                rangeset_add_range(mmio_ro_ranges, uart->io_base,
>                                   ^~~~~~~~~~~~~~
>                                   mmio_handler
> ns16550.c:768:33: note: each undeclared identifier is reported only once 
> for each function it appears in
> ns16550.c:780:20: error: variable ‘msi’ has initializer but incomplete type
>               struct msi_info msi = {
>                      ^~~~~~~~
> ns16550.c:781:18: error: ‘struct msi_info’ has no member named ‘bus’
>                   .bus = uart->ps_bdf[0],
>                    ^~~
> ns16550.c:781:24: error: excess elements in struct initializer [-Werror]
>                   .bus = uart->ps_bdf[0],
>                          ^~~~
> ns16550.c:781:24: note: (near initialization for ‘msi’)
> ns16550.c:782:18: error: ‘struct msi_info’ has no member named ‘devfn’
>                   .devfn = PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2]),
> 
> Enabling support for NS16550 PCI card on Arm would require more plumbing 
> in addition to fixing the compilation error.
> 
> Arm systems tend to have platform UART available such as NS16550, PL011. 
> So there are limited reasons to get NS16550 PCI support for now on Arm.
> 
> A new Kconfig option CONFIG_HAS_NS16550_PCI is introduced to gate all 
> the PCI code.
> 
> This option will be select automically for x86 platform and left 
> unselectable on Arm.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> [julieng: Commit message]
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> "
> 
> Cheers,
> 

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

Hi Jan,

On 19/11/2020 09:05, Jan Beulich wrote:
> On 18.11.2020 16:50, Julien Grall wrote:
>> On 16/11/2020 12:25, Rahul Singh wrote:
>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>> is enabled for ARM, compilation error is observed for ARM architecture
>>> because ARM platforms do not have full PCI support available.
>>   >
>>> 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.
>>
>> NIT: I would say "No functional change intended" to make clear this is
>> an expectation and hopefully will be correct :).
>>
>> Regarding the commit message itself, I would suggest the following to
>> address Jan's concern:
> 
> While indeed this is a much better description, I continue to think
> that the proposed Kconfig option is undesirable to have.

I am yet to see an argument into why we should keep the PCI code 
compiled on Arm when there will be no-use....

> Either,
> following the patch I've just sent, truly x86-specific things (at
> least as far as current state goes - if any of this was to be
> re-used by a future port, suitable further abstraction may be
> needed) should be guarded by CONFIG_X86 (or abstracted into arch
> hooks), or the HAS_PCI_MSI proposal would at least want further
> investigating as to its feasibility to address the issues at hand.

I would be happy with CONFIG_X86, despite the fact that this is only 
deferring the problem.

Regarding HAS_PCI_MSI, I don't really see the point of introducing given 
that we are not going to use NS16550 PCI on Arm in the forseeable 
future. So why do we need a finer graine Kconfig?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

Hi Jan,

On 19/11/2020 09:00, Jan Beulich wrote:
> On 18.11.2020 16:35, Julien Grall wrote:
>> On 18/11/2020 15:16, Jan Beulich wrote:
>>> On 18.11.2020 16:02, Rahul Singh wrote:
>>>> Hello Jan,
>>>>
>>>>> On 17 Nov 2020, at 10:55 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 16.11.2020 13:25, Rahul Singh wrote:
>>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>>>> because ARM platforms do not have full PCI support available.
>>>>>
>>>>> While you've extended the sentence, it remains unclear to me what
>>>>> compilation error it is that results here. I've requested such
>>>>> clarification for v2 already.
>>>>
>>>> Compilation error is related to the code that refer to x86  functions (create_irq()..) and MSI implementation related error.
>>>> For more details please find the attached file for compilation error.
>>>
>>> The use of mmio_ro_ranges is almost quite possibly going to remain
>>> x86-specific, but then I guess this wants abstracting in a suitable
>>> way.
>>>
>>> The remaining look to all be MSI-related, so perhaps what you want
>>> to avoid is just that part rather than everything PCI-ish?
>>
>> Not really (see more above).
> 
> Did you really mean "above", not "below"? If so, I guess I need some
> clarification. If not, I suppose I've addressed your concern by the
> 2-patch series I've just sent.

This was meant to be "below".

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 19.11.2020 10:21, Julien Grall wrote:
> Hi Jan,
> 
> On 19/11/2020 09:05, Jan Beulich wrote:
>> On 18.11.2020 16:50, Julien Grall wrote:
>>> On 16/11/2020 12:25, Rahul Singh wrote:
>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>> because ARM platforms do not have full PCI support available.
>>>   >
>>>> 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.
>>>
>>> NIT: I would say "No functional change intended" to make clear this is
>>> an expectation and hopefully will be correct :).
>>>
>>> Regarding the commit message itself, I would suggest the following to
>>> address Jan's concern:
>>
>> While indeed this is a much better description, I continue to think
>> that the proposed Kconfig option is undesirable to have.
> 
> I am yet to see an argument into why we should keep the PCI code 
> compiled on Arm when there will be no-use....

Well, see my patch suppressing building of quite a part of it.

>> Either,
>> following the patch I've just sent, truly x86-specific things (at
>> least as far as current state goes - if any of this was to be
>> re-used by a future port, suitable further abstraction may be
>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>> hooks), or the HAS_PCI_MSI proposal would at least want further
>> investigating as to its feasibility to address the issues at hand.
> 
> I would be happy with CONFIG_X86, despite the fact that this is only 
> deferring the problem.
> 
> Regarding HAS_PCI_MSI, I don't really see the point of introducing given 
> that we are not going to use NS16550 PCI on Arm in the forseeable 
> future.

And I continue to fail to see what would guarantee this: As soon
as you can plug in such a card into an Arm system, people will
want to be able use it. That's why we had to add support for it
on x86, after all.

> So why do we need a finer graine Kconfig?

Because most of the involved code is indeed MSI-related?

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

On 19/11/2020 09:53, Jan Beulich wrote:
> On 19.11.2020 10:21, Julien Grall wrote:
>> Hi Jan,
>>
>> On 19/11/2020 09:05, Jan Beulich wrote:
>>> On 18.11.2020 16:50, Julien Grall wrote:
>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>>> because ARM platforms do not have full PCI support available.
>>>>    >
>>>>> 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.
>>>>
>>>> NIT: I would say "No functional change intended" to make clear this is
>>>> an expectation and hopefully will be correct :).
>>>>
>>>> Regarding the commit message itself, I would suggest the following to
>>>> address Jan's concern:
>>>
>>> While indeed this is a much better description, I continue to think
>>> that the proposed Kconfig option is undesirable to have.
>>
>> I am yet to see an argument into why we should keep the PCI code
>> compiled on Arm when there will be no-use....
> 
> Well, see my patch suppressing building of quite a part of it.

I will let Rahul figuring out whether your patch series is sufficient to 
fix compilation issues (this is what matters right now).

> 
>>> Either,
>>> following the patch I've just sent, truly x86-specific things (at
>>> least as far as current state goes - if any of this was to be
>>> re-used by a future port, suitable further abstraction may be
>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>>> investigating as to its feasibility to address the issues at hand.
>>
>> I would be happy with CONFIG_X86, despite the fact that this is only
>> deferring the problem.
>>
>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>> that we are not going to use NS16550 PCI on Arm in the forseeable
>> future.
> 
> And I continue to fail to see what would guarantee this: As soon
> as you can plug in such a card into an Arm system, people will
> want to be able use it. That's why we had to add support for it
> on x86, after all.

Well, plug-in PCI cards on Arm has been available for quite a while... 
Yet I haven't heard anyone asking for NS16550 PCI support.

This is probably because SBSA compliant server should always provide an 
SBSA UART (a cut-down version of the PL011). So why would bother to lose 
a PCI slot for yet another UART?

> >> So why do we need a finer graine Kconfig?
> 
> Because most of the involved code is indeed MSI-related?

Possibly, yet it would not be necessary if we don't want NS16550 PCI 
support...

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Rahul Singh]

Hello,

> On 19 Nov 2020, at 10:16 am, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 19/11/2020 09:53, Jan Beulich wrote:
>> On 19.11.2020 10:21, Julien Grall wrote:
>>> Hi Jan,
>>> 
>>> On 19/11/2020 09:05, Jan Beulich wrote:
>>>> On 18.11.2020 16:50, Julien Grall wrote:
>>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>>>> because ARM platforms do not have full PCI support available.
>>>>>   >
>>>>>> 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.
>>>>> 
>>>>> NIT: I would say "No functional change intended" to make clear this is
>>>>> an expectation and hopefully will be correct :).
>>>>> 
>>>>> Regarding the commit message itself, I would suggest the following to
>>>>> address Jan's concern:
>>>> 
>>>> While indeed this is a much better description, I continue to think
>>>> that the proposed Kconfig option is undesirable to have.
>>> 
>>> I am yet to see an argument into why we should keep the PCI code
>>> compiled on Arm when there will be no-use....
>> Well, see my patch suppressing building of quite a part of it.
> 
> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now).

I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error what I observed previously. 
There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under X86 flags.

At top level:
ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
 static const struct ns16550_config __initconst uart_config[] =
                                                ^~~~~~~~~~~
ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
 static const struct ns16550_config_param __initconst uart_param[] = { 


> 
>>>> Either,
>>>> following the patch I've just sent, truly x86-specific things (at
>>>> least as far as current state goes - if any of this was to be
>>>> re-used by a future port, suitable further abstraction may be
>>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>>>> investigating as to its feasibility to address the issues at hand.
>>> 
>>> I would be happy with CONFIG_X86, despite the fact that this is only
>>> deferring the problem.
>>> 
>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>>> that we are not going to use NS16550 PCI on Arm in the forseeable
>>> future.
>> And I continue to fail to see what would guarantee this: As soon
>> as you can plug in such a card into an Arm system, people will
>> want to be able use it. That's why we had to add support for it
>> on x86, after all.
> 
> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI support.
> 
> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why would bother to lose a PCI slot for yet another UART?
> 
>> >> So why do we need a finer graine Kconfig?
>> Because most of the involved code is indeed MSI-related?
> 
> Possibly, yet it would not be necessary if we don't want NS16550 PCI support...

> 

To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed further.

1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to have compilation error 
what we are observing now when HAS_PCI is enabled.

2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation error. 
Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver.  


> Cheers,
> 
> -- 
> Julien Grall


Regards,
Rahul

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 19.11.2020 16:54, Rahul Singh wrote:
>> On 19 Nov 2020, at 10:16 am, Julien Grall <julien@xen.org> wrote:
>> On 19/11/2020 09:53, Jan Beulich wrote:
>>> Well, see my patch suppressing building of quite a part of it.
>>
>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now).
> 
> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error what I observed previously. 
> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under X86 flags.
> 
> At top level:
> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
>  static const struct ns16550_config __initconst uart_config[] =
>                                                 ^~~~~~~~~~~
> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
>  static const struct ns16550_config_param __initconst uart_param[] = { 

Looks like more code movement I need to do in my patch then. I was
never happy about the disconnected placement of these array and
the functions using them ...

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4791 bytes --]

On Thu, 19 Nov 2020, Rahul Singh wrote:
> > On 19/11/2020 09:53, Jan Beulich wrote:
> >> On 19.11.2020 10:21, Julien Grall wrote:
> >>> Hi Jan,
> >>> 
> >>> On 19/11/2020 09:05, Jan Beulich wrote:
> >>>> On 18.11.2020 16:50, Julien Grall wrote:
> >>>>> On 16/11/2020 12:25, Rahul Singh wrote:
> >>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
> >>>>>> is enabled for ARM, compilation error is observed for ARM architecture
> >>>>>> because ARM platforms do not have full PCI support available.
> >>>>>   >
> >>>>>> 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.
> >>>>> 
> >>>>> NIT: I would say "No functional change intended" to make clear this is
> >>>>> an expectation and hopefully will be correct :).
> >>>>> 
> >>>>> Regarding the commit message itself, I would suggest the following to
> >>>>> address Jan's concern:
> >>>> 
> >>>> While indeed this is a much better description, I continue to think
> >>>> that the proposed Kconfig option is undesirable to have.
> >>> 
> >>> I am yet to see an argument into why we should keep the PCI code
> >>> compiled on Arm when there will be no-use....
> >> Well, see my patch suppressing building of quite a part of it.
> > 
> > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right now).
> 
> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error what I observed previously. 
> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under X86 flags.
> 
> At top level:
> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
>  static const struct ns16550_config __initconst uart_config[] =
>                                                 ^~~~~~~~~~~
> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
>  static const struct ns16550_config_param __initconst uart_param[] = { 
> 
> 
> > 
> >>>> Either,
> >>>> following the patch I've just sent, truly x86-specific things (at
> >>>> least as far as current state goes - if any of this was to be
> >>>> re-used by a future port, suitable further abstraction may be
> >>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
> >>>> hooks), or the HAS_PCI_MSI proposal would at least want further
> >>>> investigating as to its feasibility to address the issues at hand.
> >>> 
> >>> I would be happy with CONFIG_X86, despite the fact that this is only
> >>> deferring the problem.
> >>> 
> >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
> >>> that we are not going to use NS16550 PCI on Arm in the forseeable
> >>> future.
> >> And I continue to fail to see what would guarantee this: As soon
> >> as you can plug in such a card into an Arm system, people will
> >> want to be able use it. That's why we had to add support for it
> >> on x86, after all.
> > 
> > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI support.
> > 
> > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why would bother to lose a PCI slot for yet another UART?
> > 
> >> >> So why do we need a finer graine Kconfig?
> >> Because most of the involved code is indeed MSI-related?
> > 
> > Possibly, yet it would not be necessary if we don't want NS16550 PCI support...
> 
> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed further.
> 
> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to have compilation error 
> what we are observing now when HAS_PCI is enabled.
> 
> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation error. 
> Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver.  


It doesn't matter too much to me, let's just choose one option so that you
get unblocked soon.

It looks like Jan prefers option 2) and both Julien and I are OK with
it. So let's do 2). Jan, please confirm too :-)

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 5642 bytes --]

On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Thu, 19 Nov 2020, Rahul Singh wrote:
> > > On 19/11/2020 09:53, Jan Beulich wrote:
> > >> On 19.11.2020 10:21, Julien Grall wrote:
> > >>> Hi Jan,
> > >>>
> > >>> On 19/11/2020 09:05, Jan Beulich wrote:
> > >>>> On 18.11.2020 16:50, Julien Grall wrote:
> > >>>>> On 16/11/2020 12:25, Rahul Singh wrote:
> > >>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When
> HAS_PCI
> > >>>>>> is enabled for ARM, compilation error is observed for ARM
> architecture
> > >>>>>> because ARM platforms do not have full PCI support available.
> > >>>>>   >
> > >>>>>> 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.
> > >>>>>
> > >>>>> NIT: I would say "No functional change intended" to make clear
> this is
> > >>>>> an expectation and hopefully will be correct :).
> > >>>>>
> > >>>>> Regarding the commit message itself, I would suggest the following
> to
> > >>>>> address Jan's concern:
> > >>>>
> > >>>> While indeed this is a much better description, I continue to think
> > >>>> that the proposed Kconfig option is undesirable to have.
> > >>>
> > >>> I am yet to see an argument into why we should keep the PCI code
> > >>> compiled on Arm when there will be no-use....
> > >> Well, see my patch suppressing building of quite a part of it.
> > >
> > > I will let Rahul figuring out whether your patch series is sufficient
> to fix compilation issues (this is what matters right now).
> >
> > I just checked the compilation error for ARM after enabling the HAS_PCI
> on ARM. I am observing the same compilation error what I observed
> previously.
> > There are two new errors related to struct uart_config and struct
> part_param as those struct defined globally but used under X86 flags.
> >
> > At top level:
> > ns16550.c:179:48: error: ‘uart_config’ defined but not used
> [-Werror=unused-const-variable=]
> >  static const struct ns16550_config __initconst uart_config[] =
> >                                                 ^~~~~~~~~~~
> > ns16550.c:104:54: error: ‘uart_param’ defined but not used
> [-Werror=unused-const-variable=]
> >  static const struct ns16550_config_param __initconst uart_param[] = {
> >
> >
> > >
> > >>>> Either,
> > >>>> following the patch I've just sent, truly x86-specific things (at
> > >>>> least as far as current state goes - if any of this was to be
> > >>>> re-used by a future port, suitable further abstraction may be
> > >>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
> > >>>> hooks), or the HAS_PCI_MSI proposal would at least want further
> > >>>> investigating as to its feasibility to address the issues at hand.
> > >>>
> > >>> I would be happy with CONFIG_X86, despite the fact that this is only
> > >>> deferring the problem.
> > >>>
> > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing
> given
> > >>> that we are not going to use NS16550 PCI on Arm in the forseeable
> > >>> future.
> > >> And I continue to fail to see what would guarantee this: As soon
> > >> as you can plug in such a card into an Arm system, people will
> > >> want to be able use it. That's why we had to add support for it
> > >> on x86, after all.
> > >
> > > Well, plug-in PCI cards on Arm has been available for quite a while...
> Yet I haven't heard anyone asking for NS16550 PCI support.
> > >
> > > This is probably because SBSA compliant server should always provide
> an SBSA UART (a cut-down version of the PL011). So why would bother to lose
> a PCI slot for yet another UART?
> > >
> > >> >> So why do we need a finer graine Kconfig?
> > >> Because most of the involved code is indeed MSI-related?
> > >
> > > Possibly, yet it would not be necessary if we don't want NS16550 PCI
> support...
> >
> > To fix compilation error on ARM as per the discussion there are below
> options please suggest which one to use to proceed further.
> >
> > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This
> helps also non-x86 architecture in the future not to have compilation error
> > what we are observing now when HAS_PCI is enabled.
> >
> > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce
> the new CONFIG_HAS_PCI_MSI options to fix the MSI related compilation
> error.
> > Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and
> HAS_PCI enabled for ARM in Kconfig ) I am not sure if NS16550 PCI will work
> out of the box on ARM .In that case, we might need to come back again to
> fix NS16550 driver.
>
>
> It doesn't matter too much to me, let's just choose one option so that you
> get unblocked soon.
>
> It looks like Jan prefers option 2) and both Julien and I are OK with
> it. So let's do 2). Jan, please confirm too :-)


Please don't put words in my mouth... I think introducing HAS_PCI_MSI is
short sighted.

There are no clear benefits of it when NS16550 PCI support is not going to
be enable in the foreseeable future.

I would be ok with moving everything under CONFIG_X86. IHMO this is still
shortsighted but at least we don't introduce a config that's not going to
help Arm or other any architecture to disable completely PCI support in
NS16550.

Cheers,

[-- Attachment #2: Type: text/html, Size: 7343 bytes --]

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 6762 bytes --]

On Thu, 19 Nov 2020, Julien Grall wrote:
> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Thu, 19 Nov 2020, Rahul Singh wrote:
>       > > On 19/11/2020 09:53, Jan Beulich wrote:
>       > >> On 19.11.2020 10:21, Julien Grall wrote:
>       > >>> Hi Jan,
>       > >>>
>       > >>> On 19/11/2020 09:05, Jan Beulich wrote:
>       > >>>> On 18.11.2020 16:50, Julien Grall wrote:
>       > >>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>       > >>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>       > >>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>       > >>>>>> because ARM platforms do not have full PCI support available.
>       > >>>>>   >
>       > >>>>>> 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.
>       > >>>>>
>       > >>>>> NIT: I would say "No functional change intended" to make clear this is
>       > >>>>> an expectation and hopefully will be correct :).
>       > >>>>>
>       > >>>>> Regarding the commit message itself, I would suggest the following to
>       > >>>>> address Jan's concern:
>       > >>>>
>       > >>>> While indeed this is a much better description, I continue to think
>       > >>>> that the proposed Kconfig option is undesirable to have.
>       > >>>
>       > >>> I am yet to see an argument into why we should keep the PCI code
>       > >>> compiled on Arm when there will be no-use....
>       > >> Well, see my patch suppressing building of quite a part of it.
>       > >
>       > > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right
>       now).
>       >
>       > I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error
>       what I observed previously.
>       > There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under
>       X86 flags.
>       >
>       > At top level:
>       > ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
>       >  static const struct ns16550_config __initconst uart_config[] =
>       >                                                 ^~~~~~~~~~~
>       > ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
>       >  static const struct ns16550_config_param __initconst uart_param[] = {
>       >
>       >
>       > >
>       > >>>> Either,
>       > >>>> following the patch I've just sent, truly x86-specific things (at
>       > >>>> least as far as current state goes - if any of this was to be
>       > >>>> re-used by a future port, suitable further abstraction may be
>       > >>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>       > >>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>       > >>>> investigating as to its feasibility to address the issues at hand.
>       > >>>
>       > >>> I would be happy with CONFIG_X86, despite the fact that this is only
>       > >>> deferring the problem.
>       > >>>
>       > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>       > >>> that we are not going to use NS16550 PCI on Arm in the forseeable
>       > >>> future.
>       > >> And I continue to fail to see what would guarantee this: As soon
>       > >> as you can plug in such a card into an Arm system, people will
>       > >> want to be able use it. That's why we had to add support for it
>       > >> on x86, after all.
>       > >
>       > > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI
>       support.
>       > >
>       > > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why
>       would bother to lose a PCI slot for yet another UART?
>       > >
>       > >> >> So why do we need a finer graine Kconfig?
>       > >> Because most of the involved code is indeed MSI-related?
>       > >
>       > > Possibly, yet it would not be necessary if we don't want NS16550 PCI support...
>       >
>       > To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed
>       further.
>       >
>       > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to
>       have compilation error
>       > what we are observing now when HAS_PCI is enabled.
>       >
>       > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI
>       related compilation error.
>       > Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if
>       NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. 
> 
> 
>       It doesn't matter too much to me, let's just choose one option so that you
>       get unblocked soon.
> 
>       It looks like Jan prefers option 2) and both Julien and I are OK with
>       it. So let's do 2). Jan, please confirm too :-)
> 
> 
> Please don't put words in my mouth... 

Sorry Julien, I misinterpreted one of your previous comments. Sometimes
it is difficult to do things by email. It is good that you clarified as
my goal was to reach an agreement.


> I think introducing HAS_PCI_MSI is short sighted.
> 
> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future.

I agree


> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not
> going to help Arm or other any architecture to disable completely PCI support in NS16550.

So you are suggesting a new option:

3. Guard the remaining x86 specific code *and* the MSI related
compilation errors with CONFIG_X86

Is that right?


My preference is actually option 1) but this series is already at v3 and
I don't think this decision is as important as much as unblocking
Rahul, so I am OK with the other alternatives too.

I tend to agree with you that 3) is better than 2) for the reasons you
wrote above.

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Rahul Singh]

Hello Jan,

> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 19 Nov 2020, Julien Grall wrote:
>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>>      On Thu, 19 Nov 2020, Rahul Singh wrote:
>>>> On 19/11/2020 09:53, Jan Beulich wrote:
>>>>> On 19.11.2020 10:21, Julien Grall wrote:
>>>>>> Hi Jan,
>>>>>> 
>>>>>> On 19/11/2020 09:05, Jan Beulich wrote:
>>>>>>> On 18.11.2020 16:50, Julien Grall wrote:
>>>>>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>>>>>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>>>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>>>>>>> because ARM platforms do not have full PCI support available.
>>>>>>>>    >
>>>>>>>>> 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.
>>>>>>>> 
>>>>>>>> NIT: I would say "No functional change intended" to make clear this is
>>>>>>>> an expectation and hopefully will be correct :).
>>>>>>>> 
>>>>>>>> Regarding the commit message itself, I would suggest the following to
>>>>>>>> address Jan's concern:
>>>>>>> 
>>>>>>> While indeed this is a much better description, I continue to think
>>>>>>> that the proposed Kconfig option is undesirable to have.
>>>>>> 
>>>>>> I am yet to see an argument into why we should keep the PCI code
>>>>>> compiled on Arm when there will be no-use....
>>>>> Well, see my patch suppressing building of quite a part of it.
>>>> 
>>>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right
>>      now).
>>> 
>>> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error
>>      what I observed previously.
>>> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under
>>      X86 flags.
>>> 
>>> At top level:
>>> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
>>>   static const struct ns16550_config __initconst uart_config[] =
>>>                                                  ^~~~~~~~~~~
>>> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
>>>   static const struct ns16550_config_param __initconst uart_param[] = {
>>> 
>>> 
>>>> 
>>>>>>> Either,
>>>>>>> following the patch I've just sent, truly x86-specific things (at
>>>>>>> least as far as current state goes - if any of this was to be
>>>>>>> re-used by a future port, suitable further abstraction may be
>>>>>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>>>>>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>>>>>>> investigating as to its feasibility to address the issues at hand.
>>>>>> 
>>>>>> I would be happy with CONFIG_X86, despite the fact that this is only
>>>>>> deferring the problem.
>>>>>> 
>>>>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>>>>>> that we are not going to use NS16550 PCI on Arm in the forseeable
>>>>>> future.
>>>>> And I continue to fail to see what would guarantee this: As soon
>>>>> as you can plug in such a card into an Arm system, people will
>>>>> want to be able use it. That's why we had to add support for it
>>>>> on x86, after all.
>>>> 
>>>> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI
>>      support.
>>>> 
>>>> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why
>>      would bother to lose a PCI slot for yet another UART?
>>>> 
>>>>>>> So why do we need a finer graine Kconfig?
>>>>> Because most of the involved code is indeed MSI-related?
>>>> 
>>>> Possibly, yet it would not be necessary if we don't want NS16550 PCI support...
>>> 
>>> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed
>>      further.
>>> 
>>> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to
>>      have compilation error
>>> what we are observing now when HAS_PCI is enabled.
>>> 
>>> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI
>>      related compilation error.
>>> Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if
>>      NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. 
>> 
>> 
>>      It doesn't matter too much to me, let's just choose one option so that you
>>      get unblocked soon.
>> 
>>      It looks like Jan prefers option 2) and both Julien and I are OK with
>>      it. So let's do 2). Jan, please confirm too :-)
>> 
>> 
>> Please don't put words in my mouth... 
> 
> Sorry Julien, I misinterpreted one of your previous comments. Sometimes
> it is difficult to do things by email. It is good that you clarified as
> my goal was to reach an agreement.
> 
> 
>> I think introducing HAS_PCI_MSI is short sighted.
>> 
>> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future.
> 
> I agree
> 
> 
>> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not
>> going to help Arm or other any architecture to disable completely PCI support in NS16550.
> 
> So you are suggesting a new option:
> 
> 3. Guard the remaining x86 specific code *and* the MSI related
> compilation errors with CONFIG_X86
> 
> Is that right?
> 
> 
> My preference is actually option 1) but this series is already at v3 and
> I don't think this decision is as important as much as unblocking
> Rahul, so I am OK with the other alternatives too.
> 
> I tend to agree with you that 3) is better than 2) for the reasons you
> wrote above.


Can you please provide your suggestion how to proceed on this so that I can send my next patch.
I am waiting for your reply if you are also ok for the options 3.

Thanks in advance.

Regards,
Rahul

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

Rahul,

On 23.11.2020 12:54, Rahul Singh wrote:
> Hello Jan,

as an aside - it helps if you also put the addressee of your mail
on the To list.

>> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Thu, 19 Nov 2020, Julien Grall wrote:
>>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>>>      On Thu, 19 Nov 2020, Rahul Singh wrote:
>>>>> On 19/11/2020 09:53, Jan Beulich wrote:
>>>>>> On 19.11.2020 10:21, Julien Grall wrote:
>>>>>>> Hi Jan,
>>>>>>>
>>>>>>> On 19/11/2020 09:05, Jan Beulich wrote:
>>>>>>>> On 18.11.2020 16:50, Julien Grall wrote:
>>>>>>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>>>>>>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>>>>>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>>>>>>>>> because ARM platforms do not have full PCI support available.
>>>>>>>>>    >
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> NIT: I would say "No functional change intended" to make clear this is
>>>>>>>>> an expectation and hopefully will be correct :).
>>>>>>>>>
>>>>>>>>> Regarding the commit message itself, I would suggest the following to
>>>>>>>>> address Jan's concern:
>>>>>>>>
>>>>>>>> While indeed this is a much better description, I continue to think
>>>>>>>> that the proposed Kconfig option is undesirable to have.
>>>>>>>
>>>>>>> I am yet to see an argument into why we should keep the PCI code
>>>>>>> compiled on Arm when there will be no-use....
>>>>>> Well, see my patch suppressing building of quite a part of it.
>>>>>
>>>>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right
>>>      now).
>>>>
>>>> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error
>>>      what I observed previously.
>>>> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under
>>>      X86 flags.
>>>>
>>>> At top level:
>>>> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
>>>>   static const struct ns16550_config __initconst uart_config[] =
>>>>                                                  ^~~~~~~~~~~
>>>> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
>>>>   static const struct ns16550_config_param __initconst uart_param[] = {
>>>>
>>>>
>>>>>
>>>>>>>> Either,
>>>>>>>> following the patch I've just sent, truly x86-specific things (at
>>>>>>>> least as far as current state goes - if any of this was to be
>>>>>>>> re-used by a future port, suitable further abstraction may be
>>>>>>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>>>>>>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>>>>>>>> investigating as to its feasibility to address the issues at hand.
>>>>>>>
>>>>>>> I would be happy with CONFIG_X86, despite the fact that this is only
>>>>>>> deferring the problem.
>>>>>>>
>>>>>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>>>>>>> that we are not going to use NS16550 PCI on Arm in the forseeable
>>>>>>> future.
>>>>>> And I continue to fail to see what would guarantee this: As soon
>>>>>> as you can plug in such a card into an Arm system, people will
>>>>>> want to be able use it. That's why we had to add support for it
>>>>>> on x86, after all.
>>>>>
>>>>> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI
>>>      support.
>>>>>
>>>>> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why
>>>      would bother to lose a PCI slot for yet another UART?
>>>>>
>>>>>>>> So why do we need a finer graine Kconfig?
>>>>>> Because most of the involved code is indeed MSI-related?
>>>>>
>>>>> Possibly, yet it would not be necessary if we don't want NS16550 PCI support...
>>>>
>>>> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed
>>>      further.
>>>>
>>>> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to
>>>      have compilation error
>>>> what we are observing now when HAS_PCI is enabled.
>>>>
>>>> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI
>>>      related compilation error.
>>>> Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if
>>>      NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. 
>>>
>>>
>>>      It doesn't matter too much to me, let's just choose one option so that you
>>>      get unblocked soon.
>>>
>>>      It looks like Jan prefers option 2) and both Julien and I are OK with
>>>      it. So let's do 2). Jan, please confirm too :-)
>>>
>>>
>>> Please don't put words in my mouth... 
>>
>> Sorry Julien, I misinterpreted one of your previous comments. Sometimes
>> it is difficult to do things by email. It is good that you clarified as
>> my goal was to reach an agreement.
>>
>>
>>> I think introducing HAS_PCI_MSI is short sighted.
>>>
>>> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future.
>>
>> I agree
>>
>>
>>> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not
>>> going to help Arm or other any architecture to disable completely PCI support in NS16550.
>>
>> So you are suggesting a new option:
>>
>> 3. Guard the remaining x86 specific code *and* the MSI related
>> compilation errors with CONFIG_X86
>>
>> Is that right?
>>
>>
>> My preference is actually option 1) but this series is already at v3 and
>> I don't think this decision is as important as much as unblocking
>> Rahul, so I am OK with the other alternatives too.
>>
>> I tend to agree with you that 3) is better than 2) for the reasons you
>> wrote above.
> 
> 
> Can you please provide your suggestion how to proceed on this so that I can send my next patch.
> I am waiting for your reply if you are also ok for the options 3.

I can live with 3, I guess, but I still think a separate PCI_MSI
control would be better. Please realize though that things also
depend on how the change is going to look like in the end, i.e.
I'm not going to assure you this is my final view on it. In any
event I've just sent v2 of my series, which I consider a prereq
of yours.

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

Hi Stefano,

On 20/11/2020 00:14, Stefano Stabellini wrote:
> On Thu, 19 Nov 2020, Julien Grall wrote:
>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>>        On Thu, 19 Nov 2020, Rahul Singh wrote:
>>        > > On 19/11/2020 09:53, Jan Beulich wrote:
>>        > >> On 19.11.2020 10:21, Julien Grall wrote:
>>        > >>> Hi Jan,
>>        > >>>
>>        > >>> On 19/11/2020 09:05, Jan Beulich wrote:
>>        > >>>> On 18.11.2020 16:50, Julien Grall wrote:
>>        > >>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>>        > >>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
>>        > >>>>>> is enabled for ARM, compilation error is observed for ARM architecture
>>        > >>>>>> because ARM platforms do not have full PCI support available.
>>        > >>>>>   >
>>        > >>>>>> 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.
>>        > >>>>>
>>        > >>>>> NIT: I would say "No functional change intended" to make clear this is
>>        > >>>>> an expectation and hopefully will be correct :).
>>        > >>>>>
>>        > >>>>> Regarding the commit message itself, I would suggest the following to
>>        > >>>>> address Jan's concern:
>>        > >>>>
>>        > >>>> While indeed this is a much better description, I continue to think
>>        > >>>> that the proposed Kconfig option is undesirable to have.
>>        > >>>
>>        > >>> I am yet to see an argument into why we should keep the PCI code
>>        > >>> compiled on Arm when there will be no-use....
>>        > >> Well, see my patch suppressing building of quite a part of it.
>>        > >
>>        > > I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right
>>        now).
>>        >
>>        > I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error
>>        what I observed previously.
>>        > There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under
>>        X86 flags.
>>        >
>>        > At top level:
>>        > ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
>>        >  static const struct ns16550_config __initconst uart_config[] =
>>        >                                                 ^~~~~~~~~~~
>>        > ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
>>        >  static const struct ns16550_config_param __initconst uart_param[] = {
>>        >
>>        >
>>        > >
>>        > >>>> Either,
>>        > >>>> following the patch I've just sent, truly x86-specific things (at
>>        > >>>> least as far as current state goes - if any of this was to be
>>        > >>>> re-used by a future port, suitable further abstraction may be
>>        > >>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>>        > >>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>>        > >>>> investigating as to its feasibility to address the issues at hand.
>>        > >>>
>>        > >>> I would be happy with CONFIG_X86, despite the fact that this is only
>>        > >>> deferring the problem.
>>        > >>>
>>        > >>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
>>        > >>> that we are not going to use NS16550 PCI on Arm in the forseeable
>>        > >>> future.
>>        > >> And I continue to fail to see what would guarantee this: As soon
>>        > >> as you can plug in such a card into an Arm system, people will
>>        > >> want to be able use it. That's why we had to add support for it
>>        > >> on x86, after all.
>>        > >
>>        > > Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI
>>        support.
>>        > >
>>        > > This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why
>>        would bother to lose a PCI slot for yet another UART?
>>        > >
>>        > >> >> So why do we need a finer graine Kconfig?
>>        > >> Because most of the involved code is indeed MSI-related?
>>        > >
>>        > > Possibly, yet it would not be necessary if we don't want NS16550 PCI support...
>>        >
>>        > To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed
>>        further.
>>        >
>>        > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to
>>        have compilation error
>>        > what we are observing now when HAS_PCI is enabled.
>>        >
>>        > 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI
>>        related compilation error.
>>        > Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if
>>        NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver.
>>
>>
>>        It doesn't matter too much to me, let's just choose one option so that you
>>        get unblocked soon.
>>
>>        It looks like Jan prefers option 2) and both Julien and I are OK with
>>        it. So let's do 2). Jan, please confirm too :-)
>>
>>
>> Please don't put words in my mouth...
> 
> Sorry Julien, I misinterpreted one of your previous comments. Sometimes
> it is difficult to do things by email. It is good that you clarified as
> my goal was to reach an agreement.

No worries. I would like to apologies for been harsher than I would have 
wanted on my reply. The thread had a lot of back and forth so far.

> 
> 
>> I think introducing HAS_PCI_MSI is short sighted.
>>
>> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future.
> 
> I agree
> 
> 
>> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not
>> going to help Arm or other any architecture to disable completely PCI support in NS16550.
> 
> So you are suggesting a new option:
> 
> 3. Guard the remaining x86 specific code *and* the MSI related
> compilation errors with CONFIG_X86
> 
> Is that right?

That's correct.

> My preference is actually option 1) but this series is already at v3 and
> I don't think this decision is as important as much as unblocking
> Rahul, so I am OK with the other alternatives too.

In order, my preferences are 1) 3) 2). AFAICT...

> 
> I tend to agree with you that 3) is better than 2) for the reasons you
> wrote above.

... this is the same order as you. Although I probably have a stronger 
dislike for 2) because I feel it is pushed for the wrong reasons (e.g. 
matter of taste) so far.

My view on 2) can change if Jan provides enough information into why one 
would want NS1650 PCI enabled by default on Arm but disable MSI.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7562 bytes --]

On Mon, 23 Nov 2020, Jan Beulich wrote:
> Rahul,
> 
> On 23.11.2020 12:54, Rahul Singh wrote:
> > Hello Jan,
> 
> as an aside - it helps if you also put the addressee of your mail
> on the To list.
> 
> >> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >>
> >> On Thu, 19 Nov 2020, Julien Grall wrote:
> >>> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@kernel.org> wrote:
> >>>      On Thu, 19 Nov 2020, Rahul Singh wrote:
> >>>>> On 19/11/2020 09:53, Jan Beulich wrote:
> >>>>>> On 19.11.2020 10:21, Julien Grall wrote:
> >>>>>>> Hi Jan,
> >>>>>>>
> >>>>>>> On 19/11/2020 09:05, Jan Beulich wrote:
> >>>>>>>> On 18.11.2020 16:50, Julien Grall wrote:
> >>>>>>>>> On 16/11/2020 12:25, Rahul Singh wrote:
> >>>>>>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. When HAS_PCI
> >>>>>>>>>> is enabled for ARM, compilation error is observed for ARM architecture
> >>>>>>>>>> because ARM platforms do not have full PCI support available.
> >>>>>>>>>    >
> >>>>>>>>>> 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.
> >>>>>>>>>
> >>>>>>>>> NIT: I would say "No functional change intended" to make clear this is
> >>>>>>>>> an expectation and hopefully will be correct :).
> >>>>>>>>>
> >>>>>>>>> Regarding the commit message itself, I would suggest the following to
> >>>>>>>>> address Jan's concern:
> >>>>>>>>
> >>>>>>>> While indeed this is a much better description, I continue to think
> >>>>>>>> that the proposed Kconfig option is undesirable to have.
> >>>>>>>
> >>>>>>> I am yet to see an argument into why we should keep the PCI code
> >>>>>>> compiled on Arm when there will be no-use....
> >>>>>> Well, see my patch suppressing building of quite a part of it.
> >>>>>
> >>>>> I will let Rahul figuring out whether your patch series is sufficient to fix compilation issues (this is what matters right
> >>>      now).
> >>>>
> >>>> I just checked the compilation error for ARM after enabling the HAS_PCI on ARM. I am observing the same compilation error
> >>>      what I observed previously.
> >>>> There are two new errors related to struct uart_config and struct part_param as those struct defined globally but used under
> >>>      X86 flags.
> >>>>
> >>>> At top level:
> >>>> ns16550.c:179:48: error: ‘uart_config’ defined but not used [-Werror=unused-const-variable=]
> >>>>   static const struct ns16550_config __initconst uart_config[] =
> >>>>                                                  ^~~~~~~~~~~
> >>>> ns16550.c:104:54: error: ‘uart_param’ defined but not used [-Werror=unused-const-variable=]
> >>>>   static const struct ns16550_config_param __initconst uart_param[] = {
> >>>>
> >>>>
> >>>>>
> >>>>>>>> Either,
> >>>>>>>> following the patch I've just sent, truly x86-specific things (at
> >>>>>>>> least as far as current state goes - if any of this was to be
> >>>>>>>> re-used by a future port, suitable further abstraction may be
> >>>>>>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
> >>>>>>>> hooks), or the HAS_PCI_MSI proposal would at least want further
> >>>>>>>> investigating as to its feasibility to address the issues at hand.
> >>>>>>>
> >>>>>>> I would be happy with CONFIG_X86, despite the fact that this is only
> >>>>>>> deferring the problem.
> >>>>>>>
> >>>>>>> Regarding HAS_PCI_MSI, I don't really see the point of introducing given
> >>>>>>> that we are not going to use NS16550 PCI on Arm in the forseeable
> >>>>>>> future.
> >>>>>> And I continue to fail to see what would guarantee this: As soon
> >>>>>> as you can plug in such a card into an Arm system, people will
> >>>>>> want to be able use it. That's why we had to add support for it
> >>>>>> on x86, after all.
> >>>>>
> >>>>> Well, plug-in PCI cards on Arm has been available for quite a while... Yet I haven't heard anyone asking for NS16550 PCI
> >>>      support.
> >>>>>
> >>>>> This is probably because SBSA compliant server should always provide an SBSA UART (a cut-down version of the PL011). So why
> >>>      would bother to lose a PCI slot for yet another UART?
> >>>>>
> >>>>>>>> So why do we need a finer graine Kconfig?
> >>>>>> Because most of the involved code is indeed MSI-related?
> >>>>>
> >>>>> Possibly, yet it would not be necessary if we don't want NS16550 PCI support...
> >>>>
> >>>> To fix compilation error on ARM as per the discussion there are below options please suggest which one to use to proceed
> >>>      further.
> >>>>
> >>>> 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. This helps also non-x86 architecture in the future not to
> >>>      have compilation error
> >>>> what we are observing now when HAS_PCI is enabled.
> >>>>
> >>>> 2. Guard the remaining x86 specific code with CONFIG_X86 and introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI
> >>>      related compilation error.
> >>>> Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and HAS_PCI enabled for ARM in Kconfig ) I am not sure if
> >>>      NS16550 PCI will work out of the box on ARM .In that case, we might need to come back again to fix NS16550 driver. 
> >>>
> >>>
> >>>      It doesn't matter too much to me, let's just choose one option so that you
> >>>      get unblocked soon.
> >>>
> >>>      It looks like Jan prefers option 2) and both Julien and I are OK with
> >>>      it. So let's do 2). Jan, please confirm too :-)
> >>>
> >>>
> >>> Please don't put words in my mouth... 
> >>
> >> Sorry Julien, I misinterpreted one of your previous comments. Sometimes
> >> it is difficult to do things by email. It is good that you clarified as
> >> my goal was to reach an agreement.
> >>
> >>
> >>> I think introducing HAS_PCI_MSI is short sighted.
> >>>
> >>> There are no clear benefits of it when NS16550 PCI support is not going to be enable in the foreseeable future.
> >>
> >> I agree
> >>
> >>
> >>> I would be ok with moving everything under CONFIG_X86. IHMO this is still shortsighted but at least we don't introduce a config that's not
> >>> going to help Arm or other any architecture to disable completely PCI support in NS16550.
> >>
> >> So you are suggesting a new option:
> >>
> >> 3. Guard the remaining x86 specific code *and* the MSI related
> >> compilation errors with CONFIG_X86
> >>
> >> Is that right?
> >>
> >>
> >> My preference is actually option 1) but this series is already at v3 and
> >> I don't think this decision is as important as much as unblocking
> >> Rahul, so I am OK with the other alternatives too.
> >>
> >> I tend to agree with you that 3) is better than 2) for the reasons you
> >> wrote above.
> > 
> > 
> > Can you please provide your suggestion how to proceed on this so that I can send my next patch.
> > I am waiting for your reply if you are also ok for the options 3.
> 
> I can live with 3, I guess, but I still think a separate PCI_MSI
> control would be better. Please realize though that things also
> depend on how the change is going to look like in the end, i.e.
> I'm not going to assure you this is my final view on it. In any
> event I've just sent v2 of my series, which I consider a prereq
> of yours.

It is great that we have a way forward.

I'll try to have a look at your series -- it looks pretty
straightforward.

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 23.11.2020 18:13, Julien Grall wrote:
> My view on 2) can change if Jan provides enough information into why one 
> would want NS1650 PCI enabled by default on Arm but disable MSI.

Because, like it was on x86, initially there may be no support for
MSI? I have no idea what the plans are ...

Jan

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Julien Grall]

Hi Jan,

On 24/11/2020 09:47, Jan Beulich wrote:
> On 23.11.2020 18:13, Julien Grall wrote:
>> My view on 2) can change if Jan provides enough information into why one
>> would want NS1650 PCI enabled by default on Arm but disable MSI.
> 
> Because, like it was on x86, initially there may be no support for
> MSI?

"no support for MSI" implies that there will be at least support for 
NS16550 PCI.

>  I have no idea what the plans are ...

There are no such plan on Arm for the forseeable future (read as we 
haven't seen any interested from the Arm community).

The NS16550 PCI code will stay unusable until someone effectively send a 
patch to plumb it correctly.

While I agree that disabling MSI may be nice to have to do in the 
future, this doesn't address the need for Arm. I don't want to get in 
our way the NS16550 PCI code in our way when implementing PCI (with or 
without MSI) on Arm.

Even if there were an interest, I would still expect some users (e.g. 
embedded folks) to want to compile-out unused feature (you may have a 
platform with a embedded NS16550).

So the path forward will stay either 1) or 3) for me.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Jan Beulich]

On 24.11.2020 11:22, Julien Grall wrote:
> On 24/11/2020 09:47, Jan Beulich wrote:
>> On 23.11.2020 18:13, Julien Grall wrote:
>>> My view on 2) can change if Jan provides enough information into why one
>>> would want NS1650 PCI enabled by default on Arm but disable MSI.
>>
>> Because, like it was on x86, initially there may be no support for
>> MSI?
> 
> "no support for MSI" implies that there will be at least support for 
> NS16550 PCI.
> 
>>  I have no idea what the plans are ...
> 
> There are no such plan on Arm for the forseeable future (read as we 
> haven't seen any interested from the Arm community).

Okay, so you're question wasn't so much about the "but" in there,
but the "PCI" in the first place.

> The NS16550 PCI code will stay unusable until someone effectively send a 
> patch to plumb it correctly.
> 
> While I agree that disabling MSI may be nice to have to do in the 
> future, this doesn't address the need for Arm. I don't want to get in 
> our way the NS16550 PCI code in our way when implementing PCI (with or 
> without MSI) on Arm.
> 
> Even if there were an interest, I would still expect some users (e.g. 
> embedded folks) to want to compile-out unused feature (you may have a 
> platform with a embedded NS16550).
> 
> So the path forward will stay either 1) or 3) for me.

Well, as said elsewhere - 3) it is then afaic, for making more
obvious that this is really a hack.

Jan