Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
On Wed, Jun 13, 2018 at 12:11:12PM +0200, Greg Kurz wrote:
> On Wed, 13 Jun 2018 16:57:06 +1000
> David Gibson wrote:
>
> > PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> > controller. Or more precisely to the "presentation" component of the
> > interrupt controller relevant to this cpu.
> >
> > Really, this field is machine specific. The machines which use it can
> > point it to different types of object depending on their needs, and most
> > machines don't use it at all (since they have older style PICs which don't
> > have per-cpu presentation components).
> >
> > There's also other information that's per-cpu, but platform/machine
> > specific. So replace the intc pointer with a (void *)machine_data which
> > can be managed as the machine type likes to conveniently store per cpu
> > information.
> >
> > Signed-off-by: David Gibson
> > ---
>
> This looks good, just one question below...
>
> > hw/intc/xics.c | 5 +++--
> > hw/intc/xics_spapr.c| 16 +++-
> > hw/ppc/pnv.c| 4 ++--
> > hw/ppc/pnv_core.c | 11 +--
> > hw/ppc/spapr.c | 8
> > hw/ppc/spapr_cpu_core.c | 13 ++---
> > include/hw/ppc/pnv_core.h | 9 +
> > include/hw/ppc/spapr_cpu_core.h | 10 ++
> > include/hw/ppc/xics.h | 4 ++--
> > target/ppc/cpu.h| 2 +-
> > 10 files changed, 61 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index e73e623e3b..689ad44e5f 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> > .class_size = sizeof(ICPStateClass),
> > };
> >
> > -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error
> > **errp)
> > +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > + Error **errp)
> > {
> > Error *local_err = NULL;
> > Object *obj;
> > @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type,
> > XICSFabric *xi, Error **errp)
> > obj = NULL;
> > }
> >
> > -return obj;
> > +return ICP(obj);
> > }
> >
> > /*
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 2e27b92b87..01c76717cf 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -31,6 +31,7 @@
> > #include "trace.h"
> > #include "qemu/timer.h"
> > #include "hw/ppc/spapr.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> > #include "hw/ppc/xics.h"
> > #include "hw/ppc/fdt.h"
> > #include "qapi/visitor.h"
> > @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > target_ulong cppr = args[0];
> > +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> >
> > -icp_set_cppr(ICP(cpu->intc), cppr);
> > +icp_set_cppr(spapr_cpu->icp, cppr);
> > return H_SUCCESS;
> > }
> >
> > @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> > static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > -uint32_t xirr = icp_accept(ICP(cpu->intc));
> > +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +uint32_t xirr = icp_accept(spapr_cpu->icp);
> >
> > args[0] = xirr;
> > return H_SUCCESS;
> > @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> > static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > -uint32_t xirr = icp_accept(ICP(cpu->intc));
> > +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +uint32_t xirr = icp_accept(spapr_cpu->icp);
> >
> > args[0] = xirr;
> > args[1] = cpu_get_host_ticks();
> > @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> > static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >target_ulong opcode, target_ulong *args)
> > {
> > +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > target_ulong xirr = args[0];
> >
> > -icp_eoi(ICP(cpu->intc), xirr);
> > +icp_eoi(spapr_cpu->icp, xirr);
> > return H_SUCCESS;
> > }
> >
> > @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> > target_ulong opcode, target_ulong *args)
> > {
> > uint32_t mfrr;
> > -uint32_t xirr = icp_ipoll(ICP(cpu->intc), );
> > +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +uint32_t xirr = icp_ipoll(spapr_cpu->icp, );
> >
> > args[0] = xirr;
> > args[1] = mfrr;
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index
Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
On Wed, 13 Jun 2018 16:57:06 +1000
David Gibson wrote:
> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller. Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.
>
> Really, this field is machine specific. The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
>
> There's also other information that's per-cpu, but platform/machine
> specific. So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.
>
> Signed-off-by: David Gibson
> ---
This looks good, just one question below...
> hw/intc/xics.c | 5 +++--
> hw/intc/xics_spapr.c| 16 +++-
> hw/ppc/pnv.c| 4 ++--
> hw/ppc/pnv_core.c | 11 +--
> hw/ppc/spapr.c | 8
> hw/ppc/spapr_cpu_core.c | 13 ++---
> include/hw/ppc/pnv_core.h | 9 +
> include/hw/ppc/spapr_cpu_core.h | 10 ++
> include/hw/ppc/xics.h | 4 ++--
> target/ppc/cpu.h| 2 +-
> 10 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> .class_size = sizeof(ICPStateClass),
> };
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error
> **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type,
> XICSFabric *xi, Error **errp)
> obj = NULL;
> }
>
> -return obj;
> +return ICP(obj);
> }
>
> /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
> #include "trace.h"
> #include "qemu/timer.h"
> #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> #include "hw/ppc/xics.h"
> #include "hw/ppc/fdt.h"
> #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> target_ulong cppr = args[0];
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>
> -icp_set_cppr(ICP(cpu->intc), cppr);
> +icp_set_cppr(spapr_cpu->icp, cppr);
> return H_SUCCESS;
> }
>
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> -uint32_t xirr = icp_accept(ICP(cpu->intc));
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> -uint32_t xirr = icp_accept(ICP(cpu->intc));
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>target_ulong opcode, target_ulong *args)
> {
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> target_ulong xirr = args[0];
>
> -icp_eoi(ICP(cpu->intc), xirr);
> +icp_eoi(spapr_cpu->icp, xirr);
> return H_SUCCESS;
> }
>
> @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> uint32_t mfrr;
> -uint32_t xirr = icp_ipoll(ICP(cpu->intc), );
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +uint32_t xirr = icp_ipoll(spapr_cpu->icp, );
>
> args[0] = xirr;
> args[1] = mfrr;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0b9508d94d..3a36c6ac6a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> {
> PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>
> -return cpu ? ICP(cpu->intc) : NULL;
> +return cpu ? pnv_cpu_state(cpu)->icp : NULL;
> }
>
> static void
Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
On Wed, Jun 13, 2018 at 10:46:02AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 08:57 AM, David Gibson wrote:
> > PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> > controller. Or more precisely to the "presentation" component of the
> > interrupt controller relevant to this cpu.
>
> yes and that made sense in terms of modeling because you actually have a
> set of wires between the presenter and the cores of a system.
>
> > Really, this field is machine specific. The machines which use it can
> > point it to different types of object depending on their needs, and most
> > machines don't use it at all (since they have older style PICs which don't
> > have per-cpu presentation components).
> >
> > There's also other information that's per-cpu, but platform/machine
> > specific. So replace the intc pointer with a (void *)machine_data which
> > can be managed as the machine type likes to conveniently store per cpu
> > information.
>
> ah. so you have something else the store in the machine_data.
>
> If you were defining a type, we would have some more checks when
> casting the machine_data field. We also could parent the object
> to the CPU also. This is minor.
My intention is that machine_data be a "passive" structure, not a QOM
object. Lifetime and type management are all up to the machine.
> The change should be compatible with the XIVE change which need
> to allocate a different type of presenter. So, sPAPRCPUState and
> PnvCPUState would look like :
>
> typedef struct sPAPRCPUState {
> ICPState *icp;
> XiveTCTX *tctx;
> } sPAPRCPUState;
Exactly.
> and the call to ipc_create() will move in an operation of the
> sPAPR IRQ backend, if that exists oneday, and in an operation of
> the PnvChip to handle the differences in the interrupt controller
> in use by the machine.
>
> So no big difference, but the cpu machine_data won't be populated
> from the core but from the machine. I hope this is compatible
> with the next changes.
intc was already populated from the machine.
--
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
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer
On 06/13/2018 08:57 AM, David Gibson wrote:
> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller. Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.
yes and that made sense in terms of modeling because you actually have a
set of wires between the presenter and the cores of a system.
> Really, this field is machine specific. The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
>
> There's also other information that's per-cpu, but platform/machine
> specific. So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.
ah. so you have something else the store in the machine_data.
If you were defining a type, we would have some more checks when
casting the machine_data field. We also could parent the object
to the CPU also. This is minor.
The change should be compatible with the XIVE change which need
to allocate a different type of presenter. So, sPAPRCPUState and
PnvCPUState would look like :
typedef struct sPAPRCPUState {
ICPState *icp;
XiveTCTX *tctx;
} sPAPRCPUState;
and the call to ipc_create() will move in an operation of the
sPAPR IRQ backend, if that exists oneday, and in an operation of
the PnvChip to handle the differences in the interrupt controller
in use by the machine.
So no big difference, but the cpu machine_data won't be populated
from the core but from the machine. I hope this is compatible
with the next changes.
Thanks,
C.
>
> Signed-off-by: David Gibson
> ---
> hw/intc/xics.c | 5 +++--
> hw/intc/xics_spapr.c| 16 +++-
> hw/ppc/pnv.c| 4 ++--
> hw/ppc/pnv_core.c | 11 +--
> hw/ppc/spapr.c | 8
> hw/ppc/spapr_cpu_core.c | 13 ++---
> include/hw/ppc/pnv_core.h | 9 +
> include/hw/ppc/spapr_cpu_core.h | 10 ++
> include/hw/ppc/xics.h | 4 ++--
> target/ppc/cpu.h| 2 +-
> 10 files changed, 61 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> .class_size = sizeof(ICPStateClass),
> };
>
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error
> **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> + Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type,
> XICSFabric *xi, Error **errp)
> obj = NULL;
> }
>
> -return obj;
> +return ICP(obj);
> }
>
> /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
> #include "trace.h"
> #include "qemu/timer.h"
> #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
> #include "hw/ppc/xics.h"
> #include "hw/ppc/fdt.h"
> #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> target_ulong cppr = args[0];
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>
> -icp_set_cppr(ICP(cpu->intc), cppr);
> +icp_set_cppr(spapr_cpu->icp, cppr);
> return H_SUCCESS;
> }
>
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> -uint32_t xirr = icp_accept(ICP(cpu->intc));
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> -uint32_t xirr = icp_accept(ICP(cpu->intc));
> +sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +uint32_t xirr = icp_accept(spapr_cpu->icp);
>
> args[0] = xirr;
> args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>target_ulong opcode, target_ulong *args)
> {
> +
