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

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Thursday, June 25, 2015 6:21 PM
> To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> Cc: keir@xxxxxxx; jbeulich@xxxxxxxx; Tian, Kevin; Zhang, Yang Z;
> george.dunlap@xxxxxxxxxxxxx
> Subject: Re: [v3 05/15] vt-d: VT-d Posted-Interrupts feature detection
> 
> 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@xxxxxxxxx>
> > ---
> > 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.

Seems, it is a little hard to find the one right place to perform this check.
But I will think this more.

> 
> >      /*
> >       * 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.
> 

I think I made a typo here, it should be like this:

+        if ( !iommu_intremap || !cap_intr_post(iommu->cap) || !cpu_has_cx16 )
+            iommu_intpost = 0;

Thanks,
Feng

> ~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)


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel