All of lore.kernel.org
 help / color / mirror / Atom feed
* Should KVM_GUEST stop depending on PARAVIRT?
@ 2015-07-24 17:33 Andy Lutomirski
  2015-07-27 13:59 ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Lutomirski @ 2015-07-24 17:33 UTC (permalink / raw)
  To: kvm list

PARAVIRT adds a fair amount of bloat and, AFAICT, KVM_GUEST doesn't
really need any of it.  Would it make sense to drop the dependency?

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-24 17:33 Should KVM_GUEST stop depending on PARAVIRT? Andy Lutomirski
@ 2015-07-27 13:59 ` Paolo Bonzini
  2015-07-27 15:56   ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-27 13:59 UTC (permalink / raw)
  To: Andy Lutomirski, KVM list



On 24/07/2015 19:33, Andy Lutomirski wrote:
> PARAVIRT adds a fair amount of bloat and, AFAICT, KVM_GUEST doesn't
> really need any of it.  Would it make sense to drop the dependency?

I think the main reason for PARAVIRT is that pv kernels have by default

        .read_msr = native_read_msr_safe,
        .write_msr = native_write_msr_safe,

Unfortunately Intel adds a bunch of performance measurement features
saying that "they work with this cpuid family/model/stepping" and at the
same time attach them to some non-architectural MSRs that, in principle
could be reused for something else years down the road.  This is not a
huge problem for Windows, where only tools such as vTune use these MSRs,
but it is a problem for Linux.

The alternative is ignore_msrs, but that's a very big hammer too.

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 13:59 ` Paolo Bonzini
@ 2015-07-27 15:56   ` Andy Lutomirski
  2015-07-27 17:49     ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Lutomirski @ 2015-07-27 15:56 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: KVM list

On Mon, Jul 27, 2015 at 6:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 24/07/2015 19:33, Andy Lutomirski wrote:
>> PARAVIRT adds a fair amount of bloat and, AFAICT, KVM_GUEST doesn't
>> really need any of it.  Would it make sense to drop the dependency?
>
> I think the main reason for PARAVIRT is that pv kernels have by default
>
>         .read_msr = native_read_msr_safe,
>         .write_msr = native_write_msr_safe,
>
> Unfortunately Intel adds a bunch of performance measurement features
> saying that "they work with this cpuid family/model/stepping" and at the
> same time attach them to some non-architectural MSRs that, in principle
> could be reused for something else years down the road.  This is not a
> huge problem for Windows, where only tools such as vTune use these MSRs,
> but it is a problem for Linux.
>
> The alternative is ignore_msrs, but that's a very big hammer too.

I think I'm missing something.  Does KVM_GUEST hook read_msr and
write_msr?  I don't see it.

Xen certainly needs those hooks, but I don't see why KVM would.

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 15:56   ` Andy Lutomirski
@ 2015-07-27 17:49     ` Paolo Bonzini
  2015-07-27 17:51       ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-27 17:49 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: KVM list



On 27/07/2015 17:56, Andy Lutomirski wrote:
> On Mon, Jul 27, 2015 at 6:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 24/07/2015 19:33, Andy Lutomirski wrote:
>>> PARAVIRT adds a fair amount of bloat and, AFAICT, KVM_GUEST doesn't
>>> really need any of it.  Would it make sense to drop the dependency?
>>
>> I think the main reason for PARAVIRT is that pv kernels have by default
>>
>>         .read_msr = native_read_msr_safe,
>>         .write_msr = native_write_msr_safe,
>>
>> Unfortunately Intel adds a bunch of performance measurement features
>> saying that "they work with this cpuid family/model/stepping" and at the
>> same time attach them to some non-architectural MSRs that, in principle
>> could be reused for something else years down the road.  This is not a
>> huge problem for Windows, where only tools such as vTune use these MSRs,
>> but it is a problem for Linux.
>>
>> The alternative is ignore_msrs, but that's a very big hammer too.
> 
> I think I'm missing something.  Does KVM_GUEST hook read_msr and
> write_msr?  I don't see it.

PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
KVM guest.

Paolo

> Xen certainly needs those hooks, but I don't see why KVM would.
> 
> --Andy
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 17:49     ` Paolo Bonzini
@ 2015-07-27 17:51       ` Andy Lutomirski
  2015-07-27 18:30         ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Andy Lutomirski @ 2015-07-27 17:51 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: KVM list

On Mon, Jul 27, 2015 at 10:49 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 27/07/2015 17:56, Andy Lutomirski wrote:
>> On Mon, Jul 27, 2015 at 6:59 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 24/07/2015 19:33, Andy Lutomirski wrote:
>>>> PARAVIRT adds a fair amount of bloat and, AFAICT, KVM_GUEST doesn't
>>>> really need any of it.  Would it make sense to drop the dependency?
>>>
>>> I think the main reason for PARAVIRT is that pv kernels have by default
>>>
>>>         .read_msr = native_read_msr_safe,
>>>         .write_msr = native_write_msr_safe,
>>>
>>> Unfortunately Intel adds a bunch of performance measurement features
>>> saying that "they work with this cpuid family/model/stepping" and at the
>>> same time attach them to some non-architectural MSRs that, in principle
>>> could be reused for something else years down the road.  This is not a
>>> huge problem for Windows, where only tools such as vTune use these MSRs,
>>> but it is a problem for Linux.
>>>
>>> The alternative is ignore_msrs, but that's a very big hammer too.
>>
>> I think I'm missing something.  Does KVM_GUEST hook read_msr and
>> write_msr?  I don't see it.
>
> PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
> KVM guest.
>

Still confused.  On a KVM guest (with PARAVIRT=y), doesn't read_msr do
exactly the same thing it does on native, albeit with more indirection
and patching involved?

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 17:51       ` Andy Lutomirski
@ 2015-07-27 18:30         ` Paolo Bonzini
  2015-07-27 18:45           ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-27 18:30 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: KVM list



On 27/07/2015 19:51, Andy Lutomirski wrote:
> > > I think I'm missing something.  Does KVM_GUEST hook read_msr and
> > > write_msr?  I don't see it.
> >
> > PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
> > KVM guest.
>
> Still confused.  On a KVM guest (with PARAVIRT=y), doesn't read_msr do
> exactly the same thing it does on native, albeit with more indirection
> and patching involved?

With PARAVIRT=y it never #GPs:

        .read_msr = native_read_msr_safe,
        .write_msr = native_write_msr_safe,

I don't remember if it's this way on bare-metal too.

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 18:30         ` Paolo Bonzini
@ 2015-07-27 18:45           ` Andy Lutomirski
  2015-07-27 19:04             ` Arjan van de Ven
  2015-07-28  9:57             ` Paolo Bonzini
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-07-27 18:45 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: KVM list, Arjan van de Ven

On Mon, Jul 27, 2015 at 11:30 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 27/07/2015 19:51, Andy Lutomirski wrote:
>> > > I think I'm missing something.  Does KVM_GUEST hook read_msr and
>> > > write_msr?  I don't see it.
>> >
>> > PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
>> > KVM guest.
>>
>> Still confused.  On a KVM guest (with PARAVIRT=y), doesn't read_msr do
>> exactly the same thing it does on native, albeit with more indirection
>> and patching involved?
>
> With PARAVIRT=y it never #GPs:
>
>         .read_msr = native_read_msr_safe,
>         .write_msr = native_write_msr_safe,
>
> I don't remember if it's this way on bare-metal too.

Oh, whoops, I missed the "_safe".  IMO that's just a bug, and I guess
KVM relies on it?

ISTM the host should be fixed so that a non-PARAVIRT guest won't crash
when using perf (if it indeed currently crashes) and/or the perf code
should be fixed to work without this bug^Wfeature.

Then KVM_GUEST kernels could be de-bloated by dropping PARAVIRT.

Hi Arjan- A quick and dirty measurement suggests that this would save
2-3 ms when booting a KVM_GUEST=y kernel under KVM by turning
apply_paravirt into a noop.

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 18:45           ` Andy Lutomirski
@ 2015-07-27 19:04             ` Arjan van de Ven
  2015-07-28  9:57             ` Paolo Bonzini
  1 sibling, 0 replies; 17+ messages in thread
