* [Qemu-devel] [PATCH 1/4] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN
2013-07-17 6:37 [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
@ 2013-07-17 6:37 ` Alexey Kardashevskiy
2013-07-17 6:37 ` [Qemu-devel] [PATCH 2/4] xics: add pre_save/post_load/cpu_setup dispatchers Alexey Kardashevskiy
` (3 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-07-17 6:37 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, aik, Alexander Graf, qemu-ppc, Paul Mackerras,
David Gibson
From: David Gibson <david@gibson.dropbear.id.au>
Recent PowerKVM allows the kernel to intercept some RTAS calls from the
guest directly. This is used to implement the more efficient in-kernel
XICS for example. qemu is still responsible for assigning the RTAS token
numbers however, and needs to tell the kernel which RTAS function name is
assigned to a given token value. This patch adds a convenience wrapper for
the KVM_PPC_RTAS_DEFINE_TOKEN ioctl() which is used for this purpose.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
target-ppc/kvm.c | 14 ++++++++++++++
target-ppc/kvm_ppc.h | 7 +++++++
2 files changed, 21 insertions(+)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 27e2aaf..606bdb9 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1789,6 +1789,20 @@ static int kvm_ppc_register_host_cpu_type(void)
return 0;
}
+int kvmppc_define_rtas_token(uint32_t token, const char *function)
+{
+ struct kvm_rtas_token_args args = {
+ .token = token,
+ };
+
+ if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_RTAS)) {
+ return -ENOENT;
+ }
+
+ strncpy(args.name, function, sizeof(args.name));
+
+ return kvm_vm_ioctl(kvm_state, KVM_PPC_RTAS_DEFINE_TOKEN, &args);
+}
int kvmppc_get_htab_fd(bool write)
{
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 4ae7bf2..12564ef 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -38,6 +38,7 @@ uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
#endif /* !CONFIG_USER_ONLY */
int kvmppc_fixup_cpu(PowerPCCPU *cpu);
bool kvmppc_has_cap_epr(void);
+int kvmppc_define_rtas_token(uint32_t token, const char *function);
int kvmppc_get_htab_fd(bool write);
int kvmppc_save_htab(QEMUFile *f, int fd, size_t bufsize, int64_t max_ns);
int kvmppc_load_htab_chunk(QEMUFile *f, int fd, uint32_t index,
@@ -164,6 +165,12 @@ static inline bool kvmppc_has_cap_epr(void)
return false;
}
+static inline int kvmppc_define_rtas_token(uint32_t token,
+ const char *function)
+{
+ return -1;
+}
+
static inline int kvmppc_get_htab_fd(bool write)
{
return -1;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 2/4] xics: add pre_save/post_load/cpu_setup dispatchers
2013-07-17 6:37 [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
2013-07-17 6:37 ` [Qemu-devel] [PATCH 1/4] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN Alexey Kardashevskiy
@ 2013-07-17 6:37 ` Alexey Kardashevskiy
2013-07-17 6:37 ` [Qemu-devel] [PATCH 3/4] xics: rework initialization Alexey Kardashevskiy
` (2 subsequent siblings)
4 siblings, 0 replies; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-07-17 6:37 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Alexey Kardashevskiy, Alexander Graf, qemu-ppc,
Paul Mackerras, David Gibson
The upcoming support of in-kernel XICS will redefine migration callbacks
for both ICS and ICP so classes and callback pointers are added.
This adds a cpu_setup callback to the XICS device class (as XICS-KVM
will do it different) and xics_dispatch_cpu_setup(). This also moves
the place where xics_dispatch_cpu_setup() is called a bit further to
have VCPU initialized (required for XICS-KVM).
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
hw/intc/xics.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
hw/ppc/spapr.c | 4 ++--
include/hw/ppc/xics.h | 46 +++++++++++++++++++++++++++++++++++++-
3 files changed, 104 insertions(+), 7 deletions(-)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 6b3c071..283c2dd 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -153,11 +153,35 @@ static void icp_irq(XICSState *icp, int server, int nr, uint8_t priority)
}
}
+static void icp_dispatch_pre_save(void *opaque)
+{
+ ICPState *ss = opaque;
+ ICPStateClass *info = ICP_GET_CLASS(ss);
+
+ if (info->pre_save) {
+ info->pre_save(ss);
+ }
+}
+
+static int icp_dispatch_post_load(void *opaque, int version_id)
+{
+ ICPState *ss = opaque;
+ ICPStateClass *info = ICP_GET_CLASS(ss);
+
+ if (info->post_load) {
+ info->post_load(ss);
+ }
+
+ return 0;
+}
+
static const VMStateDescription vmstate_icp_server = {
.name = "icp/server",
.version_id = 1,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
+ .pre_save = icp_dispatch_pre_save,
+ .post_load = icp_dispatch_post_load,
.fields = (VMStateField []) {
/* Sanity check */
VMSTATE_UINT32(xirr, ICPState),
@@ -192,6 +216,7 @@ static TypeInfo icp_info = {
.parent = TYPE_DEVICE,
.instance_size = sizeof(ICPState),
.class_init = icp_class_init,
+ .class_size = sizeof(ICPStateClass),
};
/*
@@ -353,10 +378,9 @@ static void ics_reset(DeviceState *dev)
}
}
-static int ics_post_load(void *opaque, int version_id)
+static int ics_post_load(ICSState *ics)
{
int i;
- ICSState *ics = opaque;
for (i = 0; i < ics->icp->nr_servers; i++) {
icp_resend(ics->icp, i);
@@ -365,6 +389,28 @@ static int ics_post_load(void *opaque, int version_id)
return 0;
}
+static void ics_dispatch_pre_save(void *opaque)
+{
+ ICSState *ics = opaque;
+ ICSStateClass *info = ICS_GET_CLASS(ics);
+
+ if (info->pre_save) {
+ info->pre_save(ics);
+ }
+}
+
+static int ics_dispatch_post_load(void *opaque, int version_id)
+{
+ ICSState *ics = opaque;
+ ICSStateClass *info = ICS_GET_CLASS(ics);
+
+ if (info->post_load) {
+ info->post_load(ics);
+ }
+
+ return 0;
+}
+
static const VMStateDescription vmstate_ics_irq = {
.name = "ics/irq",
.version_id = 1,
@@ -384,7 +430,8 @@ static const VMStateDescription vmstate_ics = {
.version_id = 1,
.minimum_version_id = 1,
.minimum_version_id_old = 1,
- .post_load = ics_post_load,
+ .pre_save = ics_dispatch_pre_save,
+ .post_load = ics_dispatch_post_load,
.fields = (VMStateField []) {
/* Sanity check */
VMSTATE_UINT32_EQUAL(nr_irqs, ICSState),
@@ -409,10 +456,12 @@ static int ics_realize(DeviceState *dev)
static void ics_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
+ ICSStateClass *k = ICS_CLASS(klass);
dc->init = ics_realize;
dc->vmsd = &vmstate_ics;
dc->reset = ics_reset;
+ k->post_load = ics_post_load;
}
static TypeInfo ics_info = {
@@ -420,6 +469,7 @@ static TypeInfo ics_info = {
.parent = TYPE_DEVICE,
.instance_size = sizeof(ICSState),
.class_init = ics_class_init,
+ .class_size = sizeof(ICSStateClass),
};
/*
@@ -612,7 +662,7 @@ static void xics_reset(DeviceState *d)
device_reset(DEVICE(icp->ics));
}
-void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
{
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
@@ -674,10 +724,12 @@ static Property xics_properties[] = {
static void xics_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
+ XICSStateClass *k = XICS_CLASS(oc);
dc->realize = xics_realize;
dc->props = xics_properties;
dc->reset = xics_reset;
+ k->cpu_setup = xics_cpu_setup;
spapr_rtas_register("ibm,set-xive", rtas_set_xive);
spapr_rtas_register("ibm,get-xive", rtas_get_xive);
@@ -694,6 +746,7 @@ static const TypeInfo xics_info = {
.name = TYPE_XICS,
.parent = TYPE_SYS_BUS_DEVICE,
.instance_size = sizeof(XICSState),
+ .class_size = sizeof(XICSStateClass),
.class_init = xics_class_init,
.instance_init = xics_initfn,
};
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 16bfab9..432f0d2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1155,8 +1155,6 @@ 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);
@@ -1170,6 +1168,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
kvmppc_set_papr(cpu);
}
+ xics_dispatch_cpu_setup(spapr->icp, cpu);
+
qemu_register_reset(spapr_cpu_reset, cpu);
}
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 66364c5..90ecaf8 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -32,6 +32,11 @@
#define TYPE_XICS "xics"
#define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
+#define XICS_CLASS(klass) \
+ OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
+#define XICS_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(XICSStateClass, (obj), TYPE_XICS)
+
#define XICS_IPI 0x2
#define XICS_BUID 0x1
#define XICS_IRQ_BASE (XICS_BUID << 12)
@@ -41,11 +46,20 @@
* (the kernel implementation supports more but we don't exploit
* that yet)
*/
+typedef struct XICSStateClass XICSStateClass;
typedef struct XICSState XICSState;
+typedef struct ICPStateClass ICPStateClass;
typedef struct ICPState ICPState;
+typedef struct ICSStateClass ICSStateClass;
typedef struct ICSState ICSState;
typedef struct ICSIRQState ICSIRQState;
+struct XICSStateClass {
+ DeviceClass parent_class;
+
+ void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
+};
+
struct XICSState {
/*< private >*/
SysBusDevice parent_obj;
@@ -59,6 +73,18 @@ struct XICSState {
#define TYPE_ICP "icp"
#define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
+#define ICP_CLASS(klass) \
+ OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
+#define ICP_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(ICPStateClass, (obj), TYPE_ICP)
+
+struct ICPStateClass {
+ DeviceClass parent_class;
+
+ void (*pre_save)(ICPState *s);
+ int (*post_load)(ICPState *s);
+};
+
struct ICPState {
/*< private >*/
DeviceState parent_obj;
@@ -72,6 +98,18 @@ struct ICPState {
#define TYPE_ICS "ics"
#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
+#define ICS_CLASS(klass) \
+ OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
+#define ICS_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
+
+struct ICSStateClass {
+ DeviceClass parent_class;
+
+ void (*pre_save)(ICSState *s);
+ int (*post_load)(ICSState *s);
+};
+
struct ICSState {
/*< private >*/
DeviceState parent_obj;
@@ -98,6 +136,12 @@ struct ICSIRQState {
qemu_irq xics_get_qirq(XICSState *icp, int irq);
void xics_set_irq_type(XICSState *icp, int irq, bool lsi);
-void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu);
+static inline void xics_dispatch_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+{
+ XICSStateClass *info = XICS_GET_CLASS(icp);
+
+ assert(info->cpu_setup);
+ info->cpu_setup(icp, cpu);
+}
#endif /* __XICS_H__ */
--
1.8.3.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 3/4] xics: rework initialization
2013-07-17 6:37 [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
2013-07-17 6:37 ` [Qemu-devel] [PATCH 1/4] target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN Alexey Kardashevskiy
2013-07-17 6:37 ` [Qemu-devel] [PATCH 2/4] xics: add pre_save/post_load/cpu_setup dispatchers Alexey Kardashevskiy
@ 2013-07-17 6:37 ` Alexey Kardashevskiy
2013-07-31 19:22 ` Andreas Färber
2013-07-17 6:37 ` [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller Alexey Kardashevskiy
2013-07-30 2:29 ` [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
4 siblings, 1 reply; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-07-17 6:37 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, Alexey Kardashevskiy, Alexander Graf, qemu-ppc,
Paul Mackerras, David Gibson
Currently RTAS and hypercalls are registered in the XICS class init
function. The upcoming XICS-KVM will inherit from XICS but will use
another API to register RTAS tokens with KVM so registration has
to move from the class init function (common for both XICS and
XICS-KVM) to the _realize function (specific to the controller).
This moves ICS creation to _realize as there is no point to create
some child devices in initfn() (ICS) and some in realize() (ICPs).
As initfn is no more needed, this removes it.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
hw/intc/xics.c | 35 +++++++++++++++--------------------
1 file changed, 15 insertions(+), 20 deletions(-)
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index 283c2dd..a359f52 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -689,9 +689,23 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
static void xics_realize(DeviceState *dev, Error **errp)
{
XICSState *icp = XICS(dev);
- ICSState *ics = icp->ics;
+ ICSState *ics;
int i;
+ spapr_rtas_register("ibm,set-xive", rtas_set_xive);
+ spapr_rtas_register("ibm,get-xive", rtas_get_xive);
+ spapr_rtas_register("ibm,int-off", rtas_int_off);
+ spapr_rtas_register("ibm,int-on", rtas_int_on);
+
+ spapr_register_hypercall(H_CPPR, h_cppr);
+ spapr_register_hypercall(H_IPI, h_ipi);
+ spapr_register_hypercall(H_XIRR, h_xirr);
+ spapr_register_hypercall(H_EOI, h_eoi);
+
+ icp->ics = ICS(object_new(TYPE_ICS));
+ ics = icp->ics;
+ object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
+
ics->nr_irqs = icp->nr_irqs;
ics->offset = XICS_IRQ_BASE;
ics->icp = icp;
@@ -707,14 +721,6 @@ static void xics_realize(DeviceState *dev, Error **errp)
}
}
-static void xics_initfn(Object *obj)
-{
- XICSState *xics = XICS(obj);
-
- xics->ics = ICS(object_new(TYPE_ICS));
- object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
-}
-
static Property xics_properties[] = {
DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1),
DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1),
@@ -730,16 +736,6 @@ static void xics_class_init(ObjectClass *oc, void *data)
dc->props = xics_properties;
dc->reset = xics_reset;
k->cpu_setup = xics_cpu_setup;
-
- spapr_rtas_register("ibm,set-xive", rtas_set_xive);
- spapr_rtas_register("ibm,get-xive", rtas_get_xive);
- spapr_rtas_register("ibm,int-off", rtas_int_off);
- spapr_rtas_register("ibm,int-on", rtas_int_on);
-
- spapr_register_hypercall(H_CPPR, h_cppr);
- spapr_register_hypercall(H_IPI, h_ipi);
- spapr_register_hypercall(H_XIRR, h_xirr);
- spapr_register_hypercall(H_EOI, h_eoi);
}
static const TypeInfo xics_info = {
@@ -748,7 +744,6 @@ static const TypeInfo xics_info = {
.instance_size = sizeof(XICSState),
.class_size = sizeof(XICSStateClass),
.class_init = xics_class_init,
- .instance_init = xics_initfn,
};
static void xics_register_types(void)
--
1.8.3.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 3/4] xics: rework initialization
2013-07-17 6:37 ` [Qemu-devel] [PATCH 3/4] xics: rework initialization Alexey Kardashevskiy
@ 2013-07-31 19:22 ` Andreas Färber
0 siblings, 0 replies; 18+ messages in thread
From: Andreas Färber @ 2013-07-31 19:22 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, Alexander Graf, Paul Mackerras,
qemu-ppc, David Gibson
Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
> Currently RTAS and hypercalls are registered in the XICS class init
> function. The upcoming XICS-KVM will inherit from XICS but will use
> another API to register RTAS tokens with KVM so registration has
> to move from the class init function (common for both XICS and
> XICS-KVM) to the _realize function (specific to the controller).
This sounds sensible, but the reason is definitely not that there is
only class_init when you have two classes, but that registration of
global state belongs into realize.
> This moves ICS creation to _realize as there is no point to create
> some child devices in initfn() (ICS) and some in realize() (ICPs).
>
> As initfn is no more needed, this removes it.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/intc/xics.c | 35 +++++++++++++++--------------------
> 1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 283c2dd..a359f52 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -689,9 +689,23 @@ static void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> static void xics_realize(DeviceState *dev, Error **errp)
> {
> XICSState *icp = XICS(dev);
> - ICSState *ics = icp->ics;
> + ICSState *ics;
> int i;
>
> + spapr_rtas_register("ibm,set-xive", rtas_set_xive);
> + spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> + spapr_rtas_register("ibm,int-off", rtas_int_off);
> + spapr_rtas_register("ibm,int-on", rtas_int_on);
> +
> + spapr_register_hypercall(H_CPPR, h_cppr);
> + spapr_register_hypercall(H_IPI, h_ipi);
> + spapr_register_hypercall(H_XIRR, h_xirr);
> + spapr_register_hypercall(H_EOI, h_eoi);
> +
> + icp->ics = ICS(object_new(TYPE_ICS));
> + ics = icp->ics;
> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> +
> ics->nr_irqs = icp->nr_irqs;
> ics->offset = XICS_IRQ_BASE;
> ics->icp = icp;
> @@ -707,14 +721,6 @@ static void xics_realize(DeviceState *dev, Error **errp)
> }
> }
>
> -static void xics_initfn(Object *obj)
> -{
> - XICSState *xics = XICS(obj);
> -
> - xics->ics = ICS(object_new(TYPE_ICS));
> - object_property_add_child(obj, "ics", OBJECT(xics->ics), NULL);
> -}
> -
> static Property xics_properties[] = {
> DEFINE_PROP_UINT32("nr_servers", XICSState, nr_servers, -1),
> DEFINE_PROP_UINT32("nr_irqs", XICSState, nr_irqs, -1),
This looks wrong, both before and after. It should be both created using
object_initialize() and added as child property in instance_init. If
TYPE_ICS is a device, then in realize it should get realized.
Regards,
Andreas
> @@ -730,16 +736,6 @@ static void xics_class_init(ObjectClass *oc, void *data)
> dc->props = xics_properties;
> dc->reset = xics_reset;
> k->cpu_setup = xics_cpu_setup;
> -
> - spapr_rtas_register("ibm,set-xive", rtas_set_xive);
> - spapr_rtas_register("ibm,get-xive", rtas_get_xive);
> - spapr_rtas_register("ibm,int-off", rtas_int_off);
> - spapr_rtas_register("ibm,int-on", rtas_int_on);
> -
> - spapr_register_hypercall(H_CPPR, h_cppr);
> - spapr_register_hypercall(H_IPI, h_ipi);
> - spapr_register_hypercall(H_XIRR, h_xirr);
> - spapr_register_hypercall(H_EOI, h_eoi);
> }
>
> static const TypeInfo xics_info = {
> @@ -748,7 +744,6 @@ static const TypeInfo xics_info = {
> .instance_size = sizeof(XICSState),
> .class_size = sizeof(XICSStateClass),
> .class_init = xics_class_init,
> - .instance_init = xics_initfn,
> };
>
> static void xics_register_types(void)
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-07-17 6:37 [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
` (2 preceding siblings ...)
2013-07-17 6:37 ` [Qemu-devel] [PATCH 3/4] xics: rework initialization Alexey Kardashevskiy
@ 2013-07-17 6:37 ` Alexey Kardashevskiy
2013-07-31 18:05 ` Anthony Liguori
2013-07-31 19:52 ` Andreas Färber
2013-07-30 2:29 ` [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
4 siblings, 2 replies; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-07-17 6:37 UTC (permalink / raw)
To: qemu-devel
Cc: Anthony Liguori, aik, Alexander Graf, qemu-ppc, Paul Mackerras,
David Gibson
From: David Gibson <david@gibson.dropbear.id.au>
Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
controller system within KVM. This patch allows qemu to initialize and
configure the in-kernel XICS, and keep its state in sync with qemu's XICS
state as necessary.
This should give considerable performance improvements. e.g. on a simple
IPI ping-pong test between hardware threads, using qemu XICS gives us
around 5,000 irqs/second, whereas the in-kernel XICS gives us around
70,000 irqs/s on the same hardware configuration.
[Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
[aik: moved to a separate device, reworked QOM]
---
Changes:
2013/07/17:
* QOM rework
2013/07/01
* fixed VMState names in order to support xics-kvm migration to xics and vice versa
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
default-configs/ppc64-softmmu.mak | 1 +
hw/intc/Makefile.objs | 1 +
hw/intc/xics_kvm.c | 469 ++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr.c | 26 ++-
include/hw/ppc/xics.h | 10 +
5 files changed, 506 insertions(+), 1 deletion(-)
create mode 100644 hw/intc/xics_kvm.c
diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
index 6d1933b..1f4550a 100644
--- a/default-configs/ppc64-softmmu.mak
+++ b/default-configs/ppc64-softmmu.mak
@@ -47,5 +47,6 @@ CONFIG_E500=y
CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
# For pSeries
CONFIG_XICS=$(CONFIG_PSERIES)
+CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
# For PReP
CONFIG_MC146818RTC=y
diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
index 2851eed..47ac442 100644
--- a/hw/intc/Makefile.objs
+++ b/hw/intc/Makefile.objs
@@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
obj-$(CONFIG_SH4) += sh_intc.o
obj-$(CONFIG_XICS) += xics.o
+obj-$(CONFIG_XICS_KVM) += xics_kvm.o
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
new file mode 100644
index 0000000..04ce1be
--- /dev/null
+++ b/hw/intc/xics_kvm.c
@@ -0,0 +1,469 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
+ *
+ * Copyright (c) 2013 David Gibson, IBM Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ */
+
+#include "hw/hw.h"
+#include "trace.h"
+#include "hw/ppc/spapr.h"
+#include "hw/ppc/xics.h"
+#include "kvm_ppc.h"
+#include "qemu/config-file.h"
+
+#include <sys/ioctl.h>
+
+typedef struct XICSStateKVM {
+ struct XICSState parent;
+
+ uint32_t set_xive_token;
+ uint32_t get_xive_token;
+ uint32_t int_off_token;
+ uint32_t int_on_token;
+ int kernel_xics_fd;
+} XICSStateKVM;
+
+/*
+ * ICP-KVM
+ */
+static void icp_get_kvm_state(ICPState *ss)
+{
+ uint64_t state;
+ struct kvm_one_reg reg = {
+ .id = KVM_REG_PPC_ICP_STATE,
+ .addr = (uintptr_t)&state,
+ };
+ int ret;
+
+ if (!ss->cs) {
+ return; /* kernel irqchip not in use */
+ }
+
+ ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, ®);
+ if (ret != 0) {
+ fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
+ " for CPU %d: %s\n", ss->cs->cpu_index, strerror(errno));
+ exit(1);
+ }
+
+ ss->xirr = state >> KVM_REG_PPC_ICP_XISR_SHIFT;
+ ss->mfrr = (state >> KVM_REG_PPC_ICP_MFRR_SHIFT)
+ & KVM_REG_PPC_ICP_MFRR_MASK;
+ ss->pending_priority = (state >> KVM_REG_PPC_ICP_PPRI_SHIFT)
+ & KVM_REG_PPC_ICP_PPRI_MASK;
+}
+
+static int icp_set_kvm_state(ICPState *ss)
+{
+ uint64_t state;
+ struct kvm_one_reg reg = {
+ .id = KVM_REG_PPC_ICP_STATE,
+ .addr = (uintptr_t)&state,
+ };
+ int ret;
+
+ if (!ss->cs) {
+ return 0; /* kernel irqchip not in use */
+ }
+
+ state = ((uint64_t)ss->xirr << KVM_REG_PPC_ICP_XISR_SHIFT)
+ | ((uint64_t)ss->mfrr << KVM_REG_PPC_ICP_MFRR_SHIFT)
+ | ((uint64_t)ss->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
+
+ ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, ®);
+ if (ret != 0) {
+ fprintf(stderr, "Unable to restore KVM interrupt controller state (0x%"
+ PRIx64 ") for CPU %d: %s\n", state, ss->cs->cpu_index,
+ strerror(errno));
+ return ret;
+ }
+
+ return 0;
+}
+
+static void icp_kvm_reset(DeviceState *dev)
+{
+ ICPState *icp = ICP(dev);
+
+ icp->xirr = 0;
+ icp->pending_priority = 0xff;
+ icp->mfrr = 0xff;
+
+ /* Make all outputs are deasserted */
+ qemu_set_irq(icp->output, 0);
+
+ icp_set_kvm_state(icp);
+}
+
+static void icp_kvm_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ ICPStateClass *k = ICP_CLASS(klass);
+
+ dc->reset = icp_kvm_reset;
+ k->pre_save = icp_get_kvm_state;
+ k->post_load = icp_set_kvm_state;
+}
+
+static TypeInfo icp_kvm_info = {
+ .name = TYPE_ICP_KVM,
+ .parent = TYPE_ICP,
+ .instance_size = sizeof(ICPState),
+ .class_init = icp_kvm_class_init,
+ .class_size = sizeof(ICPStateClass),
+};
+
+/*
+ * ICS-KVM
+ */
+static void ics_get_kvm_state(ICSState *ics)
+{
+ XICSStateKVM *icpkvm = XICS_KVM(ics->icp);
+ uint64_t state;
+ struct kvm_device_attr attr = {
+ .flags = 0,
+ .group = KVM_DEV_XICS_GRP_SOURCES,
+ .addr = (uint64_t)(uintptr_t)&state,
+ };
+ int i;
+
+ for (i = 0; i < ics->nr_irqs; i++) {
+ ICSIRQState *irq = &ics->irqs[i];
+ int ret;
+
+ attr.attr = i + ics->offset;
+
+ ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
+ if (ret != 0) {
+ fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
+ " for IRQ %d: %s\n", i + ics->offset, strerror(errno));
+ exit(1);
+ }
+
+ irq->server = state & KVM_XICS_DESTINATION_MASK;
+ irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
+ & KVM_XICS_PRIORITY_MASK;
+ /*
+ * To be consistent with the software emulation in xics.c, we
+ * split out the masked state + priority that we get from the
+ * kernel into 'current priority' (0xff if masked) and
+ * 'saved priority' (if masked, this is the priority the
+ * interrupt had before it was masked). Masking and unmasking
+ * are done with the ibm,int-off and ibm,int-on RTAS calls.
+ */
+ if (state & KVM_XICS_MASKED) {
+ irq->priority = 0xff;
+ } else {
+ irq->priority = irq->saved_priority;
+ }
+
+ if (state & KVM_XICS_PENDING) {
+ if (state & KVM_XICS_LEVEL_SENSITIVE) {
+ irq->status |= XICS_STATUS_ASSERTED;
+ } else {
+ /*
+ * A pending edge-triggered interrupt (or MSI)
+ * must have been rejected previously when we
+ * first detected it and tried to deliver it,
+ * so mark it as pending and previously rejected
+ * for consistency with how xics.c works.
+ */
+ irq->status |= XICS_STATUS_MASKED_PENDING
+ | XICS_STATUS_REJECTED;
+ }
+ }
+ }
+}
+
+static int ics_set_kvm_state(ICSState *ics)
+{
+ XICSStateKVM *icpkvm = XICS_KVM(ics->icp);
+ uint64_t state;
+ struct kvm_device_attr attr = {
+ .flags = 0,
+ .group = KVM_DEV_XICS_GRP_SOURCES,
+ .addr = (uint64_t)(uintptr_t)&state,
+ };
+ int i;
+
+ for (i = 0; i < ics->nr_irqs; i++) {
+ ICSIRQState *irq = &ics->irqs[i];
+ int ret;
+
+ attr.attr = i + ics->offset;
+
+ state = irq->server;
+ state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
+ << KVM_XICS_PRIORITY_SHIFT;
+ if (irq->priority != irq->saved_priority) {
+ assert(irq->priority == 0xff);
+ state |= KVM_XICS_MASKED;
+ }
+
+ if (ics->islsi[i]) {
+ state |= KVM_XICS_LEVEL_SENSITIVE;
+ if (irq->status & XICS_STATUS_ASSERTED) {
+ state |= KVM_XICS_PENDING;
+ }
+ } else {
+ if (irq->status & XICS_STATUS_MASKED_PENDING) {
+ state |= KVM_XICS_PENDING;
+ }
+ }
+
+ ret = ioctl(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
+ if (ret != 0) {
+ fprintf(stderr, "Unable to restore KVM interrupt controller state"
+ " for IRQs %d: %s\n", i + ics->offset, strerror(errno));
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void ics_kvm_set_irq(void *opaque, int srcno, int val)
+{
+ ICSState *ics = opaque;
+ struct kvm_irq_level args;
+ int rc;
+
+ args.irq = srcno + ics->offset;
+ if (!ics->islsi[srcno]) {
+ if (!val) {
+ return;
+ }
+ args.level = KVM_INTERRUPT_SET;
+ } else {
+ args.level = val ? KVM_INTERRUPT_SET_LEVEL : KVM_INTERRUPT_UNSET;
+ }
+ rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
+ if (rc < 0) {
+ perror("kvm_irq_line");
+ }
+}
+
+static void ics_kvm_reset(DeviceState *dev)
+{
+ ics_set_kvm_state(ICS(dev));
+}
+
+static int ics_kvm_realize(DeviceState *dev)
+{
+ ICSState *ics = ICS(dev);
+
+ ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
+ ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
+ ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
+
+ return 0;
+}
+
+static void ics_kvm_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ ICSStateClass *k = ICS_CLASS(klass);
+
+ dc->init = ics_kvm_realize;
+ dc->reset = ics_kvm_reset;
+ k->pre_save = ics_get_kvm_state;
+ k->post_load = ics_set_kvm_state;
+}
+
+static TypeInfo ics_kvm_info = {
+ .name = TYPE_ICS_KVM,
+ .parent = TYPE_ICS,
+ .instance_size = sizeof(ICSState),
+ .class_init = ics_kvm_class_init,
+};
+
+/*
+ * XICS-KVM
+ */
+static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
+{
+ CPUState *cs;
+ ICPState *ss;
+ XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
+ OBJECT(icp), TYPE_XICS_KVM);
+ XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
+
+ if (!icpkvm) {
+ return;
+ }
+
+ cs = CPU(cpu);
+ ss = &icp->ss[cs->cpu_index];
+
+ assert(cs->cpu_index < icp->nr_servers);
+ if (icpkvm->kernel_xics_fd == -1) {
+ abort();
+ }
+
+ if (icpkvm->kernel_xics_fd != -1) {
+ int ret;
+ struct kvm_enable_cap xics_enable_cap = {
+ .cap = KVM_CAP_IRQ_XICS,
+ .flags = 0,
+ .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
+ };
+
+ ss->cs = cs;
+
+ ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
+ if (ret < 0) {
+ fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
+ cs->cpu_index, strerror(errno));
+ exit(1);
+ }
+ }
+
+ /* Call emulated XICS implementation for consistency */
+ assert(xics_info);
+ xics_info->cpu_setup(icp, cpu);
+}
+
+static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+ uint32_t token,
+ uint32_t nargs, target_ulong args,
+ uint32_t nret, target_ulong rets)
+{
+ fprintf(stderr, "pseries: %s must never be called for in-kernel XICS\n",
+ __func__);
+}
+
+static void xics_kvm_realize(DeviceState *dev, Error **errp)
+{
+ XICSStateKVM *icpkvm = XICS_KVM(dev);
+ XICSState *icp = XICS(dev);
+ ICSState *ics;
+ QemuOptsList *list = qemu_find_opts("machine");
+ int i, rc;
+ struct kvm_create_device xics_create_device = {
+ .type = KVM_DEV_TYPE_XICS,
+ .flags = 0,
+ };
+
+ if (!kvm_enabled()) {
+ error_setg(errp, "KVM must be enabled for in-kernel XICS");
+ goto fail;
+ }
+
+ if (QTAILQ_EMPTY(&list->head) ||
+ !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+ "kernel_irqchip", true) ||
+ !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
+ error_setg(errp, "KVM must be enabled for in-kernel XICS");
+ return;
+ }
+
+ icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
+ icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
+ icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
+ icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
+
+ rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
+ if (rc < 0) {
+ error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
+ goto fail;
+ }
+
+ rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
+ if (rc < 0) {
+ error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
+ goto fail;
+ }
+
+ rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
+ if (rc < 0) {
+ error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
+ goto fail;
+ }
+
+ rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
+ if (rc < 0) {
+ error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off");
+ goto fail;
+ }
+
+ /* Create the kernel ICP */
+ rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
+ if (rc < 0) {
+ error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
+ goto fail;
+ }
+
+ icpkvm->kernel_xics_fd = xics_create_device.fd;
+
+ icp->ics = ICS(object_new(TYPE_ICS_KVM));
+ ics = icp->ics;
+ object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
+
+ ics->nr_irqs = icp->nr_irqs;
+ ics->offset = XICS_IRQ_BASE;
+ ics->icp = icp;
+ qdev_init_nofail(DEVICE(ics));
+
+ icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
+ for (i = 0; i < icp->nr_servers; i++) {
+ char buffer[32];
+ object_initialize(&icp->ss[i], TYPE_ICP_KVM);
+ snprintf(buffer, sizeof(buffer), "icp[%d]", i);
+ object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
+ qdev_init_nofail(DEVICE(&icp->ss[i]));
+ }
+ return;
+
+fail:
+ kvmppc_define_rtas_token(0, "ibm,set-xive");
+ kvmppc_define_rtas_token(0, "ibm,get-xive");
+ kvmppc_define_rtas_token(0, "ibm,int-on");
+ kvmppc_define_rtas_token(0, "ibm,int-off");
+ return;
+}
+
+static void xics_kvm_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+ XICSStateClass *k = XICS_CLASS(oc);
+
+ dc->realize = xics_kvm_realize;
+ k->cpu_setup = xics_kvm_cpu_setup;
+}
+
+static const TypeInfo xics_kvm_info = {
+ .name = TYPE_XICS_KVM,
+ .parent = TYPE_XICS,
+ .instance_size = sizeof(XICSStateKVM),
+ .class_init = xics_kvm_class_init,
+};
+
+static void xics_kvm_register_types(void)
+{
+ type_register_static(&xics_kvm_info);
+ type_register_static(&ics_kvm_info);
+ type_register_static(&icp_kvm_info);
+}
+
+type_init(xics_kvm_register_types)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 432f0d2..84433ee 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -148,7 +148,31 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
{
XICSState *icp = NULL;
- icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
+ if (kvm_enabled()) {
+ bool irqchip_allowed = true, irqchip_required = false;
+ QemuOptsList *list = qemu_find_opts("machine");
+
+ if (!QTAILQ_EMPTY(&list->head)) {
+ irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+ "kernel_irqchip", true);
+ irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
+ "kernel_irqchip", false);
+ }
+
+ if (irqchip_allowed) {
+ icp = try_create_xics(TYPE_XICS_KVM, nr_servers, nr_irqs);
+ }
+
+ if (irqchip_required && !icp) {
+ perror("iFailed to create in-kernel XICS\n");
+ abort();
+ }
+ }
+
+ if (!icp) {
+ icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
+ }
+
if (!icp) {
perror("Failed to create XICS\n");
abort();
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 90ecaf8..835a3d6 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -32,6 +32,9 @@
#define TYPE_XICS "xics"
#define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
+#define TYPE_XICS_KVM "xics-kvm"
+#define XICS_KVM(obj) OBJECT_CHECK(XICSStateKVM, (obj), TYPE_XICS_KVM)
+
#define XICS_CLASS(klass) \
OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
#define XICS_GET_CLASS(obj) \
@@ -73,6 +76,9 @@ struct XICSState {
#define TYPE_ICP "icp"
#define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
+#define TYPE_ICP_KVM "icp-kvm"
+#define ICP_KVM(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP_KVM)
+
#define ICP_CLASS(klass) \
OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
#define ICP_GET_CLASS(obj) \
@@ -89,6 +95,7 @@ struct ICPState {
/*< private >*/
DeviceState parent_obj;
/*< public >*/
+ CPUState *cs;
uint32_t xirr;
uint8_t pending_priority;
uint8_t mfrr;
@@ -98,6 +105,9 @@ struct ICPState {
#define TYPE_ICS "ics"
#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
+#define TYPE_ICS_KVM "icskvm"
+#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
+
#define ICS_CLASS(klass) \
OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
#define ICS_GET_CLASS(obj) \
--
1.8.3.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-07-17 6:37 ` [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller Alexey Kardashevskiy
@ 2013-07-31 18:05 ` Anthony Liguori
2013-07-31 19:52 ` Andreas Färber
1 sibling, 0 replies; 18+ messages in thread
From: Anthony Liguori @ 2013-07-31 18:05 UTC (permalink / raw)
To: Alexey Kardashevskiy, qemu-devel
Cc: Alexander Graf, qemu-ppc, Paul Mackerras, David Gibson
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> From: David Gibson <david@gibson.dropbear.id.au>
>
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM. This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
>
> This should give considerable performance improvements. e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
>
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [aik: moved to a separate device, reworked QOM]
>
> ---
> Changes:
> 2013/07/17:
> * QOM rework
>
> 2013/07/01
> * fixed VMState names in order to support xics-kvm migration to xics and vice versa
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> default-configs/ppc64-softmmu.mak | 1 +
> hw/intc/Makefile.objs | 1 +
> hw/intc/xics_kvm.c | 469 ++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr.c | 26 ++-
> include/hw/ppc/xics.h | 10 +
> 5 files changed, 506 insertions(+), 1 deletion(-)
> create mode 100644 hw/intc/xics_kvm.c
>
> diff --git a/default-configs/ppc64-softmmu.mak b/default-configs/ppc64-softmmu.mak
> index 6d1933b..1f4550a 100644
> --- a/default-configs/ppc64-softmmu.mak
> +++ b/default-configs/ppc64-softmmu.mak
> @@ -47,5 +47,6 @@ CONFIG_E500=y
> CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
> # For pSeries
> CONFIG_XICS=$(CONFIG_PSERIES)
> +CONFIG_XICS_KVM=$(and $(CONFIG_PSERIES),$(CONFIG_KVM))
> # For PReP
> CONFIG_MC146818RTC=y
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 2851eed..47ac442 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -23,3 +23,4 @@ obj-$(CONFIG_OMAP) += omap_intc.o
> obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
> obj-$(CONFIG_SH4) += sh_intc.o
> obj-$(CONFIG_XICS) += xics.o
> +obj-$(CONFIG_XICS_KVM) += xics_kvm.o
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> new file mode 100644
> index 0000000..04ce1be
> --- /dev/null
> +++ b/hw/intc/xics_kvm.c
> @@ -0,0 +1,469 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics, in-kernel emulation
> + *
> + * Copyright (c) 2013 David Gibson, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + *
> + */
> +
> +#include "hw/hw.h"
> +#include "trace.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/ppc/xics.h"
> +#include "kvm_ppc.h"
> +#include "qemu/config-file.h"
> +
> +#include <sys/ioctl.h>
> +
> +typedef struct XICSStateKVM {
> + struct XICSState parent;
'struct' is not necessary here.
> +
> + uint32_t set_xive_token;
> + uint32_t get_xive_token;
> + uint32_t int_off_token;
> + uint32_t int_on_token;
> + int kernel_xics_fd;
> +} XICSStateKVM;
> +
> +/*
> + * ICP-KVM
> + */
> +static void icp_get_kvm_state(ICPState *ss)
> +{
> + uint64_t state;
> + struct kvm_one_reg reg = {
> + .id = KVM_REG_PPC_ICP_STATE,
> + .addr = (uintptr_t)&state,
> + };
> + int ret;
> +
> + if (!ss->cs) {
> + return; /* kernel irqchip not in use */
> + }
Should this be an assert?
> +
> + ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, ®);
> + if (ret != 0) {
> + fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
> + " for CPU %d: %s\n", ss->cs->cpu_index, strerror(errno));
> + exit(1);
> + }
> +
> + ss->xirr = state >> KVM_REG_PPC_ICP_XISR_SHIFT;
> + ss->mfrr = (state >> KVM_REG_PPC_ICP_MFRR_SHIFT)
> + & KVM_REG_PPC_ICP_MFRR_MASK;
> + ss->pending_priority = (state >> KVM_REG_PPC_ICP_PPRI_SHIFT)
> + & KVM_REG_PPC_ICP_PPRI_MASK;
> +}
> +
> +static int icp_set_kvm_state(ICPState *ss)
> +{
> + uint64_t state;
> + struct kvm_one_reg reg = {
> + .id = KVM_REG_PPC_ICP_STATE,
> + .addr = (uintptr_t)&state,
> + };
> + int ret;
> +
> + if (!ss->cs) {
> + return 0; /* kernel irqchip not in use */
> + }
assert?
Regards,
Anthony Liguori
> +
> + state = ((uint64_t)ss->xirr << KVM_REG_PPC_ICP_XISR_SHIFT)
> + | ((uint64_t)ss->mfrr << KVM_REG_PPC_ICP_MFRR_SHIFT)
> + | ((uint64_t)ss->pending_priority << KVM_REG_PPC_ICP_PPRI_SHIFT);
> +
> + ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, ®);
> + if (ret != 0) {
> + fprintf(stderr, "Unable to restore KVM interrupt controller state (0x%"
> + PRIx64 ") for CPU %d: %s\n", state, ss->cs->cpu_index,
> + strerror(errno));
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void icp_kvm_reset(DeviceState *dev)
> +{
> + ICPState *icp = ICP(dev);
> +
> + icp->xirr = 0;
> + icp->pending_priority = 0xff;
> + icp->mfrr = 0xff;
> +
> + /* Make all outputs are deasserted */
> + qemu_set_irq(icp->output, 0);
> +
> + icp_set_kvm_state(icp);
> +}
> +
> +static void icp_kvm_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + ICPStateClass *k = ICP_CLASS(klass);
> +
> + dc->reset = icp_kvm_reset;
> + k->pre_save = icp_get_kvm_state;
> + k->post_load = icp_set_kvm_state;
> +}
> +
> +static TypeInfo icp_kvm_info = {
> + .name = TYPE_ICP_KVM,
> + .parent = TYPE_ICP,
> + .instance_size = sizeof(ICPState),
> + .class_init = icp_kvm_class_init,
> + .class_size = sizeof(ICPStateClass),
> +};
> +
> +/*
> + * ICS-KVM
> + */
> +static void ics_get_kvm_state(ICSState *ics)
> +{
> + XICSStateKVM *icpkvm = XICS_KVM(ics->icp);
> + uint64_t state;
> + struct kvm_device_attr attr = {
> + .flags = 0,
> + .group = KVM_DEV_XICS_GRP_SOURCES,
> + .addr = (uint64_t)(uintptr_t)&state,
> + };
> + int i;
> +
> + for (i = 0; i < ics->nr_irqs; i++) {
> + ICSIRQState *irq = &ics->irqs[i];
> + int ret;
> +
> + attr.attr = i + ics->offset;
> +
> + ret = ioctl(icpkvm->kernel_xics_fd, KVM_GET_DEVICE_ATTR, &attr);
> + if (ret != 0) {
> + fprintf(stderr, "Unable to retrieve KVM interrupt controller state"
> + " for IRQ %d: %s\n", i + ics->offset, strerror(errno));
> + exit(1);
> + }
> +
> + irq->server = state & KVM_XICS_DESTINATION_MASK;
> + irq->saved_priority = (state >> KVM_XICS_PRIORITY_SHIFT)
> + & KVM_XICS_PRIORITY_MASK;
> + /*
> + * To be consistent with the software emulation in xics.c, we
> + * split out the masked state + priority that we get from the
> + * kernel into 'current priority' (0xff if masked) and
> + * 'saved priority' (if masked, this is the priority the
> + * interrupt had before it was masked). Masking and unmasking
> + * are done with the ibm,int-off and ibm,int-on RTAS calls.
> + */
> + if (state & KVM_XICS_MASKED) {
> + irq->priority = 0xff;
> + } else {
> + irq->priority = irq->saved_priority;
> + }
> +
> + if (state & KVM_XICS_PENDING) {
> + if (state & KVM_XICS_LEVEL_SENSITIVE) {
> + irq->status |= XICS_STATUS_ASSERTED;
> + } else {
> + /*
> + * A pending edge-triggered interrupt (or MSI)
> + * must have been rejected previously when we
> + * first detected it and tried to deliver it,
> + * so mark it as pending and previously rejected
> + * for consistency with how xics.c works.
> + */
> + irq->status |= XICS_STATUS_MASKED_PENDING
> + | XICS_STATUS_REJECTED;
> + }
> + }
> + }
> +}
> +
> +static int ics_set_kvm_state(ICSState *ics)
> +{
> + XICSStateKVM *icpkvm = XICS_KVM(ics->icp);
> + uint64_t state;
> + struct kvm_device_attr attr = {
> + .flags = 0,
> + .group = KVM_DEV_XICS_GRP_SOURCES,
> + .addr = (uint64_t)(uintptr_t)&state,
> + };
> + int i;
> +
> + for (i = 0; i < ics->nr_irqs; i++) {
> + ICSIRQState *irq = &ics->irqs[i];
> + int ret;
> +
> + attr.attr = i + ics->offset;
> +
> + state = irq->server;
> + state |= (uint64_t)(irq->saved_priority & KVM_XICS_PRIORITY_MASK)
> + << KVM_XICS_PRIORITY_SHIFT;
> + if (irq->priority != irq->saved_priority) {
> + assert(irq->priority == 0xff);
> + state |= KVM_XICS_MASKED;
> + }
> +
> + if (ics->islsi[i]) {
> + state |= KVM_XICS_LEVEL_SENSITIVE;
> + if (irq->status & XICS_STATUS_ASSERTED) {
> + state |= KVM_XICS_PENDING;
> + }
> + } else {
> + if (irq->status & XICS_STATUS_MASKED_PENDING) {
> + state |= KVM_XICS_PENDING;
> + }
> + }
> +
> + ret = ioctl(icpkvm->kernel_xics_fd, KVM_SET_DEVICE_ATTR, &attr);
> + if (ret != 0) {
> + fprintf(stderr, "Unable to restore KVM interrupt controller state"
> + " for IRQs %d: %s\n", i + ics->offset, strerror(errno));
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void ics_kvm_set_irq(void *opaque, int srcno, int val)
> +{
> + ICSState *ics = opaque;
> + struct kvm_irq_level args;
> + int rc;
> +
> + args.irq = srcno + ics->offset;
> + if (!ics->islsi[srcno]) {
> + if (!val) {
> + return;
> + }
> + args.level = KVM_INTERRUPT_SET;
> + } else {
> + args.level = val ? KVM_INTERRUPT_SET_LEVEL : KVM_INTERRUPT_UNSET;
> + }
> + rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> + if (rc < 0) {
> + perror("kvm_irq_line");
> + }
> +}
> +
> +static void ics_kvm_reset(DeviceState *dev)
> +{
> + ics_set_kvm_state(ICS(dev));
> +}
> +
> +static int ics_kvm_realize(DeviceState *dev)
> +{
> + ICSState *ics = ICS(dev);
> +
> + ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> + ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
> + ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
> +
> + return 0;
> +}
> +
> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + ICSStateClass *k = ICS_CLASS(klass);
> +
> + dc->init = ics_kvm_realize;
> + dc->reset = ics_kvm_reset;
> + k->pre_save = ics_get_kvm_state;
> + k->post_load = ics_set_kvm_state;
> +}
> +
> +static TypeInfo ics_kvm_info = {
> + .name = TYPE_ICS_KVM,
> + .parent = TYPE_ICS,
> + .instance_size = sizeof(ICSState),
> + .class_init = ics_kvm_class_init,
> +};
> +
> +/*
> + * XICS-KVM
> + */
> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> + CPUState *cs;
> + ICPState *ss;
> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
> + OBJECT(icp), TYPE_XICS_KVM);
> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
> +
> + if (!icpkvm) {
> + return;
> + }
> +
> + cs = CPU(cpu);
> + ss = &icp->ss[cs->cpu_index];
> +
> + assert(cs->cpu_index < icp->nr_servers);
> + if (icpkvm->kernel_xics_fd == -1) {
> + abort();
> + }
> +
> + if (icpkvm->kernel_xics_fd != -1) {
> + int ret;
> + struct kvm_enable_cap xics_enable_cap = {
> + .cap = KVM_CAP_IRQ_XICS,
> + .flags = 0,
> + .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
> + };
> +
> + ss->cs = cs;
> +
> + ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
> + if (ret < 0) {
> + fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
> + cs->cpu_index, strerror(errno));
> + exit(1);
> + }
> + }
> +
> + /* Call emulated XICS implementation for consistency */
> + assert(xics_info);
> + xics_info->cpu_setup(icp, cpu);
> +}
> +
> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> + uint32_t token,
> + uint32_t nargs, target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + fprintf(stderr, "pseries: %s must never be called for in-kernel XICS\n",
> + __func__);
> +}
> +
> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> + XICSStateKVM *icpkvm = XICS_KVM(dev);
> + XICSState *icp = XICS(dev);
> + ICSState *ics;
> + QemuOptsList *list = qemu_find_opts("machine");
> + int i, rc;
> + struct kvm_create_device xics_create_device = {
> + .type = KVM_DEV_TYPE_XICS,
> + .flags = 0,
> + };
> +
> + if (!kvm_enabled()) {
> + error_setg(errp, "KVM must be enabled for in-kernel XICS");
> + goto fail;
> + }
> +
> + if (QTAILQ_EMPTY(&list->head) ||
> + !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> + "kernel_irqchip", true) ||
> + !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> + error_setg(errp, "KVM must be enabled for in-kernel XICS");
> + return;
> + }
> +
> + icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
> + icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
> + icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
> + icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
> +
> + rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
> + goto fail;
> + }
> +
> + rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
> + goto fail;
> + }
> +
> + rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
> + goto fail;
> + }
> +
> + rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off");
> + goto fail;
> + }
> +
> + /* Create the kernel ICP */
> + rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
> + if (rc < 0) {
> + error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> + goto fail;
> + }
> +
> + icpkvm->kernel_xics_fd = xics_create_device.fd;
> +
> + icp->ics = ICS(object_new(TYPE_ICS_KVM));
> + ics = icp->ics;
> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> +
> + ics->nr_irqs = icp->nr_irqs;
> + ics->offset = XICS_IRQ_BASE;
> + ics->icp = icp;
> + qdev_init_nofail(DEVICE(ics));
> +
> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
> + for (i = 0; i < icp->nr_servers; i++) {
> + char buffer[32];
> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
> + qdev_init_nofail(DEVICE(&icp->ss[i]));
> + }
> + return;
> +
> +fail:
> + kvmppc_define_rtas_token(0, "ibm,set-xive");
> + kvmppc_define_rtas_token(0, "ibm,get-xive");
> + kvmppc_define_rtas_token(0, "ibm,int-on");
> + kvmppc_define_rtas_token(0, "ibm,int-off");
> + return;
> +}
> +
> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + XICSStateClass *k = XICS_CLASS(oc);
> +
> + dc->realize = xics_kvm_realize;
> + k->cpu_setup = xics_kvm_cpu_setup;
> +}
> +
> +static const TypeInfo xics_kvm_info = {
> + .name = TYPE_XICS_KVM,
> + .parent = TYPE_XICS,
> + .instance_size = sizeof(XICSStateKVM),
> + .class_init = xics_kvm_class_init,
> +};
> +
> +static void xics_kvm_register_types(void)
> +{
> + type_register_static(&xics_kvm_info);
> + type_register_static(&ics_kvm_info);
> + type_register_static(&icp_kvm_info);
> +}
> +
> +type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 432f0d2..84433ee 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -148,7 +148,31 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
> {
> XICSState *icp = NULL;
>
> - icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> + if (kvm_enabled()) {
> + bool irqchip_allowed = true, irqchip_required = false;
> + QemuOptsList *list = qemu_find_opts("machine");
> +
> + if (!QTAILQ_EMPTY(&list->head)) {
> + irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> + "kernel_irqchip", true);
> + irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> + "kernel_irqchip", false);
> + }
> +
> + if (irqchip_allowed) {
> + icp = try_create_xics(TYPE_XICS_KVM, nr_servers, nr_irqs);
> + }
> +
> + if (irqchip_required && !icp) {
> + perror("iFailed to create in-kernel XICS\n");
> + abort();
> + }
> + }
> +
> + if (!icp) {
> + icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> + }
> +
> if (!icp) {
> perror("Failed to create XICS\n");
> abort();
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 90ecaf8..835a3d6 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -32,6 +32,9 @@
> #define TYPE_XICS "xics"
> #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>
> +#define TYPE_XICS_KVM "xics-kvm"
> +#define XICS_KVM(obj) OBJECT_CHECK(XICSStateKVM, (obj), TYPE_XICS_KVM)
> +
> #define XICS_CLASS(klass) \
> OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
> #define XICS_GET_CLASS(obj) \
> @@ -73,6 +76,9 @@ struct XICSState {
> #define TYPE_ICP "icp"
> #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>
> +#define TYPE_ICP_KVM "icp-kvm"
> +#define ICP_KVM(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP_KVM)
> +
> #define ICP_CLASS(klass) \
> OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
> #define ICP_GET_CLASS(obj) \
> @@ -89,6 +95,7 @@ struct ICPState {
> /*< private >*/
> DeviceState parent_obj;
> /*< public >*/
> + CPUState *cs;
> uint32_t xirr;
> uint8_t pending_priority;
> uint8_t mfrr;
> @@ -98,6 +105,9 @@ struct ICPState {
> #define TYPE_ICS "ics"
> #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>
> +#define TYPE_ICS_KVM "icskvm"
> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
> +
> #define ICS_CLASS(klass) \
> OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
> #define ICS_GET_CLASS(obj) \
> --
> 1.8.3.2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-07-17 6:37 ` [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller Alexey Kardashevskiy
2013-07-31 18:05 ` Anthony Liguori
@ 2013-07-31 19:52 ` Andreas Färber
2013-07-31 20:47 ` Peter Maydell
2013-08-01 0:14 ` Alexey Kardashevskiy
1 sibling, 2 replies; 18+ messages in thread
From: Andreas Färber @ 2013-07-31 19:52 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Anthony Liguori, qemu-devel, Alexander Graf, Paul Mackerras,
qemu-ppc, David Gibson
Hi,
Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
> From: David Gibson <david@gibson.dropbear.id.au>
>
> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
> controller system within KVM. This patch allows qemu to initialize and
> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
> state as necessary.
>
> This should give considerable performance improvements. e.g. on a simple
> IPI ping-pong test between hardware threads, using qemu XICS gives us
> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
> 70,000 irqs/s on the same hardware configuration.
>
> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [aik: moved to a separate device, reworked QOM]
>
> ---
> Changes:
> 2013/07/17:
> * QOM rework
>
> 2013/07/01
> * fixed VMState names in order to support xics-kvm migration to xics and vice versa
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Something went wrong here. ;)
[...]
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> new file mode 100644
> index 0000000..04ce1be
> --- /dev/null
> +++ b/hw/intc/xics_kvm.c
[...]
> +typedef struct XICSStateKVM {
> + struct XICSState parent;
parent_obj to align with most new devices?
> +
> + uint32_t set_xive_token;
> + uint32_t get_xive_token;
> + uint32_t int_off_token;
> + uint32_t int_on_token;
> + int kernel_xics_fd;
> +} XICSStateKVM;
[...]
> +static int ics_kvm_realize(DeviceState *dev)
> +{
> + ICSState *ics = ICS(dev);
> +
> + ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> + ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
> + ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
> +
> + return 0;
> +}
> +
> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + ICSStateClass *k = ICS_CLASS(klass);
> +
> + dc->init = ics_kvm_realize;
This is fishy - probably you want to assign to dc->realize and fix the
signature?
Andreas
> + dc->reset = ics_kvm_reset;
> + k->pre_save = ics_get_kvm_state;
> + k->post_load = ics_set_kvm_state;
> +}
> +
> +static TypeInfo ics_kvm_info = {
static const for all TypeInfos.
> + .name = TYPE_ICS_KVM,
> + .parent = TYPE_ICS,
> + .instance_size = sizeof(ICSState),
> + .class_init = ics_kvm_class_init,
> +};
> +
> +/*
> + * XICS-KVM
> + */
> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
> +{
> + CPUState *cs;
> + ICPState *ss;
> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
> + OBJECT(icp), TYPE_XICS_KVM);
> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
Are you intentionally accessing that class by name rather than using
XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
> +
> + if (!icpkvm) {
> + return;
> + }
> +
> + cs = CPU(cpu);
> + ss = &icp->ss[cs->cpu_index];
> +
> + assert(cs->cpu_index < icp->nr_servers);
> + if (icpkvm->kernel_xics_fd == -1) {
> + abort();
> + }
> +
> + if (icpkvm->kernel_xics_fd != -1) {
> + int ret;
> + struct kvm_enable_cap xics_enable_cap = {
> + .cap = KVM_CAP_IRQ_XICS,
> + .flags = 0,
> + .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
> + };
> +
> + ss->cs = cs;
> +
> + ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
> + if (ret < 0) {
> + fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
> + cs->cpu_index, strerror(errno));
error_report() in place of all fprintf(stderr, ...)? (without \n then)
> + exit(1);
> + }
> + }
> +
> + /* Call emulated XICS implementation for consistency */
> + assert(xics_info);
If you want to go safe, you could add assert(xics_info->cpu_setup).
> + xics_info->cpu_setup(icp, cpu);
> +}
> +
> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> + uint32_t token,
> + uint32_t nargs, target_ulong args,
> + uint32_t nret, target_ulong rets)
> +{
> + fprintf(stderr, "pseries: %s must never be called for in-kernel XICS\n",
> + __func__);
> +}
> +
> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
> +{
> + XICSStateKVM *icpkvm = XICS_KVM(dev);
> + XICSState *icp = XICS(dev);
> + ICSState *ics;
> + QemuOptsList *list = qemu_find_opts("machine");
> + int i, rc;
> + struct kvm_create_device xics_create_device = {
> + .type = KVM_DEV_TYPE_XICS,
> + .flags = 0,
> + };
> +
> + if (!kvm_enabled()) {
> + error_setg(errp, "KVM must be enabled for in-kernel XICS");
> + goto fail;
> + }
> +
> + if (QTAILQ_EMPTY(&list->head) ||
> + !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> + "kernel_irqchip", true) ||
Is it safe to take a value from the first element in the list? Shouldn't
merging -machine spapr,accel=kvm -machine kernel_irqchip=on be taken
into account?
> + !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
> + error_setg(errp, "KVM must be enabled for in-kernel XICS");
> + return;
> + }
> +
> + icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
> + icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
> + icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
> + icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
> +
> + rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
> + goto fail;
> + }
> +
> + rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
> + goto fail;
> + }
> +
> + rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
> + goto fail;
> + }
> +
> + rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
> + if (rc < 0) {
> + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off");
> + goto fail;
> + }
> +
> + /* Create the kernel ICP */
> + rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
> + if (rc < 0) {
> + error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
> + goto fail;
> + }
> +
> + icpkvm->kernel_xics_fd = xics_create_device.fd;
> +
> + icp->ics = ICS(object_new(TYPE_ICS_KVM));
> + ics = icp->ics;
> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
> +
> + ics->nr_irqs = icp->nr_irqs;
> + ics->offset = XICS_IRQ_BASE;
> + ics->icp = icp;
> + qdev_init_nofail(DEVICE(ics));
Since this is in a realize function, please use
object_property_set_bool(), which gives you access to the Error - to
catch it you can use a local Error *err variable.
> +
> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
> + for (i = 0; i < icp->nr_servers; i++) {
> + char buffer[32];
> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
> + qdev_init_nofail(DEVICE(&icp->ss[i]));
object_property_set_bool()
Where does icp->nr_servers come from? Is there no way to split this into
instance_init and realize?
> + }
> + return;
> +
> +fail:
> + kvmppc_define_rtas_token(0, "ibm,set-xive");
> + kvmppc_define_rtas_token(0, "ibm,get-xive");
> + kvmppc_define_rtas_token(0, "ibm,int-on");
> + kvmppc_define_rtas_token(0, "ibm,int-off");
> + return;
> +}
> +
> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + XICSStateClass *k = XICS_CLASS(oc);
xsc?
> +
> + dc->realize = xics_kvm_realize;
> + k->cpu_setup = xics_kvm_cpu_setup;
> +}
> +
> +static const TypeInfo xics_kvm_info = {
> + .name = TYPE_XICS_KVM,
> + .parent = TYPE_XICS,
> + .instance_size = sizeof(XICSStateKVM),
> + .class_init = xics_kvm_class_init,
> +};
> +
> +static void xics_kvm_register_types(void)
> +{
> + type_register_static(&xics_kvm_info);
> + type_register_static(&ics_kvm_info);
> + type_register_static(&icp_kvm_info);
> +}
> +
> +type_init(xics_kvm_register_types)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 432f0d2..84433ee 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -148,7 +148,31 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
> {
> XICSState *icp = NULL;
>
> - icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> + if (kvm_enabled()) {
> + bool irqchip_allowed = true, irqchip_required = false;
> + QemuOptsList *list = qemu_find_opts("machine");
> +
> + if (!QTAILQ_EMPTY(&list->head)) {
> + irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> + "kernel_irqchip", true);
> + irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
> + "kernel_irqchip", false);
> + }
Again, don't we have APIs to help with opts access?
> +
> + if (irqchip_allowed) {
> + icp = try_create_xics(TYPE_XICS_KVM, nr_servers, nr_irqs);
> + }
> +
> + if (irqchip_required && !icp) {
> + perror("iFailed to create in-kernel XICS\n");
"Failed to ..."?
> + abort();
> + }
> + }
> +
> + if (!icp) {
> + icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
> + }
> +
> if (!icp) {
> perror("Failed to create XICS\n");
> abort();
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 90ecaf8..835a3d6 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -32,6 +32,9 @@
> #define TYPE_XICS "xics"
> #define XICS(obj) OBJECT_CHECK(XICSState, (obj), TYPE_XICS)
>
> +#define TYPE_XICS_KVM "xics-kvm"
> +#define XICS_KVM(obj) OBJECT_CHECK(XICSStateKVM, (obj), TYPE_XICS_KVM)
TYPE_KVM_XICS, KVM_XICS() please - from the specific to base.
> +
> #define XICS_CLASS(klass) \
> OBJECT_CLASS_CHECK(XICSStateClass, (klass), TYPE_XICS)
> #define XICS_GET_CLASS(obj) \
> @@ -73,6 +76,9 @@ struct XICSState {
> #define TYPE_ICP "icp"
> #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>
> +#define TYPE_ICP_KVM "icp-kvm"
> +#define ICP_KVM(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP_KVM)
> +
> #define ICP_CLASS(klass) \
> OBJECT_CLASS_CHECK(ICPStateClass, (klass), TYPE_ICP)
> #define ICP_GET_CLASS(obj) \
> @@ -89,6 +95,7 @@ struct ICPState {
> /*< private >*/
> DeviceState parent_obj;
> /*< public >*/
> + CPUState *cs;
> uint32_t xirr;
> uint8_t pending_priority;
> uint8_t mfrr;
> @@ -98,6 +105,9 @@ struct ICPState {
> #define TYPE_ICS "ics"
> #define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>
> +#define TYPE_ICS_KVM "icskvm"
> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
> +
> #define ICS_CLASS(klass) \
> OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
> #define ICS_GET_CLASS(obj) \
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-07-31 19:52 ` Andreas Färber
@ 2013-07-31 20:47 ` Peter Maydell
2013-08-01 0:14 ` Alexey Kardashevskiy
1 sibling, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2013-07-31 20:47 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, Alexey Kardashevskiy, qemu-devel, Alexander Graf,
qemu-ppc, Paul Mackerras, David Gibson
On 31 July 2013 20:52, Andreas Färber <afaerber@suse.de> wrote:
> Hi,
>
> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>> + perror("iFailed to create in-kernel XICS\n");
>
> "Failed to ..."?
Perhaps
#ifdef __APPLE__
perror("iFailed ...");
#else
perror("Failed ...");
#endif
:-)
-- PMM
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-07-31 19:52 ` Andreas Färber
2013-07-31 20:47 ` Peter Maydell
@ 2013-08-01 0:14 ` Alexey Kardashevskiy
2013-08-01 1:29 ` Andreas Färber
1 sibling, 1 reply; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-01 0:14 UTC (permalink / raw)
To: Andreas Färber
Cc: Anthony Liguori, qemu-devel, Alexander Graf, Paul Mackerras,
qemu-ppc, David Gibson
On 08/01/2013 05:52 AM, Andreas Färber wrote:
> Hi,
>
> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>> From: David Gibson <david@gibson.dropbear.id.au>
>>
>> Recent (host) kernels support emulating the PAPR defined "XICS" interrupt
>> controller system within KVM. This patch allows qemu to initialize and
>> configure the in-kernel XICS, and keep its state in sync with qemu's XICS
>> state as necessary.
>>
>> This should give considerable performance improvements. e.g. on a simple
>> IPI ping-pong test between hardware threads, using qemu XICS gives us
>> around 5,000 irqs/second, whereas the in-kernel XICS gives us around
>> 70,000 irqs/s on the same hardware configuration.
>>
>> [Mike Qiu <qiudayu@linux.vnet.ibm.com>: fixed mistype which caused ics_set_kvm_state() to fail]
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> [aik: moved to a separate device, reworked QOM]
>>
>> ---
>> Changes:
>> 2013/07/17:
>> * QOM rework
>>
>> 2013/07/01
>> * fixed VMState names in order to support xics-kvm migration to xics and vice versa
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> Something went wrong here. ;)
Yeah. git format-patch -s gets confused if commit message has parts
separated with '---' :)
> [...]
>> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
>> new file mode 100644
>> index 0000000..04ce1be
>> --- /dev/null
>> +++ b/hw/intc/xics_kvm.c
> [...]
>> +typedef struct XICSStateKVM {
>> + struct XICSState parent;
>
> parent_obj to align with most new devices?
>
>> +
>> + uint32_t set_xive_token;
>> + uint32_t get_xive_token;
>> + uint32_t int_off_token;
>> + uint32_t int_on_token;
>> + int kernel_xics_fd;
>> +} XICSStateKVM;
> [...]
>> +static int ics_kvm_realize(DeviceState *dev)
>> +{
>> + ICSState *ics = ICS(dev);
>> +
>> + ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> + ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>> + ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
>> +
>> + return 0;
>> +}
>> +
>> +static void ics_kvm_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + ICSStateClass *k = ICS_CLASS(klass);
>> +
>> + dc->init = ics_kvm_realize;
>
> This is fishy - probably you want to assign to dc->realize and fix the
> signature?
>
> Andreas
>
>> + dc->reset = ics_kvm_reset;
>> + k->pre_save = ics_get_kvm_state;
>> + k->post_load = ics_set_kvm_state;
>> +}
>> +
>> +static TypeInfo ics_kvm_info = {
>
> static const for all TypeInfos.
>
>> + .name = TYPE_ICS_KVM,
>> + .parent = TYPE_ICS,
>> + .instance_size = sizeof(ICSState),
>> + .class_init = ics_kvm_class_init,
>> +};
>> +
>> +/*
>> + * XICS-KVM
>> + */
>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>> +{
>> + CPUState *cs;
>> + ICPState *ss;
>> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>> + OBJECT(icp), TYPE_XICS_KVM);
>> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
>
> Are you intentionally accessing that class by name rather than using
> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
not XICS, no?
>> +
>> + if (!icpkvm) {
>> + return;
>> + }
>> +
>> + cs = CPU(cpu);
>> + ss = &icp->ss[cs->cpu_index];
>> +
>> + assert(cs->cpu_index < icp->nr_servers);
>> + if (icpkvm->kernel_xics_fd == -1) {
>> + abort();
>> + }
>> +
>> + if (icpkvm->kernel_xics_fd != -1) {
>> + int ret;
>> + struct kvm_enable_cap xics_enable_cap = {
>> + .cap = KVM_CAP_IRQ_XICS,
>> + .flags = 0,
>> + .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
>> + };
>> +
>> + ss->cs = cs;
>> +
>> + ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
>> + if (ret < 0) {
>> + fprintf(stderr, "Unable to connect CPU%d to kernel XICS: %s\n",
>> + cs->cpu_index, strerror(errno));
>
> error_report() in place of all fprintf(stderr, ...)? (without \n then)
>
>> + exit(1);
>> + }
>> + }
>> +
>> + /* Call emulated XICS implementation for consistency */
>> + assert(xics_info);
>
> If you want to go safe, you could add assert(xics_info->cpu_setup).
>
>> + xics_info->cpu_setup(icp, cpu);
>> +}
>> +
>> +static void rtas_dummy(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> + uint32_t token,
>> + uint32_t nargs, target_ulong args,
>> + uint32_t nret, target_ulong rets)
>> +{
>> + fprintf(stderr, "pseries: %s must never be called for in-kernel XICS\n",
>> + __func__);
>> +}
>> +
>> +static void xics_kvm_realize(DeviceState *dev, Error **errp)
>> +{
>> + XICSStateKVM *icpkvm = XICS_KVM(dev);
>> + XICSState *icp = XICS(dev);
>> + ICSState *ics;
>> + QemuOptsList *list = qemu_find_opts("machine");
>> + int i, rc;
>> + struct kvm_create_device xics_create_device = {
>> + .type = KVM_DEV_TYPE_XICS,
>> + .flags = 0,
>> + };
>> +
>> + if (!kvm_enabled()) {
>> + error_setg(errp, "KVM must be enabled for in-kernel XICS");
>> + goto fail;
>> + }
>> +
>> + if (QTAILQ_EMPTY(&list->head) ||
>> + !qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
>> + "kernel_irqchip", true) ||
>
> Is it safe to take a value from the first element in the list? Shouldn't
> merging -machine spapr,accel=kvm -machine kernel_irqchip=on be taken
> into account?
>
>> + !kvm_check_extension(kvm_state, KVM_CAP_IRQ_XICS)) {
>> + error_setg(errp, "KVM must be enabled for in-kernel XICS");
>> + return;
>> + }
>> +
>> + icpkvm->set_xive_token = spapr_rtas_register("ibm,set-xive", rtas_dummy);
>> + icpkvm->get_xive_token = spapr_rtas_register("ibm,get-xive", rtas_dummy);
>> + icpkvm->int_off_token = spapr_rtas_register("ibm,int-off", rtas_dummy);
>> + icpkvm->int_on_token = spapr_rtas_register("ibm,int-on", rtas_dummy);
>> +
>> + rc = kvmppc_define_rtas_token(icpkvm->set_xive_token, "ibm,set-xive");
>> + if (rc < 0) {
>> + error_setg(errp, "kvmppc_define_rtas_token: ibm,set-xive");
>> + goto fail;
>> + }
>> +
>> + rc = kvmppc_define_rtas_token(icpkvm->get_xive_token, "ibm,get-xive");
>> + if (rc < 0) {
>> + error_setg(errp, "kvmppc_define_rtas_token: ibm,get-xive");
>> + goto fail;
>> + }
>> +
>> + rc = kvmppc_define_rtas_token(icpkvm->int_on_token, "ibm,int-on");
>> + if (rc < 0) {
>> + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-on");
>> + goto fail;
>> + }
>> +
>> + rc = kvmppc_define_rtas_token(icpkvm->int_off_token, "ibm,int-off");
>> + if (rc < 0) {
>> + error_setg(errp, "kvmppc_define_rtas_token: ibm,int-off");
>> + goto fail;
>> + }
>> +
>> + /* Create the kernel ICP */
>> + rc = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &xics_create_device);
>> + if (rc < 0) {
>> + error_setg_errno(errp, -rc, "Error on KVM_CREATE_DEVICE for XICS");
>> + goto fail;
>> + }
>> +
>> + icpkvm->kernel_xics_fd = xics_create_device.fd;
>> +
>> + icp->ics = ICS(object_new(TYPE_ICS_KVM));
>> + ics = icp->ics;
>> + object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
>> +
>> + ics->nr_irqs = icp->nr_irqs;
>> + ics->offset = XICS_IRQ_BASE;
>> + ics->icp = icp;
>> + qdev_init_nofail(DEVICE(ics));
>
> Since this is in a realize function, please use
> object_property_set_bool(), which gives you access to the Error - to
> catch it you can use a local Error *err variable.
>
>> +
>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>> + for (i = 0; i < icp->nr_servers; i++) {
>> + char buffer[32];
>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>
> object_property_set_bool()
? Anthony did XICS refactoring recently and that has qdev_init_nofail().
> Where does icp->nr_servers come from?
Via properties in try_create_xics() (hw/ppc/spapr.c).
> Is there no way to split this into
> instance_init and realize?
Why would we want to split?
>> + }
>> + return;
>> +
>> +fail:
>> + kvmppc_define_rtas_token(0, "ibm,set-xive");
>> + kvmppc_define_rtas_token(0, "ibm,get-xive");
>> + kvmppc_define_rtas_token(0, "ibm,int-on");
>> + kvmppc_define_rtas_token(0, "ibm,int-off");
>> + return;
>> +}
>> +
>> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(oc);
>> + XICSStateClass *k = XICS_CLASS(oc);
>
> xsc?
Whyyyy? Many places keep using "k".
>> +
>> + dc->realize = xics_kvm_realize;
>> + k->cpu_setup = xics_kvm_cpu_setup;
>> +}
>> +
>> +static const TypeInfo xics_kvm_info = {
>> + .name = TYPE_XICS_KVM,
>> + .parent = TYPE_XICS,
>> + .instance_size = sizeof(XICSStateKVM),
>> + .class_init = xics_kvm_class_init,
>> +};
>> +
>> +static void xics_kvm_register_types(void)
>> +{
>> + type_register_static(&xics_kvm_info);
>> + type_register_static(&ics_kvm_info);
>> + type_register_static(&icp_kvm_info);
>> +}
>> +
>> +type_init(xics_kvm_register_types)
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 432f0d2..84433ee 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -148,7 +148,31 @@ static XICSState *xics_system_init(int nr_servers, int nr_irqs)
>> {
>> XICSState *icp = NULL;
>>
>> - icp = try_create_xics(TYPE_XICS, nr_servers, nr_irqs);
>> + if (kvm_enabled()) {
>> + bool irqchip_allowed = true, irqchip_required = false;
>> + QemuOptsList *list = qemu_find_opts("machine");
>> +
>> + if (!QTAILQ_EMPTY(&list->head)) {
>> + irqchip_allowed = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
>> + "kernel_irqchip", true);
>> + irqchip_required = qemu_opt_get_bool(QTAILQ_FIRST(&list->head),
>> + "kernel_irqchip", false);
>> + }
>
> Again, don't we have APIs to help with opts access?
Hm, I copied this from openpic but it went further since then :)
>> +
>> + if (irqchip_allowed) {
>> + icp = try_create_xics(TYPE_XICS_KVM, nr_servers, nr_irqs);
>> + }
>> +
>> + if (irqchip_required && !icp) {
>> + perror("iFailed to create in-kernel XICS\n");
>
> "Failed to ..."?
vim...
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-01 0:14 ` Alexey Kardashevskiy
@ 2013-08-01 1:29 ` Andreas Färber
2013-08-01 2:08 ` Alexey Kardashevskiy
2013-08-02 14:57 ` Alexey Kardashevskiy
0 siblings, 2 replies; 18+ messages in thread
From: Andreas Färber @ 2013-08-01 1:29 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>> +/*
>>> + * XICS-KVM
>>> + */
>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>> +{
>>> + CPUState *cs;
>>> + ICPState *ss;
>>> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>>> + OBJECT(icp), TYPE_XICS_KVM);
>>> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
>>
>> Are you intentionally accessing that class by name rather than using
>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
>
>
> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
> not XICS, no?
OK, then I'll CC you on my upcoming virtio v2 series that introduces a
more comprehensable macro for this purpose: I would/will recommend to
use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could move
your current inline implementation - to make more obvious that it's not
a mistake.
>>> +
>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>> + for (i = 0; i < icp->nr_servers; i++) {
>>> + char buffer[32];
>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>>
>> object_property_set_bool()
>
>
> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
Nobody is perfect. ;)
The point is, this is an object, and in realize you shouldn't abort but
set errp and leave error printing and handling to your caller. The QOM
API as opposed to qdev works with an Error object that you can
error_propagate() to your caller.
(Also using qdev_* for something that is new-style QOM is ugly IMO.)
>> Where does icp->nr_servers come from?
>
> Via properties in try_create_xics() (hw/ppc/spapr.c).
Sounds tricky... Peter introduced static array properties for a similar
purpose, I believe. Don't know if that would help here.
>
>> Is there no way to split this into
>> instance_init and realize?
>
>
> Why would we want to split?
Because realize is too late to create new devices: With our targetted
late, recursive realization model it will not be possible to see and
modify such objects from management interface - only before realize.
I even have a patch on the list that would assert when that happens
during final recursive realization.
>>> + }
>>> + return;
>>> +
>>> +fail:
>>> + kvmppc_define_rtas_token(0, "ibm,set-xive");
>>> + kvmppc_define_rtas_token(0, "ibm,get-xive");
>>> + kvmppc_define_rtas_token(0, "ibm,int-on");
>>> + kvmppc_define_rtas_token(0, "ibm,int-off");
>>> + return;
>>> +}
>>> +
>>> +static void xics_kvm_class_init(ObjectClass *oc, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(oc);
>>> + XICSStateClass *k = XICS_CLASS(oc);
>>
>> xsc?
>
>
> Whyyyy? Many places keep using "k".
Many places also still use "klass" and you took the new "oc". ;)
There's no "k" in XICSStateClass, and we adopted using the initial
letters of the name components, like dc for DeviceClass and oc for
ObjectClass above, just like we sort variables from abstract to
concrete. That way, when the type hierarchy gets changed you can more
naturally add variables.
It's not a requirement, but there's no real reason to call it "k"
either, since "c" unlike "class" is not a reserved word in C++. :)
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-01 1:29 ` Andreas Färber
@ 2013-08-01 2:08 ` Alexey Kardashevskiy
2013-08-01 3:07 ` Andreas Färber
2013-08-02 14:57 ` Alexey Kardashevskiy
1 sibling, 1 reply; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-01 2:08 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
On 08/01/2013 11:29 AM, Andreas Färber wrote:
> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>>> +/*
>>>> + * XICS-KVM
>>>> + */
>>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>>> +{
>>>> + CPUState *cs;
>>>> + ICPState *ss;
>>>> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>>>> + OBJECT(icp), TYPE_XICS_KVM);
>>>> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
>>>
>>> Are you intentionally accessing that class by name rather than using
>>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
>>
>>
>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
>> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
>> not XICS, no?
>
> OK, then I'll CC you on my upcoming virtio v2 series that introduces a
> more comprehensable macro for this purpose: I would/will recommend to
> use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could move
> your current inline implementation - to make more obvious that it's not
> a mistake.
Oh. So. This has to wait till that virtio thing gets to upstream. Correct?
>>>> +
>>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>> + for (i = 0; i < icp->nr_servers; i++) {
>>>> + char buffer[32];
>>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>>>
>>> object_property_set_bool()
>>
>>
>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
>
> Nobody is perfect. ;)
That's ok, my question is more about whether I should use set_bool here and
leave emulated XICS as is or you expect me to fix emulated XICS as well and
post an additional patch or what?
> The point is, this is an object, and in realize you shouldn't abort but
> set errp and leave error printing and handling to your caller. The QOM
> API as opposed to qdev works with an Error object that you can
> error_propagate() to your caller.
>
> (Also using qdev_* for something that is new-style QOM is ugly IMO.)
>
>>> Where does icp->nr_servers come from?
>>
>> Via properties in try_create_xics() (hw/ppc/spapr.c).
>
> Sounds tricky... Peter introduced static array properties for a similar
> purpose, I believe. Don't know if that would help here.
>
>>
>>> Is there no way to split this into
>>> instance_init and realize?
>>
>>
>> Why would we want to split?
>
> Because realize is too late to create new devices: With our targetted
> late, recursive realization model it will not be possible to see and
> modify such objects from management interface - only before realize.
>
> I even have a patch on the list that would assert when that happens
> during final recursive realization.
So most this stuff has to go to instance_init and since there is no way to
prevent parent's instance_init from being called, you are basically forcing
me to introduce an abstract XICS class and inherit emulated XICS and KVM
XICS from it. Besides that, I do not any use of it. Is that correct?
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-01 2:08 ` Alexey Kardashevskiy
@ 2013-08-01 3:07 ` Andreas Färber
2013-08-01 3:22 ` Alexey Kardashevskiy
0 siblings, 1 reply; 18+ messages in thread
From: Andreas Färber @ 2013-08-01 3:07 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
Am 01.08.2013 04:08, schrieb Alexey Kardashevskiy:
> On 08/01/2013 11:29 AM, Andreas Färber wrote:
>> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>>>> +/*
>>>>> + * XICS-KVM
>>>>> + */
>>>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>>>> +{
>>>>> + CPUState *cs;
>>>>> + ICPState *ss;
>>>>> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>>>>> + OBJECT(icp), TYPE_XICS_KVM);
>>>>> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
>>>>
>>>> Are you intentionally accessing that class by name rather than using
>>>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
>>>
>>>
>>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
>>> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
>>> not XICS, no?
>>
>> OK, then I'll CC you on my upcoming virtio v2 series that introduces a
>> more comprehensable macro for this purpose: I would/will recommend to
>> use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could move
>> your current inline implementation - to make more obvious that it's not
>> a mistake.
>
> Oh. So. This has to wait till that virtio thing gets to upstream. Correct?
Not quite, there is no dependency on virtio.
a) You could do
#define KVM_XICS_GET_PARENT_CLASS(obj) \
object_class_by_name(TYPE_KVM_XICS)
for now, where you do #define TYPE_KVM_XICS etc.
Note that this is a proposal and not a rule. So far we agreed that
adding classes with fields was not working well for variable depths of
hierarchy and that open-coding the access was not ideal either. Nothing
more, nothing less.
b) You could cherry-pick just http://patchwork.ozlabs.org/patch/263863/
to update the implementation of a), but since that has not yet been
acked I would advise against doing that immediately.
But upstream is in freeze for the next two weeks anyway, so no need to rush.
c) You should participate in the review of Peter's and my proposals
rather than silently inventing your own solution. :)
Advantage of our approaches is hardcoding the current type rather than
the parent's outside of TypeInfo.
>>>>> +
>>>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>>> + for (i = 0; i < icp->nr_servers; i++) {
>>>>> + char buffer[32];
>>>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>>>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>>>>
>>>> object_property_set_bool()
>>>
>>>
>>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
>>
>> Nobody is perfect. ;)
>
> That's ok, my question is more about whether I should use set_bool here and
> leave emulated XICS as is or you expect me to fix emulated XICS as well and
> post an additional patch or what?
If that is so then yes, cleaning up your existing emulation in a patch
before this one would be a good idea.
>>>> Is there no way to split this into
>>>> instance_init and realize?
>>>
>>> Why would we want to split?
>>
>> Because realize is too late to create new devices: With our targetted
>> late, recursive realization model it will not be possible to see and
>> modify such objects from management interface - only before realize.
>>
>> I even have a patch on the list that would assert when that happens
>> during final recursive realization.
>
>
> So most this stuff has to go to instance_init and since there is no way to
> prevent parent's instance_init from being called, you are basically forcing
> me to introduce an abstract XICS class and inherit emulated XICS and KVM
> XICS from it. Besides that, I do not any use of it. Is that correct?
Sorry, I don't follow. x86 and arm do use an abstract base class, e500
doesn't iirc. But whether instance_init or realize, the parent's
implementation will/should be called.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-01 3:07 ` Andreas Färber
@ 2013-08-01 3:22 ` Alexey Kardashevskiy
2013-08-06 14:14 ` Andreas Färber
0 siblings, 1 reply; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-01 3:22 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
On 08/01/2013 01:07 PM, Andreas Färber wrote:
> Am 01.08.2013 04:08, schrieb Alexey Kardashevskiy:
>> On 08/01/2013 11:29 AM, Andreas Färber wrote:
>>> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>>>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>>>>> +/*
>>>>>> + * XICS-KVM
>>>>>> + */
>>>>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>>>>> +{
>>>>>> + CPUState *cs;
>>>>>> + ICPState *ss;
>>>>>> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>>>>>> + OBJECT(icp), TYPE_XICS_KVM);
>>>>>> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
>>>>>
>>>>> Are you intentionally accessing that class by name rather than using
>>>>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
>>>>
>>>>
>>>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
>>>> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
>>>> not XICS, no?
>>>
>>> OK, then I'll CC you on my upcoming virtio v2 series that introduces a
>>> more comprehensable macro for this purpose: I would/will recommend to
>>> use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could move
>>> your current inline implementation - to make more obvious that it's not
>>> a mistake.
>>
>> Oh. So. This has to wait till that virtio thing gets to upstream. Correct?
>
> Not quite, there is no dependency on virtio.
>
> a) You could do
> #define KVM_XICS_GET_PARENT_CLASS(obj) \
> object_class_by_name(TYPE_KVM_XICS)
TYPE_KVM_XICS or TYPE_XICS?
> for now, where you do #define TYPE_KVM_XICS etc.
> Note that this is a proposal and not a rule. So far we agreed that
> adding classes with fields was not working well for variable depths of
> hierarchy and that open-coding the access was not ideal either. Nothing
> more, nothing less.
>
> b) You could cherry-pick just http://patchwork.ozlabs.org/patch/263863/
> to update the implementation of a), but since that has not yet been
> acked I would advise against doing that immediately.
> But upstream is in freeze for the next two weeks anyway, so no need to rush.
>
> c) You should participate in the review of Peter's and my proposals
> rather than silently inventing your own solution. :)
> Advantage of our approaches is hardcoding the current type rather than
> the parent's outside of TypeInfo.
I do not understand most of the stuff you do for QOM. I mean I know
C++/RTTI pretty good and even worse - I still remember M$ DCOM/OLE2 but I
do not know to what extent you want to implement that C++ in QEMU :)
>>>>>> +
>>>>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>>>> + for (i = 0; i < icp->nr_servers; i++) {
>>>>>> + char buffer[32];
>>>>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>>>>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>>>>>
>>>>> object_property_set_bool()
>>>>
>>>>
>>>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
>>>
>>> Nobody is perfect. ;)
>>
>> That's ok, my question is more about whether I should use set_bool here and
>> leave emulated XICS as is or you expect me to fix emulated XICS as well and
>> post an additional patch or what?
>
> If that is so then yes, cleaning up your existing emulation in a patch
> before this one would be a good idea.
>
>>>>> Is there no way to split this into
>>>>> instance_init and realize?
>>>>
>>>> Why would we want to split?
>>>
>>> Because realize is too late to create new devices: With our targetted
>>> late, recursive realization model it will not be possible to see and
>>> modify such objects from management interface - only before realize.
>>>
>>> I even have a patch on the list that would assert when that happens
>>> during final recursive realization.
>>
>>
>> So most this stuff has to go to instance_init and since there is no way to
>> prevent parent's instance_init from being called, you are basically forcing
>> me to introduce an abstract XICS class and inherit emulated XICS and KVM
>> XICS from it. Besides that, I do not any use of it. Is that correct?
>
> Sorry, I don't follow. x86 and arm do use an abstract base class, e500
> doesn't iirc. But whether instance_init or realize, the parent's
> implementation will/should be called.
Openpic does not but its init() does not do much and openpic does not have
child devices but XICS does now (after Anthony's rework).
Unlike instance_init(), parent's realize() is not called automatically,
this was the main reason why I put everything into realize().
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-01 3:22 ` Alexey Kardashevskiy
@ 2013-08-06 14:14 ` Andreas Färber
0 siblings, 0 replies; 18+ messages in thread
From: Andreas Färber @ 2013-08-06 14:14 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
Am 01.08.2013 05:22, schrieb Alexey Kardashevskiy:
> On 08/01/2013 01:07 PM, Andreas Färber wrote:
>> Am 01.08.2013 04:08, schrieb Alexey Kardashevskiy:
>>> On 08/01/2013 11:29 AM, Andreas Färber wrote:
>>>> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>>>>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>>>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>>>>>> +/*
>>>>>>> + * XICS-KVM
>>>>>>> + */
>>>>>>> +static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
>>>>>>> +{
>>>>>>> + CPUState *cs;
>>>>>>> + ICPState *ss;
>>>>>>> + XICSStateKVM *icpkvm = (XICSStateKVM *) object_dynamic_cast(
>>>>>>> + OBJECT(icp), TYPE_XICS_KVM);
>>>>>>> + XICSStateClass *xics_info = XICS_CLASS(object_class_by_name(TYPE_XICS));
>>>>>>
>>>>>> Are you intentionally accessing that class by name rather than using
>>>>>> XICS_GET_CLASS(icp), which allows the KVM variant to overwrite things?
>>>>>
>>>>>
>>>>> This is KVM's CPU_setup(). I want to call non-KVM CPU_setup afterwards,
>>>>> i.e. "call parent method". XICS_GET_CLASS will return XICS_KVM class but
>>>>> not XICS, no?
>>>>
>>>> OK, then I'll CC you on my upcoming virtio v2 series that introduces a
>>>> more comprehensable macro for this purpose: I would/will recommend to
>>>> use a local macro KVM_XICS_GET_PARENT_CLASS(obj) - where you could move
>>>> your current inline implementation - to make more obvious that it's not
>>>> a mistake.
>>>
>>> Oh. So. This has to wait till that virtio thing gets to upstream. Correct?
>>
>> Not quite, there is no dependency on virtio.
>>
>> a) You could do
>> #define KVM_XICS_GET_PARENT_CLASS(obj) \
>> object_class_by_name(TYPE_KVM_XICS)
>
> TYPE_KVM_XICS or TYPE_XICS?
Sorry for late reply: When using object_class_by_name() directly, then
TYPE_XICS (or now TYPE_COMMON_XICS); when using object_get_parent() or
the wrapping OBJECT_GET_PARENT_CLASS() then TYPE_KVM_XICS, i.e. the QOM
type to which the C function belongs.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-01 1:29 ` Andreas Färber
2013-08-01 2:08 ` Alexey Kardashevskiy
@ 2013-08-02 14:57 ` Alexey Kardashevskiy
2013-08-03 2:45 ` Alexey Kardashevskiy
1 sibling, 1 reply; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-02 14:57 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
On 08/01/2013 11:29 AM, Andreas Färber wrote:
> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>>>> +
>>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>> + for (i = 0; i < icp->nr_servers; i++) {
>>>> + char buffer[32];
>>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>>>
>>> object_property_set_bool()
>>
>>
>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
>
> Nobody is perfect. ;)
>
> The point is, this is an object, and in realize you shouldn't abort but
> set errp and leave error printing and handling to your caller. The QOM
> API as opposed to qdev works with an Error object that you can
> error_propagate() to your caller.
>
> (Also using qdev_* for something that is new-style QOM is ugly IMO.)
>
>>> Where does icp->nr_servers come from?
>>
>> Via properties in try_create_xics() (hw/ppc/spapr.c).
>
> Sounds tricky... Peter introduced static array properties for a similar
> purpose, I believe. Don't know if that would help here.
Peter who? For what purpose? Could you please point to it? Cannot find it
anywhere.
>>
>>> Is there no way to split this into
>>> instance_init and realize?
>>
>>
>> Why would we want to split?
>
> Because realize is too late to create new devices: With our targetted
> late, recursive realization model it will not be possible to see and
> modify such objects from management interface - only before realize.
Oh. I lost you here. I need to create XICS with some child ICP devices (now
they are devices). The number of child devices comes from spapr.c via
properties. Now you are saying it is too late to create devices (even so
lame ones) in realize(). So I have to put the creation in instance_init().
Could you please point now I can create device-and-pass-propetries in one
shot? Thanks!
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller
2013-08-02 14:57 ` Alexey Kardashevskiy
@ 2013-08-03 2:45 ` Alexey Kardashevskiy
0 siblings, 0 replies; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-03 2:45 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Maydell, Anthony Liguori, qemu-devel, Alexander Graf,
Paul Mackerras, qemu-ppc, David Gibson
On 08/03/2013 12:57 AM, Alexey Kardashevskiy wrote:
> On 08/01/2013 11:29 AM, Andreas Färber wrote:
>> Am 01.08.2013 02:14, schrieb Alexey Kardashevskiy:
>>> On 08/01/2013 05:52 AM, Andreas Färber wrote:
>>>> Am 17.07.2013 08:37, schrieb Alexey Kardashevskiy:
>
>>>>> +
>>>>> + icp->ss = g_malloc0(icp->nr_servers*sizeof(ICPState));
>>>>> + for (i = 0; i < icp->nr_servers; i++) {
>>>>> + char buffer[32];
>>>>> + object_initialize(&icp->ss[i], TYPE_ICP_KVM);
>>>>> + snprintf(buffer, sizeof(buffer), "icp[%d]", i);
>>>>> + object_property_add_child(OBJECT(icp), buffer, OBJECT(&icp->ss[i]), NULL);
>>>>> + qdev_init_nofail(DEVICE(&icp->ss[i]));
>>>>
>>>> object_property_set_bool()
>>>
>>>
>>> ? Anthony did XICS refactoring recently and that has qdev_init_nofail().
>>
>> Nobody is perfect. ;)
>>
>> The point is, this is an object, and in realize you shouldn't abort but
>> set errp and leave error printing and handling to your caller. The QOM
>> API as opposed to qdev works with an Error object that you can
>> error_propagate() to your caller.
>>
>> (Also using qdev_* for something that is new-style QOM is ugly IMO.)
>>
>>>> Where does icp->nr_servers come from?
>>>
>>> Via properties in try_create_xics() (hw/ppc/spapr.c).
>>
>> Sounds tricky... Peter introduced static array properties for a similar
>> purpose, I believe. Don't know if that would help here.
>
> Peter who? For what purpose? Could you please point to it? Cannot find it
> anywhere.
>
>
>>>
>>>> Is there no way to split this into
>>>> instance_init and realize?
>>>
>>>
>>> Why would we want to split?
>>
>> Because realize is too late to create new devices: With our targetted
>> late, recursive realization model it will not be possible to see and
>> modify such objects from management interface - only before realize.
>
>
> Oh. I lost you here. I need to create XICS with some child ICP devices (now
> they are devices). The number of child devices comes from spapr.c via
> properties. Now you are saying it is too late to create devices (even so
> lame ones) in realize(). So I have to put the creation in instance_init().
> Could you please point now I can create device-and-pass-propetries in one
> shot? Thanks!
No, I am still missing some bits.
In spapr we try to initialize xics-kvm and if we cannot have it, we switch
to emulated xics.
I.e.:
dev = qdev_create(NULL, type);
qdev_prop_set_uint32(dev, "nr_servers", nr_servers);
qdev_prop_set_uint32(dev, "nr_irqs", nr_irqs);
if (qdev_init(dev) < 0) {
return NULL;
}
qdev_create() calls instance_init() (which cannot fail) and qdev_init()
calls realize() (which can fail and this is how I know that xics-kvm cannot
work).
instance_init() and realize() are called at once, right here. But as I
understand, realize() is not supposed to be called this way, it should be
called by QOM, no?
And about properties - before I create child devices, I need both
properties set. So it looks like I need a third property such as
"realized", call it "real_instance_init" and assign a callback on it as it
is done for realize(). Correct?
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support
2013-07-17 6:37 [Qemu-devel] RFC: [PATCH 0/4] xics: in-kernel support Alexey Kardashevskiy
` (3 preceding siblings ...)
2013-07-17 6:37 ` [Qemu-devel] [PATCH 4/4] xics: Support for in-kernel XICS interrupt controller Alexey Kardashevskiy
@ 2013-07-30 2:29 ` Alexey Kardashevskiy
4 siblings, 0 replies; 18+ messages in thread
From: Alexey Kardashevskiy @ 2013-07-30 2:29 UTC (permalink / raw)
To: Alexey Kardashevskiy, Anthony Liguori
Cc: Alexander Graf, qemu-devel, qemu-ppc, Paul Mackerras,
David Gibson
On 07/17/2013 04:37 PM, Alexey Kardashevskiy wrote:
> This is rework of in-kernel XICS on top of "[PATCH 00/11] pseries: migration
> and QOM support" + compile fix patch + XICS migration fix patch.
>
> Migration from XICS to XICS-KVM and vice versa works.
>
> In this series, XICS-KVM inherits from XICS. I do not really see the point of
> adding one more XICS-common here.
>
> Any comments? Thanks.
Anthony, ping?
>
>
> Alexey Kardashevskiy (2):
> xics: add pre_save/post_load/cpu_setup dispatchers
> xics: rework initialization
>
> David Gibson (2):
> target-ppc: Add helper for KVM_PPC_RTAS_DEFINE_TOKEN
> xics: Support for in-kernel XICS interrupt controller
>
> default-configs/ppc64-softmmu.mak | 1 +
> hw/intc/Makefile.objs | 1 +
> hw/intc/xics.c | 96 ++++++--
> hw/intc/xics_kvm.c | 469 ++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr.c | 30 ++-
> include/hw/ppc/xics.h | 56 ++++-
> target-ppc/kvm.c | 14 ++
> target-ppc/kvm_ppc.h | 7 +
> 8 files changed, 646 insertions(+), 28 deletions(-)
> create mode 100644 hw/intc/xics_kvm.c
>
--
Alexey
^ permalink raw reply [flat|nested] 18+ messages in thread