Re: [PATCH v4 4/6] VMX: Make sure PI is in proper state before install the hooks

From: "Jan Beulich" <JBeulich@suse.com>
To: Feng Wu <feng.wu@intel.com>
Cc: george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com,
	dario.faggioli@citrix.com, kevin.tian@intel.com,
	xen-devel@lists.xen.org
Subject: Re: [PATCH v4 4/6] VMX: Make sure PI is in proper state before install the hooks
Date: Mon, 26 Sep 2016 06:45:58 -0600	[thread overview]
Message-ID: <57E934A60200007800112701@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1474425470-3629-5-git-send-email-feng.wu@intel.com>

>>> On 21.09.16 at 04:37, <feng.wu@intel.com> wrote:
> We may hit the ASSERT() in vmx_vcpu_block in the current code,

There are three of them - please be explicit which one is what gets
dealt with here.

> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -956,16 +956,13 @@ void virtual_vmcs_vmwrite(const struct vcpu *v, u32 vmcs_encoding, u64 val)
>   */
>  static void pi_desc_init(struct vcpu *v)
>  {
> -    uint32_t dest;
> -
>      v->arch.hvm_vmx.pi_desc.nv = posted_intr_vector;
>  
> -    dest = cpu_physical_id(v->processor);
> -
> -    if ( x2apic_enabled )
> -        v->arch.hvm_vmx.pi_desc.ndst = dest;
> -    else
> -        v->arch.hvm_vmx.pi_desc.ndst = MASK_INSR(dest, PI_xAPIC_NDST_MASK);
> +    /*
> +     * Mark NDST as invalid, then we can use this invalid value as a
> +     * marker to whether update NDST or not in vmx_pi_hooks_assign().
> +     */
> +    v->arch.hvm_vmx.pi_desc.ndst = 0xff;

Is this a proper "invalid" indicator in the x2APIC case? I would have
assumed you want 0xffffffff in that case.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -211,14 +211,39 @@ static void vmx_pi_list_cleanup(struct vcpu *v)
>  /* This function is called when pcidevs_lock is held */
>  void vmx_pi_hooks_assign(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>      if ( !iommu_intpost || !has_hvm_container_domain(d) )
>          return;
>  
>      ASSERT(!d->arch.hvm_domain.vmx.vcpu_block);
>  
> -    d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
> +    /*
> +     * We carefully handle the timing here:
> +     * - Install the context switch first
> +     * - Then set the NDST field
> +     * - Install the block and resume hooks in the end
> +     *
> +     * This can make sure the PI (especially the NDST feild) is
> +     * in proper state when we call vmx_vcpu_block().
> +     */
>      d->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
>      d->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        unsigned int dest = cpu_physical_id(v->processor);
> +        struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +
> +	/*
> +	 * We don't need to update NDST if 'arch.hvm_domain.vmx.pi_switch_to'
> +	 * already gets called
> +	 */

Stray tabs.

> +        (void)cmpxchg(&pi_desc->ndst, 0xff,
> +                x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));

Here even more than above I think you want to derive the xAPIC
value from the wider x2APIC one not just for the value to write, but
also for the value to compare against. If you did so in pi_desc_init()
as well as here, I don't see a strict need for introducing a suitable
#define. If you don't, however, you definitely want a pair of
#define-s to one can associate both places with one another.

But overall I think this is much better than the previous version, so
thanks for doing (and testing) this.

Jan

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