* [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory.
2020-11-25 18:16 [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
@ 2020-11-25 18:16 ` Rahul Singh
2020-11-25 21:16 ` Stefano Stabellini
` (2 more replies)
2020-11-25 18:16 ` [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled Rahul Singh
` (2 subsequent siblings)
3 siblings, 3 replies; 19+ messages in thread
From: Rahul Singh @ 2020-11-25 18:16 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Jan Beulich, Paul Durrant,
Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu
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 intended.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v4:
- fixed compilation error when CONFIG_HVM is disabled
- remove iommu_update_ire_from_msi from the patch will send another patch
to fix.
---
xen/drivers/passthrough/Makefile | 3 -
xen/drivers/passthrough/pci.c | 71 +--------------------
xen/drivers/passthrough/x86/Makefile | 1 +
xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 +++++++++++++++++++
xen/include/xen/pci.h | 9 +++
5 files changed, 77 insertions(+), 73 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..3c6ab1bcb6 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();
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/include/xen/pci.h b/xen/include/xen/pci.h
index 20a54a5bb4..8e3d4d9454 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -208,4 +208,13 @@ 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);
+#ifdef CONFIG_HVM
+int arch_pci_clean_pirqs(struct domain *d);
+#else
+static inline int arch_pci_clean_pirqs(struct domain *d)
+{
+ return 0;
+}
+#endif /* CONFIG_HVM */
+
#endif /* __XEN_PCI_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory.
2020-11-25 18:16 ` [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory Rahul Singh
@ 2020-11-25 21:16 ` Stefano Stabellini
2020-11-26 9:04 ` Bertrand Marquis
2020-11-27 13:34 ` Jan Beulich
2 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2020-11-25 21:16 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant,
Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu
On Wed, 25 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 intended.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> Changes in v4:
> - fixed compilation error when CONFIG_HVM is disabled
> - remove iommu_update_ire_from_msi from the patch will send another patch
> to fix.
>
> ---
> xen/drivers/passthrough/Makefile | 3 -
> xen/drivers/passthrough/pci.c | 71 +--------------------
> xen/drivers/passthrough/x86/Makefile | 1 +
> xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 +++++++++++++++++++
> xen/include/xen/pci.h | 9 +++
> 5 files changed, 77 insertions(+), 73 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..3c6ab1bcb6 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();
> 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/include/xen/pci.h b/xen/include/xen/pci.h
> index 20a54a5bb4..8e3d4d9454 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -208,4 +208,13 @@ 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);
>
> +#ifdef CONFIG_HVM
> +int arch_pci_clean_pirqs(struct domain *d);
> +#else
> +static inline int arch_pci_clean_pirqs(struct domain *d)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_HVM */
> +
> #endif /* __XEN_PCI_H__ */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory.
2020-11-25 18:16 ` [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory Rahul Singh
2020-11-25 21:16 ` Stefano Stabellini
@ 2020-11-26 9:04 ` Bertrand Marquis
2020-11-27 13:34 ` Jan Beulich
2 siblings, 0 replies; 19+ messages in thread
From: Bertrand Marquis @ 2020-11-26 9:04 UTC (permalink / raw)
To: Rahul Singh
Cc: Xen-devel, Jan Beulich, Paul Durrant, Andrew Cooper,
George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini,
Wei Liu
> On 25 Nov 2020, at 18:16, Rahul Singh <Rahul.Singh@arm.com> 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 intended.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
>
> Changes in v4:
> - fixed compilation error when CONFIG_HVM is disabled
> - remove iommu_update_ire_from_msi from the patch will send another patch
> to fix.
>
> ---
> xen/drivers/passthrough/Makefile | 3 -
> xen/drivers/passthrough/pci.c | 71 +--------------------
> xen/drivers/passthrough/x86/Makefile | 1 +
> xen/drivers/passthrough/{io.c => x86/hvm.c} | 66 +++++++++++++++++++
> xen/include/xen/pci.h | 9 +++
> 5 files changed, 77 insertions(+), 73 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..3c6ab1bcb6 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();
> 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/include/xen/pci.h b/xen/include/xen/pci.h
> index 20a54a5bb4..8e3d4d9454 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -208,4 +208,13 @@ 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);
>
> +#ifdef CONFIG_HVM
> +int arch_pci_clean_pirqs(struct domain *d);
> +#else
> +static inline int arch_pci_clean_pirqs(struct domain *d)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_HVM */
> +
> #endif /* __XEN_PCI_H__ */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory.
2020-11-25 18:16 ` [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory Rahul Singh
2020-11-25 21:16 ` Stefano Stabellini
2020-11-26 9:04 ` Bertrand Marquis
@ 2020-11-27 13:34 ` Jan Beulich
2 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2020-11-27 13:34 UTC (permalink / raw)
To: Rahul Singh
Cc: bertrand.marquis, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel
On 25.11.2020 19:16, Rahul Singh wrote:
> --- 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>
At least the latter two of the lines you remove are clearly still
needed here, and in such a case are better to specify explicitly
than to depend on other headers including them. Since xen/sched.h
very likely also gets included indirectly anyway, I'm inclined to
suggest to drop this entire hunk (which ought to be doable while
committing). With this
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled.
2020-11-25 18:16 [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
2020-11-25 18:16 ` [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory Rahul Singh
@ 2020-11-25 18:16 ` Rahul Singh
2020-11-25 21:22 ` Stefano Stabellini
2020-11-26 9:05 ` Bertrand Marquis
2020-11-25 18:16 ` [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86 Rahul Singh
2020-11-27 20:04 ` [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Andrew Cooper
3 siblings, 2 replies; 19+ messages in thread
From: Rahul Singh @ 2020-11-25 18:16 UTC (permalink / raw)
To: xen-devel; +Cc: bertrand.marquis, rahul.singh, Jan Beulich, Paul Durrant
If mem-sharing, mem-paging, or log-dirty functionality is not enabled
for architecture when HAS_PCI is enabled, the compiler will throw an
error.
Move code to x86 specific file 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 intended.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v4:
- fixed minor comments
---
xen/drivers/passthrough/pci.c | 8 +-------
xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++
xen/include/xen/iommu.h | 2 ++
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 3c6ab1bcb6..4c21655b7d 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>
@@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
if ( !is_iommu_enabled(d) )
return 0;
- /* Prevent device assign if mem paging or mem sharing have been
- * enabled for this domain */
- if ( d != dom_io &&
- unlikely(mem_sharing_enabled(d) ||
- vm_event_check_ring(d->vm_event_paging) ||
- p2m_get_hostp2m(d)->global_logdirty) )
+ if( !arch_iommu_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 f17b1820f4..cea1032b3d 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -18,6 +18,7 @@
#include <xen/guest_access.h>
#include <xen/event.h>
#include <xen/softirq.h>
+#include <xen/vm_event.h>
#include <xsm/xsm.h>
#include <asm/hvm/io.h>
@@ -308,6 +309,18 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
return pg;
}
+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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled.
2020-11-25 18:16 ` [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled Rahul Singh
@ 2020-11-25 21:22 ` Stefano Stabellini
2020-11-26 9:05 ` Bertrand Marquis
1 sibling, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2020-11-25 21:22 UTC (permalink / raw)
To: Rahul Singh; +Cc: xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant
On Wed, 25 Nov 2020, Rahul Singh wrote:
> If mem-sharing, mem-paging, or log-dirty functionality is not enabled
> for architecture when HAS_PCI is enabled, the compiler will throw an
> error.
>
> Move code to x86 specific file 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 intended.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>
> Changes in v4:
> - fixed minor comments
>
> ---
> xen/drivers/passthrough/pci.c | 8 +-------
> xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++
> xen/include/xen/iommu.h | 2 ++
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 3c6ab1bcb6..4c21655b7d 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>
> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
> if ( !is_iommu_enabled(d) )
> return 0;
>
> - /* Prevent device assign if mem paging or mem sharing have been
> - * enabled for this domain */
> - if ( d != dom_io &&
> - unlikely(mem_sharing_enabled(d) ||
> - vm_event_check_ring(d->vm_event_paging) ||
> - p2m_get_hostp2m(d)->global_logdirty) )
> + if( !arch_iommu_use_permitted(d) )
code style:
if ( !arch_iommu_use_permitted(d) )
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled.
2020-11-25 18:16 ` [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled Rahul Singh
2020-11-25 21:22 ` Stefano Stabellini
@ 2020-11-26 9:05 ` Bertrand Marquis
2020-11-27 13:47 ` Jan Beulich
1 sibling, 1 reply; 19+ messages in thread
From: Bertrand Marquis @ 2020-11-26 9:05 UTC (permalink / raw)
To: Rahul Singh; +Cc: xen-devel, Jan Beulich, Paul Durrant
> On 25 Nov 2020, at 18:16, Rahul Singh <Rahul.Singh@arm.com> wrote:
>
> If mem-sharing, mem-paging, or log-dirty functionality is not enabled
> for architecture when HAS_PCI is enabled, the compiler will throw an
> error.
>
> Move code to x86 specific file 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 intended.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
I guess the small typo fix could be fixed by the commiter directly :-)
Cheers
Bertrand
> ---
>
> Changes in v4:
> - fixed minor comments
>
> ---
> xen/drivers/passthrough/pci.c | 8 +-------
> xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++++
> xen/include/xen/iommu.h | 2 ++
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 3c6ab1bcb6..4c21655b7d 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>
> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
> if ( !is_iommu_enabled(d) )
> return 0;
>
> - /* Prevent device assign if mem paging or mem sharing have been
> - * enabled for this domain */
> - if ( d != dom_io &&
> - unlikely(mem_sharing_enabled(d) ||
> - vm_event_check_ring(d->vm_event_paging) ||
> - p2m_get_hostp2m(d)->global_logdirty) )
> + if( !arch_iommu_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 f17b1820f4..cea1032b3d 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -18,6 +18,7 @@
> #include <xen/guest_access.h>
> #include <xen/event.h>
> #include <xen/softirq.h>
> +#include <xen/vm_event.h>
> #include <xsm/xsm.h>
>
> #include <asm/hvm/io.h>
> @@ -308,6 +309,18 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
> return pg;
> }
>
> +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
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled.
2020-11-26 9:05 ` Bertrand Marquis
@ 2020-11-27 13:47 ` Jan Beulich
0 siblings, 0 replies; 19+ messages in thread
From: Jan Beulich @ 2020-11-27 13:47 UTC (permalink / raw)
To: Bertrand Marquis, Rahul Singh; +Cc: xen-devel, Paul Durrant
On 26.11.2020 10:05, Bertrand Marquis wrote:
>
>
>> On 25 Nov 2020, at 18:16, Rahul Singh <Rahul.Singh@arm.com> wrote:
>>
>> If mem-sharing, mem-paging, or log-dirty functionality is not enabled
>> for architecture when HAS_PCI is enabled, the compiler will throw an
>> error.
>>
>> Move code to x86 specific file 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 intended.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> I guess the small typo fix could be fixed by the commiter directly :-)
Indeed, and with it
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-25 18:16 [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
2020-11-25 18:16 ` [PATCH v4 1/3] xen/pci: Move x86 specific code to x86 directory Rahul Singh
2020-11-25 18:16 ` [PATCH v4 2/3] xen/pci: solve compilation error on ARM with HAS_PCI enabled Rahul Singh
@ 2020-11-25 18:16 ` Rahul Singh
2020-11-25 18:21 ` Rahul Singh
` (2 more replies)
2020-11-27 20:04 ` [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Andrew Cooper
3 siblings, 3 replies; 19+ messages in thread
From: Rahul Singh @ 2020-11-25 18:16 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, rahul.singh, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
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 = {
^~~~~~~~
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.
Guard all remaining PCI code that is not under x86 flag with CONFIG_X86.
No functional change intended.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Changes in v4:
- As per the discussion guard all remaining PCI code with CONFIG_X86
---
xen/drivers/char/ns16550.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9235d854fe..26e601857a 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
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
#include <xen/pci.h>
#include <xen/pci_regs.h>
#include <xen/pci_ids.h>
@@ -51,7 +51,7 @@ static struct ns16550 {
unsigned int timeout_ms;
bool_t intr_works;
bool_t dw_usr_bsy;
-#ifdef CONFIG_HAS_PCI
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_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 */
@@ -66,7 +66,7 @@ static struct ns16550 {
#endif
} ns16550_com[2] = { { 0 } };
-#ifdef CONFIG_HAS_PCI
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
struct ns16550_config {
u16 vendor_id;
u16 dev_id;
@@ -256,7 +256,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
static void pci_serial_early_init(struct ns16550 *uart)
{
-#ifdef CONFIG_HAS_PCI
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
return;
@@ -355,7 +355,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
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
struct ns16550 *uart = port->uart;
if ( uart->msi )
@@ -397,7 +397,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
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
if ( uart->bar || uart->ps_bdf_enable )
{
if ( uart->param && uart->param->mmio &&
@@ -477,7 +477,7 @@ static void ns16550_suspend(struct serial_port *port)
stop_timer(&uart->timer);
-#ifdef CONFIG_HAS_PCI
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_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);
@@ -486,7 +486,7 @@ static void ns16550_suspend(struct serial_port *port)
static void _ns16550_resume(struct serial_port *port)
{
-#ifdef CONFIG_HAS_PCI
+#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
struct ns16550 *uart = port->uart;
if ( uart->bar )
--
2.17.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-25 18:16 ` [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86 Rahul Singh
@ 2020-11-25 18:21 ` Rahul Singh
2020-11-26 9:08 ` Bertrand Marquis
2020-11-25 21:28 ` Stefano Stabellini
2020-11-27 13:58 ` Jan Beulich
2 siblings, 1 reply; 19+ messages in thread
From: Rahul Singh @ 2020-11-25 18:21 UTC (permalink / raw)
To: xen-devel
Cc: Bertrand Marquis, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Hello Julien,
> On 25 Nov 2020, at 6:16 pm, Rahul Singh <rahul.singh@arm.com> wrote:
>
> 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 = {
> ^~~~~~~~
>
> 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.
>
> Guard all remaining PCI code that is not under x86 flag with CONFIG_X86.
>
> No functional change intended.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Sorry I missed to add the signed off for the commit msg. I will send the next version once reviewed done.
Signed-off-by: Julien Grall <jgrall@amazon.com>
Regards,
Rahul
> ---
>
> Changes in v4:
> - As per the discussion guard all remaining PCI code with CONFIG_X86
>
> ---
> xen/drivers/char/ns16550.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 9235d854fe..26e601857a 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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> #include <xen/pci.h>
> #include <xen/pci_regs.h>
> #include <xen/pci_ids.h>
> @@ -51,7 +51,7 @@ static struct ns16550 {
> unsigned int timeout_ms;
> bool_t intr_works;
> bool_t dw_usr_bsy;
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_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 */
> @@ -66,7 +66,7 @@ static struct ns16550 {
> #endif
> } ns16550_com[2] = { { 0 } };
>
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> struct ns16550_config {
> u16 vendor_id;
> u16 dev_id;
> @@ -256,7 +256,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>
> static void pci_serial_early_init(struct ns16550 *uart)
> {
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
> return;
>
> @@ -355,7 +355,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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> struct ns16550 *uart = port->uart;
>
> if ( uart->msi )
> @@ -397,7 +397,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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> if ( uart->bar || uart->ps_bdf_enable )
> {
> if ( uart->param && uart->param->mmio &&
> @@ -477,7 +477,7 @@ static void ns16550_suspend(struct serial_port *port)
>
> stop_timer(&uart->timer);
>
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_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);
> @@ -486,7 +486,7 @@ static void ns16550_suspend(struct serial_port *port)
>
> static void _ns16550_resume(struct serial_port *port)
> {
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> struct ns16550 *uart = port->uart;
>
> if ( uart->bar )
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-25 18:21 ` Rahul Singh
@ 2020-11-26 9:08 ` Bertrand Marquis
0 siblings, 0 replies; 19+ messages in thread
From: Bertrand Marquis @ 2020-11-26 9:08 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
> On 25 Nov 2020, at 18:21, Rahul Singh <Rahul.Singh@arm.com> wrote:
>
> Hello Julien,
>
>> On 25 Nov 2020, at 6:16 pm, Rahul Singh <rahul.singh@arm.com> wrote:
>>
>> 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 = {
>> ^~~~~~~~
>>
>> 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.
>>
>> Guard all remaining PCI code that is not under x86 flag with CONFIG_X86.
>>
>> No functional change intended.
>>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>
> Sorry I missed to add the signed off for the commit msg. I will send the next version once reviewed done.
> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
I guess the commiter can add this while commiting this patch ?
Regards
Bertrand
> Regards,
> Rahul
>> ---
>>
>> Changes in v4:
>> - As per the discussion guard all remaining PCI code with CONFIG_X86
>>
>> ---
>> xen/drivers/char/ns16550.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index 9235d854fe..26e601857a 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
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> #include <xen/pci.h>
>> #include <xen/pci_regs.h>
>> #include <xen/pci_ids.h>
>> @@ -51,7 +51,7 @@ static struct ns16550 {
>> unsigned int timeout_ms;
>> bool_t intr_works;
>> bool_t dw_usr_bsy;
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_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 */
>> @@ -66,7 +66,7 @@ static struct ns16550 {
>> #endif
>> } ns16550_com[2] = { { 0 } };
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> struct ns16550_config {
>> u16 vendor_id;
>> u16 dev_id;
>> @@ -256,7 +256,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>>
>> static void pci_serial_early_init(struct ns16550 *uart)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
>> return;
>>
>> @@ -355,7 +355,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
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> struct ns16550 *uart = port->uart;
>>
>> if ( uart->msi )
>> @@ -397,7 +397,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
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> if ( uart->bar || uart->ps_bdf_enable )
>> {
>> if ( uart->param && uart->param->mmio &&
>> @@ -477,7 +477,7 @@ static void ns16550_suspend(struct serial_port *port)
>>
>> stop_timer(&uart->timer);
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_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);
>> @@ -486,7 +486,7 @@ static void ns16550_suspend(struct serial_port *port)
>>
>> static void _ns16550_resume(struct serial_port *port)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> struct ns16550 *uart = port->uart;
>>
>> if ( uart->bar )
>> --
>> 2.17.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-25 18:16 ` [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86 Rahul Singh
2020-11-25 18:21 ` Rahul Singh
@ 2020-11-25 21:28 ` Stefano Stabellini
2020-11-27 13:58 ` Jan Beulich
2 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2020-11-25 21:28 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
[-- Attachment #1: Type: text/plain, Size: 4781 bytes --]
On Wed, 25 Nov 2020, Rahul Singh wrote:
> 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 = {
> ^~~~~~~~
>
> 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.
>
> Guard all remaining PCI code that is not under x86 flag with CONFIG_X86.
>
> No functional change intended.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
>
> Changes in v4:
> - As per the discussion guard all remaining PCI code with CONFIG_X86
>
> ---
> xen/drivers/char/ns16550.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 9235d854fe..26e601857a 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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> #include <xen/pci.h>
> #include <xen/pci_regs.h>
> #include <xen/pci_ids.h>
> @@ -51,7 +51,7 @@ static struct ns16550 {
> unsigned int timeout_ms;
> bool_t intr_works;
> bool_t dw_usr_bsy;
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_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 */
> @@ -66,7 +66,7 @@ static struct ns16550 {
> #endif
> } ns16550_com[2] = { { 0 } };
>
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> struct ns16550_config {
> u16 vendor_id;
> u16 dev_id;
> @@ -256,7 +256,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>
> static void pci_serial_early_init(struct ns16550 *uart)
> {
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
> return;
>
> @@ -355,7 +355,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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> struct ns16550 *uart = port->uart;
>
> if ( uart->msi )
> @@ -397,7 +397,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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> if ( uart->bar || uart->ps_bdf_enable )
> {
> if ( uart->param && uart->param->mmio &&
> @@ -477,7 +477,7 @@ static void ns16550_suspend(struct serial_port *port)
>
> stop_timer(&uart->timer);
>
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_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);
> @@ -486,7 +486,7 @@ static void ns16550_suspend(struct serial_port *port)
>
> static void _ns16550_resume(struct serial_port *port)
> {
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> struct ns16550 *uart = port->uart;
>
> if ( uart->bar )
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-25 18:16 ` [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86 Rahul Singh
2020-11-25 18:21 ` Rahul Singh
2020-11-25 21:28 ` Stefano Stabellini
@ 2020-11-27 13:58 ` Jan Beulich
2020-11-27 14:16 ` Bertrand Marquis
2020-11-27 14:25 ` Rahul Singh
2 siblings, 2 replies; 19+ messages in thread
From: Jan Beulich @ 2020-11-27 13:58 UTC (permalink / raw)
To: Rahul Singh
Cc: bertrand.marquis, Andrew Cooper, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
On 25.11.2020 19:16, Rahul Singh wrote:
> --- 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
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> #include <xen/pci.h>
> #include <xen/pci_regs.h>
> #include <xen/pci_ids.h>
> @@ -51,7 +51,7 @@ static struct ns16550 {
> unsigned int timeout_ms;
> bool_t intr_works;
> bool_t dw_usr_bsy;
> -#ifdef CONFIG_HAS_PCI
> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
I'm sorry to be picky, but this being a hack wants, imo, also calling
it so, by way of a code comment. Clearly this should go at one of the
first instances, yet neither of the two above are really suitable imo.
Hence I'm coming back to my prior suggestion of introducing a
consolidated #define without this becoming a Kconfig setting:
/*
* The PCI part of the code in this file currently is only known to
* work on x86. Undo this hack once the logic has been suitably
* abstracted.
*/
#if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
# define NS16550_PCI
#endif
And then use NS16550_PCI everywhere. I'd be fine making this
adjustment while committing, if I knew that (a) you're okay with it
and (b) the R-b and A-b you've already got can be kept.
Jan
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-27 13:58 ` Jan Beulich
@ 2020-11-27 14:16 ` Bertrand Marquis
2020-11-30 21:24 ` Stefano Stabellini
2020-11-27 14:25 ` Rahul Singh
1 sibling, 1 reply; 19+ messages in thread
From: Bertrand Marquis @ 2020-11-27 14:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Rahul Singh, Andrew Cooper, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
Hi Jan,
> On 27 Nov 2020, at 13:58, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.11.2020 19:16, Rahul Singh wrote:
>> --- 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
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> #include <xen/pci.h>
>> #include <xen/pci_regs.h>
>> #include <xen/pci_ids.h>
>> @@ -51,7 +51,7 @@ static struct ns16550 {
>> unsigned int timeout_ms;
>> bool_t intr_works;
>> bool_t dw_usr_bsy;
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>
> I'm sorry to be picky, but this being a hack wants, imo, also calling
> it so, by way of a code comment. Clearly this should go at one of the
> first instances, yet neither of the two above are really suitable imo.
> Hence I'm coming back to my prior suggestion of introducing a
> consolidated #define without this becoming a Kconfig setting:
>
> /*
> * The PCI part of the code in this file currently is only known to
> * work on x86. Undo this hack once the logic has been suitably
> * abstracted.
> */
> #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
> # define NS16550_PCI
> #endif
>
> And then use NS16550_PCI everywhere. I'd be fine making this
> adjustment while committing, if I knew that (a) you're okay with it
> and (b) the R-b and A-b you've already got can be kept.
>
Sounds ok to me so you can keep my R-b if you go this way.
Cheers
Bertrand
> Jan
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-27 14:16 ` Bertrand Marquis
@ 2020-11-30 21:24 ` Stefano Stabellini
0 siblings, 0 replies; 19+ messages in thread
From: Stefano Stabellini @ 2020-11-30 21:24 UTC (permalink / raw)
To: Bertrand Marquis
Cc: Jan Beulich, Rahul Singh, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel
On Fri, 27 Nov 2020, Bertrand Marquis wrote:
> Hi Jan,
>
> > On 27 Nov 2020, at 13:58, Jan Beulich <jbeulich@suse.com> wrote:
> >
> > On 25.11.2020 19:16, Rahul Singh wrote:
> >> --- 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
> >> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> >> #include <xen/pci.h>
> >> #include <xen/pci_regs.h>
> >> #include <xen/pci_ids.h>
> >> @@ -51,7 +51,7 @@ static struct ns16550 {
> >> unsigned int timeout_ms;
> >> bool_t intr_works;
> >> bool_t dw_usr_bsy;
> >> -#ifdef CONFIG_HAS_PCI
> >> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
> >
> > I'm sorry to be picky, but this being a hack wants, imo, also calling
> > it so, by way of a code comment. Clearly this should go at one of the
> > first instances, yet neither of the two above are really suitable imo.
> > Hence I'm coming back to my prior suggestion of introducing a
> > consolidated #define without this becoming a Kconfig setting:
> >
> > /*
> > * The PCI part of the code in this file currently is only known to
> > * work on x86. Undo this hack once the logic has been suitably
> > * abstracted.
> > */
> > #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
> > # define NS16550_PCI
> > #endif
> >
> > And then use NS16550_PCI everywhere. I'd be fine making this
> > adjustment while committing, if I knew that (a) you're okay with it
> > and (b) the R-b and A-b you've already got can be kept.
> >
>
> Sounds ok to me so you can keep my R-b if you go this way.
I am OK with that too
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86
2020-11-27 13:58 ` Jan Beulich
2020-11-27 14:16 ` Bertrand Marquis
@ 2020-11-27 14:25 ` Rahul Singh
1 sibling, 0 replies; 19+ messages in thread
From: Rahul Singh @ 2020-11-27 14:25 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Andrew Cooper, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu, xen-devel
Hello Jan,
> On 27 Nov 2020, at 1:58 pm, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.11.2020 19:16, Rahul Singh wrote:
>> --- 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
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>> #include <xen/pci.h>
>> #include <xen/pci_regs.h>
>> #include <xen/pci_ids.h>
>> @@ -51,7 +51,7 @@ static struct ns16550 {
>> unsigned int timeout_ms;
>> bool_t intr_works;
>> bool_t dw_usr_bsy;
>> -#ifdef CONFIG_HAS_PCI
>> +#if defined(CONFIG_X86) && defined(CONFIG_HAS_PCI)
>
> I'm sorry to be picky, but this being a hack wants, imo, also calling
> it so, by way of a code comment. Clearly this should go at one of the
> first instances, yet neither of the two above are really suitable imo.
> Hence I'm coming back to my prior suggestion of introducing a
> consolidated #define without this becoming a Kconfig setting:
>
> /*
> * The PCI part of the code in this file currently is only known to
> * work on x86. Undo this hack once the logic has been suitably
> * abstracted.
> */
> #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
> # define NS16550_PCI
> #endif
>
> And then use NS16550_PCI everywhere. I'd be fine making this
> adjustment while committing, if I knew that (a) you're okay with it
> and
Thanks for reviewing the code.I am ok with it.
> (b) the R-b and A-b you've already got can be kept.
>
> Jan
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific
2020-11-25 18:16 [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
` (2 preceding siblings ...)
2020-11-25 18:16 ` [PATCH v4 3/3] ns16550: Gate all PCI code with CONFIG_X86 Rahul Singh
@ 2020-11-27 20:04 ` Andrew Cooper
2020-11-27 20:09 ` Andrew Cooper
3 siblings, 1 reply; 19+ messages in thread
From: Andrew Cooper @ 2020-11-27 20:04 UTC (permalink / raw)
To: Rahul Singh, xen-devel
Cc: bertrand.marquis, Jan Beulich, Paul Durrant, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu
On 25/11/2020 18:16, Rahul Singh wrote:
> This patch series is v4 of preparatory work to make PCI passthrough code
> non-x86 specific.
>
> Rahul Singh (3):
> xen/pci: Move x86 specific code to x86 directory.
> xen/pci: solve compilation error on ARM with HAS_PCI enabled.
> ns16550: Gate all PCI code with CONFIG_X86
https://gitlab.com/xen-project/patchew/xen/-/pipelines/222243396
There was an ARM randconfig failure which looks relevant to the content
in this series.
~Andrew (in lieu of a real CI robot).
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific
2020-11-27 20:04 ` [PATCH v4 0/3] xen/arm: Make PCI passthrough code non-x86 specific Andrew Cooper
@ 2020-11-27 20:09 ` Andrew Cooper
0 siblings, 0 replies; 19+ messages in thread
From: Andrew Cooper @ 2020-11-27 20:09 UTC (permalink / raw)
To: Rahul Singh, xen-devel
Cc: bertrand.marquis, Jan Beulich, Paul Durrant, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu
On 27/11/2020 20:04, Andrew Cooper wrote:
> On 25/11/2020 18:16, Rahul Singh wrote:
>> This patch series is v4 of preparatory work to make PCI passthrough code
>> non-x86 specific.
>>
>> Rahul Singh (3):
>> xen/pci: Move x86 specific code to x86 directory.
>> xen/pci: solve compilation error on ARM with HAS_PCI enabled.
>> ns16550: Gate all PCI code with CONFIG_X86
> https://gitlab.com/xen-project/patchew/xen/-/pipelines/222243396
>
> There was an ARM randconfig failure which looks relevant to the content
> in this series.
Sorry - this randconfig failure was also seen against a second series,
so probably is collateral damage from elsewhere.
~Andrew
^ permalink raw reply [flat|nested] 19+ messages in thread