| Message ID | 20241129101721.17836-2-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/display/vga: Do not reset 'big_endian_fb' in vga_common_reset() | expand |
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); >
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); > >
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
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
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.
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
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.
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);
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(-)