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

Mark Cave-Ayland posted 10 patches 3 years, 11 months ago
Maintainers: Laurent Vivier <laurent@vivier.eu>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>
There is a newer version of this series
[PATCH v2 03/10] macfb: increase number of registers saved in MacfbState
Posted by Mark Cave-Ayland 3 years, 11 months ago
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))
 
 typedef struct MacfbState {
     MemoryRegion mem_vram;
-- 
2.20.1
Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState
Posted by Peter Maydell 3 years, 11 months ago
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>

thanks
-- PMM
Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState
Posted by Peter Maydell 3 years, 11 months ago
On Thu, 3 Mar 2022 at 15:25, Peter Maydell <peter.maydell@linaro.org> 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>

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

...where by "previous" I mean patch 1.

-- PMM
Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState
Posted by Mark Cave-Ayland 3 years, 11 months ago
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.


ATB,

Mark.
Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState
Posted by Peter Maydell 3 years, 11 months ago
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
Re: [PATCH v2 03/10] macfb: increase number of registers saved in MacfbState
Posted by Laurent Vivier 3 years, 11 months ago
Le 02/03/2022 à 22:27, Mark Cave-Ayland a écrit :
> 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(-)
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>