From: Arjan van de Ven @ 2015-07-27 19:04 UTC (permalink / raw)
  To: Andy Lutomirski, Paolo Bonzini; +Cc: KVM list

On 7/27/2015 11:45 AM, Andy Lutomirski wrote:

>> With PARAVIRT=y it never #GPs:
>>
>>          .read_msr = native_read_msr_safe,
>>          .write_msr = native_write_msr_safe,
>>
>> I don't remember if it's this way on bare-metal too.
>
> Oh, whoops, I missed the "_safe".  IMO that's just a bug, and I guess
> KVM relies on it?

btw it's a common misperception that _safe is actually safe;-)
they're still highly dangerous, just they won't fault on the linux side


>
> ISTM the host should be fixed so that a non-PARAVIRT guest won't crash
> when using perf (if it indeed currently crashes) and/or the perf code
> should be fixed to work without this bug^Wfeature.
>
> Then KVM_GUEST kernels could be de-bloated by dropping PARAVIRT.
>
> Hi Arjan- A quick and dirty measurement suggests that this would save
> 2-3 ms when booting a KVM_GUEST=y kernel under KVM by turning
> apply_paravirt into a noop.

nice


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-27 18:45           ` Andy Lutomirski
  2015-07-27 19:04             ` Arjan van de Ven
