diff mbox series

[PATCH-for-8.2,v4,09/10] hw/char/pl011: Add transmit FIFO to PL011State

Message ID 20231109192814.95977-10-philmd@linaro.org
State Superseded
Headers show
Series hw/char/pl011: Implement TX (async) FIFO to avoid blocking the main loop | expand

Commit Message

Philippe Mathieu-Daudé Nov. 9, 2023, 7:28 p.m. UTC 
In order to make the next commit easier to review,
introduce the transmit FIFO, but do not yet use it.

When migrating from new to old VM:
- if the fifo is empty, migration will still work because
   of the subsection.
- if the fifo is not empty, the subsection will be ignored,
  with the only consequence being that some characters will
  be dropped.

Uninline pl011_reset_tx_fifo().

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/char/pl011.h |  2 ++
 hw/char/pl011.c         | 37 +++++++++++++++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 2 deletions(-)

Comments

Richard Henderson Nov. 9, 2023, 11:24 p.m. UTC | #1 
On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
> @@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
>       }
>   };
>   
> +static bool pl011_xmit_fifo_state_needed(void *opaque)
> +{
> +    PL011State* s = opaque;
> +
> +    return !fifo8_is_empty(&s->xmit_fifo);
> +}
> +
> +static const VMStateDescription vmstate_pl011_xmit_fifo = {
> +    .name = "pl011/xmit_fifo",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = pl011_xmit_fifo_state_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_FIFO8(xmit_fifo, PL011State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static int pl011_post_load(void *opaque, int version_id)
>   {
>       PL011State* s = opaque;
> @@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
>       .subsections = (const VMStateDescription * []) {
>           &vmstate_pl011_clock,
>           NULL
> -    }
> +    },
> +    .subsections = (const VMStateDescription * []) {
> +        &vmstate_pl011_xmit_fifo,
> +        NULL
> +    },
>   };

It just occurred to me that you may need a vmstate_pl011 pre_load() to empty the FIFO, 
which will then be filled if and only if the saved vmstate_pl011_xmit_fifo subsection is 
present.

Juan, have I got this correct about how migration would or should handle a missing subsection?


r~
Juan Quintela Nov. 16, 2023, 3:48 p.m. UTC | #2 
Richard Henderson <richard.henderson@linaro.org> wrote:
> On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
>> @@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
>>       }
>>   };
>>   +static bool pl011_xmit_fifo_state_needed(void *opaque)
>> +{
>> +    PL011State* s = opaque;
>> +
>> +    return !fifo8_is_empty(&s->xmit_fifo);
>> +}
>> +
>> +static const VMStateDescription vmstate_pl011_xmit_fifo = {
>> +    .name = "pl011/xmit_fifo",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = pl011_xmit_fifo_state_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_FIFO8(xmit_fifo, PL011State),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static int pl011_post_load(void *opaque, int version_id)
>>   {
>>       PL011State* s = opaque;
>> @@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
>>       .subsections = (const VMStateDescription * []) {
>>           &vmstate_pl011_clock,
>>           NULL
>> -    }
>> +    },
>> +    .subsections = (const VMStateDescription * []) {
>> +        &vmstate_pl011_xmit_fifo,
>> +        NULL
>> +    },
>>   };
>
> It just occurred to me that you may need a vmstate_pl011 pre_load() to
> empty the FIFO, which will then be filled if and only if the saved
> vmstate_pl011_xmit_fifo subsection is present.
>
> Juan, have I got this correct about how migration would or should handle a missing subsection?

I hav'nt looked about how the device is created. But if it is created
with the fifo empty you don't need the pre_load().

I have no idea about this device, but sometimes it just happens that if
the fifo has data, you need to put an irq somewhere or mark it some
place that there is pending job on this device.

