[PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers

[PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers

[Liav Albani]

Recently I submitted patches to the SerenityOS project in regard to
enhancing the overall abstractions of x86-specific hardware support
code in the SerenityOS kernel in preparation for aarch64 support.
Then, I moved on to submit another patch to introduce support of the
isa-vga device, as we currently allow people to run an ISA-PC machine
with SerenityOS without any GUI (see this link for more details -
https://github.com/SerenityOS/serenity/pull/15259).

This all worked pretty well and with the patches being applied it is
possible to boot into a graphical environment. However, not all things
are perfect (yet). To ensure we only create a driver instance for an
isa-vga device, I try to ensure that PCI was disabled due to failed
hardware test and not because of a user decision, to ensure that we do
not try to drive a PCI stdvga device with an ISA device-targeted HW
parameters. This worked well too, but one problem is left now - I still
had to hardcode the framebuffer address to 0xE0000000 in the driver.

Honestly, it's not a big issue of its own - many devices are assumed to
exist at well-defined physical memory addresses, especially when it is
related to plain old ISA devices. However, I still want to fix this,
for these reasons:
1. Although it is reasonable to assume no more than one isa-vga device
  will exist in one machine, this could be changed easily later on.
  As it stands now, on an ISA-PC machine with no PCI bus, there are
  basically zero methods to detect hardware - you have to assume the
  hardware is there, or just to probe for it and hope for the best.

  Relying on the BIOS to do hardware detection is also a possibility
  as it knows the best upon the platform it being used on.
  In the SerenityOS project we decided for the time being to not use
  the BIOS as that will require doing hacky stuff to use 16-bit code
  segments. Also, the BIOS is not perfect and of course it does not
  give you every little detail you might want, as long as we consider
  the standard services of an x86 BIOS these days.

  For an ISA-PC machine, the assumption of one isa-vga device at
  most is OK and will be true in the future as well.

  However, for other machines, and the one I am especially interested
  in, the microvm machine, this claim could be easily revoked as the
  microvm machine exposes a device tree - we could easily place many
  ISA-VGA devices on the "System bus" of a virtual machine, essentially
  having multiple framebuffer devices on such machine setup, with no PCI
  bus being involved at all. Of course, we will need to figure out how
  to make some sort of an ISA-VGA device that resembles a bochs-display
  device - it should not have VGA capabilities because otherwise the
  devices' resources will be in conflict for VGA control of the VGA IO
  space. The Bochs VBE registers will also need to be located in
  different IO ports too for each device.

  This idea is quite neat in my opinion, because it could speed up the
  boot of such VM while still providing sufficient display capabilities
  for those we need them. It could help developers to test their OSes
  on such hardware setups to ensure multi-monitor configuration works
  reliably when there's no PCI bus at all but many framebuffer devices
  being used in one VM.

2. This is more related to the SerenityOS project - I prefer to not 
  hardcode physical addresses at all wherever I can do so. This makes
  the code cleaner and more understandable as far as I observe this from
  the angle of the person which is not me, that tries to make sense from
  the code flow.

3. The costs of adding this feature are pretty negligible compared to
  the possible value of this, especially if we apply the idea of running
  multiple ISA-VGA devices on one microvm machine. Still, the only major
  "issue" that one can point to is the fact that I bump up the Bochs VBE
  version number, which could be questionable with how the feature might
  be insignificant for many guest OSes out there.

Liav Albani (1):
  hw/display: expose linear framebuffer address in Bochs VBE registers

 hw/display/bochs-display.c     | 10 +++++++++-
 hw/display/vga.c               | 13 +++++++++++--
 include/hw/display/bochs-vbe.h |  7 ++++++-
 3 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.37.3

[PATCH 1/1] hw/display: expose linear framebuffer address in Bochs VBE registers

[Liav Albani]

This is quite useful on the isa-vga device, because it lets guest drivers
to determine where is the framebuffer located in physical memory instead
of blindly hardcoding an address. It also allows future movements of the
framebuffer to other locations.

Signed-off-by: Liav Albani <liavalb@gmail.com>
---
 hw/display/bochs-display.c     | 10 +++++++++-
 hw/display/vga.c               | 13 +++++++++++--
 include/hw/display/bochs-vbe.h |  7 ++++++-
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index 8ed734b195..aa3aa51e2f 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -77,9 +77,17 @@ static uint64_t bochs_display_vbe_read(void *ptr, hwaddr addr,
 
     switch (index) {
     case VBE_DISPI_INDEX_ID:
-        return VBE_DISPI_ID5;
+        return VBE_DISPI_ID6;
     case VBE_DISPI_INDEX_VIDEO_MEMORY_64K:
         return s->vgamem / (64 * KiB);
+    case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS1:
+        return (s->vram.addr) & 0xffff;
+    case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS2:
+        return (s->vram.addr >> 16) & 0xffff;
+    case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS3:
+        return (s->vram.addr >> 32) & 0xffff;
+    case VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS4:
+        return (s->vram.addr >> 48) & 0xffff;
     }
 
     if (index >= ARRAY_SIZE(s->vbe_regs)) {
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 50ecb1ad02..9d91946987 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -727,6 +727,14 @@ uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
         }
     } else if (s->vbe_index == VBE_DISPI_INDEX_VIDEO_MEMORY_64K) {
         val = s->vbe_size / (64 * KiB);
+    } else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS1) {
+        val = (s->vram.addr) & 0xffff;
+    } else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS2) {
+        val = (s->vram.addr >> 16) & 0xffff;
+    } else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS3) {
+        val = (s->vram.addr >> 32) & 0xffff;
+    } else if (s->vbe_index == VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS4) {
+        val = (s->vram.addr >> 48) & 0xffff;
     } else {
         val = 0;
     }
