[Qemu-devel] [0/5] Assorted pending pseries machine patches

[Qemu-devel] [0/5] Assorted pending pseries machine patches

[David Gibson]

Hi Alex,

Here's a handful of patches I've had pending in my tree for a while.
They've now been polished up and are ready for merge.

[Qemu-devel] [PATCH 1/5] target-ppc: Synchronize VPA state with KVM

[David Gibson]

For PAPR guests, KVM tracks the various areas registered with the
H_REGISTER_VPA hypercall.  For full emulation, of course, these are tracked
within qemu.  At present these values are not synchronized.  This is a
problem for reset (qemu's reset of the VPA address is not pushed to KVM)
and will also be a problem for savevm / migration.

The kernel now supports accessing the VPA state via the ONE_REG interface,
this patch adds code to qemu to use that interface to keep the qemu and
KVM ideas of the VPA state synchronized.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/kvm.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 67f3f42..f5710b7 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -63,6 +63,7 @@ static int cap_ppc_rma;
 static int cap_spapr_tce;
 static int cap_hior;
 static int cap_one_reg;
+static int cap_papr;
 
 /* XXX We have a race condition where we actually have a level triggered
  *     interrupt, but the infrastructure can't expose that yet, so the guest
@@ -95,6 +96,8 @@ int kvm_arch_init(KVMState *s)
     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
     cap_one_reg = kvm_check_extension(s, KVM_CAP_ONE_REG);
     cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
+    /* Note: we don't set cap_papr here, because this capability is
+     * only activated after this by kvmppc_set_papr() */
 
     if (!cap_interrupt_level) {
         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
@@ -652,6 +655,103 @@ static int kvm_get_fp(CPUState *cs)
     return 0;
 }
 
+#if defined(TARGET_PPC64)
+static int kvm_get_vpa(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int ret;
+
+    reg.id = KVM_REG_PPC_VPA_ADDR;
+    reg.addr = (uintptr_t)&env->vpa_addr;
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret < 0) {
+        dprintf("Unable to get VPA address from KVM: %s\n", strerror(errno));
+        return ret;
+    }
+
+    assert((uintptr_t)&env->slb_shadow_size
+           == ((uintptr_t)&env->slb_shadow_addr + 8));
+    reg.id = KVM_REG_PPC_VPA_SLB;
+    reg.addr = (uintptr_t)&env->slb_shadow_addr;
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret < 0) {
+        dprintf("Unable to get SLB shadow state from KVM: %s\n",
+                strerror(errno));
+        return ret;
+    }
+
+    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
+    reg.id = KVM_REG_PPC_VPA_DTL;
+    reg.addr = (uintptr_t)&env->dtl_addr;
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret < 0) {
+        dprintf("Unable to get dispatch trace log state from KVM: %s\n",
+                strerror(errno));
+        return ret;
+    }
+
+    return 0;
+}
+
+static int kvm_put_vpa(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int ret;
+
+    /* SLB shadow or DTL can't be registered unless a master VPA is
+     * registered.  That means when restoring state, if a VPA *is*
+     * registered, we need to set that up first.  If not, we need to
+     * deregister the others before deregistering the master VPA */
+    assert(env->vpa_addr || !(env->slb_shadow_addr || env->dtl_addr));
+
+    if (env->vpa_addr) {
+        reg.id = KVM_REG_PPC_VPA_ADDR;
+        reg.addr = (uintptr_t)&env->vpa_addr;
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret < 0) {
+            dprintf("Unable to set VPA address to KVM: %s\n", strerror(errno));
+            return ret;
+        }
+    }
+
+    assert((uintptr_t)&env->slb_shadow_size
+           == ((uintptr_t)&env->slb_shadow_addr + 8));
+    reg.id = KVM_REG_PPC_VPA_SLB;
+    reg.addr = (uintptr_t)&env->slb_shadow_addr;
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret < 0) {
+        dprintf("Unable to set SLB shadow state to KVM: %s\n", strerror(errno));
+        return ret;
+    }
+
+    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
+    reg.id = KVM_REG_PPC_VPA_DTL;
+    reg.addr = (uintptr_t)&env->dtl_addr;
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret < 0) {
+        dprintf("Unable to set dispatch trace log state to KVM: %s\n",
+                strerror(errno));
+        return ret;
+    }
+
+    if (!env->vpa_addr) {
+        reg.id = KVM_REG_PPC_VPA_ADDR;
+        reg.addr = (uintptr_t)&env->vpa_addr;
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret < 0) {
+            dprintf("Unable to set VPA address to KVM: %s\n", strerror(errno));
+            return ret;
+        }
+    }
+
+    return 0;
+}
+#endif /* TARGET_PPC64 */
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -752,6 +852,14 @@ int kvm_arch_put_registers(CPUState *cs, int level)
                 kvm_put_one_spr(cs, id, i);
             }
         }
+
+#ifdef TARGET_PPC64
+        if (cap_papr) {
+            if (kvm_put_vpa(cs) < 0) {
+                dprintf("Warning: Unable to set VPA information to KVM\n");
+            }
+        }
+#endif /* TARGET_PPC64 */
     }
 
     return ret;
@@ -953,6 +1061,15 @@ int kvm_arch_get_registers(CPUState *cs)
                 kvm_get_one_spr(cs, id, i);
             }
         }
+
+#ifdef TARGET_PPC64
+        if (cap_papr) {
+            if (kvm_get_vpa(cs) < 0) {
+                fprintf(stderr,
+                        "Warning: Unable to get VPA information from KVM\n");
+            }
+        }
+#endif
     }
 
     return 0;
@@ -1299,6 +1416,10 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
     if (ret) {
         cpu_abort(env, "This KVM version does not support PAPR\n");
     }
+
+    /* Update the capability flag so we sync the right information
+     * with kvm */
+    cap_papr = 1;
 }
 
 void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/5] target-ppc: Synchronize VPA state with KVM

[Alexander Graf]

On 14.03.2013, at 02:53, David Gibson wrote:

> For PAPR guests, KVM tracks the various areas registered with the
> H_REGISTER_VPA hypercall.  For full emulation, of course, these are tracked
> within qemu.  At present these values are not synchronized.  This is a
> problem for reset (qemu's reset of the VPA address is not pushed to KVM)
> and will also be a problem for savevm / migration.
> 
> The kernel now supports accessing the VPA state via the ONE_REG interface,
> this patch adds code to qemu to use that interface to keep the qemu and
> KVM ideas of the VPA state synchronized.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target-ppc/kvm.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 121 insertions(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 67f3f42..f5710b7 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -63,6 +63,7 @@ static int cap_ppc_rma;
> static int cap_spapr_tce;
> static int cap_hior;
> static int cap_one_reg;
> +static int cap_papr;
> 
> /* XXX We have a race condition where we actually have a level triggered
>  *     interrupt, but the infrastructure can't expose that yet, so the guest
> @@ -95,6 +96,8 @@ int kvm_arch_init(KVMState *s)
>     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
>     cap_one_reg = kvm_check_extension(s, KVM_CAP_ONE_REG);
>     cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> +    /* Note: we don't set cap_papr here, because this capability is
> +     * only activated after this by kvmppc_set_papr() */
> 
>     if (!cap_interrupt_level) {
>         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
> @@ -652,6 +655,103 @@ static int kvm_get_fp(CPUState *cs)
>     return 0;
> }
> 
> +#if defined(TARGET_PPC64)
> +static int kvm_get_vpa(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int ret;
> +
> +    reg.id = KVM_REG_PPC_VPA_ADDR;
> +    reg.addr = (uintptr_t)&env->vpa_addr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret < 0) {
> +        dprintf("Unable to get VPA address from KVM: %s\n", strerror(errno));
> +        return ret;
> +    }
> +
> +    assert((uintptr_t)&env->slb_shadow_size
> +           == ((uintptr_t)&env->slb_shadow_addr + 8));
> +    reg.id = KVM_REG_PPC_VPA_SLB;
> +    reg.addr = (uintptr_t)&env->slb_shadow_addr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret < 0) {
> +        dprintf("Unable to get SLB shadow state from KVM: %s\n",
> +                strerror(errno));
> +        return ret;
> +    }
> +
> +    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
> +    reg.id = KVM_REG_PPC_VPA_DTL;
> +    reg.addr = (uintptr_t)&env->dtl_addr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (ret < 0) {
> +        dprintf("Unable to get dispatch trace log state from KVM: %s\n",
> +                strerror(errno));
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
> +static int kvm_put_vpa(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int ret;
> +
> +    /* SLB shadow or DTL can't be registered unless a master VPA is
> +     * registered.  That means when restoring state, if a VPA *is*
> +     * registered, we need to set that up first.  If not, we need to
> +     * deregister the others before deregistering the master VPA */
> +    assert(env->vpa_addr || !(env->slb_shadow_addr || env->dtl_addr));
> +
> +    if (env->vpa_addr) {
> +        reg.id = KVM_REG_PPC_VPA_ADDR;
> +        reg.addr = (uintptr_t)&env->vpa_addr;
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret < 0) {
> +            dprintf("Unable to set VPA address to KVM: %s\n", strerror(errno));
> +            return ret;
> +        }
> +    }
> +
> +    assert((uintptr_t)&env->slb_shadow_size
> +           == ((uintptr_t)&env->slb_shadow_addr + 8));
> +    reg.id = KVM_REG_PPC_VPA_SLB;
> +    reg.addr = (uintptr_t)&env->slb_shadow_addr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret < 0) {
> +        dprintf("Unable to set SLB shadow state to KVM: %s\n", strerror(errno));
> +        return ret;
> +    }
> +
> +    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
> +    reg.id = KVM_REG_PPC_VPA_DTL;
> +    reg.addr = (uintptr_t)&env->dtl_addr;
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret < 0) {
> +        dprintf("Unable to set dispatch trace log state to KVM: %s\n",
> +                strerror(errno));
> +        return ret;
> +    }
> +
> +    if (!env->vpa_addr) {
> +        reg.id = KVM_REG_PPC_VPA_ADDR;
> +        reg.addr = (uintptr_t)&env->vpa_addr;
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret < 0) {
> +            dprintf("Unable to set VPA address to KVM: %s\n", strerror(errno));
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +#endif /* TARGET_PPC64 */
> +
> int kvm_arch_put_registers(CPUState *cs, int level)
> {
>     PowerPCCPU *cpu = POWERPC_CPU(cs);
> @@ -752,6 +852,14 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>                 kvm_put_one_spr(cs, id, i);
>             }
>         }
> +
> +#ifdef TARGET_PPC64
> +        if (cap_papr) {
> +            if (kvm_put_vpa(cs) < 0) {
> +                dprintf("Warning: Unable to set VPA information to KVM\n");
> +            }
> +        }
> +#endif /* TARGET_PPC64 */
>     }
> 
>     return ret;
> @@ -953,6 +1061,15 @@ int kvm_arch_get_registers(CPUState *cs)
>                 kvm_get_one_spr(cs, id, i);
>             }
>         }
> +
> +#ifdef TARGET_PPC64
> +        if (cap_papr) {
> +            if (kvm_get_vpa(cs) < 0) {
> +                fprintf(stderr,
> +                        "Warning: Unable to get VPA information from KVM\n");

This will give odd warnings on old HV KVM systems and PR KVM, no?


Alex

> +            }
> +        }
> +#endif
>     }
> 
>     return 0;
> @@ -1299,6 +1416,10 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
>     if (ret) {
>         cpu_abort(env, "This KVM version does not support PAPR\n");
>     }
> +
> +    /* Update the capability flag so we sync the right information
> +     * with kvm */
> +    cap_papr = 1;
> }
> 
> void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
> -- 
> 1.7.10.4
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/5] target-ppc: Synchronize VPA state with KVM

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6988 bytes --]


