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
