|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v12 1/2] vmx: VT-d posted-interrupt core logic handling
>>> On 19.02.16 at 02:55, <feng.wu@xxxxxxxxx> wrote:
> +static void vmx_vcpu_block(struct vcpu *v)
> +{
> + unsigned long flags;
> + unsigned int dest;
> + spinlock_t *old_lock;
> + spinlock_t *pi_block_list_lock =
> + &per_cpu(pi_blocked_vcpu_lock, v->processor);
> + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +
> + spin_lock_irqsave(pi_block_list_lock, flags);
> + old_lock = cmpxchg(&v->arch.hvm_vmx.pi_block_list_lock, NULL,
> + &per_cpu(pi_blocked_vcpu_lock, v->processor));
Why don't you use the local variable here?
> + /*
> + * 'v->arch.hvm_vmx.pi_block_list_lock' should be NULL before being
> assigned
> + * to a new value, since the vCPU is currently running and it cannot be
> on
> + * any blocking list.
> + */
> + ASSERT(old_lock == NULL);
> +
> + list_add_tail(&v->arch.hvm_vmx.pi_blocked_vcpu_list,
> + &per_cpu(pi_blocked_vcpu, v->processor));
> + spin_unlock_irqrestore(pi_block_list_lock, flags);
> +
> + ASSERT(!pi_test_sn(pi_desc));
> +
> + dest = cpu_physical_id(v->processor);
> +
> + ASSERT(pi_desc->ndst ==
> + (x2apic_enabled ? dest : MASK_INSR(dest, PI_xAPIC_NDST_MASK)));
May I ask for consistency between this and ...
> +static void vmx_pi_switch_to(struct vcpu *v)
> +{
> + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +
> + write_atomic(&pi_desc->ndst, x2apic_enabled ?
> + cpu_physical_id(v->processor) :
> + MASK_INSR(cpu_physical_id(v->processor),
> PI_xAPIC_NDST_MASK));
... this? I.e. to either in both cases use a local variable "dest", or
in both cases to avoid doing so?
> +static void vmx_pi_do_resume(struct vcpu *v)
> +{
> + unsigned long flags;
> + spinlock_t *pi_block_list_lock;
> + struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +
> + ASSERT(!test_bit(_VPF_blocked, &v->pause_flags));
> +
> + /*
> + * Set 'NV' field back to posted_intr_vector, so the
> + * Posted-Interrupts can be delivered to the vCPU when
> + * it is running in non-root mode.
> + */
> + write_atomic(&pi_desc->nv, posted_intr_vector);
> +
> + /* The vCPU is not on any blocking list. */
> + pi_block_list_lock = v->arch.hvm_vmx.pi_block_list_lock;
> + if ( pi_block_list_lock == NULL )
> + return;
> +
> + /* Prevent the compiler from eliminating the local variable.*/
> + barrier();
As said before - this is too late: This way you still don't guarantee
pi_block_list_lock != NULL. The barrier needs to be after the read,
and _before_ the first use. Also I think the right barrier here
would be smp_rmb().
> + spin_lock_irqsave(pi_block_list_lock, flags);
> +
> + /*
> + * v->arch.hvm_vmx.pi_block_list_lock == NULL here means the vCPU was
> + * removed from the blocking list while we are acquiring the lock.
> + */
> + if ( v->arch.hvm_vmx.pi_block_list_lock != NULL )
> + {
ASSERT(v->arch.hvm_vmx.pi_block_list_lock == pi_block_list_lock)?
> +/* This function is called when pcidevs_lock is held */
> +void vmx_pi_hooks_assign(struct domain *target)
> +{
> + if ( !iommu_intpost )
> + return;
> +
> + if ( has_hvm_container_domain(target) &&
> + !target->arch.hvm_domain.vmx.vcpu_block )
> + {
> + target->arch.hvm_domain.vmx.vcpu_block = vmx_vcpu_block;
> + target->arch.hvm_domain.vmx.pi_switch_from = vmx_pi_switch_from;
> + target->arch.hvm_domain.vmx.pi_switch_to = vmx_pi_switch_to;
> + target->arch.hvm_domain.vmx.pi_do_resume = vmx_pi_do_resume;
> + }
> +}
> +
> +/* This function is called when pcidevs_lock is held */
> +void vmx_pi_hooks_deassign(struct domain *source)
> +{
> + if ( !iommu_intpost )
> + return;
> +
> + if ( has_hvm_container_domain(source) &&
> + source->arch.hvm_domain.vmx.vcpu_block && !has_arch_pdevs(source) )
> + {
I think it would be nice for the conditions in both functions to match
up. Since you can't add has_arch_pdevs(target) to the earlier, maybe
it should be the caller of the latter to check !has_arch_pdevs(source)?
And if you then made the caller(s) of vmx_pi_hooks_assign() do so
too, the checks on ->arch.hvm_domain.vmx.vcpu_block could
apparently become ASSERT()-s instead.
Also since neither function now takes two domain pointers anymore,
please name the function parameters just "d". And finally please, in
each function, combine the two if()-s.
> @@ -112,6 +251,10 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>
> spin_lock_init(&v->arch.hvm_vmx.vmcs_lock);
>
> + INIT_LIST_HEAD(&v->arch.hvm_vmx.pi_blocked_vcpu_list);
> +
> + v->arch.hvm_vmx.pi_block_list_lock = NULL;
The latter is pointless, as struct vcpu starts out zero-initialized.
> @@ -740,6 +883,8 @@ static void vmx_ctxt_switch_from(struct vcpu *v)
> vmx_save_guest_msrs(v);
> vmx_restore_host_msrs();
> vmx_save_dr(v);
> + if (v->domain->arch.hvm_domain.vmx.pi_switch_from)
> + v->domain->arch.hvm_domain.vmx.pi_switch_from(v);
Coding style.
> @@ -752,6 +897,8 @@ static void vmx_ctxt_switch_to(struct vcpu *v)
>
> vmx_restore_guest_msrs(v);
> vmx_restore_dr(v);
> + if (v->domain->arch.hvm_domain.vmx.pi_switch_to)
> + v->domain->arch.hvm_domain.vmx.pi_switch_to(v);
Again. I'm pretty sure this has been pointed out before. And there
are more of these.
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -565,6 +565,11 @@ const char *hvm_efer_valid(const struct vcpu *v,
> uint64_t value,
> signed int cr0_pg);
> unsigned long hvm_cr4_guest_reserved_bits(const struct vcpu *v, bool_t
> restore);
>
> +#define arch_vcpu_block(v) ({ \
> + if ( (v)->domain->arch.hvm_domain.vmx.vcpu_block ) \
> + (v)->domain->arch.hvm_domain.vmx.vcpu_block((v)); \
> +})
I'd prefer if v got evaluated just once in this macro. Also note
the redundant parentheses in the function argument.
> @@ -160,6 +219,14 @@ struct arch_vmx_struct {
> struct page_info *vmwrite_bitmap;
>
> struct page_info *pml_pg;
> +
> + /*
> + * Before it is blocked, vCPU is added to the per-cpu list.
> + * VT-d engine can send wakeup notification event to the
> + * pCPU and wakeup the related vCPU.
> + */
> + struct list_head pi_blocked_vcpu_list;
> + spinlock_t *pi_block_list_lock;
> };
Didn't we settle on making this a struct with a "list" and a "lock"
member? And along those lines the local per-CPU variables added
to xen/arch/x86/hvm/vmx/vmx.c would also benefit from being
put in a struct, making more obvious their close relationship.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |