On Mon, May 31, 2021 at 09:21:25AM +0200, Jan Beulich wrote:
> On 28.05.2021 19:39, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -487,11 +487,12 @@ static int ept_invalidate_emt_range(struct p2m_domain
> > *p2m,
> > }
> >
> > int epte_get_entry_emt(struct domain *d, gfn_t gfn, mfn_t mfn,
> > - unsigned int order, bool *ipat, bool direct_mmio)
> > + unsigned int order, bool *ipat, p2m_type_t type)
> > {
> > int gmtrr_mtype, hmtrr_mtype;
> > struct vcpu *v = current;
> > unsigned long i;
> > + bool direct_mmio = type == p2m_mmio_direct;
>
> I don't think this variable is worthwhile to retain/introduce:
>
> > @@ -535,9 +536,33 @@ int epte_get_entry_emt(struct domain *d, gfn_t gfn,
> > mfn_t mfn,
> > }
> > }
> >
> > - if ( direct_mmio )
>
> With this gone, there's exactly one further use left. Preferably
> with this adjustment (which I'd be fine to make while committing, as
> long as you and/or the maintainers agree)
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
Thanks. That's fine, I was about to drop it but didn't want to introduce any
more changes than necessary.
>
> > + switch ( type )
> > + {
> > + case p2m_mmio_direct:
> > return MTRR_TYPE_UNCACHABLE;
>
> As a largely unrelated note: We really want to find a way to return
> WC here for e.g. the frame buffer of graphics cards, the more that
> hvm_get_mem_pinned_cacheattr() gets invoked only below from here
> (unlike at initial introduction of the function, where it was called
> ahead of the direct_mmio check, but still after the mfn_valid(), so
> the results were inconsistent anyway). Perhaps we should obtain the
> host MTRR setting for the page (or range) in question.
>
> As to hvm_get_mem_pinned_cacheattr(), XEN_DOMCTL_pin_mem_cacheattr
> is documented to be intended to be used on RAM only anyway ...
I also think we should make epte_get_entry_emt available to all p2m
code so it can partially replace the logic in p2m_type_to_flags to
account for cache attributes. I don't think there's much point in
keeping such different methods for accounting for cache attributes. I
know AMD lacks an ignore PAT equivalent, but there's no reason why p2m
cache attributes calculation should be done differently for AMD and
Intel AFAICT.
Roger.
