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.
