[PATCH] [PATCH] system/memory: Fix max access size

[PATCH] [PATCH] system/memory: Fix max access size

[TaiseiIto]

Before this commit, an HPET driver can not write correct values to
comparator registers of HPET timers. When I tested my HPET driver on QEMU, I
ovserved too early HPET interruptions. I investigated cause and found that
QEMU didn't write higher 32 bits of comparator registers of HPET timers even
though my HPET driver did 64 bits writing to the registers. When my HPET
driver wrote to an HPET timer comparator register with 64-bit access size,
QEMU divided once 64-bit writing into twice 32-bit writings because QEMU
allowed only up to 32-bit writing. In the twice 32-bit writings, first, QEMU
wrote lower 32 bits of the comparator register and immediately clear
HPET_TN_SETVAL flag which means whether a software can write the comparator
register of the HPET timer. Then, QEMU tried to write higher 32 bits of the
comparator register, but the writing is rejected because the HPET_TN_SETVAL
flag is already cleared. As a result, the comparator register of the HPET
timer had a incorrect value and generated too early HPET interruptions.
After this commit, QEMU allows 64-bit writings. So, once 64-bit writing to
HPET timer comparator register is not divided into twice 32-bit writings.
Therefore, the comparator register of the HPET timer has correct value. As
a result, the HPET timer generates interruptions at the correct time.

Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
---
 system/memory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/system/memory.c b/system/memory.c
index 5e6eb459d5..985a5bd2bb 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -544,7 +544,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
         access_size_min = 1;
     }
     if (!access_size_max) {
-        access_size_max = 4;
+        access_size_max = 8;
     }
 
     /* Do not allow more than one simultaneous access to a device's IO Regions */
-- 
2.34.1

Re: [PATCH] [PATCH] system/memory: Fix max access size

[Peter Maydell]

On Sat, 20 Jul 2024 at 08:56, TaiseiIto <taisei1212@outlook.jp> wrote:
>
> Before this commit, an HPET driver can not write correct values to
> comparator registers of HPET timers. When I tested my HPET driver on QEMU, I
> ovserved too early HPET interruptions. I investigated cause and found that
> QEMU didn't write higher 32 bits of comparator registers of HPET timers even
> though my HPET driver did 64 bits writing to the registers. When my HPET
> driver wrote to an HPET timer comparator register with 64-bit access size,
> QEMU divided once 64-bit writing into twice 32-bit writings because QEMU
> allowed only up to 32-bit writing. In the twice 32-bit writings, first, QEMU
> wrote lower 32 bits of the comparator register and immediately clear
> HPET_TN_SETVAL flag which means whether a software can write the comparator
> register of the HPET timer. Then, QEMU tried to write higher 32 bits of the
> comparator register, but the writing is rejected because the HPET_TN_SETVAL
> flag is already cleared. As a result, the comparator register of the HPET
> timer had a incorrect value and generated too early HPET interruptions.
> After this commit, QEMU allows 64-bit writings. So, once 64-bit writing to
> HPET timer comparator register is not divided into twice 32-bit writings.
> Therefore, the comparator register of the HPET timer has correct value. As
> a result, the HPET timer generates interruptions at the correct time.
>
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>  system/memory.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 5e6eb459d5..985a5bd2bb 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -544,7 +544,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr,
>          access_size_min = 1;
>      }
>      if (!access_size_max) {
> -        access_size_max = 4;
> +        access_size_max = 8;
>      }

This piece of code is setting the default access_size_max value
to be used by a MemoryRegionOps struct that hasn't explicitly
set that field. We use 4 because that's a pretty common setup for devices,
and also for compatibility. Changing the default because of a problem with
a specific device isn't the right fix.

