[Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Gerd Hoffmann]

This patch adds a mmio bar to the qemu standard vga which allows to
access the standard vga registers and bochs dispi interface registers
via mmio.

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/vga-pci.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/vga.c     |    6 ++--
 hw/vga_int.h |    6 +++
 3 files changed, 106 insertions(+), 3 deletions(-)

diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index 9abbada..e05e2ef 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -30,9 +30,36 @@
 #include "qemu-timer.h"
 #include "loader.h"
 
+/*
+ * QEMU Standard VGA -- MMIO area spec.
+ *
+ * Using PCI bar #2, keeping #1 free, which leaves the
+ * door open to upgrade bar #0 to 64bit.
+ *
+ * mmio area layout:
+ *   0x0000 -> 0x03ff  reserved, for possible virtio extension.
+ *   0x0400 -> 0x041f  vga ioports (0x3c0 -> 0x3df), remapped 1:1
+ *   0x0500 -> 0x0515  bochs dispi interface registers, mapped flat without
+ *                     index/data ports.  Use (index << 1) as offset for
+ *                     (16bit) register access.
+ */
+#define PCI_VGA_IOPORT_OFFSET 0x400
+#define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
+#define PCI_VGA_BOCHS_OFFSET  0x500
+#define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
+#define PCI_VGA_MMIO_SIZE     0x1000
+
+enum vga_pci_flags {
+    PCI_VGA_FLAG_ENABLE_MMIO = 1,
+};
+
 typedef struct PCIVGAState {
     PCIDevice dev;
     VGACommonState vga;
+    uint32_t flags;
+    MemoryRegion mmio;
+    MemoryRegion ioport;
+    MemoryRegion bochs;
 } PCIVGAState;
 
 static const VMStateDescription vmstate_vga_pci = {
@@ -47,6 +74,60 @@ static const VMStateDescription vmstate_vga_pci = {
     }
 };
 
+static uint64_t pci_vga_ioport_read(void *ptr, target_phys_addr_t addr,
+                                    unsigned size)
+{
+    PCIVGAState *d = ptr;
+    return vga_ioport_read(&d->vga, addr);
+}
+
+static void pci_vga_ioport_write(void *ptr, target_phys_addr_t addr,
+                                 uint64_t val, unsigned size)
+{
+    PCIVGAState *d = ptr;
+    vga_ioport_write(&d->vga, addr, val);
+}
+
+static const MemoryRegionOps pci_vga_ioport_ops = {
+    .read = pci_vga_ioport_read,
+    .write = pci_vga_ioport_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 1,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t pci_vga_bochs_read(void *ptr, target_phys_addr_t addr,
+                                   unsigned size)
+{
+    PCIVGAState *d = ptr;
+    int index = addr >> 1;
+
+    vbe_ioport_write_index(&d->vga, 0, index);
+    return vbe_ioport_read_data(&d->vga, 0);
+}
+
+static void pci_vga_bochs_write(void *ptr, target_phys_addr_t addr,
+                                uint64_t val, unsigned size)
+{
+    PCIVGAState *d = ptr;
+    int index = addr >> 1;
+
+    vbe_ioport_write_index(&d->vga, 0, index);
+    vbe_ioport_write_data(&d->vga, 0, val);
+}
+
+static const MemoryRegionOps pci_vga_bochs_ops = {
+    .read = pci_vga_bochs_read,
+    .write = pci_vga_bochs_write,
+    .valid.min_access_size = 1,
+    .valid.max_access_size = 4,
+    .impl.min_access_size = 2,
+    .impl.max_access_size = 2,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static int pci_vga_initfn(PCIDevice *dev)
 {
      PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
@@ -62,6 +143,21 @@ static int pci_vga_initfn(PCIDevice *dev)
      /* XXX: VGA_RAM_SIZE must be a power of two */
      pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
 
+     /* mmio bar for vga register access */
+     if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_MMIO)) {
+         memory_region_init(&d->mmio, "vga.mmio", 4096);
+         memory_region_init_io(&d->ioport, &pci_vga_ioport_ops, d,
+                               "vga ioports remapped", PCI_VGA_IOPORT_SIZE);
+         memory_region_init_io(&d->bochs, &pci_vga_bochs_ops, d,
+                               "bochs dispi interface", PCI_VGA_BOCHS_SIZE);
+
+         memory_region_add_subregion(&d->mmio, PCI_VGA_IOPORT_OFFSET,
+                                     &d->ioport);
+         memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
+                                     &d->bochs);
+         pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
+     }
+
      if (!dev->rom_bar) {
          /* compatibility with pc-0.13 and older */
          vga_init_vbe(s, pci_address_space(dev));
@@ -77,6 +173,7 @@ DeviceState *pci_vga_init(PCIBus *bus)
 
 static Property vga_pci_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
+    DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/vga.c b/hw/vga.c
index ec4f0c5..053f89d 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -591,7 +591,7 @@ static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
     return val;
 }
 
