of AVR target page size

of AVR target page size

[Dr. David Alan Gilbert]

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.

Hmm.

Dave

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

Re: of AVR target page size

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

Hi Dave.

What is the smallest supported page size?


On Wed, Mar 17, 2021 at 10:14 PM 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.
>
> Hmm.
>
> Dave
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 1431 bytes --]

Re: of AVR target page size

[Peter Maydell]

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...

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 :-)

thanks
-- PMM

Re: of AVR target page size

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

I guess we can add some bits TARGET_PAGE_BITS, this will make us to push
some portion of SRAM into the CPU.

Michael Rolnik

On Thu, Mar 18, 2021 at 12:33 AM 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...
>
> 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 :-)
>
> thanks
> -- PMM
>


-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 2295 bytes --]

Re: of AVR target page size

[Dr. David Alan Gilbert]

* Michael Rolnik (mrolnik@gmail.com) wrote:
> Hi Dave.
> 
> What is the smallest supported page size?

Currently 512 I think; in migration/ram.c we have:

#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
#define RAM_SAVE_FLAG_ZERO     0x02
#define RAM_SAVE_FLAG_MEM_SIZE 0x04
#define RAM_SAVE_FLAG_PAGE     0x08
#define RAM_SAVE_FLAG_EOS      0x10
#define RAM_SAVE_FLAG_CONTINUE 0x20
#define RAM_SAVE_FLAG_XBZRLE   0x40
/* 0x80 is reserved in migration.h start with 0x100 next */
#define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100

so we're already using the 0x100 (256) flag.

I spotted this yesterday because a patch tried to use the 0x200 flag.

Dave
> 
> On Wed, Mar 17, 2021 at 10:14 PM 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.
> >
> > Hmm.
> >
> > Dave
> >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> 
> -- 
> Best Regards,
> Michael Rolnik
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: of AVR target page size

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

ok. I will try to fix it.

Regards,
Michael Rolnik

On Thu, Mar 18, 2021 at 11:55 AM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> * Michael Rolnik (mrolnik@gmail.com) wrote:
> > Hi Dave.
> >
> > What is the smallest supported page size?
>
> Currently 512 I think; in migration/ram.c we have:
>
> #define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
> #define RAM_SAVE_FLAG_ZERO     0x02
> #define RAM_SAVE_FLAG_MEM_SIZE 0x04
> #define RAM_SAVE_FLAG_PAGE     0x08
> #define RAM_SAVE_FLAG_EOS      0x10
> #define RAM_SAVE_FLAG_CONTINUE 0x20
> #define RAM_SAVE_FLAG_XBZRLE   0x40
> /* 0x80 is reserved in migration.h start with 0x100 next */
> #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
>
> so we're already using the 0x100 (256) flag.
>
> I spotted this yesterday because a patch tried to use the 0x200 flag.
>
> Dave
> >
> > On Wed, Mar 17, 2021 at 10:14 PM 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.
> > >
> > > Hmm.
> > >
> > > Dave
> > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> > >
> >
> > --
> > Best Regards,
> > Michael Rolnik
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 2929 bytes --]

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

Re: of AVR target page size

[Dr. David Alan Gilbert]

* Michael Rolnik (mrolnik@gmail.com) wrote:
> ok. I will try to fix it.

Thanks, if you can that would be great.

Dave

> Regards,
> Michael Rolnik
> 
> On Thu, Mar 18, 2021 at 11:55 AM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> > * Michael Rolnik (mrolnik@gmail.com) wrote:
> > > Hi Dave.
> > >
> > > What is the smallest supported page size?
> >
> > Currently 512 I think; in migration/ram.c we have:
> >
> > #define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
> > #define RAM_SAVE_FLAG_ZERO     0x02
> > #define RAM_SAVE_FLAG_MEM_SIZE 0x04
> > #define RAM_SAVE_FLAG_PAGE     0x08
> > #define RAM_SAVE_FLAG_EOS      0x10
> > #define RAM_SAVE_FLAG_CONTINUE 0x20
> > #define RAM_SAVE_FLAG_XBZRLE   0x40
> > /* 0x80 is reserved in migration.h start with 0x100 next */
> > #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> >
> > so we're already using the 0x100 (256) flag.
> >
> > I spotted this yesterday because a patch tried to use the 0x200 flag.
> >
> > Dave
> > >
> > > On Wed, Mar 17, 2021 at 10:14 PM 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.
> > > >
> > > > Hmm.
> > > >
> > > > Dave
> > > >
> > > > --
> > > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > > >
> > > >
> > >
> > > --
> > > Best Regards,
> > > Michael Rolnik
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> 
> -- 
> Best Regards,
> Michael Rolnik
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: of AVR target page size

[Peter Maydell]

