Re: [Qemu-devel] [PATCH 3/3] vga-pci: add qext region to mmio

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Sep 23, 2014 at 02:30:57PM +0200, Gerd Hoffmann wrote:
> Add a qemu extented register range to the standard vga mmio bar.
> Right nowe there are two registers:  One readonly register returning the
> size of the region (so we can easily add more registers there if needed)
> and one endian control register, so guests (especially ppc) can flip
> the framebuffer endianness as they need it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  docs/specs/standard-vga.txt |  9 ++++++
>  hw/display/vga-pci.c        | 68 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h        |  8 ++++++
>  3 files changed, 85 insertions(+)
> 
> diff --git a/docs/specs/standard-vga.txt b/docs/specs/standard-vga.txt
> index f82773e..19d2a74 100644
> --- a/docs/specs/standard-vga.txt
> +++ b/docs/specs/standard-vga.txt
> @@ -70,3 +70,12 @@ Likewise applies to the pci variant only for obvious reasons.
>  0500 - 0515 : bochs dispi interface registers, mapped flat
>                without index/data ports.  Use (index << 1)
>                as offset for (16bit) register access.
> +
> +0600 - 0607 : qemu extended registers.  qemu 2.2+ only.
> +              The pci revision is 2 (or greater) when
> +              these registers are present.  The registers
> +              are 32bit.
> +  0600      : qemu extended register region size, in bytes.
> +  0604      : framebuffer endianness register.
> +              - 0xbebebebe indicates big endian.
> +              - 0x1e1e1e1e indicates little endian.
> diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
> index 0351d94..3394ec2 100644
> --- a/hw/display/vga-pci.c
> +++ b/hw/display/vga-pci.c
> @@ -35,10 +35,18 @@
>  #define PCI_VGA_IOPORT_SIZE   (0x3e0 - 0x3c0)
>  #define PCI_VGA_BOCHS_OFFSET  0x500
>  #define PCI_VGA_BOCHS_SIZE    (0x0b * 2)
> +#define PCI_VGA_QEXT_OFFSET   0x600
> +#define PCI_VGA_QEXT_SIZE     (2 * 4)
>  #define PCI_VGA_MMIO_SIZE     0x1000
>  
> +#define PCI_VGA_QEXT_REG_SIZE         (0 * 4)
> +#define PCI_VGA_QEXT_REG_BYTEORDER    (1 * 4)
> +#define  PCI_VGA_QEXT_LITTLE_ENDIAN   0x1e1e1e1e
> +#define  PCI_VGA_QEXT_BIG_ENDIAN      0xbebebebe
> +
>  enum vga_pci_flags {
>      PCI_VGA_FLAG_ENABLE_MMIO = 1,
> +    PCI_VGA_FLAG_ENABLE_QEXT = 2,
>  };
>  
>  typedef struct PCIVGAState {
> @@ -48,6 +56,7 @@ typedef struct PCIVGAState {
>      MemoryRegion mmio;
>      MemoryRegion ioport;
>      MemoryRegion bochs;
> +    MemoryRegion qext;
>  } PCIVGAState;
>  
>  static const VMStateDescription vmstate_vga_pci = {
> @@ -140,6 +149,46 @@ static const MemoryRegionOps pci_vga_bochs_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static uint64_t pci_vga_qext_read(void *ptr, hwaddr addr, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +
> +    switch (addr) {
> +    case PCI_VGA_QEXT_REG_SIZE:
> +        return PCI_VGA_QEXT_SIZE;
> +    case PCI_VGA_QEXT_REG_BYTEORDER:
> +        return d->vga.big_endian_fb ?
> +            PCI_VGA_QEXT_BIG_ENDIAN : PCI_VGA_QEXT_LITTLE_ENDIAN;
> +    default:
> +        return 0;
> +    }
> +}
> +
> +static void pci_vga_qext_write(void *ptr, hwaddr addr,
> +                               uint64_t val, unsigned size)
> +{
> +    PCIVGAState *d = ptr;
> +
> +    switch (addr) {
> +    case PCI_VGA_QEXT_REG_BYTEORDER:
> +        if (val == PCI_VGA_QEXT_BIG_ENDIAN) {
> +            d->vga.big_endian_fb = true;
> +        }
> +        if (val == PCI_VGA_QEXT_LITTLE_ENDIAN) {
> +            d->vga.big_endian_fb = false;
> +        }
> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps pci_vga_qext_ops = {
> +    .read = pci_vga_qext_read,
> +    .write = pci_vga_qext_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
>  static int pci_std_vga_initfn(PCIDevice *dev)
>  {
>      PCIVGAState *d = DO_UPCAST(PCIVGAState, dev, dev);
> @@ -167,6 +216,15 @@ static int pci_std_vga_initfn(PCIDevice *dev)
>                                      &d->ioport);
>          memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
>                                      &d->bochs);
> +
> +        if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
> +            memory_region_init_io(&d->qext, NULL, &pci_vga_qext_ops, d,
> +                                  "qemu extended regs", PCI_VGA_QEXT_SIZE);
> +            memory_region_add_subregion(&d->mmio, PCI_VGA_QEXT_OFFSET,
> +                                        &d->qext);
> +            pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +        }
> +
>          pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>      }
>  
> @@ -199,6 +257,14 @@ static int pci_secondary_vga_initfn(PCIDevice *dev)
>      memory_region_add_subregion(&d->mmio, PCI_VGA_BOCHS_OFFSET,
>                                  &d->bochs);
>  
> +    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
> +        memory_region_init_io(&d->qext, NULL, &pci_vga_qext_ops, d,
> +                              "qemu extended regs", PCI_VGA_QEXT_SIZE);
> +        memory_region_add_subregion(&d->mmio, PCI_VGA_QEXT_OFFSET,
> +                                    &d->qext);
> +        pci_set_byte(&d->dev.config[PCI_REVISION_ID], 2);
> +    }
> +
>      pci_register_bar(&d->dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &s->vram);
>      pci_register_bar(&d->dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
>  
> @@ -215,11 +281,13 @@ static void pci_secondary_vga_reset(DeviceState *dev)
>  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_BIT("qext", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>  static Property secondary_pci_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> +    DEFINE_PROP_BIT("qext", PCIVGAState, flags, PCI_VGA_FLAG_ENABLE_QEXT, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 77316d5..23acc1f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -306,6 +306,14 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>              .driver   = "intel-hda",\
>              .property = "old_msi_addr",\
>              .value    = "on",\
> +        },{\
> +            .driver   = "VGA",\
> +            .property = "qext",\
> +            .value    = "off",\

I'd prefer a more friendly name like "qemu-extended-registers".
There's very little documentation for properties besides
the name, so the name has to be explicit.


> +        },{\
> +            .driver   = "secondary-vga",\
> +            .property = "qext",\
> +            .value    = "off",\
>          }
>  
>  #define PC_COMPAT_2_0 \
> -- 
> 1.8.3.1