@ 2015-07-28  9:57             ` Paolo Bonzini
  2015-07-29  0:23               ` Andy Lutomirski
  1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-28  9:57 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: KVM list, Arjan van de Ven



On 27/07/2015 20:45, Andy Lutomirski wrote:
> On Mon, Jul 27, 2015 at 11:30 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 27/07/2015 19:51, Andy Lutomirski wrote:
>>>>> I think I'm missing something.  Does KVM_GUEST hook read_msr and
>>>>> write_msr?  I don't see it.
>>>>
>>>> PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
>>>> KVM guest.
>>>
>>> Still confused.  On a KVM guest (with PARAVIRT=y), doesn't read_msr do
>>> exactly the same thing it does on native, albeit with more indirection
>>> and patching involved?
>>
>> With PARAVIRT=y it never #GPs:
>>
>>         .read_msr = native_read_msr_safe,
>>         .write_msr = native_write_msr_safe,
>>
>> I don't remember if it's this way on bare-metal too.
> 
> Oh, whoops, I missed the "_safe".  IMO that's just a bug, and I guess
> KVM relies on it?
> 
> ISTM the host should be fixed so that a non-PARAVIRT guest won't crash
> when using perf (if it indeed currently crashes)

It does.

> and/or the perf code
> should be fixed to work without this bug^Wfeature.

