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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.01.2021 12:40, Roger Pau Monné wrote:
> On Tue, Jan 19, 2021 at 11:16:06AM +0000, Andrew Cooper wrote:
>> On 19/01/2021 11:14, Roger Pau Monné wrote:
>>> On Mon, Jan 18, 2021 at 05:48:37PM +0000, Andrew Cooper wrote:
>>>> 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.
>>> /*
>>>  * Old versions of Xen are broken when creating grant/foreign maps,
>>>  * and will never create IOMMU entries for such mappings. This was
>>>  * fixed in later versions of Xen, but guests wanting to work on
>>>  * unpatched versions will need to use a bounce buffer in order to
>>>  * avoid sending grant/foreign maps to devices. Whether such bounce
>>>  * buffer mechanism is not needed is indicated by the presence of the
>>>  * following CPUID flag.
>>>  */
>>>
>>> Does that seem better?
>>
>> Better, but the key point is that all DMA, emulated or real, needs
>> bounce buffering because the guest kernel doesn't know the difference. 
>> *This* is why the flag needs to be always present, because otherwise a
>> guest will bounce buffer DMA for emulated devices.
> 
> I think this is clear from the text as I explicitly say 'devices'
> (not emulated or passed through devices).
> 
> /*
>  * Old versions of Xen are broken when creating grant/foreign maps,
>  * and will never create IOMMU entries for such mappings. This was
>  * fixed in later versions of Xen, but guests wanting to work on
>  * unpatched versions will need to use a bounce buffer in order to
>  * avoid sending grant/foreign maps to devices. Whether such bounce
>  * buffer mechanism is not needed is indicated by the presence of the
>  * following CPUID flag. Not exposing the flag will force guests to
>  * fallback to bounce buffering when sending grant/foreign maps to any
>  * device.
>  */
> 
> If not I don't mind just using the list that you provided if Jan
> agrees.

I'd be okay with the latest version, but I think Andrew's list is
more clear, so I'd prefer that one.

Jan