Re: [PATCH v4] VMX: use a single, global APIC access page

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Apr 13, 2021 at 11:24:09AM +0200, Jan Beulich wrote:
> On 12.04.2021 17:31, Roger Pau Monné wrote:
> > On Mon, Apr 12, 2021 at 12:40:48PM +0200, Jan Beulich wrote:
> >> The address of this page is used by the CPU only to recognize when to
> >> access the virtual APIC page instead. No accesses would ever go to this
> >> page. It only needs to be present in the (CPU) page tables so that
> >> address translation will produce its address as result for respective
> >> accesses.
> >>
> >> By making this page global, we also eliminate the need to refcount it,
> >> or to assign it to any domain in the first place.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> >> ---
> >> v4: Set PGC_extra on the page. Make shadow mode work.
> >> v3: Split p2m insertion change to a separate patch.
> >> v2: Avoid insertion when !has_vlapic(). Split off change to
> >>     p2m_get_iommu_flags().
> >> ---
> >> I did further consider not allocating any real page at all, but just
> >> using the address of some unpopulated space (which would require
> >> announcing this page as reserved to Dom0, so it wouldn't put any PCI
> >> MMIO BARs there). But I thought this would be too controversial, because
> >> of the possible risks associated with this.
> > 
> > Really seems more trouble than reward. Also there are systems with
> > MMIO regions in holes on the memory map, like the issue I had with the
> > Intel pinctrl stuff that had an MMIO region in a hole on the memory
> > map [0], so I'm not sure Xen would be in a position to select a
> > suitable unpopulated page anyway.
> > 
> > [0] https://lore.kernel.org/xen-devel/YFx80wYt%2FKcHanC7@smile.fi.intel.com/
> 
> Yeah, I had seen that. What I'm having trouble to understand is how the
> OS will know to avoid that range for e.g. placing BARs.

No idea really, I assume they expect that you parse all possible ACPI
devices from the dynamic tables before deciding on any BAR placements?

I still consider a bug that the pinctrl MMIO region is not marked as
reserved in the memory map.

> >> +    {
> >> +        const struct page_info *pg = mfn_to_page(mfn);
> >> +
> >> +        if ( !page_get_owner(pg) && (pg->count_info & PGC_extra) )
> >> +        {
> >> +            ASSERT(type == p2m_mmio_direct);
> >> +            return 0;
> > 
> > Are there any other pages that could pass this check? I don't think
> > so, but wanted to assert.
> 
> "Normal" extra pages have an owner, so no, there aren't any others.
> If and when any appear, this may need further customizing, albeit
> generally I'd hope further pages matching this pattern would also
> want similar treatment.

I wonder whether we want to add an assert here to make sure only the
APIC access page receives this special handling by the shadow code,
but maybe that's a bit too much?

Thanks, Roger.