You can call it even feature^Wbug, I won't take it personal. :)  It does
not prevent scary messages (such as "intel_rapl: no valid rapl domains
found in package 0") in the logs for example.  See
https://bugzilla.redhat.com/show_bug.cgi?id=1178491 for a discussion
about such scary messages.

Unfortunately, fixing the host vs. fixing the guest is an arms race.

Fixing the host would entail adding support for a shitload of
non-architectural MSRs.  Most of the time there is not even any correct
value that you can report for those MSRs (what's the FSB speed of a
virtual machine or its power consumption?) so there is nontrivial risk
that the guest code would anyway break or keep logging scary messages;
in which case we're back to square 1.

Fixing the guest (perf and thermal and cpufreq and whatever) code isn't
any better.  First of all, I wouldn't be surprised if maintainers
opposed the "fixes" (see the above BZ for an example, and see PeterZ's
love of KVM and "virt crap" for a hint).  Second, it would break again
with all new processors, assuming that the patches are accepted in the
first place.

So while the _safe hack is a hack and does result in scary messages
sometimes, we cannot really do much better than it just by modifying the
drivers, and at least the arms race is avoided.  Perhaps one alternative
is to disable the thermal and cpufreq subsystems altogether if running
on a virt platform?

Paolo

> Then KVM_GUEST kernels could be de-bloated by dropping PARAVIRT.
> 
> Hi Arjan- A quick and dirty measurement suggests that this would save
> 2-3 ms when booting a KVM_GUEST=y kernel under KVM by turning
> apply_paravirt into a noop.
> 
> --Andy
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-28  9:57             ` Paolo Bonzini
@ 2015-07-29  0:23               ` Andy Lutomirski
  2015-07-29  4:49                 ` Venkatesh Srinivas
  2015-07-29  9:19                 ` Peter Zijlstra
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-07-29  0:23 UTC (permalink / raw)
  To: Paolo Bonzini, Peter Zijlstra; +Cc: KVM list, Arjan van de Ven

Let's add peterz?

On Tue, Jul 28, 2015 at 2:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 27/07/2015 20:45, Andy Lutomirski wrote:
>> On Mon, Jul 27, 2015 at 11:30 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 27/07/2015 19:51, Andy Lutomirski wrote:
>>>>>> I think I'm missing something.  Does KVM_GUEST hook read_msr and
>>>>>> write_msr?  I don't see it.
>>>>>
>>>>> PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
>>>>> KVM guest.
>>>>
>>>> Still confused.  On a KVM guest (with PARAVIRT=y), doesn't read_msr do
>>>> exactly the same thing it does on native, albeit with more indirection
>>>> and patching involved?
>>>
>>> With PARAVIRT=y it never #GPs:
>>>
>>>         .read_msr = native_read_msr_safe,
>>>         .write_msr = native_write_msr_safe,
>>>
>>> I don't remember if it's this way on bare-metal too.
>>
>> Oh, whoops, I missed the "_safe".  IMO that's just a bug, and I guess
>> KVM relies on it?
>>
>> ISTM the host should be fixed so that a non-PARAVIRT guest won't crash
>> when using perf (if it indeed currently crashes)
>
> It does.
>
>> and/or the perf code
>> should be fixed to work without this bug^Wfeature.
>
> You can call it even feature^Wbug, I won't take it personal. :)  It does
> not prevent scary messages (such as "intel_rapl: no valid rapl domains
> found in package 0") in the logs for example.  See
> https://bugzilla.redhat.com/show_bug.cgi?id=1178491 for a discussion
> about such scary messages.
>
> Unfortunately, fixing the host vs. fixing the guest is an arms race.
>
> Fixing the host would entail adding support for a shitload of
> non-architectural MSRs.  Most of the time there is not even any correct
> value that you can report for those MSRs (what's the FSB speed of a
> virtual machine or its power consumption?) so there is nontrivial risk
> that the guest code would anyway break or keep logging scary messages;
> in which case we're back to square 1.
>
> Fixing the guest (perf and thermal and cpufreq and whatever) code isn't
> any better.  First of all, I wouldn't be surprised if maintainers
> opposed the "fixes" (see the above BZ for an example, and see PeterZ's
> love of KVM and "virt crap" for a hint).  Second, it would break again
> with all new processors, assuming that the patches are accepted in the
> first place.
>
> So while the _safe hack is a hack and does result in scary messages
> sometimes, we cannot really do much better than it just by modifying the
> drivers, and at least the arms race is avoided.  Perhaps one alternative
> is to disable the thermal and cpufreq subsystems altogether if running
> on a virt platform?

PeterZ, can we fix this for real instead of relying on
CONFIG_PARAVIRT=y accidentally turning all msr accesses into "safe"
accesses?  We have the CPUID "hypervisor" bit, but I still don't fully
understand the problem.

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  0:23               ` Andy Lutomirski
@ 2015-07-29  4:49                 ` Venkatesh Srinivas
  2015-07-29  8:27                   ` Paolo Bonzini
  2015-07-29  9:19                 ` Peter Zijlstra
  1 sibling, 1 reply; 17+ messages in thread
From: Venkatesh Srinivas @ 2015-07-29  4:49 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: Paolo Bonzini, Peter Zijlstra, KVM list, Arjan van de Ven

> On Tue, Jul 28, 2015 at 2:57 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 27/07/2015 20:45, Andy Lutomirski wrote:
>>> On Mon, Jul 27, 2015 at 11:30 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>
>>>> On 27/07/2015 19:51, Andy Lutomirski wrote:
>>>>>>> I think I'm missing something.  Does KVM_GUEST hook read_msr and
>>>>>>> write_msr?  I don't see it.
>>>>>>
>>>>>> PARAVIRT does, and it's the main reason why you'd want PARAVIRT for a
>>>>>> KVM guest.
>>>>>
>>>>> Still confused.  On a KVM guest (with PARAVIRT=y), doesn't read_msr do
>>>>> exactly the same thing it does on native, albeit with more indirection
>>>>> and patching involved?
>>>>
>>>> With PARAVIRT=y it never #GPs:
>>>>
>>>>         .read_msr = native_read_msr_safe,
>>>>         .write_msr = native_write_msr_safe,
>>>>
>>>> I don't remember if it's this way on bare-metal too.
>>>
>>> Oh, whoops, I missed the "_safe".  IMO that's just a bug, and I guess
>>> KVM relies on it?
>>>
>>> ISTM the host should be fixed so that a non-PARAVIRT guest won't crash
>>> when using perf (if it indeed currently crashes)
>>
>> It does.
>>
>>> and/or the perf code
>>> should be fixed to work without this bug^Wfeature.
>>
>> You can call it even feature^Wbug, I won't take it personal. :)  It does
>> not prevent scary messages (such as "intel_rapl: no valid rapl domains
>> found in package 0") in the logs for example.  See
>> https://bugzilla.redhat.com/show_bug.cgi?id=1178491 for a discussion
>> about such scary messages.

This bug is not openly readable.

Which MSR accesses (in perf/etc) don't use safe {RD,WR}MSR accesses currently?

-- vs;

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  4:49                 ` Venkatesh Srinivas
@ 2015-07-29  8:27                   ` Paolo Bonzini
  0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-29  8:27 UTC (permalink / raw)
  To: Venkatesh Srinivas, Andy Lutomirski
  Cc: Peter Zijlstra, KVM list, Arjan van de Ven



On 29/07/2015 06:49, Venkatesh Srinivas wrote:
>>> You can call it even feature^Wbug, I won't take it personal. :)  It does
>>> not prevent scary messages (such as "intel_rapl: no valid rapl domains
>>> found in package 0") in the logs for example.  See
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1178491 for a discussion
>>> about such scary messages.
> 
> This bug is not openly readable.

Oops, opened now.

> Which MSR accesses (in perf/etc) don't use safe {RD,WR}MSR accesses currently?

For example see boost_state in acpi-cpufreq.c and KVM commit
22d48b2d2aa0 ("KVM: svm: writes to MSR_K7_HWCR generates GPE in guest",
2014-06-26).

See also commit dc9b2d933a1d5782b70977024f862759c8ebb2f7 in KVM.

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  0:23               ` Andy Lutomirski
  2015-07-29  4:49                 ` Venkatesh Srinivas
@ 2015-07-29  9:19                 ` Peter Zijlstra
  2015-07-29  9:24                   ` Paolo Bonzini
  1 sibling, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2015-07-29  9:19 UTC (permalink / raw)
  To: Andy Lutomirski; +Cc: Paolo Bonzini, KVM list, Arjan van de Ven

On Tue, Jul 28, 2015 at 05:23:22PM -0700, Andy Lutomirski wrote:
> 
> PeterZ, can we fix this for real instead of relying on
> CONFIG_PARAVIRT=y accidentally turning all msr accesses into "safe"
> accesses?  We have the CPUID "hypervisor" bit, but I still don't fully
> understand the problem.

So a whole bunch of PMU drivers already probe the MSRs at init time
using a combination of rdmsrl_safe() and wrmsrl_safe() to see if:

 1) its there at all
 2) if its there, it 'works' like it ought to

See for example: arch/x86/kernel/cpu/perf_event.c:check_hw_exists().

I have no problem using the _safe() methods to probe MSRs in init
routines and failing the init of the driver etc.

What I do object to is sprinkling _safe() all over the driver itself and
creating horrid error paths all over (yes, people have proposed crazy
things like that).

Another example is the LBR probing in
arch/x86/kernel/cpu/perf_event_intel.c:intel_pmu_init(). This is the one
people wanted to use _safe() crud for all throughout the driver, instead
of simply disabling the LBR at init time.


As to the 'bug' at hand, I really don't see the problem. The RAPL driver
says there's no valid RAPL domains, this is true, there aren't any on
the virtual machine, so what?



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  9:19                 ` Peter Zijlstra
@ 2015-07-29  9:24                   ` Paolo Bonzini
  2015-07-29  9:40                     ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-29  9:24 UTC (permalink / raw)
  To: Peter Zijlstra, Andy Lutomirski; +Cc: KVM list, Arjan van de Ven



On 29/07/2015 11:19, Peter Zijlstra wrote:
> On Tue, Jul 28, 2015 at 05:23:22PM -0700, Andy Lutomirski wrote:
>> PeterZ, can we fix this for real instead of relying on
>> CONFIG_PARAVIRT=y accidentally turning all msr accesses into "safe"
>> accesses?  We have the CPUID "hypervisor" bit, but I still don't fully
>> understand the problem.
> 
> So a whole bunch of PMU drivers already probe the MSRs at init time
> using a combination of rdmsrl_safe() and wrmsrl_safe() to see if:
> 
>  1) its there at all
>  2) if its there, it 'works' like it ought to
> 
> See for example: arch/x86/kernel/cpu/perf_event.c:check_hw_exists().

