Re: [PATCH v12 for-xen-4.5 16/20] x86/VPMU: Handle PMU interrupts for PV guests

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 10/01/2014 09:18 AM, Jan Beulich wrote:
>>>> On 01.10.14 at 14:53, <boris.ostrovsky@oracle.com> wrote:
>> On 10/01/2014 02:49 AM, Jan Beulich wrote:
>>>>>> On 30.09.14 at 18:37, <boris.ostrovsky@oracle.com> wrote:
>>>> On 09/30/2014 11:44 AM, Jan Beulich wrote:
>>>>>>>> +            {
>>>>>>>> +                r->cs = cur_regs->cs;
>>>>>>>> +                if ( sampled->arch.flags & TF_kernel_mode )
>>>>>>>> +                    r->cs &= ~3;
>>>>>>> And once again I wonder how the consumer of this data is to tell
>>>>>>> apart guest kernel and hypervisor addresses.
>>>>>> Based on the RIP --- perf, for example, searches through various symbol
>>>>>> tables.
>>>>> That doesn't help when profiling HVM/PVH guests - addresses are
>>>>> ambiguous in that case.
>>>> Hypervisor traces are only sent to dom0, which is currently PV only. The
>>>> key here, of course, is the word 'currently'.
>>> So you completely ignore PVH Dom0? Experimental or not, I don't
>>> think that's the way to go.
>> As I mentioned in an earlier reply, I will set domain_id in the reported
>> structure to DOMID_XEN when we are reporting hypervisor sample.
>>
>>> Furthermore the check around this is
>>> once again using sampled, not sampling.
>> Which check are you referring to?
> The if() right outside (above) the still visible patch context.

Why should it be 'sampling'? I am collecting registers from sampled vcpu 
so I need to look at that domain's flags to determine the mode, don't I?


>
>>> Looking at the separation of hypervisor vs guest context to report
>>> again
>>>
>>>               /* Non-privileged domains are always in XENPMU_MODE_SELF mode */
>>>               if ( (vpmu_mode & XENPMU_MODE_SELF) ||
>>>                    (!is_hardware_domain(sampled->domain) &&
>>>                     !is_idle_vcpu(sampled)) )
>>>                   cur_regs = guest_cpu_user_regs();
>>>               else
>>>                   cur_regs = regs;
>>>
>>> I now additionally wonder why the condition here isn't just the SELF
>>> check: If the interrupt happened while in the hypervisor, why would
>>> you override this unconditionally to report a guest sample instead?
>>> Shouldn't the profiling domain tell you what it wants in that case
>>> (global vs guest local view)?
>> The second part of the check (!is_hardware_domain(sampled->domain) &&
>> !is_idle_vcpu(sampled)) is to prevent sending hypervisor sample to a
>> non-privileged guest. vpmu_mode may be, for example, XENPMU_MODE_HV but
>> that only means that dom0 can get hypervisor samples.
> Right, but that's not what the code above does: Instead of sending
> the hypervisor sample to Dom0 it converts it to a guest mode one.

Oh, I see --- when we get interrupted while in a non-privileged guest's 
context (but in hypervisor) I send guest's registers, not Xen's.

I think just SELF check is not sufficient though, we need to make sure 
that we are not sending hypervisor sample to non-dom0. So
     if ( (vpmu_mode & XENPMU_MODE_SELF) || 
!is_hardware_domain(sampling->domain) )

-boris