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.
