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

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

[Isaku Yamahata]

commit 508db40e2c8714a43016a77b7bc32a5fe15577a9
Author: Isaku Yamahata <yamahata@valinux.co.jp>
Date:   Thu Jan 15 21:23:12 2009 +0900

    vga: initialize vram area to 0xff
    
    Initialize vram memory in xen_vga_populate_vram().
    and some fixes.
    
    Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index f2ec05b..e1a416b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3167,8 +3167,9 @@ static int cirrus_vga_load(QEMUFile *f, void *opaque, int version_id)
          * ourselves */
         s->vram_gmfn = vga_acc ? s->lfb_addr : VRAM_RESERVED_ADDRESS;
         xen_vga_populate_vram(s->vram_gmfn, s->vram_size);
+    } else {
+        xen_vga_vram_map(s->vram_gmfn, s->vram_size);
     }
-    xen_vga_vram_map(s->vram_gmfn, s->vram_size);
     if (version_id < 3 || (!vga_acc && !t))
         qemu_get_buffer(f, s->vram_ptr, s->vram_size);
 
diff --git a/hw/vga.c b/hw/vga.c
index 2ae6e85..4c3b57e 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2251,7 +2251,6 @@ static int vga_load(QEMUFile *f, void *opaque, int version_id)
         /* Old guest, VRAM is not mapped, we have to restore it ourselves */
         if (!s->vram_gmfn) {
             xen_vga_populate_vram(VRAM_RESERVED_ADDRESS, s->vram_size);
-            xen_vga_vram_map(VRAM_RESERVED_ADDRESS, s->vram_size);
             s->vram_gmfn = VRAM_RESERVED_ADDRESS;
             qemu_get_buffer(f, s->vram_ptr, s->vram_size); 
         }
@@ -2447,6 +2446,12 @@ void xen_vga_populate_vram(uint64_t vram_addr, uint32_t vga_ram_size)
         exit(1);
     }
     free(pfn_list);
+
+    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);
 }
 
 /* Mapping the video memory from GPFN space  */
@@ -2482,6 +2487,10 @@ void xen_vga_vram_map(uint64_t vram_addr, uint32_t vga_ram_size)
 #ifdef CONFIG_STUBDOM
     xenfb_pv_display_start(vram);
 #endif
+    /* 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);
 }
 
 /* when used on xen environment, the vga_ram_base is not used */
@@ -2526,7 +2535,6 @@ void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
 
     if (!restore) {
         xen_vga_populate_vram(VRAM_RESERVED_ADDRESS, s->vram_size);
-        xen_vga_vram_map(VRAM_RESERVED_ADDRESS, s->vram_size);
         s->vram_gmfn = VRAM_RESERVED_ADDRESS;
     }
 


-- 
yamahata

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

[Stefano Stabellini]

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?

> +    /* 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.

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

[Isaku Yamahata]

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


> > +    /* 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.
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);



-- 
yamahata

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

[Stefano Stabellini]

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.

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH 2/2] ioemu: initialize vram area to 0xff"):
> [stuff]

We seem to be reaching agreement here ?  I'm afraid I've slightly lost
track of exactly which hunks of Isaku's patch are now to be
applied ...

Ian.

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

[Stefano Stabellini]

Ian Jackson wrote:

> Stefano Stabellini writes ("Re: [PATCH 2/2] ioemu: initialize vram area to 0xff"):
>> [stuff]
> 
> We seem to be reaching agreement here ?  I'm afraid I've slightly lost
> track of exactly which hunks of Isaku's patch are now to be
> applied ...
> 
> Ian.
> 



Something like this:

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index f2ec05b..e1a416b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3167,8 +3167,9 @@ static int cirrus_vga_load(QEMUFile *f, void *opaque, int version_id)
          * ourselves */
         s->vram_gmfn = vga_acc ? s->lfb_addr : VRAM_RESERVED_ADDRESS;
         xen_vga_populate_vram(s->vram_gmfn, s->vram_size);
+    } else {
+        xen_vga_vram_map(s->vram_gmfn, s->vram_size);
     }
-    xen_vga_vram_map(s->vram_gmfn, s->vram_size);
     if (version_id < 3 || (!vga_acc && !t))
         qemu_get_buffer(f, s->vram_ptr, s->vram_size);
 
diff --git a/hw/vga.c b/hw/vga.c
index 2ae6e85..d1d61ea 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2251,7 +2251,6 @@ static int vga_load(QEMUFile *f, void *opaque, int version_id)
         /* Old guest, VRAM is not mapped, we have to restore it ourselves */
         if (!s->vram_gmfn) {
             xen_vga_populate_vram(VRAM_RESERVED_ADDRESS, s->vram_size);
-            xen_vga_vram_map(VRAM_RESERVED_ADDRESS, s->vram_size);
             s->vram_gmfn = VRAM_RESERVED_ADDRESS;
             qemu_get_buffer(f, s->vram_ptr, s->vram_size); 
         }
@@ -2447,6 +2446,12 @@ void xen_vga_populate_vram(uint64_t vram_addr, uint32_t vga_ram_size)
         exit(1);
     }
     free(pfn_list);
+
+    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);
 }
 
 /* Mapping the video memory from GPFN space  */
@@ -2456,7 +2461,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);
@@ -2526,7 +2530,6 @@ void vga_common_init(VGAState *s, DisplayState *ds, uint8_t *vga_ram_base,
 
     if (!restore) {
         xen_vga_populate_vram(VRAM_RESERVED_ADDRESS, s->vram_size);
-        xen_vga_vram_map(VRAM_RESERVED_ADDRESS, s->vram_size);
         s->vram_gmfn = VRAM_RESERVED_ADDRESS;
     }

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

[Ian Jackson]

Stefano Stabellini writes ("Re: [PATCH 2/2] ioemu: initialize vram area to 0xff"):
> Ian Jackson wrote:
> > We seem to be reaching agreement here ?  I'm afraid I've slightly lost
> > track of exactly which hunks of Isaku's patch are now to be
> > applied ...
> 
> Something like this:

Great, thanks.  Applied.

Ian.