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

Re: [PATCH v6 02/12] IOMMU/x86: new command line option to suppress use of superpage mappings


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 28 Jun 2022 14:52:13 +0200
  • 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=ugGCxt2Oi3lZMs2xzCtuqrxzegRH1SjXuinG3eg9hsw=; b=XIvDXK2YS0dHFlZp/H6p7AmZz88oYHvZnZM/FiAx198N1dbwPSkNMsDcOkOdFeZBLkq7vbbEMJlIUs4a+59FBPDD1xqyR4F8TP1F3JElRMIEPGFgBy6XyTDVbA4ChWTj1dTKP5KmveeTsCPFDg+LxNon+DSQqQ+fOvgWFgPouKZf1mlQwvhKDGQdFOZ3QMXJPKaUwHpDBr0oDNsxErI6IQPMOHiVVTmrcosBgIt8vpddnFXNpbXaJwvnJ4PIgVqjYW6zHU0QAYPhg23GkNR6FY86xDgVBN7/NwEi/JwBmolHbuJ8fDC5pYe6dPPgV1rYax8Ni+2AE9N6q56EdfeBqA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CRkvZ50ISYiIdN4DQ8l7dNx5ugdvYs/xYqFd9PvJ8bH4fyzncJlFbIKA6c4sOIc3L+wv7FjhxirERlRuGxsPURTNtXoAshQ6DY0Ghn7WjuCTcmq4YR1STw6x7DbRrk9JJ2v7udZuoF4oorjlYZZQVo6PVH/WN47cYGIteG101G093j/4uZ8GktokaWWfYwl2UJLShFH4FmD4lzqDXhB9SOJdY3WUI6pxAAY5Wl64AcyV7N3iGPZuO0h/NAG+e+cy1fhQUiAHXjdSIV0n71xTlA9PO+eFVZbLLRDQvKXlgcsCJ6N5zVJNFeqCViPma96blTA94j7S7WhEcGFsw7qRRQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>, Wei Liu <wl@xxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>
  • Delivery-date: Tue, 28 Jun 2022 12:52:46 +0000
  • Ironport-data: A9a23:6JBunqqrrvLo2tSEU99zO3ptqQVeBmIIZBIvgKrLsJaIsI4StFCzt garIBnSPquIMDCkKNpzbdi/9E0O6p/Wx9BrTQE/rH9kRiwap5uZCYyVIHmrMnLJJKUvbq7GA +byyDXkBJppJpMJjk71atANlVEliefQAOCU5NfsYkidfyc9IMsaoU8lyrRRbrJA24DjWVvT4 4Oq+qUzBXf+s9JKGjNMg068gEsHUMTa4Fv0aXRnOJinFHeH/5UkJMp3yZOZdhMUcaENdgKOf M7RzanRw4/s10xF5uVJMFrMWhZirrb6ZWBig5fNMkSoqkAqSicais7XOBeAAKv+Zvrgc91Zk b1wWZKMpQgBOr/DwfoDVChjCTxME6xjwJTWcEeOvpnGp6HGWyOEL/RGKmgTZNdd0MAnRGZE+ LofNSwHaQ2Fi6Su2rWnR+Jwh8Mlas72IIcYvXImxjbcZRokacmbH+OWupkFgXFp2JAm8fX2P qL1bRJ1axvNeVtXM0o/A5Mihua4wHL4dlW0rXrK+vNqvzSDnWSd1pDTaYHfUdqMff4Nj2eCi k/HuEnFLQEzYYn3JT2ttyjEavX0tSHxVZ8WFba43uV3m1DVzWsWYDUGWF3+rfSnh0qWX9NEN 1dS6icotbI19kGgUp/6RRLQiGaNoxo0S9dWVeog52ml1a788wufQG8eQVZpa9E4tclwWT0j0 HeImc/kAXpkt7j9dJ6G3rKdrDf3My5FK2YHPHUAVVFcvYmlp5wvhBXSSNolCLSyktD+BTD3x XaNsTQ6gLIQy8UM0s1X4Gz6vt5lnbCRJiZd2+kddjnNAt9RDGJ9W7GV1A==
  • Ironport-hdrordr: A9a23:OHyt1aiPFFR4/jb4WKH7feEqy3BQX0h13DAbv31ZSRFFG/FwyP rCoB1L73XJYWgqM03I+eruBEBPewK4yXdQ2/hoAV7EZnichILIFvAa0WKG+VHd8kLFltK1uZ 0QEJSWTeeAd2SS7vyKnzVQcexQp+VvmZrA7Ym+854ud3ANV0gJ1XYENu/xKDwTeOApP+taKH LKjfA32gZINE5nJ/hSQRI+Lpv+juyOsKijTQ8NBhYh5gXLpTS06ITiGxzd+hsFSTtAzZor7G CAymXCl+6emsD+7iWZ+37Y7pxQltek4txfBPaUgsxQDjn3kA6naKloRrXHljEop+OE7kosjb D30l8dFvU2z0mUUnC+oBPr1QWl+DEy60X6wVvdunfnqdyRfkNPN+NxwaZiNjfJ4Uspu99xlI hR2XiCipZRBRTc2Azg+tnhTXhR5wWJiEtntdRWo21UUIMYZrMUh5cY5llpHJAJGz+/wJw7Ed NpENrX6J9tAB+nhkjizyhSKeGXLzQO9k/seDlAhiXV6UkaoJlB9TpX+CRF9U1wtq7USPF/lp H52+pT5fRzp/QtHNNA7dc6MLWK41P2MGLx2UKpUCPa/fI8SgTwgq+yxokJz8eXX7FN5KcOuf 36ISFlXCgJCgjTNfE=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Jun 09, 2022 at 12:17:23PM +0200, Jan Beulich wrote:
