Re: [Xen-devel] [PATCH v2 3/5] VMX: Add posted interrupt supporting

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.04.13 at 08:51, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
> +static void vmx_sync_pir_to_irr(struct vcpu *v)
> +{
> +    struct vlapic *vlapic = vcpu_vlapic(v);
> +    u64 val[4];

This should be DECLARE_BITMAP(), ...

> +    u32 group, i;
> +
> +    if ( !pi_test_and_clear_on(&v->arch.hvm_vmx.pi_desc) )
> +        return;
> +
> +    for ( group = 0; group < 4; group++ )

... and use ARRAY_SIZE() here.

> +        val[group] = pi_get_pir(&v->arch.hvm_vmx.pi_desc, group);

Which will require adjustments to the types underlying this call.

> +
> +    for_each_set_bit(i, val, MAX_VECTOR)

I'd also favor you not adding new uses of this redundant and
mis-named #define - please use NR_VECTORS.

> +#define for_each_set_bit(bit, addr, size)                   \
> +    for ( (bit) = find_first_bit((addr), (size));           \
> +          (bit) < (size);                                   \
> +          (bit) = find_next_bit((addr), (size), (bit) + 1) )

Please drop the pointless parentheses (but keep the ones that
are needed).

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -99,6 +99,33 @@ void vmx_update_exception_bitmap(struct vcpu *v);
>  void vmx_update_cpu_exec_control(struct vcpu *v);
>  void vmx_update_secondary_exec_control(struct vcpu *v);
>  
> +#define POSTED_INTR_ON  0
> +static inline int pi_test_and_set_pir(int vector, struct pi_desc *pi_desc)
> +{
> +    return test_and_set_bit(vector, (unsigned long *)pi_desc->pir);
> +}
> +
> +static inline int pi_test_and_set_on(struct pi_desc *pi_desc)
> +{
> +    return test_and_set_bit(POSTED_INTR_ON,
> +            (unsigned long *)&pi_desc->control);
> +}
> +
> +static inline void pi_set_on(struct pi_desc *pi_desc)
> +{
> +    return set_bit(POSTED_INTR_ON, (unsigned long *)&pi_desc->control);
> +}
> +
> +static inline int pi_test_and_clear_on(struct pi_desc *pi_desc)
> +{
> +    return test_and_clear_bit(POSTED_INTR_ON,
> +            (unsigned long *)&pi_desc->control);
> +}

I don't see why any of the casts would be necessary. Please drop
them.

Jan


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