Re: [Qemu-devel] [PATCH 3/3] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Wednesday, June 11, 2014 6:35 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
>
> On 06/10/2014 05:06 PM, Bharat Bhushan wrote:
> > This patch adds software breakpoint, hardware breakpoint and hardware
> > watchpoint support for ppc. If the debug interrupt is not handled then
> > this is injected to guest.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > hw/ppc/e500.c| 1 +
> > target-ppc/kvm.c | 304
> > ++---
> --
> > target-ppc/kvm_ppc.h | 1 +
> > 3 files changed, 278 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..514c595
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
> > if (kvm_enabled()) {
> > kvmppc_init();
> > }
> > +kvmppc_e500_hw_breakpoint_init();
> > }
> >
> > static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> > a/target-ppc/kvm.c b/target-ppc/kvm.c index 1d2384d..f5fbec6 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -38,6 +38,7 @@
> > #include "hw/ppc/ppc.h"
> > #include "sysemu/watchdog.h"
> > #include "trace.h"
> > +#include "exec/gdbstub.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs)
> >
> > static int kvmppc_inject_debug_exception(CPUState *cs)
> > {
> > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +CPUPPCState *env = &cpu->env;
> > +struct kvm_sregs sregs;
> > +int ret;
> > +
> > +if (!cap_booke_sregs) {
> > +return -1;
> > +}
> > +
> > +ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> > +if (ret < 0) {
> > +return -1;
> > +}
> > +
>
> I don't think any of this code should ever run for non-e500, no?
You mean the code below in this function?
>
> > +if (sregs.u.e.features & KVM_SREGS_E_ED) {
>
> Hrm - we never seem to set E_ED in kvm?
Uhh, you are right. Going through the whole discussion about interrupt
injection to guest I found that one patch missed for upstream.
Will send that patch
>
> > +sregs.u.e.dsrr0 = env->nip;
> > +sregs.u.e.dsrr1 = env->msr;
> > +} else {
> > +sregs.u.e.csrr0 = env->nip;
> > +sregs.u.e.csrr1 = env->msr;
> > +}
> > +
> > +sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR;
> > +sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR];
> > +
> > +ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> > +if (ret < 0) {
> > +return -1;
> > +}
> > +
> > +env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG);
> > +
> > return 0;
> > }
> >
> > @@ -1275,6 +1308,239 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> uint32_t dcrn, uint32_t dat
> > return 0;
> > }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +uint32_t sc = tswap32(debug_inst_opcode);
>
> Heh - this will become a lot of fun for real LE host as well as guest systems.
I am trying to understand the problem here, We want to byteswap opcode only if
it is mixed endian (host and guest are of different endianess) case?
> For now just remove the tswap and add a comment that this needs fixing for LE.
>
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0)
> > ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) {
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +uint32_t sc;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) ||
> > +sc != tswap32(debug_inst_opcode) ||
>
> Same here.
>
> In fact, neither of the 2 operations are in a fast path. Can't we just fetch
> the
> debug inst opcode on demand in a function here?
Ok will do that.
> That will allow for easier byte
> swapping depending on the guest's MSR.LE setting later as well.
>
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
> > {
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static struct HWBreakpoint {
> > +target_ulong addr;
> > +int type;
> > +} hw_breakpoint[6];
> > +
> > +static int nb_hw_breakpoint;
> > +static int nb_hw_watchpoint;
> > +static int max_hw_breakpoint = 4;
> > +static int max_hw_watchpoint = 2;
> > +
> > +void kvmppc_e500_hw_breakpoint_init(void)
> > +{
> > +max_hw_breakpoint = 2;
> > +max_hw_watchpoint = 2;
>
> Can we somehow get this information from kvm and set it in kvm_arch_init?
Will add one_reg to get this information.
> Worst
> case we'll have to look at the cpu class and derive it from there, but it
> really
> should liv
Re: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices
> -Original Message-
> From: qemu-devel-bounces+bharat.bhushan=freescale@nongnu.org [mailto:qemu-
> devel-bounces+bharat.bhushan=freescale@nongnu.org] On Behalf Of Alexander
> Graf
> Sent: Wednesday, June 04, 2014 5:59 PM
> To: qemu-...@nongnu.org
> Cc: qemu-devel@nongnu.org; eric.au...@linaro.org
> Subject: [Qemu-devel] [PATCH 4/5] PPC: e500: Support platform devices
>
> For e500 our approach to supporting platform devices is to create a simple
> bus from the guest's point of view within which we map platform devices
> dynamically.
>
> We allocate memory regions always within the "platform" hole in address
> space and map IRQs to predetermined IRQ lines that are reserved for platform
> device usage.
>
> This maps really nicely into device tree logic, so we can just tell the
> guest about our virtual simple bus in device tree as well.
>
> Signed-off-by: Alexander Graf
> ---
> default-configs/ppc-softmmu.mak | 1 +
> default-configs/ppc64-softmmu.mak | 1 +
> hw/ppc/e500.c | 221
> ++
> hw/ppc/e500.h | 1 +
> hw/ppc/e500plat.c | 1 +
> 5 files changed, 225 insertions(+)
>
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 33f8d84..d6ec8b9 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PREP=y
> CONFIG_MAC=y
> CONFIG_E500=y
> CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
> +CONFIG_PLATFORM=y
> # For PReP
> CONFIG_MC146818RTC=y
> CONFIG_ETSEC=y
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-
> softmmu.mak
> index 37a15b7..06677bf 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -45,6 +45,7 @@ CONFIG_PSERIES=y
> CONFIG_PREP=y
> CONFIG_MAC=y
> CONFIG_E500=y
> +CONFIG_PLATFORM=y
> CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
> # For pSeries
> CONFIG_XICS=$(CONFIG_PSERIES)
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 33d54b3..bc26215 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -36,6 +36,7 @@
> #include "exec/address-spaces.h"
> #include "qemu/host-utils.h"
> #include "hw/pci-host/ppce500.h"
> +#include "hw/platform/device.h"
>
> #define EPAPR_MAGIC(0x45504150)
> #define BINARY_DEVICE_TREE_FILE"mpc8544ds.dtb"
> @@ -47,6 +48,14 @@
>
> #define RAM_SIZES_ALIGN(64UL << 20)
>
> +#define E500_PLATFORM_BASE 0xF000ULL
> +#define E500_PLATFORM_HOLE (128ULL * 1024 * 1024) /* 128 MB */
> +#define E500_PLATFORM_PAGE_SHIFT 12
> +#define E500_PLATFORM_HOLE_PAGES (E500_PLATFORM_HOLE >> \
> +E500_PLATFORM_PAGE_SHIFT)
> +#define E500_PLATFORM_FIRST_IRQ5
How we come to value "5" ?
How it is ensured that this does not overlap with IRQ numbers taken by PCI
devices (with no-msi case)?
> +#define E500_PLATFORM_NUM_IRQS 10
> +
> /* TODO: parameterize */
> #define MPC8544_CCSRBAR_BASE 0xE000ULL
> #define MPC8544_CCSRBAR_SIZE 0x0010ULL
> @@ -122,6 +131,62 @@ static void dt_serial_create(void *fdt, unsigned long
> long
> offset,
> }
> }
>
> +typedef struct PlatformDevtreeData {
> +void *fdt;
> +const char *mpic;
> +int irq_start;
> +const char *node;
> +} PlatformDevtreeData;
> +
> +static int platform_device_create_devtree(Object *obj, void *opaque)
> +{
> +PlatformDevtreeData *data = opaque;
> +Object *dev;
> +PlatformDeviceState *pdev;
> +
> +dev = object_dynamic_cast(obj, TYPE_PLATFORM_DEVICE);
> +pdev = (PlatformDeviceState *)dev;
> +
> +if (!pdev) {
> +/* Container, traverse it for children */
> +return object_child_foreach(obj, platform_device_create_devtree,
> data);
> +}
> +
> +return 0;
> +}
> +
> +static void platform_create_devtree(void *fdt, const char *node, uint64_t
> addr,
> +const char *mpic, int irq_start,
> +int nr_irqs)
> +{
> +const char platcomp[] = "qemu,platform\0simple-bus";
> +PlatformDevtreeData data;
> +
> +/* Create a /platform node that we can put all devices into */
> +
> +qemu_fdt_add_subnode(fdt, node);
> +qemu_fdt_setprop(fdt, node, "compatible", platcomp, sizeof(platcomp));
> +qemu_fdt_setprop_string(fdt, node, "device_type", "platform");
> +
> +/* Our platform hole is less than 32bit big, so 1 cell is enough for
> address
> + and size */
> +qemu_fdt_setprop_cells(fdt, node, "#size-cells", 1);
> +qemu_fdt_setprop_cells(fdt, node, "#address-cells", 1);
> +qemu_fdt_setprop_cells(fdt, node, "ranges", 0, addr >> 32, addr,
> + E500_PLATFORM_HOLE);
> +
> +qemu_fdt_setprop_phandle(fdt, node, "interrupt-parent", mpic);
> +
> +/* Loop through all devices and create nodes for known ones */
> +
> +data.fdt =
Re: [Qemu-devel] [PATCH 3/3] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Friday, June 13, 2014 4:55 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
>
>
> On 12.06.14 09:05, bharat.bhus...@freescale.com wrote:
> >
> >> -Original Message-
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Wednesday, June 11, 2014 6:35 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3] ppc debug: Add debug stub support
> >>
> >> On 06/10/2014 05:06 PM, Bharat Bhushan wrote:
> >>> This patch adds software breakpoint, hardware breakpoint and
> >>> hardware watchpoint support for ppc. If the debug interrupt is not
> >>> handled then this is injected to guest.
> >>>
> >>> Signed-off-by: Bharat Bhushan
> >>> ---
> >>>hw/ppc/e500.c| 1 +
> >>>target-ppc/kvm.c | 304
> ++---
> >> --
> >>>target-ppc/kvm_ppc.h | 1 +
> >>>3 files changed, 278 insertions(+), 28 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..514c595
> >>> 100644
> >>> --- a/hw/ppc/e500.c
> >>> +++ b/hw/ppc/e500.c
> >>> @@ -853,6 +853,7 @@ void ppce500_init(MachineState *machine,
> >>> PPCE500Params
> >> *params)
> >>>if (kvm_enabled()) {
> >>>kvmppc_init();
> >>>}
> >>> +kvmppc_e500_hw_breakpoint_init();
> >>>}
> >>>
> >>>static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> >>> a/target-ppc/kvm.c b/target-ppc/kvm.c index 1d2384d..f5fbec6 100644
> >>> --- a/target-ppc/kvm.c
> >>> +++ b/target-ppc/kvm.c
> >>> @@ -38,6 +38,7 @@
> >>>#include "hw/ppc/ppc.h"
> >>>#include "sysemu/watchdog.h"
> >>>#include "trace.h"
> >>> +#include "exec/gdbstub.h"
> >>>
> >>>//#define DEBUG_KVM
> >>>
> >>> @@ -768,6 +769,38 @@ static int kvm_put_vpa(CPUState *cs)
> >>>
> >>>static int kvmppc_inject_debug_exception(CPUState *cs)
> >>>{
> >>> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>> +CPUPPCState *env = &cpu->env;
> >>> +struct kvm_sregs sregs;
> >>> +int ret;
> >>> +
> >>> +if (!cap_booke_sregs) {
> >>> +return -1;
> >>> +}
> >>> +
> >>> +ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> >>> +if (ret < 0) {
> >>> +return -1;
> >>> +}
> >>> +
> >> I don't think any of this code should ever run for non-e500, no?
> > You mean the code below in this function?
>
> Yeah :).
Why you think accessing sregs (cssr0/1, dsrr0/1 and ioctl) is e500 specific.
Are not these valid for 4xx as well?
>
> >
> >>> +if (sregs.u.e.features & KVM_SREGS_E_ED) {
> >> Hrm - we never seem to set E_ED in kvm?
> > Uhh, you are right. Going through the whole discussion about interrupt
> injection to guest I found that one patch missed for upstream.
> > Will send that patch
> >
> >>> +sregs.u.e.dsrr0 = env->nip;
> >>> +sregs.u.e.dsrr1 = env->msr;
> >>> +} else {
> >>> +sregs.u.e.csrr0 = env->nip;
> >>> +sregs.u.e.csrr1 = env->msr;
> >>> +}
> >>> +
> >>> +sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR;
> >>> +sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR];
> >>> +
> >>> +ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> >>> +if (ret < 0) {
> >>> +return -1;
> >>> +}
> >>> +
> >>> +env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG);
> >>> +
> >>>return 0;
> >>>}
> >>>
> >>> @@ -1275,6 +1308,239 @@ static int
> >>> kvmppc_handle_dcr_write(CPUPPCState *env,
> >> uint32_t dcrn, uint32_t dat
> >>>return 0;
> >>>}
> >>>
> >>> +int kvm_arch_insert_sw_breakpoint(CPU
Re: [Qemu-devel] [Qemu-ppc] [PATCH] qemu/target-ppc: software breakpoint support
> -Original Message-
> From: qemu-ppc-bounces+bharat.bhushan=freescale@nongnu.org
> [mailto:qemu-ppc-
> bounces+bharat.bhushan=freescale@nongnu.org] On Behalf Of Madhavan
> Srinivasan
> Sent: Sunday, June 15, 2014 2:38 AM
> To: ag...@suse.de; pau...@samba.org
> Cc: Madhavan Srinivasan; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: [Qemu-ppc] [PATCH] qemu/target-ppc: software breakpoint support
>
> This patch adds software breakpoint support for ppc. Patch
> allows to insert and remove sw breakpoints using Qemu
> gdbserver on ppc.
>
> Patch use "abs" instruction as sw breakpoint instruction.
> This instruction with primary opcode 31 and extended opcode 360
> has been dropped from PowerISA (in v2.01) and will generate an
> Emulation assistance interrupt in hypervisor when executed.
> KVM also has to be updated with this to make use of sw breakpoint.
There are already a patch "ppc debug: Add debug stub support" in review which
implements software breakpoint
In addition this patch also implements h/w breakpoints/watchpoints for BOOKE.
But I actually do not know how h/w breakpoints works on BOOK3S and you can help
me figuring out what can be shared between booke and book3s.
>
> Signed-off-by: Madhavan Srinivasan
> ---
> target-ppc/kvm.c | 38 +++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9974b10..fa4b4ec 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1201,6 +1201,17 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> uint32_t dcrn, uint32_t dat
> return 0;
> }
>
> +static int kvmppc_handle_debug(CPUState *cs, struct kvm_debug_exit_arch
> *arch_info)
> +{
> + int ret = -1;
> +
> + if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> + ret = EXCP_DEBUG;
> + }
> +
> + return ret;
> +}
> +
> int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -1240,7 +1251,10 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run
> *run)
> watchdog_perform_action();
> ret = 0;
> break;
> -
> +case KVM_EXIT_DEBUG:
> +DPRINTF("handle debug exit pc = 0x%lx\n", run->debug.arch.address);
> +ret = kvmppc_handle_debug(cs, &run->debug.arch);
> +break;
> default:
> fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> ret = -1;
> @@ -1901,14 +1915,29 @@ void kvm_arch_init_irq_routing(KVMState *s)
> {
> }
>
> +static const uint8_t dbg_int[] = { 0x7c, 0x00, 0x02, 0xd0};
This should not be hardcoded but we should get this from KVM. Please refer to
patch "ppc debug: Add debug stub support" to know how to get debug instruction.
Thanks
-Bharat
> int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct kvm_sw_breakpoint
> *bp)
> {
> -return -EINVAL;
> + if( cpu_memory_rw_debug(cpu, bp->pc, (uint8_t *)&bp->saved_insn,
> sizeof(dbg_int), 0) ||
> + cpu_memory_rw_debug(cpu, bp->pc, (uint8_t *)dbg_int,
> sizeof(dbg_int),
> 1) ){
> + return -EINVAL;
> + }
> + return 0;
> }
>
> int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct kvm_sw_breakpoint
> *bp)
> {
> -return -EINVAL;
> + uint8_t tmp[4];
> +
> + if (cpu_memory_rw_debug(cpu, bp->pc, tmp, sizeof(dbg_int), 0)) {
> + return -EINVAL;
> + } else if (memcmp(tmp, dbg_int, sizeof(dbg_int))) {
> + return -EINVAL;
> + } else if (cpu_memory_rw_debug(cpu, bp->pc, (uint8_t *)&bp->saved_insn,
> sizeof(dbg_int), 1)) {
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int
> type)
> @@ -1927,6 +1956,9 @@ void kvm_arch_remove_all_hw_breakpoints(void)
>
> void kvm_arch_update_guest_debug(CPUState *cpu, struct kvm_guest_debug *dbg)
> {
> + if (kvm_sw_breakpoints_active(cpu)) {
> + dbg->control |= KVM_GUESTDBG_USE_SW_BP;
> + }
> }
>
> struct kvm_get_htab_buf {
> --
> 1.8.3.1
>
Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 17, 2014 1:46 PM
> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>
>
> On 17.06.14 09:08, Bharat Bhushan wrote:
> > This patch adds software breakpoint, hardware breakpoint and hardware
> > watchpoint support for ppc. If the debug interrupt is not handled then
> > this is injected to guest.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > v1->v2:
> > - factored out e500 specific code based on exception model
> POWERPC_EXCP_BOOKE.
> > - Not supporting ppc440
> >
> > hw/ppc/e500.c| 3 +
> > target-ppc/kvm.c | 355
> > ++---
> --
> > target-ppc/kvm_ppc.h | 1 +
> > 3 files changed, 330 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..47caa84
> > 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -853,6 +853,9 @@ void ppce500_init(MachineState *machine, PPCE500Params
> *params)
> > if (kvm_enabled()) {
> > kvmppc_init();
> > }
> > +
> > +/* E500 supports 2 h/w breakpoints and 2 watchpoints */
> > +kvmppc_hw_breakpoint_init(2, 2);
>
> This does not belong into the machine file.
What about calling this from init_proc_e500() in target-ppc/translate_init.c ?
>
> > }
> >
> > static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> > a/target-ppc/kvm.c b/target-ppc/kvm.c index 70f77d1..994a618 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -38,6 +38,7 @@
> > #include "hw/ppc/ppc.h"
> > #include "sysemu/watchdog.h"
> > #include "trace.h"
> > +#include "exec/gdbstub.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -759,11 +760,55 @@ static int kvm_put_vpa(CPUState *cs)
> > }
> > #endif /* TARGET_PPC64 */
> >
> > -static int kvmppc_inject_debug_exception(CPUState *cs)
> > +static int kvmppc_e500_inject_debug_exception(CPUState *cs)
> > {
> > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +CPUPPCState *env = &cpu->env;
> > +struct kvm_sregs sregs;
> > +int ret;
> > +
> > +if (!cap_booke_sregs) {
> > +return -1;
> > +}
> > +
> > +ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> > +if (ret < 0) {
> > +return -1;
> > +}
> > +
> > +if (sregs.u.e.features & KVM_SREGS_E_ED) {
> > +sregs.u.e.dsrr0 = env->nip;
> > +sregs.u.e.dsrr1 = env->msr;
> > +} else {
> > +sregs.u.e.csrr0 = env->nip;
> > +sregs.u.e.csrr1 = env->msr;
> > +}
> > +
> > +sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR;
> > +sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR];
> > +
> > +ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> > +if (ret < 0) {
> > +return -1;
> > +}
> > +
> > +env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DEBUG);
>
> I think it makes sense to move this into kvmppc_inject_exception(). Then we
> have
> everything dealing with pending_interrupts in one spot.
Will do
>
> > +
> > return 0;
> > }
> >
> > +static int kvmppc_inject_debug_exception(CPUState *cs) {
> > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +CPUPPCState *env = &cpu->env;
> > +
> > +if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > +return kvmppc_e500_inject_debug_exception(cs);
> > +}
>
> Yes, exactly the way I wanted to see it :). Please make this a switch though -
> that'll make it easier for others to plug in later.
Will do
>
> > +
> > +return -1;
> > +}
> > +
> > static void kvmppc_inject_exception(CPUState *cs)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1268,6 +1313,276 @@
> > static int kvmppc_handle_dcr_write(CPUPPCState *env, uint32_t dcrn, uint32_t
> dat
> > return 0;
> > }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +/* Mixed endian case is not handled */
> > +uint32_t sc = debug_inst_opcode;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0)
> > ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) {
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +uint32_t sc;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) ||
> > +sc != debug_inst_opcode ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
> > {
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +#define MAX_HW_BKPTS 4
> > +
> > +static struct HWBreakpoint {
> > +target_ulong addr;
> > +int type;
> > +} hw_breakpoint[MAX_HW_BKPTS];
>
> This struct contains both watchpoints and breakpoints, no? It really should be
> named accordingly. Maybe only call them points? Not sure :).
May be hw_debug_points/ hw_wb_poi
Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 17, 2014 3:20 PM
> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>
>
> On 17.06.14 11:14, bharat.bhus...@freescale.com wrote:
> >
> >> -Original Message-
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Tuesday, June 17, 2014 1:46 PM
> >> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>
> >>
> >> On 17.06.14 09:08, Bharat Bhushan wrote:
> >>> This patch adds software breakpoint, hardware breakpoint and
> >>> hardware watchpoint support for ppc. If the debug interrupt is not
> >>> handled then this is injected to guest.
> >>>
> >>> Signed-off-by: Bharat Bhushan
> >>> ---
> >>> v1->v2:
> >>>- factored out e500 specific code based on exception model
> >> POWERPC_EXCP_BOOKE.
> >>>- Not supporting ppc440
> >>>
> >>>hw/ppc/e500.c| 3 +
> >>>target-ppc/kvm.c | 355
> ++---
> >> --
> >>>target-ppc/kvm_ppc.h | 1 +
> >>>3 files changed, 330 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..47caa84
> >>> 100644
> >>> --- a/hw/ppc/e500.c
> >>> +++ b/hw/ppc/e500.c
> >>> @@ -853,6 +853,9 @@ void ppce500_init(MachineState *machine,
> >>> PPCE500Params
> >> *params)
> >>>if (kvm_enabled()) {
> >>>kvmppc_init();
> >>>}
> >>> +
> >>> +/* E500 supports 2 h/w breakpoints and 2 watchpoints */
> >>> +kvmppc_hw_breakpoint_init(2, 2);
> >> This does not belong into the machine file.
> > What about calling this from init_proc_e500() in
> > target-ppc/translate_init.c ?
>
> I think it makes sense to leave it in KVM land. Why not do it lazily on
> insert_hw_breakpoint?
You mean setting in kvm_arch_insert_hw_breakpoint() when called first time;
something like:
static bool init = 0;
if (!init) {
if (env->excp_model == POWERPC_EXCP_BOOKE) {
max_hw_breakpoint = 2;
max_hw_watchpoint = 2;
} else
// Add for book3s max_hw_watchpoint = 1;
}
init = 1;
}
>
> >
> >>>}
> >>>
> >>>static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> >>> a/target-ppc/kvm.c b/target-ppc/kvm.c index 70f77d1..994a618 100644
> >>> --- a/target-ppc/kvm.c
> >>> +++ b/target-ppc/kvm.c
> >>> @@ -38,6 +38,7 @@
> >>>#include "hw/ppc/ppc.h"
> >>>#include "sysemu/watchdog.h"
> >>>#include "trace.h"
> >>> +#include "exec/gdbstub.h"
> >>>
> >>>//#define DEBUG_KVM
> >>>
> >>> @@ -759,11 +760,55 @@ static int kvm_put_vpa(CPUState *cs)
> >>>}
> >>>#endif /* TARGET_PPC64 */
> >>>
> >>> -static int kvmppc_inject_debug_exception(CPUState *cs)
> >>> +static int kvmppc_e500_inject_debug_exception(CPUState *cs)
> >>>{
> >>> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>> +CPUPPCState *env = &cpu->env;
> >>> +struct kvm_sregs sregs;
> >>> +int ret;
> >>> +
> >>> +if (!cap_booke_sregs) {
> >>> +return -1;
> >>> +}
> >>> +
> >>> +ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> >>> +if (ret < 0) {
> >>> +return -1;
> >>> +}
> >>> +
> >>> +if (sregs.u.e.features & KVM_SREGS_E_ED) {
> >>> +sregs.u.e.dsrr0 = env->nip;
> >>> +sregs.u.e.dsrr1 = env->msr;
> >>> +} else {
> >>> +sregs.u.e.csrr0 = env->nip;
> >>> +sregs.u.e.csrr1 = env->msr;
> >>> +}
> >>> +
> >>> +sregs.u.e.update_special = KVM_SREGS_E_UPDATE_DBSR;
> >>> +sregs.u.e.dbsr = env->spr[SPR_BOOKE_DBSR];
> >>> +
> >>> +ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> >>> +if (ret < 0) {
>
Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 17, 2014 4:33 PM
> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>
>
> On 17.06.14 13:01, bharat.bhus...@freescale.com wrote:
> >>>>>>> +static void kvm_arch_e500_update_guest_debug(CPUState *cs,
> >>>>>>> + struct
> >>>>>>> +kvm_guest_debug
> >>>>>>> +*dbg) {
> >>>>>>> +int n;
> >>>>>>> +
> >>>>>>> +if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> >>>>>>> +dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
> >>>>>>> +memset(dbg->arch.bp, 0, sizeof(dbg->arch.bp));
> >>>>>>> +for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint;
> >>>>>>> + n++) {
> >>>>>> Boundary check against dbg->arch.bp missing.
> >>>>> Did not get, what you mean by " dbg->arch.bp missing" ?
> >>>> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
> >>>> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite
> >>>> memory we don't want to overwrite.
> >>> Actually this will never overflow here because nb_hw_breakpoint and
> >> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> >>> Do you thing that to be double safe we can add a check?
> >> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
> >> What if nb_hw_breakpoint becomes 17?
> > nb_hw_breakpoint can never be more than max_hw_breakpoint, how
> nb_hw_breakpoint can be 17 ?
>
> Someone comes along and bumps up max_hw_breakpoint to 17?
You mean some buggy code in qemu does this?
Thanks
-Bharat
> Just add an
> assert() somewhere that makes sure we can't run over bp :).
>
>
> Alex
Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support
> > +static void kvm_arch_e500_update_guest_debug(CPUState *cs,
> > + struct
> > +kvm_guest_debug
> > +*dbg) {
> > +int n;
> > +
> > +if (nb_hw_breakpoint + nb_hw_watchpoint > 0) {
> > +dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW_BP;
> > +memset(dbg->arch.bp, 0, sizeof(dbg->arch.bp));
> > +for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++)
> > + {
> Boundary check against dbg->arch.bp missing.
> >>> Did not get, what you mean by " dbg->arch.bp missing" ?
> >> dbg->arch.bp is an array of a certain size. If nb_hw_breakpoint +
> >> nb_hw_watchpoint > ARRAY_SIZE(dbg->arch.bp) we might overwrite memory
> >> we don't want to overwrite.
> > Actually this will never overflow here because nb_hw_breakpoint and
> nb_hw_watchpoint overflow in taken care in in hw_insert_breakpoint().
> > Do you thing that to be double safe we can add a check?
>
> We only check against an overflow of hw_breakpoint[], not dbg->arch.bp.
> What if nb_hw_breakpoint becomes 17?
nb_hw_breakpoint can never be more than max_hw_breakpoint, how nb_hw_breakpoint
can be 17 ?
Thanks
-Bharat
Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 17, 2014 4:14 PM
> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>
>
> On 17.06.14 12:40, bharat.bhus...@freescale.com wrote:
> >
> >> -Original Message-
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Tuesday, June 17, 2014 3:20 PM
> >> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>
> >>
> >> On 17.06.14 11:14, bharat.bhus...@freescale.com wrote:
> >>>> -Original Message-
> >>>> From: Alexander Graf [mailto:ag...@suse.de]
> >>>> Sent: Tuesday, June 17, 2014 1:46 PM
> >>>> To: Bhushan Bharat-R65777; qemu-...@nongnu.org;
> >>>> qemu-devel@nongnu.org
> >>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>>>
> >>>>
> >>>> On 17.06.14 09:08, Bharat Bhushan wrote:
> >>>>> This patch adds software breakpoint, hardware breakpoint and
> >>>>> hardware watchpoint support for ppc. If the debug interrupt is not
> >>>>> handled then this is injected to guest.
> >>>>>
> >>>>> Signed-off-by: Bharat Bhushan
> >>>>> ---
> >>>>> v1->v2:
> >>>>> - factored out e500 specific code based on exception model
> >>>> POWERPC_EXCP_BOOKE.
> >>>>> - Not supporting ppc440
> >>>>>
> >>>>> hw/ppc/e500.c| 3 +
> >>>>> target-ppc/kvm.c | 355
> >> ++---
> >>>> --
> >>>>> target-ppc/kvm_ppc.h | 1 +
> >>>>> 3 files changed, 330 insertions(+), 29 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index a973c18..47caa84
> >>>>> 100644
> >>>>> --- a/hw/ppc/e500.c
> >>>>> +++ b/hw/ppc/e500.c
> >>>>> @@ -853,6 +853,9 @@ void ppce500_init(MachineState *machine,
> >>>>> PPCE500Params
> >>>> *params)
> >>>>> if (kvm_enabled()) {
> >>>>> kvmppc_init();
> >>>>> }
> >>>>> +
> >>>>> +/* E500 supports 2 h/w breakpoints and 2 watchpoints */
> >>>>> +kvmppc_hw_breakpoint_init(2, 2);
> >>>> This does not belong into the machine file.
> >>> What about calling this from init_proc_e500() in
> >>> target-ppc/translate_init.c
> ?
> >> I think it makes sense to leave it in KVM land. Why not do it lazily
> >> on insert_hw_breakpoint?
> > You mean setting in kvm_arch_insert_hw_breakpoint() when called first time;
> something like:
> >
> > static bool init = 0;
> >
> > if (!init) {
> > if (env->excp_model == POWERPC_EXCP_BOOKE) {
> > max_hw_breakpoint = 2;
> > max_hw_watchpoint = 2;
> > } else
> >// Add for book3s max_hw_watchpoint = 1;
> > }
> > init = 1;
> > }
>
> I would probably reuse max_hw_breakpoint as a hint whether it's initialized
> and
> put all of this into a small function, but yes :).
Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get
"env" reference in this function. Prototype of this is:
int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len, int
type);
I will suggest that we initialize this from kvm_arch_init_vcpu(). This way we
are still in KVM zone.
Thanks
-Bharat
>
> >
> >>>>> }
> >>>>>
> >>>>> static int e500_ccsr_initfn(SysBusDevice *dev) diff --git
> >>>>> a/target-ppc/kvm.c b/target-ppc/kvm.c index 70f77d1..994a618
> >>>>> 100644
> >>>>> --- a/target-ppc/kvm.c
> >>>>> +++ b/target-ppc/kvm.c
> >>>>> @@ -38,6 +38,7 @@
> >>>>> #include "hw/ppc/ppc.h"
> >>>>> #include "sysemu/watchdog.h"
> >>>>> #include "trace.h"
> >>>>> +#include "exec/gdbstub.h"
> >>>>>
> >>>>> //#define DEBUG_KVM
> >>>>&g
Re: [Qemu-devel] [PATCH 3/3 v2] ppc debug: Add debug stub support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 24, 2014 5:02 PM
> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
>
>
> On 18.06.14 06:39, bharat.bhus...@freescale.com wrote:
> >
> >> -Original Message-
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Tuesday, June 17, 2014 4:14 PM
> >> To: Bhushan Bharat-R65777; qemu-...@nongnu.org; qemu-devel@nongnu.org
> >> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>
> >>
> >> On 17.06.14 12:40, bharat.bhus...@freescale.com wrote:
> >>>> -Original Message-
> >>>> From: Alexander Graf [mailto:ag...@suse.de]
> >>>> Sent: Tuesday, June 17, 2014 3:20 PM
> >>>> To: Bhushan Bharat-R65777; qemu-...@nongnu.org;
> >>>> qemu-devel@nongnu.org
> >>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>>>
> >>>>
> >>>> On 17.06.14 11:14, bharat.bhus...@freescale.com wrote:
> >>>>>> -Original Message-
> >>>>>> From: Alexander Graf [mailto:ag...@suse.de]
> >>>>>> Sent: Tuesday, June 17, 2014 1:46 PM
> >>>>>> To: Bhushan Bharat-R65777; qemu-...@nongnu.org;
> >>>>>> qemu-devel@nongnu.org
> >>>>>> Subject: Re: [PATCH 3/3 v2] ppc debug: Add debug stub support
> >>>>>>
> >>>>>>
> >>>>>> On 17.06.14 09:08, Bharat Bhushan wrote:
> >>>>>>> This patch adds software breakpoint, hardware breakpoint and
> >>>>>>> hardware watchpoint support for ppc. If the debug interrupt is
> >>>>>>> not handled then this is injected to guest.
> >>>>>>>
> >>>>>>> Signed-off-by: Bharat Bhushan
> >>>>>>> ---
> >>>>>>> v1->v2:
> >>>>>>> - factored out e500 specific code based on exception model
> >>>>>> POWERPC_EXCP_BOOKE.
> >>>>>>> - Not supporting ppc440
> >>>>>>>
> >>>>>>> hw/ppc/e500.c| 3 +
> >>>>>>> target-ppc/kvm.c | 355
> >>>> ++---
> >>>>>> --
> >>>>>>> target-ppc/kvm_ppc.h | 1 +
> >>>>>>> 3 files changed, 330 insertions(+), 29 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c index
> >>>>>>> a973c18..47caa84
> >>>>>>> 100644
> >>>>>>> --- a/hw/ppc/e500.c
> >>>>>>> +++ b/hw/ppc/e500.c
> >>>>>>> @@ -853,6 +853,9 @@ void ppce500_init(MachineState *machine,
> >>>>>>> PPCE500Params
> >>>>>> *params)
> >>>>>>> if (kvm_enabled()) {
> >>>>>>> kvmppc_init();
> >>>>>>> }
> >>>>>>> +
> >>>>>>> +/* E500 supports 2 h/w breakpoints and 2 watchpoints */
> >>>>>>> +kvmppc_hw_breakpoint_init(2, 2);
> >>>>>> This does not belong into the machine file.
> >>>>> What about calling this from init_proc_e500() in
> >>>>> target-ppc/translate_init.c
> >> ?
> >>>> I think it makes sense to leave it in KVM land. Why not do it
> >>>> lazily on insert_hw_breakpoint?
> >>> You mean setting in kvm_arch_insert_hw_breakpoint() when called
> >>> first time;
> >> something like:
> >>> static bool init = 0;
> >>>
> >>> if (!init) {
> >>> if (env->excp_model == POWERPC_EXCP_BOOKE) {
> >>> max_hw_breakpoint = 2;
> >>> max_hw_watchpoint = 2;
> >>> } else
> >>> // Add for book3s max_hw_watchpoint = 1;
> >>>}
> >>>init = 1;
> >>> }
> >> I would probably reuse max_hw_breakpoint as a hint whether it's
> >> initialized and put all of this into a small function, but yes :).
> > Ahh, we cannot do this in kvm_arch_insert_hw_breakpoint() as we can not get
> "env" reference in this function. Prototype of this is:
> > int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong len,
> > int type);
>
> Just use first_cpu? :)
>
> >
> > I will suggest that we initialize this from kvm_arch_init_vcpu(). This way
> > we
> are still in KVM zone.
>
> That works too, yes.
V3 version of path is based on this :)
Thanks
-Bharat
>
>
>
> Alex
Re: [Qemu-devel] [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 24, 2014 6:35 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ma...@linux.vnet.ibm.com
> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
>
>
> On 24.06.14 14:10, Bharat Bhushan wrote:
> > This patch allow insert/remove software breakpoint
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > target-ppc/kvm.c | 71
> > +-
> --
> > 1 file changed, 57 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 5238de7..8e2dbb3 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1317,6 +1317,53 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> uint32_t dcrn, uint32_t dat
> > return 0;
> > }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +/* Mixed endian case is not handled */
> > +uint32_t sc = debug_inst_opcode;
>
> What if debug_inst_opcode has never been set (thus is 0)?
Can "0" be a debug_inst_code ?
> In that case we should fail the insert, no?
Yes, will checking for "0" is sufficient or we need a cap_ also ?
Thanks
-Bharat
>
>
> Alex
Re: [Qemu-devel] [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 24, 2014 6:50 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ma...@linux.vnet.ibm.com
> Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support
>
>
> On 24.06.14 14:10, Bharat Bhushan wrote:
> > This patch adds hardware breakpoint and hardware watchpoint support
> > for ppc. If the debug interrupt is not handled then this is injected
> > to guest.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > v2->v3
> > - Shared as much code as much possible for futuristic book3s support
> > - Initializing number of hw breakpoint/watchpoints from KVM world
> > - Other minor cleanup/fixes
> >
> > target-ppc/kvm.c | 248
> > +++--
> --
> > 1 file changed, 233 insertions(+), 15 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 8e2dbb3..4fb0efd 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -38,6 +38,7 @@
> > #include "hw/ppc/ppc.h"
> > #include "sysemu/watchdog.h"
> > #include "trace.h"
> > +#include "exec/gdbstub.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -410,6 +411,44 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > return ppc_get_vcpu_dt_id(POWERPC_CPU(cpu));
> > }
> >
> > +/* e500 supports 2 h/w breakpoint and 2 watchpoint.
> > + * book3s supports only 1 watchpoint, so array size
> > + * of 4 is sufficient for now.
> > + */
> > +#define MAX_HW_BKPTS 4
> > +
> > +static struct HWBreakpoint {
> > +target_ulong addr;
> > +int type;
> > +} hw_debug_points[MAX_HW_BKPTS];
> > +
> > +static CPUWatchpoint hw_watchpoint;
> > +
> > +/* Default there is no breakpoint and watchpoint supported */ static
> > +int max_hw_breakpoint; static int max_hw_watchpoint; static int
> > +nb_hw_breakpoint; static int nb_hw_watchpoint;
> > +
> > +static void kvmppc_hw_debug_points_init(CPUPPCState *cenv) {
> > +static bool initialize = true;
> > +
> > +if (initialize) {
> > +if (cenv->excp_model == POWERPC_EXCP_BOOKE) {
> > +max_hw_breakpoint = 2;
> > +max_hw_watchpoint = 2;
> > +}
> > +
> > +initialize = false;
> > +}
> > +
> > +if ((max_hw_breakpoint + max_hw_watchpoint) > MAX_HW_BKPTS) {
> > +fprintf(stderr, "Error initializing h/w breakpoints\n");
> > +return;
> > +}
> > +}
> > +
> > int kvm_arch_init_vcpu(CPUState *cs)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -437,6 +476,7 @@ int
> > kvm_arch_init_vcpu(CPUState *cs)
> > }
> >
> > kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
> > +kvmppc_hw_debug_points_init(cenv);
> >
> > return ret;
> > }
> > @@ -1343,24 +1383,216 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs,
> struct kvm_sw_breakpoint *bp)
> > return 0;
> > }
> >
> > +static int find_hw_breakpoint(target_ulong addr, int type) {
> > +int n;
> > +
> > +assert((nb_hw_breakpoint + nb_hw_watchpoint)
> > + <= ARRAY_SIZE(hw_debug_points));
> > +
> > +for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
> > +if (hw_debug_points[n].addr == addr && hw_debug_points[n].type ==
> type) {
> > +return n;
> > +}
> > +}
> > +
> > +return -1;
> > +}
> > +
> > +static int find_hw_watchpoint(target_ulong addr, int *flag) {
> > +int n;
> > +
> > +n = find_hw_breakpoint(addr, GDB_WATCHPOINT_ACCESS);
> > +if (n >= 0) {
> > +*flag = BP_MEM_ACCESS;
> > +return n;
> > +}
> > +
> > +n = find_hw_breakpoint(addr, GDB_WATCHPOINT_WRITE);
> > +if (n >= 0) {
> > +*flag = BP_MEM_WRITE;
> > +return n;
> > +}
> > +
> > +n = find_hw_breakpoint(addr, GDB_WATCHPOINT_READ);
> > +if (n >= 0) {
> > +*flag = BP_MEM_READ;
> > +return n;
> > +}
> > +
> > +return -1;
> > +}
> > +
> > +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> > + target_ulong len, int type) {
> > +assert((nb_hw_breakpoint + nb_hw_watchpoint)
> > + <= ARRAY_SIZE(hw_debug_points));
> > +
> > +hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr;
> > +hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].type = type;
>
> Imagine the following:
>
>nb_hw_breakpoint = 2
>nb_hw_watchpoint = 2
>
> The assert above succeeds, because 4 <= 4. However, the array shuffling below
> accesses memory that is out of bounds: hw_debug_points[4].
Right, this is just " < ";
but why not this crashed for me :( ?
>
> > +
> > +switch (type) {
> > +case GDB_BREAKPOINT_HW:
> > +if (nb_hw_breakpoint >= max_hw_breakpoint) {
> > +return -ENOBUFS;
> > +}
> > +
> > +if (find_hw_breakpoint(addr, type) >= 0) {
> > +return -EEXIST;
> > +}
> > +
> > +nb_hw_breakpoint++;
> > +break;
> >
Re: [Qemu-devel] [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, June 24, 2014 8:21 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ma...@linux.vnet.ibm.com
> Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint support
>
>
> On 24.06.14 16:37, bharat.bhus...@freescale.com wrote:
> >
> >> -Original Message-
> >> From: Alexander Graf [mailto:ag...@suse.de]
> >> Sent: Tuesday, June 24, 2014 6:50 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org;
> >> ma...@linux.vnet.ibm.com
> >> Subject: Re: [PATCH 5/5 v3][RESEND] ppc: Add hw breakpoint watchpoint
> >> support
> >>
> >>
> >> On 24.06.14 14:10, Bharat Bhushan wrote:
> >>> This patch adds hardware breakpoint and hardware watchpoint support
> >>> for ppc. If the debug interrupt is not handled then this is injected
> >>> to guest.
> >>>
> >>> Signed-off-by: Bharat Bhushan
> >>> ---
> >>> v2->v3
> >>>- Shared as much code as much possible for futuristic book3s support
> >>>- Initializing number of hw breakpoint/watchpoints from KVM world
> >>>- Other minor cleanup/fixes
> >>>
> >>>target-ppc/kvm.c | 248
> >>> +++--
> >> --
> >>>1 file changed, 233 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> >>> 8e2dbb3..4fb0efd 100644
> >>> --- a/target-ppc/kvm.c
> >>> +++ b/target-ppc/kvm.c
> >>> @@ -38,6 +38,7 @@
> >>>#include "hw/ppc/ppc.h"
> >>>#include "sysemu/watchdog.h"
> >>>#include "trace.h"
> >>> +#include "exec/gdbstub.h"
> >>>
> >>>//#define DEBUG_KVM
> >>>
> >>> @@ -410,6 +411,44 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> >>>return ppc_get_vcpu_dt_id(POWERPC_CPU(cpu));
> >>>}
> >>>
> >>> +/* e500 supports 2 h/w breakpoint and 2 watchpoint.
> >>> + * book3s supports only 1 watchpoint, so array size
> >>> + * of 4 is sufficient for now.
> >>> + */
> >>> +#define MAX_HW_BKPTS 4
> >>> +
> >>> +static struct HWBreakpoint {
> >>> +target_ulong addr;
> >>> +int type;
> >>> +} hw_debug_points[MAX_HW_BKPTS];
> >>> +
> >>> +static CPUWatchpoint hw_watchpoint;
> >>> +
> >>> +/* Default there is no breakpoint and watchpoint supported */
> >>> +static int max_hw_breakpoint; static int max_hw_watchpoint; static
> >>> +int nb_hw_breakpoint; static int nb_hw_watchpoint;
> >>> +
> >>> +static void kvmppc_hw_debug_points_init(CPUPPCState *cenv) {
> >>> +static bool initialize = true;
> >>> +
> >>> +if (initialize) {
> >>> +if (cenv->excp_model == POWERPC_EXCP_BOOKE) {
> >>> +max_hw_breakpoint = 2;
> >>> +max_hw_watchpoint = 2;
> >>> +}
> >>> +
> >>> +initialize = false;
> >>> +}
> >>> +
> >>> +if ((max_hw_breakpoint + max_hw_watchpoint) > MAX_HW_BKPTS) {
> >>> +fprintf(stderr, "Error initializing h/w breakpoints\n");
> >>> +return;
> >>> +}
> >>> +}
> >>> +
> >>>int kvm_arch_init_vcpu(CPUState *cs)
> >>>{
> >>>PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -437,6 +476,7 @@ int
> >>> kvm_arch_init_vcpu(CPUState *cs)
> >>>}
> >>>
> >>>kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST,
> >>> &debug_inst_opcode);
> >>> +kvmppc_hw_debug_points_init(cenv);
> >>>
> >>>return ret;
> >>>}
> >>> @@ -1343,24 +1383,216 @@ int kvm_arch_remove_sw_breakpoint(CPUState
> >>> *cs,
> >> struct kvm_sw_breakpoint *bp)
> >>>return 0;
> >>>}
> >>>
> >>> +static int find_hw_breakpoint(target_ulong addr, int type) {
> >>> +int n;
> >>> +
> >>> +assert((nb_hw_breakpoint + nb_hw_watchpoint)
> >>> + <= ARRAY_SIZE(hw
Re: [Qemu-devel] [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
> -Original Message-
> From: Madhavan Srinivasan [mailto:ma...@linux.vnet.ibm.com]
> Sent: Tuesday, June 24, 2014 8:59 PM
> To: Bhushan Bharat-R65777; ag...@suse.de
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 4/5 v3][RESEND] ppc: Add software breakpoint support
>
> On Tuesday 24 June 2014 05:40 PM, Bharat Bhushan wrote:
> > This patch allow insert/remove software breakpoint
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > target-ppc/kvm.c | 71
> > +---
> > 1 file changed, 57 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 5238de7..8e2dbb3 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1317,6 +1317,53 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> uint32_t dcrn, uint32_t dat
> > return 0;
> > }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +/* Mixed endian case is not handled */
> > +uint32_t sc = debug_inst_opcode;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0)
> > ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 1)) {
>
> Instead of hard coding, can we use sizeof ()?
Yes
>
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +uint32_t sc;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, 4, 0) ||
> > +sc != debug_inst_opcode ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1))
> > {
> > +return -EINVAL;
> > +}
> > +
>
> Same. Can we use sizeof?
Yes
>
> > +return 0;
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
> > +*dbg) {
> > +/* Software Breakpoint updates */
> > +if (kvm_sw_breakpoints_active(cs)) {
> > +dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> > +}
> > +}
> > +
> > +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) {
> > +CPUState *cs = CPU(cpu);
> > +struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> > +int handle = 0;
> > +
> > +if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> > +handle = 1;
> > +}
> > +
> > +return handle;
> > +}
> > +
> > int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run) {
> > PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1357,6 +1404,16 @@ int
> > kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> > ret = 0;
> > break;
> >
> > +case KVM_EXIT_DEBUG:
> > +DPRINTF("handle debug exception\n");
> > +if (kvm_handle_debug(cpu, run)) {
> > +ret = EXCP_DEBUG;
> > +break;
> > +}
> > +/* re-enter, this exception was guest-internal */
>
> Kindly can you explain when this will happen?
If the debug interrupt condition (breakpoint/watchpoint etc) is not set by
qemu, i.e that is set by guest.
Thanks
-Bharat
>
> > +ret = 0;
> > +break;
> > +
> > default:
> > fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> > ret = -1;
> > @@ -2044,16 +2101,6 @@ void kvm_arch_init_irq_routing(KVMState *s) {
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > -return -EINVAL;
> > -}
> > -
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cpu, struct
> > kvm_sw_breakpoint *bp) -{
> > -return -EINVAL;
> > -}
> > -
> > int kvm_arch_insert_hw_breakpoint(target_ulong addr, target_ulong
> > len, int type) {
> > return -EINVAL;
> > @@ -2068,10 +2115,6 @@ void kvm_arch_remove_all_hw_breakpoints(void)
> > {
> > }
> >
> > -void kvm_arch_update_guest_debug(CPUState *cpu, struct
> > kvm_guest_debug *dbg) -{ -}
> > -
> > struct kvm_get_htab_buf {
> > struct kvm_get_htab_header header;
> > /*
> >
Re: [Qemu-devel] [PATCH v5 07/10] hw/vfio/platform: add vfio-platform support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Monday, August 11, 2014 3:06 PM
> To: Eric Auger; eric.au...@st.com; christoffer.d...@linaro.org; qemu-
> de...@nongnu.org; Phillips Kim-R1AAHA; a.r...@virtualopensystems.com
> Cc: will.dea...@arm.com; kvm...@lists.cs.columbia.edu;
> alex.william...@redhat.com; Bhushan Bharat-R65777; peter.mayd...@linaro.org;
> Yoder Stuart-B08248; a.mota...@virtualopensystems.com; patc...@linaro.org;
> joel.sch...@amd.com; Kim Phillips
> Subject: Re: [PATCH v5 07/10] hw/vfio/platform: add vfio-platform support
>
>
> On 09.08.14 16:25, Eric Auger wrote:
> > Minimal VFIO platform implementation supporting
> > - register space user mapping,
> > - IRQ assignment based on eventfds handled on qemu side.
> >
> > irqfd kernel acceleration comes in a subsequent patch.
> >
> > Signed-off-by: Kim Phillips
> > Signed-off-by: Eric Auger
> >
> > ---
> >
> > v4 -> v5:
> > - vfio-plaform.h included first
> > - cleanup error handling in *populate*, vfio_get_device,
> >vfio_enable_intp
> > - vfio_put_device not called anymore
> > - add some includes to follow vfio policy
> >
> > v3 -> v4:
> > [Eric Auger]
> > - merge of "vfio: Add initial IRQ support in platform device"
> >to get a full functional patch although perfs are limited.
> > - removal of unrealize function since I currently understand
> >it is only used with device hot-plug feature.
> >
> > v2 -> v3:
> > [Eric Auger]
> > - further factorization between PCI and platform (VFIORegion,
> >VFIODevice). same level of functionality.
> >
> > <= v2:
> > [Kim Philipps]
> > - Initial Creation of the device supporting register space mapping
> > ---
> > hw/vfio/Makefile.objs | 1 +
> > hw/vfio/platform.c | 517
>
> > include/hw/vfio/vfio-platform.h | 77 ++
> > 3 files changed, 595 insertions(+)
> > create mode 100644 hw/vfio/platform.c
> > create mode 100644 include/hw/vfio/vfio-platform.h
> >
> > diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> > index e31f30e..c5c76fe 100644
> > --- a/hw/vfio/Makefile.objs
> > +++ b/hw/vfio/Makefile.objs
> > @@ -1,4 +1,5 @@
> > ifeq ($(CONFIG_LINUX), y)
> > obj-$(CONFIG_SOFTMMU) += common.o
> > obj-$(CONFIG_PCI) += pci.o
> > +obj-$(CONFIG_SOFTMMU) += platform.o
> > endif
> > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> > new file mode 100644
> > index 000..f1a1b55
> > --- /dev/null
> > +++ b/hw/vfio/platform.c
> > @@ -0,0 +1,517 @@
> > +/*
> > + * vfio based device assignment support - platform devices
> > + *
> > + * Copyright Linaro Limited, 2014
> > + *
> > + * Authors:
> > + * Kim Phillips
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2. See
> > + * the COPYING file in the top-level directory.
> > + *
> > + * Based on vfio based PCI device assignment support:
> > + * Copyright Red Hat, Inc. 2012
> > + */
> > +
> > +#include
> > +#include
> > +
> > +#include "hw/vfio/vfio-platform.h"
> > +#include "qemu/error-report.h"
> > +#include "qemu/range.h"
> > +#include "sysemu/sysemu.h"
> > +#include "exec/memory.h"
> > +#include "qemu/queue.h"
> > +#include "hw/sysbus.h"
> > +
> > +extern const MemoryRegionOps vfio_region_ops;
> > +extern const MemoryListener vfio_memory_listener;
> > +extern QLIST_HEAD(, VFIOGroup) group_list;
> > +extern QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
> > +void vfio_put_device(VFIOPlatformDevice *vdev);
> > +
> > +/*
> > + * It is mandatory to pass a VFIOPlatformDevice since VFIODevice
> > + * is not a QOM Object and cannot be passed to memory region functions
> > +*/
> > +static void vfio_map_region(VFIOPlatformDevice *vdev, int nr)
> > +{
> > +VFIORegion *region = vdev->regions[nr];
> > +unsigned size = region->size;
> > +char name[64];
> > +
> > +if (!size) {
> > +return;
> > +}
> > +
> > +snprintf(name, sizeof(name), "VFIO %s region %d",
> > + vdev->vbasedev.name, nr);
> > +
> > +/* A "slow" read/write mapping underlies all regions */
> > +memory_region_init_io(®ion->mem, OBJECT(vdev), &vfio_region_ops,
> > + region, name, size);
> > +
> > +strncat(name, " mmap", sizeof(name) - strlen(name) - 1);
> > +
> > +if (vfio_mmap_region(OBJECT(vdev), region, ®ion->mem,
> > + ®ion->mmap_mem, ®ion->mmap, size, 0, name))
> > {
> > +error_report("%s unsupported. Performance may be slow", name);
> > +}
> > +}
> > +
> > +static void print_regions(VFIOPlatformDevice *vdev)
> > +{
> > +int i;
> > +
> > +DPRINTF("Device \"%s\" counts %d region(s):\n",
> > + vdev->vbasedev.name, vdev->vbasedev.num_regions);
> > +
> > +for (i = 0; i < vdev->vbasedev.num_regions; i++) {
> > +DPRINTF("- region %d flags = 0x%lx, size = 0x%lx, "
> > +"fd= %d, offset = 0x%lx\n",
> > +vdev->regions[i]->nr,
> > +
Re: [Qemu-devel] [PATCH 3/4 v7] ppc: Add software breakpoint support
> -Original Message-
> From: Madhavan Srinivasan [mailto:ma...@linux.vnet.ibm.com]
> Sent: Friday, July 11, 2014 10:51 AM
> To: Bhushan Bharat-R65777; ag...@suse.de
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH 3/4 v7] ppc: Add software breakpoint support
>
> On Thursday 10 July 2014 07:49 PM, Bharat Bhushan wrote:
> > This patch allow insert/remove software breakpoint.
> >
> > When QEMU is not able to handle debug exception then we inject program
> > exception to guest because for software breakpoint QEMU uses a
> > ehpriv-1 instruction; So there cannot be any reason that we are in
> > qemu with exit reason KVM_EXIT_DEBUG for guest set debug exception,
> > only possibility is guest executed ehpriv-1 privilege instruction and
> > that's why we are injecting program exception.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > v6->v7
> > - Moved exception injection to this patch
> > - Inject the fault directly
> >
> > target-ppc/kvm.c | 87
> > +++-
> > 1 file changed, 73 insertions(+), 14 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > e00a20f..afa2291 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -1275,6 +1275,69 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> uint32_t dcrn, uint32_t dat
> > return 0;
> > }
> >
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +/* Mixed endian case is not handled */
> > +uint32_t sc = debug_inst_opcode;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> > +sizeof(sc), 0) ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, sizeof(sc), 1)) {
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct
> > +kvm_sw_breakpoint *bp) {
> > +uint32_t sc;
> > +
> > +if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&sc, sizeof(sc), 0) ||
> > +sc != debug_inst_opcode ||
> > +cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> > +sizeof(sc), 1)) {
> > +return -EINVAL;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug
> > +*dbg) {
> > +/* Software Breakpoint updates */
> > +if (kvm_sw_breakpoints_active(cs)) {
> > +dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
> > +}
> > +}
> > +
> > +static int kvm_handle_debug(PowerPCCPU *cpu, struct kvm_run *run) {
> > +CPUState *cs = CPU(cpu);
> > +CPUPPCState *env = &cpu->env;
> > +struct kvm_debug_exit_arch *arch_info = &run->debug.arch;
> > +int handle = 0;
> > +
> > +if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
> > +handle = 1;
> > +} else {
> > +/* QEMU is not able to handle debug exception, so inject
> > + * program exception to guest;
> > + * Yes program exception NOT debug exception !!
> > + * For software breakpoint QEMU uses a ehpriv-1 instruction;
> > + * So there cannot be any reason that we are here for guest
> > + * set debug exception, only possibility is guest executed a
> > + * privilege instruction and that's why we are injecting
> > + * program exception.
> > + */
> > + cs->exception_index = POWERPC_EXCP_PROGRAM;
> > + env->error_code = POWERPC_EXCP_INVAL;
> > + ppc_cpu_do_interrupt(cs);
> > +}
> > +
>
> Excellent. This is the change I had as part of server side patch for no sw
> breakpoint case. Also have one more addition to this, which I found in the
> debug.
>
> Only issue in here (using TCG for injecting) is that, KVM gives us PC, but
> incase of TCG, it uses nip. So nip gets decremented in ppc_cpu_do_interrupt
> function ending up sending the wrong pc to guest.
This is a good catch, I did not hit this because of some other issue (srr0/1
not getting sync properly on Booke-hv.
Thanks
-Bharat
> So Alex suggested to increment the nip by 4 before calling the
> ppc_cpu_do_interrupt function. Also kindly add cpu_synchronize_state before
> calling since we are changing the register values.
>
> Regards
> Maddy
>
> > +return handle;
> > +}
> > +
> > int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run) {
> > PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -1315,6 +1378,16 @@ int
> > kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> > ret = 0;
> > break;
> >
> > +case KVM_EXIT_DEBUG:
> > +DPRINTF("handle debug exception\n");
> > +if (kvm_handle_debug(cpu, run)) {
> > +ret = EXCP_DEBUG;
> > +break;
> > +}
> > +/* re-enter, this exception was guest-internal */
> > +ret = 0;
> > +break;
> > +
> > default:
> > fprintf(stderr, "KVM: unknown e
Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing
> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Friday, December 20, 2013 12:02 AM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
>
> On Thu, Dec 19, 2013 at 05:28:17PM +0100, Alexander Graf wrote:
> >
> > On 19.12.2013, at 16:39, bharat.bhus...@freescale.com wrote:
> >
> > >
> > >
> > >> -Original Message-
> > >> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > >> Sent: Thursday, December 19, 2013 3:55 AM
> > >> To: Alexander Graf
> > >> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers;
> > >> qemu-ppc; Bhushan
> > >> Bharat-R65777
> > >> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > >>
> > >> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> > >>>
> > >>> On 28.11.2013, at 07:35, Bharat Bhushan wrote:
> > >>>
> > >>>> This patch adds pci pin to irq_num routing callback Without this
> > >>>> patch we gets below warning
> > >>>>
> > >>>> "
> > >>>> PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > >>>> qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > >>>> (e500-pcihost) "
> > >>>>
> > >>>> Signed-off-by: Bharat Bhushan
> > >>>> ---
> > >>>> hw/pci-host/ppce500.c | 20 ++--
> > >>>> 1 files changed, 18 insertions(+), 2 deletions(-)
> > >>>>
> > >>>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > >>>> 49bfcc6..3c4cf9e 100644
> > >>>> --- a/hw/pci-host/ppce500.c
> > >>>> +++ b/hw/pci-host/ppce500.c
> > >>>> @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > >>>>struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > >>>>uint32_t gasket_time;
> > >>>>qemu_irq irq[PCI_NUM_PINS];
> > >>>> +uint32_t irq_num[PCI_NUM_PINS];
> > >>>>uint32_t first_slot;
> > >>>>/* mmio maps */
> > >>>>MemoryRegion container;
> > >>>> @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > >>>> *pci_dev, int pin)
> > >>>>
> > >>>> static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > >>>> -qemu_irq *pic = opaque;
> > >>>> +PPCE500PCIState *s = opaque;
> > >>>> +qemu_irq *pic = s->irq;;
> > >>>
> > >>> Double semicolon?
> > >
> > > Ok, will correct.
> > >
> > >>>
> > >>>>
> > >>>>pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin ,
> > >>>> level);
> > >>>>
> > >>>>qemu_set_irq(pic[pin], level); }
> > >>>>
> > >>>> +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> > >>>> +pin) {
> > >>>> +PCIINTxRoute route;
> > >>>> +PPCE500PCIState *s = opaque;
> > >>>> +
> > >>>> +route.mode = PCI_INTX_ENABLED;
> > >>>> +route.irq = s->irq_num[pin];
> > >>>> +
> > >>>> +pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__,
> > >>>> + pin,
> > >> route.irq);
> > >>>> +return route;
> > >>>> +}
> > >>>> +
> > >>>> static const VMStateDescription vmstate_pci_outbound = {
> > >>>>.name = "pci_outbound",
> > >>>>.version_id = 0,
> > >>>> @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > >>>> *dev)
> > >>>>
> > >>>>for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > >>>>sysbus_init_irq(dev, &s->irq[i]);
> > >>>> +s->irq_num[i] = i + 1;
> > >>>
> > >>> Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()?
> > >>> I don't
> > >> understand the purpose of this whole exercise to be honest.
> > >>>
> > >>> Michael, could you please shed some light on this?
> > >>>
> > >>>
> > >>> Alex
> > >>
> > >> This is printed by pci_device_route_intx_to_irq - it's used by
> > >> device assignment and vfio to figure out which irq does a given pci
> > >> device
> drive.
> > >
> > > Yes, exactly same reason.
> >
> > Is there any way we could get rid of the information duplication? The fact
> that INTA/B/C/D are mapped to 1,2,3,4 is really a configuration parameter that
> should only live at a single spot.
> >
> >
> > Alex
>
> Yes. In fact I had the idea to only have something like
> pci_device_route_intx_to_irq and call it once for all interrupts and cache
> that,
> then use this to send interrupts directly to apic.
> Redo this each time routing changes.
> I had a patch like this (and I think Jan had one too), but Anthony said he'll
> rewrite all interrupt routing using QOM so I dropped it. I'll try to resurrect
> it.
So do we want to have this patch almost in this shape and hope Anthony's
changes will handle this well or wait for Anthony patches first ?
Thanks
-Bharat
>
Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Friday, December 20, 2013 4:01 PM
> To: Bhushan Bharat-R65777
> Cc: Michael S. Tsirkin; Wood Scott-B07421; qemu-ppc; QEMU Developers; Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
>
>
> On 20.12.2013, at 10:42, Bharat Bhushan wrote:
>
> > This patch adds pci pin to irq_num routing callback.
> > This callback is called from pci_device_route_intx_to_irq to find
> > which pci device maps to which irq. This is used for pci-device passthrough
> using vfio.
> >
> > Also without this patch we gets below warning
> >
> > "
> > PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > (e500-pcihost) "
> > and Legacy interrupt does not work with pci device passthrough.
> >
> > Signed-off-by: Bharat Bhushan
> > Acked-by: Michael S. Tsirkin
> > ---
> > hw/pci-host/ppce500.c | 20 ++--
> > 1 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > 71e5ca9..ffea782 100644
> > --- a/hw/pci-host/ppce500.c
> > +++ b/hw/pci-host/ppce500.c
> > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > uint32_t gasket_time;
> > qemu_irq irq[PCI_NUM_PINS];
> > +uint32_t irq_num[PCI_NUM_PINS];
> > uint32_t first_slot;
> > /* mmio maps */
> > MemoryRegion container;
> > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > *pci_dev, int pin)
> >
> > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > -qemu_irq *irq = opaque;
> > +PPCE500PCIState *s = opaque;
> > +qemu_irq *pic = s->irq;
> >
> > pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> >
> > qemu_set_irq(irq[pin], level);
> > }
> >
> > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int pin)
> > +{
> > +PCIINTxRoute route;
> > +PPCE500PCIState *s = opaque;
> > +
> > +route.mode = PCI_INTX_ENABLED;
> > +route.irq = s->irq_num[pin];
> > +
> > +pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin,
> route.irq);
> > +return route;
> > +}
> > +
> > static const VMStateDescription vmstate_pci_outbound = {
> > .name = "pci_outbound",
> > .version_id = 0,
> > @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > *dev)
> >
> > for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > sysbus_init_irq(dev, &s->irq[i]);
> > +s->irq_num[i] = i + 1;
>
> I still don't like how you duplicate the logic which MPIC irq line is
> associated
> with which pci host controller irq line. Please pass this information into the
> pcihost object somehow from ppce500_init() so that it's only at a single spot.
>
> If you like, you can just use qom/qdev properties for that, but it needs to be
> configured from the outside.
Ok :)
I thought we guys agreed that let this patch come as is and Anthony's work will
help globally.
Thanks
-Bharat
>
>
> Alex
>
>
Re: [Qemu-devel] [PATCH 1/2] ppc-e500: some pci related cleanup
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Thursday, December 19, 2013 3:18 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/2] ppc-e500: some pci related cleanup
>
>
> On 28.11.2013, at 07:35, Bharat Bhushan wrote:
>
> > - Use PCI_NUM_PINS rather than hardcoding
> > - use "pin" wherever possible
>
> I assume you mean the PCI A/B/C/D pin with "pin".
Yes
>
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > hw/pci-host/ppce500.c | 14 +++---
> > hw/ppc/e500.c | 12 +++-
> > 2 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > f00793d..49bfcc6 100644
> > --- a/hw/pci-host/ppce500.c
> > +++ b/hw/pci-host/ppce500.c
> > @@ -87,7 +87,7 @@ struct PPCE500PCIState {
> > struct pci_outbound pob[PPCE500_PCI_NR_POBS];
> > struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > uint32_t gasket_time;
> > -qemu_irq irq[4];
> > +qemu_irq irq[PCI_NUM_PINS];
> > uint32_t first_slot;
> > /* mmio maps */
> > MemoryRegion container;
> > @@ -252,26 +252,26 @@ static const MemoryRegionOps e500_pci_reg_ops = {
> > .endianness = DEVICE_BIG_ENDIAN,
> > };
> >
> > -static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
> > +static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
>
> This function converts pin -> irq, so that's fine.
>
> > {
> > int devno = pci_dev->devfn >> 3;
> > int ret;
> >
> > -ret = ppce500_pci_map_irq_slot(devno, irq_num);
> > +ret = ppce500_pci_map_irq_slot(devno, pin);
> >
> > pci_debug("%s: devfn %x irq %d -> %d devno:%x\n", __func__,
> > - pci_dev->devfn, irq_num, ret, devno);
> > + pci_dev->devfn, pin, ret, devno);
> >
> > return ret;
> > }
> >
> > -static void mpc85xx_pci_set_irq(void *opaque, int irq_num, int level)
> > +static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
>
> While this one ...
>
> > {
> > qemu_irq *pic = opaque;
> >
> > -pci_debug("%s: PCI irq %d, level:%d\n", __func__, irq_num, level);
> > +pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> >
> > -qemu_set_irq(pic[irq_num], level);
> > +qemu_set_irq(pic[pin], level);
>
> ... sets an actual irq number on the PIC, so this is not a pin.
pic[] is array of intA/B/C/B pin, no ?
Thanks
-Bharat
>
>
> The rest looks good to me :).
>
>
> Alex
>
>
Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing
> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Thursday, December 19, 2013 9:20 PM
> To: Bhushan Bharat-R65777
> Cc: Alexander Graf; Wood Scott-B07421; QEMU Developers; qemu-ppc
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
>
> On Thu, Dec 19, 2013 at 03:39:58PM +, bharat.bhus...@freescale.com wrote:
> >
> >
> > > -Original Message-
> > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > Sent: Thursday, December 19, 2013 3:55 AM
> > > To: Alexander Graf
> > > Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers;
> > > qemu-ppc; Bhushan
> > > Bharat-R65777
> > > Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
> > >
> > > On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> > > >
> > > > On 28.11.2013, at 07:35, Bharat Bhushan wrote:
> > > >
> > > > > This patch adds pci pin to irq_num routing callback Without this
> > > > > patch we gets below warning
> > > > >
> > > > > "
> > > > > PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > > > > qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > > > > (e500-pcihost) "
> > > > >
> > > > > Signed-off-by: Bharat Bhushan
> > > > > ---
> > > > > hw/pci-host/ppce500.c | 20 ++--
> > > > > 1 files changed, 18 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > > > > 49bfcc6..3c4cf9e 100644
> > > > > --- a/hw/pci-host/ppce500.c
> > > > > +++ b/hw/pci-host/ppce500.c
> > > > > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > > > > struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > > > > uint32_t gasket_time;
> > > > > qemu_irq irq[PCI_NUM_PINS];
> > > > > +uint32_t irq_num[PCI_NUM_PINS];
> > > > > uint32_t first_slot;
> > > > > /* mmio maps */
> > > > > MemoryRegion container;
> > > > > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > > > > *pci_dev, int pin)
> > > > >
> > > > > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > > > > -qemu_irq *pic = opaque;
> > > > > +PPCE500PCIState *s = opaque;
> > > > > +qemu_irq *pic = s->irq;;
> > > >
> > > > Double semicolon?
> >
> > Ok, will correct.
> >
> > > >
> > > > >
> > > > > pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin ,
> > > > > level);
> > > > >
> > > > > qemu_set_irq(pic[pin], level); }
> > > > >
> > > > > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque,
> > > > > +int
> > > > > +pin) {
> > > > > +PCIINTxRoute route;
> > > > > +PPCE500PCIState *s = opaque;
> > > > > +
> > > > > +route.mode = PCI_INTX_ENABLED;
> > > > > +route.irq = s->irq_num[pin];
> > > > > +
> > > > > +pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__,
> > > > > + pin,
> > > route.irq);
> > > > > +return route;
> > > > > +}
> > > > > +
> > > > > static const VMStateDescription vmstate_pci_outbound = {
> > > > > .name = "pci_outbound",
> > > > > .version_id = 0,
> > > > > @@ -350,12 +364,13 @@ static int
> > > > > e500_pcihost_initfn(SysBusDevice
> > > > > *dev)
> > > > >
> > > > > for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > > > > sysbus_init_irq(dev, &s->irq[i]);
> > > > > +s->irq_num[i] = i + 1;
> > > >
> > > > Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()?
> > > > I don't
> > > understand the purpose of this whole exercise to be honest.
> > > >
> > > > Michael, could you please shed some light on this?
> > > >
> > > >
> > > > Alex
> > >
> > > This is printed by pci_device_route_intx_to_irq - it'
Re: [Qemu-devel] [PATCH 2/2] ppc-e500: implement PCI INTx routing
> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Thursday, December 19, 2013 3:55 AM
> To: Alexander Graf
> Cc: Bhushan Bharat-R65777; Wood Scott-B07421; QEMU Developers; qemu-ppc;
> Bhushan
> Bharat-R65777
> Subject: Re: [PATCH 2/2] ppc-e500: implement PCI INTx routing
>
> On Wed, Dec 18, 2013 at 10:53:32PM +0100, Alexander Graf wrote:
> >
> > On 28.11.2013, at 07:35, Bharat Bhushan wrote:
> >
> > > This patch adds pci pin to irq_num routing callback Without this
> > > patch we gets below warning
> > >
> > > "
> > > PCI: Bug - unimplemented PCI INTx routing (e500-pcihost)
> > > qemu-system-ppc64: PCI: Bug - unimplemented PCI INTx routing
> > > (e500-pcihost) "
> > >
> > > Signed-off-by: Bharat Bhushan
> > > ---
> > > hw/pci-host/ppce500.c | 20 ++--
> > > 1 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > > 49bfcc6..3c4cf9e 100644
> > > --- a/hw/pci-host/ppce500.c
> > > +++ b/hw/pci-host/ppce500.c
> > > @@ -88,6 +88,7 @@ struct PPCE500PCIState {
> > > struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > > uint32_t gasket_time;
> > > qemu_irq irq[PCI_NUM_PINS];
> > > +uint32_t irq_num[PCI_NUM_PINS];
> > > uint32_t first_slot;
> > > /* mmio maps */
> > > MemoryRegion container;
> > > @@ -267,13 +268,26 @@ static int mpc85xx_pci_map_irq(PCIDevice
> > > *pci_dev, int pin)
> > >
> > > static void mpc85xx_pci_set_irq(void *opaque, int pin, int level) {
> > > -qemu_irq *pic = opaque;
> > > +PPCE500PCIState *s = opaque;
> > > +qemu_irq *pic = s->irq;;
> >
> > Double semicolon?
Ok, will correct.
> >
> > >
> > > pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> > >
> > > qemu_set_irq(pic[pin], level);
> > > }
> > >
> > > +static PCIINTxRoute e500_route_intx_pin_to_irq(void *opaque, int
> > > +pin) {
> > > +PCIINTxRoute route;
> > > +PPCE500PCIState *s = opaque;
> > > +
> > > +route.mode = PCI_INTX_ENABLED;
> > > +route.irq = s->irq_num[pin];
> > > +
> > > +pci_debug("%s: PCI irq-pin = %d, irq_num= %d\n", __func__, pin,
> route.irq);
> > > +return route;
> > > +}
> > > +
> > > static const VMStateDescription vmstate_pci_outbound = {
> > > .name = "pci_outbound",
> > > .version_id = 0,
> > > @@ -350,12 +364,13 @@ static int e500_pcihost_initfn(SysBusDevice
> > > *dev)
> > >
> > > for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> > > sysbus_init_irq(dev, &s->irq[i]);
> > > +s->irq_num[i] = i + 1;
> >
> > Doesn't this duplicate the logic from ppce500_pci_map_irq_slot()? I don't
> understand the purpose of this whole exercise to be honest.
> >
> > Michael, could you please shed some light on this?
> >
> >
> > Alex
>
> This is printed by pci_device_route_intx_to_irq - it's used by device
> assignment
> and vfio to figure out which irq does a given pci device drive.
Yes, exactly same reason.
Thanks
-Bharat
>
> > > }
> > >
> > > memory_region_init(&s->pio, OBJECT(s), "pci-pio",
> > > PCIE500_PCI_IOLEN);
> > >
> > > b = pci_register_bus(DEVICE(dev), NULL, mpc85xx_pci_set_irq,
> > > - mpc85xx_pci_map_irq, s->irq, address_space_mem,
> > > + mpc85xx_pci_map_irq, s, address_space_mem,
> > > &s->pio, PCI_DEVFN(s->first_slot, 0), 4,
> TYPE_PCI_BUS);
> > > h->bus = b;
> > >
> > > @@ -373,6 +388,7 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
> > > memory_region_add_subregion(&s->container, PCIE500_REG_BASE,
> > > &s->iomem);
> > > sysbus_init_mmio(dev, &s->container);
> > > sysbus_init_mmio(dev, &s->pio);
> > > +pci_bus_set_route_irq_fn(b, e500_route_intx_pin_to_irq);
> > >
> > > return 0;
> > > }
> > > --
> > > 1.7.0.4
> > >
> > >
>
Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/4] PPC: e500 pci host: Add support for ATMUs
> -Original Message-
> From: qemu-ppc-bounces+bharat.bhushan=freescale@nongnu.org
> [mailto:qemu-ppc-
> bounces+bharat.bhushan=freescale@nongnu.org] On Behalf Of Alexander Graf
> Sent: Thursday, November 13, 2014 3:27 AM
> To: qemu-...@nongnu.org
> Cc: Yoder Stuart-B08248; qemu-devel@nongnu.org
> Subject: [Qemu-ppc] [PATCH v3 4/4] PPC: e500 pci host: Add support for ATMUs
>
> The e500 PCI controller has configurable windows that allow a guest OS
> to selectively map parts of the PCI bus space to CPU address space and
> to selectively map parts of the CPU address space for DMA requests into
> PCI visible address ranges.
>
> So far, we've simply assumed that this mapping is 1:1 and ignored it.
>
> However, the PCICSRBAR (CCSR mapped in PCI bus space) always has to live
> inside the first 32bits of address space. This means if we always treat
> all mappings as 1:1, this map will collide with our RAM map from the CPU's
> point of view.
>
> So this patch adds proper ATMU support which allows us to keep the PCICSRBAR
> below 32bits local to the PCI bus and have another, different window to PCI
> BARs at the upper end of address space. We leverage this on e500plat though,
> mpc8544ds stays virtually 1:1 like it was before, but now also goes via ATMU.
>
> With this patch, I can run guests with lots of RAM and not coincidently access
> MSI-X mappings while I really want to access RAM.
>
> Signed-off-by: Alexander Graf
> ---
> hw/pci-host/ppce500.c | 113
> +++---
> hw/ppc/e500.c | 6 +--
> hw/ppc/e500.h | 2 +
> hw/ppc/e500plat.c | 2 +
> hw/ppc/mpc8544ds.c| 2 +
> 5 files changed, 115 insertions(+), 10 deletions(-)
>
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 1b4c0f0..574f8b2 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -62,11 +62,19 @@
> #define PPCE500_PCI_NR_POBS 5
> #define PPCE500_PCI_NR_PIBS 3
>
> +#define PIWAR_EN0x8000 /* Enable */
> +#define PIWAR_PF0x2000 /* prefetch */
> +#define PIWAR_TGI_LOCAL 0x00f0 /* target - local memory */
> +#define PIWAR_READ_SNOOP0x0005
> +#define PIWAR_WRITE_SNOOP 0x5000
> +#define PIWAR_SZ_MASK 0x003f
> +
> struct pci_outbound {
> uint32_t potar;
> uint32_t potear;
> uint32_t powbar;
> uint32_t powar;
> +MemoryRegion mem;
> };
>
> struct pci_inbound {
> @@ -74,6 +82,7 @@ struct pci_inbound {
> uint32_t piwbar;
> uint32_t piwbear;
> uint32_t piwar;
> +MemoryRegion mem;
> };
>
> #define TYPE_PPC_E500_PCI_HOST_BRIDGE "e500-pcihost"
> @@ -91,10 +100,13 @@ struct PPCE500PCIState {
> uint32_t irq_num[PCI_NUM_PINS];
> uint32_t first_slot;
> uint32_t first_pin_irq;
> +AddressSpace bm_as;
> +MemoryRegion bm;
> /* mmio maps */
> MemoryRegion container;
> MemoryRegion iomem;
> MemoryRegion pio;
> +MemoryRegion busmem;
> };
>
> #define TYPE_PPC_E500_PCI_BRIDGE "e500-host-bridge"
> @@ -181,6 +193,71 @@ static uint64_t pci_reg_read4(void *opaque, hwaddr addr,
> return value;
> }
>
> +/* DMA mapping */
> +static void e500_update_piw(PPCE500PCIState *pci, int idx)
> +{
> +uint64_t tar = ((uint64_t)pci->pib[idx].pitar) << 12;
> +uint64_t wbar = ((uint64_t)pci->pib[idx].piwbar) << 12;
> +uint64_t war = pci->pib[idx].piwar;
> +uint64_t size = 2ULL << (war & PIWAR_SZ_MASK);
> +MemoryRegion *address_space_mem = get_system_memory();
> +MemoryRegion *mem = &pci->pib[idx].mem;
> +MemoryRegion *bm = &pci->bm;
> +char *name;
> +
> +if (memory_region_is_mapped(mem)) {
> +/* Before we modify anything, unmap and destroy the region */
> +memory_region_del_subregion(bm, mem);
> +object_unparent(OBJECT(mem));
> +}
> +
> +if (!(war & PIWAR_EN)) {
> +/* Not enabled, nothing to do */
> +return;
> +}
> +
> +name = g_strdup_printf("PCI Inbound Window %d", idx);
> +memory_region_init_alias(mem, OBJECT(pci), name, address_space_mem, tar,
> + size);
> +memory_region_add_subregion_overlap(bm, wbar, mem, -1);
> +g_free(name);
> +
> +pci_debug("%s: Added window of size=%#lx from PCI=%#lx to CPU=%#lx\n",
> + __func__, size, wbar, tar);
> +}
> +
> +/* BAR mapping */
> +static void e500_update_pow(PPCE500PCIState *pci, int idx)
> +{
> +uint64_t tar = ((uint64_t)pci->pob[idx].potar) << 12;
> +uint64_t wbar = ((uint64_t)pci->pob[idx].powbar) << 12;
> +uint64_t war = pci->pob[idx].powar;
> +uint64_t size = 2ULL << (war & PIWAR_SZ_MASK);
> +MemoryRegion *mem = &pci->pob[idx].mem;
> +MemoryRegion *address_space_mem = get_system_memory();
> +char *name;
> +
> +if (memory_region_is_mapped(mem)) {
> +/* Before we modify anything, unmap and destroy the region */
> +
Re: [Qemu-devel] [PATCH 5/5 v5] ppc: Add hw breakpoint watchpoint support
> -Original Message-
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Thursday, June 26, 2014 3:52 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; ma...@linux.vnet.ibm.com
> Subject: Re: [PATCH 5/5 v5] ppc: Add hw breakpoint watchpoint support
>
>
> On 26.06.14 09:23, Bharat Bhushan wrote:
> > This patch adds hardware breakpoint and hardware watchpoint support
> > for ppc. If the debug interrupt is not handled then this is injected
> > to guest.
> >
> > Signed-off-by: Bharat Bhushan
> > ---
> > v4->v5
> > - cleanup in kvmppc_hw_debug_points_init()
> > - replaced assert in kvm_arch_insert_hw_breakpoint() with return.
> > This allows gdb to through warning "You may have requested too many
> > hardware breakpoints/watchpoints.". Now user can remove extra breakpint
> > and continue.
> >
> > target-ppc/kvm.c | 238
> > +++--
> --
> > 1 file changed, 223 insertions(+), 15 deletions(-)
> >
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index
> > 3195491..3347c1b 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -38,6 +38,7 @@
> > #include "hw/ppc/ppc.h"
> > #include "sysemu/watchdog.h"
> > #include "trace.h"
> > +#include "exec/gdbstub.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -410,6 +411,38 @@ unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > return ppc_get_vcpu_dt_id(POWERPC_CPU(cpu));
> > }
> >
> > +/* e500 supports 2 h/w breakpoint and 2 watchpoint.
> > + * book3s supports only 1 watchpoint, so array size
> > + * of 4 is sufficient for now.
> > + */
> > +#define MAX_HW_BKPTS 4
> > +
> > +static struct HWBreakpoint {
> > +target_ulong addr;
> > +int type;
> > +} hw_debug_points[MAX_HW_BKPTS];
> > +
> > +static CPUWatchpoint hw_watchpoint;
> > +
> > +/* Default there is no breakpoint and watchpoint supported */ static
> > +int max_hw_breakpoint; static int max_hw_watchpoint; static int
> > +nb_hw_breakpoint; static int nb_hw_watchpoint;
> > +
> > +static void kvmppc_hw_debug_points_init(CPUPPCState *cenv) {
> > +if (cenv->excp_model == POWERPC_EXCP_BOOKE) {
> > +max_hw_breakpoint = 2;
> > +max_hw_watchpoint = 2;
> > +}
> > +
> > +if ((max_hw_breakpoint + max_hw_watchpoint) > MAX_HW_BKPTS) {
> > +fprintf(stderr, "Error initializing h/w breakpoints\n");
> > +return;
> > +}
> > +}
> > +
> > int kvm_arch_init_vcpu(CPUState *cs)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(cs); @@ -437,6 +470,7 @@ int
> > kvm_arch_init_vcpu(CPUState *cs)
> > }
> >
> > kvm_get_one_reg(cs, KVM_REG_PPC_DEBUG_INST, &debug_inst_opcode);
> > +kvmppc_hw_debug_points_init(cenv);
> >
> > return ret;
> > }
> > @@ -1345,24 +1379,212 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs,
> struct kvm_sw_breakpoint *bp)
> > return 0;
> > }
> >
> > +static int find_hw_breakpoint(target_ulong addr, int type) {
> > +int n;
> > +
> > +assert((nb_hw_breakpoint + nb_hw_watchpoint)
> > + <= ARRAY_SIZE(hw_debug_points));
> > +
> > +for (n = 0; n < nb_hw_breakpoint + nb_hw_watchpoint; n++) {
> > +if (hw_debug_points[n].addr == addr && hw_debug_points[n].type ==
> type) {
> > +return n;
> > +}
> > +}
> > +
> > +return -1;
> > +}
> > +
> > +static int find_hw_watchpoint(target_ulong addr, int *flag) {
> > +int n;
> > +
> > +n = find_hw_breakpoint(addr, GDB_WATCHPOINT_ACCESS);
> > +if (n >= 0) {
> > +*flag = BP_MEM_ACCESS;
> > +return n;
> > +}
> > +
> > +n = find_hw_breakpoint(addr, GDB_WATCHPOINT_WRITE);
> > +if (n >= 0) {
> > +*flag = BP_MEM_WRITE;
> > +return n;
> > +}
> > +
> > +n = find_hw_breakpoint(addr, GDB_WATCHPOINT_READ);
> > +if (n >= 0) {
> > +*flag = BP_MEM_READ;
> > +return n;
> > +}
> > +
> > +return -1;
> > +}
> > +
> > +int kvm_arch_insert_hw_breakpoint(target_ulong addr,
> > + target_ulong len, int type) {
> > +if ((nb_hw_breakpoint + nb_hw_watchpoint) >=
> > ARRAY_SIZE(hw_debug_points))
> > + return -ENOBUFS;
> > +
> > +hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].addr = addr;
> > +hw_debug_points[nb_hw_breakpoint + nb_hw_watchpoint].type = type;
> > +
> > +switch (type) {
> > +case GDB_BREAKPOINT_HW:
> > +if (nb_hw_breakpoint >= max_hw_breakpoint) {
> > +return -ENOBUFS;
> > +}
> > +
> > +if (find_hw_breakpoint(addr, type) >= 0) {
> > +return -EEXIST;
> > +}
> > +
> > +nb_hw_breakpoint++;
> > +break;
> > +
> > +case GDB_WATCHPOINT_WRITE:
> > +case GDB_WATCHPOINT_READ:
> > +case GDB_WATCHPOINT_ACCESS:
> > +if (nb_hw_watchpoint >= max_hw_watchpoint) {
> > +return -ENOBUFS;
> > +}
> > +
> > +if (find_hw_breakpoint(addr, type) >= 0) {
> > +return -EEXIST;
>
Re: [Qemu-devel] [PATCH 5/5 v5] ppc: Add hw breakpoint watchpoint support
> >>> +
> >>> +static void kvm_e500_handle_debug(CPUState *cs, int handle) {
> >>> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> >>> +CPUPPCState *env = &cpu->env;
> >>> +
> >>> +env->spr[SPR_BOOKE_DBSR] = 0;
> >> How would KVM ever know that DBSR is now 0?
> > Yes, guest will not come to know of this "0" value. I was wrong, thinking
> about that this will be used in inject_debug_exception(), but
> inject_debug_exception() is not called in this flow.
> >
> >> And why do we need this in the first
> >> place? The guest's DBSR value doesn't get set on debug interrupts
> >> unless we call inject_debug_exception().
> > vcpu->arch.dbsr is set on debug exception in KVM and that is what guest
> > sees,
> so we should clear DBSR.
>
> Ah, ok. So do we actually need inject_debug_exception()?
If not then how we to inject interrupt to guest ?
>
> >
> >> So there's no need to clear it either, no?
> > So I think I need a one_reg interface to set/clear DBSR.
>
> The sregs interface should be good enough for now, no?
So basically we want two things
1) Just clear DBSR in KVM when debug interrupts are handled by QEMU
2) Set DSRR0/1 or CSRR0/1 and set DBSR when interrupt are not handled by QEMU
For (1); We can add a function which will use SREGS interface to clear DBSR.
For (2); we will use inject_debug_exception().
Thanks
-Bharat
>
>
> Alex
