[PATCH for 8.2] accel/tcg/cputlb: Fix iotlb page alignment check

[PATCH for 8.2] accel/tcg/cputlb: Fix iotlb page alignment check

[LIU Zhiwei]

For ram memory region the iotlb(which will be filled into the xlat_section
of CPUTLBEntryFull) is calculated as:

iotlb = memory_region_get_ram_addr(section->mr) + xlat;

1) xlat here is the offset_within_region of a MemoryRegionSection, which maybe
not TARGET_PAGE_BITS aligned.
2) The ram_addr_t returned by memory_region_get_ram_addr is always
HOST PAGE ALIGNED.

So we cann't assert the sum of them is TARGET_PAGE_BITS aligend.
A fail case has been give by the link:
https://lore.kernel.org/all/b68ab7d3-d3d3-9f81-569d-454ae9c11b16@linaro.org/T/

Fixes: dff1ab68d8c5 ("accel/tcg: Fix the comment for CPUTLBEntryFull")
Signed-off-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
---
 accel/tcg/cputlb.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index db3f93fda9..7a50a21a2e 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1168,7 +1168,6 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
     write_flags = read_flags;
     if (is_ram) {
         iotlb = memory_region_get_ram_addr(section->mr) + xlat;
-        assert(!(iotlb & ~TARGET_PAGE_MASK));
         /*
          * Computing is_clean is expensive; avoid all that unless
          * the page is actually writable.
@@ -1231,9 +1230,8 @@ void tlb_set_page_full(CPUState *cpu, int mmu_idx,
 
     /* refill the tlb */
     /*
-     * When memory region is ram, iotlb contains a TARGET_PAGE_BITS
-     * aligned ram_addr_t of the page base of the target RAM.
-     * Otherwise, iotlb contains
+     * When memory region is ram, iotlb contains ram_addr_t of the page base
+     * of the target RAM. Otherwise, iotlb contains
      *  - a physical section number in the lower TARGET_PAGE_BITS
      *  - the offset within section->mr of the page base (I/O, ROMD) with the
      *    TARGET_PAGE_BITS masked off.
-- 
2.17.1

Re: [PATCH for 8.2] accel/tcg/cputlb: Fix iotlb page alignment check

[Richard Henderson]

On 12/7/23 18:06, LIU Zhiwei wrote:
> For ram memory region the iotlb(which will be filled into the xlat_section
> of CPUTLBEntryFull) is calculated as:
> 
> iotlb = memory_region_get_ram_addr(section->mr) + xlat;
> 
> 1) xlat here is the offset_within_region of a MemoryRegionSection, which maybe
> not TARGET_PAGE_BITS aligned.

The reason we require these bits to be zero is because
(A) The lowest bits must be zero so that we may test alignment,
(B) The "middle" bits, above alignment and below TARGET_PAGE_BITS,
     are used for TLB_FLAGS_MASK.

If iotlb has these bits non-zero, the softmmu comparison will not work correctly.

> 2) The ram_addr_t returned by memory_region_get_ram_addr is always
> HOST PAGE ALIGNED.

RAM blocks can have larger alignment than host page.  See QEMU_VMALLOC_ALIGN and 
qemu_ram_mmap.

But I can see a path by which it *is* only host page aligned, which could fail if the 
guest has larger alignment than the host.  Fortunately, this is rare -- only alpha, cris, 
openrisc and sparc64 have 8k pages, and tricore has 16k pages, while supporting system 
mode.  Hexagon has 64k pages but does not yet support system mode.

We should fix that, but I don't think it's urgent.


> So we cann't assert the sum of them is TARGET_PAGE_BITS aligend.
> A fail case has been give by the link:
> https://lore.kernel.org/all/b68ab7d3-d3d3-9f81-569d-454ae9c11b16@linaro.org/T/

I think the macfb device is at fault here, not your assert.


r~

Re: [PATCH for 8.2] accel/tcg/cputlb: Fix iotlb page alignment check

[Mark Cave-Ayland]

On 08/12/2023 17:42, Richard Henderson wrote:

> On 12/7/23 18:06, LIU Zhiwei wrote:
>> For ram memory region the iotlb(which will be filled into the xlat_section
>> of CPUTLBEntryFull) is calculated as:
>>
>> iotlb = memory_region_get_ram_addr(section->mr) + xlat;
>>
>> 1) xlat here is the offset_within_region of a MemoryRegionSection, which maybe
>> not TARGET_PAGE_BITS aligned.
> 
> The reason we require these bits to be zero is because
> (A) The lowest bits must be zero so that we may test alignment,
> (B) The "middle" bits, above alignment and below TARGET_PAGE_BITS,
>      are used for TLB_FLAGS_MASK.
> 
> If iotlb has these bits non-zero, the softmmu comparison will not work correctly.
> 
>> 2) The ram_addr_t returned by memory_region_get_ram_addr is always
>> HOST PAGE ALIGNED.
> 
> RAM blocks can have larger alignment than host page.  See QEMU_VMALLOC_ALIGN and 
> qemu_ram_mmap.
> 
> But I can see a path by which it *is* only host page aligned, which could fail if the 
> guest has larger alignment than the host.  Fortunately, this is rare -- only alpha, 
> cris, openrisc and sparc64 have 8k pages, and tricore has 16k pages, while supporting 
> system mode.  Hexagon has 64k pages but does not yet support system mode.
> 
> We should fix that, but I don't think it's urgent.

FWIW I did check the MMU configuration with the m68k test case, and I can see that 
the ROM configures the MMU to use 8k pages.

>> So we cann't assert the sum of them is TARGET_PAGE_BITS aligend.
>> A fail case has been give by the link:
>> https://lore.kernel.org/all/b68ab7d3-d3d3-9f81-569d-454ae9c11b16@linaro.org/T/
> 
> I think the macfb device is at fault here, not your assert.

The memory region is set up using a standard memory_region_init_rom() and 
memory_region_add_subregion() combination (see 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/nubus/nubus-device.c?ref_type=heads#L79) 
so should it be that the ROM memory region must be aligned to TARGET_PAGE_MASK?

This feels odd because I don't believe there are any other alignment restrictions for 
the memory API, and if there are, shouldn't the assert() fire during 
memory_region_add_subregion() rather than during the first memory access at runtime?


ATB,

Mark.