diff mbox series

[23/27] hw/core/or-irq: Support more than 16 inputs to an OR gate

Message ID 20180521140402.23318-24-peter.maydell@linaro.org
State Superseded
Headers show
Series iommu: support txattrs, support TCG execution, implement TZ MPC | expand

Commit Message

Peter Maydell May 21, 2018, 2:03 p.m. UTC 
For the IoTKit MPC support, we need to wire together the
interrupt outputs of 17 MPCs; this exceeds the current
value of MAX_OR_LINES. Increase MAX_OR_LINES to 32 (which
should be enough for anyone).

The tricky part is retaining the migration compatibility for
existing OR gates; we add a subsection which is only used
for larger OR gates, and define it such that we can freely
increase MAX_OR_LINES in future (or even move to a dynamically
allocated levels[] array without an upper size limit) without
breaking compatibility.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 include/hw/or-irq.h |  5 ++++-
 hw/core/or-irq.c    | 39 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 3 deletions(-)

-- 
2.17.0

Comments

Paolo Bonzini May 21, 2018, 2:34 p.m. UTC | #1 
On 21/05/2018 16:03, Peter Maydell wrote:
> + * VMState array; devices with more inputs than this need to

> + * migrate the extra lines via a subsection.

> + * The subsection migrates as much of the levels[] array as is needed

> + * (including repeating the first 16 elements), to avoid the awkwardness

> + * of splitting it in two to meet the requirements of VMSTATE_VARRAY_UINT16.

> + */

> +#define OLD_MAX_OR_LINES 16

> +#if MAX_OR_LINES < OLD_MAX_OR_LINES

> +#error MAX_OR_LINES must be at least 16 for migration compatibility

> +#endif

> +

> +static bool vmstate_extras_needed(void *opaque)

> +{

> +    qemu_or_irq *s = OR_IRQ(opaque);

> +

> +    return s->num_lines >= OLD_MAX_OR_LINES;

> +}

> +

> +static const VMStateDescription vmstate_or_irq_extras = {

> +    .name = "or-irq-extras",

> +    .version_id = 1,

> +    .minimum_version_id = 1,

> +    .needed = vmstate_extras_needed,

> +    .fields = (VMStateField[]) {

> +        VMSTATE_VARRAY_UINT16_UNSAFE(levels, qemu_or_irq, num_lines, 0,

> +                                     vmstate_info_bool, bool),

> +        VMSTATE_END_OF_LIST(),

> +    },

> +};

> +



Why do the levels have to be migrated at all?  It should be enough if
the IRQ level is either migrated manually, or restored (e.g. in
post_save callbacks) through other data that is migrated.

Paolo
Peter Maydell May 21, 2018, 3:02 p.m. UTC | #2 
On 21 May 2018 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Why do the levels have to be migrated at all?  It should be enough if

> the IRQ level is either migrated manually, or restored (e.g. in

> post_save callbacks) through other data that is migrated.


This is standard behaviour for devices: they track their
inbound irq/gpio lines, and then that becomes internal state for
them that must be migrated.

If we didn't migrate the input line state, then after a migration
the levels[] array would be all zeroes, and the next time a
connected device signalled a high-to-low transition we'd take
the output line low even if it should not be (because we'd have
forgotten that some other input lines were high).

In a different world, the state would be in the qemu_irq line itself
(in the same way that in hardware signal lines are their own state),
but we can't get there from here.

thanks
-- PMM
Paolo Bonzini May 30, 2018, 4:59 p.m. UTC | #3 
On 21/05/2018 17:02, Peter Maydell wrote:
> On 21 May 2018 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote:

>> Why do the levels have to be migrated at all?  It should be enough if

>> the IRQ level is either migrated manually, or restored (e.g. in

>> post_save callbacks) through other data that is migrated.

> This is standard behaviour for devices: they track their

> inbound irq/gpio lines, and then that becomes internal state for

> them that must be migrated.


But or_irq's input are another device's outbound lines, so tracking them
should not be necessary.  The other device would do it for or_irq.

Paolo

> If we didn't migrate the input line state, then after a migration

> the levels[] array would be all zeroes, and the next time a

> connected device signalled a high-to-low transition we'd take

> the output line low even if it should not be (because we'd have

> forgotten that some other input lines were high).

> 

> In a different world, the state would be in the qemu_irq line itself

> (in the same way that in hardware signal lines are their own state),

