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

RE: [PATCH 2/2] VT-d: Tylersburg isoch DMAR unit with no TLB space


  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: "Tian, Kevin" <kevin.tian@xxxxxxxxx>
  • Date: Tue, 12 Oct 2021 04:40:07 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.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=cCFKs9erdRcacidxxprgWQWJ+ULIZ4pcgNIxefaRizA=; b=GORpbdqm43rFEtd0Qrv9cSHID6fDD+gJXj6g0Aya8Blu/YgUL/VzZkpJW+fhsRoe86gnbJYn68BsB+hAlQO94532V5xgtUPi8V/qWlTIX2L46jMUlddGBKFeyF0DPAXw0pk8rvT5HeADdmni8OVIanuQ5aEBdDh65C+zd84N8nXB5y7DQ73DsfOl/vXhziG9phM6PCp81oSgqkr3TnDgo1gaM2FH3AVbgb87FHij5DgRtVH6OYVjjErMnixXjfRoZm58kvTzMCkmOf/5k3W1by0CwDRYMRr/zdnquB2Tj0m0yZRbIwHowPClcly/k27Ld0dZqXTYdljJNZHuhRT3rQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=UxKrZia/TlW9aexlPblbb/Xq6NtzR1TadVkbr9Y8647Lg8t2UOG/pwvvm/uzFQY2t06f6jkyZj91x9YygPc+AxDPxD/KZjWGglQlqq/loAzvA/xqg9SZz+G3FFkuC++tuRomuCD+0x22NJKenwFJIaAMSKGgvpSTDHC+ZtwrI2dM3q6ST7GvKv7N5t9UNpC8nM8yhywLz/LvvBlCq8P7ruce0Gm1466ajhHe1xn6KyRuOObsytIFd3NT7bXboATdPycPpzHntyhFlPr0FOXCXI+/+y28lgWMrwDeN9SaH0XjcVhkNNu4HG9REMeEWkXpmqW6MY08P5YVaC59lMtrng==
  • Authentication-results: suse.com; dkim=none (message not signed) header.d=none;suse.com; dmarc=none action=none header.from=intel.com;
  • Cc: Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Tue, 12 Oct 2021 04:40:29 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHXvnz7yqUSecH0tE2jO7udKupRsKvOx/6Q
  • Thread-topic: [PATCH 2/2] VT-d: Tylersburg isoch DMAR unit with no TLB space

> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Monday, October 11, 2021 4:50 PM
> 
> BIOSes, when enabling the dedicated DMAR unit for the sound device,
> need to also set a non-zero number of TLB entries in a respective
> system management register (VTISOCHCTRL). At least one BIOS is known
> to fail to do so, causing the VT-d engine to deadlock when used.
> 
> Vaguely based on Linux'es e0fc7e0b4b5e ("intel-iommu: Yet another BIOS
> workaround: Isoch DMAR unit with no TLB space").
> 
> To limit message string redundancy, fold parts with the IGD quirk logic.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> RFC: This requires MMCFG availability before Dom0 starts up, which is
>      not generally given. We may want/need to e.g. (ab)use the
>      .enable_device() hook to actually disable translation if MMCFG
>      accesses become available only in the course of Dom0 booting.

make sense

> RFC: While following Linux in this regard, I'm not convinced of issuing
>      the warning about the number of TLB entries when firmware set more
>      than 16 (I can observe 20 on the on matching system I have access
>      to.)

me either. Since you already observed 20 on one system, changing the
check to >=16 makes more sense.

The rest looks good to me:

        Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

