Re: [Xen-devel] [PATCH v5 4/7] VMX: Make sure PI is in proper state before install the hooks

[Jan Beulich]

>>> On 24.10.16 at 09:45,  wrote:

> 
>> -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Monday, October 24, 2016 3:23 PM
>> To: Wu, Feng 
>> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com;
>> george.dun...@eu.citrix.com; Tian, Kevin ; xen-
>> de...@lists.xen.org 
>> Subject: RE: [PATCH v5 4/7] VMX: Make sure PI is in proper state before 
> install
>> the hooks
>> 
>> >>> On 17.10.16 at 08:26,  wrote:
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: Wednesday, October 12, 2016 9:45 PM
>> >> >>> On 11.10.16 at 02:57,  wrote:
>> >> >  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 = 0x;
>> >>
>> >> I think I had at the same time asked to make this a #define, so the
>> >> two (currently) instance can be connected together.
>> >
>> > Sorry, Maybe I didn't get that. Do you mean I need to define a Macro
>> > for 0x, so we can use it here and in vmx.c?
>> 
>> Yes.
> 
> Thanks for confirm. Which one do you like? Put the macro in vmx.h
> or asm/apic.h?

This being (for now at least) VMX-specific, I think the former is to be
preferred.

Jan

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

Re: [Xen-devel] [PATCH v5 4/7] VMX: Make sure PI is in proper state before install the hooks

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, October 24, 2016 3:23 PM
> To: Wu, Feng 
> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com;
> george.dun...@eu.citrix.com; Tian, Kevin ; xen-
> de...@lists.xen.org
> Subject: RE: [PATCH v5 4/7] VMX: Make sure PI is in proper state before 
> install
> the hooks
> 
> >>> On 17.10.16 at 08:26,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: Wednesday, October 12, 2016 9:45 PM
> >> >>> On 11.10.16 at 02:57,  wrote:
> >> >  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 = 0x;
> >>
> >> I think I had at the same time asked to make this a #define, so the
> >> two (currently) instance can be connected together.
> >
> > Sorry, Maybe I didn't get that. Do you mean I need to define a Macro
> > for 0x, so we can use it here and in vmx.c?
> 
> Yes.

Thanks for confirm. Which one do you like? Put the macro in vmx.h
or asm/apic.h?

Thanks,
Feng

> 
> Jan

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

Re: [Xen-devel] [PATCH v5 4/7] VMX: Make sure PI is in proper state before install the hooks

[Jan Beulich]

>>> On 17.10.16 at 08:26,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: Wednesday, October 12, 2016 9:45 PM
>> >>> On 11.10.16 at 02:57,  wrote:
>> >  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 = 0x;
>> 
>> I think I had at the same time asked to make this a #define, so the
>> two (currently) instance can be connected together.
> 
> Sorry, Maybe I didn't get that. Do you mean I need to define a Macro
> for 0x, so we can use it here and in vmx.c?

Yes.

Jan

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

Re: [Xen-devel] [PATCH v5 4/7] VMX: Make sure PI is in proper state before install the hooks

[Wu, Feng]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Wednesday, October 12, 2016 9:45 PM
> To: Wu, Feng 
> Cc: andrew.coop...@citrix.com; dario.faggi...@citrix.com;
> george.dun...@eu.citrix.com; Tian, Kevin ; xen-
> de...@lists.xen.org
> Subject: Re: [PATCH v5 4/7] VMX: Make sure PI is in proper state before 
> install
> the hooks
> 
> >>> On 11.10.16 at 02:57,  wrote:
> >  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 = 0x;
> 
> I think I had at the same time asked to make this a #define, so the
> two (currently) instance can be connected together.

Sorry, Maybe I didn't get that. Do you mean I need to define a Macro
for 0x, so we can use it here and in vmx.c?

Thanks,
Feng

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

Re: [Xen-devel] [PATCH v5 4/7] VMX: Make sure PI is in proper state before install the hooks

[Jan Beulich]

>>> On 11.10.16 at 02:57,  wrote:
>  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 = 0x;

I think I had at the same time asked to make this a #define, so the
two (currently) instance can be connected together.

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -211,14 +211,39 @@ static void vmx_pi_do_resume(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

I think you mean "got" or "has got". Also I think you'd better refer to
the actual VMX function instead of the hook pointer field.

Jan


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

[Xen-devel] [PATCH v5 4/7] VMX: Make sure PI is in proper state before install the hooks

[Feng Wu]

We may hit the last ASSERT() in vmx_vcpu_block in the current code,
since vmx_vcpu_block() may get called before vmx_pi_switch_to()
has been installed or executed. Here We use cmpxchg to update
the NDST field, this can make sure we only update the NDST when
vmx_pi_switch_to() has not been called. So the NDST is in a
proper state in vmx_vcpu_block().

Suggested-by: Jan Beulich 
Signed-off-by: Feng Wu 
---
v5:
- Use 0x as the invalid value for NDST field.

 xen/arch/x86/hvm/vmx/vmcs.c | 13 +
 xen/arch/x86/hvm/vmx/vmx.c  | 27 ++-
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 1bd875a..10976bd 100644
--- 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 = 0x;
 }
 
 static int construct_vmcs(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index b8b152a..b14c84e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -211,14 +211,39 @@ static void vmx_pi_do_resume(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
+ */
+(void)cmpxchg(&pi_desc->ndst, 0x,
+x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK));
+}
+
+d->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
 d->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume;
 }
 
-- 
2.1.0


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