On Thu, 18 Mar 2021 at 10:18, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> 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

You should just be able to check against TARGET_PAGE_BITS_MIN;
we set that for both the TARGET_PAGE_BITS_VARY and the don't-vary
case.

thanks
-- PMM

Re: of AVR target page size

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 18 Mar 2021 at 10:18, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> > 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
> 
> You should just be able to check against TARGET_PAGE_BITS_MIN;
> we set that for both the TARGET_PAGE_BITS_VARY and the don't-vary
> case.

Oh yes, just:

diff --git a/migration/ram.c b/migration/ram.c
index 52537f14ac..a7269955b5 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);
@@ -4090,5 +4092,6 @@ static SaveVMHandlers savevm_ram_handlers = {
 void ram_mig_init(void)
 {
     qemu_mutex_init(&XBZRLE.lock);
+    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= (1 << TARGET_PAGE_BITS_MIN));
     register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
 }


works; lets keep that in mind somewhere after Michael fixes AVR.

Dave

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

Re: of AVR target page size

[Peter Maydell]

On Thu, 18 Mar 2021 at 10:25, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> Oh yes, just:
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 52537f14ac..a7269955b5 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);
> @@ -4090,5 +4092,6 @@ static SaveVMHandlers savevm_ram_handlers = {
>  void ram_mig_init(void)
>  {
>      qemu_mutex_init(&XBZRLE.lock);
> +    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= (1 << TARGET_PAGE_BITS_MIN));
>      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
>  }
>
>
> works; lets keep that in mind somewhere after Michael fixes AVR.

You don't have a great deal of headroom even after getting AVR
to change, by the way -- TARGET_PAGE_BITS_MIN for Arm is 10.
So you might want to think about ways to eg reclaim usage of
that "obsolete, not used" RAM_SAVE_FLAG_FULL bit.

Also, what does the
 /* 0x80 is reserved in migration.h start with 0x100 next */
comment refer to? migration.h has no instances of "RAM_SAVE"
or 0x80 or 1 << 7...

thanks
-- PMM

Re: of AVR target page size

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 18 Mar 2021 at 10:25, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> > Oh yes, just:
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 52537f14ac..a7269955b5 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);
> > @@ -4090,5 +4092,6 @@ static SaveVMHandlers savevm_ram_handlers = {
> >  void ram_mig_init(void)
> >  {
> >      qemu_mutex_init(&XBZRLE.lock);
> > +    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= (1 << TARGET_PAGE_BITS_MIN));
> >      register_savevm_live("ram", 0, 4, &savevm_ram_handlers, &ram_state);
> >  }
> >
> >
> > works; lets keep that in mind somewhere after Michael fixes AVR.
> 
> You don't have a great deal of headroom even after getting AVR
> to change, by the way -- TARGET_PAGE_BITS_MIN for Arm is 10.
> So you might want to think about ways to eg reclaim usage of
> that "obsolete, not used" RAM_SAVE_FLAG_FULL bit.

Yep, I've been warning anyone who adds one for ages

> Also, what does the
>  /* 0x80 is reserved in migration.h start with 0x100 next */
> comment refer to? migration.h has no instances of "RAM_SAVE"
> or 0x80 or 1 << 7...

It looks like it got moved to qemu-file.h a few years ago
as RAM_SAVE_FLAG_HOOK.

Dave

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

Re: of AVR target page size

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

how do I test my fix? Is there a procedure?

Thanks,
Michael Rolnik

On Thu, Mar 18, 2021 at 12:45 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > On Thu, 18 Mar 2021 at 10:25, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > > Oh yes, just:
> > >
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 52537f14ac..a7269955b5 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);
> > > @@ -4090,5 +4092,6 @@ static SaveVMHandlers savevm_ram_handlers = {
> > >  void ram_mig_init(void)
> > >  {
> > >      qemu_mutex_init(&XBZRLE.lock);
> > > +    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= (1 <<
> TARGET_PAGE_BITS_MIN));
> > >      register_savevm_live("ram", 0, 4, &savevm_ram_handlers,
> &ram_state);
> > >  }
> > >
> > >
> > > works; lets keep that in mind somewhere after Michael fixes AVR.
> >
> > You don't have a great deal of headroom even after getting AVR
> > to change, by the way -- TARGET_PAGE_BITS_MIN for Arm is 10.
> > So you might want to think about ways to eg reclaim usage of
> > that "obsolete, not used" RAM_SAVE_FLAG_FULL bit.
>
> Yep, I've been warning anyone who adds one for ages
>
> > Also, what does the
> >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > comment refer to? migration.h has no instances of "RAM_SAVE"
> > or 0x80 or 1 << 7...
>
> It looks like it got moved to qemu-file.h a few years ago
> as RAM_SAVE_FLAG_HOOK.
>
> Dave
>
> > thanks
> > -- PMM
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 2960 bytes --]

