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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2013-04-17:
>>>> 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(), ...
Yes.

> 
>> +    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.
> 
Yes.

>> +        val[group] = pi_get_pir(&v->arch.hvm_vmx.pi_desc, group);
> 
> Which will require adjustments to the types underlying this call.
What do you mean? Do you suggest pass pir[group] instead pi_desc?

> 
>> +
>> +    for_each_set_bit(i, val, MAX_VECTOR)
> 
> I'd also favor you not adding new uses of this redundant and
Is there better way to iterate the setting bit?

> 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).
Sure.

> 
>> --- 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.
Sure.

> 
> Jan


Best regards,
Yang



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