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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.01.2021 16:54, Roger Pau Monné wrote:
> 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?

SGTM.

>>> --- 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?

Hmm, this would further move us away from the goal of the comment
making sufficiently clear that a conditional shouldn't be (re-)
introduced here, I would think. Since I can't seem to think of a
good way to express this more briefly than in the description,
and if maybe you can't either, perhaps there's no choice then to
leave it as is, hoping that people would look at the commit before
proposing a further change here.

Jan