If we wanted to change the default we would need to audit every MemoryRegionOps
struct in the codebase and explicitly set the value to 4 if it wasn't
already set.
(If we wanted to change the default I think I would vote for requiring
the fields to be
explicitly set everywhere and having no default at all. But that's a
lot of work and
it's not worth doing.)

If the HPET timer device is supposed to permit 64 bit writes and it is not
doing so, then that needs to be fixed in the HPET timer device model, by
making sure that its read/write functions correctly handle the size=8 case
and then setting access_size_max =8 in its MemoryRegionOps struct.

thanks
-- PMM

Re: [PATCH] [PATCH] system/memory: Fix max access size

[Paolo Bonzini]

On Sat, Jul 20, 2024 at 4:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> If the HPET timer device is supposed to permit 64 bit writes and it is not
> doing so, then that needs to be fixed in the HPET timer device model, by
> making sure that its read/write functions correctly handle the size=8 case
> and then setting access_size_max =8 in its MemoryRegionOps struct.

It does, and I've started looking into it[1].

The replacement for this patch is simple (on top of that branch):

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 5e60fedc089..ac55dd1ebd6 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -637,6 +637,10 @@ static const MemoryRegionOps hpet_ram_ops = {
         .min_access_size = 4,
         .max_access_size = 8,
     },
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
     .endianness = DEVICE_NATIVE_ENDIAN,
 };

I'll now look into the other patch for interrupts. Thanks for testing
my changes!

Paolo

[1] https://gitlab.com/bonzini/qemu/-/commits/hpet

Re: [PATCH] [PATCH] system/memory: Fix max access size

[Philippe Mathieu-Daudé]

On 22/7/24 13:53, Paolo Bonzini wrote:
> On Sat, Jul 20, 2024 at 4:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> If the HPET timer device is supposed to permit 64 bit writes and it is not
>> doing so, then that needs to be fixed in the HPET timer device model, by
>> making sure that its read/write functions correctly handle the size=8 case
>> and then setting access_size_max =8 in its MemoryRegionOps struct.
> 
> It does, and I've started looking into it[1].
> 
> The replacement for this patch is simple (on top of that branch):
> 
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 5e60fedc089..ac55dd1ebd6 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -637,6 +637,10 @@ static const MemoryRegionOps hpet_ram_ops = {
>           .min_access_size = 4,
>           .max_access_size = 8,
>       },
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,

It seems this model could be simplified using .min_access_size = 8,
letting the core memory layer handle the % 4 byte accesses (the
read path is obvious, the write one a bit less). Nothing urgent,
possibly a BitSizedTask.

> +    },
>       .endianness = DEVICE_NATIVE_ENDIAN,
>   };
> 
> I'll now look into the other patch for interrupts. Thanks for testing
> my changes!
> 
> Paolo
> 
> [1] https://gitlab.com/bonzini/qemu/-/commits/hpet
> 

Re: [PATCH] [PATCH] system/memory: Fix max access size

[Paolo Bonzini]

On Mon, Jul 22, 2024 at 4:29 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
> It seems this model could be simplified using .min_access_size = 8,
> letting the core memory layer handle the % 4 byte accesses (the
> read path is obvious, the write one a bit less). Nothing urgent,
> possibly a BitSizedTask.

The memory access core is not able to do a read-modify-write
operation. You can get a smaller read from a bigger one or split a
large read/write into many small ones, but it cannot convert a small
write into a large one. Even splitting a large read/write into many
small ones is only possible if they don't have side effects.

Paolo

> > +    },
> >       .endianness = DEVICE_NATIVE_ENDIAN,
> >   };
> >
> > I'll now look into the other patch for interrupts. Thanks for testing
> > my changes!
> >
> > Paolo
> >
> > [1] https://gitlab.com/bonzini/qemu/-/commits/hpet
> >
>

Re: [PATCH] [PATCH] system/memory: Fix max access size

[Peter Maydell]

On Mon, 22 Jul 2024 at 17:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Mon, Jul 22, 2024 at 4:29 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> > It seems this model could be simplified using .min_access_size = 8,
> > letting the core memory layer handle the % 4 byte accesses (the
> > read path is obvious, the write one a bit less). Nothing urgent,
> > possibly a BitSizedTask.
>
> The memory access core is not able to do a read-modify-write
> operation. You can get a smaller read from a bigger one or split a
> large read/write into many small ones, but it cannot convert a small
> write into a large one. Even splitting a large read/write into many
> small ones is only possible if they don't have side effects.

IIRC it will convert a small write into a large one, but
it does so by zero-extending the value and doing a single
large write. (This is the behaviour you want for some
devices.)

thanks
-- PMM