On Thu, Jul 31, 2025 at 08:21:24PM +0000, dm...@proton.me wrote:
> On Thu, Jul 31, 2025 at 10:52:08PM +0300, Grygorii Strashko wrote:
> > Hi Denis,
> >
> > On 31.07.25 22:21, dm...@proton.me wrote:
> > > From: Denis Mukhin <dmuk...@ford.com>
> > >
> > > 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 <dmuk...@ford.com>
> > > ---
> > > 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
> >
>
>
