Re: [PATCH v4 7/8] tools/xl: enable NS16550-compatible UART emulator for PVH (x86)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2025 21:22, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> 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 <dmukhin@xxxxxxxx>
> ---
> 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