@@ -753,7 +761,8 @@ void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
                 val == VBE_DISPI_ID2 ||
                 val == VBE_DISPI_ID3 ||
                 val == VBE_DISPI_ID4 ||
-                val == VBE_DISPI_ID5) {
+                val == VBE_DISPI_ID5 ||
+                val == VBE_DISPI_ID6) {
                 s->vbe_regs[s->vbe_index] = val;
             }
             break;
@@ -1830,7 +1839,7 @@ void vga_common_reset(VGACommonState *s)
     s->bank_offset = 0;
     s->vbe_index = 0;
     memset(s->vbe_regs, '\0', sizeof(s->vbe_regs));
-    s->vbe_regs[VBE_DISPI_INDEX_ID] = VBE_DISPI_ID5;
+    s->vbe_regs[VBE_DISPI_INDEX_ID] = VBE_DISPI_ID6;
     s->vbe_start_addr = 0;
     s->vbe_line_offset = 0;
     s->vbe_bank_mask = (s->vram_size >> 16) - 1;
diff --git a/include/hw/display/bochs-vbe.h b/include/hw/display/bochs-vbe.h
index bc2f046eee..383474b9d1 100644
--- a/include/hw/display/bochs-vbe.h
+++ b/include/hw/display/bochs-vbe.h
@@ -19,8 +19,12 @@
 #define VBE_DISPI_INDEX_VIRT_HEIGHT     0x7
 #define VBE_DISPI_INDEX_X_OFFSET        0x8
 #define VBE_DISPI_INDEX_Y_OFFSET        0x9
-#define VBE_DISPI_INDEX_NB              0xa /* size of vbe_regs[] */
 #define VBE_DISPI_INDEX_VIDEO_MEMORY_64K 0xa /* read-only, not in vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS1 0xb /* read-only, not in vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS2 0xc /* read-only, not in vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS3 0xd /* read-only, not in vbe_regs */
+#define VBE_DISPI_VIDEO_MEMORY_PHYSICAL_ADDRESS4 0xe /* read-only, not in vbe_regs */
+#define VBE_DISPI_INDEX_NB              0xe /* size of vbe_regs[] */
 
 /* VBE_DISPI_INDEX_ID */
 #define VBE_DISPI_ID0                   0xB0C0
@@ -29,6 +33,7 @@
 #define VBE_DISPI_ID3                   0xB0C3
 #define VBE_DISPI_ID4                   0xB0C4
 #define VBE_DISPI_ID5                   0xB0C5
+#define VBE_DISPI_ID6                   0xB0C6
 
 /* VBE_DISPI_INDEX_ENABLE */
 #define VBE_DISPI_DISABLED              0x00
-- 
2.37.3

