On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
<eiakov...@linux.microsoft.com> wrote:
>
> PL011 can be in either of 2 modes depending guest config: FIFO and
> single register. The last mode could be viewed as a 1-element-deep FIFO.
>
> Current code open-codes a bunch of depth-dependent logic. Refactor FIFO
> depth handling code to isolate calculating current FIFO depth.
>
> One functional (albeit guest-invisible) side-effect of this change is
> that previously we would always increment s->read_pos in UARTDR read
> handler even if FIFO was disabled, now we are limiting read_pos to not
> exceed FIFO depth (read_pos itself is reset to 0 if user disables FIFO).
>
> Signed-off-by: Evgeny Iakovlev <eiakov...@linux.microsoft.com>
> ---
> hw/char/pl011.c | 25 +++++++++++++------------
> include/hw/char/pl011.h | 5 ++++-
> 2 files changed, 17 insertions(+), 13 deletions(-)
Looking at this again, I realised that there's a subtle point
here about migration compatibility. If we do a VM migration
from an older version of QEMU without this change to a newer
version that does have this change, the incoming migration state
might indicate that we have FIFOs disabled, and there's a character
in read_fifo[] that isn't in array element 0 (because the old
code doesn't put it there). I think this works out OK because
the codepath in the UARTDR read-from-FIFO will first read the
character from read_fifo[read_pos], which will be the non-zero
read_pos as set by the old QEMU, before constraining it to be
0 when it does the advance of read_pos; and the pl011_put_fifo
code doesn't care about the actual value of read_pos.
But this is kind of tricky to reason about, and fragile to
future changes in the code, so I feel like it would be better
to have a migration post_load function that sanitizes the
incoming state to enforce the invariant assumed by the new code, i.e.
if (pl011_fifo_depth(s) == 1 && s->read_count > 0 && s->read_pos > 0) {
/*
* Older versions of QEMU didn't ensure that the single
* character in the FIFO in FIFO-disabled mode is in
* element 0 of the array; convert to follow the current
* code's assumptions.
*/
s->read_fifo[0] = s->read_fifo[s->read_pos];
s->read_pos = 0;
}
If we're putting in a post-load function we can also sanitize
the incoming migration stream to fail the migration on bogus
(possibly malicious) data like read_pos > ARRAY_SIZE(read_fifo)
or read_count > fifo depth.
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index c076813423..329cc6926d 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -81,6 +81,12 @@ static void pl011_update(PL011State *s)
> }
> }
>
> +static inline unsigned pl011_get_fifo_depth(PL011State *s)
> +{
> + /* Note: FIFO depth is expected to be power-of-2 */
> + return s->lcr & 0x10 ? PL011_FIFO_DEPTH : 1;
> +}
> +
> static uint64_t pl011_read(void *opaque, hwaddr offset,
> unsigned size)
> {
> @@ -94,8 +100,7 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
> c = s->read_fifo[s->read_pos];
> if (s->read_count > 0) {
> s->read_count--;
> - if (++s->read_pos == 16)
> - s->read_pos = 0;
> + s->read_pos = (s->read_pos + 1) & (pl011_get_fifo_depth(s) - 1);
> }
> if (s->read_count == 0) {
> s->flags |= PL011_FLAG_RXFE;
> @@ -273,11 +278,7 @@ static int pl011_can_receive(void *opaque)
> PL011State *s = (PL011State *)opaque;
> int r;
>
> - if (s->lcr & 0x10) {
> - r = s->read_count < 16;
> - } else {
> - r = s->read_count < 1;
> - }
> + r = s->read_count < pl011_get_fifo_depth(s);
> trace_pl011_can_receive(s->lcr, s->read_count, r);
> return r;
> }
> @@ -286,15 +287,15 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
> {
> PL011State *s = (PL011State *)opaque;
> int slot;
> + unsigned pipe_depth;
>
> - slot = s->read_pos + s->read_count;
> - if (slot >= 16)
> - slot -= 16;
> + pipe_depth = pl011_get_fifo_depth(s);
> + slot = (s->read_pos + s->read_count) & (pipe_depth - 1);
> s->read_fifo[slot] = value;
> s->read_count++;
> s->flags &= ~PL011_FLAG_RXFE;
> trace_pl011_put_fifo(value, s->read_count);
> - if (!(s->lcr & 0x10) || s->read_count == 16) {
> + if (s->read_count == pipe_depth) {
> trace_pl011_put_fifo_full();
> s->flags |= PL011_FLAG_RXFF;
> }
thanks
-- PMM
