[v2,0/8] qemu-ga: add support for Windows

On Mon, Feb 06, 2012 at 07:09:42PM -0500, Kevin O'Connor wrote:
> On Mon, Feb 06, 2012 at 01:43:42PM -0200, Luiz Capitulino wrote:
> > "Kevin O'Connor" <kevin@koconnor.net> wrote:
> > > On Fri, Feb 03, 2012 at 05:16:27PM -0200, Luiz Capitulino wrote:
> > > > On Fri, 03 Feb 2012 11:23:05 -0600
> > > > Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> > > > > I'd been tracking Gerd's QMP wakeup series as the s3 resolution we need 
> > > > > for guest-suspend, is that still the case?
> > > > 
> > > > Yes. But now I remembered about a seabios bug with S3... Need to check if
> > > > it were already addressed.
> > > 
> > > I'm not aware of any recent S3 defects in SeaBIOS.  If there is a
> > > defect, please let me know.
> > > 
> > > (I am aware of recent discussions on SeaBIOS and it running the
> > > vgabios on s3-resume, but I would not classify that issue as a
> > > defect.)
> > 
> > The problem is that, the screen goes black after resuming from S3. Gleb
> > debugged it a bit and he said that it was caused by a change in seabios.
> > 
> > Please, take a look at the last three comments in this bz:
> > 
> >  https://bugzilla.redhat.com/show_bug.cgi?id=772614
> 
> Perhaps a semantic distinction, but I don't consider that to be a
> seabios defect.
> 
Non optimal default. The default didn't change BTW, but it was config
parameter before and we changed it for RHEL. Now config parameter is
gone.

> In any case, I don't think this was addressed.  Gerd published a patch
> that can address this in qemu:
> http://www.seabios.org/pipermail/seabios/2012-January/002944.html
> 
Strictly speaking the patch is incorrect since it introduces the file
for all architectures, but I do not think qemu is the right place to
tune SeaBIOS defaults. I propose this patch instead:
---

Run vgabios during S3 resume by default on QEMU. QEMU still able to modify
SeaBIOS behavior if it wishes so by providing etc/s3-resume-vga-init file.

Gleb Natapov <gleb@redhat.com>
--
			Gleb.

On Tue, Feb 07, 2012 at 10:44:39AM +0200, Gleb Natapov wrote:
> On Mon, Feb 06, 2012 at 07:09:42PM -0500, Kevin O'Connor wrote:
> > Perhaps a semantic distinction, but I don't consider that to be a
> > seabios defect.
> > 
> Non optimal default. The default didn't change BTW, but it was config
> parameter before and we changed it for RHEL. Now config parameter is
> gone.

Config parameter moved from compile time to runtime.  But I agree it
is unfortunate that knowledge of that didn't get to the right people.

> > In any case, I don't think this was addressed.  Gerd published a patch
> > that can address this in qemu:
> > http://www.seabios.org/pipermail/seabios/2012-January/002944.html
> > 
> Strictly speaking the patch is incorrect since it introduces the file
> for all architectures, but I do not think qemu is the right place to
> tune SeaBIOS defaults. I propose this patch instead:
[...]
>      // Load some config settings that impact VGA.
>      EnforceChecksum = romfile_loadint("etc/optionroms-checksum", 1);
> -    S3ResumeVgaInit = romfile_loadint("etc/s3-resume-vga-init", 0);
> +    S3ResumeVgaInit = romfile_loadint("etc/s3-resume-vga-init", !CONFIG_COREBOOT);
>      ScreenAndDebug = romfile_loadint("etc/screen-and-debug", 1);

I'm concerned about the VGA passthrough case.  (I know that's not
common and has other issues, but I also know several people have been
working with it.)  As near as I can tell, running the VGA rom on S3
resume has as much chance of breaking things as helping things.  It's
fine for the cirrus/bochsvga vgaroms that are totally under our
control, but it'd be an open guess for any third-party code.  (Again,
if someone has documentation to the contrary please let me know.)

So, compiling this into SeaBIOS doesn't seems like the right choice to
me.

-Kevin

On Tue, Feb 07, 2012 at 07:35:34PM -0500, Kevin O'Connor wrote:
> > > In any case, I don't think this was addressed.  Gerd published a patch
> > > that can address this in qemu:
> > > http://www.seabios.org/pipermail/seabios/2012-January/002944.html
> > > 
> > Strictly speaking the patch is incorrect since it introduces the file
> > for all architectures, but I do not think qemu is the right place to
> > tune SeaBIOS defaults. I propose this patch instead:
> [...]
> >      // Load some config settings that impact VGA.
> >      EnforceChecksum = romfile_loadint("etc/optionroms-checksum", 1);
> > -    S3ResumeVgaInit = romfile_loadint("etc/s3-resume-vga-init", 0);
> > +    S3ResumeVgaInit = romfile_loadint("etc/s3-resume-vga-init", !CONFIG_COREBOOT);
> >      ScreenAndDebug = romfile_loadint("etc/screen-and-debug", 1);
> 
> I'm concerned about the VGA passthrough case.  (I know that's not
> common and has other issues, but I also know several people have been
> working with it.)  As near as I can tell, running the VGA rom on S3
> resume has as much chance of breaking things as helping things.  It's
> fine for the cirrus/bochsvga vgaroms that are totally under our
> control, but it'd be an open guess for any third-party code.  (Again,
> if someone has documentation to the contrary please let me know.)
> 
VGA passthrough does not work with QEMU without code changes. Whoever
works on it will have to provide etc/s3-resume-vga-init file with
appropriate value. My patch above does not remove run time selection, it
only changes the default.

> So, compiling this into SeaBIOS doesn't seems like the right choice to
> me.
> 
It is still run time selectable. I think it is best of both worlds.

--
			Gleb.

On Wed, Feb 08, 2012 at 09:18:24AM +0200, Gleb Natapov wrote:
> On Tue, Feb 07, 2012 at 07:35:34PM -0500, Kevin O'Connor wrote:
> > I'm concerned about the VGA passthrough case.  (I know that's not
> > common and has other issues, but I also know several people have been
> > working with it.)  As near as I can tell, running the VGA rom on S3
> > resume has as much chance of breaking things as helping things.  It's
> > fine for the cirrus/bochsvga vgaroms that are totally under our
> > control, but it'd be an open guess for any third-party code.  (Again,
> > if someone has documentation to the contrary please let me know.)
> > 
> VGA passthrough does not work with QEMU without code changes. Whoever
> works on it will have to provide etc/s3-resume-vga-init file with
> appropriate value. My patch above does not remove run time selection, it
> only changes the default.

True.

I view running the vgabios on s3 a hack and think an explicit "please
apply hack" flag is nicer than the inverse.

However, it's clear this hack helps the majority of qemu/kvm users.
So, I'm okay with changing the default.  It is a change of default
though (upstream kvm/qemu has never run the vgabios on s3 resume
before).  So, we need to make sure there's proper notice of the change
and assuming no objection I'll go forward with it.

-Kevin