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.