> but we can't get there from here.
Peter Maydell May 30, 2018, 5:35 p.m. UTC | #4 
On 30 May 2018 at 17:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 21/05/2018 17:02, Peter Maydell wrote:

>> On 21 May 2018 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>> Why do the levels have to be migrated at all?  It should be enough if

>>> the IRQ level is either migrated manually, or restored (e.g. in

>>> post_save callbacks) through other data that is migrated.

>> This is standard behaviour for devices: they track their

>> inbound irq/gpio lines, and then that becomes internal state for

>> them that must be migrated.

>

> But or_irq's input are another device's outbound lines, so tracking them

> should not be necessary.  The other device would do it for or_irq.


There's no mechanism in qemu_irq for the destination end to ask
the source end about its current value. The information flow
is strictly one-way.

thanks
-- PMM
Paolo Bonzini May 31, 2018, 10:21 a.m. UTC | #5 
On 30/05/2018 19:35, Peter Maydell wrote:
> On 30 May 2018 at 17:59, Paolo Bonzini <pbonzini@redhat.com> wrote:

>> On 21/05/2018 17:02, Peter Maydell wrote:

>>> On 21 May 2018 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>>> Why do the levels have to be migrated at all?  It should be enough if

>>>> the IRQ level is either migrated manually, or restored (e.g. in

>>>> post_save callbacks) through other data that is migrated.

>>> This is standard behaviour for devices: they track their

>>> inbound irq/gpio lines, and then that becomes internal state for

>>> them that must be migrated.

>>

>> But or_irq's input are another device's outbound lines, so tracking them

>> should not be necessary.  The other device would do it for or_irq.

> 

> There's no mechanism in qemu_irq for the destination end to ask

> the source end about its current value. The information flow

> is strictly one-way.


On postload, the source must call qemu_set_irq and that will cause an
update of the or_irq, won't it?

Paolo
Peter Maydell May 31, 2018, 10:50 a.m. UTC | #6 
On 31 May 2018 at 11:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 30/05/2018 19:35, Peter Maydell wrote:

>> On 30 May 2018 at 17:59, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>> On 21/05/2018 17:02, Peter Maydell wrote:

>>>> On 21 May 2018 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>>>> Why do the levels have to be migrated at all?  It should be enough if

>>>>> the IRQ level is either migrated manually, or restored (e.g. in

>>>>> post_save callbacks) through other data that is migrated.

>>>> This is standard behaviour for devices: they track their

>>>> inbound irq/gpio lines, and then that becomes internal state for

>>>> them that must be migrated.

>>>

>>> But or_irq's input are another device's outbound lines, so tracking them

>>> should not be necessary.  The other device would do it for or_irq.

>>

>> There's no mechanism in qemu_irq for the destination end to ask

>> the source end about its current value. The information flow

>> is strictly one-way.

>

> On postload, the source must call qemu_set_irq and that will cause an

> update of the or_irq, won't it?


No, calling qemu_set_irq in postload is a bug. (You don't know
which order the state of the source and destination devices is
restored, so asserting a signal in postload would have different
effects depending on whether the destination had already had
its state restored or not.) The system design relies on each
device keeping track of (and migrating) the state of the input
lines it cares about.

thanks
-- PMM
Paolo Bonzini May 31, 2018, 11:50 a.m. UTC | #7 
On 31/05/2018 12:50, Peter Maydell wrote:
> On 31 May 2018 at 11:21, Paolo Bonzini <pbonzini@redhat.com> wrote:

>> On 30/05/2018 19:35, Peter Maydell wrote:

>>> On 30 May 2018 at 17:59, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>>> On 21/05/2018 17:02, Peter Maydell wrote:

>>>>> On 21 May 2018 at 15:34, Paolo Bonzini <pbonzini@redhat.com> wrote:

>>>>>> Why do the levels have to be migrated at all?  It should be enough if

>>>>>> the IRQ level is either migrated manually, or restored (e.g. in

>>>>>> post_save callbacks) through other data that is migrated.

>>>>> This is standard behaviour for devices: they track their

>>>>> inbound irq/gpio lines, and then that becomes internal state for

>>>>> them that must be migrated.

>>>>

>>>> But or_irq's input are another device's outbound lines, so tracking them

>>>> should not be necessary.  The other device would do it for or_irq.

>>>

>>> There's no mechanism in qemu_irq for the destination end to ask

>>> the source end about its current value. The information flow

>>> is strictly one-way.

>>

>> On postload, the source must call qemu_set_irq and that will cause an

>> update of the or_irq, won't it?

