Re: [Xen-devel] [PATCH v13 1/2] vmx: VT-d posted-interrupt core logic handling

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.16 at 09:34, <feng.wu@xxxxxxxxx> wrote:
> +static void vmx_vcpu_block(struct vcpu *v)
> +{
> +    unsigned long flags;
> +    unsigned int dest;
> +    spinlock_t *old_lock = pi_blocking_list_lock(v);
> +    spinlock_t *pi_blocking_list_lock = 
> &vmx_pi_blocking_list_lock(v->processor);
> +    struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> +
> +    spin_lock_irqsave(pi_blocking_list_lock, flags);
> +    old_lock = cmpxchg(&pi_blocking_list_lock(v), old_lock,
> +                       &vmx_pi_blocking_list_lock(v->processor));

See my comment on v12.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -2283,9 +2283,17 @@ static int reassign_device_ownership(
>      if ( ret )
>          return ret;
>  
> +    if ( !target->arch.hvm_domain.vmx.vcpu_block )
> +        vmx_pi_hooks_assign(target);

Why not just if ( !has_arch_pdevs(target) )?

>      ret = domain_context_mapping(target, devfn, pdev);
>      if ( ret )
> +    {
> +        if ( target->arch.hvm_domain.vmx.vcpu_block && 
> !has_arch_pdevs(target) )
> +            vmx_pi_hooks_deassign(target);

Same here.

> @@ -2293,6 +2301,9 @@ static int reassign_device_ownership(
>          pdev->domain = target;
>      }
>  
> +    if ( source->arch.hvm_domain.vmx.vcpu_block && !has_arch_pdevs(source) )
> +        vmx_pi_hooks_deassign(source);

And here.

> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -565,6 +565,12 @@ 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) ({                                                
>   \
> +    void (*func) (struct vcpu *) = 
> (v)->domain->arch.hvm_domain.vmx.vcpu_block;\
> +    if ( func )                                                              
>   \
> +        func(v);                                                             
>   \
> +})

See my comment on v12. The code structure actually was better
there, and all you needed to do is introduce a local variable.

> @@ -101,6 +160,17 @@ struct pi_desc {
>  
>  #define NR_PML_ENTRIES   512
>  
> +#define pi_blocking_vcpu_list(v)    \
> +    ((v)->arch.hvm_vmx.pi_blocking_vcpu_info.pi_blocking_vcpu_list)
> +
> +#define pi_blocking_list_lock(v)    \
> +    ((v)->arch.hvm_vmx.pi_blocking_vcpu_info.pi_blocking_list_lock)

The latest when writing this it should have occurred to you that
there are too many pi_blocking_ prefixes. Please strive to name
thinks such that macros like these aren't really necessary. The
same naturally applies to struct vmx_pi_blocking_vcpu, albeit
there the VMX maintainer have the final say.

Jan


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