Patchwork PPC: Get MMU state on register sync

login
register
mail settings
Submitter Alexander Graf
Date Nov. 24, 2009, 8:01 a.m.
Message ID <1259049712-8165-2-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/39157/
State New
Headers show

Comments

Alexander Graf - Nov. 24, 2009, 8:01 a.m.
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(-)
Jan Kiszka - Nov. 24, 2009, 6:01 p.m.
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?

Jan
Alexander Graf - Nov. 24, 2009, 6:04 p.m.
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.

I definitely will implement SET_SREGS as soon as your sync split is in, as that's IMHO only really required on migration.


Alex
Jan Kiszka - Nov. 24, 2009, 6:12 p.m.
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.

> 
> 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.

Jan
Alexander Graf - Nov. 24, 2009, 6:14 p.m.
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.

>> 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.


Alex
Jan Kiszka - Nov. 24, 2009, 6:33 p.m.
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?

Jan
Alexander Graf - Nov. 24, 2009, 6:46 p.m.
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 :-).


Alex
Jan Kiszka - Nov. 24, 2009, 6:49 p.m.
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.

Jan
Alexander Graf - Nov. 24, 2009, 6:53 p.m.
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?

Alex
Jan Kiszka - Nov. 24, 2009, 7:03 p.m.
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
Alexander Graf - Nov. 24, 2009, 7:10 p.m.
On 24.11.2009, at 20:03, Jan Kiszka wrote:

> 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.

At least with the firmware we have now, we start in real mode anyways, then set all 16 SRs (or 16 SLB fake entries) and then go into paged mode.

So we're safe for now. As I said I will implement the SET_SREGS in the sync when your split is done and I can limit sregs sets to reset.

Alex

Patch

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;
 }