Re: [PATCH] x86/CPUID: unconditionally set XEN_HVM_CPUID_IOMMU_MAPPINGS

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

On Mon, Jan 18, 2021 at 12:05:12PM +0100, Jan Beulich wrote:
> On 15.01.2021 16:01, Roger Pau Monne wrote:
> > This is a revert of f5cfa0985673 plus a rework of the comment that
> > accompanies the setting of the flag so we don't forget why it needs to
> > be unconditionally set: it's indicating whether the version of Xen has
> > the original issue fixed and IOMMU entries are created for
> > grant/foreign maps.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Despite my earlier ack I came to think that the description and
> comment still don't make it quite clear what was actually wrong
> with the prior change, and hence they also don't really guard
> against the same getting done again (perhaps even by me). May I
> ask that you add a paragraph above and ...

What about adding:

"If the flag is only exposed when the IOMMU is enabled the guest could
resort to use bounce buffers when running backends as it would assume
the underlying Xen version still has the bug present and thus
grant/foreign maps cannot be used with devices."

To the commit log?

> > --- a/xen/arch/x86/traps.c
> > +++ b/xen/arch/x86/traps.c
> > @@ -1049,11 +1049,10 @@ void cpuid_hypervisor_leaves(const struct vcpu *v, 
> > uint32_t leaf,
> >              res->a |= XEN_HVM_CPUID_X2APIC_VIRT;
> >  
> >          /*
> > -         * Indicate that memory mapped from other domains (either grants or
> > -         * foreign pages) has valid IOMMU entries.
> > +         * Unconditionally set the flag to indicate this version of Xen has
> > +         * been fixed to create IOMMU mappings for grant/foreign maps.
> >           */
> > -        if ( is_iommu_enabled(d) )
> > -            res->a |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
> > +        res->a |= XEN_HVM_CPUID_IOMMU_MAPPINGS;
> 
> ... try to clarify the "Unconditionally" here?

I guess Unconditionally doesn't make much sense, so would be better to
start the sentence with 'Set ...' instead?

Thanks, Roger.