Re: [Xen-devel] [PATCH v4 3/4] VT-d PI: restrict the vcpu number on a given pcpu

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Jul 07, 2017 at 09:57:47AM -0600, Jan Beulich wrote:
>>>> On 07.07.17 at 08:48, <chao.gao@xxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -95,22 +95,91 @@ static DEFINE_PER_CPU(struct vmx_pi_blocking_vcpu, 
>> vmx_pi_blocking);
>>  uint8_t __read_mostly posted_intr_vector;
>>  static uint8_t __read_mostly pi_wakeup_vector;
>>  
>> +/*
>> + * Protect critical sections to avoid adding a blocked vcpu to a destroyed
>> + * blocking list.
>> + */
>> +static DEFINE_SPINLOCK(remote_pbl_operation);
>
>What is "pbl" supposed to stand for?

pi blocking list.

>
>> +#define remote_pbl_operation_begin(flags)                   \
>> +({                                                          \
>> +    spin_lock_irqsave(&remote_pbl_operation, flags);        \
>> +})
>> +
>> +#define remote_pbl_operation_done(flags)                    \
>> +({                                                          \
>> +    spin_unlock_irqrestore(&remote_pbl_operation, flags);   \
>> +})
>
>No need for the ({ }) here.
>
>But then I don't understand what this is needed for in the first
>place. If this is once again about CPU offlining, then I can only
>repeat that such happens in stop_machine context. Otherwise

But I don't think vmx_pi_desc_fixup() happens in stop_machine context,
please refer to cpu_callback() function in hvm.c and the time
notifier_call_chain(CPU_DEAD) is called in cpu_down().

Our goal here is to avoid adding one entry to a destroyed list.
To avoid destruction happens during adding, we can put these two
process in critical sections, like

add:
        remote_pbl_operation_begin()
        add one entry to the list
        remote_pbl_operation_end()

destroy:
        remote_pbl_operation_begin()
        destruction
        remote_pbl_operation_end()

Destruction may happen before we enter the critical section.
so adding should be:

add:
        remote_pbl_operation_begin()
        check the list is still valid
        add one entry to the list
        remote_pbl_operation_end()

In this patch, we choose an online cpu's list. The list should be valid
for the list is always destroyed after offline.

>I'm afraid the comment ahead of this code section needs
>adjustment, as I can't interpret it in another way.

Thanks
Chao

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