-static uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
+uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
 {
     VGACommonState *s = opaque;
     uint32_t val;
@@ -627,13 +627,13 @@ static uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
     return val;
 }
 
-static void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val)
+void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val)
 {
     VGACommonState *s = opaque;
     s->vbe_index = val;
 }
 
-static void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
+void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
 {
     VGACommonState *s = opaque;
 
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 330a32f..5b68490 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -208,7 +208,13 @@ void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
 void ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
 
 int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
+
+#ifdef CONFIG_BOCHS_VBE
 void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
+uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr);
+void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val);
+void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val);
+#endif
 
 extern const uint8_t sr_mask[8];
 extern const uint8_t gr_mask[16];
-- 
1.7.1

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Benjamin Herrenschmidt]

On Tue, 2012-09-18 at 11:51 +0200, Gerd Hoffmann wrote:
> This patch adds a mmio bar to the qemu standard vga which allows to
> access the standard vga registers and bochs dispi interface registers
> via mmio.

I had a patch like that somewhere (or is that it ? :-)

I dropped it in favor of a more interesting approach doing a virtio-vga,
which Anthony and I have been hacking on a bit, but due to time
constraints haven't really finished at this point.

In any case, I'm fine with this patch but does it help anybody ?

Cheers,
Ben.

> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/vga-pci.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/vga.c     |    6 ++--
>  hw/vga_int.h |    6 +++
>  3 files changed, 106 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> index 9abbada..e05e2ef 100644
> --- a/hw/vga-pci.c
> +++ b/hw/vga-pci.c
> @@ -30,9 +30,36 @@
>  #include "qemu-timer.h"
>  #include "loader.h"
>  
> +/*
> + * QEMU Standard VGA -- MMIO area spec.
> + *
> + * Using PCI bar #2, keeping #1 free, which leaves the
> + * door open to upgrade bar #0 to 64bit.
> + *
> + * mmio area layout:
> + *   0x0000 -> 0x03ff  reserved, for possible virtio extension.
> + *   0x0400 -> 0x041f  vga ioports (0x3c0 -> 0x3df), remapped 1:1
> + *   0x0500 -> 0x0515  bochs dispi interface registers, mapped flat without
> + *                     index/data ports.  Use (index << 1) as offset for
> + *                     (16bit) register access.
> + */
> +#define PCI_VGA_IOPORT_OFFSET 0x400
> +#define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
> +#define PCI_VGA_BOCHS_OFFSET  0x500
> +#define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
> +#define PCI_VGA_MMIO_SIZE     0x1000
> +
> +enum vga_pci_flags {
> +    PCI_VGA_FLAG_ENABLE_MMIO = 1,
> +};
> +
>  typedef struct PCIVGAState {
>      PCIDevice dev;
>      VGACommonState vga;
> +    uint32_t flags;
> +    MemoryRegion mmio;
> +    MemoryRegion ioport;
> +    MemoryRegion bochs;
>  } PCIVGAState;
>  
>  static const VMStateDescription vmstate_vga_pci = {
> @@ -47,6 +74,60 @@ static const VMStateDescription vmstate_vga_pci = {
>      }
>  };
>  
> +static uint64_t pci_vga_ioport_read(void *ptr, target_phys_addr_t addr,
> +                                    unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    return vga_ioport_read(&d->vga, addr);
> +}
> +
> +static void pci_vga_ioport_write(void *ptr, target_phys_addr_t addr,
> +                                 uint64_t val, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    vga_ioport_write(&d->vga, addr, val);
> +}
> +
> +static const MemoryRegionOps pci_vga_ioport_ops = {
> +    .read = pci_vga_ioport_read,
> +    .write = pci_vga_ioport_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 1,
> +    .impl.max_access_size = 1,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t pci_vga_bochs_read(void *ptr, target_phys_addr_t addr,
> +                                   unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    int index = addr >> 1;
> +
> +    vbe_ioport_write_index(&d->vga, 0, index);
> +    return vbe_ioport_read_data(&d->vga, 0);
> +}
> +
> +static void pci_vga_bochs_write(void *ptr, target_phys_addr_t addr,
> +                                uint64_t val, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    int index = addr >> 1;
> +
> +    vbe_ioport_write_index(&d->vga, 0, index);
> +    vbe_ioport_write_data(&d->vga, 0, val);
> +}
> +
> +static const MemoryRegionOps pci_vga_bochs_ops = {
> +    .read = pci_vga_bochs_read,
> +    .write = pci_vga_bochs_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 2,
> +    .impl.max_access_size = 2,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
>  static int pci_vga_initfn(PCIDevice *dev)
>  {
>       PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
> @@ -62,6 +143,21 @@ static int pci_vga_initfn(PCIDevice *dev)
>       /* XXX: VGA_RAM_SIZE must be a power of two */
>       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
>  
> +     /* mmio bar for vga register access */
> +     if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_MMIO)) {
> +         memory_region_init(&d->mmio, "vga.mmio", 4096);
> +         memory_region_init_io(&d->ioport, &pci_vga_ioport_ops, d,
> +                               "vga ioports remapped", PCI_VGA_IOPORT_SIZE);
> +         memory_region_init_io(&d->bochs, &pci_vga_bochs_ops, d,
> +                               "bochs dispi interface", PCI_VGA_BOCHS_SIZE);
> +
> +         memory_region_add_subregion(&d->mmio, PCI_VGA_IOPORT_OFFSET,
> +                                     &d->ioport);
> +         memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
> +                                     &d->bochs);
> +         pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> +     }
> +
>       if (!dev->rom_bar) {
>           /* compatibility with pc-0.13 and older */
>           vga_init_vbe(s, pci_address_space(dev));
> @@ -77,6 +173,7 @@ DeviceState *pci_vga_init(PCIBus *bus)
>  
>  static Property vga_pci_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> +    DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/vga.c b/hw/vga.c
> index ec4f0c5..053f89d 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -591,7 +591,7 @@ static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
>      return val;
>  }
>  
> -static uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
> +uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
>  {
>      VGACommonState *s = opaque;
>      uint32_t val;
> @@ -627,13 +627,13 @@ static uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
>      return val;
>  }
>  
> -static void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val)
> +void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val)
>  {
>      VGACommonState *s = opaque;
>      s->vbe_index = val;
>  }
>  
> -static void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
> +void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
>  {
>      VGACommonState *s = opaque;
>  
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 330a32f..5b68490 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -208,7 +208,13 @@ void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
>  void ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
>  
>  int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
> +
> +#ifdef CONFIG_BOCHS_VBE
>  void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
> +uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr);
> +void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val);
> +void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val);
> +#endif
>  
>  extern const uint8_t sr_mask[8];
>  extern const uint8_t gr_mask[16];

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Gerd Hoffmann]

