[Qemu-devel] Re: [PATCH] PPC: Get MMU state on register sync

[Jan Kiszka <jan.kiszka@siemens.com>]

Alexander Graf wrote:
> On 24.11.2009, at 19:49, Jan Kiszka wrote:
> 
>> Alexander Graf wrote:
>>> On 24.11.2009, at 19:33, Jan Kiszka wrote:
>>>
>>>> Alexander Graf wrote:
>>>>> On 24.11.2009, at 19:12, Jan Kiszka wrote:
>>>>>
>>>>>> Alexander Graf wrote:
>>>>>>> On 24.11.2009, at 19:01, Jan Kiszka wrote:
>>>>>>>
>>>>>>>> Alexander Graf wrote:
>>>>>>>>> While x86 only needs to sync cr0-4 to know all about its MMU state and enable
>>>>>>>>> qemu to resolve virtual to physical addresses, we need to sync all of the
>>>>>>>>> segment registers on PPC to know which mapping we're in.
>>>>>>>>>
>>>>>>>>> So let's grab the segment register contents to be able to use the "x" monitor
>>>>>>>>> command and also enable the gdbstub to resolve virtual addresses.
>>>>>>>>>
>>>>>>>>> I sent the corresponding KVM patch to the KVM ML some minutes ago.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>>>> ---
>>>>>>>>> target-ppc/kvm.c |   30 ++++++++++++++++++++++++++++++
>>>>>>>>> 1 files changed, 30 insertions(+), 0 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>>>>>>>>> index 4e1c65f..566513f 100644
>>>>>>>>> --- a/target-ppc/kvm.c
>>>>>>>>> +++ b/target-ppc/kvm.c
>>>>>>>>> @@ -98,12 +98,17 @@ int kvm_arch_put_registers(CPUState *env)
>>>>>>>>> int kvm_arch_get_registers(CPUState *env)
>>>>>>>>> {
>>>>>>>>>  struct kvm_regs regs;
>>>>>>>>> +    struct kvm_sregs sregs;
>>>>>>>>>  uint32_t i, ret;
>>>>>>>>>
>>>>>>>>>  ret = kvm_vcpu_ioctl(env, KVM_GET_REGS, &regs);
>>>>>>>>>  if (ret < 0)
>>>>>>>>>      return ret;
>>>>>>>>>
>>>>>>>>> +    ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
>>>>>>>>> +    if (ret < 0)
>>>>>>>>> +        return ret;
>>>>>>>>> +
>>>>>>>>>  env->ctr = regs.ctr;
>>>>>>>>>  env->lr = regs.lr;
>>>>>>>>>  env->xer = regs.xer;
>>>>>>>>> @@ -125,6 +130,31 @@ int kvm_arch_get_registers(CPUState *env)
>>>>>>>>>  for (i = 0;i < 32; i++)
>>>>>>>>>      env->gpr[i] = regs.gpr[i];
>>>>>>>>>
>>>>>>>>> +#ifdef KVM_CAP_PPC_SEGSTATE
>>>>>>>>> +    if (kvm_check_extension(env->kvm_state, KVM_CAP_PPC_SEGSTATE)) {
>>>>>>>>> +        env->sdr1 = sregs.sdr1;
>>>>>>>>> +    
>>>>>>>>> +        /* Sync SLB */
>>>>>>>>> +        for (i = 0; i < 64; i++) {
>>>>>>>>> +            ppc_store_slb(env, sregs.ppc64.slb[i].slbe,
>>>>>>>>> +                               sregs.ppc64.slb[i].slbv);
>>>>>>>>> +        }
>>>>>>>>> +    
>>>>>>>>> +        /* Sync SRs */
>>>>>>>>> +        for (i = 0; i < 16; i++) {
>>>>>>>>> +            env->sr[i] = sregs.ppc32.sr[i];
>>>>>>>>> +        }
>>>>>>>>> +    
>>>>>>>>> +        /* Sync BATs */
>>>>>>>>> +        for (i = 0; i < 8; i++) {
>>>>>>>>> +            env->DBAT[0][i] = sregs.ppc32.dbat[i] & 0xffffffff;
>>>>>>>>> +            env->DBAT[1][i] = sregs.ppc32.dbat[i] >> 32;
>>>>>>>>> +            env->IBAT[0][i] = sregs.ppc32.ibat[i] & 0xffffffff;
>>>>>>>>> +            env->IBAT[1][i] = sregs.ppc32.ibat[i] >> 32;
>>>>>>>>> +        }
>>>>>>>>> +    }
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>>  return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>> What about KVM_SET_SREGS in kvm_arch_put_registers? E.g. to play back
>>>>>>>> potential changes to that special registers someone did via gdb?
>>>>>>> I don't think you can actually change the segment values. At least I can't imagine why.
>>>>>> Dunno about PPC in this regard and how much value it has, but we have
>>>>>> segment register access via gdb for x86.
>>>>> The segments here are more like PLM4 on x86.
>>>> Even that will be settable one day (gdb just do not yet know much about
>>>> x86 system management registers).
>>>>
>>>>>>> I definitely will implement SET_SREGS as soon as your sync split is in, as that's IMHO only really required on migration.
>>>>>>>
>>>>>> Migration is, of course, the major use case.
>>>>>>
>>>>>> Still I wonder why not making this API symmetric when already touching it.
>>>>> I was afraid to introduce performance regressions - setting the segments means flushing the complete shadow MMU.
>>>>>
>>>> Unless it costs milliseconds, not really critical, given how often
>>>> registers are synchronized.
>>>>
>>>> BTW, I noticed that ppc only syncs the SREGS once on init, not on reset
>>>> - are they static?
>>> So far SREGS are only used for setting the PVR (cpuid in x86 speech). There's no need to reset that on reset :-).
>> Then I don't get why you need to re-read them during runtime - user
>> space should know the state and should be able push it into the CPUState
>> on init.
> 
> Eeh. The SREGS contain:
> 
> - PVR
> - Segment register contents
> - BATs (another MMU thing for linear direct mapping)
> 
> On init we send SREGS to set PVR. Later on sync we get SREGS to get the segment registers.
> 
> You think it would have been better to create a new ioctl?

No, but I think you might miss a proper reset of some SREGS elements
when the VM goes through reset. If those states may change during guest
runtime, a hard reset should send them back into their hard reset state.

You can do this by adding yet another extraordinary SET_SREGS to the
reset callback or - that was my original point - by symmetrically adding
GET_SREGS and SET_SREGS to the register state sync.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux