| Message ID | 1396530891-6352-3-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On 03.04.14 15:14, Alexey Kardashevskiy wrote: > This advertises Data Address Breakpoint Register Extension (DABRX) to > the guest via hyperrtas list and enables it to migrate. Do all CPUs we support (970 anyone) have DABRX support? Also who handles this hcall in the TCG case? What about older host kernels that don't support xdabr yet? What about PR KVM? Alex > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/ppc/spapr.c | 1 + > target-ppc/translate_init.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index a11e121..451c473 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, > uint32_t start_prop = cpu_to_be32(initrd_base); > uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); > char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt" > + "\0hcall-xdabr" > "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode"; > char qemu_hypertas_prop[] = "hcall-memop1"; > uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)}; > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index d07e186..1627bb0 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env) > SPR_NOACCESS, SPR_NOACCESS, > &spr_read_generic, &spr_write_generic, > KVM_REG_PPC_PMC6, 0x00000000); > + spr_register_kvm(env, SPR_DABRX, "DABRX", > + SPR_NOACCESS, SPR_NOACCESS, > + SPR_NOACCESS, SPR_NOACCESS, > + KVM_REG_PPC_DABRX, 0x00000000); > #endif /* !CONFIG_USER_ONLY */ > gen_spr_amr(env); > /* XXX : not implemented */
On 4/3/2014 8:14 AM, Alexey Kardashevskiy wrote: > This advertises Data Address Breakpoint Register Extension (DABRX) to > the guest via hyperrtas list and enables it to migrate. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/ppc/spapr.c | 1 + > target-ppc/translate_init.c | 4 ++++ > 2 files changed, 5 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index a11e121..451c473 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, > uint32_t start_prop = cpu_to_be32(initrd_base); > uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); > char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt" > + "\0hcall-xdabr" > "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode"; > char qemu_hypertas_prop[] = "hcall-memop1"; > uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)}; It isn't clear to me what is enabled with this. Alexey: can you provide a little explanation of what adding this string actually does? And I assume the spelling of "xdabr" (versus "dabrx") is intentional? > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index d07e186..1627bb0 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env) > SPR_NOACCESS, SPR_NOACCESS, > &spr_read_generic, &spr_write_generic, > KVM_REG_PPC_PMC6, 0x00000000); > + spr_register_kvm(env, SPR_DABRX, "DABRX", > + SPR_NOACCESS, SPR_NOACCESS, > + SPR_NOACCESS, SPR_NOACCESS, > + KVM_REG_PPC_DABRX, 0x00000000); > #endif /* !CONFIG_USER_ONLY */ > gen_spr_amr(env); > /* XXX : not implemented */ > I see a problem with this and it is caused by some of the P8 code that I had added a while back. The P8 init code (init_proc_POWER8) calls this init_proc_POWER7 routine. So by adding DABRX to the P7 code means P8 gets it for free. Unfortunately, P8 doesn't have DABRX ... it supports the new debug facilities (DAWR[X]). The DABR and IABR registers are in the same boat. I think the init_proc_POWER8 code needs to become a self-sufficienct version. I can fix and send to you or you can fix yourself ... let me know.
On 04/04/2014 05:42 AM, Tom Musta wrote: > On 4/3/2014 8:14 AM, Alexey Kardashevskiy wrote: >> This advertises Data Address Breakpoint Register Extension (DABRX) to >> the guest via hyperrtas list and enables it to migrate. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> hw/ppc/spapr.c | 1 + >> target-ppc/translate_init.c | 4 ++++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index a11e121..451c473 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, >> uint32_t start_prop = cpu_to_be32(initrd_base); >> uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); >> char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt" >> + "\0hcall-xdabr" >> "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode"; >> char qemu_hypertas_prop[] = "hcall-memop1"; >> uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)}; > > > It isn't clear to me what is enabled with this. Alexey: can you provide a little explanation > of what adding this string actually does? Yes, I will. I just got this patch from someone else and did not really try to understand it :) > And I assume the spelling of "xdabr" (versus "dabrx") > is intentional? Sure: SPAPR 2.7: H_SET_XDABR / 14.5.4.3.7 0x134 Normal If Extended DABR option is implemented hcall-xdabr >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index d07e186..1627bb0 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env) >> SPR_NOACCESS, SPR_NOACCESS, >> &spr_read_generic, &spr_write_generic, >> KVM_REG_PPC_PMC6, 0x00000000); >> + spr_register_kvm(env, SPR_DABRX, "DABRX", >> + SPR_NOACCESS, SPR_NOACCESS, >> + SPR_NOACCESS, SPR_NOACCESS, >> + KVM_REG_PPC_DABRX, 0x00000000); >> #endif /* !CONFIG_USER_ONLY */ >> gen_spr_amr(env); >> /* XXX : not implemented */ >> > > I see a problem with this and it is caused by some of the P8 code that I had added a while back. > The P8 init code (init_proc_POWER8) calls this init_proc_POWER7 routine. So by adding DABRX > to the P7 code means P8 gets it for free. Unfortunately, P8 doesn't have DABRX ... it supports > the new debug facilities (DAWR[X]). The DABR and IABR registers are in the same boat. I think > the init_proc_POWER8 code needs to become a self-sufficienct version. > > I can fix and send to you or you can fix yourself ... let me know. Since I'll be touching this code soon, I can make copy content of init_proc_POWER7 to init_proc_POWER8 and remove DABRX if this is what you mean. Ok?
On 04/04/2014 12:19 AM, Alexander Graf wrote: > > On 03.04.14 15:14, Alexey Kardashevskiy wrote: >> This advertises Data Address Breakpoint Register Extension (DABRX) to >> the guest via hyperrtas list and enables it to migrate. > > Do all CPUs we support (970 anyone) have DABRX support? 970MP and 970FX do. Support them too? Who cares? :) > Also who handles this hcall in the TCG case? Good point... > What about older host kernels that don't > support xdabr yet? They will ignore FW_FEATURE_XDABR, no? > What about PR KVM? Oh. Nothing. And we do not want to make this "hcall-xdabr" conditional, right? Drop the whole patch? I am really confused now. > > > Alex > >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> hw/ppc/spapr.c | 1 + >> target-ppc/translate_init.c | 4 ++++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index a11e121..451c473 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, >> uint32_t start_prop = cpu_to_be32(initrd_base); >> uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); >> char hypertas_prop[] = >> "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt" >> + "\0hcall-xdabr" >> >> "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode"; >> char qemu_hypertas_prop[] = "hcall-memop1"; >> uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)}; >> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c >> index d07e186..1627bb0 100644 >> --- a/target-ppc/translate_init.c >> +++ b/target-ppc/translate_init.c >> @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env) >> SPR_NOACCESS, SPR_NOACCESS, >> &spr_read_generic, &spr_write_generic, >> KVM_REG_PPC_PMC6, 0x00000000); >> + spr_register_kvm(env, SPR_DABRX, "DABRX", >> + SPR_NOACCESS, SPR_NOACCESS, >> + SPR_NOACCESS, SPR_NOACCESS, >> + KVM_REG_PPC_DABRX, 0x00000000); >> #endif /* !CONFIG_USER_ONLY */ >> gen_spr_amr(env); >> /* XXX : not implemented */ >
On 04/04/2014 08:13 AM, Alexey Kardashevskiy wrote: > On 04/04/2014 12:19 AM, Alexander Graf wrote: >> On 03.04.14 15:14, Alexey Kardashevskiy wrote: >>> This advertises Data Address Breakpoint Register Extension (DABRX) to >>> the guest via hyperrtas list and enables it to migrate. >> Do all CPUs we support (970 anyone) have DABRX support? > 970MP and 970FX do. Support them too? Who cares? :) Well, we do support running KVM on these and for KVM we default to -cpu host, so they're important to keep in mind. > >> Also who handles this hcall in the TCG case? > Good point... > >> What about older host kernels that don't >> support xdabr yet? > They will ignore FW_FEATURE_XDABR, no? How does a host kernel ignore anything here? We're telling the guest that we support an hcall without asking the host at all. > >> What about PR KVM? > Oh. Nothing. And we do not want to make this "hcall-xdabr" conditional, > right? Drop the whole patch? I am really confused now. I think we should properly check whether we can handle this hcall and/or implement a handler for TCG and hosts which don't handle it themselves. Alex
On 4/3/2014 7:51 PM, Alexey Kardashevskiy wrote: > Since I'll be touching this code soon, I can make copy content of > init_proc_POWER7 to init_proc_POWER8 and remove DABRX if this is what you > mean. Ok? Yes it is. Thanks, Alexey.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index a11e121..451c473 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -307,6 +307,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base, uint32_t start_prop = cpu_to_be32(initrd_base); uint32_t end_prop = cpu_to_be32(initrd_base + initrd_size); char hypertas_prop[] = "hcall-pft\0hcall-term\0hcall-dabr\0hcall-interrupt" + "\0hcall-xdabr" "\0hcall-tce\0hcall-vio\0hcall-splpar\0hcall-bulk\0hcall-set-mode"; char qemu_hypertas_prop[] = "hcall-memop1"; uint32_t refpoints[] = {cpu_to_be32(0x4), cpu_to_be32(0x4)}; diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index d07e186..1627bb0 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -7010,6 +7010,10 @@ static void init_proc_POWER7 (CPUPPCState *env) SPR_NOACCESS, SPR_NOACCESS, &spr_read_generic, &spr_write_generic, KVM_REG_PPC_PMC6, 0x00000000); + spr_register_kvm(env, SPR_DABRX, "DABRX", + SPR_NOACCESS, SPR_NOACCESS, + SPR_NOACCESS, SPR_NOACCESS, + KVM_REG_PPC_DABRX, 0x00000000); #endif /* !CONFIG_USER_ONLY */ gen_spr_amr(env); /* XXX : not implemented */
This advertises Data Address Breakpoint Register Extension (DABRX) to the guest via hyperrtas list and enables it to migrate. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/ppc/spapr.c | 1 + target-ppc/translate_init.c | 4 ++++ 2 files changed, 5 insertions(+)