> 
> I assume an implication is that the device in this case then may not be
> passed through to guests, but I don't see us enforcing the same anywhere
> for graphics devices when "no-igfx" is in use. Yet here I would want to
> follow whatever pre-existing model ...
> 
> --- a/xen/drivers/passthrough/vtd/extern.h
> +++ b/xen/drivers/passthrough/vtd/extern.h
> @@ -100,6 +100,7 @@ int msi_msg_write_remap_rte(struct msi_d
>  int intel_setup_hpet_msi(struct msi_desc *);
> 
>  int is_igd_vt_enabled_quirk(void);
> +bool is_azalia_tlb_enabled(const struct acpi_drhd_unit *);
>  void platform_quirks_init(void);
>  void vtd_ops_preamble_quirk(struct vtd_iommu *iommu);
>  void vtd_ops_postamble_quirk(struct vtd_iommu *iommu);
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -750,27 +750,43 @@ static void iommu_enable_translation(str
>      u32 sts;
>      unsigned long flags;
>      struct vtd_iommu *iommu = drhd->iommu;
> +    static const char crash_fmt[] = "%s; crash Xen for security purpose\n";
> 
>      if ( drhd->gfx_only )
>      {
> +        static const char disable_fmt[] = XENLOG_WARNING VTDPREFIX
> +                                          " %s; disabling IGD VT-d engine\n";
> +
>          if ( !iommu_igfx )
>          {
> -            printk(XENLOG_INFO VTDPREFIX
> -                   "Passed iommu=no-igfx option.  Disabling IGD VT-d 
> engine.\n");
> +            printk(disable_fmt, "passed iommu=no-igfx option");
>              return;
>          }
> 
>          if ( !is_igd_vt_enabled_quirk() )
>          {
> +            static const char msg[] = "firmware did not enable IGD for VT
> properly";
> +
>              if ( force_iommu )
> -                panic("BIOS did not enable IGD for VT properly, crash Xen for
> security purpose\n");
> +                panic(crash_fmt, msg);
> 
> -            printk(XENLOG_WARNING VTDPREFIX
> -                   "BIOS did not enable IGD for VT properly.  Disabling IGD 
> VT-d
> engine.\n");
> +            printk(disable_fmt, msg);
>              return;
>          }
>      }
> 
> +    if ( !is_azalia_tlb_enabled(drhd) )
> +    {
> +        static const char msg[] = "firmware did not enable TLB for sound
> device";
> +
> +        if ( force_iommu )
> +            panic(crash_fmt, msg);
> +
> +        printk(XENLOG_WARNING VTDPREFIX " %s; disabling ISOCH VT-d
> engine\n",
> +               msg);
> +        return;
> +    }
> +
>      /* apply platform specific errata workarounds */
>      vtd_ops_preamble_quirk(iommu);
> 
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -100,6 +100,69 @@ static void __init cantiga_b3_errata_ini
>          is_cantiga_b3 = 1;
>  }
> 
> +/*
> + * QUIRK to work around certain BIOSes enabling the ISOCH DMAR unit for
> the
> + * Azalia sound device, but not giving it any TLB entries, causing it to
> + * deadlock.
> + */
> +bool is_azalia_tlb_enabled(const struct acpi_drhd_unit *drhd)
> +{
> +    pci_sbdf_t sbdf;
> +    unsigned int vtisochctrl;
> +
> +    /* Only dedicated units are of interest. */
> +    if ( drhd->include_all || drhd->scope.devices_cnt != 1 )
> +        return true;
> +
> +    /* Check for the specific device. */
> +    sbdf = PCI_SBDF2(drhd->segment, drhd->scope.devices[0]);
> +    if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL ||
> +         pci_conf_read16(sbdf, PCI_DEVICE_ID) != 0x3a3e )
> +        return true;
> +
> +    /* Check for the corresponding System Management Registers device. */
> +    sbdf = PCI_SBDF(drhd->segment, 0, 0x14, 0);
> +    if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL ||
> +         pci_conf_read16(sbdf, PCI_DEVICE_ID) != 0x342e )
> +        return true;
> +
> +    vtisochctrl = pci_conf_read32(sbdf, 0x188);
> +    if ( vtisochctrl == 0xffffffff )
> +    {
> +        printk(XENLOG_WARNING VTDPREFIX
> +               " Cannot access VTISOCHCTRL at this time\n");
> +        return true;
> +    }
> +
> +    /*
> +     * If Azalia DMA is routed to the non-isoch DMAR unit, that's fine in
> +     * principle, but not consistent with the ACPI tables.
> +     */
> +    if ( vtisochctrl & 1 )
> +    {
> +        printk(XENLOG_WARNING VTDPREFIX
> +               " Inconsistency between chipset registers and ACPI tables\n");
> +        return true;
> +    }
> +
> +    /* Drop all bits other than the number of TLB entries. */
> +    vtisochctrl &= 0x1c;
> +
> +    /* If we have the recommended number of TLB entries, fine. */
> +    if ( vtisochctrl == 16 )
> +        return true;
> +
> +    /* Zero TLB entries? */
> +    if ( !vtisochctrl )
> +        return false;
> +
> +    printk(XENLOG_WARNING VTDPREFIX
> +           " Recommended TLB entries for ISOCH unit is 16; firmware set 
> %u\n",
> +           vtisochctrl);
> +
> +    return true;
> +}
> +
>  /* check for Sandybridge IGD device ID's */
>  static void __init snb_errata_init(void)
>  {


 


Rackspace

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