On Fri, Mar 15, 2013 at 01:22:08PM +0100, Alexander Graf wrote:
> 
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
> > For PAPR guests, KVM tracks the various areas registered with the
> > H_REGISTER_VPA hypercall.  For full emulation, of course, these are tracked
> > within qemu.  At present these values are not synchronized.  This is a
> > problem for reset (qemu's reset of the VPA address is not pushed to KVM)
> > and will also be a problem for savevm / migration.
> > 
> > The kernel now supports accessing the VPA state via the ONE_REG interface,
> > this patch adds code to qemu to use that interface to keep the qemu and
> > KVM ideas of the VPA state synchronized.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > target-ppc/kvm.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 121 insertions(+)
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 67f3f42..f5710b7 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -63,6 +63,7 @@ static int cap_ppc_rma;
> > static int cap_spapr_tce;
> > static int cap_hior;
> > static int cap_one_reg;
> > +static int cap_papr;
> > 
> > /* XXX We have a race condition where we actually have a level triggered
> >  *     interrupt, but the infrastructure can't expose that yet, so the guest
> > @@ -95,6 +96,8 @@ int kvm_arch_init(KVMState *s)
> >     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> >     cap_one_reg = kvm_check_extension(s, KVM_CAP_ONE_REG);
> >     cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> > +    /* Note: we don't set cap_papr here, because this capability is
> > +     * only activated after this by kvmppc_set_papr() */
> > 
> >     if (!cap_interrupt_level) {
> >         fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
> > @@ -652,6 +655,103 @@ static int kvm_get_fp(CPUState *cs)
> >     return 0;
> > }
> > 
> > +#if defined(TARGET_PPC64)
> > +static int kvm_get_vpa(CPUState *cs)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    int ret;
> > +
> > +    reg.id = KVM_REG_PPC_VPA_ADDR;
> > +    reg.addr = (uintptr_t)&env->vpa_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to get VPA address from KVM: %s\n", strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    assert((uintptr_t)&env->slb_shadow_size
> > +           == ((uintptr_t)&env->slb_shadow_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_SLB;
> > +    reg.addr = (uintptr_t)&env->slb_shadow_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to get SLB shadow state from KVM: %s\n",
> > +                strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_DTL;
> > +    reg.addr = (uintptr_t)&env->dtl_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to get dispatch trace log state from KVM: %s\n",
> > +                strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int kvm_put_vpa(CPUState *cs)
> > +{
> > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct kvm_one_reg reg;
> > +    int ret;
> > +
> > +    /* SLB shadow or DTL can't be registered unless a master VPA is
> > +     * registered.  That means when restoring state, if a VPA *is*
> > +     * registered, we need to set that up first.  If not, we need to
> > +     * deregister the others before deregistering the master VPA */
> > +    assert(env->vpa_addr || !(env->slb_shadow_addr || env->dtl_addr));
> > +
> > +    if (env->vpa_addr) {
> > +        reg.id = KVM_REG_PPC_VPA_ADDR;
> > +        reg.addr = (uintptr_t)&env->vpa_addr;
> > +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +        if (ret < 0) {
> > +            dprintf("Unable to set VPA address to KVM: %s\n", strerror(errno));
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    assert((uintptr_t)&env->slb_shadow_size
> > +           == ((uintptr_t)&env->slb_shadow_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_SLB;
> > +    reg.addr = (uintptr_t)&env->slb_shadow_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to set SLB shadow state to KVM: %s\n", strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    assert((uintptr_t)&env->dtl_size == ((uintptr_t)&env->dtl_addr + 8));
> > +    reg.id = KVM_REG_PPC_VPA_DTL;
> > +    reg.addr = (uintptr_t)&env->dtl_addr;
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +    if (ret < 0) {
> > +        dprintf("Unable to set dispatch trace log state to KVM: %s\n",
> > +                strerror(errno));
> > +        return ret;
> > +    }
> > +
> > +    if (!env->vpa_addr) {
> > +        reg.id = KVM_REG_PPC_VPA_ADDR;
> > +        reg.addr = (uintptr_t)&env->vpa_addr;
> > +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +        if (ret < 0) {
> > +            dprintf("Unable to set VPA address to KVM: %s\n", strerror(errno));
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +#endif /* TARGET_PPC64 */
> > +
> > int kvm_arch_put_registers(CPUState *cs, int level)
> > {
> >     PowerPCCPU *cpu = POWERPC_CPU(cs);
> > @@ -752,6 +852,14 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> >                 kvm_put_one_spr(cs, id, i);
> >             }
> >         }
> > +
> > +#ifdef TARGET_PPC64
> > +        if (cap_papr) {
> > +            if (kvm_put_vpa(cs) < 0) {
> > +                dprintf("Warning: Unable to set VPA information to KVM\n");
> > +            }
> > +        }
> > +#endif /* TARGET_PPC64 */
> >     }
> > 
> >     return ret;
> > @@ -953,6 +1061,15 @@ int kvm_arch_get_registers(CPUState *cs)
> >                 kvm_get_one_spr(cs, id, i);
> >             }
> >         }
> > +
> > +#ifdef TARGET_PPC64
> > +        if (cap_papr) {
> > +            if (kvm_get_vpa(cs) < 0) {
> > +                fprintf(stderr,
> > +                        "Warning: Unable to get VPA information from KVM\n");
> 
> This will give odd warnings on old HV KVM systems and PR KVM, no?

Drat, yes.  Converted the corresponding one on the get path to dprintf
but forgot this one.  Will resend with other pending pseries patches.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[Qemu-devel] [PATCH 2/5] pseries: Remove "busname" property for PCI host bridge

[David Gibson]

Currently the "spapr-pci-host-bridge" device has a "busname" property which
can be used to override the default assignment of qbus names for the bus
subordinate to the PHB.  We use that for the default primary PCI bus, to
make libvirt happy, which expects there to be a bus named simply "pci".
The default qdev core logic would name the bus "pci.0", and the pseries
code would otherwise name it "pci@800000020000000" which is the name it
is given in the device tree based on its BUID.

The "busname" property is rather clunky though, so this patch simplifies
things by just using a special case hack for the default PHB, setting
busname to "pci" when index=0.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c |    2 +-
 hw/spapr_pci.c |   30 ++++++++++++++++++++++--------
 hw/spapr_pci.h |    4 +---
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd24113..9a13697 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -852,7 +852,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     /* Set up PCI */
     spapr_pci_rtas_init();
 
-    phb = spapr_create_phb(spapr, 0, "pci");
+    phb = spapr_create_phb(spapr, 0);
 
     for (i = 0; i < nb_nics; i++) {
         NICInfo *nd = &nd_table[i];
diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
index 36adbc5..42c8b61 100644
--- a/hw/spapr_pci.c
+++ b/hw/spapr_pci.c
@@ -518,6 +518,7 @@ static int spapr_phb_init(SysBusDevice *s)
 {
     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
+    const char *busname;
     char *namebuf;
     int i;
     PCIBus *bus;
@@ -575,9 +576,6 @@ static int spapr_phb_init(SysBusDevice *s)
     }
 
     sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
-    if (!sphb->busname) {
-        sphb->busname = sphb->dtbusname;
-    }
 
     namebuf = alloca(strlen(sphb->dtbusname) + 32);
 
@@ -621,7 +619,26 @@ static int spapr_phb_init(SysBusDevice *s)
                                     &sphb->msiwindow);
     }
 
-    bus = pci_register_bus(DEVICE(s), sphb->busname,
+    /*
+     * Selecting a busname is more complex than you'd think, due to
+     * interacting constraints.  If the user has specified an id
+     * explicitly for the phb , then we want to use the qdev default
+     * of naming the bus based on the bridge device (so the user can
+     * then assign devices to it in the way they expect).  For the
+     * first / default PCI bus (index=0) we want to use just "pci"
+     * because libvirt expects there to be a bus called, simply,
+     * "pci".  Otherwise, we use the same name as in the device tree,
+     * since it's unique by construction, and makes the guest visible
+     * BUID clear.
+     */
+    if (s->qdev.id) {
+        busname = NULL;
+    } else if (sphb->index == 0) {
+        busname = "pci";
+    } else {
+        busname = sphb->dtbusname;
+    }
+    bus = pci_register_bus(DEVICE(s), busname,
                            pci_spapr_set_irq, pci_spapr_map_irq, sphb,
                            &sphb->memspace, &sphb->iospace,
                            PCI_DEVFN(0, 0), PCI_NUM_PINS);
@@ -663,7 +680,6 @@ static void spapr_phb_reset(DeviceState *qdev)
 }
 
 static Property spapr_phb_properties[] = {
-    DEFINE_PROP_STRING("busname", sPAPRPHBState, busname),
     DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
     DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, -1),
     DEFINE_PROP_HEX32("liobn", sPAPRPHBState, dma_liobn, -1),
@@ -694,14 +710,12 @@ static const TypeInfo spapr_phb_info = {
     .class_init    = spapr_phb_class_init,
 };
 
-PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index,
-                               const char *busname)
+PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
 {
     DeviceState *dev;
 
     dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
     qdev_prop_set_uint32(dev, "index", index);
-    qdev_prop_set_string(dev, "busname", busname);
     qdev_init_nofail(dev);
 
     return PCI_HOST_BRIDGE(dev);
diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
index 8bb3c62..8bd8a66 100644
--- a/hw/spapr_pci.h
+++ b/hw/spapr_pci.h
@@ -39,7 +39,6 @@ typedef struct sPAPRPHBState {
 
     int32_t index;
     uint64_t buid;
-    char *busname;
     char *dtbusname;
 
     MemoryRegion memspace, iospace;
@@ -82,8 +81,7 @@ static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
     return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
 }
 
-PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index,
-                               const char *busname);
+PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
 
 int spapr_populate_pci_dt(sPAPRPHBState *phb,
                           uint32_t xics_phandle,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/5] pseries: Remove "busname" property for PCI host bridge

[Alexander Graf]

On 14.03.2013, at 02:53, David Gibson wrote:

> Currently the "spapr-pci-host-bridge" device has a "busname" property which
> can be used to override the default assignment of qbus names for the bus
> subordinate to the PHB.  We use that for the default primary PCI bus, to
> make libvirt happy, which expects there to be a bus named simply "pci".
> The default qdev core logic would name the bus "pci.0", and the pseries
> code would otherwise name it "pci@800000020000000" which is the name it
> is given in the device tree based on its BUID.
> 
> The "busname" property is rather clunky though, so this patch simplifies
> things by just using a special case hack for the default PHB, setting
> busname to "pci" when index=0.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Thanks, applied to ppc-next.

Alex

> ---
> hw/ppc/spapr.c |    2 +-
> hw/spapr_pci.c |   30 ++++++++++++++++++++++--------
> hw/spapr_pci.h |    4 +---
> 3 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index fd24113..9a13697 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -852,7 +852,7 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>     /* Set up PCI */
>     spapr_pci_rtas_init();
> 
> -    phb = spapr_create_phb(spapr, 0, "pci");
> +    phb = spapr_create_phb(spapr, 0);
> 
>     for (i = 0; i < nb_nics; i++) {
>         NICInfo *nd = &nd_table[i];
> diff --git a/hw/spapr_pci.c b/hw/spapr_pci.c
> index 36adbc5..42c8b61 100644
> --- a/hw/spapr_pci.c
> +++ b/hw/spapr_pci.c
> @@ -518,6 +518,7 @@ static int spapr_phb_init(SysBusDevice *s)
> {
>     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>     PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    const char *busname;
>     char *namebuf;
>     int i;
>     PCIBus *bus;
> @@ -575,9 +576,6 @@ static int spapr_phb_init(SysBusDevice *s)
>     }
> 
>     sphb->dtbusname = g_strdup_printf("pci@%" PRIx64, sphb->buid);
> -    if (!sphb->busname) {
> -        sphb->busname = sphb->dtbusname;
> -    }
> 
>     namebuf = alloca(strlen(sphb->dtbusname) + 32);
> 
> @@ -621,7 +619,26 @@ static int spapr_phb_init(SysBusDevice *s)
>                                     &sphb->msiwindow);
>     }
> 
> -    bus = pci_register_bus(DEVICE(s), sphb->busname,
> +    /*
> +     * Selecting a busname is more complex than you'd think, due to
> +     * interacting constraints.  If the user has specified an id
> +     * explicitly for the phb , then we want to use the qdev default
> +     * of naming the bus based on the bridge device (so the user can
> +     * then assign devices to it in the way they expect).  For the
> +     * first / default PCI bus (index=0) we want to use just "pci"
> +     * because libvirt expects there to be a bus called, simply,
> +     * "pci".  Otherwise, we use the same name as in the device tree,
> +     * since it's unique by construction, and makes the guest visible
> +     * BUID clear.
> +     */
> +    if (s->qdev.id) {
> +        busname = NULL;
> +    } else if (sphb->index == 0) {
> +        busname = "pci";
> +    } else {
> +        busname = sphb->dtbusname;
> +    }
> +    bus = pci_register_bus(DEVICE(s), busname,
>                            pci_spapr_set_irq, pci_spapr_map_irq, sphb,
>                            &sphb->memspace, &sphb->iospace,
>                            PCI_DEVFN(0, 0), PCI_NUM_PINS);
> @@ -663,7 +680,6 @@ static void spapr_phb_reset(DeviceState *qdev)
> }
> 
> static Property spapr_phb_properties[] = {
> -    DEFINE_PROP_STRING("busname", sPAPRPHBState, busname),
>     DEFINE_PROP_INT32("index", sPAPRPHBState, index, -1),
>     DEFINE_PROP_HEX64("buid", sPAPRPHBState, buid, -1),
>     DEFINE_PROP_HEX32("liobn", sPAPRPHBState, dma_liobn, -1),
> @@ -694,14 +710,12 @@ static const TypeInfo spapr_phb_info = {
>     .class_init    = spapr_phb_class_init,
> };
> 
> -PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index,
> -                               const char *busname)
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index)
> {
>     DeviceState *dev;
> 
>     dev = qdev_create(NULL, TYPE_SPAPR_PCI_HOST_BRIDGE);
>     qdev_prop_set_uint32(dev, "index", index);
> -    qdev_prop_set_string(dev, "busname", busname);
>     qdev_init_nofail(dev);
> 
>     return PCI_HOST_BRIDGE(dev);
> diff --git a/hw/spapr_pci.h b/hw/spapr_pci.h
> index 8bb3c62..8bd8a66 100644
> --- a/hw/spapr_pci.h
> +++ b/hw/spapr_pci.h
> @@ -39,7 +39,6 @@ typedef struct sPAPRPHBState {
> 
>     int32_t index;
>     uint64_t buid;
> -    char *busname;
>     char *dtbusname;
> 
>     MemoryRegion memspace, iospace;
> @@ -82,8 +81,7 @@ static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>     return xics_get_qirq(spapr->icp, phb->lsi_table[pin].irq);
> }
> 
> -PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index,
> -                               const char *busname);
> +PCIHostState *spapr_create_phb(sPAPREnvironment *spapr, int index);
> 
> int spapr_populate_pci_dt(sPAPRPHBState *phb,
>                           uint32_t xics_phandle,
> -- 
> 1.7.10.4
> 

[Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[David Gibson]

PAPR requires that the device tree's CPU nodes have several properties
with information about the L1 cache.  We already create two of these
properties, but with incorrect names - "[id]cache-block-size" instead
of "[id]-cache-block-size" (note the extra hyphen).

We were also missing some of the required cache properties.  This
patch adds the [id]-cache-line-size properties (which have the same
values as the block size properties in all current cases).  We also
add the [id]-cache-size properties.

Adding the cache sizes requires some extra infrastructure in the
general target-ppc code to (optionally) set the cache sizes for
various CPUs.  The CPU family descriptions in translate_init.c can set
these sizes - this patch adds correct information for POWER7, I'm
leaving other CPU types to people who have a physical example to
verify against.  In addition, for -cpu host we take the values
advertised by the host (if available) and use those to override the
information based on PVR.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              |   20 ++++++++++++++++++--
 target-ppc/cpu.h            |    1 +
 target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
 target-ppc/translate_init.c |    4 ++++
 4 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9a13697..7293082 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
 
         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
-        _FDT((fdt_property_cell(fdt, "dcache-block-size",
+        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
                                 env->dcache_line_size)));
-        _FDT((fdt_property_cell(fdt, "icache-block-size",
+        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
+                                env->dcache_line_size)));
+        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
+                                env->icache_line_size)));
+        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
                                 env->icache_line_size)));
