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

Re: [PATCH v2 3/3] AMD/IOMMU: iommu_enable vs iommu_intremap


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 2 Nov 2021 12:03:04 +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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=wB2TYvJTVIrRzT9/IGWTICwyTVqjUW4WAYZ9tFQpO7c=; b=nSQpWhHj0phxR4ax6iwmvFFZNUgQZOMJ6BOv/NAu7425GT25/CILxb/c5Lp+/TrHC44NIDlu5LypYS+XXmDPSSjmnh6UoCDHxKaeOkFScnNafN82OV6pGi872nKJ0kNMmYG++WWk/XWlcv/K/bBQjiKC/nuFZ0B+z9YZ+E8vI8XGVORXL57ZoRl2UqeMZa1cmRG05QPLYf5kCzUOeSfNzrYnkZJEMBfd4a34AnqJkVgsP1KtOs8eR2O1nV4bKsOHomQ+M2/+NRrBDiNArhmshctWIulE+SP58Ec6vPdrCS87LDnALuaIDPjZ8LK0dI5Gs5m2CEu+78UtdUT+wAzqPA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NDUqKlvWUMDO98TooSlsoLNYhoTMQiWfYh/i+rT1+RE0ky4nt9KmuueiKejUYwD3BnC1aqBS/G9jHf6kkNGrlFWm7JUgjlAA9vaBN9pbHY0q2W8+wlsg00tHBMS3nusQxGSuEOqZxxcQG+bMRnt6mlLSSfxw1JvHKW++St/EOQreSDcuvsThLJuzDKIo/nTIiYWmps1O//8PQgi1aH2azsTQ/EEnMLwMyICUU5X/Gu+qwFu/AikpBY3NOI29xixPR/A4usaQzHPbh2nP5kjJLilDVov+8lZc1y60S4Nwn/IlqDAfIayuy2wlfoJ7l9/KoTHhmcd60ET2ut6VPx0KDw==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Tue, 02 Nov 2021 11:03:36 +0000
  • Ironport-data: A9a23:6Jx6qq8HwERRnWkF5TKfDrUDbXmTJUtcMsCJ2f8bNWPcYEJGY0x3z GEZUW2FbvjbYWTxetB3Pt+08UgCusSEnIIyTlBvqiA8E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si9AttENlFEkvU2ybuOU5NXsZ2YhGmeIdA970Ug6wrdh2NYx6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPh8+ fwUjZPhZT5yEfTIgNs+UkFeDSxhaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcFg21t2JsXR54yY eI2RWU3MT/dbCdzMwYHVowEpqC1piPwJmgwRFW9+vNsvjm7IBZK+KfpGMrYfJqNX8o9tl6Ru 2bu72n/RBYAO7S3yzCI73atje/nhj7gVcQZE7jQ3u5nhhify3IeDDUSVECnur+ph0imQdVdJ kcIvC00osAPGFeDF4enGUfi+Tjd40BaC4E4//AGBB+lx5v9uj6WV0Q9FREYbMQZl85uQyIny Qrc9z/2PgBHvLqQQHOb076bqzKuJCQYRVM/iT84oRgtuIe6/txq5v7bZpM6SfPu0IWpcd3l6 2nS9HBWulkFsSIcO0xXF3jjiinkmJXGRxVdCu7/DjP8tVMRiGJIiuWVBbnnARRocNbxorqp5 iFsdy2iAAYmV8jleMulG71lIV1Rz6zZWAAweHY2d3Xbyxyj+mS4Yadb6yxkKUFiP64sIGGyP RCC41sOvsUKYBNGiJObharrUqzGKoC7TbzYug38NIISMvCdiifepEmCmnJ8L0iyyRNxwMnTy L+QcNq2DGZyNEiU5GHeegvp6pdynnpW7TqKHfjTlk37uZLDNC/9YepUazOmM7FmhJ5oVS2Iq r6zwePRkE4BOAA/CwGKmbMuwacidCNmWMup95QPHgNBSyI/cFwc5zbq6epJU6RunrhPl/eO+ Xe4W0RCz0H4i2GBIgKPAk2Popu0NXqmhX5kbyEqI3iy3H0vPdSm4KsFLsNldrg77u1zi/VzS qBdKcmHB/1OTBXB+igcMsah/NAzKkzziFLcJTehbRg+Y4VkG17D9Oj7c1a97yIJFCe265cz+ uXyygPBTJMfbA1+F8KKOum3xla8sCFFyuJ/VkfFOPdJf0Do/NQ4IiD9lKZvccoNNQ/C1n2R0 APPWUUUouzEookU9tjVhP/b89f1QrUmRkcDRjvV97e7MyXe71GP+44YXbbaZy3ZWUP15L6mO bdfwcbjPaBVh11NqYd9TepmlPps+9v1qrZG5Q14B3GXPU+zA7ZtL3Taj8lCsqpBmu1QtQesA x/d/9BbPfOCOd//EU5XLw0gN7zR2fYRkzjUzPI0PESlu3MnoOvZCR1fb0uWlShQDLppK4d0k +4utfkf5xG7lhd3YM2NiTpZ9jjUI3ENO0n9Wkr23GM/ZtIX92x/
  • Ironport-hdrordr: A9a23:W4OWx6B3NdS6Ew/lHegwsceALOsnbusQ8zAXPh9KJyC9I/b2qy nxppgmPH/P6Ar4WBkb6La90Y27MA7hHPlOkPUs1NaZLXPbUQ6TTb2KgrGSpgEIdxeOktK1kJ 0QDJSWa+eAfWSS7/yKmDVQeuxIqLLsndHK9IWuvEuFDzsaEJ2Ihz0JezpzeXcGPTWua6BJc6 Z1saF81kSdkDksH4mGL0hAe9KGi8zAlZrgbxJDLxk76DOWhTftzLLhCRCX0joXTjsKmN4ZgC f4uj28wp/mn+Cwyxfa2WOWx5NKmOH5wt8GIMCXkMAaJhjllw7tToV8XL+puiwzvYiUmRoXue iJhy1lE9V46nvXcG3wiRzx2zP42DJr0HPmwU/wuwqqneXJABYBT+ZRj4NQdRXUr2A6ustn7a 5N12WF87JKEBLphk3Glpj1fiAvsnDxjWspkOYVgXAae5AZcqVtoYsW+14QOIscHRj99JssHI BVfYDhDc5tABGnhk3izyxSKITGZAV2Iv7GeDlNhiWt6UkUoJgjpHFog/D2nR87hdsAotd/lq L5259T5cRzp/ktHNRA7dc6MLmK41P2MGbx2UKpUB/a/fI8SjjwQ6Ce2sRD2AjtQu1Q8KcP
  • Ironport-sdr: Hv/L4ZfG7Yv+m34r6NrKRvQUrkg/QS5Lj0cSIsk6Vqe/qHkcFKDHTjVkLA+L6tVrKj2fMNlB+Y S1cvtQ54HankoLbFguZi808BKM7GYi2r0rCEShqZNkGW3kkC3PoKm9J2iR68XKGAddhbGj30Uk 7rYl0pKwsEEiEzgZShlKpAFJVSBkMe4gF4/dnZ4FtCx4ur75mzDRYWm1KQzjuz4gQi7fF8MIoc w0R0OeZOAuLoVPmj5i1uYW/2P+Fpe4agYauNHxzAABLuy+jHNtz1/j/irtp8YDELigyH8ADTUR 1W4XT5jQ03KyGzmhaNz93SnK
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Tue, Nov 02, 2021 at 11:13:08AM +0100, Jan Beulich wrote:
> On 25.10.2021 12:28, Roger Pau Monné wrote:
> > On Thu, Oct 21, 2021 at 11:59:02AM +0200, Jan Beulich wrote:
> >> The two are really meant to be independent settings; iov_supports_xt()
> >> using || instead of && was simply wrong. The corrected check is,
> >> however, redundant, just like the (correct) one in iov_detect(): These
> >> hook functions are unreachable without acpi_ivrs_init() installing the
> >> iommu_init_ops pointer, which it does only upon success. (Unlike for
> >> VT-d there is no late clearing of iommu_enable due to quirks, and any
> >> possible clearing of iommu_intremap happens only after iov_supports_xt()
> >> has run.)
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >> ---
> >> In fact in iov_detect() it could be iommu_enable alone which gets
> >> checked, but this felt overly aggressive to me. Instead I'm getting the
> >> impression that the function may wrongly not get called when "iommu=off"
> >> but interrupt remapping is in use: We'd not get the interrupt handler
> >> installed, and hence interrupt remapping related events would never get
> >> reported. (Same on VT-d, FTAOD.)
> > 
> > I've spend a non-trivial amount of time looking into this before
> > reading this note. AFAICT you could set iommu=off and still get x2APIC
> > enabled and relying on interrupt remapping.
> 
> Right, contrary to ...
> 
> >> For iov_supports_xt() the question is whether, like VT-d's
> >> intel_iommu_supports_eim(), it shouldn't rather check iommu_intremap
> >> alone (in which case it would need to remain a check rather than getting
> >> converted to ASSERT()).
> > 
> > Hm, no, I don't think so. I think iommu_enable should take precedence
> > over iommu_intremap, and having iommu_enable == false should force
> > interrupt remapping to be reported as disabled. Note that disabling it
> > in iommu_setup is too late, as the APIC initialization will have
> > already taken place.
> > 
> > It's my reading of the command line parameter documentation that
> > setting iommu=off should disable all usage of the IOMMU, and that
> > includes the interrupt remapping support (ie: a user should not need
> > to set iommu=off,no-intremap)
> 
> ... that documentation. But I think it's the documentation that
> wants fixing, such that iommu=off really only control DMA remap.

IMO I think it's confusing to have sub-options that could be enabled
when you set the global one to off. I would expect `iommu=off` to
disable all the iommu related options, and I think it's fair for
people to expect that behavior.

I'm unsure whether it's fair to change the documentation now, we
should instead fix the code, so that people using `iommu=off` get the
expected behavior. Then we would likely need to introduce a way to
disable just dma remapping (dmaremap, similar to intremap). That
would make a much better and saner interface IMO.

Thanks, Roger.



 


Rackspace

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