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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.