[4/4,experimental] add optinal 64bit vram bar to qxl

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:

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(-)

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

Hi,

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

Yes.

>> -    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?

The 64bit bar isn't additional memory.  Both 32bit and 64bit bar are
backed by the same memory, the 64bit bar is just a different way to
access it.  So it doesn't make sense at all to make the 64bit bar
smaller than the 32bit bar.  The other way around makes sense, to save
address space below 4G.

cheers,
  Gerd

On Mon, Feb 20, 2012 at 09:00:14AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > I guess you will s/4/QXL_VRAM64_RANGE_INDEX/ when you send the
> > spice-protocol patch?
> 
> Yes.
> 
> >> -    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?
> 
> The 64bit bar isn't additional memory.  Both 32bit and 64bit bar are
> backed by the same memory, the 64bit bar is just a different way to
> access it.  So it doesn't make sense at all to make the 64bit bar
> smaller than the 32bit bar.  The other way around makes sense, to save
> address space below 4G.

Ok, thanks for the patient repetition of your clear commit message :)

ACK

> 
> cheers,
>   Gerd
>