Re: of AVR target page size

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Wed, 17 Mar 2021 at 20:17, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > Hi Michael,
> >   I noticed your AVR code defines:
> >
> >   #define TARGET_PAGE_BITS 8
> >
> > and has an explanation of why.
> >
> > Note however that's not going to work with the current live
> > migration/snapshotting code, since you're a couple of bits smaller
> > than the smallest page size we had so far, and for many years
> > the RAM migration code has stolen the bottom few bits of the address
> > as a flag field, and has already used 0x100 up; see migration/ram.c
> > RAM_SAVE_FLAG_*    - and it's actually tricky to change it, because if
> > you change it then it'll break migration compatibility with existing
> > qemu's.
> 
> If you want to use low bits as flags for other stuff, you
> should have a compile time assert that you have the number
> of bits you expect, or otherwise force a compile error.
> Otherwise you'll end up with unpleasant surprises like this one...

Yes, I think that's been around for a long time.

> I think that for the cpu-all.h uses of low bits we would
> end up with a compile error for excessively small TARGET_PAGE_BITS
> because we define the bits like this:
> #define TLB_DISCARD_WRITE   (1 << (TARGET_PAGE_BITS_MIN - 6))
> and I expect the compiler will complain if the RHS of the '<<'
> is a negative constant. But I don't know if that's deliberate
> or a happy accident :-)


Something like this, does fail for AVR:

>From 8a617836955ebba1a4932d238fce600be51b9182 Mon Sep 17 00:00:00 2001
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Date: Thu, 18 Mar 2021 10:17:27 +0000
Subject: [PATCH] migration: Check TARGET_PAGE_SIZE vs RAM flags

migration/ram.c steals the bottom few bits of address for flags;
check that we don't run into TARGET_PAGE_SIZE

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 1ee7ff9c6d..f053d45f3c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -81,6 +81,8 @@
 /* 0x80 is reserved in migration.h start with 0x100 next */
 #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
 
+#define RAM_SAVE_FLAG__MAX RAM_SAVE_FLAG_COMPRESS_PAGE
+
 static inline bool is_zero_range(uint8_t *p, uint64_t size)
 {
     return buffer_is_zero(p, size);
@@ -4101,5 +4103,10 @@ static SaveVMHandlers savevm_ram_handlers = {
 void ram_mig_init(void)
 {
     qemu_mutex_init(&XBZRLE.lock);
+#ifndef TARGET_PAGE_BITS_VARY
+    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= TARGET_PAGE_SIZE);
+#else
+    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= (1 << TARGET_PAGE_BITS_MIN));
+#endif
     register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
 }
-- 
2.30.2

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK