Re: [Qemu-devel] [PATCH 2/4] spapr: Enable DABRX special register

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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?



-- 
Alexey