Re: [PATCH 2/2] ioemu: initialize vram area to 0xff

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

Isaku Yamahata wrote:

> On Thu, Jan 15, 2009 at 12:49:08PM +0000, Stefano Stabellini wrote:
>> Isaku Yamahata wrote:
>>
>>> +
>>> +    xen_vga_vram_map(vram_addr, vga_ram_size);
>>> +
>>> +    /* Win2K seems to assume that the pattern buffer is at 0xff
>>> +       initially ! */
>>> +    memset(xen_vga_state->vram_ptr, 0xff, vga_ram_size);
>>
>> Are you sure that this is really needed?
>> Isn't the Bios supposed to take care of that?
> 
> To be honest I'm not very sure.
> Without those lines, on ia64 garbage remains on screen after
> the initialization by guest firmware.
> And 'git annotate' told me that the lines came from the following
> change set.
> But you eliminated them by patch 2/4. I don't know why you were able
> to safely eliminate it.
> 
> commit 78e127efdbac82f148ed4f7b42f8f4f2873b4c5b
> Author: bellard <bellard@c046a42c-6fe2-441c-8c8c-71466251a162>
> Date:   Tue Jun 8 00:58:26 2004 +0000
> 
>     set memory size to 4MB for 5446 - fixed memory size probe (aka Win2000 bug) - fixed interlace support
>    
>    
>    git-svn-id: svn://svn.savannah.nongnu.org/qemu/trunk@914 c046a42c-6fe2-441c-8c8c-71466251a162
> 


I eliminated them because Windows 2000 worked fine for me even without
those lines.
In any case if you need them we can just add them back, they are
harmless (as long as they are executed after mapping the framebuffer of
course).

 
>>> +    /* If some display is already working, we need to update it now */
>>> +    ds= xen_vga_state->ds;
>>> +    if (ds)
>>> +        dpy_update(ds, 0,0, ds->width,ds->height);
>>
>> I don't think this is needed as well: xen_vga_vram_map is called either
>> during the initialization state or when loading the vm state file.
>> At both points the display shouldn't need any update.
> 
> I just supposed that it had been missed by cut and paset
> because the local variable, ds, isn't eliminated.


No it wasn't a cut and past error, it was deliberate.

> Then what you want is something like the following, isn't it?
> diff --git a/hw/vga.c b/hw/vga.c
> index 8aa61c6..9c45b1b 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -2456,7 +2456,6 @@ void xen_vga_vram_map(uint64_t vram_addr, uint32_t vga_ram_size)
>      xen_pfn_t *pfn_list;
>      int i;
>      void *vram;
> -    DisplayState *ds;
>  
>      fprintf(logfile, "mapping video RAM from %llx\n",
>  	    (unsigned long long)vram_addr);
> 
> 
> 


Yes, I forgot to remove the local variable.