Great.  Of course it's even better if you can probe it with CPUID (e.g.
aperfmperf in intel_pstate.c), then there's no need for the _safe variant.

> I have no problem using the _safe() methods to probe MSRs in init
> routines and failing the init of the driver etc.
> 
> What I do object to is sprinkling _safe() all over the driver itself and
> creating horrid error paths all over (yes, people have proposed crazy
> things like that).
> 
> Another example is the LBR probing in
> arch/x86/kernel/cpu/perf_event_intel.c:intel_pmu_init(). This is the one
> people wanted to use _safe() crud for all throughout the driver, instead
> of simply disabling the LBR at init time.

I agree that check_msr is okay.

> As to the 'bug' at hand, I really don't see the problem. The RAPL driver
> says there's no valid RAPL domains, this is true, there aren't any on
> the virtual machine, so what?

Well, people have complained about it because it's KERN_ERR.  Do you
think it is okay to downgrade this (perhaps not even just on VMs) to info?

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  9:24                   ` Paolo Bonzini
@ 2015-07-29  9:40                     ` Peter Zijlstra
  2015-07-29  9:41                       ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2015-07-29  9:40 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Andy Lutomirski, KVM list, Arjan van de Ven

On Wed, Jul 29, 2015 at 11:24:09AM +0200, Paolo Bonzini wrote:
> 
> 
> On 29/07/2015 11:19, Peter Zijlstra wrote:
> > On Tue, Jul 28, 2015 at 05:23:22PM -0700, Andy Lutomirski wrote:
> >> PeterZ, can we fix this for real instead of relying on
> >> CONFIG_PARAVIRT=y accidentally turning all msr accesses into "safe"
> >> accesses?  We have the CPUID "hypervisor" bit, but I still don't fully
> >> understand the problem.
> > 
> > So a whole bunch of PMU drivers already probe the MSRs at init time
> > using a combination of rdmsrl_safe() and wrmsrl_safe() to see if:
> > 
> >  1) its there at all
> >  2) if its there, it 'works' like it ought to
> > 
> > See for example: arch/x86/kernel/cpu/perf_event.c:check_hw_exists().
> 
> Great.  Of course it's even better if you can probe it with CPUID (e.g.
> aperfmperf in intel_pstate.c), then there's no need for the _safe variant.

