Re: [PATCH v7 15/16] xen/domain: allocate d->irq_caps before arch-specific initialization

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the patch.

On Tue, Sep 9, 2025 at 12:12 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Make sure that NS16550 emulator does not share virtual device IRQ with the
> physical one. This is needed for enabling NS16550 emulator for PVH hwdom
> (dom0).
>
> To do that, move per-domain interrupt rangeset allocation before arch-specific
> code. Add irqs_setup_access() to setup the initial rangeset.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - n/a
> ---
>  xen/arch/x86/dom0_build.c       | 1 -
>  xen/arch/x86/hvm/dom0_build.c   | 7 +++++++
>  xen/arch/x86/include/asm/irq.h  | 2 ++
>  xen/arch/x86/irq.c              | 8 ++++++++
>  xen/arch/x86/pv/dom0_build.c    | 3 +++
>  xen/common/domain.c             | 8 ++++++--
>  xen/common/emul/vuart/ns16x50.c | 9 +++++++++
>  7 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> index 26202b33345c..9dc87efbf3e8 100644
> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -442,7 +442,6 @@ int __init dom0_setup_permissions(struct domain *d)
>
>      rc |= iomem_permit_access(d, 0UL,
>                                PFN_DOWN(1UL << domain_max_paddr_bits(d)) - 1);
> -    rc |= irqs_permit_access(d, 1, nr_irqs_gsi - 1);
>
>      /* Local APIC. */
>      if ( mp_lapic_addr != 0 )
> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> index 5551f9044836..245a42dec9aa 100644
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -1348,6 +1348,13 @@ int __init dom0_construct_pvh(const struct boot_domain 
> *bd)
>           */
>          pvh_setup_mmcfg(d);
>
> +        rc = irqs_setup_access(d);
> +        if ( rc )
> +        {
> +            printk("%pd unable to setup IRQ rangeset: %d\n", d, rc);
> +            return rc;
> +        }
> +
>          /*
>           * Setup permissions early so that calls to add MMIO regions to the
>           * p2m as part of vPCI setup don't fail due to permission checks.
> diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
> index 8c81f66434a8..8bffec3bbfee 100644
> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -231,4 +231,6 @@ int allocate_and_map_gsi_pirq(struct domain *d, int 
> index, int *pirq_p);
>  int allocate_and_map_msi_pirq(struct domain *d, int index, int *pirq_p,
>                                int type, struct msi_info *msi);
>
> +int irqs_setup_access(struct domain *d);
> +
>  #endif /* _ASM_HW_IRQ_H */
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 556134f85aa0..079277be719d 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -3046,3 +3046,11 @@ int allocate_and_map_msi_pirq(struct domain *d, int 
> index, int *pirq_p,
>
>      return ret;
>  }
> +
> +int irqs_setup_access(struct domain *d)
> +{
> +    if ( is_hardware_domain(d) )
> +        return irqs_permit_access(d, 1, nr_irqs_gsi - 1);
> +
> +    return 0;
> +}
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 2b8b4d869ee7..1a092b802833 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -1037,6 +1037,9 @@ static int __init dom0_construct(const struct 
> boot_domain *bd)
>      rc = ioports_setup_access(d);
>      BUG_ON(rc != 0);
>
> +    rc = irqs_setup_access(d);
> +    BUG_ON(rc != 0);
> +
>      rc = dom0_setup_permissions(d);
>      BUG_ON(rc != 0);
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 775c33928585..edf76b02e1a1 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -952,6 +952,11 @@ struct domain *domain_create(domid_t domid,
>      radix_tree_init(&d->pirq_tree);
>  #endif
>
> +    err = -ENOMEM;
> +    d->irq_caps = rangeset_new(d, "Interrupts", 0);
> +    if ( !d->irq_caps )
> +        goto fail;
> +
>      if ( (err = arch_domain_create(d, config, flags)) != 0 )
>          goto fail;
>      init_status |= INIT_arch;
> @@ -961,8 +966,7 @@ struct domain *domain_create(domid_t domid,
>
>      err = -ENOMEM;
>      d->iomem_caps = rangeset_new(d, "I/O Memory", RANGESETF_prettyprint_hex);
> -    d->irq_caps   = rangeset_new(d, "Interrupts", 0);
> -    if ( !d->iomem_caps || !d->irq_caps )
> +    if ( !d->iomem_caps )
>          goto fail;
>
>      if ( (err = xsm_domain_create(XSM_HOOK, d, config->ssidref)) != 0 )
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index ea34c3ae598a..6bd58ba5540b 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -797,6 +797,15 @@ static int ns16x50_init(void *arg)
>          return rc;
>      }
>
> +    /* Disallow sharing physical IRQ */

Should this be undone on teardown and error paths?


Best regards,
Mykola


> +    rc = irq_deny_access(d, info->irq);
> +    if ( rc )
> +    {
> +        ns16x50_err(info, "virtual IRQ#%d: conflict w/ physical IRQ: %d\n",
> +                    info->irq, rc);
> +        return rc;
> +    }
> +
>      /* NB: report 115200 baud rate. */
>      vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & UINT8_MAX;
>      vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & UINT8_MAX;
> --
> 2.51.0
>
>