[PATCH v2 0/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[PATCH v2 0/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Philippe Mathieu-Daudé]

Since v1: Split in 2 distinct patches

Philippe Mathieu-Daudé (2):
  hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()
  hw/display/vga: Remove pointless VGACommonState::default_endian_fb

 hw/display/vga_int.h | 1 -
 hw/display/vga.c     | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

-- 
2.45.2

[PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Philippe Mathieu-Daudé]

The 'pci-vga' device allow setting a 'big-endian-framebuffer'
property since commit 3c2784fc864 ("vga: Expose framebuffer
byteorder as a QOM property"). Similarly, the 'virtio-vga'
device since commit 8be61ce2ce3 ("virtio-vga: implement
big-endian-framebuffer property").

Both call vga_common_reset() in their reset handler, respectively
pci_secondary_vga_reset() and virtio_vga_base_reset_hold(), which
reset 'big_endian_fb', overwritting the property. This is not
correct: the hardware is expected to keep its configured
endianness during resets.

Move 'big_endian_fb' assignment from vga_common_reset() to
vga_common_init() which is called once when the common VGA state
is initialized.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/vga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 892fedc8dce..b074b58c90d 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1873,7 +1873,6 @@ void vga_common_reset(VGACommonState *s)
     s->cursor_start = 0;
     s->cursor_end = 0;
     s->cursor_offset = 0;
-    s->big_endian_fb = s->default_endian_fb;
     memset(s->invalidated_y_table, '\0', sizeof(s->invalidated_y_table));
     memset(s->last_palette, '\0', sizeof(s->last_palette));
     memset(s->last_ch_attr, '\0', sizeof(s->last_ch_attr));
@@ -2266,6 +2265,7 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
      * all target endian dependencies from this file.
      */
     s->default_endian_fb = target_words_bigendian();
+    s->big_endian_fb = s->default_endian_fb;
 
     vga_dirty_log_start(s);
 
-- 
2.45.2

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Philippe Mathieu-Daudé]

ping?

On 29/11/24 11:17, Philippe Mathieu-Daudé wrote:
> The 'pci-vga' device allow setting a 'big-endian-framebuffer'
> property since commit 3c2784fc864 ("vga: Expose framebuffer
> byteorder as a QOM property"). Similarly, the 'virtio-vga'
> device since commit 8be61ce2ce3 ("virtio-vga: implement
> big-endian-framebuffer property").
> 
> Both call vga_common_reset() in their reset handler, respectively
> pci_secondary_vga_reset() and virtio_vga_base_reset_hold(), which
> reset 'big_endian_fb', overwritting the property. This is not
> correct: the hardware is expected to keep its configured
> endianness during resets.
> 
> Move 'big_endian_fb' assignment from vga_common_reset() to
> vga_common_init() which is called once when the common VGA state
> is initialized.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/display/vga.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 892fedc8dce..b074b58c90d 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1873,7 +1873,6 @@ void vga_common_reset(VGACommonState *s)
>       s->cursor_start = 0;
>       s->cursor_end = 0;
>       s->cursor_offset = 0;
> -    s->big_endian_fb = s->default_endian_fb;
>       memset(s->invalidated_y_table, '\0', sizeof(s->invalidated_y_table));
>       memset(s->last_palette, '\0', sizeof(s->last_palette));
>       memset(s->last_ch_attr, '\0', sizeof(s->last_ch_attr));
> @@ -2266,6 +2265,7 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
>        * all target endian dependencies from this file.
>        */
>       s->default_endian_fb = target_words_bigendian();
> +    s->big_endian_fb = s->default_endian_fb;
>   
>       vga_dirty_log_start(s);
>   

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Benjamin Herrenschmidt]

It's been a long time, I only have vague memories of this :-) But I
think it should be ok.

It does definitely make sense in the virtio case and similar where
the property is set once for the instance.

For bochs and ati, there's a register to configure it as well, so there
*may* be an expectation that it gets reset there, I'm less certain.

So tentatively...

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>


On Mon, 2024-12-02 at 20:48 +0100, Philippe Mathieu-Daudé wrote:
> ping?
> 
> On 29/11/24 11:17, Philippe Mathieu-Daudé wrote:
> > The 'pci-vga' device allow setting a 'big-endian-framebuffer'
> > property since commit 3c2784fc864 ("vga: Expose framebuffer
> > byteorder as a QOM property"). Similarly, the 'virtio-vga'
> > device since commit 8be61ce2ce3 ("virtio-vga: implement
> > big-endian-framebuffer property").
> > 
> > Both call vga_common_reset() in their reset handler, respectively
> > pci_secondary_vga_reset() and virtio_vga_base_reset_hold(), which
> > reset 'big_endian_fb', overwritting the property. This is not
> > correct: the hardware is expected to keep its configured
> > endianness during resets.
> > 
> > Move 'big_endian_fb' assignment from vga_common_reset() to
> > vga_common_init() which is called once when the common VGA state
> > is initialized.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   hw/display/vga.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/vga.c b/hw/display/vga.c
> > index 892fedc8dce..b074b58c90d 100644
> > --- a/hw/display/vga.c
> > +++ b/hw/display/vga.c
> > @@ -1873,7 +1873,6 @@ void vga_common_reset(VGACommonState *s)
> >       s->cursor_start = 0;
> >       s->cursor_end = 0;
> >       s->cursor_offset = 0;
> > -    s->big_endian_fb = s->default_endian_fb;
> >       memset(s->invalidated_y_table, '\0', sizeof(s-
> > >invalidated_y_table));
> >       memset(s->last_palette, '\0', sizeof(s->last_palette));
> >       memset(s->last_ch_attr, '\0', sizeof(s->last_ch_attr));
> > @@ -2266,6 +2265,7 @@ bool vga_common_init(VGACommonState *s,
> > Object *obj, Error **errp)
> >        * all target endian dependencies from this file.
> >        */
> >       s->default_endian_fb = target_words_bigendian();
> > +    s->big_endian_fb = s->default_endian_fb;
> >   
> >       vga_dirty_log_start(s);
> >   

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Gerd Hoffmann]

  Hi,

> For bochs and ati, there's a register to configure it as well, so there
> *may* be an expectation that it gets reset there, I'm less certain.

The default is there for backward compatibility reasons and it should
default to machine byte order so you get something working even in case
the guest does not explicitly set the byte order via register.  Which
should be increasingly rare these days, the register was added in
(checking git log) 2014: commit b5682aa4ca79 ("vga-pci: add qext region
to mmio")

The only case I'm aware of where the byte order is actually switched is
booting a ppc64le guest in a pseries machine, where the opal firmware
runs in bigendian mode and the linux kernel runs in little endian mode.

So here the changed reset behavior could actually make a difference, but
you will only notice if the opal firmware does *not* set the byte order
register.

take care,
  Gerd

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[BALATON Zoltan]

On Tue, 3 Dec 2024, Gerd Hoffmann wrote:
>  Hi,
>
>> For bochs and ati, there's a register to configure it as well, so there
>> *may* be an expectation that it gets reset there, I'm less certain.
>
> The default is there for backward compatibility reasons and it should
> default to machine byte order so you get something working even in case
> the guest does not explicitly set the byte order via register.  Which
> should be increasingly rare these days, the register was added in
> (checking git log) 2014: commit b5682aa4ca79 ("vga-pci: add qext region
> to mmio")
>
> The only case I'm aware of where the byte order is actually switched is
> booting a ppc64le guest in a pseries machine, where the opal firmware
> runs in bigendian mode and the linux kernel runs in little endian mode.
>
> So here the changed reset behavior could actually make a difference, but
> you will only notice if the opal firmware does *not* set the byte order
> register.

I'm not sure about ati-vga but I set the vga.big_endian_fb on mode switch 
if the guest uses big endian mode. I've added this for MacOS 9 I think 
which does use that. I've also put some notes on what I've found different 
drivers use in commit 8bb9a2b26d83a. AFAIK the ati chip starts as little 
endian so maybe reseting it in ati_vga_reset() if vga_common_reset() does 
not do that any more may be needed but I can't tell for sure that's why I 
did not comment on this patch so far.

Regards,
BALATON Zoltan

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Benjamin Herrenschmidt]

On Tue, 2024-12-03 at 11:51 +0100, Gerd Hoffmann wrote:
> 
> The only case I'm aware of where the byte order is actually switched is
> booting a ppc64le guest in a pseries machine, where the opal firmware
> runs in bigendian mode and the linux kernel runs in little endian mode.
> 
> So here the changed reset behavior could actually make a difference, but
> you will only notice if the opal firmware does *not* set the byte order
> register.

OPAL (well skiboot) doesn't display anything anyways (or at least it
didn't when I wrote it :-). It just boots Linux as a bootloader. So as
long as Linux itself sets the register it should be fine.

Cheers,
Ben.

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Gerd Hoffmann]

On Fri, Dec 06, 2024 at 10:00:37AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2024-12-03 at 11:51 +0100, Gerd Hoffmann wrote:
> > 
> > The only case I'm aware of where the byte order is actually switched is
> > booting a ppc64le guest in a pseries machine, where the opal firmware
> > runs in bigendian mode and the linux kernel runs in little endian mode.
> > 
> > So here the changed reset behavior could actually make a difference, but
> > you will only notice if the opal firmware does *not* set the byte order
> > register.
> 
> OPAL (well skiboot) doesn't display anything anyways (or at least it
> didn't when I wrote it :-). It just boots Linux as a bootloader. So as
> long as Linux itself sets the register it should be fine.

Oh, mixed up the firmware names, it's SLOF not OPAL.

sorry for the confusion,
  Gerd

Re: [PATCH-for-9.2? v2 1/2] hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset()

[Benjamin Herrenschmidt]

On Fri, 2024-12-06 at 10:28 +0100, Gerd Hoffmann wrote:
> > 
> > OPAL (well skiboot) doesn't display anything anyways (or at least
> > it
> > didn't when I wrote it :-). It just boots Linux as a bootloader. So
> > as
> > long as Linux itself sets the register it should be fine.
> 
> Oh, mixed up the firmware names, it's SLOF not OPAL.
> 
> sorry for the confusion,

Looking at SLOF source, it doesn't know about the endian register at
all indeed, it also doesn't put an endian propery in the device-node,
and probably expects the fb to be BE on reboot. Gosh I also wrote that
code... a looong time ago, I had forgotten all about it.

On SPAPR where endian *is* switched, qemu will also switch the endian
of the framebuffer on an explicit mode switch via the H_SET_MODE
hypercall. But afaik that does NOT include a reboot (at least from 10mn
browsing the code).

So I think the patch will have the effect of breaking the framebuffer
in SLOF on reboot when using SPAPR with an LE OS.

Something like this might work around it:

--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1737,6 +1737,9 @@ static void spapr_machine_reset(MachineState *machine, ResetType type)
 
     spapr_clear_pending_events(spapr);
 
+    /* Switch VGA to big endian */
+    spapr_pci_switch_vga(spapr, true);
+
     /*
      * We place the device tree just below either the top of the RMA,
      * or just below 2GB, whichever is lower, so that it can be

I can't test right now, I might later this week, ping me if you don't
hear from me.

Cheers,
Ben.

[PATCH v2 2/2] hw/display/vga: Remove pointless VGACommonState::default_endian_fb

[Philippe Mathieu-Daudé]

'default_endian_fb' value is always target_words_bigendian(),
remove it as it isn't very useful.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/vga_int.h | 1 -
 hw/display/vga.c     | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index f77c1c11457..5dcdecfd422 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -135,7 +135,6 @@ typedef struct VGACommonState {
     bool full_update_text;
     bool full_update_gfx;
     bool big_endian_fb;
-    bool default_endian_fb;
     bool global_vmstate;
     /* hardware mouse cursor support */
     uint32_t invalidated_y_table[VGA_MAX_HEIGHT / 32];
diff --git a/hw/display/vga.c b/hw/display/vga.c
index b074b58c90d..6dbbbf49073 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2116,7 +2116,7 @@ static bool vga_endian_state_needed(void *opaque)
      * default one, thus ensuring backward compatibility for
      * migration of the common case
      */
-    return s->default_endian_fb != s->big_endian_fb;
+    return s->big_endian_fb != target_words_bigendian();
 }
 
 static const VMStateDescription vmstate_vga_endian = {
@@ -2264,8 +2264,7 @@ bool vga_common_init(VGACommonState *s, Object *obj, Error **errp)
      * into a device attribute set by the machine/platform to remove
      * all target endian dependencies from this file.
      */
-    s->default_endian_fb = target_words_bigendian();
-    s->big_endian_fb = s->default_endian_fb;
+    s->big_endian_fb = target_words_bigendian();
 
     vga_dirty_log_start(s);
 
-- 
2.45.2