> 

> No, calling qemu_set_irq in postload is a bug. (You don't know

> which order the state of the source and destination devices is

> restored, so asserting a signal in postload would have different

> effects depending on whether the destination had already had

> its state restored or not.)


Hmm, I suppose the x86 world is a bit more "hierarchical" and you cannot
create a qemu_irq loop - and creating the sink always before the source
ensures that migration works fine with post_load.  I'm pretty sure that
there are devices that do that, and an interesting case (for the sake of
this thread) is the IOMMU, which prompted the introduction of
MigrationPriority.  Thanks for the lesson! :)

Maybe we should add your text to docs/devel/migration.rst, under "Device
ordering", because it explicitly mentions interrupt controllers.

Paolo
Peter Maydell May 31, 2018, 11:59 a.m. UTC | #8 
On 31 May 2018 at 12:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 31/05/2018 12:50, Peter Maydell wrote:

>> No, calling qemu_set_irq in postload is a bug. (You don't know

>> which order the state of the source and destination devices is

>> restored, so asserting a signal in postload would have different

>> effects depending on whether the destination had already had

>> its state restored or not.)

>

> Hmm, I suppose the x86 world is a bit more "hierarchical" and you cannot

> create a qemu_irq loop - and creating the sink always before the source

> ensures that migration works fine with post_load.  I'm pretty sure that

> there are devices that do that


If there are then they are broken, because that's not how
qemu_irqs work... (Similarly, you can't assert an irq
in a reset method, because of ordering problems.)

>, and an interesting case (for the sake of

> this thread) is the IOMMU, which prompted the introduction of

> MigrationPriority.  Thanks for the lesson! :)


This sounds like a workaround for a bug in the device implementation.
Devices shouldn't care about which order they're restored in.

thanks
-- PMM
diff mbox series

Patch

diff --git a/include/hw/or-irq.h b/include/hw/or-irq.h
index 3f6fc1b58a..5a31e5a188 100644
--- a/include/hw/or-irq.h
+++ b/include/hw/or-irq.h
@@ -31,7 +31,10 @@ 
 
 #define TYPE_OR_IRQ "or-irq"
 
-#define MAX_OR_LINES      16
+/* This can safely be increased if necessary without breaking
+ * migration compatibility (as long as it remains greater than 15).
+ */
+#define MAX_OR_LINES      32
 
 typedef struct OrIRQState qemu_or_irq;
 
diff --git a/hw/core/or-irq.c b/hw/core/or-irq.c
index f9d76c4641..a86901b673 100644
--- a/hw/core/or-irq.c
+++ b/hw/core/or-irq.c
@@ -66,14 +66,49 @@  static void or_irq_init(Object *obj)
     qdev_init_gpio_out(DEVICE(obj), &s->out_irq, 1);
 }
 
+/* The original version of this device had a fixed 16 entries in its
+ * VMState array; devices with more inputs than this need to
+ * migrate the extra lines via a subsection.
+ * The subsection migrates as much of the levels[] array as is needed
+ * (including repeating the first 16 elements), to avoid the awkwardness
+ * of splitting it in two to meet the requirements of VMSTATE_VARRAY_UINT16.
+ */
+#define OLD_MAX_OR_LINES 16
+#if MAX_OR_LINES < OLD_MAX_OR_LINES
+#error MAX_OR_LINES must be at least 16 for migration compatibility
+#endif
+
+static bool vmstate_extras_needed(void *opaque)
+{
+    qemu_or_irq *s = OR_IRQ(opaque);
+
+    return s->num_lines >= OLD_MAX_OR_LINES;
+}
+
+static const VMStateDescription vmstate_or_irq_extras = {
+    .name = "or-irq-extras",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = vmstate_extras_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_VARRAY_UINT16_UNSAFE(levels, qemu_or_irq, num_lines, 0,
+                                     vmstate_info_bool, bool),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
 static const VMStateDescription vmstate_or_irq = {
     .name = TYPE_OR_IRQ,
     .version_id = 1,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL_ARRAY(levels, qemu_or_irq, MAX_OR_LINES),
+        VMSTATE_BOOL_SUB_ARRAY(levels, qemu_or_irq, 0, OLD_MAX_OR_LINES),
         VMSTATE_END_OF_LIST(),
-    }
+    },
+    .subsections = (const VMStateDescription*[]) {
+        &vmstate_or_irq_extras,
+        NULL
+    },
 };
 
 static Property or_irq_properties[] = {