From: David Gibson <david@gibson.dropbear.id.au>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Greg Kurz <groug@kaod.org>
Subject: Re: [Qemu-devel] [PATCH 1/2] ppc/xics: rework the ICS classes inheritance tree
Date: Mon, 25 Jun 2018 16:27:28 +1000 [thread overview]
Message-ID: <20180625062728.GD22971@umbus.fritz.box> (raw)
In-Reply-To: <20180620102413.5400-2-clg@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 12931 bytes --]
On Wed, Jun 20, 2018 at 12:24:12PM +0200, Cédric Le Goater wrote:
> This moves the common ICS code of the realize and reset handlers of
> the ICS_SIMPLE class under the ICS_BASE class. The vmstate is also
> moved down one class.
>
> The benefits are that the ICS_KVM class can directly inherit from
> ICS_BASE class and not from the intermediate ICS_SIMPLE. It makes the
> class hierarchy much cleaner and removes duplicated code. DeviceRealize
> and DeviceReset handlers are introduce so that parent handlers are
> called from the inheriting classes.
>
> What is left in the top classes is the low level interface to access
> the KVM XICS device in ICS_KVM and the XICS emulating handlers in
> ICS_SIMPLE.
>
> This should not break migration compatibility.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
I like the idea here, but I'm finding it pretty hard to follow the
patch and convince myself its really safe. How difficult would it be
to rework to separate out the change from base calls derived
->realize (and reset), to the more normal device_set_parent whatnot
from the actual consolidate of the classes?
> ---
> include/hw/ppc/xics.h | 4 +-
> hw/intc/xics.c | 166 ++++++++++++++++++++++++++++----------------------
> hw/intc/xics_kvm.c | 47 +++++++-------
> hw/ppc/spapr.c | 2 +-
> 4 files changed, 123 insertions(+), 96 deletions(-)
>
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7d4..adc5f437b118 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -114,7 +114,9 @@ struct PnvICPState {
> struct ICSStateClass {
> DeviceClass parent_class;
>
> - void (*realize)(ICSState *s, Error **errp);
> + DeviceRealize parent_realize;
> + DeviceReset parent_reset;
> +
> void (*pre_save)(ICSState *s);
> int (*post_load)(ICSState *s, int version_id);
> void (*reject)(ICSState *s, uint32_t irq);
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b53..b351262d1db9 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -547,9 +547,61 @@ static void ics_simple_eoi(ICSState *ics, uint32_t nr)
> }
> }
>
> -static void ics_simple_reset(void *dev)
> +static void ics_simple_reset(DeviceState *dev)
> {
> - ICSState *ics = ICS_SIMPLE(dev);
> + ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> +
> + icsc->parent_reset(dev);
> +}
> +
> +static void ics_simple_reset_handler(void *dev)
> +{
> + ics_simple_reset(dev);
> +}
> +
> +static void ics_simple_realize(DeviceState *dev, Error **errp)
> +{
> + ICSState *ics = ICS_BASE(dev);
> + ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
> + Error *local_err = NULL;
> +
> + icsc->parent_realize(dev, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> + ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
> +
> + qemu_register_reset(ics_simple_reset_handler, ics);
> +}
> +
> +static void ics_simple_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + ICSStateClass *isc = ICS_BASE_CLASS(klass);
> +
> + device_class_set_parent_realize(dc, ics_simple_realize,
> + &isc->parent_realize);
> + device_class_set_parent_reset(dc, ics_simple_reset,
> + &isc->parent_reset);
> +
> + isc->reject = ics_simple_reject;
> + isc->resend = ics_simple_resend;
> + isc->eoi = ics_simple_eoi;
> +}
> +
> +static const TypeInfo ics_simple_info = {
> + .name = TYPE_ICS_SIMPLE,
> + .parent = TYPE_ICS_BASE,
> + .instance_size = sizeof(ICSState),
> + .class_init = ics_simple_class_init,
> + .class_size = sizeof(ICSStateClass),
> +};
> +
> +static void ics_base_reset(DeviceState *dev)
> +{
> + ICSState *ics = ICS_BASE(dev);
> int i;
> uint8_t flags[ics->nr_irqs];
>
> @@ -566,7 +618,35 @@ static void ics_simple_reset(void *dev)
> }
> }
>
> -static int ics_simple_dispatch_pre_save(void *opaque)
> +static void ics_base_realize(DeviceState *dev, Error **errp)
> +{
> + ICSState *ics = ICS_BASE(dev);
> + Object *obj;
> + Error *err = NULL;
> +
> + obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> + if (!obj) {
> + error_propagate(errp, err);
> + error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
> + return;
> + }
> + ics->xics = XICS_FABRIC(obj);
> +
> + if (!ics->nr_irqs) {
> + error_setg(errp, "Number of interrupts needs to be greater 0");
> + return;
> + }
> + ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +}
> +
> +static void ics_base_instance_init(Object *obj)
> +{
> + ICSState *ics = ICS_BASE(obj);
> +
> + ics->offset = XICS_IRQ_BASE;
> +}
> +
> +static int ics_base_dispatch_pre_save(void *opaque)
> {
> ICSState *ics = opaque;
> ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
> @@ -578,7 +658,7 @@ static int ics_simple_dispatch_pre_save(void *opaque)
> return 0;
> }
>
> -static int ics_simple_dispatch_post_load(void *opaque, int version_id)
> +static int ics_base_dispatch_post_load(void *opaque, int version_id)
> {
> ICSState *ics = opaque;
> ICSStateClass *info = ICS_BASE_GET_CLASS(ics);
> @@ -590,7 +670,7 @@ static int ics_simple_dispatch_post_load(void *opaque, int version_id)
> return 0;
> }
>
> -static const VMStateDescription vmstate_ics_simple_irq = {
> +static const VMStateDescription vmstate_ics_base_irq = {
> .name = "ics/irq",
> .version_id = 2,
> .minimum_version_id = 1,
> @@ -604,95 +684,36 @@ static const VMStateDescription vmstate_ics_simple_irq = {
> },
> };
>
> -static const VMStateDescription vmstate_ics_simple = {
> +static const VMStateDescription vmstate_ics_base = {
> .name = "ics",
> .version_id = 1,
> .minimum_version_id = 1,
> - .pre_save = ics_simple_dispatch_pre_save,
> - .post_load = ics_simple_dispatch_post_load,
> + .pre_save = ics_base_dispatch_pre_save,
> + .post_load = ics_base_dispatch_post_load,
> .fields = (VMStateField[]) {
> /* Sanity check */
> VMSTATE_UINT32_EQUAL(nr_irqs, ICSState, NULL),
>
> VMSTATE_STRUCT_VARRAY_POINTER_UINT32(irqs, ICSState, nr_irqs,
> - vmstate_ics_simple_irq,
> + vmstate_ics_base_irq,
> ICSIRQState),
> VMSTATE_END_OF_LIST()
> },
> };
>
> -static void ics_simple_initfn(Object *obj)
> -{
> - ICSState *ics = ICS_SIMPLE(obj);
> -
> - ics->offset = XICS_IRQ_BASE;
> -}
> -
> -static void ics_simple_realize(ICSState *ics, Error **errp)
> -{
> - if (!ics->nr_irqs) {
> - error_setg(errp, "Number of interrupts needs to be greater 0");
> - return;
> - }
> - ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> - ics->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs);
> -
> - qemu_register_reset(ics_simple_reset, ics);
> -}
> -
> -static Property ics_simple_properties[] = {
> +static Property ics_base_properties[] = {
> DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> -static void ics_simple_class_init(ObjectClass *klass, void *data)
> -{
> - DeviceClass *dc = DEVICE_CLASS(klass);
> - ICSStateClass *isc = ICS_BASE_CLASS(klass);
> -
> - isc->realize = ics_simple_realize;
> - dc->props = ics_simple_properties;
> - dc->vmsd = &vmstate_ics_simple;
> - isc->reject = ics_simple_reject;
> - isc->resend = ics_simple_resend;
> - isc->eoi = ics_simple_eoi;
> -}
> -
> -static const TypeInfo ics_simple_info = {
> - .name = TYPE_ICS_SIMPLE,
> - .parent = TYPE_ICS_BASE,
> - .instance_size = sizeof(ICSState),
> - .class_init = ics_simple_class_init,
> - .class_size = sizeof(ICSStateClass),
> - .instance_init = ics_simple_initfn,
> -};
> -
> -static void ics_base_realize(DeviceState *dev, Error **errp)
> -{
> - ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
> - ICSState *ics = ICS_BASE(dev);
> - Object *obj;
> - Error *err = NULL;
> -
> - obj = object_property_get_link(OBJECT(dev), ICS_PROP_XICS, &err);
> - if (!obj) {
> - error_propagate(errp, err);
> - error_prepend(errp, "required link '" ICS_PROP_XICS "' not found: ");
> - return;
> - }
> - ics->xics = XICS_FABRIC(obj);
> -
> -
> - if (icsc->realize) {
> - icsc->realize(ics, errp);
> - }
> -}
> -
> static void ics_base_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->realize = ics_base_realize;
> + dc->reset = ics_base_reset;
> + dc->props = ics_base_properties;
> + dc->vmsd = &vmstate_ics_base;
> }
>
> static const TypeInfo ics_base_info = {
> @@ -700,6 +721,7 @@ static const TypeInfo ics_base_info = {
> .parent = TYPE_DEVICE,
> .abstract = true,
> .instance_size = sizeof(ICSState),
> + .instance_init = ics_base_instance_init,
> .class_init = ics_base_class_init,
> .class_size = sizeof(ICSStateClass),
> };
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 8dba2f84e71e..57d0ebbfaa8a 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -304,44 +304,47 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val)
> }
> }
>
> -static void ics_kvm_reset(void *dev)
> +static void ics_kvm_reset(DeviceState *dev)
> {
> - ICSState *ics = ICS_SIMPLE(dev);
> - int i;
> - uint8_t flags[ics->nr_irqs];
> -
> - for (i = 0; i < ics->nr_irqs; i++) {
> - flags[i] = ics->irqs[i].flags;
> - }
> + ICSStateClass *icsc = ICS_BASE_GET_CLASS(dev);
>
> - memset(ics->irqs, 0, sizeof(ICSIRQState) * ics->nr_irqs);
> + icsc->parent_reset(dev);
>
> - for (i = 0; i < ics->nr_irqs; i++) {
> - ics->irqs[i].priority = 0xff;
> - ics->irqs[i].saved_priority = 0xff;
> - ics->irqs[i].flags = flags[i];
> - }
> + ics_set_kvm_state(ICS_KVM(dev), 1);
> +}
>
> - ics_set_kvm_state(ics, 1);
> +static void ics_kvm_reset_handler(void *dev)
> +{
> + ics_kvm_reset(dev);
> }
>
> -static void ics_kvm_realize(ICSState *ics, Error **errp)
> +static void ics_kvm_realize(DeviceState *dev, Error **errp)
> {
> - if (!ics->nr_irqs) {
> - error_setg(errp, "Number of interrupts needs to be greater 0");
> + ICSState *ics = ICS_BASE(dev);
> + ICSStateClass *icsc = ICS_BASE_GET_CLASS(ics);
> + Error *local_err = NULL;
> +
> + icsc->parent_realize(dev, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> return;
> }
> - ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> +
> ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs);
>
> - qemu_register_reset(ics_kvm_reset, ics);
> + qemu_register_reset(ics_kvm_reset_handler, ics);
> }
>
> static void ics_kvm_class_init(ObjectClass *klass, void *data)
> {
> + DeviceClass *dc = DEVICE_CLASS(klass);
> ICSStateClass *icsc = ICS_BASE_CLASS(klass);
>
> - icsc->realize = ics_kvm_realize;
> + device_class_set_parent_realize(dc, ics_kvm_realize,
> + &icsc->parent_realize);
> + device_class_set_parent_reset(dc, ics_kvm_reset,
> + &icsc->parent_reset);
> +
> icsc->pre_save = ics_get_kvm_state;
> icsc->post_load = ics_set_kvm_state;
> icsc->synchronize_state = ics_synchronize_state;
> @@ -349,7 +352,7 @@ static void ics_kvm_class_init(ObjectClass *klass, void *data)
>
> static const TypeInfo ics_kvm_info = {
> .name = TYPE_ICS_KVM,
> - .parent = TYPE_ICS_SIMPLE,
> + .parent = TYPE_ICS_BASE,
> .instance_size = sizeof(ICSState),
> .class_init = ics_kvm_class_init,
> };
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 78186500e917..468539100327 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -136,7 +136,7 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> goto error;
> }
>
> - return ICS_SIMPLE(obj);
> + return ICS_BASE(obj);
>
> error:
> error_propagate(errp, local_err);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2018-06-25 6:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-20 10:24 [Qemu-devel] [PATCH 0/2] ppc/xics: rework the ICS classes inheritance tree Cédric Le Goater
2018-06-20 10:24 ` [Qemu-devel] [PATCH 1/2] " Cédric Le Goater
2018-06-25 6:27 ` David Gibson [this message]
2018-06-25 6:48 ` Cédric Le Goater
2018-06-20 10:24 ` [Qemu-devel] [PATCH 2/2] ppc/xics: introduce ICP DeviceRealize and DeviceReset handlers Cédric Le Goater
2018-06-25 6:58 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180625062728.GD22971@umbus.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=clg@kaod.org \
--cc=groug@kaod.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).