Re: [PATCH v3 3/3] x86/dom0: be less restrictive with the Interrupt Address Range

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 20, 2025 at 05:05:44PM +0100, Jan Beulich wrote:
> On 20.02.2025 16:40, Roger Pau Monné wrote:
> > On Thu, Feb 20, 2025 at 02:30:38PM +0100, Jan Beulich wrote:
> >> On 20.02.2025 09:55, Roger Pau Monné wrote:
> >>> On Thu, Feb 20, 2025 at 09:33:46AM +0100, Jan Beulich wrote:
> >>>> On 19.02.2025 17:48, Roger Pau Monne wrote:
> >>>>> Note that the restriction to map the local APIC page is enforced
> >>>>> separately, and that continues to be present.  Additionally make sure 
> >>>>> the
> >>>>> emulated local APIC page is also not mapped, in case dom0 is using it.
> >>>>
> >>>> But that's in GFN space, not in MFN one. Why would that matter for 
> >>>> iomem_caps?
> >>>
> >>> It's required to avoid arch_iommu_hwdom_init() creating an identity
> >>> mapping for APIC_DEFAULT_PHYS_BASE, which would prevent the local APIC
> >>> emulation from being used.
> >>
> >> Hmm, yes, on one hand such a mapping would be created by default, as we
> >> default to "dom0-iommu=map-reserved". Otoh that mapping would be replaced
> >> before Dom0 is actually started, via the domain_creation_finished() hook.
> >> On (modern) VMX that is. So yes, on old VMX and on SVM the slot would need
> >> to remain unpopulated. Otoh, when the physical LAPICs are elsewhere and
> >> when the domain is in x2APIC mode, there would be no reason to disallow
> >> Dom0 access to that page.
> > 
> > Right, but that's now how dom0 is started ATM, as the local APIC is
> > unconditionally started in xAPIC mode and at APIC_DEFAULT_PHYS_BASE.
> > 
> > I could use vlapic_base_address() against vCPU#0 vlapic, but even in
> > guest_wrmsr_apic_base() we don't allow moving the local APIC MMIO
> > region, and hence I assumed it was fine to just use
> > APIC_DEFAULT_PHYS_BASE here.  Note in pvh_setup_acpi_madt() Xen also
> > hardcodes the local APIC address to APIC_DEFAULT_PHYS_BASE.
> > 
> > Would you be fine if I expand the comment so it's:
> > 
> >     /* If using an emulated local APIC make sure its MMIO is unpopulated. */
> >     if ( has_vlapic(d) )
> >     {
> >         /* Xen doesn't allow changing the local APIC MMIO window position. 
> > */
> >         mfn = paddr_to_pfn(APIC_DEFAULT_PHYS_BASE);
> >         rc |= iomem_deny_access(d, mfn, mfn);
> >     }
> 
> That will do, I think. Then:
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> >> That would apparently mean fiddling with
> >> iomem_caps once all vCPU-s have entered x2APIC mode.
> > 
> > Urg, that seems ugly.  It would also need undoing if the APICs are
> > reverted to xAPIC mode?
> 
> Right.
> 
> >> With LAPICs not
> >> normally being elsewhere, question is whether this corner case actually
> >> needs dealing with. Yet even if not, commentary may want extending, just
> >> to make clear the case was considered?
> > 
> > As said above, for both HVM and PVH Xen doesn't allow moving the APIC
> > MMIO window to anything different than APIC_DEFAULT_PHYS_BASE.
> 
> I was talking about the real one Xen uses.

Oh, OK, I was confused by the context I think, sorry then.

Roger.