Re: [Xen-devel] [PATCH v4 13/17] x86/VPMU: Add privileged PMU mode

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.02.14 at 17:13, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 02/04/2014 11:01 AM, Jan Beulich wrote:
>>>>> On 04.02.14 at 16:53, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
>>> On 02/04/2014 06:31 AM, Jan Beulich wrote:
>>>>>>> On 21.01.14 at 20:08, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 
>>>>>>> wrote:
>>>>> +            gregs = &v->arch.vpmu.xenpmu_data->pmu.r.regs;
>>>>> +            gregs->cs = cs;
>>>> And now you store a NUL selector (i.e. just the RPL bits) into the
>>>> output field?
>>>>>            }
>>>>> -        else if ( !is_control_domain(current->domain) &&
>>>>> -                 !is_idle_vcpu(current) )
>>>>> +        else
>>>>>            {
>>>>> -            /* PV guest */
>>>>> +            /* HVM guest */
>>>>> +            struct segment_register cs;
>>>>> +
>>>>>                gregs = guest_cpu_user_regs();
>>>>>                memcpy(&v->arch.vpmu.xenpmu_data->pmu.r.regs,
>>>>>                       gregs, sizeof(struct cpu_user_regs));
>>>>> +
>>>>> +            hvm_get_segment_register(current, x86_seg_cs, &cs);
>>>>> +            gregs = &v->arch.vpmu.xenpmu_data->pmu.r.regs;
>>>>> +            gregs->cs = cs.attr.fields.dpl;
>>>> And here too? If that's intended, a code comment is a must.
>>> This is HVM-only path, PVH or PV don't go here so cs should be valid.
>> Isn't the reply of mine a few lines up in PV code? And why would
>> the selector being wrong for HVM be okay?
> 
> 
> This clause is for privileged profiling: we are in PV clause even though 
> the interrupt is taken by an HVM guest.
> 
> The diff is somewhat difficult to follow so here is the flow:
> 
>     // in privileged mode 'v' is dom0's CPU.
>     if ( !is_hvm_domain(v->domain) || (vpmu_mode & XENPMU_MODE_PRIV) )
>     {
>          if ( !is_hvm_domain(current->domain) )
>          {
>               // either PV (including dom0) or Xen is interrupted
>          }
>          else
>          {
>               // This is the clause we are discussing. 'current' is HVM
>               hvm_get_segment_register(current, x86_seg_cs, &cs);
>          }
>          send_guest_vcpu_virq(v, VIRQ_XENPMU);
>          return 1;
>     }
> 
> So I think CS should be correct for the guest, no?

Honestly - I can't tell. All I can tell is that there's a bogus setting
of cs to 0 or 3 (depending on whether in kernel mode) or to the
dpl field of the descriptor read from the hardware. All of which
are wrong without a cleat comment stating why doing it this way
is okay/acceptable/necessary-for-the-time-being.

So either drop this bogus code, or comment it properly.

Jan


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