Re: [v3 05/15] vt-d: VT-d Posted-Interrupts feature detection

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/06/15 06:18, Feng Wu wrote:
> VT-d Posted-Interrupts is an enhancement to CPU side Posted-Interrupt.
> With VT-d Posted-Interrupts enabled, external interrupts from
> direct-assigned devices can be delivered to guests without VMM
> intervention when guest is running in non-root mode.
>
> This patch adds feature detection logic for VT-d posted-interrupt.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
> v3:
> - Remove the "if no intremap then no intpost" logic in
>   intel_vtd_setup(), it is covered in the iommu_setup().
> - Add "if no intremap then no intpost" logic in the end
>   of init_vtd_hw() which is called by vtd_resume().
>
> So the logic exists in the following three places:
> - parse_iommu_param()
> - iommu_setup()
> - init_vtd_hw()
>
>  xen/drivers/passthrough/vtd/iommu.c | 18 ++++++++++++++++--
>  xen/drivers/passthrough/vtd/iommu.h |  1 +
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 9053a1f..4221185 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2071,6 +2071,9 @@ static int init_vtd_hw(void)
>                  disable_intremap(drhd->iommu);
>      }
>  
> +    if ( !iommu_intremap )
> +        iommu_intpost = 0;
> +

Yet again, this curious logic.

There should be exactly 1 place which performs this dependency check,
not scattered in multiple places.

>      /*
>       * Set root entries for each VT-d engine.  After set root entry,
>       * must globally invalidate context cache, and then globally
> @@ -2133,8 +2136,8 @@ int __init intel_vtd_setup(void)
>      }
>  
>      /* We enable the following features only if they are supported by all VT-d
> -     * engines: Snoop Control, DMA passthrough, Queued Invalidation and
> -     * Interrupt Remapping.
> +     * engines: Snoop Control, DMA passthrough, Queued Invalidation, Interrupt
> +     * Remapping, and Posted Interrupt
>       */
>      for_each_drhd_unit ( drhd )
>      {
> @@ -2162,6 +2165,15 @@ int __init intel_vtd_setup(void)
>          if ( iommu_intremap && !ecap_intr_remap(iommu->ecap) )
>              iommu_intremap = 0;
>  
> +        /*
> +         * We cannot use posted interrupt if X86_FEATURE_CX16 is
> +         * not supported, since we count on this feature to
> +         * atomically update 16-byte IRTE in posted format.
> +         */
> +        if ( !iommu_intremap &&
> +             (!cap_intr_post(iommu->cap) || !cpu_has_cx16) )
> +            iommu_intpost = 0;

intremap is not relevant to CX16 being a prerequisite of intpost.

~Andrew

> +
>          if ( !vtd_ept_page_compatible(iommu) )
>              iommu_hap_pt_share = 0;
>  
> @@ -2187,6 +2199,7 @@ int __init intel_vtd_setup(void)
>      P(iommu_passthrough, "Dom0 DMA Passthrough");
>      P(iommu_qinval, "Queued Invalidation");
>      P(iommu_intremap, "Interrupt Remapping");
> +    P(iommu_intpost, "Posted Interrupt");
>      P(iommu_hap_pt_share, "Shared EPT tables");
>  #undef P
>  
> @@ -2206,6 +2219,7 @@ int __init intel_vtd_setup(void)
>      iommu_passthrough = 0;
>      iommu_qinval = 0;
>      iommu_intremap = 0;
> +    iommu_intpost = 0;
>      return ret;
>  }
>  
> diff --git a/xen/drivers/passthrough/vtd/iommu.h b/xen/drivers/passthrough/vtd/iommu.h
> index 80f8830..e807253 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -69,6 +69,7 @@
>  /*
>   * Decoding Capability Register
>   */
> +#define cap_intr_post(c)       (((c) >> 59) & 1)
>  #define cap_read_drain(c)      (((c) >> 55) & 1)
>  #define cap_write_drain(c)     (((c) >> 54) & 1)
>  #define cap_max_amask_val(c)   (((c) >> 48) & 0x3f)