* [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0
@ 2025-10-22 11:52 Nikita Novikov
2025-10-22 15:24 ` Philippe Mathieu-Daudé
2025-10-22 15:32 ` Richard Henderson
0 siblings, 2 replies; 6+ messages in thread
From: Nikita Novikov @ 2025-10-22 11:52 UTC (permalink / raw)
To: richard.henderson; +Cc: pbonzini, qemu-devel, qemu-riscv, Nikita Novikov
Recent debugging of misaligned access handling on RISC-V revealed that we
always call `tlb_fill` with `memop_size == 0`. This behavior effectively
disables natural alignment checks in `riscv_tlb_fill_align()`, because we
have to fall back from `memop_size` to `size` when computing the alignment bits.
With `memop_size == 0`, misaligned cross-page stores end up reported as
`store access fault` (AF, cause=7) instead of the expected
`store page fault` (PF, cause=15), since the “misalign” path triggers before
the second page translation can fault. This breaks misaligned accesses at
page boundaries.
After switching to pass the real `l->memop` into `tlb_fill`, the cross-page
faults are no longer mis-classified as AF.
Fixes: ec03dd972378 ("accel/tcg: Hoist first page lookup above pointer_wrap")
Signed-off-by: Nikita Novikov <n.novikov@syntacore.com>
---
accel/tcg/cputlb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 631f1fe135..271c061be1 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1782,7 +1782,7 @@ static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
* If the lookup potentially resized the table, refresh the
* first CPUTLBEntryFull pointer.
*/
- if (mmu_lookup1(cpu, &l->page[1], 0, l->mmu_idx, type, ra)) {
+ if (mmu_lookup1(cpu, &l->page[1], l->memop, l->mmu_idx, type, ra)) {
uintptr_t index = tlb_index(cpu, l->mmu_idx, addr);
l->page[0].full = &cpu->neg.tlb.d[l->mmu_idx].fulltlb[index];
}
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0
2025-10-22 11:52 [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0 Nikita Novikov
@ 2025-10-22 15:24 ` Philippe Mathieu-Daudé
2025-10-22 15:32 ` Richard Henderson
1 sibling, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-10-22 15:24 UTC (permalink / raw)
To: Nikita Novikov, richard.henderson; +Cc: pbonzini, qemu-devel, qemu-riscv
On 22/10/25 13:52, Nikita Novikov wrote:
> Recent debugging of misaligned access handling on RISC-V revealed that we
> always call `tlb_fill` with `memop_size == 0`. This behavior effectively
> disables natural alignment checks in `riscv_tlb_fill_align()`, because we
> have to fall back from `memop_size` to `size` when computing the alignment bits.
>
> With `memop_size == 0`, misaligned cross-page stores end up reported as
> `store access fault` (AF, cause=7) instead of the expected
> `store page fault` (PF, cause=15), since the “misalign” path triggers before
> the second page translation can fault. This breaks misaligned accesses at
> page boundaries.
>
> After switching to pass the real `l->memop` into `tlb_fill`, the cross-page
> faults are no longer mis-classified as AF.
>
> Fixes: ec03dd972378 ("accel/tcg: Hoist first page lookup above pointer_wrap")
>
> Signed-off-by: Nikita Novikov <n.novikov@syntacore.com>
> ---
> accel/tcg/cputlb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Good catch!
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0
2025-10-22 11:52 [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0 Nikita Novikov
2025-10-22 15:24 ` Philippe Mathieu-Daudé
@ 2025-10-22 15:32 ` Richard Henderson
2025-10-22 18:59 ` Nikita Novikov
1 sibling, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2025-10-22 15:32 UTC (permalink / raw)
To: Nikita Novikov; +Cc: pbonzini, qemu-devel, qemu-riscv
On 10/22/25 06:52, Nikita Novikov wrote:
> Recent debugging of misaligned access handling on RISC-V revealed that we
> always call `tlb_fill` with `memop_size == 0`. This behavior effectively
> disables natural alignment checks in `riscv_tlb_fill_align()`, because we
> have to fall back from `memop_size` to `size` when computing the alignment bits.
>
> With `memop_size == 0`, misaligned cross-page stores end up reported as
> `store access fault` (AF, cause=7) instead of the expected
> `store page fault` (PF, cause=15), since the “misalign” path triggers before
> the second page translation can fault. This breaks misaligned accesses at
> page boundaries.
>
> After switching to pass the real `l->memop` into `tlb_fill`, the cross-page
> faults are no longer mis-classified as AF.
>
> Fixes: ec03dd972378 ("accel/tcg: Hoist first page lookup above pointer_wrap")
>
> Signed-off-by: Nikita Novikov <n.novikov@syntacore.com>
> ---
> accel/tcg/cputlb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 631f1fe135..271c061be1 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1782,7 +1782,7 @@ static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
> * If the lookup potentially resized the table, refresh the
> * first CPUTLBEntryFull pointer.
> */
> - if (mmu_lookup1(cpu, &l->page[1], 0, l->mmu_idx, type, ra)) {
> + if (mmu_lookup1(cpu, &l->page[1], l->memop, l->mmu_idx, type, ra)) {
> uintptr_t index = tlb_index(cpu, l->mmu_idx, addr);
> l->page[0].full = &cpu->neg.tlb.d[l->mmu_idx].fulltlb[index];
> }
How is the memop really applicable to the second half of a split-page operation?
r~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0
2025-10-22 15:32 ` Richard Henderson
@ 2025-10-22 18:59 ` Nikita Novikov
2025-10-22 19:20 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Nikita Novikov @ 2025-10-22 18:59 UTC (permalink / raw)
To: Richard Henderson; +Cc: n.novikov, pbonzini, qemu-devel, qemu-riscv
On Wed, Oct 22, 2025 at 10:32:31AM -0500, Richard Henderson wrote:
> On 10/22/25 06:52, Nikita Novikov wrote:
> > Recent debugging of misaligned access handling on RISC-V revealed that we
> > always call `tlb_fill` with `memop_size == 0`. This behavior effectively
> > disables natural alignment checks in `riscv_tlb_fill_align()`, because we
> > have to fall back from `memop_size` to `size` when computing the alignment bits.
> >
> > With `memop_size == 0`, misaligned cross-page stores end up reported as
> > `store access fault` (AF, cause=7) instead of the expected
> > `store page fault` (PF, cause=15), since the “misalign” path triggers before
> > the second page translation can fault. This breaks misaligned accesses at
> > page boundaries.
> >
> > After switching to pass the real `l->memop` into `tlb_fill`, the cross-page
> > faults are no longer mis-classified as AF.
> >
> > Fixes: ec03dd972378 ("accel/tcg: Hoist first page lookup above pointer_wrap")
> >
> > Signed-off-by: Nikita Novikov <n.novikov@syntacore.com>
> > ---
> > accel/tcg/cputlb.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> > index 631f1fe135..271c061be1 100644
> > --- a/accel/tcg/cputlb.c
> > +++ b/accel/tcg/cputlb.c
> > @@ -1782,7 +1782,7 @@ static bool mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
> > * If the lookup potentially resized the table, refresh the
> > * first CPUTLBEntryFull pointer.
> > */
> > - if (mmu_lookup1(cpu, &l->page[1], 0, l->mmu_idx, type, ra)) {
> > + if (mmu_lookup1(cpu, &l->page[1], l->memop, l->mmu_idx, type, ra)) {
> > uintptr_t index = tlb_index(cpu, l->mmu_idx, addr);
> > l->page[0].full = &cpu->neg.tlb.d[l->mmu_idx].fulltlb[index];
> > }
>
> How is the memop really applicable to the second half of a split-page operation?
>
Because the second half is still part of the same guest memory operation. It must obey
the same size, alignment, and atomicity rules. Passing the real memop ensures correct
alignment and atomic checks even if the access crosses a page boundary.
>
> r~
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0
2025-10-22 18:59 ` Nikita Novikov
@ 2025-10-22 19:20 ` Richard Henderson
2025-10-23 6:38 ` Nikita Novikov
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2025-10-22 19:20 UTC (permalink / raw)
To: Nikita Novikov; +Cc: n.novikov, pbonzini, qemu-devel, qemu-riscv
On 10/22/25 13:59, Nikita Novikov wrote:
>>> Fixes: ec03dd972378 ("accel/tcg: Hoist first page lookup above pointer_wrap")
This cannot be true, btw, because ...
>>> - if (mmu_lookup1(cpu, &l->page[1], 0, l->mmu_idx, type, ra)) {
>>> + if (mmu_lookup1(cpu, &l->page[1], l->memop, l->mmu_idx, type, ra)) {
... this line did not change with that patch.
>>> uintptr_t index = tlb_index(cpu, l->mmu_idx, addr);
>>> l->page[0].full = &cpu->neg.tlb.d[l->mmu_idx].fulltlb[index];
>>> }
>>
>> How is the memop really applicable to the second half of a split-page operation?
>>
> Because the second half is still part of the same guest memory operation. It must obey
> the same size, alignment, and atomicity rules. Passing the real memop ensures correct
> alignment and atomic checks even if the access crosses a page boundary.
How?
Let's use a concrete example: Access MO_64 | MO_UNALN at 0x1fffd.
The first tlb_fill gets to see the start address 0x1fffd, and the length 3 (and also the
memop).
The second tlb_fill gets to see the second page address 0x20000 and the length 5 (but not
the memop).
Exactly what is the second tlb_fill going to do with 0x20000 and MO_64 | MO_UNALN?
r~
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0
2025-10-22 19:20 ` Richard Henderson
@ 2025-10-23 6:38 ` Nikita Novikov
0 siblings, 0 replies; 6+ messages in thread
From: Nikita Novikov @ 2025-10-23 6:38 UTC (permalink / raw)
To: Richard Henderson; +Cc: pbonzini, qemu-devel, qemu-riscv, n.novikov
On 22/10/2025 22:20, Richard Henderson wrote:
> On 10/22/25 13:59, Nikita Novikov wrote:
>>>> Fixes: ec03dd972378 ("accel/tcg: Hoist first page lookup above
>>>> pointer_wrap")
>
> This cannot be true, btw, because ...
>
>>>> - if (mmu_lookup1(cpu, &l->page[1], 0, l->mmu_idx, type, ra)) {
>>>> + if (mmu_lookup1(cpu, &l->page[1], l->memop, l->mmu_idx,
>>>> type, ra)) {
>
> ... this line did not change with that patch.
My bad, sorry. I suggest it will be better to use `Related-to`, because
actually this patch makes both halves of a split-page access consistent
or did I misunderstand you?
>>>> uintptr_t index = tlb_index(cpu, l->mmu_idx, addr);
>>>> l->page[0].full =
>>>> &cpu->neg.tlb.d[l->mmu_idx].fulltlb[index];
>>>> }
>>>
>>> How is the memop really applicable to the second half of a
>>> split-page operation?
>>>
>> Because the second half is still part of the same guest memory
>> operation. It must obey
>> the same size, alignment, and atomicity rules. Passing the real memop
>> ensures correct
>> alignment and atomic checks even if the access crosses a page boundary.
>
> How?
>
> Let's use a concrete example: Access MO_64 | MO_UNALN at 0x1fffd.
>
> The first tlb_fill gets to see the start address 0x1fffd, and the
> length 3 (and also the memop).
>
> The second tlb_fill gets to see the second page address 0x20000 and
> the length 5 (but not the memop).
>
> Exactly what is the second tlb_fill going to do with 0x20000 and MO_64
> | MO_UNALN?
>
The guest does a 64-bit access with flags MO_64 | MO_UNALN at address
0x1fffd. The first `tlb_fill` handles bytes from 0x1fffd-0x1ffff with
length 3, and sees the real `memop`, so it knows the access is unaligned
but allowed. The second `tlb_fill` handles bytes 0x20000-0x20004 with
length 5. If it gets `memop == 0`, it loses information about the access
type, alignment and atomicity. With the real memop it stays consistent
with the first half, because it knows this is part of one 64-bit
unaligned access, not a separate normal one. For `MO_UNALN` it changes
nothing, but for atomic or alignment-restricted cases it prevents the
second page from being treated incorrectly as a normal split access.
Also it does not raise an alignment fault, and sets the same slow-path
flags (ex. `TLB_CHECK_ALIGNED`) and region checks.
I faced with this bug while debugging a case where `memop_size == 1`,
because `memop_size(0) == 1`. According to the code it means the second
half doesn’t carry any valid access information, but it's not true.
>
> r~
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-23 6:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-22 11:52 [PATCH] accel/tcg: Pass actual memop_size to tlb_fill instead of 0 Nikita Novikov
2025-10-22 15:24 ` Philippe Mathieu-Daudé
2025-10-22 15:32 ` Richard Henderson
2025-10-22 18:59 ` Nikita Novikov
2025-10-22 19:20 ` Richard Henderson
2025-10-23 6:38 ` Nikita Novikov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).