Re: [Qemu-devel] [Spice-devel] [PATCH 4/4] [experimental] add optinal 64bit vram bar to qxl

[Alon Levy <alevy@redhat.com>]

On Fri, Feb 17, 2012 at 04:10:33PM +0100, Gerd Hoffmann wrote:
> This patch adds an 64bit pci bar for vram.  It is turned off by default.
> It can be enabled by setting the size of the 64bit bar to be larger than
> the 32bit bar.  Both 32bit and 64bit bar refer to the same memory.  Only
> the first part of the memory is available via 32bit bar.
> 
> The intention is to allow large vram sizes for 64bit guests, by allowing
> the vram bar being mapped above 4G, so we don't have to squeeze it into
> the pci I/O window below 4G.
> 
> With vram_size_mb=16 and vram64_size_mb=256 it looks like this:
> 

The vram variables are a little confusing, but I guess I can live with
it, and it will be more natural going forward.

I guess you will s/4/QXL_VRAM64_RANGE_INDEX/ when you send the
spice-protocol patch?

Other then one more note below, looks good.

> 00:02.0 VGA compatible controller: Red Hat, Inc. Device 0100 (rev 02) (prog-if 00 [VGA controller])
>         Subsystem: Red Hat, Inc Device 1100
>         Physical Slot: 2
>         Flags: fast devsel, IRQ 10
>         Memory at f8000000 (32-bit, non-prefetchable) [size=64M]
>         Memory at fc000000 (32-bit, non-prefetchable) [size=16M]
>         Memory at fd020000 (32-bit, non-prefetchable) [size=8K]
>         I/O ports at c5a0 [size=32]
>         Memory at ffe0000000 (64-bit, prefetchable) [size=256M]
>         Expansion ROM at fd000000 [disabled] [size=64K]
> 
> [ needs patched seabios ]
> ---
>  hw/qxl.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
>  hw/qxl.h |    3 +++
>  2 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index 87ad49a..e38bb29 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -914,6 +914,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
>      static const int regions[] = {
>          QXL_RAM_RANGE_INDEX,
>          QXL_VRAM_RANGE_INDEX,
> +        4 /* vram 64bit */,
>      };
>      uint64_t guest_start;
>      uint64_t guest_end;
> @@ -960,6 +961,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
>          virt_start = (intptr_t)memory_region_get_ram_ptr(&d->vga.vram);
>          break;
>      case QXL_VRAM_RANGE_INDEX:
> +    case 4 /* vram 64bit */:
>          virt_start = (intptr_t)memory_region_get_ram_ptr(&d->vram_bar);
>          break;
>      default:
> @@ -1564,18 +1566,28 @@ static void qxl_init_ramsize(PCIQXLDevice *qxl, uint32_t ram_min_mb)
>          qxl->vga.vram_size = ram_min_mb * 1024 * 1024;
>      }
>  
> -    /* vram (surfaces, bar 1) */
> +    /* vram32 (surfaces, 32bit, bar 1) */
> +    if (qxl->vram32_size_mb != -1) {
> +        qxl->vram32_size = qxl->vram32_size_mb * 1024 * 1024;
> +    }
> +    if (qxl->vram32_size < 4096) {
> +        qxl->vram32_size = 4096;
> +    }
> +
> +    /* vram (surfaces, 64bit, bar 4+5) */
>      if (qxl->vram_size_mb != -1) {
>          qxl->vram_size = qxl->vram_size_mb * 1024 * 1024;
>      }
> -    if (qxl->vram_size < 4096) {
> -        qxl->vram_size = 4096;
> +    if (qxl->vram_size < qxl->vram32_size) {
> +        qxl->vram_size = qxl->vram32_size;

Am I reading correctly that you want the 64bit bar to be at least the
size of the 32bit bar? why?

>      }
> +
>      if (qxl->revision == 1) {
> +        qxl->vram32_size = 4096;
>          qxl->vram_size = 4096;
>      }
> -
>      qxl->vga.vram_size = msb_mask(qxl->vga.vram_size * 2 - 1);
> +    qxl->vram32_size = msb_mask(qxl->vram32_size * 2 - 1);
>      qxl->vram_size = msb_mask(qxl->vram_size * 2 - 1);
>  }
>  
> @@ -1619,6 +1631,8 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>  
>      memory_region_init_ram(&qxl->vram_bar, "qxl.vram", qxl->vram_size);
>      vmstate_register_ram(&qxl->vram_bar, &qxl->pci.qdev);
> +    memory_region_init_alias(&qxl->vram32_bar, "qxl.vram32", &qxl->vram_bar,
> +                             0, qxl->vram32_size);
>  
>      io_size = msb_mask(QXL_IO_RANGE_SIZE * 2 - 1);
>      if (qxl->revision == 1) {
> @@ -1642,7 +1656,29 @@ static int qxl_init_common(PCIQXLDevice *qxl)
>                       PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->vga.vram);
>  
>      pci_register_bar(&qxl->pci, QXL_VRAM_RANGE_INDEX,
> -                     PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->vram_bar);
> +                     PCI_BASE_ADDRESS_SPACE_MEMORY, &qxl->vram32_bar);
> +
> +    if (qxl->vram32_size < qxl->vram_size) {
> +        /*
> +         * Make the 64bit vram bar show up only in case it is
> +         * configured to be larger than the 32bit vram bar.
> +         */
> +        pci_register_bar(&qxl->pci, 4,
> +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
> +                         PCI_BASE_ADDRESS_MEM_TYPE_64 |
> +                         PCI_BASE_ADDRESS_MEM_PREFETCH,
> +                         &qxl->vram_bar);
> +    }
> +
> +    /* print pci bar details */
> +    dprint(qxl, 1, "ram/%s: %d MB [region 0]\n",
> +           qxl->id == 0 ? "pri" : "sec",
> +           qxl->vga.vram_size / (1024*1024));
> +    dprint(qxl, 1, "vram/32: %d MB [region 1]\n",
> +           qxl->vram32_size / (1024*1024));
> +    dprint(qxl, 1, "vram/64: %d MB %s\n",
> +           qxl->vram_size / (1024*1024),
> +           qxl->vram32_size < qxl->vram_size ? "[region 4]" : "[unmapped]");
>  
>      qxl->ssd.qxl.base.sif = &qxl_interface.base;
>      qxl->ssd.qxl.id = qxl->id;
> @@ -1859,7 +1895,7 @@ static VMStateDescription qxl_vmstate = {
>  static Property qxl_properties[] = {
>          DEFINE_PROP_UINT32("ram_size", PCIQXLDevice, vga.vram_size,
>                             64 * 1024 * 1024),
> -        DEFINE_PROP_UINT32("vram_size", PCIQXLDevice, vram_size,
> +        DEFINE_PROP_UINT32("vram_size", PCIQXLDevice, vram32_size,
>                             64 * 1024 * 1024),
>          DEFINE_PROP_UINT32("revision", PCIQXLDevice, revision,
>                             QXL_DEFAULT_REVISION),
> @@ -1867,7 +1903,8 @@ static Property qxl_properties[] = {
>          DEFINE_PROP_UINT32("guestdebug", PCIQXLDevice, guestdebug, 0),
>          DEFINE_PROP_UINT32("cmdlog", PCIQXLDevice, cmdlog, 0),
>          DEFINE_PROP_UINT32("ram_size_mb",  PCIQXLDevice, ram_size_mb, -1),
> -        DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram_size_mb, -1),
> +        DEFINE_PROP_UINT32("vram_size_mb", PCIQXLDevice, vram32_size_mb, 0),
> +        DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, 0),
>          DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/qxl.h b/hw/qxl.h
> index d062991..a1b1240 100644
> --- a/hw/qxl.h
> +++ b/hw/qxl.h
> @@ -86,6 +86,8 @@ typedef struct PCIQXLDevice {
>      /* vram pci bar */
>      uint32_t           vram_size;
>      MemoryRegion       vram_bar;
> +    uint32_t           vram32_size;
> +    MemoryRegion       vram32_bar;
>  
>      /* io bar */
>      MemoryRegion       io_bar;
> @@ -93,6 +95,7 @@ typedef struct PCIQXLDevice {
>      /* user-friendly properties (in megabytes) */
>      uint32_t          ram_size_mb;
>      uint32_t          vram_size_mb;
> +    uint32_t          vram32_size_mb;
>  } PCIQXLDevice;
>  
>  #define PANIC_ON(x) if ((x)) {                         \
> -- 
> 1.7.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel