Re: [Xen-devel] [v2 06/11] vmx: add help functions to support PML

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.04.15 at 09:23, <kai.huang@xxxxxxxxxxxxxxx> wrote:

> 
> On 04/17/2015 02:58 PM, Jan Beulich wrote:
>>>>> On 17.04.15 at 08:51, <kai.huang@xxxxxxxxxxxxxxx> wrote:
>>> On 04/17/2015 02:23 PM, Jan Beulich wrote:
>>>>>>> On 17.04.15 at 05:10, <kai.huang@xxxxxxxxxxxxxxx> wrote:
>>>>> On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>>>>>>> On 15.04.15 at 09:03, <kai.huang@xxxxxxxxxxxxxxx> wrote:
>>>>>>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>>>>>>> +{
>>>>>>> +    uint64_t *pml_buf;
>>>>>>> +    unsigned long pml_idx;
>>>>>>> +
>>>>>>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>>>>>>> +
>>>>>>> +    vmx_vmcs_enter(v);
>>>>>>> +
>>>>>>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
>>>>>> Don't you require the vCPU to be non-running or current when you
>>>>>> get here? If so, perhaps add a respective ASSERT()?
>>>>> Yes an ASSERT would be better.
>>>>>
>>>>> v->pause_count will be increased if vcpu is kicked out by domain_pause
>>>>> explicitly, but looks the same thing won't be done if vcpu is kicked out
>>>>> by PML buffer full VMEXIT. So should the ASSERT be done like below?
>>>>>
>>>>> ASSERT(atomic_read(&v->pause_count) || (v == current));
>>>> For one I'd reverse the two parts. And then I think pause count
>>>> being non-zero is not a sufficient condition - if a non-synchronous
>>>> pause was issued against the vCPU it may still be running. I'd
>>>> suggest !vcpu_runnable(v) && !v->is_running, possibly with the
>>>> pause count check instead of the runnable one if the only
>>>> permitted case where v != current requires the vCPU to be
>>>> paused.
>>> The vmx_vcpu_flush_pml_buffer is only supposed to be called in below cases:
>>>
>>>       - When PML full VMEXIT happens
>>>       - In paging_log_dirty_op & hap_track_dirty_vram, before reporting
>>> dirty pages to userspace.
>>>       - In vmx_vcpu_disable_pml, called from vmx_vcpu_destroy, or when
>>> log-dirty mode is disabled.
>>>
>>> In the latter two cases, domain_pause is guaranteed to be called before
>>> vmx_vcpu_flush_pml_buffer is called, therefore looks there's no
>>> possibility of non-synchronous pause of the vcpu.
>>>
>>> Or are you suggesting we should suppose this function can be called from
>>> any caller, and meanwhile is able to act reasonably?
>> No. All I'm saying is in order to protect against eventual undue
>> future callers, it should assert that its preconditions are met. I.e.
>> if the vCPU is expected to be paused, check that the pause
>> count in non-zero _and_ that the pause actually took effect.
> I see. I will do as you suggested:
> 
> ASSERT((v == current) || (!vcpu_runnable(v) && !v->is_running));
> 
> And v != current should be the only case requires the vcpu to be paused.

But if you require (or at least expect) the vCPU to be paused, this
isn't what I suggested. Afaict

ASSERT((v == current) || (!v->is_running && atomic_read(v->pause_count)));

would then be the right check (and, while not be a full guarantee that
things wouldn't change behind your back, would at least increase
chances that the vCPU's runnable state won't change, as the vCPU
could have been non-runnable for reasons other than having been
paused).

Jan


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