+
+        if (env->l1_dcache_size) {
+            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
+        } else {
+            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
+        }
+        if (env->l1_icache_size) {
+            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
+        } else {
+            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
+        }
+
         _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq)));
         _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq)));
         _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr)));
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 0b2d0b9..793ae0f 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -994,6 +994,7 @@ struct CPUPPCState {
 
     int dcache_line_size;
     int icache_line_size;
+    int l1_dcache_size, l1_icache_size;
 
     /* Those resources are used during exception processing */
     /* CPU model definition */
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index f5710b7..eaa11a1 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1612,14 +1612,49 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
     }
 }
 
+typedef struct PowerPCHostCPUClass {
+    /*< private >*/
+    PowerPCCPUClass parent_class;
+    /*< public >*/
+
+    DeviceRealize parent_realize;
+} PowerPCHostCPUClass;
+
+#define POWERPC_HOST_CPU_CLASS(klass) \
+    OBJECT_CLASS_CHECK(PowerPCHostCPUClass, (klass), TYPE_HOST_POWERPC_CPU)
+#define POWERPC_HOST_CPU_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PowerPCHostCPUClass, (obj), TYPE_HOST_POWERPC_CPU)
+
 static void kvmppc_host_cpu_initfn(Object *obj)
 {
     assert(kvm_enabled());
 }
 
+static void kvmppc_host_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(dev);
+    CPUPPCState *env = &cpu->env;
+    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_GET_CLASS(dev);
+    uint32_t val;
+
+    (*hpcc->parent_realize)(dev, errp);
+
+    val = kvmppc_read_int_cpu_dt("d-cache-size");
+    if (val != -1) {
+        env->l1_dcache_size = val;
+    }
+
+    val = kvmppc_read_int_cpu_dt("i-cache-size");
+    if (val != -1) {
+        env->l1_icache_size = val;
+    }
+}
+
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 {
+    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_CLASS(oc);
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
     uint32_t vmx = kvmppc_get_vmx();
     uint32_t dfp = kvmppc_get_dfp();
 
@@ -1634,6 +1669,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
         /* Only override when we know what the host supports */
         alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
     }
+
+    hpcc->parent_realize = dc->realize;
+    dc->realize = kvmppc_host_cpu_realizefn;
 }
 
 int kvmppc_fixup_cpu(PowerPCCPU *cpu)
@@ -1654,6 +1692,7 @@ static int kvm_ppc_register_host_cpu_type(void)
     TypeInfo type_info = {
         .name = TYPE_HOST_POWERPC_CPU,
         .instance_init = kvmppc_host_cpu_initfn,
+        .class_size = sizeof(PowerPCHostCPUClass),
         .class_init = kvmppc_host_cpu_class_init,
     };
     uint32_t host_pvr = mfpvr();
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 3cf440e..594053e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -6989,6 +6989,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
     init_excp_POWER7(env);
     env->dcache_line_size = 128;
     env->icache_line_size = 128;
