Re: [Qemu-devel] [PATCH V2 5/5] vga-cirrus: Workaround during restore when using Xen.

[Jan Kiszka <jan.kiszka@siemens.com>]

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.perard@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