On 09/18/12 12:32, Benjamin Herrenschmidt wrote:
> On Tue, 2012-09-18 at 11:51 +0200, Gerd Hoffmann wrote:
>> This patch adds a mmio bar to the qemu standard vga which allows to
>> access the standard vga registers and bochs dispi interface registers
>> via mmio.
> 
> I had a patch like that somewhere (or is that it ? :-)
> 
> I dropped it in favor of a more interesting approach doing a virtio-vga,
> which Anthony and I have been hacking on a bit, but due to time
> constraints haven't really finished at this point.

Yea, has been quiet on this front for a while, thats why I looked into this.

> In any case, I'm fine with this patch but does it help anybody ?

Well, it gives you time to finish virtio-vga ;)

I have a linux kernel driver too, although not kms/drm but fbdev.

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Avi Kivity]

On 09/18/2012 12:51 PM, Gerd Hoffmann wrote:
> This patch adds a mmio bar to the qemu standard vga which allows to
> access the standard vga registers and bochs dispi interface registers
> via mmio.
> 
> diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> index 9abbada..e05e2ef 100644
> --- a/hw/vga-pci.c
> +++ b/hw/vga-pci.c
> @@ -30,9 +30,36 @@
>  #include "qemu-timer.h"
>  #include "loader.h"
>  
> +/*
> + * QEMU Standard VGA -- MMIO area spec.
> + *
> + * Using PCI bar #2, keeping #1 free, which leaves the
> + * door open to upgrade bar #0 to 64bit.
> + *
> + * mmio area layout:
> + *   0x0000 -> 0x03ff  reserved, for possible virtio extension.
> + *   0x0400 -> 0x041f  vga ioports (0x3c0 -> 0x3df), remapped 1:1

Do they support word accesses to set both index and data?

> + *   0x0500 -> 0x0515  bochs dispi interface registers, mapped flat without
> + *                     index/data ports.  Use (index << 1) as offset for
> + *                     (16bit) register access.
> + */

