| Message ID | 1322150893-887-6-git-send-email-anthony.perard@citrix.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 24 Nov 2011, Anthony PERARD wrote: > During the initialisation of the machine at restore time, the access to the > VRAM will fail because QEMU does not know yet the right guest address to map, > so the vram_ptr is NULL. > > So this patch avoid using a NULL pointer during initialisation, and try to get > another vram_ptr if the call failed the first time. This patch looks really bad, however I am not sure how to improve it. Let's see if I get this straight: on Xen the videoram is saved and restored by Xen like normal guest's ram. Therefore we skip a second videoram allocation in vga_common_init, returning NULL. As a consequence vga.vram_ptr is going to be invalid until cirrus_update_memory_access is called, when the cirrus state is restored and we finally know the position of the videoram in the guest address space, so we can map it again. > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > hw/cirrus_vga.c | 20 +++++++++++++++++--- > 1 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c > index c7e365b..d092c74 100644 > --- a/hw/cirrus_vga.c > +++ b/hw/cirrus_vga.c > @@ -32,6 +32,7 @@ > #include "console.h" > #include "vga_int.h" > #include "loader.h" > +#include "sysemu.h" > > /* > * TODO: > @@ -2696,6 +2697,17 @@ static int cirrus_post_load(void *opaque, int version_id) > s->vga.gr[0x01] = s->cirrus_shadow_gr1 & 0x0f; > > cirrus_update_memory_access(s); > + if (!s->vga.vram_ptr) { > + /* Try again > + * The initial call to get_ram_ptr can fail with Xen during a migration > + * because the VRAM has been moved arround. > + * (Moved with a set_memory and a xen_add_to_physmap) > + * After cirrus_update_memory_access, the Xen function will know were > + * the memory actually is, and fix the address before give back a > + * ram_ptr. > + */ > + s->vga.vram_ptr = memory_region_get_ram_ptr(&s->vga.vram); > + } > /* force refresh */ > s->vga.graphic_mode = -1; > cirrus_update_bank_ptr(s, 0); I would change the comment to: "At this point vga.vram_ptr can be invalid on Xen because we need to know the position of the videoram in the guest physical address space in order to be able to map it. After cirrus_update_memory_access we do know where the videoram is, so let's map it now." > @@ -2784,9 +2796,11 @@ static void cirrus_reset(void *opaque) > } > s->vga.cr[0x27] = s->device_id; > > - /* Win2K seems to assume that the pattern buffer is at 0xff > - initially ! */ > - memset(s->vga.vram_ptr, 0xff, s->real_vram_size); > + if (!runstate_check(RUN_STATE_PREMIGRATE)) { > + /* Win2K seems to assume that the pattern buffer is at 0xff > + initially ! */ > + memset(s->vga.vram_ptr, 0xff, s->real_vram_size); > + } > this is not too bad, I suppose that the videoram is going to be written again at restore time anyway so at least it saves some cycles
On Thu, Nov 24, 2011 at 18:30, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > >> @@ -2784,9 +2796,11 @@ static void cirrus_reset(void *opaque) >> } >> s->vga.cr[0x27] = s->device_id; >> >> - /* Win2K seems to assume that the pattern buffer is at 0xff >> - initially ! */ >> - memset(s->vga.vram_ptr, 0xff, s->real_vram_size); >> + if (!runstate_check(RUN_STATE_PREMIGRATE)) { >> + /* Win2K seems to assume that the pattern buffer is at 0xff >> + initially ! */ >> + memset(s->vga.vram_ptr, 0xff, s->real_vram_size); >> + } >> > > this is not too bad, I suppose that the videoram is going to be written > again at restore time anyway so at least it saves some cycles Actually, I think the next time that this vram will be written again is, when the guest is actually "waked-up" and wrote something there. Otherwise, the "restore" of the vram is done before QEMU start. So, the memset could leave some weard stuff the screen (a white screen?).
> On Thu, Nov 24, 2011 at 18:30, Stefano Stabellini > <stefano.stabellini@eu.citrix.com> wrote: > > > >> @@ -2784,9 +2796,11 @@ static void cirrus_reset(void *opaque) > >> Â Â Â } > >> Â Â Â s->vga.cr[0x27] = s->device_id; > >> > >> - Â Â /* Win2K seems to assume that the pattern buffer is at 0xff > >> - Â Â Â initially ! */ > >> - Â Â memset(s->vga.vram_ptr, 0xff, s->real_vram_size); > >> + Â Â if (!runstate_check(RUN_STATE_PREMIGRATE)) { > >> + Â Â Â Â /* Win2K seems to assume that the pattern buffer is at 0xff > >> + Â Â Â Â Â initially ! */ > >> + Â Â Â Â memset(s->vga.vram_ptr, 0xff, s->real_vram_size); > >> + Â Â } > >> > > > > this is not too bad, I suppose that the videoram is going to be written > > again at restore time anyway so at least it saves some cycles > > Actually, I think the next time that this vram will be written again > is, when the guest is actually "waked-up" and wrote something there. > Otherwise, the "restore" of the vram is done before QEMU start. So, > the memset could leave some weard stuff the screen (a white screen?). So this is the succession of events: - vga_common_init allocates the videoram - cirrus_reset sets set videoram to 0xff - load_vmstate the old videoram is copied over, overwriting what cirrus_reset has done therefore setting the videoram to 0xff when resuming is a waste of efforts
On Fri, Nov 25, 2011 at 11:51, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: >> On Thu, Nov 24, 2011 at 18:30, Stefano Stabellini >> <stefano.stabellini@eu.citrix.com> wrote: >> > >> >> @@ -2784,9 +2796,11 @@ static void cirrus_reset(void *opaque) >> >> } >> >> s->vga.cr[0x27] = s->device_id; >> >> >> >> - /* Win2K seems to assume that the pattern buffer is at 0xff >> >> - initially ! */ >> >> - memset(s->vga.vram_ptr, 0xff, s->real_vram_size); >> >> + if (!runstate_check(RUN_STATE_PREMIGRATE)) { >> >> + /* Win2K seems to assume that the pattern buffer is at 0xff >> >> + initially ! */ >> >> + memset(s->vga.vram_ptr, 0xff, s->real_vram_size); >> >> + } >> >> >> > >> > this is not too bad, I suppose that the videoram is going to be written >> > again at restore time anyway so at least it saves some cycles >> >> Actually, I think the next time that this vram will be written again >> is, when the guest is actually "waked-up" and wrote something there. >> Otherwise, the "restore" of the vram is done before QEMU start. So, >> the memset could leave some weard stuff the screen (a white screen?). > > So this is the succession of events: > > - vga_common_init > allocates the videoram > > - cirrus_reset > sets set videoram to 0xff > > - load_vmstate > the old videoram is copied over, overwriting what cirrus_reset has done > > therefore setting the videoram to 0xff when resuming is a waste of > efforts Ooops, I reduce my answer to the only Xen case. So I agree with you.
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c index c7e365b..d092c74 100644 --- a/hw/cirrus_vga.c +++ b/hw/cirrus_vga.c @@ -32,6 +32,7 @@ #include "console.h" #include "vga_int.h" #include "loader.h" +#include "sysemu.h" /* * TODO: @@ -2696,6 +2697,17 @@ static int cirrus_post_load(void *opaque, int version_id) s->vga.gr[0x01] = s->cirrus_shadow_gr1 & 0x0f; cirrus_update_memory_access(s); + if (!s->vga.vram_ptr) { + /* Try again + * The initial call to get_ram_ptr can fail with Xen during a migration + * because the VRAM has been moved arround. + * (Moved with a set_memory and a xen_add_to_physmap) + * After cirrus_update_memory_access, the Xen function will know were + * the memory actually is, and fix the address before give back a + * ram_ptr. + */ + s->vga.vram_ptr = memory_region_get_ram_ptr(&s->vga.vram); + } /* force refresh */ s->vga.graphic_mode = -1; cirrus_update_bank_ptr(s, 0); @@ -2784,9 +2796,11 @@ static void cirrus_reset(void *opaque) } s->vga.cr[0x27] = s->device_id; - /* Win2K seems to assume that the pattern buffer is at 0xff - initially ! */ - memset(s->vga.vram_ptr, 0xff, s->real_vram_size); + if (!runstate_check(RUN_STATE_PREMIGRATE)) { + /* Win2K seems to assume that the pattern buffer is at 0xff + initially ! */ + memset(s->vga.vram_ptr, 0xff, s->real_vram_size); + } s->cirrus_hidden_dac_lockindex = 5; s->cirrus_hidden_dac_data = 0;
During the initialisation of the machine at restore time, the access to the VRAM will fail because QEMU does not know yet the right guest address to map, so the vram_ptr is NULL. So this patch avoid using a NULL pointer during initialisation, and try to get another vram_ptr if the call failed the first time. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- hw/cirrus_vga.c | 20 +++++++++++++++++--- 1 files changed, 17 insertions(+), 3 deletions(-)