Re: [PATCH] hw/char/stm32l4x5_usart.c: Fix ACK and min access size

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 9 Sept 2024 at 18:40, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> (Cc'ing Arnaud & Inès who are listed as maintainers)
>
> On 6/9/24 18:12, Peter Maydell wrote:
> > On Mon, 2 Sept 2024 at 14:38, Jacob Abrams <satur9nine@gmail.com> wrote:
> >>
> >> These changes allow the official STM32L4xx HAL UART driver to function
> >> properly with the b-l475e-iot01a machine.
> >>
> >> Modifying USART_CR1 TE bit should alter USART_ISR TEACK bit, and
> >> likewise for RE and REACK bit.
> >>
> >> USART registers may be accessed via 16-bit instructions.
> >>
> >> Reseting USART_CR1 UE bit should restore ISR to default value.
> >>
> >> Fixes: 87b77e6e01ca ("hw/char/stm32l4x5_usart: Enable serial read and write")
> >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2540
> >> Signed-off-by: Jacob Abrams <satur9nine@gmail.com>
> >
> > Thanks for this patch. I have one question below, and one
> > minor nit.
> >
> >> ---
> >>   hw/char/stm32l4x5_usart.c          | 29 +++++++++++++++++++---
> >>   tests/qtest/stm32l4x5_usart-test.c | 39 +++++++++++++++++++++++++++++-
> >>   2 files changed, 64 insertions(+), 4 deletions(-)
>
>
> >>   static void stm32l4x5_update_irq(Stm32l4x5UsartBaseState *s)
> >>   {
> >>       if (((s->isr & R_ISR_WUF_MASK) && (s->cr3 & R_CR3_WUFIE_MASK))        ||
> >> @@ -367,7 +389,7 @@ static void stm32l4x5_usart_base_reset_hold(Object *obj, ResetType type)
> >>       s->brr = 0x00000000;
> >>       s->gtpr = 0x00000000;
> >>       s->rtor = 0x00000000;
> >> -    s->isr = 0x020000C0;
> >> +    s->isr = ISR_RESET_VALUE;
> >>       s->rdr = 0x00000000;
> >>       s->tdr = 0x00000000;
> >>
> >> @@ -456,6 +478,7 @@ static void stm32l4x5_usart_base_write(void *opaque, hwaddr addr,
> >>       case A_CR1:
> >>           s->cr1 = value;
> >>           stm32l4x5_update_params(s);
> >> +        stm32l4x5_update_isr(s);
> >>           stm32l4x5_update_irq(s);
> >>           return;
> >>       case A_CR2:
> >> @@ -508,12 +531,12 @@ static const MemoryRegionOps stm32l4x5_usart_base_ops = {
> >>       .endianness = DEVICE_NATIVE_ENDIAN,
> >>       .valid = {
> >>           .max_access_size = 4,
> >> -        .min_access_size = 4,
> >> +        .min_access_size = 2,
> >>           .unaligned = false
> >>       },
> >>       .impl = {
> >>           .max_access_size = 4,
> >> -        .min_access_size = 4,
> >> +        .min_access_size = 2,
> >>           .unaligned = false
> >>       },
> >
> > The effect of these is that a 16-bit write not aligned
> > to a (4-aligned) register offset will generate a GUEST_ERROR
> > logged message, and a 16-bit write aligned to a 4-aligned
> > register offset will write the value zero-extended to 32 bits.
> > That seems reasonable to me.
>
> Peter, are you describing the .valid.min_access_size 4 -> 2 change
> or the .impl.min_access_size one?

I was intending to summarise the effects of making the code
changes above (both .impl and .valid), without any reference
to what the real hardware behaviour might or might not be
(as a starter for figuring out whether the change is reasonable).

> My understanding of the implementation is a 32-bit one:
>
>    REG32(CR1, 0x00)
>
>    struct Stm32l4x5UsartBaseState {
>        ...
>        uint32_t cr1;
>
>    static void stm32l4x5_usart_base_write(void *opaque, hwaddr addr,
>                                    uint64_t val64, unsigned int size)
>    {
>        ...
>        switch (addr) {
>        case A_CR1:
>            s->cr1 = value;
>
> Am I missing something?

If we make the .impl and .valid changes, then the result is
that we permit 16 bit writes to come through to the read
and write functions. Since we don't make any changes to
those functions to specially handle size == 2, you get the
effects of the existing code. If the 16 bit write is aligned
to a 4-aligned register offset it will match one of the A_*
cases, and will write 16-bit-value-zero-extended to it.
If the 16 bit write isn't to a 4-aligned offset it will fall
into the "default" case and be logged as a GUEST_ERROR.

Did I miss some aspect of what the behaviour change is?

-- PMM