BAR should disappear with -M old.

> +
> +static const MemoryRegionOps pci_vga_ioport_ops = {
> +    .read = pci_vga_ioport_read,
> +    .write = pci_vga_ioport_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 1,
> +    .impl.max_access_size = 1,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};

Looks like word writes are supported provided the memory API breaks up
writes in little endian order.  Better to make it explicit.



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

>> + *   0x0400 -> 0x041f  vga ioports (0x3c0 -> 0x3df), remapped 1:1
> 
> Do they support word accesses to set both index and data?
> 
>> + *   0x0500 -> 0x0515  bochs dispi interface registers, mapped flat without
>> + *                     index/data ports.  Use (index << 1) as offset for
>> + *                     (16bit) register access.
>> + */
> 
> BAR should disappear with -M old.

Sure.  I have a patch in flight which adds the pc-1.3 machine type, once
this is in I can easily add the compat stuff.

>> +static const MemoryRegionOps pci_vga_ioport_ops = {
>> +    .read = pci_vga_ioport_read,
>> +    .write = pci_vga_ioport_write,
>> +    .valid.min_access_size = 1,
>> +    .valid.max_access_size = 4,
>> +    .impl.min_access_size = 1,
>> +    .impl.max_access_size = 1,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
> 
> Looks like word writes are supported provided the memory API breaks up
> writes in little endian order.  Better to make it explicit.

Like the attached incremental patch?

cheers,
  Gerd


[-- Attachment #2: 0001-fixup-vga-mmio.patch --]
[-- Type: text/plain, Size: 1752 bytes --]

>From d46b14dfd74dcc37fe187dc76cd681ad7dbff2d5 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Wed, 19 Sep 2012 13:31:04 +0200
Subject: [PATCH] [fixup] vga mmio

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/vga-pci.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index e05e2ef..7fd305d 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -78,14 +78,33 @@ static uint64_t pci_vga_ioport_read(void *ptr, target_phys_addr_t addr,
                                     unsigned size)
 {
     PCIVGAState *d = ptr;
-    return vga_ioport_read(&d->vga, addr);
+    uint64_t ret = 0;
+
+    switch (size) {
+    case 1:
+        ret = vga_ioport_read(&d->vga, addr);
+        break;
+    case 2:
+        ret  = vga_ioport_read(&d->vga, addr);
+        ret |= vga_ioport_read(&d->vga, addr+1) << 8;
+        break;
+    }
+    return ret;
 }
 
 static void pci_vga_ioport_write(void *ptr, target_phys_addr_t addr,
                                  uint64_t val, unsigned size)
 {
     PCIVGAState *d = ptr;
-    vga_ioport_write(&d->vga, addr, val);
+    switch (size) {
+    case 1:
+        vga_ioport_write(&d->vga, addr, val);
+        break;
+    case 2:
+        vga_ioport_write(&d->vga, addr, val & 0xff);
+        vga_ioport_write(&d->vga, addr+1, (val >> 8) & 0xff);
+        break;
+    }
 }
 
 static const MemoryRegionOps pci_vga_ioport_ops = {
@@ -94,7 +113,7 @@ static const MemoryRegionOps pci_vga_ioport_ops = {
     .valid.min_access_size = 1,
     .valid.max_access_size = 4,
     .impl.min_access_size = 1,
-    .impl.max_access_size = 1,
+    .impl.max_access_size = 2,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-- 
1.7.1

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Avi Kivity]

On 09/19/2012 02:35 PM, Gerd Hoffmann wrote:

>> Looks like word writes are supported provided the memory API breaks up
>> writes in little endian order.  Better to make it explicit.
> 
> Like the attached incremental patch?

Very like.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Blue Swirl]

On Tue, Sep 18, 2012 at 9:51 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> This patch adds a mmio bar to the qemu standard vga which allows to
> access the standard vga registers and bochs dispi interface registers
> via mmio.
>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/vga-pci.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/vga.c     |    6 ++--
>  hw/vga_int.h |    6 +++
>  3 files changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/hw/vga-pci.c b/hw/vga-pci.c
> index 9abbada..e05e2ef 100644
> --- a/hw/vga-pci.c
> +++ b/hw/vga-pci.c
> @@ -30,9 +30,36 @@
>  #include "qemu-timer.h"
>  #include "loader.h"
>
> +/*
> + * QEMU Standard VGA -- MMIO area spec.
> + *
> + * Using PCI bar #2, keeping #1 free, which leaves the
> + * door open to upgrade bar #0 to 64bit.
> + *
> + * mmio area layout:
> + *   0x0000 -> 0x03ff  reserved, for possible virtio extension.
> + *   0x0400 -> 0x041f  vga ioports (0x3c0 -> 0x3df), remapped 1:1
> + *   0x0500 -> 0x0515  bochs dispi interface registers, mapped flat without
> + *                     index/data ports.  Use (index << 1) as offset for
> + *                     (16bit) register access.
> + */
> +#define PCI_VGA_IOPORT_OFFSET 0x400
> +#define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
> +#define PCI_VGA_BOCHS_OFFSET  0x500
> +#define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
> +#define PCI_VGA_MMIO_SIZE     0x1000
> +
> +enum vga_pci_flags {
> +    PCI_VGA_FLAG_ENABLE_MMIO = 1,
> +};
> +
>  typedef struct PCIVGAState {
>      PCIDevice dev;
>      VGACommonState vga;
> +    uint32_t flags;
> +    MemoryRegion mmio;
> +    MemoryRegion ioport;
> +    MemoryRegion bochs;
>  } PCIVGAState;
>
>  static const VMStateDescription vmstate_vga_pci = {
> @@ -47,6 +74,60 @@ static const VMStateDescription vmstate_vga_pci = {
>      }
>  };
>
> +static uint64_t pci_vga_ioport_read(void *ptr, target_phys_addr_t addr,
> +                                    unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    return vga_ioport_read(&d->vga, addr);
> +}
> +
> +static void pci_vga_ioport_write(void *ptr, target_phys_addr_t addr,
> +                                 uint64_t val, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    vga_ioport_write(&d->vga, addr, val);
> +}
> +
> +static const MemoryRegionOps pci_vga_ioport_ops = {
> +    .read = pci_vga_ioport_read,
> +    .write = pci_vga_ioport_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 1,
> +    .impl.max_access_size = 1,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static uint64_t pci_vga_bochs_read(void *ptr, target_phys_addr_t addr,
> +                                   unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    int index = addr >> 1;
> +
> +    vbe_ioport_write_index(&d->vga, 0, index);
> +    return vbe_ioport_read_data(&d->vga, 0);

These functions are only available with CONFIG_BOCHS_VBE #defined, so
this code should be conditional as well.

But building without CONFIG_BOCHS_VBE is not very useful since it's
used by the BIOS and there's no display output without it IIRC.

> +}
> +
> +static void pci_vga_bochs_write(void *ptr, target_phys_addr_t addr,
> +                                uint64_t val, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +    int index = addr >> 1;
> +
> +    vbe_ioport_write_index(&d->vga, 0, index);
> +    vbe_ioport_write_data(&d->vga, 0, val);
> +}
> +
> +static const MemoryRegionOps pci_vga_bochs_ops = {
> +    .read = pci_vga_bochs_read,
> +    .write = pci_vga_bochs_write,
> +    .valid.min_access_size = 1,
> +    .valid.max_access_size = 4,
> +    .impl.min_access_size = 2,
> +    .impl.max_access_size = 2,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
>  static int pci_vga_initfn(PCIDevice *dev)
>  {
>       PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
> @@ -62,6 +143,21 @@ static int pci_vga_initfn(PCIDevice *dev)
>       /* XXX: VGA_RAM_SIZE must be a power of two */
>       pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
>
> +     /* mmio bar for vga register access */
> +     if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_MMIO)) {
> +         memory_region_init(&d->mmio, "vga.mmio", 4096);
> +         memory_region_init_io(&d->ioport, &pci_vga_ioport_ops, d,
> +                               "vga ioports remapped", PCI_VGA_IOPORT_SIZE);
> +         memory_region_init_io(&d->bochs, &pci_vga_bochs_ops, d,
> +                               "bochs dispi interface", PCI_VGA_BOCHS_SIZE);

Also this region should only be available with CONFIG_BOCHS_VBE.

> +
> +         memory_region_add_subregion(&d->mmio, PCI_VGA_IOPORT_OFFSET,
> +                                     &d->ioport);
> +         memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
> +                                     &d->bochs);
> +         pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
> +     }
> +
>       if (!dev->rom_bar) {
>           /* compatibility with pc-0.13 and older */
>           vga_init_vbe(s, pci_address_space(dev));
> @@ -77,6 +173,7 @@ DeviceState *pci_vga_init(PCIBus *bus)
>
>  static Property vga_pci_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> +    DEFINE_PROP_BIT("mmio", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_MMIO, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/vga.c b/hw/vga.c
> index ec4f0c5..053f89d 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -591,7 +591,7 @@ static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
>      return val;
>  }
>
> -static uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
> +uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
>  {
>      VGACommonState *s = opaque;
>      uint32_t val;
> @@ -627,13 +627,13 @@ static uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr)
>      return val;
>  }
>
> -static void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val)
> +void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val)
>  {
>      VGACommonState *s = opaque;
>      s->vbe_index = val;
>  }
>
> -static void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
> +void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val)
>  {
>      VGACommonState *s = opaque;
>
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 330a32f..5b68490 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -208,7 +208,13 @@ void vga_invalidate_scanlines(VGACommonState *s, int y1, int y2);
>  void ppm_save(const char *filename, struct DisplaySurface *ds, Error **errp);
>
>  int vga_ioport_invalid(VGACommonState *s, uint32_t addr);
> +
> +#ifdef CONFIG_BOCHS_VBE
>  void vga_init_vbe(VGACommonState *s, MemoryRegion *address_space);
> +uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr);
> +void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val);
> +void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val);
> +#endif
>
>  extern const uint8_t sr_mask[8];
>  extern const uint8_t gr_mask[16];
> --
> 1.7.1
>
>

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Gerd Hoffmann]

  Hi,