+
+    env->l1_dcache_size = 0x8000;
+    env->l1_icache_size = 0x8000;
+
     /* Allocate hardware IRQ controller */
     ppcPOWER7_irq_init(env);
     /* Can't find information on what this should be on reset.  This
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Alexander Graf]

On 14.03.2013, at 02:53, David Gibson wrote:

> PAPR requires that the device tree's CPU nodes have several properties
> with information about the L1 cache.  We already create two of these
> properties, but with incorrect names - "[id]cache-block-size" instead
> of "[id]-cache-block-size" (note the extra hyphen).
> 
> We were also missing some of the required cache properties.  This
> patch adds the [id]-cache-line-size properties (which have the same
> values as the block size properties in all current cases).  We also
> add the [id]-cache-size properties.
> 
> Adding the cache sizes requires some extra infrastructure in the
> general target-ppc code to (optionally) set the cache sizes for
> various CPUs.  The CPU family descriptions in translate_init.c can set
> these sizes - this patch adds correct information for POWER7, I'm
> leaving other CPU types to people who have a physical example to
> verify against.  In addition, for -cpu host we take the values
> advertised by the host (if available) and use those to override the
> information based on PVR.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
> target-ppc/cpu.h            |    1 +
> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
> target-ppc/translate_init.c |    4 ++++
> 4 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9a13697..7293082 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
> 
>         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>                                 env->dcache_line_size)));
> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
> +                                env->dcache_line_size)));
> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
> +                                env->icache_line_size)));
> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>                                 env->icache_line_size)));
> +
> +        if (env->l1_dcache_size) {
> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
> +        } else {
> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> +        }
> +        if (env->l1_icache_size) {
> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
> +        } else {
> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> +        }

The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?


Alex

> +
>         _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq)));
>         _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq)));
>         _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr)));
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 0b2d0b9..793ae0f 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -994,6 +994,7 @@ struct CPUPPCState {
> 
>     int dcache_line_size;
>     int icache_line_size;
> +    int l1_dcache_size, l1_icache_size;
> 
>     /* Those resources are used during exception processing */
>     /* CPU model definition */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index f5710b7..eaa11a1 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1612,14 +1612,49 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>     }
> }
> 
> +typedef struct PowerPCHostCPUClass {
> +    /*< private >*/
> +    PowerPCCPUClass parent_class;
> +    /*< public >*/
> +
> +    DeviceRealize parent_realize;
> +} PowerPCHostCPUClass;
> +
> +#define POWERPC_HOST_CPU_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(PowerPCHostCPUClass, (klass), TYPE_HOST_POWERPC_CPU)
> +#define POWERPC_HOST_CPU_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(PowerPCHostCPUClass, (obj), TYPE_HOST_POWERPC_CPU)
> +
> static void kvmppc_host_cpu_initfn(Object *obj)
> {
>     assert(kvm_enabled());
> }
> 
> +static void kvmppc_host_cpu_realizefn(DeviceState *dev, Error **errp)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    CPUPPCState *env = &cpu->env;
> +    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_GET_CLASS(dev);
> +    uint32_t val;
> +
> +    (*hpcc->parent_realize)(dev, errp);
> +
> +    val = kvmppc_read_int_cpu_dt("d-cache-size");
> +    if (val != -1) {
> +        env->l1_dcache_size = val;
> +    }
> +
> +    val = kvmppc_read_int_cpu_dt("i-cache-size");
> +    if (val != -1) {
> +        env->l1_icache_size = val;
> +    }
> +}
> +
> static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> {
> +    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_CLASS(oc);
>     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>     uint32_t vmx = kvmppc_get_vmx();
>     uint32_t dfp = kvmppc_get_dfp();
> 
> @@ -1634,6 +1669,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>         /* Only override when we know what the host supports */
>         alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
>     }
> +
> +    hpcc->parent_realize = dc->realize;
> +    dc->realize = kvmppc_host_cpu_realizefn;
> }
> 
> int kvmppc_fixup_cpu(PowerPCCPU *cpu)
> @@ -1654,6 +1692,7 @@ static int kvm_ppc_register_host_cpu_type(void)
>     TypeInfo type_info = {
>         .name = TYPE_HOST_POWERPC_CPU,
>         .instance_init = kvmppc_host_cpu_initfn,
> +        .class_size = sizeof(PowerPCHostCPUClass),
>         .class_init = kvmppc_host_cpu_class_init,
>     };
>     uint32_t host_pvr = mfpvr();
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 3cf440e..594053e 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -6989,6 +6989,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
>     init_excp_POWER7(env);
>     env->dcache_line_size = 128;
>     env->icache_line_size = 128;
> +
> +    env->l1_dcache_size = 0x8000;
> +    env->l1_icache_size = 0x8000;
> +
>     /* Allocate hardware IRQ controller */
>     ppcPOWER7_irq_init(env);
>     /* Can't find information on what this should be on reset.  This
> -- 
> 1.7.10.4
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3728 bytes --]

On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote:
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
> > PAPR requires that the device tree's CPU nodes have several properties
> > with information about the L1 cache.  We already create two of these
> > properties, but with incorrect names - "[id]cache-block-size" instead
> > of "[id]-cache-block-size" (note the extra hyphen).
> > 
> > We were also missing some of the required cache properties.  This
> > patch adds the [id]-cache-line-size properties (which have the same
> > values as the block size properties in all current cases).  We also
> > add the [id]-cache-size properties.
> > 
> > Adding the cache sizes requires some extra infrastructure in the
> > general target-ppc code to (optionally) set the cache sizes for
> > various CPUs.  The CPU family descriptions in translate_init.c can set
> > these sizes - this patch adds correct information for POWER7, I'm
> > leaving other CPU types to people who have a physical example to
> > verify against.  In addition, for -cpu host we take the values
> > advertised by the host (if available) and use those to override the
> > information based on PVR.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c              |   20 ++++++++++++++++++--
> > target-ppc/cpu.h            |    1 +
> > target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
> > target-ppc/translate_init.c |    4 ++++
> > 4 files changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 9a13697..7293082 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
> >         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
> > 
> >         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
> > -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
> > +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
> >                                 env->dcache_line_size)));
> > -        _FDT((fdt_property_cell(fdt, "icache-block-size",
> > +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
> > +                                env->dcache_line_size)));
> > +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
> > +                                env->icache_line_size)));
> > +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
> >                                 env->icache_line_size)));
> > +
> > +        if (env->l1_dcache_size) {
> > +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
> > +        } else {
> > +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> > +        }
> > +        if (env->l1_icache_size) {
> > +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
> > +        } else {
> > +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> > +        }
> 
> The L1 sizes should come from the class, not env, right? Andreas,
> any ideas on this?

Well.. initially I was going to put them in class.  But then it
occurred to me that the class represents a family of similar CPUs, not
a single precise CPU model.  Total cache sizes are the sort of thing
that could easily vary between minor revisions, although I don't know
if they have in practice.  So I thought it was safer to put these in
env.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Andreas Färber]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 16.03.2013 08:10, schrieb David Gibson:
> On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote:
>> On 14.03.2013, at 02:53, David Gibson wrote:
>> 
>>> PAPR requires that the device tree's CPU nodes have several
>>> properties with information about the L1 cache.  We already
>>> create two of these properties, but with incorrect names -
>>> "[id]cache-block-size" instead of "[id]-cache-block-size" (note
>>> the extra hyphen).
>>> 
>>> We were also missing some of the required cache properties.
>>> This patch adds the [id]-cache-line-size properties (which have
>>> the same values as the block size properties in all current
>>> cases).  We also add the [id]-cache-size properties.
>>> 
>>> Adding the cache sizes requires some extra infrastructure in
>>> the general target-ppc code to (optionally) set the cache sizes
>>> for various CPUs.  The CPU family descriptions in
>>> translate_init.c can set these sizes - this patch adds correct
>>> information for POWER7, I'm leaving other CPU types to people
>>> who have a physical example to verify against.  In addition,
>>> for -cpu host we take the values advertised by the host (if
>>> available) and use those to override the information based on
>>> PVR.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- 
>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++-- 
>>> target-ppc/cpu.h            |    1 + target-ppc/kvm.c
>>> |   39 +++++++++++++++++++++++++++++++++++++++ 
>>> target-ppc/translate_init.c |    4 ++++ 4 files changed, 62
>>> insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index
>>> 9a13697..7293082 100644 --- a/hw/ppc/spapr.c +++
>>> b/hw/ppc/spapr.c @@ -333,10 +333,26 @@ static void
>>> *spapr_create_fdt_skel(const char *cpu_model, 
>>> _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>> 
>>> _FDT((fdt_property_cell(fdt, "cpu-version",
>>> env->spr[SPR_PVR]))); -        _FDT((fdt_property_cell(fdt,
>>> "dcache-block-size", +        _FDT((fdt_property_cell(fdt,
>>> "d-cache-block-size", env->dcache_line_size))); -
>>> _FDT((fdt_property_cell(fdt, "icache-block-size", +
>>> _FDT((fdt_property_cell(fdt, "d-cache-line-size", +
>>> env->dcache_line_size))); +        _FDT((fdt_property_cell(fdt,
>>> "i-cache-block-size", +
>>> env->icache_line_size))); +        _FDT((fdt_property_cell(fdt,
>>> "i-cache-line-size", env->icache_line_size))); + +        if
>>> (env->l1_dcache_size) { +
>>> _FDT((fdt_property_cell(fdt, "d-cache-size",
>>> env->l1_dcache_size))); +        } else { +
>>> fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n"); +
>>> } +        if (env->l1_icache_size) { +
>>> _FDT((fdt_property_cell(fdt, "i-cache-size",
>>> env->l1_icache_size))); +        } else { +
>>> fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n"); +
>>> }
>> 
>> The L1 sizes should come from the class, not env, right?
>> Andreas, any ideas on this?
> 
> Well.. initially I was going to put them in class.  But then it 
> occurred to me that the class represents a family of similar CPUs,
> not a single precise CPU model.  Total cache sizes are the sort of
> thing that could easily vary between minor revisions, although I
> don't know if they have in practice.

Actually that is irrelevant: As it stands, we can do neither
model-specific instance_init nor class_init due to the old
POWERPC_DEF_SVR() macros. Adding support for either seems equally
invasive.

As I just replied to Alex, the question to ask is whether you want the
user to fiddle with this value or not.

Andreas

> So I thought it was safer to put these in env.

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nrnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend￶rffer; HRB 16746 AG Nrnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJRRvYUAAoJEPou0S0+fgE/TuUP/2ZqXQw9klNYkkKfeA8lml8R
jX6NsdIGDxAh8BQMea5YEVfTUUqHa5ygO4HMSBT42GvWR4gY7lmDO08d2FrtO3Oz
ybWf1OubcNm3rp9vwtGKKWxWAiMBSjv9KqAR2gYd305e15ADr3wzRPRPyUtOXO6r
ute+snXu3rI49wCngAF3zBhQnq+HLyUcLpMn/ss/p0086e5LaPhPjEjUDRoKLED6
xqjG3x65rknpWdRPWtpZJHEhPfuDBKpZTS0XdPUkBQ48F2D0yZ3SbRHfIjnikEtV
G43RXBYAUK449HmJY0/hz5W51Nm81f1Sg04uXDLsG9K7mBNUcw/TxgjzW+7ByVyk
+zjCHsC0aimX7owIgMPNd8wtWXmxs4JxdC7icjDtZUNKUSFHC6plMPInlZdIkTU5
brJq02I16lQCYHfnFkFdUM+pwV7r4B0FtymirEX3G8l3n5ddoFmj4tUmLPdjxcHL
Dzn5LMOGNsvL0hqf5eW5JCcq4FMGIYKkeo839+p2u4nnoYSpNIB7mseZzzTTGEYH
HLMTXRijpX9Po65fMgPkiyv9Mv3w2N7UjjP2E9a2VJk2oRsEMO0GXrV3OUJBD64U
3qmB+xvjLgijrDvlYTWvZtOhqIRuqjtS+La2qxtbWE8Mf3YYZ1UxVFPd82qQf0zw
luXozSihIlcx55xtw9Lr
=ejJS
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4206 bytes --]

On Mon, Mar 18, 2013 at 12:10:12PM +0100, Andreas Färber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Am 16.03.2013 08:10, schrieb David Gibson:
> > On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote:
> >> On 14.03.2013, at 02:53, David Gibson wrote:
> >> 
> >>> PAPR requires that the device tree's CPU nodes have several
> >>> properties with information about the L1 cache.  We already
> >>> create two of these properties, but with incorrect names -
> >>> "[id]cache-block-size" instead of "[id]-cache-block-size" (note
> >>> the extra hyphen).
> >>> 
> >>> We were also missing some of the required cache properties.
> >>> This patch adds the [id]-cache-line-size properties (which have
> >>> the same values as the block size properties in all current
> >>> cases).  We also add the [id]-cache-size properties.
> >>> 
> >>> Adding the cache sizes requires some extra infrastructure in
> >>> the general target-ppc code to (optionally) set the cache sizes
> >>> for various CPUs.  The CPU family descriptions in
> >>> translate_init.c can set these sizes - this patch adds correct
> >>> information for POWER7, I'm leaving other CPU types to people
> >>> who have a physical example to verify against.  In addition,
> >>> for -cpu host we take the values advertised by the host (if
> >>> available) and use those to override the information based on
> >>> PVR.
> >>> 
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- 
> >>> hw/ppc/spapr.c              |   20 ++++++++++++++++++-- 
> >>> target-ppc/cpu.h            |    1 + target-ppc/kvm.c
> >>> |   39 +++++++++++++++++++++++++++++++++++++++ 
> >>> target-ppc/translate_init.c |    4 ++++ 4 files changed, 62
> >>> insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index
> >>> 9a13697..7293082 100644 --- a/hw/ppc/spapr.c +++
> >>> b/hw/ppc/spapr.c @@ -333,10 +333,26 @@ static void
> >>> *spapr_create_fdt_skel(const char *cpu_model, 
> >>> _FDT((fdt_property_string(fdt, "device_type", "cpu")));
> >>> 
> >>> _FDT((fdt_property_cell(fdt, "cpu-version",
> >>> env->spr[SPR_PVR]))); -        _FDT((fdt_property_cell(fdt,
> >>> "dcache-block-size", +        _FDT((fdt_property_cell(fdt,
> >>> "d-cache-block-size", env->dcache_line_size))); -
> >>> _FDT((fdt_property_cell(fdt, "icache-block-size", +
> >>> _FDT((fdt_property_cell(fdt, "d-cache-line-size", +
> >>> env->dcache_line_size))); +        _FDT((fdt_property_cell(fdt,
> >>> "i-cache-block-size", +
> >>> env->icache_line_size))); +        _FDT((fdt_property_cell(fdt,
> >>> "i-cache-line-size", env->icache_line_size))); + +        if
> >>> (env->l1_dcache_size) { +
> >>> _FDT((fdt_property_cell(fdt, "d-cache-size",
> >>> env->l1_dcache_size))); +        } else { +
> >>> fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n"); +
> >>> } +        if (env->l1_icache_size) { +
> >>> _FDT((fdt_property_cell(fdt, "i-cache-size",
> >>> env->l1_icache_size))); +        } else { +
> >>> fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n"); +
> >>> }
> >> 
> >> The L1 sizes should come from the class, not env, right?
> >> Andreas, any ideas on this?
> > 
> > Well.. initially I was going to put them in class.  But then it 
> > occurred to me that the class represents a family of similar CPUs,
> > not a single precise CPU model.  Total cache sizes are the sort of
> > thing that could easily vary between minor revisions, although I
> > don't know if they have in practice.
> 
> Actually that is irrelevant: As it stands, we can do neither
> model-specific instance_init nor class_init due to the old
> POWERPC_DEF_SVR() macros. Adding support for either seems equally
> invasive.
> 
> As I just replied to Alex, the question to ask is whether you want the
> user to fiddle with this value or not.

User to fiddle, probably not.  But I was thinking of submodel specific
conditionals in the init_proc function.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Andreas Färber]

Am 15.03.2013 13:27, schrieb Alexander Graf:
> 
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
>> PAPR requires that the device tree's CPU nodes have several properties
>> with information about the L1 cache.  We already create two of these
>> properties, but with incorrect names - "[id]cache-block-size" instead
>> of "[id]-cache-block-size" (note the extra hyphen).
>>
>> We were also missing some of the required cache properties.  This
>> patch adds the [id]-cache-line-size properties (which have the same
>> values as the block size properties in all current cases).  We also
>> add the [id]-cache-size properties.
>>
>> Adding the cache sizes requires some extra infrastructure in the
>> general target-ppc code to (optionally) set the cache sizes for
>> various CPUs.  The CPU family descriptions in translate_init.c can set
>> these sizes - this patch adds correct information for POWER7, I'm
>> leaving other CPU types to people who have a physical example to
>> verify against.  In addition, for -cpu host we take the values
>> advertised by the host (if available) and use those to override the
>> information based on PVR.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>> target-ppc/cpu.h            |    1 +
>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>> target-ppc/translate_init.c |    4 ++++
>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 9a13697..7293082 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>
>>         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>                                 env->dcache_line_size)));
>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>> +                                env->dcache_line_size)));
>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>> +                                env->icache_line_size)));
>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>                                 env->icache_line_size)));
>> +
>> +        if (env->l1_dcache_size) {
>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>> +        } else {
>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>> +        }
>> +        if (env->l1_icache_size) {
>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>> +        } else {
>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>> +        }
> 
> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?

Generally speaking,

CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
for legacy grouping reasons).

PowerPCCPU: If you ever intend to let the user override this value
(per-instance) from the command line.

PowerPCCPUClass: If the value is always constant at runtime.

I can't tell from a brief look at this patch which may be the case here.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Alexander Graf]

On 18.03.2013, at 11:54, Andreas Färber wrote:

> Am 15.03.2013 13:27, schrieb Alexander Graf:
>> 
>> On 14.03.2013, at 02:53, David Gibson wrote:
>> 
>>> PAPR requires that the device tree's CPU nodes have several properties
>>> with information about the L1 cache.  We already create two of these
>>> properties, but with incorrect names - "[id]cache-block-size" instead
>>> of "[id]-cache-block-size" (note the extra hyphen).
>>> 
>>> We were also missing some of the required cache properties.  This
>>> patch adds the [id]-cache-line-size properties (which have the same
>>> values as the block size properties in all current cases).  We also
>>> add the [id]-cache-size properties.
>>> 
>>> Adding the cache sizes requires some extra infrastructure in the
>>> general target-ppc code to (optionally) set the cache sizes for
>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>> these sizes - this patch adds correct information for POWER7, I'm
>>> leaving other CPU types to people who have a physical example to
>>> verify against.  In addition, for -cpu host we take the values
>>> advertised by the host (if available) and use those to override the
>>> information based on PVR.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>> target-ppc/cpu.h            |    1 +
>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>> target-ppc/translate_init.c |    4 ++++
>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 9a13697..7293082 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>>        _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>> 
>>>        _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>>                                env->dcache_line_size)));
>>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>>> +                                env->dcache_line_size)));
>>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>>> +                                env->icache_line_size)));
>>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>>                                env->icache_line_size)));
>>> +
>>> +        if (env->l1_dcache_size) {
>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>> +        } else {
>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>> +        }
>>> +        if (env->l1_icache_size) {
>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>> +        } else {
>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>> +        }
>> 
>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
> 
> Generally speaking,
> 
> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
> for legacy grouping reasons).
> 
> PowerPCCPU: If you ever intend to let the user override this value
> (per-instance) from the command line.
> 
> PowerPCCPUClass: If the value is always constant at runtime.
> 
> I can't tell from a brief look at this patch which may be the case here.

Imagine the following:

  PPC
    `- POWER7
        |- POWER7_v10
        `- POWER7_v20

Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.


Alex

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Andreas Färber]

Am 18.03.2013 12:05, schrieb Alexander Graf:
> 
> On 18.03.2013, at 11:54, Andreas Färber wrote:
> 
>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>
>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>
>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>> with information about the L1 cache.  We already create two of these
>>>> properties, but with incorrect names - "[id]cache-block-size" instead
>>>> of "[id]-cache-block-size" (note the extra hyphen).
>>>>
>>>> We were also missing some of the required cache properties.  This
>>>> patch adds the [id]-cache-line-size properties (which have the same
>>>> values as the block size properties in all current cases).  We also
>>>> add the [id]-cache-size properties.
>>>>
>>>> Adding the cache sizes requires some extra infrastructure in the
>>>> general target-ppc code to (optionally) set the cache sizes for
>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>> leaving other CPU types to people who have a physical example to
>>>> verify against.  In addition, for -cpu host we take the values
>>>> advertised by the host (if available) and use those to override the
>>>> information based on PVR.
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>> target-ppc/cpu.h            |    1 +
>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>> target-ppc/translate_init.c |    4 ++++
>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 9a13697..7293082 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>>>        _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>>>
>>>>        _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>>>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>>>                                env->dcache_line_size)));
>>>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>>>> +                                env->dcache_line_size)));
>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>>>> +                                env->icache_line_size)));
>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>>>                                env->icache_line_size)));
>>>> +
>>>> +        if (env->l1_dcache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>> +        }
>>>> +        if (env->l1_icache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>> +        }
>>>
>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>
>> Generally speaking,
>>
>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>> for legacy grouping reasons).
>>
>> PowerPCCPU: If you ever intend to let the user override this value
>> (per-instance) from the command line.
>>
>> PowerPCCPUClass: If the value is always constant at runtime.
>>
>> I can't tell from a brief look at this patch which may be the case here.
> 
> Imagine the following:
> 
>   PPC
>     `- POWER7
>         |- POWER7_v10
>         `- POWER7_v20
> 
> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.

Alex, you asked for an answer here, but I don't spot a question. :-)

I'm guessing requirements like these mean that we need to introduce a
new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
macro to put additional code into class_init and instance_init
respectively and let _DEF() and _DEF_SVR() pass nothing there...
Possibly add specific macros wrapping the above to keep the model list
readable.

Either way there's a trade-off between keeping easy macros hiding QOM
boilerplate code vs. having high flexibility of what code to put in
there - that's why I resisted your requests to hide too much in macros
at the family level.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Alexander Graf]

On 19.03.2013, at 12:06, Andreas Färber wrote:

> Am 18.03.2013 12:05, schrieb Alexander Graf:
>> 
>> On 18.03.2013, at 11:54, Andreas Färber wrote:
>> 
>>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>> 
>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>> 
>>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>>> with information about the L1 cache.  We already create two of these
>>>>> properties, but with incorrect names - "[id]cache-block-size" instead
>>>>> of "[id]-cache-block-size" (note the extra hyphen).
>>>>> 
>>>>> We were also missing some of the required cache properties.  This
>>>>> patch adds the [id]-cache-line-size properties (which have the same
>>>>> values as the block size properties in all current cases).  We also
>>>>> add the [id]-cache-size properties.
>>>>> 
>>>>> Adding the cache sizes requires some extra infrastructure in the
>>>>> general target-ppc code to (optionally) set the cache sizes for
>>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>>> leaving other CPU types to people who have a physical example to
>>>>> verify against.  In addition, for -cpu host we take the values
>>>>> advertised by the host (if available) and use those to override the
>>>>> information based on PVR.
>>>>> 
>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>> ---
>>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>>> target-ppc/cpu.h            |    1 +
>>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>>> target-ppc/translate_init.c |    4 ++++
>>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 9a13697..7293082 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>>>>       _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>>>> 
>>>>>       _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>>>>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>>>>                               env->dcache_line_size)));
>>>>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>>>>> +                                env->dcache_line_size)));
>>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>>>>> +                                env->icache_line_size)));
>>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>>>>                               env->icache_line_size)));
>>>>> +
>>>>> +        if (env->l1_dcache_size) {
>>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>>> +        } else {
>>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>>> +        }
>>>>> +        if (env->l1_icache_size) {
>>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>>> +        } else {
>>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>>> +        }
>>>> 
>>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>> 
>>> Generally speaking,
>>> 
>>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>>> for legacy grouping reasons).
>>> 
>>> PowerPCCPU: If you ever intend to let the user override this value
>>> (per-instance) from the command line.
>>> 
>>> PowerPCCPUClass: If the value is always constant at runtime.
>>> 
>>> I can't tell from a brief look at this patch which may be the case here.
>> 
>> Imagine the following:
>> 
>>  PPC
>>    `- POWER7
>>        |- POWER7_v10
>>        `- POWER7_v20
>> 
>> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.
> 
> Alex, you asked for an answer here, but I don't spot a question. :-)
> 
> I'm guessing requirements like these mean that we need to introduce a
> new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
> macro to put additional code into class_init and instance_init
> respectively and let _DEF() and _DEF_SVR() pass nothing there...
> Possibly add specific macros wrapping the above to keep the model list
> readable.
> 
> Either way there's a trade-off between keeping easy macros hiding QOM
> boilerplate code vs. having high flexibility of what code to put in
> there - that's why I resisted your requests to hide too much in macros
> at the family level.

Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which would pass in a model specific initialization function in addition to the easy to read defaults? :)


Alex

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Andreas Färber]

Am 19.03.2013 12:09, schrieb Alexander Graf:
> 
> On 19.03.2013, at 12:06, Andreas Färber wrote:
> 
>> Am 18.03.2013 12:05, schrieb Alexander Graf:
>>>
>>> On 18.03.2013, at 11:54, Andreas Färber wrote:
>>>
>>>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>>>
>>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>>>
>>>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>>>> with information about the L1 cache.  We already create two of these
>>>>>> properties, but with incorrect names - "[id]cache-block-size" instead
>>>>>> of "[id]-cache-block-size" (note the extra hyphen).
>>>>>>
>>>>>> We were also missing some of the required cache properties.  This
>>>>>> patch adds the [id]-cache-line-size properties (which have the same
>>>>>> values as the block size properties in all current cases).  We also
>>>>>> add the [id]-cache-size properties.
>>>>>>
>>>>>> Adding the cache sizes requires some extra infrastructure in the
>>>>>> general target-ppc code to (optionally) set the cache sizes for
>>>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>>>> leaving other CPU types to people who have a physical example to
>>>>>> verify against.  In addition, for -cpu host we take the values
>>>>>> advertised by the host (if available) and use those to override the
>>>>>> information based on PVR.
>>>>>>
>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>> ---
>>>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>>>> target-ppc/cpu.h            |    1 +
>>>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>>>> target-ppc/translate_init.c |    4 ++++
>>>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>> index 9a13697..7293082 100644
>>>>>> --- a/hw/ppc/spapr.c
>>>>>> +++ b/hw/ppc/spapr.c
>>>>>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>>>>>       _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>>>>>
>>>>>>       _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>>>>>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>>>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>>>>>                               env->dcache_line_size)));
>>>>>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>>>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>>>>>> +                                env->dcache_line_size)));
>>>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>>>>>> +                                env->icache_line_size)));
>>>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>>>>>                               env->icache_line_size)));
>>>>>> +
>>>>>> +        if (env->l1_dcache_size) {
>>>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>>>> +        } else {
>>>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>>>> +        }
>>>>>> +        if (env->l1_icache_size) {
>>>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>>>> +        } else {
>>>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>>>> +        }
>>>>>
>>>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>>>
>>>> Generally speaking,
>>>>
>>>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>>>> for legacy grouping reasons).
>>>>
>>>> PowerPCCPU: If you ever intend to let the user override this value
>>>> (per-instance) from the command line.
>>>>
>>>> PowerPCCPUClass: If the value is always constant at runtime.
>>>>
>>>> I can't tell from a brief look at this patch which may be the case here.
>>>
>>> Imagine the following:
>>>
>>>  PPC
>>>    `- POWER7
>>>        |- POWER7_v10
>>>        `- POWER7_v20
>>>
>>> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.
>>
>> Alex, you asked for an answer here, but I don't spot a question. :-)
>>
>> I'm guessing requirements like these mean that we need to introduce a
>> new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
>> macro to put additional code into class_init and instance_init
>> respectively and let _DEF() and _DEF_SVR() pass nothing there...
>> Possibly add specific macros wrapping the above to keep the model list
>> readable.
>>
>> Either way there's a trade-off between keeping easy macros hiding QOM
>> boilerplate code vs. having high flexibility of what code to put in
>> there - that's why I resisted your requests to hide too much in macros
>> at the family level.
> 
> Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which would pass in a model specific initialization function in addition to the easy to read defaults? :)

A function would work as well, yes. But unless you've reworked the list
you do need the SVR in the base macro.
For the case at hand you only need code for class_init but I was
thinking ahead to also prepare the instance_init equivalent while at it. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Alexander Graf]

On 19.03.2013, at 12:16, Andreas Färber wrote:

> Am 19.03.2013 12:09, schrieb Alexander Graf:
>> 
>> On 19.03.2013, at 12:06, Andreas Färber wrote:
>> 
>>> Am 18.03.2013 12:05, schrieb Alexander Graf:
>>>> 
>>>> On 18.03.2013, at 11:54, Andreas Färber wrote:
>>>> 
>>>>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>>>> 
>>>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>>>> 
>>>>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>>>>> with information about the L1 cache.  We already create two of these
>>>>>>> properties, but with incorrect names - "[id]cache-block-size" instead
>>>>>>> of "[id]-cache-block-size" (note the extra hyphen).
>>>>>>> 
>>>>>>> We were also missing some of the required cache properties.  This
>>>>>>> patch adds the [id]-cache-line-size properties (which have the same
>>>>>>> values as the block size properties in all current cases).  We also
>>>>>>> add the [id]-cache-size properties.
>>>>>>> 
>>>>>>> Adding the cache sizes requires some extra infrastructure in the
>>>>>>> general target-ppc code to (optionally) set the cache sizes for
>>>>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>>>>> leaving other CPU types to people who have a physical example to
>>>>>>> verify against.  In addition, for -cpu host we take the values
>>>>>>> advertised by the host (if available) and use those to override the
>>>>>>> information based on PVR.
>>>>>>> 
>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>> ---
>>>>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>>>>> target-ppc/cpu.h            |    1 +
>>>>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>>>>> target-ppc/translate_init.c |    4 ++++
>>>>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>> index 9a13697..7293082 100644
>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>>>>>>      _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>>>>>> 
>>>>>>>      _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>>>>>>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>>>>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>>>>>>                              env->dcache_line_size)));
>>>>>>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>>>>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>>>>>>> +                                env->dcache_line_size)));
>>>>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>>>>>>> +                                env->icache_line_size)));
>>>>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>>>>>>                              env->icache_line_size)));
>>>>>>> +
>>>>>>> +        if (env->l1_dcache_size) {
>>>>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>>>>> +        } else {
>>>>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>>>>> +        }
>>>>>>> +        if (env->l1_icache_size) {
>>>>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>>>>> +        } else {
>>>>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>>>>> +        }
>>>>>> 
>>>>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>>>> 
>>>>> Generally speaking,
>>>>> 
>>>>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>>>>> for legacy grouping reasons).
>>>>> 
>>>>> PowerPCCPU: If you ever intend to let the user override this value
>>>>> (per-instance) from the command line.
>>>>> 
>>>>> PowerPCCPUClass: If the value is always constant at runtime.
>>>>> 
>>>>> I can't tell from a brief look at this patch which may be the case here.
>>>> 
>>>> Imagine the following:
>>>> 
>>>> PPC
>>>>   `- POWER7
>>>>       |- POWER7_v10
>>>>       `- POWER7_v20
>>>> 
>>>> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.
>>> 
>>> Alex, you asked for an answer here, but I don't spot a question. :-)
>>> 
>>> I'm guessing requirements like these mean that we need to introduce a
>>> new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
>>> macro to put additional code into class_init and instance_init
>>> respectively and let _DEF() and _DEF_SVR() pass nothing there...
>>> Possibly add specific macros wrapping the above to keep the model list
>>> readable.
>>> 
>>> Either way there's a trade-off between keeping easy macros hiding QOM
>>> boilerplate code vs. having high flexibility of what code to put in
>>> there - that's why I resisted your requests to hide too much in macros
>>> at the family level.
>> 
>> Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which would pass in a model specific initialization function in addition to the easy to read defaults? :)
> 
> A function would work as well, yes. But unless you've reworked the list
> you do need the SVR in the base macro.
> For the case at hand you only need code for class_init but I was
> thinking ahead to also prepare the instance_init equivalent while at it. ;)

What does that do? Populate env variables with defaults depending on the class?


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3946 bytes --]

On Mon, Mar 18, 2013 at 11:54:05AM +0100, Andreas Färber wrote:
> Am 15.03.2013 13:27, schrieb Alexander Graf:
> > 
> > On 14.03.2013, at 02:53, David Gibson wrote:
> > 
> >> PAPR requires that the device tree's CPU nodes have several properties
> >> with information about the L1 cache.  We already create two of these
> >> properties, but with incorrect names - "[id]cache-block-size" instead
> >> of "[id]-cache-block-size" (note the extra hyphen).
> >>
> >> We were also missing some of the required cache properties.  This
> >> patch adds the [id]-cache-line-size properties (which have the same
> >> values as the block size properties in all current cases).  We also
> >> add the [id]-cache-size properties.
> >>
> >> Adding the cache sizes requires some extra infrastructure in the
> >> general target-ppc code to (optionally) set the cache sizes for
> >> various CPUs.  The CPU family descriptions in translate_init.c can set
> >> these sizes - this patch adds correct information for POWER7, I'm
> >> leaving other CPU types to people who have a physical example to
> >> verify against.  In addition, for -cpu host we take the values
> >> advertised by the host (if available) and use those to override the
> >> information based on PVR.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
> >> target-ppc/cpu.h            |    1 +
> >> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
> >> target-ppc/translate_init.c |    4 ++++
> >> 4 files changed, 62 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 9a13697..7293082 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
> >>         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
> >>
> >>         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
> >> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
> >> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
> >>                                 env->dcache_line_size)));
> >> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
> >> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
> >> +                                env->dcache_line_size)));
> >> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
> >> +                                env->icache_line_size)));
> >> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
> >>                                 env->icache_line_size)));
> >> +
> >> +        if (env->l1_dcache_size) {
> >> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
> >> +        } else {
> >> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> >> +        }
> >> +        if (env->l1_icache_size) {
> >> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
> >> +        } else {
> >> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> >> +        }
> > 
> > The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
> 
> Generally speaking,
> 
> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
> for legacy grouping reasons).
> 
> PowerPCCPU: If you ever intend to let the user override this value
> (per-instance) from the command line.
> 
> PowerPCCPUClass: If the value is always constant at runtime.
> 
> I can't tell from a brief look at this patch which may be the case here.

Ok, on that rationale I'll rework to move it to the class.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Alexander Graf]

On 19.03.2013, at 02:00, David Gibson wrote:

> On Mon, Mar 18, 2013 at 11:54:05AM +0100, Andreas Färber wrote:
>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>> 
>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>> 
>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>> with information about the L1 cache.  We already create two of these
>>>> properties, but with incorrect names - "[id]cache-block-size" instead
>>>> of "[id]-cache-block-size" (note the extra hyphen).
>>>> 
>>>> We were also missing some of the required cache properties.  This
>>>> patch adds the [id]-cache-line-size properties (which have the same
>>>> values as the block size properties in all current cases).  We also
>>>> add the [id]-cache-size properties.
>>>> 
>>>> Adding the cache sizes requires some extra infrastructure in the
>>>> general target-ppc code to (optionally) set the cache sizes for
>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>> leaving other CPU types to people who have a physical example to
>>>> verify against.  In addition, for -cpu host we take the values
>>>> advertised by the host (if available) and use those to override the
>>>> information based on PVR.
>>>> 
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>> target-ppc/cpu.h            |    1 +
>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>> target-ppc/translate_init.c |    4 ++++
>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 9a13697..7293082 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -333,10 +333,26 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>>>>        _FDT((fdt_property_string(fdt, "device_type", "cpu")));
>>>> 
>>>>        _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
>>>> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>>>>                                env->dcache_line_size)));
>>>> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
>>>> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
>>>> +                                env->dcache_line_size)));
>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
>>>> +                                env->icache_line_size)));
>>>> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>>>>                                env->icache_line_size)));
>>>> +
>>>> +        if (env->l1_dcache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>> +        }
>>>> +        if (env->l1_icache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>> +        }
>>> 
>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>> 
>> Generally speaking,
>> 
>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>> for legacy grouping reasons).
>> 
>> PowerPCCPU: If you ever intend to let the user override this value
>> (per-instance) from the command line.
>> 
>> PowerPCCPUClass: If the value is always constant at runtime.
>> 
>> I can't tell from a brief look at this patch which may be the case here.
> 
> Ok, on that rationale I'll rework to move it to the class.

Thanks :). I think we decided a while ago that subversions should also be individual (sub)classes, so that all of this makes sense again ;).


Alex

[Qemu-devel] [PATCH 4/5] target-ppc: Remove CONFIG_PSERIES dependency in kvm.c

[David Gibson]

target-ppc/kvm.c has an #ifdef on CONFIG_PSERIES, for the handling of
KVM exits due to a PAPR hypercall from the guest.  However, since commit
e4c8b28cde12d01ada8fe869567dc5717a2dfcb7 "ppc: express FDT dependency of
pSeries and e500 boards via default-configs/", this hasn't worked properly.
That patch altered the configuration setup so that although CONFIG_PSERIES
is visible from the Makefiles, it is not visible from C files.  This broke
the pseries machine when KVM is in use.

This patch makes a quick and dirty fix, by removing the CONFIG_PSERIES
dependency, replacing it with TARGET_PPC64 (since removing it entirely
leads to type mismatch errors).  Technically this breaks the build when
configured with --disable-fdt, since that disables CONFIG_PSERIES on
TARGET_PPC64.  However, it turns out the build was already broken in that
case, so this fixes pseries kvm without breaking anything extra.  I'm
looking into how to fix that build breakage, but I don't think that need
delay applying this patch.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/kvm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index eaa11a1..72d5398 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1193,7 +1193,7 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
         dprintf("handle halt\n");
         ret = kvmppc_handle_halt(env);
         break;
-#ifdef CONFIG_PSERIES
+#if defined(TARGET_PPC64)
     case KVM_EXIT_PAPR_HCALL:
         dprintf("handle PAPR hypercall\n");
         run->papr_hcall.ret = spapr_hypercall(cpu,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 4/5] target-ppc: Remove CONFIG_PSERIES dependency in kvm.c

[Alexander Graf]

On 14.03.2013, at 02:53, David Gibson wrote:

> target-ppc/kvm.c has an #ifdef on CONFIG_PSERIES, for the handling of
> KVM exits due to a PAPR hypercall from the guest.  However, since commit
> e4c8b28cde12d01ada8fe869567dc5717a2dfcb7 "ppc: express FDT dependency of
> pSeries and e500 boards via default-configs/", this hasn't worked properly.
> That patch altered the configuration setup so that although CONFIG_PSERIES
> is visible from the Makefiles, it is not visible from C files.  This broke
> the pseries machine when KVM is in use.
> 
> This patch makes a quick and dirty fix, by removing the CONFIG_PSERIES
> dependency, replacing it with TARGET_PPC64 (since removing it entirely
> leads to type mismatch errors).  Technically this breaks the build when
> configured with --disable-fdt, since that disables CONFIG_PSERIES on
> TARGET_PPC64.  However, it turns out the build was already broken in that
> case, so this fixes pseries kvm without breaking anything extra.  I'm
> looking into how to fix that build breakage, but I don't think that need
> delay applying this patch.

The proper way to fix this is to have dtc be a submodule (see patches on the ML) and simply require it for the PPC build.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Thanks, applied to ppc-next.

> ---
> target-ppc/kvm.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index eaa11a1..72d5398 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1193,7 +1193,7 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>         dprintf("handle halt\n");
>         ret = kvmppc_handle_halt(env);
>         break;
> -#ifdef CONFIG_PSERIES
> +#if defined(TARGET_PPC64)
>     case KVM_EXIT_PAPR_HCALL:
>         dprintf("handle PAPR hypercall\n");
>         run->papr_hcall.ret = spapr_hypercall(cpu,
> -- 
> 1.7.10.4
> 

[Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[David Gibson]

Currently, the pseries machine initializes the cpus, then the XICS
interrupt controller.  However, to support the upcoming in-kernel XICS
implementation we will need to initialize the irq controller before the
vcpus.  This patch makes the necesssary rearrangement.  This means the
xics init code can no longer auto-detect the number of cpus ("interrupt
servers" in XICS terminology) and so we must pass that in explicitly from
the platform code.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
Signed-off-by: Ben Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c |   12 +++++++-----
 hw/ppc/xics.c  |   57 +++++++++++++++++++++++++-------------------------------
 hw/xics.h      |    3 ++-
 3 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7293082..b2c9b42 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -791,6 +791,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
         spapr->htab_shift++;
     }
 
+    /* Set up Interrupt Controller before we create the VCPUs */
+    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
+                                  XICS_IRQS);
+    spapr->next_irq = XICS_IRQ_BASE;
+
     /* init CPUs */
     if (cpu_model == NULL) {
         cpu_model = kvm_enabled() ? "host" : "POWER7";
@@ -803,6 +808,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
         }
         env = &cpu->env;
 
