[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/CPUID: unconditionally set XEN_HVM_CPUID_IOMMU_MAPPINGS


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 19 Jan 2021 11:16:06 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=K5NGH41KdJcRsc7zPwSo5vMWCwAHnm2+xzCNgZvW4XY=; b=ZE+q92tUS1J8u77IBngRkNng8FQZ6lCvuoSWOmeTV4zkRNDKEd/UeW0LYNIk/jZeYkGoDmJMFgQv5YkMY9G7ZbQe8PFeL+5p5d3c6sMGpIAbZ8S0SCBjAjEM84sJtGYslkVmBOjA2dNwImyP9LR30+0q3QJlHu37mnG9Nn36WujhQfzm4rZc01p+MFXZ/BXxBgI+gSVbPiI7qFiovTCVhcP/uLAVp89hnZu8z2w/uHBTH67HKi0Z7VAni0bDjy7e82V9aR8ie18u3aIXyW7X5kqnUpH7ZQJMWPT0pZVF93JAdWv8grsux+sp7QUWOk2qJmQKKaYFU1CwFqs9FsFWkg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TeecZC/B98A1hFtEoFAmyldhfu/ppDll/Wj7MMiGslfQ51jNuN8u42XNulQFAnwJyxp/7toQCCJtEnAUF4oqRwH7s1RP2iak0WPPVaTobsIoIuA8FBS+6a5zE7hVftf/g7ghusnQ7w9m0DaTtBx3FoM2W6ps/5SSzIa+kdVDHZneC2N/9n+1dNtfg8cOqzwt8JHpz56zyi2yhi9RIeAqmwlLqxXO+3m4yME8558bpCjGsN48RFa57Wr7XW8GLnGZL/hcN/fj8+Ja8D6haeIRTIbLwoSxK1LdMZKjcE8fGasWX5tiirJXWG13P6P7ACeR9loUCifHKHpMv9+1Ffg/fw==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 19 Jan 2021 11:16:31 +0000
  • Ironport-sdr: m3b6varXolqI1iNWmWdvMnkX2EZ4MJpMKZT0tbvuOlaeNAO/JdfF4C0FIuMozk8OpBZEIzkKVl 7k6JOk6bgMal2yMwg5IW8Kh0tIVIi+D8KHBxsUk3IkL2aoHKrZaLXBJjYgvdrD7TF4bdZHr8yb Fy0/qvoqWccn4gFnMEDhskT14QwB2EdylLayh/nalMD0wMN6OcH3MLh+fB+iNO0Ex1FMubOqmw URi0MVY4eg8QRtd/dJjVC88y9ko3Zq88Wli3hkdKg1ZYTjPctfKWAOEoLRpeFseHxilEI0+lH7 +dQ=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.