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

[Jan Kiszka <jan.kiszka@xxxxxxxxxxx>]

On 2011-12-12 15:41, Stefano Stabellini wrote:
> On Mon, 12 Dec 2011, Jan Kiszka wrote:
>> 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@xxxxxxxxxx>
>>>>> ---
>>>>>  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.
> 
> Unfortunately that cannot work because the allocation is done by
> vga_common_init before any state has been loaded.
> So even if we saved the physmap QLIST in a vmstate record, it wouldn't
> be available yet when vga_common_init tries to allocate the memory.

If you run two VMs with identical setup, one from cold start to
operational mode, the other into an incoming migration, the guest
physical memory layout the host sees is different? Hmm, maybe if you
reorder devices in the command line.

Really, I think this is something inherently incompatible with the
current memory API. If Xen has this unfixable special "requirement"
(it's rather a design issue IMHO), adjust the API and adapt all devices.
Hot-fixing only a single one this way is no good idea long term.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel