Re: [PATCH 07/19] hw/microblaze: Restrict MemoryRegionOps are implemented as 32-bit

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

On 5/11/24 23:24, Philippe Mathieu-Daudé wrote:
> On 5/11/24 14:04, Philippe Mathieu-Daudé wrote:
> > All these MemoryRegionOps read() and write() handlers are
> > implemented expecting 32-bit accesses. Clarify that setting
> > .impl.min/max_access_size fields.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>
> > ---
> >   hw/char/xilinx_uartlite.c | 4 ++++
> >   hw/intc/xilinx_intc.c     | 4 ++++
> >   hw/net/xilinx_ethlite.c   | 4 ++++
> >   hw/timer/xilinx_timer.c   | 4 ++++
> >   4 files changed, 16 insertions(+)
> >
> > diff --git a/hw/char/xilinx_uartlite.c b/hw/char/xilinx_uartlite.c
> > index a69ad769cc4..892efe81fee 100644
> > --- a/hw/char/xilinx_uartlite.c
> > +++ b/hw/char/xilinx_uartlite.c
> > @@ -170,6 +170,10 @@ static const MemoryRegionOps uart_ops = {
> >       .read = uart_read,
> >       .write = uart_write,
> >       .endianness = DEVICE_NATIVE_ENDIAN,
> > +    .impl = {
> > +        .min_access_size = 4,

Odd. The change makes the qtests pass, but here I'm modifying .impl,
not .valid... Since .valid.min_access_size = 1, SBI is a valid
opcode, no need to use SWI.

> > +        .max_access_size = 4,
> > +    },
> >       .valid = {
> >           .min_access_size = 1,
> >           .max_access_size = 4,
>
> To have qtests working I need to squash:
>
> -- >8 --
> diff --git a/tests/qtest/boot-serial-test.c 
> b/tests/qtest/boot-serial-test.c
> index 3b92fa5d506..6d9291c8ae2 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -57,7 +57,7 @@ static const uint8_t kernel_pls3adsp1800[] = {
>       0xb0, 0x00, 0x84, 0x00,                 /* imm   0x8400 */
>       0x30, 0x60, 0x00, 0x04,                 /* addik r3,r0,4 */
>       0x30, 0x80, 0x00, 0x54,                 /* addik r4,r0,'T' */
> -    0xf0, 0x83, 0x00, 0x00,                 /* sbi   r4,r3,0 */
> +    0xf8, 0x83, 0x00, 0x00,                 /* swi   r4,r3,0 */
>       0xb8, 0x00, 0xff, 0xfc                  /* bri   -4  loop */
>   };
>
> @@ -65,7 +65,7 @@ static const uint8_t kernel_plml605[] = {
>       0xe0, 0x83, 0x00, 0xb0,                 /* imm   0x83e0 */
>       0x00, 0x10, 0x60, 0x30,                 /* addik r3,r0,0x1000 */
>       0x54, 0x00, 0x80, 0x30,                 /* addik r4,r0,'T' */
> -    0x00, 0x00, 0x83, 0xf0,                 /* sbi   r4,r3,0 */
> +    0x00, 0x00, 0x83, 0xf8,                 /* swi   r4,r3,0 */
>       0xfc, 0xff, 0x00, 0xb8                  /* bri   -4  loop */
>   };
> ---
>
> to access the uart by 32-bit instead of 8-bit.