From: Denis Mukhin <dmukhin@ford.com>
PVH domains use vIOAPIC, not vPIC and NS16550 emulates ISA IRQs which cannot
be asserted on vIOAPIC.
{map,unmap}_domain_emuirq_pirq() infrastructure is modified by adding new
type of interrupt resources 'IRQ_EMU' which means 'emulated device IRQ'
(similarly to IRQ_MSI_EMU).
This is necessary to for IOAPIC emulation code to skip IRQ->PIRQ mapping
(vioapic_hwdom_map_gsi()) when guest OS unmasks vIOAPIC pin corresponding to
virtual device's IRQ.
Also, hvm_gsi_eoi() is modified to trigger assertion in hvm_gsi_deassert()
path for ISA IRQs.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v4:
- dropped xl bits
- cosmetic renames
- fix hvm_gsi_eoi()
- Link to v4: https://lore.kernel.org/xen-devel/20250731192130.3948419-8-dmukhin@ford.com/
---
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/hvm/vioapic.c | 10 ++++++++++
xen/arch/x86/include/asm/irq.h | 3 ++-
xen/arch/x86/irq.c | 4 ++--
xen/arch/x86/physdev.c | 8 ++++----
xen/common/emul/vuart/ns16x50.c | 32 +++++++++++++++++++++++++++++--
xen/drivers/passthrough/x86/hvm.c | 13 ++++++++-----
7 files changed, 57 insertions(+), 15 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 19fd86ce88d2..0815d0b31827 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1048,7 +1048,7 @@ int arch_domain_soft_reset(struct domain *d)
write_lock(&d->event_lock);
for ( i = 0; i < d->nr_pirqs ; i++ )
{
- if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
+ if ( domain_irq_to_emuirq(d, i) != IRQ_UNBOUND )
{
ret = unmap_domain_pirq_emuirq(d, i);
if ( ret )
diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index 7c725f9e471f..fd073f6fba4b 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -177,6 +177,16 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
ASSERT(is_hardware_domain(currd));
+ /*
+ * Interrupt is claimed by one of the platform virtual devices (e.g.
+ * NS16550); do nothing.
+ */
+ write_lock(&currd->event_lock);
+ ret = domain_irq_to_emuirq(currd, gsi);
+ write_unlock(&currd->event_lock);
+ if ( ret != IRQ_UNBOUND )
+ return 0;
+
/* Interrupt has been unmasked, bind it now. */
ret = mp_register_gsi(gsi, trig, pol);
if ( ret == -EEXIST )
diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 8bffec3bbfee..3b24decb05e4 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -212,7 +212,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
__ret ? radix_tree_ptr_to_int(__ret) : 0; \
})
#define PIRQ_ALLOCATED (-1)
-#define domain_pirq_to_emuirq(d, pirq) pirq_field(d, pirq, \
+#define domain_irq_to_emuirq(d, pirq) pirq_field(d, pirq, \
arch.hvm.emuirq, IRQ_UNBOUND)
#define domain_emuirq_to_pirq(d, emuirq) ({ \
void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
@@ -221,6 +221,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
#define IRQ_UNBOUND (-1)
#define IRQ_PT (-2)
#define IRQ_MSI_EMU (-3)
+#define IRQ_EMU (-4)
bool cpu_has_pending_apic_eoi(void);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 079277be719d..7a8093cd3238 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2790,7 +2790,7 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq)
return -EINVAL;
}
- old_emuirq = domain_pirq_to_emuirq(d, pirq);
+ old_emuirq = domain_irq_to_emuirq(d, pirq);
if ( emuirq != IRQ_PT )
old_pirq = domain_emuirq_to_pirq(d, emuirq);
@@ -2845,7 +2845,7 @@ int unmap_domain_pirq_emuirq(struct domain *d, int pirq)
ASSERT(rw_is_write_locked(&d->event_lock));
- emuirq = domain_pirq_to_emuirq(d, pirq);
+ emuirq = domain_irq_to_emuirq(d, pirq);
if ( emuirq == IRQ_UNBOUND )
{
dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 4dfa1c019105..90a9e7d2f120 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -145,7 +145,7 @@ int physdev_unmap_pirq(struct domain *d, int pirq)
if ( is_hvm_domain(d) && has_pirq(d) )
{
write_lock(&d->event_lock);
- if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )
+ if ( domain_irq_to_emuirq(d, pirq) != IRQ_UNBOUND )
ret = unmap_domain_pirq_emuirq(d, pirq);
write_unlock(&d->event_lock);
if ( d == current->domain || ret )
@@ -191,10 +191,10 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
if ( is_pv_domain(currd) || domain_pirq_to_irq(currd, eoi.irq) > 0 )
pirq_guest_eoi(pirq);
if ( is_hvm_domain(currd) &&
- domain_pirq_to_emuirq(currd, eoi.irq) > 0 )
+ domain_irq_to_emuirq(currd, eoi.irq) > 0 )
{
struct hvm_irq *hvm_irq = hvm_domain_irq(currd);
- int gsi = domain_pirq_to_emuirq(currd, eoi.irq);
+ int gsi = domain_irq_to_emuirq(currd, eoi.irq);
/* if this is a level irq and count > 0, send another
* notification */
@@ -267,7 +267,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
irq_status_query.flags = 0;
if ( is_hvm_domain(currd) &&
domain_pirq_to_irq(currd, irq) <= 0 &&
- domain_pirq_to_emuirq(currd, irq) == IRQ_UNBOUND )
+ domain_irq_to_emuirq(currd, irq) == IRQ_UNBOUND )
{
ret = -EINVAL;
break;
diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
index aea38304b60c..1126b53c30df 100644
--- a/xen/common/emul/vuart/ns16x50.c
+++ b/xen/common/emul/vuart/ns16x50.c
@@ -287,7 +287,9 @@ static void ns16x50_irq_assert(const struct vuart_ns16x50 *vdev)
const struct vuart_info *info = vdev->info;
int vector;
- if ( has_vpic(d) ) /* HVM */
+ if ( has_vioapic(d) && !has_vpic(d) ) /* PVH */
+ vector = hvm_ioapic_assert(d, info->irq, false);
+ else if ( has_vpic(d) ) /* HVM */
vector = hvm_isa_irq_assert(d, info->irq, vioapic_get_vector);
else
ASSERT_UNREACHABLE();
@@ -300,7 +302,9 @@ static void ns16x50_irq_deassert(const struct vuart_ns16x50 *vdev)
struct domain *d = vdev->owner;
const struct vuart_info *info = vdev->info;
- if ( has_vpic(d) ) /* HVM */
+ if ( has_vioapic(d) && !has_vpic(d) ) /* PVH */
+ hvm_ioapic_deassert(d, info->irq);
+ else if ( has_vpic(d) ) /* HVM */
hvm_isa_irq_deassert(d, info->irq);
else
ASSERT_UNREACHABLE();
@@ -803,6 +807,17 @@ static int ns16x50_init(void *arg)
return rc;
}
+ /* Claim virtual IRQ */
+ write_lock(&d->event_lock);
+ rc = map_domain_emuirq_pirq(d, info->irq, IRQ_EMU);
+ write_unlock(&d->event_lock);
+ if ( rc )
+ {
+ ns16x50_err(info, "virtual IRQ#%d: cannot claim: %d\n",
+ info->irq, rc);
+ return rc;
+ }
+
/* NB: report 115200 baud rate. */
vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & 0xff;
vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & 0xff;
@@ -822,9 +837,22 @@ static int ns16x50_init(void *arg)
static void cf_check ns16x50_deinit(void *arg)
{
struct vuart_ns16x50 *vdev = arg;
+ const struct vuart_info *info;
+ struct domain *d;
+ int rc;
ASSERT(vdev);
+ d = vdev->owner;
+ info = vdev->info;
+
+ write_lock(&d->event_lock);
+ rc = unmap_domain_pirq_emuirq(d, info->irq);
+ write_unlock(&d->event_lock);
+ if ( rc )
+ ns16x50_err(vdev, "virtual IRQ#%d: cannot unclaim: %d\n",
+ info->irq, rc);
+
spin_lock(&vdev->lock);
ns16x50_fifo_tx_flush(vdev);
spin_unlock(&vdev->lock);
diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
index a2ca7e0e570c..11711d20a7ea 100644
--- a/xen/drivers/passthrough/x86/hvm.c
+++ b/xen/drivers/passthrough/x86/hvm.c
@@ -923,13 +923,16 @@ static void __hvm_dpci_eoi(struct domain *d,
static void hvm_gsi_eoi(struct domain *d, unsigned int gsi)
{
struct pirq *pirq = pirq_info(d, gsi);
+ int rc;
+
+ /* Check if GSI is claimed by one of the virtual devices. */
+ rc = domain_irq_to_emuirq(d, gsi);
+ if ( rc != IRQ_UNBOUND )
+ hvm_gsi_deassert(d, gsi);
/* Check if GSI is actually mapped. */
- if ( !pirq_dpci(pirq) )
- return;
-
- hvm_gsi_deassert(d, gsi);
- hvm_pirq_eoi(pirq);
+ if ( pirq_dpci(pirq) )
+ hvm_pirq_eoi(pirq);
}
static int cf_check _hvm_dpci_isairq_eoi(
--
2.51.0On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
> From: Denis Mukhin <dmukhin@ford.com>
>
> PVH domains use vIOAPIC, not vPIC and NS16550 emulates ISA IRQs which cannot
> be asserted on vIOAPIC.
One option is to enable the vPIT for PVH domains when the NS16550
emulator is enabled. Would that resolve the problem? That would be a
simpler solution compared to adding IRQ_EMU because the IRQ_* logic is
already quite complex.
Alternatively...
> {map,unmap}_domain_emuirq_pirq() infrastructure is modified by adding new
> type of interrupt resources 'IRQ_EMU' which means 'emulated device IRQ'
> (similarly to IRQ_MSI_EMU).
>
> This is necessary to for IOAPIC emulation code to skip IRQ->PIRQ mapping
> (vioapic_hwdom_map_gsi()) when guest OS unmasks vIOAPIC pin corresponding to
> virtual device's IRQ.
>
> Also, hvm_gsi_eoi() is modified to trigger assertion in hvm_gsi_deassert()
> path for ISA IRQs.
>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v4:
> - dropped xl bits
> - cosmetic renames
> - fix hvm_gsi_eoi()
> - Link to v4: https://lore.kernel.org/xen-devel/20250731192130.3948419-8-dmukhin@ford.com/
> ---
> xen/arch/x86/domain.c | 2 +-
> xen/arch/x86/hvm/vioapic.c | 10 ++++++++++
> xen/arch/x86/include/asm/irq.h | 3 ++-
> xen/arch/x86/irq.c | 4 ++--
> xen/arch/x86/physdev.c | 8 ++++----
> xen/common/emul/vuart/ns16x50.c | 32 +++++++++++++++++++++++++++++--
> xen/drivers/passthrough/x86/hvm.c | 13 ++++++++-----
> 7 files changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 19fd86ce88d2..0815d0b31827 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1048,7 +1048,7 @@ int arch_domain_soft_reset(struct domain *d)
> write_lock(&d->event_lock);
> for ( i = 0; i < d->nr_pirqs ; i++ )
> {
> - if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
> + if ( domain_irq_to_emuirq(d, i) != IRQ_UNBOUND )
> {
> ret = unmap_domain_pirq_emuirq(d, i);
> if ( ret )
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 7c725f9e471f..fd073f6fba4b 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -177,6 +177,16 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
>
> ASSERT(is_hardware_domain(currd));
>
> + /*
> + * Interrupt is claimed by one of the platform virtual devices (e.g.
> + * NS16550); do nothing.
> + */
> + write_lock(&currd->event_lock);
> + ret = domain_irq_to_emuirq(currd, gsi);
> + write_unlock(&currd->event_lock);
> + if ( ret != IRQ_UNBOUND )
> + return 0;
..alternatively, we could have an add-hoc check here? Not very nice but
at least it would be very simple.
In other words, adding vPIT is preferable in my opinion but as a second
option I would consider an ad-hoc check. I would try to avoid adding
IRQ_EMU -- that should be the last resort in my view.
> /* Interrupt has been unmasked, bind it now. */
> ret = mp_register_gsi(gsi, trig, pol);
> if ( ret == -EEXIST )
> diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
> index 8bffec3bbfee..3b24decb05e4 100644
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -212,7 +212,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
> __ret ? radix_tree_ptr_to_int(__ret) : 0; \
> })
> #define PIRQ_ALLOCATED (-1)
> -#define domain_pirq_to_emuirq(d, pirq) pirq_field(d, pirq, \
> +#define domain_irq_to_emuirq(d, pirq) pirq_field(d, pirq, \
> arch.hvm.emuirq, IRQ_UNBOUND)
> #define domain_emuirq_to_pirq(d, emuirq) ({ \
> void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
> @@ -221,6 +221,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
> #define IRQ_UNBOUND (-1)
> #define IRQ_PT (-2)
> #define IRQ_MSI_EMU (-3)
> +#define IRQ_EMU (-4)
>
> bool cpu_has_pending_apic_eoi(void);
>
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 079277be719d..7a8093cd3238 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2790,7 +2790,7 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, int emuirq)
> return -EINVAL;
> }
>
> - old_emuirq = domain_pirq_to_emuirq(d, pirq);
> + old_emuirq = domain_irq_to_emuirq(d, pirq);
> if ( emuirq != IRQ_PT )
> old_pirq = domain_emuirq_to_pirq(d, emuirq);
>
> @@ -2845,7 +2845,7 @@ int unmap_domain_pirq_emuirq(struct domain *d, int pirq)
>
> ASSERT(rw_is_write_locked(&d->event_lock));
>
> - emuirq = domain_pirq_to_emuirq(d, pirq);
> + emuirq = domain_irq_to_emuirq(d, pirq);
> if ( emuirq == IRQ_UNBOUND )
> {
> dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
> diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
> index 4dfa1c019105..90a9e7d2f120 100644
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -145,7 +145,7 @@ int physdev_unmap_pirq(struct domain *d, int pirq)
> if ( is_hvm_domain(d) && has_pirq(d) )
> {
> write_lock(&d->event_lock);
> - if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )
> + if ( domain_irq_to_emuirq(d, pirq) != IRQ_UNBOUND )
> ret = unmap_domain_pirq_emuirq(d, pirq);
> write_unlock(&d->event_lock);
> if ( d == current->domain || ret )
> @@ -191,10 +191,10 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( is_pv_domain(currd) || domain_pirq_to_irq(currd, eoi.irq) > 0 )
> pirq_guest_eoi(pirq);
> if ( is_hvm_domain(currd) &&
> - domain_pirq_to_emuirq(currd, eoi.irq) > 0 )
> + domain_irq_to_emuirq(currd, eoi.irq) > 0 )
> {
> struct hvm_irq *hvm_irq = hvm_domain_irq(currd);
> - int gsi = domain_pirq_to_emuirq(currd, eoi.irq);
> + int gsi = domain_irq_to_emuirq(currd, eoi.irq);
>
> /* if this is a level irq and count > 0, send another
> * notification */
> @@ -267,7 +267,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> irq_status_query.flags = 0;
> if ( is_hvm_domain(currd) &&
> domain_pirq_to_irq(currd, irq) <= 0 &&
> - domain_pirq_to_emuirq(currd, irq) == IRQ_UNBOUND )
> + domain_irq_to_emuirq(currd, irq) == IRQ_UNBOUND )
> {
> ret = -EINVAL;
> break;
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index aea38304b60c..1126b53c30df 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -287,7 +287,9 @@ static void ns16x50_irq_assert(const struct vuart_ns16x50 *vdev)
> const struct vuart_info *info = vdev->info;
> int vector;
>
> - if ( has_vpic(d) ) /* HVM */
> + if ( has_vioapic(d) && !has_vpic(d) ) /* PVH */
> + vector = hvm_ioapic_assert(d, info->irq, false);
> + else if ( has_vpic(d) ) /* HVM */
> vector = hvm_isa_irq_assert(d, info->irq, vioapic_get_vector);
> else
> ASSERT_UNREACHABLE();
> @@ -300,7 +302,9 @@ static void ns16x50_irq_deassert(const struct vuart_ns16x50 *vdev)
> struct domain *d = vdev->owner;
> const struct vuart_info *info = vdev->info;
>
> - if ( has_vpic(d) ) /* HVM */
> + if ( has_vioapic(d) && !has_vpic(d) ) /* PVH */
> + hvm_ioapic_deassert(d, info->irq);
> + else if ( has_vpic(d) ) /* HVM */
> hvm_isa_irq_deassert(d, info->irq);
> else
> ASSERT_UNREACHABLE();
> @@ -803,6 +807,17 @@ static int ns16x50_init(void *arg)
> return rc;
> }
>
> + /* Claim virtual IRQ */
> + write_lock(&d->event_lock);
> + rc = map_domain_emuirq_pirq(d, info->irq, IRQ_EMU);
> + write_unlock(&d->event_lock);
> + if ( rc )
> + {
> + ns16x50_err(info, "virtual IRQ#%d: cannot claim: %d\n",
> + info->irq, rc);
> + return rc;
> + }
> +
> /* NB: report 115200 baud rate. */
> vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & 0xff;
> vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & 0xff;
> @@ -822,9 +837,22 @@ static int ns16x50_init(void *arg)
> static void cf_check ns16x50_deinit(void *arg)
> {
> struct vuart_ns16x50 *vdev = arg;
> + const struct vuart_info *info;
> + struct domain *d;
> + int rc;
>
> ASSERT(vdev);
>
> + d = vdev->owner;
> + info = vdev->info;
> +
> + write_lock(&d->event_lock);
> + rc = unmap_domain_pirq_emuirq(d, info->irq);
> + write_unlock(&d->event_lock);
> + if ( rc )
> + ns16x50_err(vdev, "virtual IRQ#%d: cannot unclaim: %d\n",
> + info->irq, rc);
> +
> spin_lock(&vdev->lock);
> ns16x50_fifo_tx_flush(vdev);
> spin_unlock(&vdev->lock);
> diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c
> index a2ca7e0e570c..11711d20a7ea 100644
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -923,13 +923,16 @@ static void __hvm_dpci_eoi(struct domain *d,
> static void hvm_gsi_eoi(struct domain *d, unsigned int gsi)
> {
> struct pirq *pirq = pirq_info(d, gsi);
> + int rc;
> +
> + /* Check if GSI is claimed by one of the virtual devices. */
> + rc = domain_irq_to_emuirq(d, gsi);
> + if ( rc != IRQ_UNBOUND )
> + hvm_gsi_deassert(d, gsi);
>
> /* Check if GSI is actually mapped. */
> - if ( !pirq_dpci(pirq) )
> - return;
> -
> - hvm_gsi_deassert(d, gsi);
> - hvm_pirq_eoi(pirq);
> + if ( pirq_dpci(pirq) )
> + hvm_pirq_eoi(pirq);
> }
>
> static int cf_check _hvm_dpci_isairq_eoi(
> --
> 2.51.0
>
On Fri, Aug 29, 2025 at 03:21:13PM -0700, Stefano Stabellini wrote: > On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > > From: Denis Mukhin <dmukhin@ford.com> > > > > PVH domains use vIOAPIC, not vPIC and NS16550 emulates ISA IRQs which cannot > > be asserted on vIOAPIC. > > One option is to enable the vPIT for PVH domains when the NS16550 > emulator is enabled. Would that resolve the problem? That would be a > simpler solution compared to adding IRQ_EMU because the IRQ_* logic is > already quite complex. vPIC? (PIT is timer). I tried to enable vPIC for hwdom, but there's some more work to make it work :-/ > > Alternatively... [..] > > > --- a/xen/arch/x86/hvm/vioapic.c > > +++ b/xen/arch/x86/hvm/vioapic.c > > @@ -177,6 +177,16 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, > > > > ASSERT(is_hardware_domain(currd)); > > > > + /* > > + * Interrupt is claimed by one of the platform virtual devices (e.g. > > + * NS16550); do nothing. > > + */ > > + write_lock(&currd->event_lock); > > + ret = domain_irq_to_emuirq(currd, gsi); > > + write_unlock(&currd->event_lock); > > + if ( ret != IRQ_UNBOUND ) > > + return 0; > > ..alternatively, we could have an add-hoc check here? Not very nice but > at least it would be very simple. > > In other words, adding vPIT is preferable in my opinion but as a second > option I would consider an ad-hoc check. I would try to avoid adding > IRQ_EMU -- that should be the last resort in my view. In my opinion, IRQ_EMU falls into category of an ad-hoc. I think vioapic_hwdom_map_gsi() should not depend on the presence of certain virtual devices but rely on some global flag, e.g. via quering domain_irq_to_emuirq() infra.
On Fri, 5 Sep 2025, dmukhin@xen.org wrote: > On Fri, Aug 29, 2025 at 03:21:13PM -0700, Stefano Stabellini wrote: > > On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > > > From: Denis Mukhin <dmukhin@ford.com> > > > > > > PVH domains use vIOAPIC, not vPIC and NS16550 emulates ISA IRQs which cannot > > > be asserted on vIOAPIC. > > > > One option is to enable the vPIT for PVH domains when the NS16550 > > emulator is enabled. Would that resolve the problem? That would be a > > simpler solution compared to adding IRQ_EMU because the IRQ_* logic is > > already quite complex. > > vPIC? (PIT is timer). > > I tried to enable vPIC for hwdom, but there's some more work to make it work > :-/ > > > > > Alternatively... > [..] > > > > > --- a/xen/arch/x86/hvm/vioapic.c > > > +++ b/xen/arch/x86/hvm/vioapic.c > > > @@ -177,6 +177,16 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, > > > > > > ASSERT(is_hardware_domain(currd)); > > > > > > + /* > > > + * Interrupt is claimed by one of the platform virtual devices (e.g. > > > + * NS16550); do nothing. > > > + */ > > > + write_lock(&currd->event_lock); > > > + ret = domain_irq_to_emuirq(currd, gsi); > > > + write_unlock(&currd->event_lock); > > > + if ( ret != IRQ_UNBOUND ) > > > + return 0; > > > > ..alternatively, we could have an add-hoc check here? Not very nice but > > at least it would be very simple. > > > > In other words, adding vPIT is preferable in my opinion but as a second > > option I would consider an ad-hoc check. I would try to avoid adding > > IRQ_EMU -- that should be the last resort in my view. > > In my opinion, IRQ_EMU falls into category of an ad-hoc. > > I think vioapic_hwdom_map_gsi() should not depend on the presence of certain > virtual devices but rely on some global flag, e.g. via quering > domain_irq_to_emuirq() infra. Hi Denis, the reason why IRQ_EMU is dangerous is that there are a bunch of checks emuirq != IRQ_PT and also emuirq != IRQ_MSI_EMU which I don't know if they would still behave correctly after the introduction of IRQ_EMU. I would prefer to avoid the problem if possible...
On Fri, Sep 05, 2025 at 04:01:15PM -0700, Stefano Stabellini wrote: > On Fri, 5 Sep 2025, dmukhin@xen.org wrote: > > On Fri, Aug 29, 2025 at 03:21:13PM -0700, Stefano Stabellini wrote: > > > On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > > > > From: Denis Mukhin <dmukhin@ford.com> > > > > > > > > PVH domains use vIOAPIC, not vPIC and NS16550 emulates ISA IRQs which cannot > > > > be asserted on vIOAPIC. > > > > > > One option is to enable the vPIT for PVH domains when the NS16550 > > > emulator is enabled. Would that resolve the problem? That would be a > > > simpler solution compared to adding IRQ_EMU because the IRQ_* logic is > > > already quite complex. > > > > vPIC? (PIT is timer). > > > > I tried to enable vPIC for hwdom, but there's some more work to make it work > > :-/ > > > > > > > > Alternatively... > > [..] > > > > > > > --- a/xen/arch/x86/hvm/vioapic.c > > > > +++ b/xen/arch/x86/hvm/vioapic.c > > > > @@ -177,6 +177,16 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig, > > > > > > > > ASSERT(is_hardware_domain(currd)); > > > > > > > > + /* > > > > + * Interrupt is claimed by one of the platform virtual devices (e.g. > > > > + * NS16550); do nothing. > > > > + */ > > > > + write_lock(&currd->event_lock); > > > > + ret = domain_irq_to_emuirq(currd, gsi); > > > > + write_unlock(&currd->event_lock); > > > > + if ( ret != IRQ_UNBOUND ) > > > > + return 0; > > > > > > ..alternatively, we could have an add-hoc check here? Not very nice but > > > at least it would be very simple. > > > > > > In other words, adding vPIT is preferable in my opinion but as a second > > > option I would consider an ad-hoc check. I would try to avoid adding > > > IRQ_EMU -- that should be the last resort in my view. > > > > In my opinion, IRQ_EMU falls into category of an ad-hoc. > > > > I think vioapic_hwdom_map_gsi() should not depend on the presence of certain > > virtual devices but rely on some global flag, e.g. via quering > > domain_irq_to_emuirq() infra. > > Hi Denis, the reason why IRQ_EMU is dangerous is that there are a bunch > of checks emuirq != IRQ_PT and also emuirq != IRQ_MSI_EMU which I don't > know if they would still behave correctly after the introduction of > IRQ_EMU. != IRQ_PT checks (arch/x86/irq.c) are done from map_domain_emuirq_pirq() and unmap_domain_pirq_emuirq() which I use for IRQ_EMU case. Those checks are needed to correctly allocate an "ad-hoc flag" in a radix "IRQ_EMU" tree, i.e. IRQ_EMU case is handled correctly. != IRQ_MSI_EMU check (there's only one in arch/x86/hvm/irq.c) is done from hvm_inject_msi(), it guarantees that IRQ_EMU case is handled correctly. > > I would prefer to avoid the problem if possible...
© 2016 - 2025 Red Hat, Inc.