On 2011-12-12 14:18, Stefano Stabellini wrote:
> On Sat, 10 Dec 2011, Jan Kiszka wrote:
>> On 2011-12-09 22:54, 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.
>>>
>>> Signed-off-by: Anthony PERARD <anthony.per...@citrix.com>
>>> ---
>>> hw/cirrus_vga.c | 16 +++++++++++++---
>>> 1 files changed, 13 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>>> index c7e365b..2e049c9 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,13 @@ 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) {
>>> + /* 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. */
>>> + 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 +2792,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;
>>
>> Is there really no way to fix this properly in the Xen layer?
>
> We thought about this issue for some time but we couldn't come up with
> anything better.
> To summarize the problem:
>
> - on restore the videoram has already been loaded in the guest physical
> address space by Xen;
>
> - we don't know exactly where it is yet, because it has been loaded at
> the *last* address it was mapped to (see map_linear_vram_bank that
> moves the videoram);
>
> - we want to avoid allocating the videoram twice (actually the second
> allocation would fail because of memory constraints);
>
>
>
> So the solution (I acknowledge that it looks more like an hack than a
> solution) is:
>
> - wait for cirrus to load its state so that we know where the videoram
> is;
Why can't you store this information in some additional Xen-specific
vmstate? The fact that memory_region_get_ram_ptr has to return NULL
looks bogus to me. It's against the QEMU semantics. Other devices may
only be fine as they are not (yet) used with Xen.
>
> - map the videoram into qemu's address space at that point.
>
>
>
> Anything else we came up with was even worse, for example:
>
> - independently save the position of the videoram and then when
> vga_common_init tries to allocate it for a second time give back the
> old videoram instead;
>
> - move back the videoram to the original position and then move it again
> to the new position.
>
>
>
>> This looks
>> highly fragile as specifically the second hunk appears unrelated to Xen.
>
> I think that the second chuck should be in a separate patch because it
> is unrelated to Xen. On restore it is useless to memset the videoram, so
> for performance reasons it would be a good idea to avoid it on all
> platforms. Also it happens to crash Qemu on Xen but that is another
> story ;-)
>
> I think we should also add a comment:
>
> "useles to memset the videoram on restore, the old videoram is going to
> be copied over soon anyway"
That's in fact a different story and maybe worth optimizing due to the
size of that buffer. But please do not call the state "PREMIGRATE". It's
rather "INCOMING[_MIGRATION]".
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
