Re: [PATCH] x86/CPUID: suppress IOMMU related hypervisor leaf data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2021 16:45, Roger Pau Monné wrote:
> On Mon, Jan 04, 2021 at 04:16:18PM +0100, Jan Beulich wrote:
>> On 04.01.2021 15:56, Roger Pau Monné wrote:
>>> On Mon, Jan 04, 2021 at 02:43:52PM +0100, Jan Beulich wrote:
>>>> On 28.12.2020 11:54, Roger Pau Monné wrote:
>>>>> On Mon, Nov 09, 2020 at 11:54:09AM +0100, Jan Beulich wrote:
>>>>>> Now that the IOMMU for guests can't be enabled "on demand" anymore,
>>>>>> there's also no reason to expose the related CPUID bit "just in case".
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>
>>>>> I'm not sure this is helpful from a guest PoV.
>>>>>
>>>>> How does the guest know whether it has pass through devices, and thus
>>>>> whether it needs to check if this flag is present or not in order to
>>>>> safely pass foreign mapped pages (or grants) to the underlying devices?
>>>>>
>>>>> Ie: prior to this change I would just check whether the flag is
>>>>> present in CPUID to know whether FreeBSD needs to use a bounce buffer
>>>>> in blkback and netback when running as a domU. If this is now
>>>>> conditionally set only when the IOMMU is enabled for the guest I
>>>>> also need to figure a way to know whether the domU has any passed
>>>>> through device or not, which doesn't seem trivial.
>>>>
>>>> I'm afraid I don't understand your concern and/or description of
>>>> the scenario. Prior to the change, the bit was set unconditionally.
>>>> To me, _that_ was making the bit useless - no point in checking
>>>> something which is always set anyway (leaving aside old Xen
>>>> versions).
>>>
>>> This bit was used to differentiate between versions of Xen that don't
>>> create IOMMU mappings for grants/foreign maps (and so are broken) vs
>>> versions of Xen that do create such mappings. If the bit is not set
>>> HVM domains need a bounce buffer in blkback/netback in order to avoid
>>> sending grants to devices.
>>
>> Neither the comment in cpuid.h nor that in traps.c have any mention
>> of this, and the constant's name also doesn't imply such.
>>
>>> Now it's my understand that with this change sometimes the bit might
>>> not be set not because we are running on an unfixed Xen version, but
>>> because there's no IOMMU assigned to the domain, so the guest will
>>> fallback to use a bounce buffer.
>>
>> ... if it expects to ever map a foreign domain's pages.
>>
>> I can see that reverting the change is one way to address the issue.
>> Such a revert shouldn't be a plain one then, but one adjusting one
>> or both of the the comments to indicate the _real_ purpose of this
>> flag. (We probably better don't rename the constant, as we can't
>> easily drop the old name from the public interface anyway.)
> 
> I'm happy to send the revert, but do you have any suggestion about the
> fixed comments?
> 
> Maybe adding something like:
> 
> /*
>  * Unditionally set the flag to notice this version of Xen has been
>  * fixed to create IOMMU mappings for grant/foreign maps.
>  */

Sounds reasonable. I assume you mean "Unconditionally". And my
English isn't sufficient to tell whether "notice" is suitable
here; I would have used e.g. "indicate".

Jan