Re: [PATCH] x86/hvm: set 'ipat' in EPT for special pages

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2020 12:46, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
> 
> All non-MMIO ranges (i.e those not mapping real device MMIO regions) that
> map valid MFNs are normally marked MTRR_TYPE_WRBACK and 'ipat' is set. Hence
> when PV drivers running in a guest populate the BAR space of the Xen Platform
> PCI Device with pages such as the Shared Info page or Grant Table pages,
> accesses to these pages will be cachable.
> 
> However, should IOMMU mappings be enabled be enabled for the guest then these
> accesses become uncachable. This has a substantial negative effect on I/O
> throughput of PV devices. Arguably PV drivers should bot be using BAR space to
> host the Shared Info and Grant Table pages but it is currently commonplace for
> them to do this and so this problem needs mitigation. Hence this patch makes
> sure the 'ipat' bit is set for any special page regardless of where in GFN
> space it is mapped.
> 
> NOTE: Clearly this mitigation only applies to Intel EPT. It is not obvious
>       that there is any similar mitigation possible for AMD NPT. Downstreams
>       such as Citrix XenServer have been carrying a patch similar to this for
>       several releases though.
> 
> Signed-off-by: Paul Durrant <pdurrant@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

However, ...

> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -830,7 +830,8 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>          return MTRR_TYPE_UNCACHABLE;
>      }
>  
> -    if ( !is_iommu_enabled(d) && !cache_flush_permitted(d) )
> +    if ( (!is_iommu_enabled(d) && !cache_flush_permitted(d)) ||
> +         is_special_page(mfn_to_page(mfn)) )
>      {
>          *ipat = 1;
>          return MTRR_TYPE_WRBACK;

... shouldn't we leverages this (right away?) to do away with the
APIC access page special case a few lines up from here? It is my
understanding that vmx_alloc_vlapic_mapping() uses
set_mmio_p2m_entry() just in order to get ipat set on the resulting
EPT entry. Yet with the allocation using MEMF_no_refcount, this will
now be the result even if no artificial MMIO mapping was created.

Jan