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

[George Dunlap <george.dunlap@citrix.com>]

On 02/02/16 04:48, Wu, Feng wrote:
>>>>> +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.
>>>>> +     */
>>>>> +    if ( pi_desc->nv != posted_intr_vector )
>>>>> +        write_atomic(&pi_desc->nv, posted_intr_vector);
>>>>
>>>> Perhaps this was discussed before, but I don't recall and now
>>>> wonder - why inside an if()? This is a simple memory write on
>>>> x86.
>>>
>>> The initial purpose is that if NV is already equal to 'posted_intr_vector',
>>> we can save the following atomically write operation. There are some
>>> volatile stuff and barriers in write_atomic().
>>
>> But what does the final generated code look like? As I said, I'm
>> sure it's just a single MOV. And putting a conditional around it will
>> likely make things slower rather than faster.
> 
> Looking at the implementation of wirte_atomic(), it has volatile key
> word barrier inside, if you think this is not a big deal, I am fine to
> remove the check.

Oh, right -- so set_sn and clear_sn use the set/clear bits, which are
atomic read-modify-write, which we'd like to avoid on the vmexit/vmentry
paths (which is why we have the scheduler hooks); but this is just a
straight up write, so it's OK.

>>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>>> @@ -2293,6 +2293,8 @@ static int reassign_device_ownership(
>>>>>          pdev->domain = target;
>>>>>      }
>>>>>
>>>>> +    vmx_pi_hooks_reassign(source, target);
>>>>> +
>>>>>      return ret;
>>>>>  }
>>>>
>>>> I think you need to clear source's hooks here, but target's need to
>>>> get set ahead of the actual assignment.
>>>
>>> I think this is the place where the device is moved from
>>> &source->arch.pdev_list to &target->arch.pdev_list, if that is the
>>> case, we should clear source and set target here, right?
>>
>> No - target needs to be ready to deal with events from the device
>> _before_ the device actually gets assigned to it.
> 
> I still don't get your point. I still think this is the place where the device
> is being got assigned. Or maybe you can share more info about the
> place "_before_ the device actually gets assigned to it " ?

What happens if a device generates a PI interrupt *immediately* after
domain_context_mapping(), but before calling vmx_pi_hooks_reassign()?

 -George