On 31.07.2025 21:22, dm...@proton.me wrote:
> From: Denis Mukhin <dmuk...@ford.com>
>
> Enable virtual NS16550 for PVH domains in xl.
>
> {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 <dmuk...@ford.com>
> ---
> Changes since v3:
> - new patch
> ---
> tools/libs/light/libxl_x86.c | 2 +-
> xen/arch/x86/domain.c | 2 ++
> xen/arch/x86/hvm/vioapic.c | 10 ++++++++++
> xen/arch/x86/include/asm/irq.h | 1 +
> xen/common/emul/vuart/vuart-ns16550.c | 27 +++++++++++++++++++++++++--
> xen/drivers/passthrough/x86/hvm.c | 9 ++++-----
> 6 files changed, 43 insertions(+), 8 deletions(-)
Given this diffstat, how come the patch prefix is "tools/xl:"? You don't even
touch xl ...
> --- a/xen/common/emul/vuart/vuart-ns16550.c
> +++ b/xen/common/emul/vuart/vuart-ns16550.c
> @@ -355,7 +355,9 @@ static void ns16550_irq_assert(const struct vuart_ns16550
> *vdev)
> struct domain *d = vdev->owner;
> int vector;
>
> - if ( has_vpic(d) ) /* HVM */
> + if ( has_vioapic(d) && !has_vpic(d) ) /* PVH */
> + vector = hvm_ioapic_assert(d, vdev->irq, false);
> + else if ( has_vpic(d) ) /* HVM */
> vector = hvm_isa_irq_assert(d, vdev->irq, vioapic_get_vector);
Why not
if ( has_vpic(d) ) /* HVM */
vector = hvm_isa_irq_assert(d, vdev->irq, vioapic_get_vector);
else if ( has_vioapic(d) ) /* PVH */
vector = hvm_ioapic_assert(d, vdev->irq, false);
Less code churn and maybe even less generated code.
> @@ -367,7 +369,9 @@ static void ns16550_irq_deassert(const struct
> vuart_ns16550 *vdev)
> {
> struct domain *d = vdev->owner;
>
> - if ( has_vpic(d) ) /* HVM */
> + if ( has_vioapic(d) && !has_vpic(d) ) /* PVH */
> + hvm_ioapic_deassert(d, vdev->irq);
> + else if ( has_vpic(d) ) /* HVM */
> hvm_isa_irq_deassert(d, vdev->irq);
Same here then.
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -924,12 +924,11 @@ static void hvm_gsi_eoi(struct domain *d, unsigned int
> gsi)
> {
> struct pirq *pirq = pirq_info(d, gsi);
>
> - /* Check if GSI is actually mapped. */
> - if ( !pirq_dpci(pirq) )
> - return;
> -
> hvm_gsi_deassert(d, gsi);
> - hvm_pirq_eoi(pirq);
> +
> + /* Check if GSI is actually mapped. */
> + if ( pirq_dpci(pirq) )
> + hvm_pirq_eoi(pirq);
> }
The correctness of this change (in particular hvm_gsi_deassert() now always
running) wants reasoning about in the description.
Jan