Re: of AVR target page size

[Peter Maydell]

On Thu, 18 Mar 2021 at 10:45, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > Also, what does the
> >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > comment refer to? migration.h has no instances of "RAM_SAVE"
> > or 0x80 or 1 << 7...
>
> It looks like it got moved to qemu-file.h a few years ago
> as RAM_SAVE_FLAG_HOOK.

Could we put the define of RAM_SAVE_FLAG_HOOK in migration.h
with all the other RAM_SAVE_FLAG defines ? It looks like the two
places that use it already include migration.h...

thanks
-- PMM

Re: of AVR target page size

[Dr. David Alan Gilbert]

* Michael Rolnik (mrolnik@gmail.com) wrote:
> how do I test my fix? Is there a procedure?

As long as your TARGET_PAGE_SIZE is now 512 or bigger you should be OK
as long as your AVR stuff still works.  If you want you can try and do a
live migrate between two copies of qemu, but that does assume you'vre
wired up the vmstate info for all your devices.

Dave

> Thanks,
> Michael Rolnik
> 
> On Thu, Mar 18, 2021 at 12:45 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > On Thu, 18 Mar 2021 at 10:25, Dr. David Alan Gilbert
> > > <dgilbert@redhat.com> wrote:
> > > > Oh yes, just:
> > > >
> > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > index 52537f14ac..a7269955b5 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);
> > > > @@ -4090,5 +4092,6 @@ static SaveVMHandlers savevm_ram_handlers = {
> > > >  void ram_mig_init(void)
> > > >  {
> > > >      qemu_mutex_init(&XBZRLE.lock);
> > > > +    QEMU_BUILD_BUG_ON(RAM_SAVE_FLAG__MAX >= (1 <<
> > TARGET_PAGE_BITS_MIN));
> > > >      register_savevm_live("ram", 0, 4, &savevm_ram_handlers,
> > &ram_state);
> > > >  }
> > > >
> > > >
> > > > works; lets keep that in mind somewhere after Michael fixes AVR.
> > >
> > > You don't have a great deal of headroom even after getting AVR
> > > to change, by the way -- TARGET_PAGE_BITS_MIN for Arm is 10.
> > > So you might want to think about ways to eg reclaim usage of
> > > that "obsolete, not used" RAM_SAVE_FLAG_FULL bit.
> >
> > Yep, I've been warning anyone who adds one for ages
> >
> > > Also, what does the
> > >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > > comment refer to? migration.h has no instances of "RAM_SAVE"
> > > or 0x80 or 1 << 7...
> >
> > It looks like it got moved to qemu-file.h a few years ago
> > as RAM_SAVE_FLAG_HOOK.
> >
> > Dave
> >
> > > thanks
> > > -- PMM
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> 
> -- 
> Best Regards,
> Michael Rolnik
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: of AVR target page size

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 18 Mar 2021 at 10:45, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > Also, what does the
> > >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > > comment refer to? migration.h has no instances of "RAM_SAVE"
> > > or 0x80 or 1 << 7...
> >
> > It looks like it got moved to qemu-file.h a few years ago
> > as RAM_SAVE_FLAG_HOOK.
> 
> Could we put the define of RAM_SAVE_FLAG_HOOK in migration.h
> with all the other RAM_SAVE_FLAG defines ? It looks like the two
> places that use it already include migration.h...

Do you mean move the rest of the RAM_SAVE_FLAG_* from migration/ram.c
into migration.h?

We could do - although they're never used by anything else.

Dave

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

Re: of AVR target page size

[Peter Maydell]

On Thu, 18 Mar 2021 at 20:05, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Peter Maydell (peter.maydell@linaro.org) wrote:
> > On Thu, 18 Mar 2021 at 10:45, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > >
> > > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > > Also, what does the
> > > >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > > > comment refer to? migration.h has no instances of "RAM_SAVE"
> > > > or 0x80 or 1 << 7...
> > >
> > > It looks like it got moved to qemu-file.h a few years ago
> > > as RAM_SAVE_FLAG_HOOK.
> >
> > Could we put the define of RAM_SAVE_FLAG_HOOK in migration.h
> > with all the other RAM_SAVE_FLAG defines ? It looks like the two
> > places that use it already include migration.h...
>
> Do you mean move the rest of the RAM_SAVE_FLAG_* from migration/ram.c
> into migration.h?
>
> We could do - although they're never used by anything else.

Oh, I'd missed that the other RAM_SAVE_FLAG_* are in a C file.
Yes, I think they would be better in a .h file, so the whole set
of definitions can be together. migration/ram.h seems like a
plausible place.

thanks
-- PMM