* of AVR target page size @ 2021-03-17 20:14 Dr. David Alan Gilbert 2021-03-17 21:32 ` Michael Rolnik 2021-03-17 22:33 ` Peter Maydell 0 siblings, 2 replies; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-17 20:14 UTC (permalink / raw) To: mrolnik; +Cc: philmd, qemu-devel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-17 20:14 of AVR target page size Dr. David Alan Gilbert @ 2021-03-17 21:32 ` Michael Rolnik 2021-03-18 9:55 ` Dr. David Alan Gilbert 2021-03-17 22:33 ` Peter Maydell 1 sibling, 1 reply; 17+ messages in thread From: Michael Rolnik @ 2021-03-17 21:32 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: Philippe Mathieu-Daudé, QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-17 21:32 ` Michael Rolnik @ 2021-03-18 9:55 ` Dr. David Alan Gilbert 2021-03-18 10:15 ` Michael Rolnik 0 siblings, 1 reply; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 9:55 UTC (permalink / raw) To: Michael Rolnik; +Cc: Philippe Mathieu-Daudé, QEMU Developers * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 9:55 ` Dr. David Alan Gilbert @ 2021-03-18 10:15 ` Michael Rolnik 2021-03-18 10:21 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 17+ messages in thread From: Michael Rolnik @ 2021-03-18 10:15 UTC (permalink / raw) To: Dr. David Alan Gilbert; +Cc: Philippe Mathieu-Daudé, QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:15 ` Michael Rolnik @ 2021-03-18 10:21 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 10:21 UTC (permalink / raw) To: Michael Rolnik; +Cc: Philippe Mathieu-Daudé, QEMU Developers * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-17 20:14 of AVR target page size Dr. David Alan Gilbert 2021-03-17 21:32 ` Michael Rolnik @ 2021-03-17 22:33 ` Peter Maydell 2021-03-18 7:37 ` Michael Rolnik 2021-03-18 10:18 ` Dr. David Alan Gilbert 1 sibling, 2 replies; 17+ messages in thread From: Peter Maydell @ 2021-03-17 22:33 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-17 22:33 ` Peter Maydell @ 2021-03-18 7:37 ` Michael Rolnik 2021-03-18 10:18 ` Dr. David Alan Gilbert 1 sibling, 0 replies; 17+ messages in thread From: Michael Rolnik @ 2021-03-18 7:37 UTC (permalink / raw) To: Peter Maydell Cc: Richard Henderson, Philippe Mathieu-Daudé, Dr. David Alan Gilbert, QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-17 22:33 ` Peter Maydell 2021-03-18 7:37 ` Michael Rolnik @ 2021-03-18 10:18 ` Dr. David Alan Gilbert 2021-03-18 10:21 ` Peter Maydell 1 sibling, 1 reply; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 10:18 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers * 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:18 ` Dr. David Alan Gilbert @ 2021-03-18 10:21 ` Peter Maydell 2021-03-18 10:25 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 17+ messages in thread From: Peter Maydell @ 2021-03-18 10:21 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:21 ` Peter Maydell @ 2021-03-18 10:25 ` Dr. David Alan Gilbert 2021-03-18 10:34 ` Peter Maydell 0 siblings, 1 reply; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 10:25 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers * 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:25 ` Dr. David Alan Gilbert @ 2021-03-18 10:34 ` Peter Maydell 2021-03-18 10:45 ` Dr. David Alan Gilbert 0 siblings, 1 reply; 17+ messages in thread From: Peter Maydell @ 2021-03-18 10:34 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:34 ` Peter Maydell @ 2021-03-18 10:45 ` Dr. David Alan Gilbert 2021-03-18 11:03 ` Michael Rolnik 2021-03-18 11:12 ` Peter Maydell 0 siblings, 2 replies; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 10:45 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:45 ` Dr. David Alan Gilbert @ 2021-03-18 11:03 ` Michael Rolnik 2021-03-18 20:03 ` Dr. David Alan Gilbert 2021-03-18 11:12 ` Peter Maydell 1 sibling, 1 reply; 17+ messages in thread From: Michael Rolnik @ 2021-03-18 11:03 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Peter Maydell, Richard Henderson, Philippe Mathieu-Daudé, QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 11:03 ` Michael Rolnik @ 2021-03-18 20:03 ` Dr. David Alan Gilbert 0 siblings, 0 replies; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 20:03 UTC (permalink / raw) To: Michael Rolnik Cc: Peter Maydell, Richard Henderson, Philippe Mathieu-Daudé, QEMU Developers * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 10:45 ` Dr. David Alan Gilbert 2021-03-18 11:03 ` Michael Rolnik @ 2021-03-18 11:12 ` Peter Maydell 2021-03-18 20:05 ` Dr. David Alan Gilbert 1 sibling, 1 reply; 17+ messages in thread From: Peter Maydell @ 2021-03-18 11:12 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 11:12 ` Peter Maydell @ 2021-03-18 20:05 ` Dr. David Alan Gilbert 2021-03-18 20:14 ` Peter Maydell 0 siblings, 1 reply; 17+ messages in thread From: Dr. David Alan Gilbert @ 2021-03-18 20:05 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: of AVR target page size 2021-03-18 20:05 ` Dr. David Alan Gilbert @ 2021-03-18 20:14 ` Peter Maydell 0 siblings, 0 replies; 17+ messages in thread From: Peter Maydell @ 2021-03-18 20:14 UTC (permalink / raw) To: Dr. David Alan Gilbert Cc: Philippe Mathieu-Daudé, Richard Henderson, Michael Rolnik, QEMU Developers 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-03-18 20:16 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-17 20:14 of AVR target page size Dr. David Alan Gilbert 2021-03-17 21:32 ` Michael Rolnik 2021-03-18 9:55 ` Dr. David Alan Gilbert 2021-03-18 10:15 ` Michael Rolnik 2021-03-18 10:21 ` Dr. David Alan Gilbert 2021-03-17 22:33 ` Peter Maydell 2021-03-18 7:37 ` Michael Rolnik 2021-03-18 10:18 ` Dr. David Alan Gilbert 2021-03-18 10:21 ` Peter Maydell 2021-03-18 10:25 ` Dr. David Alan Gilbert 2021-03-18 10:34 ` Peter Maydell 2021-03-18 10:45 ` Dr. David Alan Gilbert 2021-03-18 11:03 ` Michael Rolnik 2021-03-18 20:03 ` Dr. David Alan Gilbert 2021-03-18 11:12 ` Peter Maydell 2021-03-18 20:05 ` Dr. David Alan Gilbert 2021-03-18 20:14 ` Peter Maydell
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).