[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/CPUID: unconditionally set XEN_HVM_CPUID_IOMMU_MAPPINGS
On 19.01.2021 12:40, Roger Pau Monné wrote: > 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. I'd be okay with the latest version, but I think Andrew's list is more clear, so I'd prefer that one. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |