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

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 19 Jan 2021 12:14:06 +0100
  • 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=KpnD95Z9UanBglA9Vp4cyfe5s2EpjixZu+W9ehU9258=; b=P4u833Ev5JYONVszQn2U9x29fDbBQprUjaDt5kYevCZcuip4i7+JxJh0wxus86E5srA/6GQUZAoSfWGRcGuPjBc321o4F7ZVntsthbWD971DuKt/GUdbLz/jmuXH/H7vSZ/tIYgt9btZLhpAxLferJ9z6HrgpYdR+llK19V8Syv/CfO/KzrYw3Ngrbhxe92bKYSLLhy8doAli+8r3oY171zUgAmx61TIIbXvtLnkCL3pmI+nA1N0P91k/MKc2A+yalwmSnQP7EwzrpQKROiW48CmpmPbhg+yIKcvtdw3eI+sXcNyG7OXJ/boaBCzQu6l4Q7/CY7QAIqx7f+Rgp/yzA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=broK9tKjvmly0hw8WszdAXOotXj5pxIAiFNpLCxZ2lfW8CvgF55BK/TUtnBQinCcxojxI8G44ToW02ZY9v0TwtaJaBXZH3bPMveKT3ranaq/l5zfuu49ZTGvNYHcQP+NQ9ZNpv7LblGzydc+n5/1sqFuMnA8DKjYqH3fb5+xbkrGriOGGFzurClfDOrO1VlPl02SqJHP7C2KWLt6G3DwatNhQJfXIAmyy2D1uMJO9Kf6q4zoQ0R5B/6I9+WdzNyslfXOyW/IDpO5Mdw1keMFlBO9QpbKOg3moKm5u8FYJVux+Da2kgfsLTro5B6It7ghQTljrvYheFqHeJhdNDWouQ==
  • Authentication-results: esa2.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:14:38 +0000
  • Ironport-sdr: EgdGM4Cz3f2sxTFqlROAHrDGjyLAu7anSAvfpebCnZQSfwg8NMbIN8J7kyMLY249I6g8Co3JHG QfM0htV6d9Oq5+uKkmbmBUlHOiUCuXm1bJHf2Vv7jXmGQl0k7+EY9C8iMBYZo24yEPhvRdSKbQ mp7GIJxJlC2FUi2ZQwaLsFCwuzh8OhCrSiurp7YM7tsS7KH/o3ZtntqKisoPBeH4PbVZIuWO9/ IyOoEuJJTXqkS3c6qFGRT2mfy6SAWVMZ6DUUsuru37zO/+Smjb9L6+Z305G4mOV4jb/+4F+3nm 2gk=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

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