+        xics_cpu_setup(spapr->icp, cpu);
+
         /* Set time-base frequency to 512 MHz */
         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
 
@@ -842,11 +849,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
     }
     g_free(filename);
 
-
-    /* Set up Interrupt Controller */
-    spapr->icp = xics_system_init(XICS_IRQS);
-    spapr->next_irq = XICS_IRQ_BASE;
-
     /* Set up EPOW events infrastructure */
     spapr_events_init(spapr);
 
diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
index c3ef12f..374da5b 100644
--- a/hw/ppc/xics.c
+++ b/hw/ppc/xics.c
@@ -521,45 +521,38 @@ static void xics_reset(void *opaque)
     }
 }
 
-struct icp_state *xics_system_init(int nr_irqs)
+void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
 {
-    CPUPPCState *env;
-    CPUState *cpu;
-    int max_server_num;
-    struct icp_state *icp;
-    struct ics_state *ics;
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    struct icp_server_state *ss = &icp->ss[cs->cpu_index];
 
-    max_server_num = -1;
-    for (env = first_cpu; env != NULL; env = env->next_cpu) {
-        cpu = CPU(ppc_env_get_cpu(env));
-        if (cpu->cpu_index > max_server_num) {
-            max_server_num = cpu->cpu_index;
-        }
-    }
+    assert(cs->cpu_index < icp->nr_servers);
 
-    icp = g_malloc0(sizeof(*icp));
-    icp->nr_servers = max_server_num + 1;
-    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
+    switch (PPC_INPUT(env)) {
+    case PPC_FLAGS_INPUT_POWER7:
+        ss->output = env->irq_inputs[POWER7_INPUT_INT];
+        break;
 
-    for (env = first_cpu; env != NULL; env = env->next_cpu) {
-        cpu = CPU(ppc_env_get_cpu(env));
-        struct icp_server_state *ss = &icp->ss[cpu->cpu_index];
+    case PPC_FLAGS_INPUT_970:
+        ss->output = env->irq_inputs[PPC970_INPUT_INT];
+        break;
 
-        switch (PPC_INPUT(env)) {
-        case PPC_FLAGS_INPUT_POWER7:
-            ss->output = env->irq_inputs[POWER7_INPUT_INT];
-            break;
+    default:
+        fprintf(stderr, "XICS interrupt controller does not support this CPU "
+                "bus model\n");
+        abort();
+    }
+}
 
-        case PPC_FLAGS_INPUT_970:
-            ss->output = env->irq_inputs[PPC970_INPUT_INT];
-            break;
+struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
+{
+    struct icp_state *icp;
+    struct ics_state *ics;
 
-        default:
-            hw_error("XICS interrupt model does not support this CPU bus "
-                     "model\n");
-            exit(1);
-        }
-    }
+    icp = g_malloc0(sizeof(*icp));
+    icp->nr_servers = nr_servers;
+    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
 
     ics = g_malloc0(sizeof(*ics));
     ics->nr_irqs = nr_irqs;
diff --git a/hw/xics.h b/hw/xics.h
index c3bf008..6bce042 100644
--- a/hw/xics.h
+++ b/hw/xics.h
@@ -35,6 +35,7 @@ struct icp_state;
 qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
 void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
 
-struct icp_state *xics_system_init(int nr_irqs);
+struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
+void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
 
 #endif /* __XICS_H__ */
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Alexander Graf]

