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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/01/2021 17:10, Roger Pau Monné wrote:
> On Mon, Jan 18, 2021 at 05:04:19PM +0100, Jan Beulich wrote:
>> 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:
>>>>> --- 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.
> /*
>  * Unconditionally set the flag to indicate this version of Xen has
>  * been fixed to create IOMMU mappings for grant/foreign maps.
>  *
>  * NB: this flag shouldn't be made conditional on IOMMU presence, as
>  * it could force guests to resort to using bounce buffers when using
>  * grant/foreign maps with devices.
>  */
>
> Would be better? (albeit too verbose maybe).

The comment should be rather more direct.

1) Xen 4.10 and older was broken WRT grant maps requesting a DMA
mapping, and forgot to honour the guest's request.
2) 4.11 (and presumably backports) fixed the bug, so the map hypercall
actually did what the guest asked.
3) To work around the bug, guests must bounce buffer all DMA, because it
doesn't know whether the DMA is originating from an emulated or a real
device.
4) This flag tells guests it is safe not to bounce-buffer all DMA to
work around the bug.

~Andrew