Re: [PATCH 2/2] KVM: PPC: Book3E: Emulate MCSRR0/1 SPR and rfmci instruction

[Scott Wood <scottwood@freescale.com>]

On 07/09/2013 04:49:32 PM, Alexander Graf wrote:
>=20
> On 09.07.2013, at 20:29, Scott Wood wrote:
>=20
> > On 07/09/2013 12:46:32 PM, Alexander Graf wrote:
> >> On 07/09/2013 07:16 PM, Scott Wood wrote:
> >>> On 07/08/2013 01:45:58 PM, Alexander Graf wrote:
> >>>> On 03.07.2013, at 15:30, Mihai Caraman wrote:
> >>>> > Some guests are making use of return from machine check =20
> instruction
> >>>> > to do crazy things even though the 64-bit kernel doesn't =20
> handle yet
> >>>> > this interrupt. Emulate MCSRR0/1 SPR and rfmci instruction =20
> accordingly.
> >>>> >
> >>>> > Signed-off-by: Mihai Caraman <mihai.caraman@freescale.com>
> >>>> > ---
> >>>> > arch/powerpc/include/asm/kvm_host.h |    1 +
> >>>> > arch/powerpc/kvm/booke_emulate.c    |   25 =20
> +++++++++++++++++++++++++
> >>>> > arch/powerpc/kvm/timing.c           |    1 +
> >>>> > 3 files changed, 27 insertions(+), 0 deletions(-)
> >>>> >
> >>>> > diff --git a/arch/powerpc/include/asm/kvm_host.h =20
> b/arch/powerpc/include/asm/kvm_host.h
> >>>> > index af326cd..0466789 100644
> >>>> > --- a/arch/powerpc/include/asm/kvm_host.h
> >>>> > +++ b/arch/powerpc/include/asm/kvm_host.h
> >>>> > @@ -148,6 +148,7 @@ enum kvm_exit_types {
> >>>> >     EMULATED_TLBWE_EXITS,
> >>>> >     EMULATED_RFI_EXITS,
> >>>> >     EMULATED_RFCI_EXITS,
> >>>> > +    EMULATED_RFMCI_EXITS,
> >>>> I would quite frankly prefer to see us abandon the whole exit =20
> timing framework in the kernel and instead use trace points. Then we =20
> don't have to maintain all of this randomly exercised code.
> >>> Would this map well to tracepoints?  We're not trying to track =20
> discrete events, so much as accumulated time spent in different areas.
> >> I think so. We'd just have to emit tracepoints as soon as we enter =20
> handle_exit and in prepare_to_enter. Then a user space program should =20
> have everything it needs to create statistics out of that. It would =20
> certainly simplify the entry/exit path.
> >
> > I was hoping that wasn't going to be your answer. :-)
> >
> > Such a change would introduce a new dependency, more complexity, =20
> and the possibility for bad totals to result from a ring buffer =20
> filling faster than userspace can drain it.
>=20
> Well, at least it would allow for optional tracing :). Today you have =20
> to change a compile flag to enable / disable timing stats.
>=20
> >
> > I also don't see how it would simplify entry/exit, since we'd still =20
> need to take timestamps in the same places, in order to record a =20
> final event that says how long a particular event took.
>=20
> Not sure I understand. What the timing stats do is that they measure =20
> the time between [exit ... entry], right? We'd do the same thing, =20
> just all in C code. That means we would become slightly less =20
> accurate, but gain dynamic enabling of the traces and get rid of all =20
> the timing stat asm code.

Compile-time enabling bothers me less than a loss of accuracy (not just =20
a small loss by moving into C code, but a potential for a large loss if =20
we overflow the buffer) and a dependency on a userspace tool (both in =20
terms of the tool needing to be written, and in the hassle of ensuring =20
that it's present in the root filesystem of whatever system I'm =20
testing).  And the whole mechanism will be more complicated.

Lots of debug options are enabled at build time; why must this be =20
different?

-Scott=