From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [v3 05/15] vt-d: VT-d Posted-Interrupts feature detection Date: Thu, 25 Jun 2015 11:21:03 +0100 Message-ID: <558BD60F.3020008@citrix.com> References: <1435123109-10481-1-git-send-email-feng.wu@intel.com> <1435123109-10481-6-git-send-email-feng.wu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1435123109-10481-6-git-send-email-feng.wu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Feng Wu , xen-devel@lists.xen.org Cc: yang.z.zhang@intel.com, george.dunlap@eu.citrix.com, kevin.tian@intel.com, keir@xen.org, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org 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 > --- > 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)