Reliable CPUID enumeration would be nice indeed. Note that CPUID isn't
ideal either, for instance the CPUID HT bit doesn't indicate if the CPU
has hyperthreading enabled, just that it is able to report extended
topology information.

Determining if a CPU has hyperthreading enabled is extremely difficult,
and basically boils down to init all cpus, see if there are siblings
anywhere.

And then try and distinguish between the case where HT is disabled and
someone hot-unplugged all siblings :-)

> > As to the 'bug' at hand, I really don't see the problem. The RAPL driver
> > says there's no valid RAPL domains, this is true, there aren't any on
> > the virtual machine, so what?
> 
> Well, people have complained about it because it's KERN_ERR.  Do you
> think it is okay to downgrade this (perhaps not even just on VMs) to info?

Ah, do people really have nothing better to do? ;-) Seems like a petty
complaint.

Sure, it seems our check_hw_exists() does the same thing:

        printk("%sFailed to access perfctr msr (MSR %x is %Lx)\n",
                boot_cpu_has(X86_FEATURE_HYPERVISOR) ? KERN_INFO : KERN_ERR,
                reg, val_new);

so ERR for real hardware, INFO for hypervisor thingies.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  9:40                     ` Peter Zijlstra
@ 2015-07-29  9:41                       ` Paolo Bonzini
  2015-07-30  0:14                         ` Andy Lutomirski
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2015-07-29  9:41 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Andy Lutomirski, KVM list, Arjan van de Ven



On 29/07/2015 11:40, Peter Zijlstra wrote:
> > Well, people have complained about it because it's KERN_ERR.  Do you
> > think it is okay to downgrade this (perhaps not even just on VMs) to info?
>
> Ah, do people really have nothing better to do? ;-) Seems like a petty
> complaint.
> 
> Sure, it seems our check_hw_exists() does the same thing:
> 
>         printk("%sFailed to access perfctr msr (MSR %x is %Lx)\n",
>                 boot_cpu_has(X86_FEATURE_HYPERVISOR) ? KERN_INFO : KERN_ERR,
>                 reg, val_new);
> 
> so ERR for real hardware, INFO for hypervisor thingies.

Great, I'll send a patch then!

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: Should KVM_GUEST stop depending on PARAVIRT?
  2015-07-29  9:41                       ` Paolo Bonzini