> Before actually enabling their use, provide a means to suppress it in
> case of problems. Note that using the option can also affect the sharing
> of page tables in the VT-d / EPT combination: If EPT would use large
> page mappings but the option is in effect, page table sharing would be
> suppressed (to properly fulfill the admin request).
> 
> Requested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v6: New.
> 
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1405,7 +1405,7 @@ detection of systems known to misbehave
>  
>  ### iommu
>      = List of [ <bool>, verbose, debug, force, required, 
> quarantine[=scratch-page],
> -                sharept, intremap, intpost, crash-disable,
> +                sharept, superpages, intremap, intpost, crash-disable,
>                  snoop, qinval, igfx, amd-iommu-perdev-intremap,
>                  dom0-{passthrough,strict} ]
>  
> @@ -1481,6 +1481,12 @@ boolean (e.g. `iommu=no`) can override t
>  
>      This option is ignored on ARM, and the pagetables are always shared.
>  
> +*   The `superpages` boolean controls whether superpage mappings may be used
> +    in IOMMU page tables.  If using this option is necessary to fix an issue,
> +    please report a bug.
> +
> +    This option is only valid on x86.
> +
>  *   The `intremap` boolean controls the Interrupt Remapping sub-feature, and
>      is active by default on compatible hardware.  On x86 systems, the first
>      generation of IOMMUs only supported DMA remapping, and Interrupt 
> Remapping
> --- a/xen/arch/x86/include/asm/iommu.h
> +++ b/xen/arch/x86/include/asm/iommu.h
> @@ -132,7 +132,7 @@ extern bool untrusted_msi;
>  int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq,
>                     const uint8_t gvec);
>  
> -extern bool iommu_non_coherent;
> +extern bool iommu_non_coherent, iommu_superpages;
>  
>  static inline void iommu_sync_cache(const void *addr, unsigned int size)
>  {
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -88,6 +88,8 @@ static int __init cf_check parse_iommu_p
>              iommu_igfx = val;
>          else if ( (val = parse_boolean("qinval", s, ss)) >= 0 )
>              iommu_qinval = val;
> +        else if ( (val = parse_boolean("superpages", s, ss)) >= 0 )
> +            iommu_superpages = val;
>  #endif
>          else if ( (val = parse_boolean("verbose", s, ss)) >= 0 )
>              iommu_verbose = val;
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2213,7 +2213,8 @@ static bool __init vtd_ept_page_compatib
>      if ( rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP, ept_cap) != 0 ) 
>          return false;
>  
> -    return (ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) &&
> +    return iommu_superpages &&
> +           (ept_has_2mb(ept_cap) && opt_hap_2mb) <= cap_sps_2mb(vtd_cap) &&
>             (ept_has_1gb(ept_cap) && opt_hap_1gb) <= cap_sps_1gb(vtd_cap);

Isn't this too strict in requesting iommu superpages to be enabled
regardless of whether EPT has superpage support?

Shouldn't this instead be:

return iommu_superpages ? ((ept_has_2mb(ept_cap) && opt_hap_2mb) <= 
cap_sps_2mb(vtd_cap) &&
                           (ept_has_1gb(ept_cap) && opt_hap_1gb) <= 
cap_sps_1gb(vtd_cap))
                        : !((ept_has_2mb(ept_cap) && opt_hap_2mb) ||
                            (ept_has_1gb(ept_cap) && opt_hap_1gb));

I think we want to introduce some local variables to store EPT
superpage availability, as the lines are too long.

The rest LGTM.

Thanks, Roger.



 


Rackspace

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