>> +    vbe_ioport_write_index(&d->vga, 0, index);
>> +    return vbe_ioport_read_data(&d->vga, 0);
> 
> These functions are only available with CONFIG_BOCHS_VBE #defined, so
> this code should be conditional as well.
> 
> But building without CONFIG_BOCHS_VBE is not very useful since it's
> used by the BIOS and there's no display output without it IIRC.

Well, text mode is still there, but no (by modern standards) useful
graphics modes, only standard vga ones (i.e. up to 800x600 @ 256 colors
or something like that).

I guess it is better to just remove CONFIG_BOCHS_VBE, /me goes prepare a
patch.

cheers,
  Gerd

Re: [Qemu-devel] [RfC PATCH] vga: add mmio bar to standard vga

[Blue Swirl]

On Thu, Sep 20, 2012 at 5:43 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>>> +    vbe_ioport_write_index(&d->vga, 0, index);
>>> +    return vbe_ioport_read_data(&d->vga, 0);
>>
>> These functions are only available with CONFIG_BOCHS_VBE #defined, so
>> this code should be conditional as well.
>>
>> But building without CONFIG_BOCHS_VBE is not very useful since it's
>> used by the BIOS and there's no display output without it IIRC.
>
> Well, text mode is still there, but no (by modern standards) useful
> graphics modes, only standard vga ones (i.e. up to 800x600 @ 256 colors
> or something like that).
>
> I guess it is better to just remove CONFIG_BOCHS_VBE, /me goes prepare a
> patch.

Bochs support could be disabled with a property, but again the value
is probably low.

>
> cheers,
>   Gerd
>