@ 2015-07-30  0:14                         ` Andy Lutomirski
  0 siblings, 0 replies; 17+ messages in thread
From: Andy Lutomirski @ 2015-07-30  0:14 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Peter Zijlstra, KVM list, Arjan van de Ven

On Wed, Jul 29, 2015 at 2:41 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 29/07/2015 11:40, Peter Zijlstra wrote:
>> > Well, people have complained about it because it's KERN_ERR.  Do you
>> > think it is okay to downgrade this (perhaps not even just on VMs) to info?
>>
>> Ah, do people really have nothing better to do? ;-) Seems like a petty
>> complaint.
>>
>> Sure, it seems our check_hw_exists() does the same thing:
>>
>>         printk("%sFailed to access perfctr msr (MSR %x is %Lx)\n",
>>                 boot_cpu_has(X86_FEATURE_HYPERVISOR) ? KERN_INFO : KERN_ERR,
>>                 reg, val_new);
>>
>> so ERR for real hardware, INFO for hypervisor thingies.
>
> Great, I'll send a patch then!
>

With that patch applied, would anything actually go wrong if we did
something like this:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/commit/?h=x86/pv_msr

If not, I'll send it out and we can see if anything awful shakes out.
If the sky doesn't fall, then maybe KVM_GUEST can drop its PARAVIRT
dependency.

--Andy

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-07-30  0:15 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-24 17:33 Should KVM_GUEST stop depending on PARAVIRT? Andy Lutomirski
2015-07-27 13:59 ` Paolo Bonzini
2015-07-27 15:56   ` Andy Lutomirski
2015-07-27 17:49     ` Paolo Bonzini
2015-07-27 17:51       ` Andy Lutomirski
2015-07-27 18:30         ` Paolo Bonzini
2015-07-27 18:45           ` Andy Lutomirski
2015-07-27 19:04             ` Arjan van de Ven
2015-07-28  9:57             ` Paolo Bonzini
2015-07-29  0:23               ` Andy Lutomirski
2015-07-29  4:49                 ` Venkatesh Srinivas
2015-07-29  8:27                   ` Paolo Bonzini
2015-07-29  9:19                 ` Peter Zijlstra
2015-07-29  9:24                   ` Paolo Bonzini
2015-07-29  9:40                     ` Peter Zijlstra
2015-07-29  9:41                       ` Paolo Bonzini
2015-07-30  0:14                         ` Andy Lutomirski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.