Re: [PATCH v4 3/8] x86/domain: allocate d->{iomem,irq}_caps before arch-specific initialization

[dmkhn@xxxxxxxxx]

On Thu, Jul 31, 2025 at 08:21:24PM +0000, dmkhn@xxxxxxxxx wrote:
> On Thu, Jul 31, 2025 at 10:52:08PM +0300, Grygorii Strashko wrote:
> > Hi Denis,
> >
> > On 31.07.25 22:21, dmkhn@xxxxxxxxx wrote:
> > > From: Denis Mukhin <dmukhin@xxxxxxxx>
> > >
> > > Move IRQ/IOMEM rangesets allocation before arch_domain_create().
> > >
> > > That guarantees that arch-specific code could access those rangesets to
> > > register traps for emulation.
> > >
> > > It is necessary for those emulators registering trap handlers and ensuring
> > > that emulated IRQs are not shared with the physical IRQs.
> > >
> > > Move dom0_setup_permissions() call right after I/O rangesets are 
> > > allocated.
> > >
> > > Move pvh_setup_mmcfg() inside dom0_setup_permissions() close to the place
> > > where MMCFG ranges are initialized.
> > >
> > > Not a functional change.
> > >
> > > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > > ---
> > > Chanhes since v3:
> > > - new patch
> > > ---
> > >   xen/arch/x86/dom0_build.c     | 26 +++++++++++++++++++++++
> > >   xen/arch/x86/hvm/dom0_build.c | 39 -----------------------------------
> > >   xen/arch/x86/hvm/hvm.c        | 16 ++++++++++++++
> > >   xen/common/domain.c           | 12 +++++------
> > >   4 files changed, 48 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
> > > index 0b467fd4a4fc..e965f506a3c8 100644
> > > --- a/xen/arch/x86/dom0_build.c
> > > +++ b/xen/arch/x86/dom0_build.c
> > > @@ -471,6 +471,24 @@ static void __init 
> > > process_dom0_ioports_disable(struct domain *dom0)
> > >       }
> > >   }
> > >
> > > +static void __hwdom_init setup_mmcfg(struct domain *d)
> > > +{
> > > +    unsigned int i;
> > > +    int rc;
> > > +
> > > +    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> > > +    {
> > > +        rc = register_vpci_mmcfg_handler(d, pci_mmcfg_config[i].address,
> > > +                                         
> > > pci_mmcfg_config[i].start_bus_number,
> > > +                                         
> > > pci_mmcfg_config[i].end_bus_number,
> > > +                                         
> > > pci_mmcfg_config[i].pci_segment);
> > > +        if ( rc )
> > > +            printk("Unable to setup MMCFG handler at %#lx for segment 
> > > %u\n",
> > > +                   pci_mmcfg_config[i].address,
> > > +                   pci_mmcfg_config[i].pci_segment);
> > > +    }
> > > +}
> > > +
> > >   int __init dom0_setup_permissions(struct domain *d)
> >
> > It could be i'm missing smth, but ^ function is __init while ...
> >
> > >   {
> > >       unsigned long mfn;
> > > @@ -480,6 +498,14 @@ int __init dom0_setup_permissions(struct domain *d)
> > >       if ( pv_shim )
> > >           return 0;
> > >
> > > +    /*
> > > +     * MMCFG initialization must be performed before setting domain
> > > +     * permissions, as the MCFG areas must not be part of the domain 
> > > IOMEM
> > > +     * accessible regions.
> > > +     */
> > > +    if ( is_hvm_domain(d) )
> > > +        setup_mmcfg(d);
> > > +
> > >       /* The hardware domain is initially permitted full I/O 
> > > capabilities. */
> > >       rc = ioports_permit_access(d, 0, 0xFFFF);
> > >       rc |= iomem_permit_access(d, 0UL,
> > > diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
> > > index 5551f9044836..6f47c9eeeaa6 100644
> > > --- a/xen/arch/x86/hvm/dom0_build.c
> > > +++ b/xen/arch/x86/hvm/dom0_build.c
> > > @@ -1310,24 +1310,6 @@ static int __init pvh_setup_acpi(struct domain *d, 
> > > paddr_t start_info)
> > >       return 0;
> > >   }
> > >
> > > -static void __hwdom_init pvh_setup_mmcfg(struct domain *d)
> > > -{
> > > -    unsigned int i;
> > > -    int rc;
> > > -
> > > -    for ( i = 0; i < pci_mmcfg_config_num; i++ )
> > > -    {
> > > -        rc = register_vpci_mmcfg_handler(d, pci_mmcfg_config[i].address,
> > > -                                         
> > > pci_mmcfg_config[i].start_bus_number,
> > > -                                         
> > > pci_mmcfg_config[i].end_bus_number,
> > > -                                         
> > > pci_mmcfg_config[i].pci_segment);
> > > -        if ( rc )
> > > -            printk("Unable to setup MMCFG handler at %#lx for segment 
> > > %u\n",
> > > -                   pci_mmcfg_config[i].address,
> > > -                   pci_mmcfg_config[i].pci_segment);
> > > -    }
> > > -}
> > > -
> > >   int __init dom0_construct_pvh(const struct boot_domain *bd)
> > >   {
> > >       paddr_t entry, start_info;
> > > @@ -1339,27 +1321,6 @@ int __init dom0_construct_pvh(const struct 
> > > boot_domain *bd)
> > >       if ( bd->kernel == NULL )
> > >           panic("Missing kernel boot module for %pd construction\n", d);
> > >
> > > -    if ( is_hardware_domain(d) )
> > > -    {
> > > -        /*
> > > -         * MMCFG initialization must be performed before setting domain
> > > -         * permissions, as the MCFG areas must not be part of the domain 
> > > IOMEM
> > > -         * accessible regions.
> > > -         */
> > > -        pvh_setup_mmcfg(d);
> > > -
> > > -        /*
> > > -         * 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.
> > > -         */
> > > -        rc = dom0_setup_permissions(d);
> > > -        if ( rc )
> > > -        {
> > > -            printk("%pd unable to setup permissions: %d\n", d, rc);
> > > -            return rc;
> > > -        }
> > > -    }
> > > -
> > >       /*
> > >        * Craft dom0 physical memory map and set the paging allocation. 
> > > This must
> > >        * be done before the iommu initializion, since iommu 
> > > initialization code
> > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > > index cb8ecd050d41..b7edb1d6555d 100644
> > > --- a/xen/arch/x86/hvm/hvm.c
> > > +++ b/xen/arch/x86/hvm/hvm.c
> > > @@ -35,6 +35,7 @@
> > >   #include <asm/hap.h>
> > >   #include <asm/current.h>
> > >   #include <asm/debugreg.h>
> > > +#include <asm/dom0_build.h>
> > >   #include <asm/e820.h>
> > >   #include <asm/regs.h>
> > >   #include <asm/cpufeature.h>
> > > @@ -651,6 +652,17 @@ int hvm_domain_initialise(struct domain *d,
> > >               goto fail1;
> > >           }
> > >           memset(d->arch.hvm.io_bitmap, ~0, HVM_IOBITMAP_SIZE);
> > > +
> > > +        /*
> > > +         * 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.
> > > +         */
> > > +        rc = dom0_setup_permissions(d);
> >
> > ... here hvm_domain_initialise() is not __init?
> 
> No, you're right, I missed this, thanks!
> 
> Good catch!

So addressing it will require some code movement, like
dom0_setup_permissions() outside of arch/x86/dom0_build.c...

> 
> >
> > [...]
> >
> >
> > --
> > Best regards,
> > -grygorii
> >
> 
>