Re: [PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers

[Gerd Hoffmann]

  Hi,

> 1. Although it is reasonable to assume no more than one isa-vga device
>   will exist in one machine, this could be changed easily later on.

Nope.  Even if you fix the framebuffer address conflict you still have
the io address conflict.

>   As it stands now, on an ISA-PC machine with no PCI bus, there are
>   basically zero methods to detect hardware - you have to assume the
>   hardware is there, or just to probe for it and hope for the best.

Yep.  That's why isa-pc is pretty much unused these days.

>   However, for other machines, and the one I am especially interested
>   in, the microvm machine, this claim could be easily revoked as the
>   microvm machine exposes a device tree - we could easily place many
>   ISA-VGA devices on the "System bus" of a virtual machine, essentially
>   having multiple framebuffer devices on such machine setup, with no PCI
>   bus being involved at all. Of course, we will need to figure out how
>   to make some sort of an ISA-VGA device that resembles a bochs-display
>   device - it should not have VGA capabilities because otherwise the
>   devices' resources will be in conflict for VGA control of the VGA IO
>   space. The Bochs VBE registers will also need to be located in
>   different IO ports too for each device.

When you want build a sysbus variant of the bochs-display device and
make that discoverable by the guest somehow (dt or acpi) you can expose
both io ports and framebuffer address that way.  No need to touch the
bochs dispi interface for that.

>   This idea is quite neat in my opinion, because it could speed up the
>   boot of such VM while still providing sufficient display capabilities
>   for those we need them. It could help developers to test their OSes
>   on such hardware setups to ensure multi-monitor configuration works
>   reliably when there's no PCI bus at all but many framebuffer devices
>   being used in one VM.

Why not just use virtio-gpu?

> 2. This is more related to the SerenityOS project - I prefer to not 
>   hardcode physical addresses at all wherever I can do so. This makes
>   the code cleaner and more understandable as far as I observe this from
>   the angle of the person which is not me, that tries to make sense from
>   the code flow.

Yea, fully agree, but why continue to use non-discoverable isa bus
devices then?

> 3. The costs of adding this feature are pretty negligible compared to
>   the possible value of this, especially if we apply the idea of running
>   multiple ISA-VGA devices on one microvm machine. Still, the only major
>   "issue" that one can point to is the fact that I bump up the Bochs VBE
>   version number, which could be questionable with how the feature might
>   be insignificant for many guest OSes out there.

Touching isa-vga at this point doesn't make sense at all.  We simply
can't move around the framebuffer without screwing up users.

Also I don't see any actual value in this.  Even considering the
multiple devices case the patch is a partial solution only (handles
the framebuffer but not the ioports) which is pointless.

take care,
  Gerd

Re: [PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers

[Liav Albani]

On 9/21/22 09:14, Gerd Hoffmann wrote:
> Nope.  Even if you fix the framebuffer address conflict you still have
> the io address conflict.
Yeah, that is why I explicitly said that this is needed to be fixed as 
well in later patches.
> Yep.  That's why isa-pc is pretty much unused these days.

Well, I can't say I use it frequently, but I do test it with the 
SerenityOS kernel and it works pretty well.
The SerenityOS kernel is able to drive an isa-vga device just fine after 
my patches were merged yesterday (in the GitHub pull request I provided 
a link to), so I do see that machine type as a valuable test platform.

> When you want build a sysbus variant of the bochs-display device and
> make that discoverable by the guest somehow (dt or acpi) you can expose
> both io ports and framebuffer address that way.  No need to touch the
> bochs dispi interface for that.
This is an interesting idea. A sysbus-bochs-display device might work 
well as you suggested,
instead of using an isa-vga device.
>
>>    This idea is quite neat in my opinion, because it could speed up the
>>    boot of such VM while still providing sufficient display capabilities
>>    for those we need them. It could help developers to test their OSes
>>    on such hardware setups to ensure multi-monitor configuration works
>>    reliably when there's no PCI bus at all but many framebuffer devices
>>    being used in one VM.
> Why not just use virtio-gpu?

Trying to run this command:
qemu-system-x86_64 -M microvm -m 2048 -device virtio-gpu

Results in:
qemu-system-x86_64: -device virtio-gpu: No 'PCI' bus found for device 
'virtio-gpu-pci'

The idea was to not use PCI at all but still to have multiple 
framebuffer devices. So clearly virtio-gpu
is not usable in this situation.

>
>> 2. This is more related to the SerenityOS project - I prefer to not
>>    hardcode physical addresses at all wherever I can do so. This makes
>>    the code cleaner and more understandable as far as I observe this from
>>    the angle of the person which is not me, that tries to make sense from
>>    the code flow.
> Yea, fully agree, but why continue to use non-discoverable isa bus
> devices then?

On the ISA-PC machine, I still want to be able to boot into a graphical 
environment with the SerenityOS kernel. The only option is
to use the isa-vga device, which works OK.
On the microvm machine, it is really not that important if I use the 
isa-vga device or a sysbus-bochs-display device (when I implement that
device).
I just want to support as many x86 platform configurations as possible - 
modern non-PCI machines, ISA-PC machines and regular i440fx/Q35 machines.

>
>> 3. The costs of adding this feature are pretty negligible compared to
>>    the possible value of this, especially if we apply the idea of running
>>    multiple ISA-VGA devices on one microvm machine. Still, the only major
>>    "issue" that one can point to is the fact that I bump up the Bochs VBE
>>    version number, which could be questionable with how the feature might
>>    be insignificant for many guest OSes out there.
> Touching isa-vga at this point doesn't make sense at all.  We simply
> can't move around the framebuffer without screwing up users.
That's an issue I didn't consider, but this is not a real problem on the 
microvm machine
if you use the device tree approach to expose the resources of the 
device, which in some sense makes it unnecessary
to use the bochs dispi interface to expose the framebuffer physical address.
>
> Also I don't see any actual value in this.  Even considering the
> multiple devices case the patch is a partial solution only (handles
> the framebuffer but not the ioports) which is pointless.
Considering the possibility of exposing the framebuffer address within 
the device tree blob, it is indeed not making more value
to go with this approach. I'm still fond of the idea to create a sysbus 
variant of the bochs-display device, so I might work on that instead :)

Best regards,
Liav

Re: [PATCH 0/1] hw/display: expose linear framebuffer address in Bochs VBE registers

[Gerd Hoffmann]

  Hi,

> > Why not just use virtio-gpu?
> 
> Trying to run this command:
> qemu-system-x86_64 -M microvm -m 2048 -device virtio-gpu

'-device virtio-gpu-device'

Might also need '-global virtio-mmio.force-legacy=false' to switch
virtio-mmio into 1.0 mode.

take care,
  Gerd