[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:40:51 +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=rj810LY3epO6SYOKGHfY0262DTSZFEwi2xkUCFlkfcY=; b=am/kreHO48HTxbw2L0bi9YRZJmERG4KIAkiQpUH/mg00FK5kJOgk5hwTev2pVePWZ8RWnN4Q/dkRT5qMJ0JSrWtgfpkq56FypAOdRrwO8H+i+lPbcTFggfqepNTBnkfhf9ZZ047+TiDmIss71Ll6kvH1uhn42mQK21a961HS2co8hYjHvWQfE1zmq6X35BUcHY/Wqp4G188xn8SewS2Q3Zoa7VPQwysyKvzyqjhLSc9fZToUy5yF12/1oLir757ZnZ6tag90PQHooLnnE8FPEFwAbHLjoUydT29xHg7E7fHnACg797BtClduP69kl9rTmWStW3gqGlPm3ZiN+gqZ2w==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FutnV/6dKW2/K/cAgCtr3j8Gkm9iETdyFMyq3v7JU5IWzTZDRLW+QWgcny4QvOawWeIiMgHASpx3QL9jQY37470TW7vwkODwMwbiY4wrlyBIZ6AcIJ8YCu1v3YbD8+buR5418k+fl4SgBmJnepNGsBu+jU4H0PFNC7p72at9ekk3rRxT2EFDD5P82XLpIe5kK31ObjxZ4zDtF6rnwyoZy6xdH4si/DZij+rsip8i0E58kJ0NNXYq2gyEIH7kITHtrm9tA2JwsH9xGMwcb0FLc00VOhsY+iGeAhvXuTsXv0PFvHWXvVEKonKlb2lYB8OMR3alnIhy91Xu+RNCLFv5LA==
  • 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:41:06 +0000
  • Ironport-sdr: 1eFZN7hLzXmR5VNUtFSDzCMtsQZAi20BoCUA7vF6jdz9qu7Kyqag2xkbIQ2oQmDU3Gyv/QiBDh UtFYHLR6xRR8M7ASq+8qJE8N0woACLXWqe9EXnZ1xbB4y0ACWDpEjE0Utctsh46nRfC7jwiZAq 6LqVvwCdEBH+5jNLfMML2iS4DlfHwIRkewCZwJByY3a/QsHOofSPvqZe21AKFuy4GBSzJwHLN1 ZIYblRLM/tyoY2gPA93cg6Z4wYqbcDiXKnjxSvtZXF2MgUPTsYosrd6rOaiEnP5Gmxrv8ieiIx D04=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

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