[Qemu-devel] [PATCH] Don't explicitly set BAR values for VMware VGA

[Qemu-devel] [PATCH] Don't explicitly set BAR values for VMware VGA

[Anthony Liguori]

Right now we set explict base addresses for the PCI IO regions in the VMware
VGA device.  We don't register the second region at all and instead directly
map the physical memory.

The problem is, the addresses we're setting in the BAR is not taken into
account in the e820 mapping.

This patch removes the explicit BARs and registers the second region through
the normal PCI code.

I've only tested with a Linux guest and the open source VMware VGA driver.

This patch needs -p2 to apply against the QEMU CVS tree.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qemu/hw/vmware_vga.c b/qemu/hw/vmware_vga.c
index bd96e6b..ec7b1cd 100644
--- a/qemu/hw/vmware_vga.c
+++ b/qemu/hw/vmware_vga.c
@@ -60,6 +60,8 @@ struct vmsvga_state_s {
     int vram_size;
 #endif
     uint8_t *vram;
+    uint32_t vram_addr;
+    int iomemtype;
 
     int index;
     int scratch_size;
@@ -644,7 +646,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return ((s->depth + 7) >> 3) * s->new_width;
 
     case SVGA_REG_FB_START:
-        return SVGA_MEM_BASE;
+        return s->vram_addr;
 
     case SVGA_REG_FB_OFFSET:
         return 0x0;
@@ -671,7 +673,7 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
         return caps;
 
     case SVGA_REG_MEM_START:
-        return SVGA_MEM_BASE + s->vram_size - SVGA_FIFO_SIZE;
+        return s->vram_addr + s->vram_size - SVGA_FIFO_SIZE;
 
     case SVGA_REG_MEM_SIZE:
         return SVGA_FIFO_SIZE;
@@ -953,7 +955,7 @@ static void vmsvga_screen_dump(void *opaque, const char *filename)
 static uint32_t vmsvga_vram_readb(void *opaque, target_phys_addr_t addr)
 {
     struct vmsvga_state_s *s = (struct vmsvga_state_s *) opaque;
-    addr -= SVGA_MEM_BASE;
+    addr -= s->vram_addr;
     if (addr < s->fb_size)
         return *(uint8_t *) (s->ds->data + addr);
     else
@@ -963,7 +965,7 @@ static uint32_t vmsvga_vram_readb(void *opaque, target_phys_addr_t addr)
 static uint32_t vmsvga_vram_readw(void *opaque, target_phys_addr_t addr)
 {
     struct vmsvga_state_s *s = (struct vmsvga_state_s *) opaque;
-    addr -= SVGA_MEM_BASE;
+    addr -= s->vram_addr;
     if (addr < s->fb_size)
         return *(uint16_t *) (s->ds->data + addr);
     else
@@ -973,7 +975,7 @@ static uint32_t vmsvga_vram_readw(void *opaque, target_phys_addr_t addr)
 static uint32_t vmsvga_vram_readl(void *opaque, target_phys_addr_t addr)
 {
     struct vmsvga_state_s *s = (struct vmsvga_state_s *) opaque;
-    addr -= SVGA_MEM_BASE;
+    addr -= s->vram_addr;
     if (addr < s->fb_size)
         return *(uint32_t *) (s->ds->data + addr);
     else
@@ -984,7 +986,7 @@ static void vmsvga_vram_writeb(void *opaque, target_phys_addr_t addr,
                 uint32_t value)
 {
     struct vmsvga_state_s *s = (struct vmsvga_state_s *) opaque;
-    addr -= SVGA_MEM_BASE;
+    addr -= s->vram_addr;
     if (addr < s->fb_size)
         *(uint8_t *) (s->ds->data + addr) = value;
     else
@@ -995,7 +997,7 @@ static void vmsvga_vram_writew(void *opaque, target_phys_addr_t addr,
                 uint32_t value)
 {
     struct vmsvga_state_s *s = (struct vmsvga_state_s *) opaque;
-    addr -= SVGA_MEM_BASE;
+    addr -= s->vram_addr;
     if (addr < s->fb_size)
         *(uint16_t *) (s->ds->data + addr) = value;
     else
@@ -1006,7 +1008,7 @@ static void vmsvga_vram_writel(void *opaque, target_phys_addr_t addr,
                 uint32_t value)
 {
     struct vmsvga_state_s *s = (struct vmsvga_state_s *) opaque;
-    addr -= SVGA_MEM_BASE;
+    addr -= s->vram_addr;
     if (addr < s->fb_size)
         *(uint32_t *) (s->ds->data + addr) = value;
     else
@@ -1081,10 +1083,10 @@ static void vmsvga_init(struct vmsvga_state_s *s, DisplayState *ds,
                 uint8_t *vga_ram_base, unsigned long vga_ram_offset,
                 int vga_ram_size)
 {
-    int iomemtype;
     s->ds = ds;
     s->vram = vga_ram_base;
     s->vram_size = vga_ram_size;
+    s->vram_addr = 0;
 
     s->scratch_size = SVGA_SCRATCH_SIZE;
     s->scratch = (uint32_t *) qemu_malloc(s->scratch_size * 4);
@@ -1092,13 +1094,11 @@ static void vmsvga_init(struct vmsvga_state_s *s, DisplayState *ds,
     vmsvga_reset(s);
 
 #ifdef DIRECT_VRAM
-    iomemtype = cpu_register_io_memory(0, vmsvga_vram_read,
-                    vmsvga_vram_write, s);
+    s->iomemtype = cpu_register_io_memory(0, vmsvga_vram_read,
+					  vmsvga_vram_write, s);
 #else
-    iomemtype = vga_ram_offset | IO_MEM_RAM;
+    s->iomemtype = vga_ram_offset | IO_MEM_RAM;
 #endif
-    cpu_register_physical_memory(SVGA_MEM_BASE, vga_ram_size,
-                    iomemtype);
 
     graphic_console_init(ds, vmsvga_update_display,
                     vmsvga_invalidate_display, vmsvga_screen_dump, s);
@@ -1133,24 +1133,30 @@ static int pci_vmsvga_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
-static void pci_vmsvga_map_ioport(PCIDevice *pci_dev, int region_num,
+static void pci_vmsvga_map(PCIDevice *pci_dev, int region_num,
                 uint32_t addr, uint32_t size, int type)
 {
     struct pci_vmsvga_state_s *d = (struct pci_vmsvga_state_s *) pci_dev;
     struct vmsvga_state_s *s = &d->chip;
 
-    register_ioport_read(addr + SVGA_IO_MUL * SVGA_INDEX_PORT,
-                    1, 4, vmsvga_index_read, s);
-    register_ioport_write(addr + SVGA_IO_MUL * SVGA_INDEX_PORT,
-                    1, 4, vmsvga_index_write, s);
-    register_ioport_read(addr + SVGA_IO_MUL * SVGA_VALUE_PORT,
-                    1, 4, vmsvga_value_read, s);
-    register_ioport_write(addr + SVGA_IO_MUL * SVGA_VALUE_PORT,
-                    1, 4, vmsvga_value_write, s);
-    register_ioport_read(addr + SVGA_IO_MUL * SVGA_BIOS_PORT,
-                    1, 4, vmsvga_bios_read, s);
-    register_ioport_write(addr + SVGA_IO_MUL * SVGA_BIOS_PORT,
-                    1, 4, vmsvga_bios_write, s);
+    if (region_num == 0) {
+	register_ioport_read(addr + SVGA_IO_MUL * SVGA_INDEX_PORT,
+			     1, 4, vmsvga_index_read, s);
+	register_ioport_write(addr + SVGA_IO_MUL * SVGA_INDEX_PORT,
+			      1, 4, vmsvga_index_write, s);
+	register_ioport_read(addr + SVGA_IO_MUL * SVGA_VALUE_PORT,
+			     1, 4, vmsvga_value_read, s);
+	register_ioport_write(addr + SVGA_IO_MUL * SVGA_VALUE_PORT,
+			      1, 4, vmsvga_value_write, s);
+	register_ioport_read(addr + SVGA_IO_MUL * SVGA_BIOS_PORT,
+			     1, 4, vmsvga_bios_read, s);
+	register_ioport_write(addr + SVGA_IO_MUL * SVGA_BIOS_PORT,
+			      1, 4, vmsvga_bios_write, s);
+    } else {
+        cpu_register_physical_memory(addr, s->vram_size, s->iomemtype);
+	s->vram_addr = addr;
+	s->vram = s->vram_ptr;
+    }
 }
 
 #define PCI_VENDOR_ID_VMWARE		0x15ad
@@ -1182,14 +1188,6 @@ void pci_vmsvga_init(PCIBus *bus, DisplayState *ds, uint8_t *vga_ram_base,
     s->card.config[0x0c]		= 0x08;		/* Cache line size */
     s->card.config[0x0d]		= 0x40;		/* Latency timer */
     s->card.config[0x0e]		= PCI_CLASS_HEADERTYPE_00h;
-    s->card.config[0x10]		= ((SVGA_IO_BASE >>  0) & 0xff) | 1;
-    s->card.config[0x11]		=  (SVGA_IO_BASE >>  8) & 0xff;
-    s->card.config[0x12]		=  (SVGA_IO_BASE >> 16) & 0xff;
-    s->card.config[0x13]		=  (SVGA_IO_BASE >> 24) & 0xff;
-    s->card.config[0x18]		= (SVGA_MEM_BASE >>  0) & 0xff;
-    s->card.config[0x19]		= (SVGA_MEM_BASE >>  8) & 0xff;
-    s->card.config[0x1a]		= (SVGA_MEM_BASE >> 16) & 0xff;
-    s->card.config[0x1b]		= (SVGA_MEM_BASE >> 24) & 0xff;
     s->card.config[0x2c]		= PCI_VENDOR_ID_VMWARE & 0xff;
     s->card.config[0x2d]		= PCI_VENDOR_ID_VMWARE >> 8;
     s->card.config[0x2e]		= SVGA_PCI_DEVICE_ID & 0xff;
@@ -1197,7 +1195,10 @@ void pci_vmsvga_init(PCIBus *bus, DisplayState *ds, uint8_t *vga_ram_base,
     s->card.config[0x3c]		= 0xff;		/* End */
 
     pci_register_io_region(&s->card, 0, 0x10,
-                    PCI_ADDRESS_SPACE_IO, pci_vmsvga_map_ioport);
+			   PCI_ADDRESS_SPACE_IO, pci_vmsvga_map);
+
+    pci_register_io_region(&s->card, 1, vga_ram_size,
+			   PCI_ADDRESS_SPACE_MEM, pci_vmsvga_map);
 
     vmsvga_init(&s->chip, ds, vga_ram_base, vga_ram_offset, vga_ram_size);
 

[Qemu-devel] Re: [kvm-devel] [PATCH] Don't explicitly set BAR values for VMware VGA

[Anthony Liguori]

Anthony Liguori wrote:
> Right now we set explict base addresses for the PCI IO regions in the VMware
> VGA device.  We don't register the second region at all and instead directly
> map the physical memory.
>
> The problem is, the addresses we're setting in the BAR is not taken into
> account in the e820 mapping.
>   

Actually, it looks like the e820 map is okay, but I do get some errors 
in the gust about remapping that range so I do think this patch is correct.

Regards,

Anthony Liguori

> This patch removes the explicit BARs and registers the second region through
> the normal PCI code.
>
> I've only tested with a Linux guest and the open source VMware VGA driver.
>
> This patch needs -p2 to apply against the QEMU CVS tree.
>
>   

[Qemu-devel] Re: [PATCH] Don't explicitly set BAR values for VMware VGA

[andrzej zaborowski]

On 22/02/2008, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Right now we set explict base addresses for the PCI IO regions in the VMware
>  VGA device.  We don't register the second region at all and instead directly
>  map the physical memory.
>
>  The problem is, the addresses we're setting in the BAR is not taken into
>  account in the e820 mapping.
>
>  This patch removes the explicit BARs and registers the second region through
>  the normal PCI code.
>
>  I've only tested with a Linux guest and the open source VMware VGA driver.

I have a very similar patch on my HD but I haven't included it because
it causes my testing Ms Windows install to stop detecting the card.  I
just tested your patch and the same thing happens, i.e. with the patch
it works as a vga card but is detected as an Unknown adapter and only
the 640x480x8 mode can be used.  I can't explain this.

Currently the io port numbers can be set by the guest and the memory
io regions are fixed.  Earlier both settings were hardcoded.  This is
because in one version of the X driver (which was/is the only
documentation available) these settings were metioned as *the* correct
values for this card.  This may of course cause different types of
breakage but so far worked ok, except when it was found that in
various combinations qemu segfaulted due to different PCI cards
registering the same port range as our default.  When this happened I
tried to make these settings settable through PCI registers but found
that this broke Ms Windows.  Luckily Ms Windows still worked if only
port ranges were assigned dynamically and the segfault went away so I
left it at this but it perhaps needs better looking at.
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.

[Qemu-devel] Re: [PATCH] Don't explicitly set BAR values for VMware VGA

[Anthony Liguori]

andrzej zaborowski wrote:
> On 22/02/2008, Anthony Liguori <aliguori@us.ibm.com> wrote:
>   
>> Right now we set explict base addresses for the PCI IO regions in the VMware
>>  VGA device.  We don't register the second region at all and instead directly
>>  map the physical memory.
>>
>>  The problem is, the addresses we're setting in the BAR is not taken into
>>  account in the e820 mapping.
>>
>>  This patch removes the explicit BARs and registers the second region through
>>  the normal PCI code.
>>
>>  I've only tested with a Linux guest and the open source VMware VGA driver.
>>     
>
> I have a very similar patch on my HD but I haven't included it because
> it causes my testing Ms Windows install to stop detecting the card.  I
> just tested your patch and the same thing happens, i.e. with the patch
> it works as a vga card but is detected as an Unknown adapter and only
> the 640x480x8 mode can be used.  I can't explain this.
>   

Can you describe how you setup your Windows install?  I'll try to 
reproduce it and dig into it.

Regards,

Anthony Liguori

> Currently the io port numbers can be set by the guest and the memory
> io regions are fixed.  Earlier both settings were hardcoded.  This is
> because in one version of the X driver (which was/is the only
> documentation available) these settings were metioned as *the* correct
> values for this card.  This may of course cause different types of
> breakage but so far worked ok, except when it was found that in
> various combinations qemu segfaulted due to different PCI cards
> registering the same port range as our default.  When this happened I
> tried to make these settings settable through PCI registers but found
> that this broke Ms Windows.  Luckily Ms Windows still worked if only
> port ranges were assigned dynamically and the segfault went away so I
> left it at this but it perhaps needs better looking at.
>   

[Qemu-devel] Re: [PATCH] Don't explicitly set BAR values for VMware VGA

[andrzej zaborowski]

On 23/02/2008, Anthony Liguori <aliguori@us.ibm.com> wrote:
> andrzej zaborowski wrote:
>  > I have a very similar patch on my HD but I haven't included it because
>  > it causes my testing Ms Windows install to stop detecting the card.  I
>  > just tested your patch and the same thing happens, i.e. with the patch
>  > it works as a vga card but is detected as an Unknown adapter and only
>  > the 640x480x8 mode can be used.  I can't explain this.
>  >
>
>
> Can you describe how you setup your Windows install?  I'll try to
>  reproduce it and dig into it.

Oh, good question, and I think the answer be the reason why it's not
working here (*slaps self*).  I'll apply the patch if you can confirm
that it works with some Ms Windows install.

I just launched the VM and checked what kind of Ms Windows set up this
is and it's a "Windows XP professional 2002" with VMware Tools
installed, and all the files on it have last modification date in 2004
or earlier.  Apparently I stole the already set up VM from my dad's
computer on which he used VMware, somewhere in 2004.  I then converted
the image to raw and always performed my tests with -snapshot on.
This may be why the system is unwilling to use different base
addresses.
-- 
Please do not print this email unless absolutely necessary. Spread
environmental awareness.

[Qemu-devel] Re: [kvm-devel] [PATCH] Don't explicitly set BAR values for VMware VGA

[Anthony Liguori]

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

andrzej zaborowski wrote:
> Oh, good question, and I think the answer be the reason why it's not
> working here (*slaps self*).  I'll apply the patch if you can confirm
> that it works with some Ms Windows install.
>   

I just tried with Windows XP and I have no problem detecting the card 
with this patch.

FWIW, I needed to use the following patch to avoid SEGVs as it seems 
there's a bug in the emulation where the update region is larger than 
the screen.  My first impression is that it's related to cursor drawing 
as it only happens when I click on the start button and the update 
region height is 36 pixels which looks like a cursor size to me.  This 
is true with or without the BAR patch though.  I'll look into it a 
little more and see what's going on.

Regards,

Anthony Liguori

> I just launched the VM and checked what kind of Ms Windows set up this
> is and it's a "Windows XP professional 2002" with VMware Tools
> installed, and all the files on it have last modification date in 2004
> or earlier.  Apparently I stole the already set up VM from my dad's
> computer on which he used VMware, somewhere in 2004.  I then converted
> the image to raw and always performed my tests with -snapshot on.
> This may be why the system is unwilling to use different base
> addresses.
>   


[-- Attachment #2: qemu-vmware-check-update.patch --]
[-- Type: text/x-diff, Size: 1193 bytes --]

diff --git a/qemu/hw/vmware_vga.c b/qemu/hw/vmware_vga.c
index f2a298e..0204d88 100644
--- a/qemu/hw/vmware_vga.c
+++ b/qemu/hw/vmware_vga.c
@@ -295,12 +295,31 @@ static inline void vmsvga_update_rect(struct vmsvga_state_s *s,
                 int x, int y, int w, int h)
 {
 #ifndef DIRECT_VRAM
-    int line = h;
-    int bypl = s->bypp * s->width;
-    int width = s->bypp * w;
-    int start = s->bypp * x + bypl * y;
-    uint8_t *src = s->vram + start;
-    uint8_t *dst = s->ds->data + start;
+    int line;
+    int bypl;
+    int width;
+    int start;
+    uint8_t *src;
+    uint8_t *dst;
+
+    if ((x + w) > s->ds->width) {
+	fprintf(stderr, "update width too large x: %d, w: %d\n", x, w);
+	x = MIN(x, s->ds->width);
+	w = s->ds->width - x;
+    }
+
+    if ((y + h) > s->ds->height) {
+	fprintf(stderr, "update height too large y: %d, h: %d\n", y, h);
+	y = MIN(y, s->ds->height);
+	h = s->ds->height - y;
+    }
+
+    line = h;
+    bypl = s->bypp * s->width;
+    width = s->bypp * w;
+    start = s->bypp * x + bypl * y;
+    src = s->vram + start;
+    dst = s->ds->data + start;
 
     for (; line > 0; line --, src += bypl, dst += bypl)
         memcpy(dst, src, width);

[Qemu-devel] Re: [PATCH] Don't explicitly set BAR values for VMware VGA

[Avi Kivity]

Anthony Liguori wrote:
> Right now we set explict base addresses for the PCI IO regions in the VMware
> VGA device.  We don't register the second region at all and instead directly
> map the physical memory.
>
> The problem is, the addresses we're setting in the BAR is not taken into
> account in the e820 mapping.
>
> This patch removes the explicit BARs and registers the second region through
> the normal PCI code.
>
> I've only tested with a Linux guest and the open source VMware VGA driver.
>
> This patch needs -p2 to apply against the QEMU CVS tree.
>
>   

Since it seems like qemu will apply this soon, I'll pick it up via the 
next merge.

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