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

[dmkhn@xxxxxxxxx]

On Mon, Aug 04, 2025 at 01:06:32PM +0200, Jan Beulich wrote:
> 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 ...

Yeah, my bad, that should have been saying something like
"enable IRQ emulation via vIOAPIC"

Will update.

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