[PATCH] x86/CPUID: unconditionally set XEN_HVM_CPUID_IOMMU_MAPPINGS

[PATCH] x86/CPUID: unconditionally set XEN_HVM_CPUID_IOMMU_MAPPINGS

[Roger Pau Monne]

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@citrix.com>
---
 xen/arch/x86/traps.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 4bd2cb6a1a..d491b1741e 100644
--- 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;
 
         /* Indicate presence of vcpu id and set it in ebx */
         res->a |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
-- 
2.29.2

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

[Jan Beulich]

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@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

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

[Jan Beulich]

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@citrix.com>

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

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

Jan

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

[Roger Pau Monné]

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@citrix.com>
> 
> 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.

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

[Jan Beulich]

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@citrix.com>
>>
>> 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

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

[Roger Pau Monné]

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).

Thanks, Roger.

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

[Andrew Cooper]

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

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

[Roger Pau Monné]

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?

Thanks, Roger.

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

[Andrew Cooper]

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.

~Andrew

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

[Roger Pau Monné]

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.

Thanks, Roger.

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

[Jan Beulich]

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