Later, Juan.
Philippe Mathieu-Daudé July 17, 2024, 1:34 p.m. UTC | #3 
On 16/11/23 16:48, Juan Quintela wrote:
> Richard Henderson <richard.henderson@linaro.org> wrote:
>> On 11/9/23 11:28, Philippe Mathieu-Daudé wrote:
>>> @@ -436,6 +438,24 @@ static const VMStateDescription vmstate_pl011_clock = {
>>>        }
>>>    };
>>>    +static bool pl011_xmit_fifo_state_needed(void *opaque)
>>> +{
>>> +    PL011State* s = opaque;
>>> +
>>> +    return !fifo8_is_empty(&s->xmit_fifo);
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_pl011_xmit_fifo = {
>>> +    .name = "pl011/xmit_fifo",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .needed = pl011_xmit_fifo_state_needed,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_FIFO8(xmit_fifo, PL011State),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>    static int pl011_post_load(void *opaque, int version_id)
>>>    {
>>>        PL011State* s = opaque;
>>> @@ -487,7 +507,11 @@ static const VMStateDescription vmstate_pl011 = {
>>>        .subsections = (const VMStateDescription * []) {
>>>            &vmstate_pl011_clock,
>>>            NULL
>>> -    }
>>> +    },
>>> +    .subsections = (const VMStateDescription * []) {
>>> +        &vmstate_pl011_xmit_fifo,
>>> +        NULL
>>> +    },
>>>    };
>>
>> It just occurred to me that you may need a vmstate_pl011 pre_load() to
>> empty the FIFO, which will then be filled if and only if the saved
>> vmstate_pl011_xmit_fifo subsection is present.
>>
>> Juan, have I got this correct about how migration would or should handle a missing subsection?
> 
> I hav'nt looked about how the device is created. But if it is created
> with the fifo empty you don't need the pre_load().

This is indeed the case. Thank you Juan!
diff mbox series

Patch

diff --git a/include/hw/char/pl011.h b/include/hw/char/pl011.h
index d853802132..20898f43a6 100644
--- a/include/hw/char/pl011.h
+++ b/include/hw/char/pl011.h
@@ -18,6 +18,7 @@ 
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
 #include "qom/object.h"
+#include "qemu/fifo8.h"
 
 #define TYPE_PL011 "pl011"
 OBJECT_DECLARE_SIMPLE_TYPE(PL011State, PL011)
@@ -53,6 +54,7 @@  struct PL011State {
     Clock *clk;
     bool migrate_clk;
     const unsigned char *id;
+    Fifo8 xmit_fifo;
 };
 
 DeviceState *pl011_create(hwaddr addr, qemu_irq irq, Chardev *chr);
diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 727decd428..f474f56780 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -147,11 +147,13 @@  static inline void pl011_reset_rx_fifo(PL011State *s)
     s->flags |= PL011_FLAG_RXFE;
 }
 
-static inline void pl011_reset_tx_fifo(PL011State *s)
+static void pl011_reset_tx_fifo(PL011State *s)
 {
     /* Reset FIFO flags */
     s->flags &= ~PL011_FLAG_TXFF;
     s->flags |= PL011_FLAG_TXFE;
+
+    fifo8_reset(&s->xmit_fifo);
 }
 
 static void pl011_write_txdata(PL011State *s, uint8_t data)
@@ -436,6 +438,24 @@  static const VMStateDescription vmstate_pl011_clock = {
     }
 };
 
+static bool pl011_xmit_fifo_state_needed(void *opaque)
+{
+    PL011State* s = opaque;
+
+    return !fifo8_is_empty(&s->xmit_fifo);
+}
+
+static const VMStateDescription vmstate_pl011_xmit_fifo = {
+    .name = "pl011/xmit_fifo",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = pl011_xmit_fifo_state_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_FIFO8(xmit_fifo, PL011State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static int pl011_post_load(void *opaque, int version_id)
 {
     PL011State* s = opaque;
@@ -487,7 +507,11 @@  static const VMStateDescription vmstate_pl011 = {
     .subsections = (const VMStateDescription * []) {
         &vmstate_pl011_clock,
         NULL
-    }
+    },
+    .subsections = (const VMStateDescription * []) {
+        &vmstate_pl011_xmit_fifo,
+        NULL
+    },
 };
 
 static Property pl011_properties[] = {
@@ -502,6 +526,7 @@  static void pl011_init(Object *obj)
     PL011State *s = PL011(obj);
     int i;
 
+    fifo8_create(&s->xmit_fifo, PL011_FIFO_DEPTH);
     memory_region_init_io(&s->iomem, OBJECT(s), &pl011_ops, s, "pl011", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
     for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
@@ -514,6 +539,13 @@  static void pl011_init(Object *obj)
     s->id = pl011_id_arm;
 }
 
+static void pl011_finalize(Object *obj)
+{
+    PL011State *s = PL011(obj);
+
+    fifo8_destroy(&s->xmit_fifo);
+}
+
 static void pl011_realize(DeviceState *dev, Error **errp)
 {
     PL011State *s = PL011(dev);
@@ -557,6 +589,7 @@  static const TypeInfo pl011_arm_info = {
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(PL011State),
     .instance_init = pl011_init,
+    .instance_finalize = pl011_finalize,
     .class_init    = pl011_class_init,
 };