On 14.03.2013, at 02:53, David Gibson wrote:

> Currently, the pseries machine initializes the cpus, then the XICS
> interrupt controller.  However, to support the upcoming in-kernel XICS
> implementation we will need to initialize the irq controller before the
> vcpus.  This patch makes the necesssary rearrangement.  This means the

We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:

  * create vcpus
  * create XICS
  * foreach (vcpu)
      * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)

However, that means we still need to know the maximum number of supported vcpus during the create phase. That number can be bigger than smp_cpus though, since you probably want to support hotplug add of CPUs later on.

Can't we just make the number of supported "interrupt servers" a constant?


Alex

> xics init code can no longer auto-detect the number of cpus ("interrupt
> servers" in XICS terminology) and so we must pass that in explicitly from
> the platform code.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> Signed-off-by: Ben Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c |   12 +++++++-----
> hw/ppc/xics.c  |   57 +++++++++++++++++++++++++-------------------------------
> hw/xics.h      |    3 ++-
> 3 files changed, 34 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7293082..b2c9b42 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -791,6 +791,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>         spapr->htab_shift++;
>     }
> 
> +    /* Set up Interrupt Controller before we create the VCPUs */
> +    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
> +                                  XICS_IRQS);
> +    spapr->next_irq = XICS_IRQ_BASE;
> +
>     /* init CPUs */
>     if (cpu_model == NULL) {
>         cpu_model = kvm_enabled() ? "host" : "POWER7";
> @@ -803,6 +808,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>         }
>         env = &cpu->env;
> 
> +        xics_cpu_setup(spapr->icp, cpu);
> +
>         /* Set time-base frequency to 512 MHz */
>         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> 
> @@ -842,11 +849,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>     }
>     g_free(filename);
> 
> -
> -    /* Set up Interrupt Controller */
> -    spapr->icp = xics_system_init(XICS_IRQS);
> -    spapr->next_irq = XICS_IRQ_BASE;
> -
>     /* Set up EPOW events infrastructure */
>     spapr_events_init(spapr);
> 
> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
> index c3ef12f..374da5b 100644
> --- a/hw/ppc/xics.c
> +++ b/hw/ppc/xics.c
> @@ -521,45 +521,38 @@ static void xics_reset(void *opaque)
>     }
> }
> 
> -struct icp_state *xics_system_init(int nr_irqs)
> +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
> {
> -    CPUPPCState *env;
> -    CPUState *cpu;
> -    int max_server_num;
> -    struct icp_state *icp;
> -    struct ics_state *ics;
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    struct icp_server_state *ss = &icp->ss[cs->cpu_index];
> 
> -    max_server_num = -1;
> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        cpu = CPU(ppc_env_get_cpu(env));
> -        if (cpu->cpu_index > max_server_num) {
> -            max_server_num = cpu->cpu_index;
> -        }
> -    }
> +    assert(cs->cpu_index < icp->nr_servers);
> 
> -    icp = g_malloc0(sizeof(*icp));
> -    icp->nr_servers = max_server_num + 1;
> -    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> +    switch (PPC_INPUT(env)) {
> +    case PPC_FLAGS_INPUT_POWER7:
> +        ss->output = env->irq_inputs[POWER7_INPUT_INT];
> +        break;
> 
> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> -        cpu = CPU(ppc_env_get_cpu(env));
> -        struct icp_server_state *ss = &icp->ss[cpu->cpu_index];
> +    case PPC_FLAGS_INPUT_970:
> +        ss->output = env->irq_inputs[PPC970_INPUT_INT];
> +        break;
> 
> -        switch (PPC_INPUT(env)) {
> -        case PPC_FLAGS_INPUT_POWER7:
> -            ss->output = env->irq_inputs[POWER7_INPUT_INT];
> -            break;
> +    default:
> +        fprintf(stderr, "XICS interrupt controller does not support this CPU "
> +                "bus model\n");
> +        abort();
> +    }
> +}
> 
> -        case PPC_FLAGS_INPUT_970:
> -            ss->output = env->irq_inputs[PPC970_INPUT_INT];
> -            break;
> +struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
> +{
> +    struct icp_state *icp;
> +    struct ics_state *ics;
> 
> -        default:
> -            hw_error("XICS interrupt model does not support this CPU bus "
> -                     "model\n");
> -            exit(1);
> -        }
> -    }
> +    icp = g_malloc0(sizeof(*icp));
> +    icp->nr_servers = nr_servers;
> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> 
>     ics = g_malloc0(sizeof(*ics));
>     ics->nr_irqs = nr_irqs;
> diff --git a/hw/xics.h b/hw/xics.h
> index c3bf008..6bce042 100644
> --- a/hw/xics.h
> +++ b/hw/xics.h
> @@ -35,6 +35,7 @@ struct icp_state;
> qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
> void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
> 
> -struct icp_state *xics_system_init(int nr_irqs);
> +struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
> +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
> 
> #endif /* __XICS_H__ */
> -- 
> 1.7.10.4
> 

