Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState

[Peter Maydell]

On Thu, 3 Mar 2022 at 17:44, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 03/03/2022 15:25, Peter Maydell wrote:
>
> > On Wed, 2 Mar 2022 at 21:31, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> The MacOS toolbox ROM accesses a number of addresses between 0x0 and 0x200 
> >> during
> >> initialisation and resolution changes. Whilst the function of many of these
> >> registers is unknown, it is worth the minimal cost of saving these extra 
> >> values as
> >> part of migration to help future-proof the migration stream for the q800 
> >> machine
> >> as it starts to stabilise.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>   hw/display/macfb.c         | 8 ++++++++
> >>   include/hw/display/macfb.h | 3 ++-
> >>   2 files changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> >> index fb54b460c1..dfdae90144 100644
> >> --- a/hw/display/macfb.c
> >> +++ b/hw/display/macfb.c
> >> @@ -537,6 +537,10 @@ static uint64_t macfb_ctrl_read(void *opaque,
> >>       case DAFB_MODE_SENSE:
> >>           val = macfb_sense_read(s);
> >>           break;
> >> +    default:
> >> +        if (addr < MACFB_CTRL_TOPADDR) {
> >> +            val = s->regs[addr >> 2];
> >> +        }
> >>       }
> >>
> >>       trace_macfb_ctrl_read(addr, val, size);
> >> @@ -592,6 +596,10 @@ static void macfb_ctrl_write(void *opaque,
> >>               macfb_invalidate_display(s);
> >>           }
> >>           break;
> >> +    default:
> >> +        if (addr < MACFB_CTRL_TOPADDR) {
> >> +            s->regs[addr >> 2] = val;
> >> +        }
> >>       }
> >>
> >>       trace_macfb_ctrl_write(addr, val, size);
> >> diff --git a/include/hw/display/macfb.h b/include/hw/display/macfb.h
> >> index 6d9f0f7869..55a50d3fb0 100644
> >> --- a/include/hw/display/macfb.h
> >> +++ b/include/hw/display/macfb.h
> >> @@ -48,7 +48,8 @@ typedef struct MacFbMode {
> >>       uint32_t offset;
> >>   } MacFbMode;
> >>
> >> -#define MACFB_NUM_REGS      8
> >> +#define MACFB_CTRL_TOPADDR  0x200
> >> +#define MACFB_NUM_REGS      (MACFB_CTRL_TOPADDR / sizeof(uint32_t))
> >
> > You should either bump the vmstate_macfb version numbers here,
> > or at least note in the commit message that although it's a
> > migration break we know nobody's migrating this device because
> > of the bug we just fixed in the previous commit.
> >
> > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> I can do this if you like, although until the last 2 patches anything that is 
> using
> the disk will fail, and that's just about everything because DMA requests 
> require
> guest support to move the data from the ESP to the CPU.
>
> In terms of the q800 machine there is an implicit assumption that there will 
> be more
> migration breaks to come, mainly because there are new devices to be added to 
> the
> q800 machine in my outstanding MacOS patches that will break migration again 
> once. So
> until these are finally merged I don't think it's worth trying to stabilise 
> the
> migration stream.

Yeah, fair enough; just put a note in the commit message to
that effect.  (Mostly bumping the migration version is about making
the error message nicer if somebody does do a mismatched save/load.)

thanks
-- PMM