Re: [Qemu-devel] [PATCH 1/3] kvm: ppc: booke206: use MMU API

[Alexander Graf]

On 17.06.2011, at 22:39, Scott Wood wrote:

> Share the TLB array with KVM.  This allows us to set the initial TLB
> both on initial boot and reset, is useful for debugging, and could
> eventually be used to support migration.
> 
> Signed-off-by: Scott Wood <address@hidden>
> ---
> hw/ppce500_mpc8544ds.c |    2 +
> target-ppc/cpu.h       |    2 +
> target-ppc/kvm.c       |   85 ++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 89 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
> index 5ac8843..3cdeb43 100644
> --- a/hw/ppce500_mpc8544ds.c
> +++ b/hw/ppce500_mpc8544ds.c
> @@ -192,6 +192,8 @@ static void mmubooke_create_initial_mapping(CPUState *env,
>     tlb->mas2 = va & TARGET_PAGE_MASK;
>     tlb->mas7_3 = pa & TARGET_PAGE_MASK;
>     tlb->mas7_3 |= MAS3_UR | MAS3_UW | MAS3_UX | MAS3_SR | MAS3_SW | MAS3_SX;
> +
> +    env->tlb_dirty = true;
> }
> 
> static void mpc8544ds_cpu_reset(void *opaque)
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 46d86be..8191ed2 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -921,6 +921,8 @@ struct CPUPPCState {
>     ppc_tlb_t tlb;   /* TLB is optional. Allocate them only if needed        
> */
>     /* 403 dedicated access protection registers */
>     target_ulong pb[4];
> +    bool tlb_dirty;   /* Set to non-zero when modifying TLB                  
> */
> +    bool kvm_sw_tlb;  /* non-zero if KVM SW TLB API is active                
> */
> #endif
> 
>     /* Other registers */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index e7b1b10..9a88fc9 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -122,6 +122,51 @@ static int kvm_arch_sync_sregs(CPUState *cenv)
>     return kvm_vcpu_ioctl(cenv, KVM_SET_SREGS, &sregs);
> }
> 
> +static int kvm_booke206_tlb_init(CPUState *env)
> +{
> +#if defined(KVM_CAP_SW_TLB) && defined(KVM_MMU_FSL_BOOKE_NOHV)

Those hopefully shouldn't be required anymore soon - when Jan's patches make it 
upstream. Jan, how's progress on that front?

> +    struct kvm_book3e_206_tlb_params params = {};

Hrm - I'm not familiar with that initialization. What exactly does it do? Set 
the struct contents to 0? Is this properly standardized? Usually, I see 
memset(0)s for that.

> +    struct kvm_config_tlb cfg = {};
> +    size_t array_len;
> +    unsigned int entries = 0;
> +    int ret, i;
> +
> +    if (!kvm_enabled() ||
> +        !kvm_check_extension(env->kvm_state, KVM_CAP_SW_TLB)) {
> +        return 0;
> +    }
> +
> +    for (i = 0; i < ARRAY_SIZE(params.tlb_sizes); i++) {

Please make this MAX(..., BOOKE206_MAX_TLBN) - I'd hope the compiler is clever 
enough to figure out we're dealing with a constant here and that way the code 
looks more secure (even though it's the same in practice).

Alternatively, you could just do an assert(... == BOOKE206_MAX_TLBN); before.

> +        params.tlb_sizes[i] = booke206_tlb_size(env, i);
> +        params.tlb_ways[i] = booke206_tlb_ways(env, i);
> +        entries += params.tlb_sizes[i];
> +    }
> +
> +    if (entries != env->nb_tlb) {
> +        cpu_abort(env, "%s: nb_tlb mismatch\n", __func__);
> +    }
> +

assert(sizeof(struct kvm_book3e_206_tlb_entry) == sizeof(ppcmas_tlb_t));

> +    array_len = sizeof(struct kvm_book3e_206_tlb_entry) * entries;
> +    env->tlb_dirty = true;
> +
> +    cfg.array = (uintptr_t)env->tlb.tlbm;
> +    cfg.array_len = sizeof(ppcmas_tlb_t) * entries;
> +    cfg.params = (uintptr_t)&params;
> +    cfg.mmu_type = KVM_MMU_FSL_BOOKE_NOHV;
> +
> +    ret = kvm_vcpu_ioctl(env, KVM_CONFIG_TLB, &cfg);
> +    if (ret < 0) {
> +        fprintf(stderr, "%s: couldn't KVM_CONFIG_TLB: %s\n",
> +                __func__, strerror(-ret));
> +        return ret;
> +    }
> +
> +    env->kvm_sw_tlb = true;
> +#endif
> +
> +    return 0;
> +}
> +
> int kvm_arch_init_vcpu(CPUState *cenv)
> {
>     int ret;
> @@ -133,6 +178,14 @@ int kvm_arch_init_vcpu(CPUState *cenv)
> 
>     idle_timer = qemu_new_timer_ns(vm_clock, kvm_kick_env, cenv);
> 

Please add a comment here, explaining the occasional reader what we're doing 
here

> +    switch (cenv->mmu_model) {
> +    case POWERPC_MMU_BOOKE206:
> +        ret = kvm_booke206_tlb_init(cenv);
> +        break;
> +    default:
> +        break;
> +    }
> +
>     return ret;
> }
> 
> @@ -140,6 +193,33 @@ void kvm_arch_reset_vcpu(CPUState *env)
> {
> }
> 
> +static void kvm_sw_tlb_put(CPUState *env)
> +{
> +#if defined(KVM_CAP_SW_TLB)

See above

> +    struct kvm_dirty_tlb dirty_tlb;
> +    unsigned char *bitmap;
> +    int ret;
> +
> +    if (!env->kvm_sw_tlb) {
> +        return;
> +    }
> +
> +    bitmap = qemu_malloc((env->nb_tlb + 7) / 8);
> +    memset(bitmap, 0xFF, (env->nb_tlb + 7) / 8);
> +
> +    dirty_tlb.bitmap = (uintptr_t)bitmap;
> +    dirty_tlb.num_dirty = env->nb_tlb;

Pretty simple for now, but I like the idea of starting simple :). Would it make 
sense to keep the bitmap allocated throughout the lifetime of env?


Alex