Re: [Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Benjamin Herrenschmidt]

On Fri, 2013-03-15 at 13:33 +0100, Alexander Graf wrote:
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
> > Currently, the pseries machine initializes the cpus, then the XICS
> > interrupt controller.  However, to support the upcoming in-kernel XICS
> > implementation we will need to initialize the irq controller before the
> > vcpus.  This patch makes the necesssary rearrangement.  This means the
> 
> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
> 
>   * create vcpus
>   * create XICS
>   * foreach (vcpu)
>       * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)

This is stupid. Why have the VCPU initialize itself for non-kernel
interrupts and *then* switch it over ?

You guys are tiring me of needing about 2 years to iron out a simple
API just to end up with the worst possible crap in the end.

Ben.

> However, that means we still need to know the maximum number of supported vcpus during the create phase. That number can be bigger than smp_cpus though, since you probably want to support hotplug add of CPUs later on.
> 
> Can't we just make the number of supported "interrupt servers" a constant?
> 
> 
> Alex
> 
> > xics init code can no longer auto-detect the number of cpus ("interrupt
> > servers" in XICS terminology) and so we must pass that in explicitly from
> > the platform code.
> > 
> > Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > Signed-off-by: Ben Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c |   12 +++++++-----
> > hw/ppc/xics.c  |   57 +++++++++++++++++++++++++-------------------------------
> > hw/xics.h      |    3 ++-
> > 3 files changed, 34 insertions(+), 38 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 7293082..b2c9b42 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -791,6 +791,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> >         spapr->htab_shift++;
> >     }
> > 
> > +    /* Set up Interrupt Controller before we create the VCPUs */
> > +    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
> > +                                  XICS_IRQS);
> > +    spapr->next_irq = XICS_IRQ_BASE;
> > +
> >     /* init CPUs */
> >     if (cpu_model == NULL) {
> >         cpu_model = kvm_enabled() ? "host" : "POWER7";
> > @@ -803,6 +808,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> >         }
> >         env = &cpu->env;
> > 
> > +        xics_cpu_setup(spapr->icp, cpu);
> > +
> >         /* Set time-base frequency to 512 MHz */
> >         cpu_ppc_tb_init(env, TIMEBASE_FREQ);
> > 
> > @@ -842,11 +849,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
> >     }
> >     g_free(filename);
> > 
> > -
> > -    /* Set up Interrupt Controller */
> > -    spapr->icp = xics_system_init(XICS_IRQS);
> > -    spapr->next_irq = XICS_IRQ_BASE;
> > -
> >     /* Set up EPOW events infrastructure */
> >     spapr_events_init(spapr);
> > 
> > diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
> > index c3ef12f..374da5b 100644
> > --- a/hw/ppc/xics.c
> > +++ b/hw/ppc/xics.c
> > @@ -521,45 +521,38 @@ static void xics_reset(void *opaque)
> >     }
> > }
> > 
> > -struct icp_state *xics_system_init(int nr_irqs)
> > +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
> > {
> > -    CPUPPCState *env;
> > -    CPUState *cpu;
> > -    int max_server_num;
> > -    struct icp_state *icp;
> > -    struct ics_state *ics;
> > +    CPUState *cs = CPU(cpu);
> > +    CPUPPCState *env = &cpu->env;
> > +    struct icp_server_state *ss = &icp->ss[cs->cpu_index];
> > 
> > -    max_server_num = -1;
> > -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> > -        cpu = CPU(ppc_env_get_cpu(env));
> > -        if (cpu->cpu_index > max_server_num) {
> > -            max_server_num = cpu->cpu_index;
> > -        }
> > -    }
> > +    assert(cs->cpu_index < icp->nr_servers);
> > 
> > -    icp = g_malloc0(sizeof(*icp));
> > -    icp->nr_servers = max_server_num + 1;
> > -    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> > +    switch (PPC_INPUT(env)) {
> > +    case PPC_FLAGS_INPUT_POWER7:
> > +        ss->output = env->irq_inputs[POWER7_INPUT_INT];
> > +        break;
> > 
> > -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> > -        cpu = CPU(ppc_env_get_cpu(env));
> > -        struct icp_server_state *ss = &icp->ss[cpu->cpu_index];
> > +    case PPC_FLAGS_INPUT_970:
> > +        ss->output = env->irq_inputs[PPC970_INPUT_INT];
> > +        break;
> > 
> > -        switch (PPC_INPUT(env)) {
> > -        case PPC_FLAGS_INPUT_POWER7:
> > -            ss->output = env->irq_inputs[POWER7_INPUT_INT];
> > -            break;
> > +    default:
> > +        fprintf(stderr, "XICS interrupt controller does not support this CPU "
> > +                "bus model\n");
> > +        abort();
> > +    }
> > +}
> > 
> > -        case PPC_FLAGS_INPUT_970:
> > -            ss->output = env->irq_inputs[PPC970_INPUT_INT];
> > -            break;
> > +struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
> > +{
> > +    struct icp_state *icp;
> > +    struct ics_state *ics;
> > 
> > -        default:
> > -            hw_error("XICS interrupt model does not support this CPU bus "
> > -                     "model\n");
> > -            exit(1);
> > -        }
> > -    }
> > +    icp = g_malloc0(sizeof(*icp));
> > +    icp->nr_servers = nr_servers;
> > +    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
> > 
> >     ics = g_malloc0(sizeof(*ics));
> >     ics->nr_irqs = nr_irqs;
> > diff --git a/hw/xics.h b/hw/xics.h
> > index c3bf008..6bce042 100644
> > --- a/hw/xics.h
> > +++ b/hw/xics.h
> > @@ -35,6 +35,7 @@ struct icp_state;
> > qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
> > void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
> > 
> > -struct icp_state *xics_system_init(int nr_irqs);
> > +struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
> > +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
> > 
> > #endif /* __XICS_H__ */
> > -- 
> > 1.7.10.4
> > 

Re: [Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Alexander Graf]

Am 16.03.2013 um 04:14 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:

> On Fri, 2013-03-15 at 13:33 +0100, Alexander Graf wrote:
>> On 14.03.2013, at 02:53, David Gibson wrote:
>> 
>>> Currently, the pseries machine initializes the cpus, then the XICS
>>> interrupt controller.  However, to support the upcoming in-kernel XICS
>>> implementation we will need to initialize the irq controller before the
>>> vcpus.  This patch makes the necesssary rearrangement.  This means the
>> 
>> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
>> 
>>  * create vcpus
>>  * create XICS
>>  * foreach (vcpu)
>>      * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
> 
> This is stupid. Why have the VCPU initialize itself for non-kernel
> interrupts and *then* switch it over ?

Because non-kernel initialization is a nop.

Alex

