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

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

Zhang, Yang Z wrote on 2013-04-09:
> Jan Beulich wrote on 2013-04-09:
>>>>> On 09.04.13 at 08:01, Yang Zhang <yang.z.zhang@xxxxxxxxx> wrote:
>>> +static void vmx_deliver_posted_intr(struct vcpu *v, u8 vector, u8 
>>> trig_mode)
>>> +{
>>> +    vlapic_set_tmr(vcpu_vlapic(v), vector, trig_mode);
>>> +
>>> +    vmx_update_eoi_exit_bitmap(v, vector, trig_mode);
>>> +
>>> +    if ( pi_test_and_set_pir(vector, &v->arch.hvm_vmx.pi_desc) )
>>> +        return;
>>> +
>>> +    if ( unlikely(v->arch.hvm_vmx.eoi_exitmap_changed) )
>>> +    {
>>> +        /* If EOI exitbitmap needs to changed or notification vector
>>> +         * can't be allocated, interrupt will not be injected till
>>> +         * VMEntry as it used to be
>>> +         */
>>> +        pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc);
>> 
>> Why is this a test-and-set when the result is unused?
>> 
>> 
>>> +        goto out;
>>> +    }
>>> +
>>> +    if ( !pi_test_and_set_on(&v->arch.hvm_vmx.pi_desc) )
>> 
>> And if it's just because you don't want to have a simple "set" only
>> operation, then the two instances should be folded, and the result
>> stored in a local variable.
> Yes. Only want to set on bit. Perhaps it is better to define a separate 
> function
> pi_set_on() for this to make the logic more clear.
> 
>> 
>>> +static void vmx_sync_pir_to_irr(struct vcpu *v)
>>> +{
>>> +    struct vlapic *vlapic = vcpu_vlapic(v);
>>> +    u32 val;
>>> +    int offset, group;
>> 
>> unsigned.
> Sure.
> 
>>> +
>>> +    if ( !pi_test_and_clear_on(&v->arch.hvm_vmx.pi_desc) )
>>> +        return;
>>> +
>>> +    for (group = 0; group < 8; group++ )
>> 
>> Formatting.
> Sure.
> 
>> 
>>> +    {
>>> +        val = pi_get_pir(&v->arch.hvm_vmx.pi_desc, group);
>>> +        offset = APIC_IRR + 0x10 * group;
>>> +        *((uint32_t *)(&vlapic->regs->data[offset])) |= val;
>> 
>> Can't you use vlapic_set_vector() here (even if that means
>> looping over vectors individually rather than groups), to add the
>> necessary atomicity (I don't see how you avoid races with other
>> updates) and to avoid the ugly cast?
Which races?

Best regards,
Yang



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