| Message ID | 1441186198-991-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On 02/09/15 11:29, Alexey Kardashevskiy wrote: > For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. > The existing QEMU H_SET_MODE handler does not support this but > the KVM handler in HV KVM does. However it is not enabled. > > This enables the in-kernel H_SET_MODE handler which handles: > - Completed Instruction Address Breakpoint Register > - Watch point 0 registers. > > The rest is still handled in QEMU. > > Reported-by: Anton Blanchard <anton@samba.org> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/ppc/spapr.c | 1 + > target-ppc/kvm.c | 5 +++++ > target-ppc/kvm_ppc.h | 5 +++++ > 3 files changed, 11 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ae82565..7df0e15 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1728,6 +1728,7 @@ static void ppc_spapr_init(MachineState *machine) > if (kvm_enabled()) { > /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */ > kvmppc_enable_logical_ci_hcalls(); > + kvmppc_enable_set_mode_hcall(); > } > > /* allocate RAM */ > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index 110436d..9cf5308 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -1953,6 +1953,11 @@ void kvmppc_enable_logical_ci_hcalls(void) > kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE); > } > > +void kvmppc_enable_set_mode_hcall(void) > +{ > + kvmppc_enable_hcall(kvm_state, H_SET_MODE); > +} > + > void kvmppc_set_papr(PowerPCCPU *cpu) > { > CPUState *cs = CPU(cpu); > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > index 4d30e27..0714ba0 100644 > --- a/target-ppc/kvm_ppc.h > +++ b/target-ppc/kvm_ppc.h > @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env); > int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); > int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); > void kvmppc_enable_logical_ci_hcalls(void); > +void kvmppc_enable_set_mode_hcall(void); > void kvmppc_set_papr(PowerPCCPU *cpu); > int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); > void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); > @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void) > { > } > > +static inline void kvmppc_enable_set_mode_hcall(void) > +{ > +} This patch looks basically fine for me ... but I just started wondering whether we want to add such kvmppc_enable_* wrappers for all h-calls that we're going to enable in the future? IMHO it would be more elegant to add a function called kvmppc_enable_supported_hcalls() to target-ppc/kvm.c, which then turns on all wanted h-calls. hw/ppc/spapr.c would then be agnostic of the h-calls which are enabled by that function. We then also only need one wrapper in kvm_ppc.h - the one for kvmppc_enable_supported_hcalls(). What do you think? Thomas
> Am 02.09.2015 um 15:35 schrieb Thomas Huth <thuth@redhat.com>: > >> On 02/09/15 11:29, Alexey Kardashevskiy wrote: >> For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. >> The existing QEMU H_SET_MODE handler does not support this but >> the KVM handler in HV KVM does. However it is not enabled. >> >> This enables the in-kernel H_SET_MODE handler which handles: >> - Completed Instruction Address Breakpoint Register >> - Watch point 0 registers. >> >> The rest is still handled in QEMU. >> >> Reported-by: Anton Blanchard <anton@samba.org> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> hw/ppc/spapr.c | 1 + >> target-ppc/kvm.c | 5 +++++ >> target-ppc/kvm_ppc.h | 5 +++++ >> 3 files changed, 11 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index ae82565..7df0e15 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1728,6 +1728,7 @@ static void ppc_spapr_init(MachineState *machine) >> if (kvm_enabled()) { >> /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */ >> kvmppc_enable_logical_ci_hcalls(); >> + kvmppc_enable_set_mode_hcall(); >> } >> >> /* allocate RAM */ >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> index 110436d..9cf5308 100644 >> --- a/target-ppc/kvm.c >> +++ b/target-ppc/kvm.c >> @@ -1953,6 +1953,11 @@ void kvmppc_enable_logical_ci_hcalls(void) >> kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE); >> } >> >> +void kvmppc_enable_set_mode_hcall(void) >> +{ >> + kvmppc_enable_hcall(kvm_state, H_SET_MODE); >> +} >> + >> void kvmppc_set_papr(PowerPCCPU *cpu) >> { >> CPUState *cs = CPU(cpu); >> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h >> index 4d30e27..0714ba0 100644 >> --- a/target-ppc/kvm_ppc.h >> +++ b/target-ppc/kvm_ppc.h >> @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env); >> int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); >> int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); >> void kvmppc_enable_logical_ci_hcalls(void); >> +void kvmppc_enable_set_mode_hcall(void); >> void kvmppc_set_papr(PowerPCCPU *cpu); >> int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); >> void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); >> @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void) >> { >> } >> >> +static inline void kvmppc_enable_set_mode_hcall(void) >> +{ >> +} > > This patch looks basically fine for me ... but I just started wondering > whether we want to add such kvmppc_enable_* wrappers for all h-calls > that we're going to enable in the future? > > IMHO it would be more elegant to add a function called > kvmppc_enable_supported_hcalls() to target-ppc/kvm.c, which then turns > on all wanted h-calls. hw/ppc/spapr.c would then be agnostic of the > h-calls which are enabled by that function. We then also only need one > wrapper in kvm_ppc.h - the one for kvmppc_enable_supported_hcalls(). > What do you think? You may want to conditionalize different in-kernel hcall enablement (machine version for example). You also may want to do the enable from a device rather than machine code, like woth the h_random hypercall maybe. So for now I think we're better off with individual calls. Alex
On Wed, Sep 02, 2015 at 07:29:58PM +1000, Alexey Kardashevskiy wrote: > For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. > The existing QEMU H_SET_MODE handler does not support this but > the KVM handler in HV KVM does. However it is not enabled. > > This enables the in-kernel H_SET_MODE handler which handles: > - Completed Instruction Address Breakpoint Register > - Watch point 0 registers. > > The rest is still handled in QEMU. > > Reported-by: Anton Blanchard <anton@samba.org> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Won't this break auto-switching of VGA endianness, since that is triggered from the qemu implementation of H_SET_MODE? > --- > hw/ppc/spapr.c | 1 + > target-ppc/kvm.c | 5 +++++ > target-ppc/kvm_ppc.h | 5 +++++ > 3 files changed, 11 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ae82565..7df0e15 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1728,6 +1728,7 @@ static void ppc_spapr_init(MachineState *machine) > if (kvm_enabled()) { > /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */ > kvmppc_enable_logical_ci_hcalls(); > + kvmppc_enable_set_mode_hcall(); > } > > /* allocate RAM */ > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index 110436d..9cf5308 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -1953,6 +1953,11 @@ void kvmppc_enable_logical_ci_hcalls(void) > kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE); > } > > +void kvmppc_enable_set_mode_hcall(void) > +{ > + kvmppc_enable_hcall(kvm_state, H_SET_MODE); > +} > + > void kvmppc_set_papr(PowerPCCPU *cpu) > { > CPUState *cs = CPU(cpu); > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h > index 4d30e27..0714ba0 100644 > --- a/target-ppc/kvm_ppc.h > +++ b/target-ppc/kvm_ppc.h > @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env); > int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); > int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); > void kvmppc_enable_logical_ci_hcalls(void); > +void kvmppc_enable_set_mode_hcall(void); > void kvmppc_set_papr(PowerPCCPU *cpu); > int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); > void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); > @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void) > { > } > > +static inline void kvmppc_enable_set_mode_hcall(void) > +{ > +} > + > static inline void kvmppc_set_papr(PowerPCCPU *cpu) > { > }
On 09/03/2015 09:18 AM, David Gibson wrote: > On Wed, Sep 02, 2015 at 07:29:58PM +1000, Alexey Kardashevskiy wrote: >> For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. >> The existing QEMU H_SET_MODE handler does not support this but >> the KVM handler in HV KVM does. However it is not enabled. >> >> This enables the in-kernel H_SET_MODE handler which handles: >> - Completed Instruction Address Breakpoint Register >> - Watch point 0 registers. >> >> The rest is still handled in QEMU. >> >> Reported-by: Anton Blanchard <anton@samba.org> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Won't this break auto-switching of VGA endianness, since that is > triggered from the qemu implementation of H_SET_MODE? Only breakpoints are handled in the kernel, at least in upstream and powerkvm, for the rest it is H_TOO_HARD and then handled in QEMU so it won't break endianness. > >> --- >> hw/ppc/spapr.c | 1 + >> target-ppc/kvm.c | 5 +++++ >> target-ppc/kvm_ppc.h | 5 +++++ >> 3 files changed, 11 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index ae82565..7df0e15 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1728,6 +1728,7 @@ static void ppc_spapr_init(MachineState *machine) >> if (kvm_enabled()) { >> /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */ >> kvmppc_enable_logical_ci_hcalls(); >> + kvmppc_enable_set_mode_hcall(); >> } >> >> /* allocate RAM */ >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >> index 110436d..9cf5308 100644 >> --- a/target-ppc/kvm.c >> +++ b/target-ppc/kvm.c >> @@ -1953,6 +1953,11 @@ void kvmppc_enable_logical_ci_hcalls(void) >> kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE); >> } >> >> +void kvmppc_enable_set_mode_hcall(void) >> +{ >> + kvmppc_enable_hcall(kvm_state, H_SET_MODE); >> +} >> + >> void kvmppc_set_papr(PowerPCCPU *cpu) >> { >> CPUState *cs = CPU(cpu); >> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h >> index 4d30e27..0714ba0 100644 >> --- a/target-ppc/kvm_ppc.h >> +++ b/target-ppc/kvm_ppc.h >> @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env); >> int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); >> int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); >> void kvmppc_enable_logical_ci_hcalls(void); >> +void kvmppc_enable_set_mode_hcall(void); >> void kvmppc_set_papr(PowerPCCPU *cpu); >> int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); >> void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); >> @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void) >> { >> } >> >> +static inline void kvmppc_enable_set_mode_hcall(void) >> +{ >> +} >> + >> static inline void kvmppc_set_papr(PowerPCCPU *cpu) >> { >> } >
On 02/09/15 19:34, Alexander Graf wrote: > > >> Am 02.09.2015 um 15:35 schrieb Thomas Huth <thuth@redhat.com>: >> >>> On 02/09/15 11:29, Alexey Kardashevskiy wrote: >>> For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. >>> The existing QEMU H_SET_MODE handler does not support this but >>> the KVM handler in HV KVM does. However it is not enabled. >>> >>> This enables the in-kernel H_SET_MODE handler which handles: >>> - Completed Instruction Address Breakpoint Register >>> - Watch point 0 registers. >>> >>> The rest is still handled in QEMU. >>> >>> Reported-by: Anton Blanchard <anton@samba.org> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> hw/ppc/spapr.c | 1 + >>> target-ppc/kvm.c | 5 +++++ >>> target-ppc/kvm_ppc.h | 5 +++++ >>> 3 files changed, 11 insertions(+) ... >>> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h >>> index 4d30e27..0714ba0 100644 >>> --- a/target-ppc/kvm_ppc.h >>> +++ b/target-ppc/kvm_ppc.h >>> @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env); >>> int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); >>> int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); >>> void kvmppc_enable_logical_ci_hcalls(void); >>> +void kvmppc_enable_set_mode_hcall(void); >>> void kvmppc_set_papr(PowerPCCPU *cpu); >>> int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); >>> void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); >>> @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void) >>> { >>> } >>> >>> +static inline void kvmppc_enable_set_mode_hcall(void) >>> +{ >>> +} >> >> This patch looks basically fine for me ... but I just started wondering >> whether we want to add such kvmppc_enable_* wrappers for all h-calls >> that we're going to enable in the future? >> >> IMHO it would be more elegant to add a function called >> kvmppc_enable_supported_hcalls() to target-ppc/kvm.c, which then turns >> on all wanted h-calls. hw/ppc/spapr.c would then be agnostic of the >> h-calls which are enabled by that function. We then also only need one >> wrapper in kvm_ppc.h - the one for kvmppc_enable_supported_hcalls(). >> What do you think? > > You may want to conditionalize different in-kernel hcall enablement (machine version for example). You also may want to do the enable from a device rather than machine code, like woth the h_random hypercall maybe. > > So for now I think we're better off with individual calls. We could still handle such situations with additional wrappers. Simply call my suggested function kvmppc_enable_default_hcalls() instead, then it is clear that it only enables the h-calls that are always enabled by default. Thomas
On Wed, Sep 02, 2015 at 07:29:58PM +1000, Alexey Kardashevskiy wrote: > For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. > The existing QEMU H_SET_MODE handler does not support this but > the KVM handler in HV KVM does. However it is not enabled. > > This enables the in-kernel H_SET_MODE handler which handles: > - Completed Instruction Address Breakpoint Register > - Watch point 0 registers. > > The rest is still handled in QEMU. > > Reported-by: Anton Blanchard <anton@samba.org> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Applied to spapr-dev; I'll move it into spapr-next once it's had a little more testing.
On 09/04/2015 02:48 PM, David Gibson wrote: > On Wed, Sep 02, 2015 at 07:29:58PM +1000, Alexey Kardashevskiy wrote: >> For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. >> The existing QEMU H_SET_MODE handler does not support this but >> the KVM handler in HV KVM does. However it is not enabled. >> >> This enables the in-kernel H_SET_MODE handler which handles: >> - Completed Instruction Address Breakpoint Register >> - Watch point 0 registers. >> >> The rest is still handled in QEMU. >> >> Reported-by: Anton Blanchard <anton@samba.org> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Applied to spapr-dev; I'll move it into spapr-next once it's had a > little more testing. Another thing to keep in mind is that at some point in the past the host kernel used to handle H_SET_MODE and QEMU did not have the handler at all, now this functionality is in QEMU but some RHEL kernel could have picked a kernel with the in-kernel handler.
On Sun, Sep 06, 2015 at 04:14:25PM +1000, Alexey Kardashevskiy wrote: > On 09/04/2015 02:48 PM, David Gibson wrote: > >On Wed, Sep 02, 2015 at 07:29:58PM +1000, Alexey Kardashevskiy wrote: > >>For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. > >>The existing QEMU H_SET_MODE handler does not support this but > >>the KVM handler in HV KVM does. However it is not enabled. > >> > >>This enables the in-kernel H_SET_MODE handler which handles: > >>- Completed Instruction Address Breakpoint Register > >>- Watch point 0 registers. > >> > >>The rest is still handled in QEMU. > >> > >>Reported-by: Anton Blanchard <anton@samba.org> > >>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > >Applied to spapr-dev; I'll move it into spapr-next once it's had a > >little more testing. > > Another thing to keep in mind is that at some point in the past the host > kernel used to handle H_SET_MODE and QEMU did not have the handler at all, > now this functionality is in QEMU but some RHEL kernel could have picked a > kernel with the in-kernel handler. Not one that's supported as a KVM host, I believe.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index ae82565..7df0e15 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1728,6 +1728,7 @@ static void ppc_spapr_init(MachineState *machine) if (kvm_enabled()) { /* Enable H_LOGICAL_CI_* so SLOF can talk to in-kernel devices */ kvmppc_enable_logical_ci_hcalls(); + kvmppc_enable_set_mode_hcall(); } /* allocate RAM */ diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 110436d..9cf5308 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -1953,6 +1953,11 @@ void kvmppc_enable_logical_ci_hcalls(void) kvmppc_enable_hcall(kvm_state, H_LOGICAL_CI_STORE); } +void kvmppc_enable_set_mode_hcall(void) +{ + kvmppc_enable_hcall(kvm_state, H_SET_MODE); +} + void kvmppc_set_papr(PowerPCCPU *cpu) { CPUState *cs = CPU(cpu); diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h index 4d30e27..0714ba0 100644 --- a/target-ppc/kvm_ppc.h +++ b/target-ppc/kvm_ppc.h @@ -25,6 +25,7 @@ int kvmppc_get_hasidle(CPUPPCState *env); int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len); int kvmppc_set_interrupt(PowerPCCPU *cpu, int irq, int level); void kvmppc_enable_logical_ci_hcalls(void); +void kvmppc_enable_set_mode_hcall(void); void kvmppc_set_papr(PowerPCCPU *cpu); int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version); void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy); @@ -112,6 +113,10 @@ static inline void kvmppc_enable_logical_ci_hcalls(void) { } +static inline void kvmppc_enable_set_mode_hcall(void) +{ +} + static inline void kvmppc_set_papr(PowerPCCPU *cpu) { }
For setting debug watchpoints, sPAPR guests use H_SET_MODE hypercall. The existing QEMU H_SET_MODE handler does not support this but the KVM handler in HV KVM does. However it is not enabled. This enables the in-kernel H_SET_MODE handler which handles: - Completed Instruction Address Breakpoint Register - Watch point 0 registers. The rest is still handled in QEMU. Reported-by: Anton Blanchard <anton@samba.org> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/ppc/spapr.c | 1 + target-ppc/kvm.c | 5 +++++ target-ppc/kvm_ppc.h | 5 +++++ 3 files changed, 11 insertions(+)