> You guys are tiring me of needing about 2 years to iron out a simple
> API just to end up with the worst possible crap in the end.
> 
> Ben.
> 
>> However, that means we still need to know the maximum number of supported vcpus during the create phase. That number can be bigger than smp_cpus though, since you probably want to support hotplug add of CPUs later on.
>> 
>> Can't we just make the number of supported "interrupt servers" a constant?
>> 
>> 
>> Alex
>> 
>>> xics init code can no longer auto-detect the number of cpus ("interrupt
>>> servers" in XICS terminology) and so we must pass that in explicitly from
>>> the platform code.
>>> 
>>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
>>> Signed-off-by: Ben Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> hw/ppc/spapr.c |   12 +++++++-----
>>> hw/ppc/xics.c  |   57 +++++++++++++++++++++++++-------------------------------
>>> hw/xics.h      |    3 ++-
>>> 3 files changed, 34 insertions(+), 38 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 7293082..b2c9b42 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -791,6 +791,11 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>>        spapr->htab_shift++;
>>>    }
>>> 
>>> +    /* Set up Interrupt Controller before we create the VCPUs */
>>> +    spapr->icp = xics_system_init(smp_cpus * kvmppc_smt_threads() / smp_threads,
>>> +                                  XICS_IRQS);
>>> +    spapr->next_irq = XICS_IRQ_BASE;
>>> +
>>>    /* init CPUs */
>>>    if (cpu_model == NULL) {
>>>        cpu_model = kvm_enabled() ? "host" : "POWER7";
>>> @@ -803,6 +808,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>>        }
>>>        env = &cpu->env;
>>> 
>>> +        xics_cpu_setup(spapr->icp, cpu);
>>> +
>>>        /* Set time-base frequency to 512 MHz */
>>>        cpu_ppc_tb_init(env, TIMEBASE_FREQ);
>>> 
>>> @@ -842,11 +849,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>>>    }
>>>    g_free(filename);
>>> 
>>> -
>>> -    /* Set up Interrupt Controller */
>>> -    spapr->icp = xics_system_init(XICS_IRQS);
>>> -    spapr->next_irq = XICS_IRQ_BASE;
>>> -
>>>    /* Set up EPOW events infrastructure */
>>>    spapr_events_init(spapr);
>>> 
>>> diff --git a/hw/ppc/xics.c b/hw/ppc/xics.c
>>> index c3ef12f..374da5b 100644
>>> --- a/hw/ppc/xics.c
>>> +++ b/hw/ppc/xics.c
>>> @@ -521,45 +521,38 @@ static void xics_reset(void *opaque)
>>>    }
>>> }
>>> 
>>> -struct icp_state *xics_system_init(int nr_irqs)
>>> +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu)
>>> {
>>> -    CPUPPCState *env;
>>> -    CPUState *cpu;
>>> -    int max_server_num;
>>> -    struct icp_state *icp;
>>> -    struct ics_state *ics;
>>> +    CPUState *cs = CPU(cpu);
>>> +    CPUPPCState *env = &cpu->env;
>>> +    struct icp_server_state *ss = &icp->ss[cs->cpu_index];
>>> 
>>> -    max_server_num = -1;
>>> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>> -        cpu = CPU(ppc_env_get_cpu(env));
>>> -        if (cpu->cpu_index > max_server_num) {
>>> -            max_server_num = cpu->cpu_index;
>>> -        }
>>> -    }
>>> +    assert(cs->cpu_index < icp->nr_servers);
>>> 
>>> -    icp = g_malloc0(sizeof(*icp));
>>> -    icp->nr_servers = max_server_num + 1;
>>> -    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
>>> +    switch (PPC_INPUT(env)) {
>>> +    case PPC_FLAGS_INPUT_POWER7:
>>> +        ss->output = env->irq_inputs[POWER7_INPUT_INT];
>>> +        break;
>>> 
>>> -    for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>> -        cpu = CPU(ppc_env_get_cpu(env));
>>> -        struct icp_server_state *ss = &icp->ss[cpu->cpu_index];
>>> +    case PPC_FLAGS_INPUT_970:
>>> +        ss->output = env->irq_inputs[PPC970_INPUT_INT];
>>> +        break;
>>> 
>>> -        switch (PPC_INPUT(env)) {
>>> -        case PPC_FLAGS_INPUT_POWER7:
>>> -            ss->output = env->irq_inputs[POWER7_INPUT_INT];
>>> -            break;
>>> +    default:
>>> +        fprintf(stderr, "XICS interrupt controller does not support this CPU "
>>> +                "bus model\n");
>>> +        abort();
>>> +    }
>>> +}
>>> 
>>> -        case PPC_FLAGS_INPUT_970:
>>> -            ss->output = env->irq_inputs[PPC970_INPUT_INT];
>>> -            break;
>>> +struct icp_state *xics_system_init(int nr_servers, int nr_irqs)
>>> +{
>>> +    struct icp_state *icp;
>>> +    struct ics_state *ics;
>>> 
>>> -        default:
>>> -            hw_error("XICS interrupt model does not support this CPU bus "
>>> -                     "model\n");
>>> -            exit(1);
>>> -        }
>>> -    }
>>> +    icp = g_malloc0(sizeof(*icp));
>>> +    icp->nr_servers = nr_servers;
>>> +    icp->ss = g_malloc0(icp->nr_servers*sizeof(struct icp_server_state));
>>> 
>>>    ics = g_malloc0(sizeof(*ics));
>>>    ics->nr_irqs = nr_irqs;
>>> diff --git a/hw/xics.h b/hw/xics.h
>>> index c3bf008..6bce042 100644
>>> --- a/hw/xics.h
>>> +++ b/hw/xics.h
>>> @@ -35,6 +35,7 @@ struct icp_state;
>>> qemu_irq xics_get_qirq(struct icp_state *icp, int irq);
>>> void xics_set_irq_type(struct icp_state *icp, int irq, bool lsi);
>>> 
>>> -struct icp_state *xics_system_init(int nr_irqs);
>>> +struct icp_state *xics_system_init(int nr_servers, int nr_irqs);
>>> +void xics_cpu_setup(struct icp_state *icp, PowerPCCPU *cpu);
>>> 
>>> #endif /* __XICS_H__ */
>>> -- 
>>> 1.7.10.4
> 
> 

Re: [Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Benjamin Herrenschmidt]

On Sat, 2013-03-16 at 06:33 +0100, Alexander Graf wrote:
> >> We're changing that notion in the in-kernel XICS discussions.  The
> flow will look like this:
> >> 
> >>  * create vcpus
> >>  * create XICS
> >>  * foreach (vcpu)
> >>      * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
> > 
> > This is stupid. Why have the VCPU initialize itself for non-kernel
> > interrupts and *then* switch it over ?
> 
> Because non-kernel initialization is a nop.

And ?

Ben.

Re: [Qemu-devel] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Alexander Graf]

Am 16.03.2013 um 06:41 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:

> On Sat, 2013-03-16 at 06:33 +0100, Alexander Graf wrote:
>>>> We're changing that notion in the in-kernel XICS discussions.  The
>> flow will look like this:
>>>> 
>>>> * create vcpus
>>>> * create XICS
>>>> * foreach (vcpu)
>>>>     * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
>>> 
>>> This is stupid. Why have the VCPU initialize itself for non-kernel
>>> interrupts and *then* switch it over ?
>> 
>> Because non-kernel initialization is a nop.
> 
> And ?

Compared to the previous aproach of a vm wide irq type, we win:

  - less vm global state
  - possibility of defining the 'cpu nr' from the pic's point of view
  - the same mechanism and code paths for hotplug and non-hotplug
  - possibility of supporting multiple pics
  - less implicit assumptions

While the code changes required to make this work should be minimal over the "set irq architecture" approach. It really makes life easier for everyone.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

On Fri, Mar 15, 2013 at 01:33:18PM +0100, Alexander Graf wrote:
> 
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
> > Currently, the pseries machine initializes the cpus, then the XICS
> > interrupt controller.  However, to support the upcoming in-kernel XICS
> > implementation we will need to initialize the irq controller before the
> > vcpus.  This patch makes the necesssary rearrangement.  This means the
> 
> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
> 
>   * create vcpus
>   * create XICS
>   * foreach (vcpu)
>       * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
> 
> However, that means we still need to know the maximum number of
> supported vcpus during the create phase. That number can be bigger
> than smp_cpus though, since you probably want to support hotplug add
> of CPUs later on.
> 
> Can't we just make the number of supported "interrupt servers" a
> constant?

I suppose, but we need an allocation for each one, so its a bit ugly.
In any case although the comment is a bit out of date, this patch also
creates a logical place to put per-cpu XICS initialization which we
will still need for the new interface.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Alexander Graf]

On 18.03.2013, at 03:55, David Gibson wrote:

> On Fri, Mar 15, 2013 at 01:33:18PM +0100, Alexander Graf wrote:
>> 
>> On 14.03.2013, at 02:53, David Gibson wrote:
>> 
>>> Currently, the pseries machine initializes the cpus, then the XICS
>>> interrupt controller.  However, to support the upcoming in-kernel XICS
>>> implementation we will need to initialize the irq controller before the
>>> vcpus.  This patch makes the necesssary rearrangement.  This means the
>> 
>> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
>> 
>>  * create vcpus
>>  * create XICS
>>  * foreach (vcpu)
>>      * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
>> 
>> However, that means we still need to know the maximum number of
>> supported vcpus during the create phase. That number can be bigger
>> than smp_cpus though, since you probably want to support hotplug add
>> of CPUs later on.
>> 
>> Can't we just make the number of supported "interrupt servers" a
>> constant?
> 
> I suppose, but we need an allocation for each one, so its a bit ugly.
> In any case although the comment is a bit out of date, this patch also
> creates a logical place to put per-cpu XICS initialization which we
> will still need for the new interface.

So how would you model CPU hotplug add?


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Mon, Mar 18, 2013 at 04:12:11AM +0100, Alexander Graf wrote:
> 
> On 18.03.2013, at 03:55, David Gibson wrote:
> 
> > On Fri, Mar 15, 2013 at 01:33:18PM +0100, Alexander Graf wrote:
> >> 
> >> On 14.03.2013, at 02:53, David Gibson wrote:
> >> 
> >>> Currently, the pseries machine initializes the cpus, then the XICS
> >>> interrupt controller.  However, to support the upcoming in-kernel XICS
> >>> implementation we will need to initialize the irq controller before the
> >>> vcpus.  This patch makes the necesssary rearrangement.  This means the
> >> 
> >> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
> >> 
> >>  * create vcpus
> >>  * create XICS
> >>  * foreach (vcpu)
> >>      * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
> >> 
> >> However, that means we still need to know the maximum number of
> >> supported vcpus during the create phase. That number can be bigger
> >> than smp_cpus though, since you probably want to support hotplug add
> >> of CPUs later on.
> >> 
> >> Can't we just make the number of supported "interrupt servers" a
> >> constant?
> > 
> > I suppose, but we need an allocation for each one, so its a bit ugly.
> > In any case although the comment is a bit out of date, this patch also
> > creates a logical place to put per-cpu XICS initialization which we
> > will still need for the new interface.
> 
> So how would you model CPU hotplug add?

Add headroom to the XICS setup based on whatever information we have
about maximum pluggable CPUs.  To use the PAPR hotplug interfaces we
already need a notion of max # CPUs, we can't have that just be open
ended.  We'd also add a call to xics_cpu_setup() to the hotplug add
path, obviously.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 5/5] pseries: Move XICS initialization before cpu initialization

[Alexander Graf]

On 18.03.2013, at 04:38, David Gibson wrote:

> On Mon, Mar 18, 2013 at 04:12:11AM +0100, Alexander Graf wrote:
>> 
>> On 18.03.2013, at 03:55, David Gibson wrote:
>> 
>>> On Fri, Mar 15, 2013 at 01:33:18PM +0100, Alexander Graf wrote:
>>>> 
>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>> 
>>>>> Currently, the pseries machine initializes the cpus, then the XICS
>>>>> interrupt controller.  However, to support the upcoming in-kernel XICS
>>>>> implementation we will need to initialize the irq controller before the
>>>>> vcpus.  This patch makes the necesssary rearrangement.  This means the
>>>> 
>>>> We're changing that notion in the in-kernel XICS discussions.  The flow will look like this:
>>>> 
>>>> * create vcpus
>>>> * create XICS
>>>> * foreach (vcpu)
>>>>     * enable_cap(vcpu, CAP_XICS_SERVER, xics_handle)
>>>> 
>>>> However, that means we still need to know the maximum number of
>>>> supported vcpus during the create phase. That number can be bigger
>>>> than smp_cpus though, since you probably want to support hotplug add
>>>> of CPUs later on.
>>>> 
>>>> Can't we just make the number of supported "interrupt servers" a
>>>> constant?
>>> 
>>> I suppose, but we need an allocation for each one, so its a bit ugly.
>>> In any case although the comment is a bit out of date, this patch also
>>> creates a logical place to put per-cpu XICS initialization which we
>>> will still need for the new interface.
>> 
>> So how would you model CPU hotplug add?
> 
> Add headroom to the XICS setup based on whatever information we have
> about maximum pluggable CPUs.  To use the PAPR hotplug interfaces we
> already need a notion of max # CPUs, we can't have that just be open
> ended.  We'd also add a call to xics_cpu_setup() to the hotplug add
> path, obviously.

Yeah. So for that you also need the different code order. Sorry for the confusion :).

Thanks, applied to ppc-next.


Alex