| Message ID | CAOiua5Ojg9s24eaQkfDuUBMJffABuBEMkohHTjuF5b=-oE3ADw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] qxl: add additional vram regions to Xen physmap | expand |
+Xenia, Ray On Thu, 6 Mar 2025, Alessandro Muggianu wrote: > Hi all, > > Using QXL graphics on Windows 10 hvm domains causes the guest to become > extremely slow. The behaviour will happen as soon as Windows loads the > driver, so the VM will initially work normally while the OS is loading. > > This was reproduced on the current master but to my knowledge it's > always been an issue for Windows 10 guests on Xen (issue is not present > on KVM). > > The normal VGA display adapter uses a single vram memory region which is > registered as the Xen framebuffer with xen_register_framebuffer(). > This will cause it to be mapped to the Xen physmap in xen_add_to_physmap(). > > In the case of one or multiple QXL devices, only the first memory region > of the first (primary) device will be registered with Xen as framebuffer > and added to physmap (since it reuses the vga code), while I think all > other memory regions will be accessed via the IOREQ server, which > probably has too much overhead and seems to be the cause of the > unresponsive guest. > > We made an attempt at fixing the problem by forcing those memory regions > to be added to the Xen physmap in xen_add_to_physmap(). > > This solves the performance issue and seems to be enough to make > QXL work. Multi-screen, additional resolutions, etc., all seems fine. > > However, there is a lot I don't understand so I am not sure the change > is sensible. Hoping to get some expert eyes on this. > > I see these issues with the current change: > > * Broken migration. When trying to restore the domain, this assertion > will fail for the qxl memory regions I added to the physmap (the ones > named "qxl.vram" or "qxl.vgavram"), because the address returned by > xen_replace_cache_entry() is different from what we get from > memory_region_get_ram_ptr(): > > qemu-system-i386: ../hw/i386/xen/xen-hvm.c:317: > xen_add_to_physmap: Assertion `p && p == > memory_region_get_ram_ptr(mr)' failed. > > If I understand correctly, we try to recreate the physmap entry for > the region by calling xen_replace_cache_entry(), which retrieves > the guest memory address using xenforeignmemory_map2(), since the VRAM > should belong to the guest and not QEMU (however this might not be the > case for the QXL memory regions?). The address we obtain should > also match the one obtained through memory_region_get_ram_ptr(), > which (I think) will come from the restored VM state. > > In qxl.c, I'm only calling memory_region_get_ram_ptr(&qxl->vram_bar) > to ensure the region is mapped on the host (not doing so will cause > issues later), but I'm not using the returned pointer anywhere. > Maybe it's supposed to be saved with the VM state? > > * Now that I'm using a list of regions registered as Xen framebuffer, I > don't know what to do in xen_sync_dirty_bitmap(). At the moment I > forced it to only call memory_region_set_dirty() on the "original" > framebuffer in a quick and dirty way, since it seems like we get there > only from the vga code but not from the QXL code, which seems to use > memory_region_set_dirty() instead (from qxl_set_dirty()). > > * I used memory_region_set_log(<qxl_mem_region>, true, DIRTY_MEMORY_VGA) > or the regions won't be actually added in arch_xen_set_memory(). I don't > know if that's the right call, I just copied what we do for std VGA. > > Hoping there isn't a fundamental issue with the approach, it would be > great to have this working! > > The issue should be easy to reproduce but please let me know if I failed > to provide some important information. > > Thank you, > > Alessandro > > --- > > diff --git a/hw/display/qxl.c b/hw/display/qxl.c > index 7178dec85d..80dc0b2131 100644 > --- a/hw/display/qxl.c > +++ b/hw/display/qxl.c > @@ -29,6 +29,7 @@ > #include "qemu/main-loop.h" > #include "qemu/module.h" > #include "hw/qdev-properties.h" > +#include "hw/xen/xen.h" > #include "sysemu/runstate.h" > #include "migration/vmstate.h" > #include "trace.h" > @@ -2139,11 +2140,14 @@ static void qxl_realize_common(PCIQXLDevice > *qxl, Error **errp) > memory_region_init_alias(&qxl->vram32_bar, OBJECT(qxl), "qxl.vram32", > &qxl->vram_bar, 0, qxl->vram32_size); > > + memory_region_get_ram_ptr(&qxl->vram_bar); > memory_region_init_io(&qxl->io_bar, OBJECT(qxl), &qxl_io_ops, qxl, > "qxl-ioports", io_size); > if (qxl->have_vga) { > vga_dirty_log_start(&qxl->vga); > } > + xen_register_framebuffer(&qxl->vram_bar); > + memory_region_set_log(&qxl->vram_bar, true, DIRTY_MEMORY_VGA); > memory_region_set_flush_coalesced(&qxl->io_bar); > > > @@ -2268,6 +2272,9 @@ static void qxl_realize_secondary(PCIDevice > *dev, Error **errp) > qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */ > > qxl_realize_common(qxl, errp); > + > + xen_register_framebuffer(&qxl->vga.vram); > + memory_region_set_log(&qxl->vga.vram, true, DIRTY_MEMORY_VGA); > } > > static int qxl_pre_save(void *opaque) > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c > index 4f6446600c..33c7c14804 100644 > --- a/hw/i386/xen/xen-hvm.c > +++ b/hw/i386/xen/xen-hvm.c > @@ -14,6 +14,8 @@ > #include "qapi/qapi-commands-migration.h" > #include "trace.h" > > +#include "exec/ramblock.h" > + > #include "hw/i386/pc.h" > #include "hw/irq.h" > #include "hw/i386/apic-msidef.h" > @@ -26,7 +28,7 @@ > #include "exec/target_page.h" > > static MemoryRegion ram_640k, ram_lo, ram_hi; > -static MemoryRegion *framebuffer; > +static QLIST_HEAD(, XenFramebuffer) xen_framebuffer; > static bool xen_in_migration; > > /* Compatibility with older version */ > @@ -175,6 +177,17 @@ static void xen_ram_init(PCMachineState *pcms, > } > } > > +static MemoryRegion *get_framebuffer_by_name(const char *name) { > + XenFramebuffer *fb = NULL; > + > + QLIST_FOREACH(fb, &xen_framebuffer, list) { > + if (g_strcmp0(memory_region_name(fb->mr), name) == 0) { > + return fb->mr; > + } > + } > + return NULL; > +} > + > static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size, > int page_mask) > { > @@ -254,6 +267,7 @@ static int xen_add_to_physmap(XenIOState *state, > unsigned long nr_pages; > int rc = 0; > XenPhysmap *physmap = NULL; > + XenFramebuffer *fb = NULL; > hwaddr pfn, start_gpfn; > hwaddr phys_offset = memory_region_get_ram_addr(mr); > const char *mr_name; > @@ -269,9 +283,14 @@ static int xen_add_to_physmap(XenIOState *state, > * the linear framebuffer to be that region. > * Avoid tracking any regions that is not videoram and avoid tracking > * the legacy vga region. */ > - if (mr == framebuffer && start_addr > 0xbffff) { > - goto go_physmap; > + if (start_addr > 0xbffff) { > + QLIST_FOREACH(fb, &xen_framebuffer, list) { > + if (mr == fb->mr) { > + goto go_physmap; > + } > + } > } > + > return -1; > > go_physmap: > @@ -293,6 +312,7 @@ go_physmap: > /* Now when we have a physmap entry we can replace a dummy mapping with > * a real one of guest foreign memory. */ > uint8_t *p = xen_replace_cache_entry(phys_offset, start_addr, size); > + // For qxl.vram this assert will fail > assert(p && p == memory_region_get_ram_ptr(mr)); > > return 0; > @@ -406,7 +426,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > #define ENODATA ENOENT > #endif > if (errno == ENODATA) { > - memory_region_set_dirty(framebuffer, 0, size); > + MemoryRegion *fb = get_framebuffer_by_name("vga.vram"); > + memory_region_set_dirty(fb, 0, size); > DPRINTF("xen: track_dirty_vram failed (0x" HWADDR_FMT_plx > ", 0x" HWADDR_FMT_plx "): %s\n", > start_addr, start_addr + size, strerror(errno)); > @@ -419,8 +440,10 @@ static void xen_sync_dirty_bitmap(XenIOState *state, > while (map != 0) { > j = ctzl(map); > map &= ~(1ul << j); > - memory_region_set_dirty(framebuffer, > - (i * width + j) * page_size, page_size); > + MemoryRegion *fb = get_framebuffer_by_name("vga.vram"); > + memory_region_set_dirty(fb, > + (i * width + j) * page_size, page_size); > + > }; > } > } > @@ -618,6 +641,8 @@ void xen_hvm_init_pc(PCMachineState *pcms, > MemoryRegion **ram_memory) > > xen_is_stubdomain = xen_check_stubdomain(state->xenstore); > > + QLIST_INIT(&xen_framebuffer); > + > QLIST_INIT(&xen_physmap); > xen_read_physmap(state); > > @@ -658,7 +683,12 @@ err: > > void xen_register_framebuffer(MemoryRegion *mr) > { > - framebuffer = mr; > + XenFramebuffer *fb = NULL; > + > + fb= g_new(XenFramebuffer, 1); > + fb->mr = mr; > + > + QLIST_INSERT_HEAD(&xen_framebuffer, fb, list); > } > > void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length) > diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h > index 3d796235dc..8eba992c55 100644 > --- a/include/hw/xen/xen-hvm-common.h > +++ b/include/hw/xen/xen-hvm-common.h > @@ -43,6 +43,13 @@ static inline ioreq_t > *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu) > > #define BUFFER_IO_MAX_DELAY 100 > > +typedef struct XenFramebuffer { > + MemoryRegion *mr; > + > + QLIST_ENTRY(XenFramebuffer) list; > +} XenFramebuffer; > + > + > typedef struct XenPhysmap { > hwaddr start_addr; > ram_addr_t size; >
diff --git a/hw/display/qxl.c b/hw/display/qxl.c index 7178dec85d..80dc0b2131 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -29,6 +29,7 @@ #include "qemu/main-loop.h" #include "qemu/module.h" #include "hw/qdev-properties.h" +#include "hw/xen/xen.h" #include "sysemu/runstate.h" #include "migration/vmstate.h" #include "trace.h" @@ -2139,11 +2140,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp) memory_region_init_alias(&qxl->vram32_bar, OBJECT(qxl), "qxl.vram32", &qxl->vram_bar, 0, qxl->vram32_size); + memory_region_get_ram_ptr(&qxl->vram_bar); memory_region_init_io(&qxl->io_bar, OBJECT(qxl), &qxl_io_ops, qxl, "qxl-ioports", io_size); if (qxl->have_vga) { vga_dirty_log_start(&qxl->vga); } + xen_register_framebuffer(&qxl->vram_bar); + memory_region_set_log(&qxl->vram_bar, true, DIRTY_MEMORY_VGA); memory_region_set_flush_coalesced(&qxl->io_bar); @@ -2268,6 +2272,9 @@ static void qxl_realize_secondary(PCIDevice *dev, Error **errp) qxl->id = qemu_console_get_index(qxl->vga.con); /* == channel_id */ qxl_realize_common(qxl, errp); + + xen_register_framebuffer(&qxl->vga.vram); + memory_region_set_log(&qxl->vga.vram, true, DIRTY_MEMORY_VGA); } static int qxl_pre_save(void *opaque) diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c index 4f6446600c..33c7c14804 100644 --- a/hw/i386/xen/xen-hvm.c +++ b/hw/i386/xen/xen-hvm.c @@ -14,6 +14,8 @@ #include "qapi/qapi-commands-migration.h" #include "trace.h" +#include "exec/ramblock.h" + #include "hw/i386/pc.h" #include "hw/irq.h" #include "hw/i386/apic-msidef.h" @@ -26,7 +28,7 @@ #include "exec/target_page.h" static MemoryRegion ram_640k, ram_lo, ram_hi; -static MemoryRegion *framebuffer; +static QLIST_HEAD(, XenFramebuffer) xen_framebuffer; static bool xen_in_migration; /* Compatibility with older version */ @@ -175,6 +177,17 @@ static void xen_ram_init(PCMachineState *pcms, } } +static MemoryRegion *get_framebuffer_by_name(const char *name) { + XenFramebuffer *fb = NULL; + + QLIST_FOREACH(fb, &xen_framebuffer, list) { + if (g_strcmp0(memory_region_name(fb->mr), name) == 0) { + return fb->mr; + } + } + return NULL; +} + static XenPhysmap *get_physmapping(hwaddr start_addr, ram_addr_t size, int page_mask) { @@ -254,6 +267,7 @@ static int xen_add_to_physmap(XenIOState *state, unsigned long nr_pages; int rc = 0; XenPhysmap *physmap = NULL; + XenFramebuffer *fb = NULL; hwaddr pfn, start_gpfn; hwaddr phys_offset = memory_region_get_ram_addr(mr); const char *mr_name; @@ -269,9 +283,14 @@ static int xen_add_to_physmap(XenIOState *state, * the linear framebuffer to be that region. * Avoid tracking any regions that is not videoram and avoid tracking * the legacy vga region. */ - if (mr == framebuffer && start_addr > 0xbffff) { - goto go_physmap; + if (start_addr > 0xbffff) { + QLIST_FOREACH(fb, &xen_framebuffer, list) { + if (mr == fb->mr) { + goto go_physmap; + } + } } + return -1; go_physmap: @@ -293,6 +312,7 @@ go_physmap: /* Now when we have a physmap entry we can replace a dummy mapping with * a real one of guest foreign memory. */ uint8_t *p = xen_replace_cache_entry(phys_offset, start_addr, size); + // For qxl.vram this assert will fail assert(p && p == memory_region_get_ram_ptr(mr)); return 0; @@ -406,7 +426,8 @@ static void xen_sync_dirty_bitmap(XenIOState *state, #define ENODATA ENOENT #endif if (errno == ENODATA) { - memory_region_set_dirty(framebuffer, 0, size); + MemoryRegion *fb = get_framebuffer_by_name("vga.vram"); + memory_region_set_dirty(fb, 0, size); DPRINTF("xen: track_dirty_vram failed (0x" HWADDR_FMT_plx ", 0x" HWADDR_FMT_plx "): %s\n", start_addr, start_addr + size, strerror(errno)); @@ -419,8 +440,10 @@ static void xen_sync_dirty_bitmap(XenIOState *state, while (map != 0) { j = ctzl(map); map &= ~(1ul << j); - memory_region_set_dirty(framebuffer, - (i * width + j) * page_size, page_size); + MemoryRegion *fb = get_framebuffer_by_name("vga.vram"); + memory_region_set_dirty(fb, + (i * width + j) * page_size, page_size); + }; } } @@ -618,6 +641,8 @@ void xen_hvm_init_pc(PCMachineState *pcms, MemoryRegion **ram_memory) xen_is_stubdomain = xen_check_stubdomain(state->xenstore); + QLIST_INIT(&xen_framebuffer); + QLIST_INIT(&xen_physmap); xen_read_physmap(state); @@ -658,7 +683,12 @@ err: void xen_register_framebuffer(MemoryRegion *mr) { - framebuffer = mr; + XenFramebuffer *fb = NULL; + + fb= g_new(XenFramebuffer, 1); + fb->mr = mr; + + QLIST_INSERT_HEAD(&xen_framebuffer, fb, list); } void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length) diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h index 3d796235dc..8eba992c55 100644 --- a/include/hw/xen/xen-hvm-common.h +++ b/include/hw/xen/xen-hvm-common.h @@ -43,6 +43,13 @@ static inline ioreq_t *xen_vcpu_ioreq(shared_iopage_t *shared_page, int vcpu) #define BUFFER_IO_MAX_DELAY 100 +typedef struct XenFramebuffer { + MemoryRegion *mr; + + QLIST_ENTRY(XenFramebuffer) list; +} XenFramebuffer; + + typedef struct XenPhysmap { hwaddr start_addr;
Hi all, Using QXL graphics on Windows 10 hvm domains causes the guest to become extremely slow. The behaviour will happen as soon as Windows loads the driver, so the VM will initially work normally while the OS is loading. This was reproduced on the current master but to my knowledge it's always been an issue for Windows 10 guests on Xen (issue is not present on KVM). The normal VGA display adapter uses a single vram memory region which is registered as the Xen framebuffer with xen_register_framebuffer(). This will cause it to be mapped to the Xen physmap in xen_add_to_physmap(). In the case of one or multiple QXL devices, only the first memory region of the first (primary) device will be registered with Xen as framebuffer and added to physmap (since it reuses the vga code), while I think all other memory regions will be accessed via the IOREQ server, which probably has too much overhead and seems to be the cause of the unresponsive guest. We made an attempt at fixing the problem by forcing those memory regions to be added to the Xen physmap in xen_add_to_physmap(). This solves the performance issue and seems to be enough to make QXL work. Multi-screen, additional resolutions, etc., all seems fine. However, there is a lot I don't understand so I am not sure the change is sensible. Hoping to get some expert eyes on this. I see these issues with the current change: * Broken migration. When trying to restore the domain, this assertion will fail for the qxl memory regions I added to the physmap (the ones named "qxl.vram" or "qxl.vgavram"), because the address returned by xen_replace_cache_entry() is different from what we get from memory_region_get_ram_ptr(): qemu-system-i386: ../hw/i386/xen/xen-hvm.c:317: xen_add_to_physmap: Assertion `p && p == memory_region_get_ram_ptr(mr)' failed. If I understand correctly, we try to recreate the physmap entry for the region by calling xen_replace_cache_entry(), which retrieves the guest memory address using xenforeignmemory_map2(), since the VRAM should belong to the guest and not QEMU (however this might not be the case for the QXL memory regions?). The address we obtain should also match the one obtained through memory_region_get_ram_ptr(), which (I think) will come from the restored VM state. In qxl.c, I'm only calling memory_region_get_ram_ptr(&qxl->vram_bar) to ensure the region is mapped on the host (not doing so will cause issues later), but I'm not using the returned pointer anywhere. Maybe it's supposed to be saved with the VM state? * Now that I'm using a list of regions registered as Xen framebuffer, I don't know what to do in xen_sync_dirty_bitmap(). At the moment I forced it to only call memory_region_set_dirty() on the "original" framebuffer in a quick and dirty way, since it seems like we get there only from the vga code but not from the QXL code, which seems to use memory_region_set_dirty() instead (from qxl_set_dirty()). * I used memory_region_set_log(<qxl_mem_region>, true, DIRTY_MEMORY_VGA) or the regions won't be actually added in arch_xen_set_memory(). I don't know if that's the right call, I just copied what we do for std VGA. Hoping there isn't a fundamental issue with the approach, it would be great to have this working! The issue should be easy to reproduce but please let me know if I failed to provide some important information. Thank you, Alessandro --- ram_addr_t size;