Re: [Qemu-devel] Re: [PATCH 2/3] qemu: make cirrus init value pci spec compliant

[Gleb Natapov <gleb@redhat.com>]

On Fri, Oct 09, 2009 at 02:59:29PM +0200, Michael S. Tsirkin wrote:
> On Fri, Oct 09, 2009 at 01:49:03PM +0200, Gleb Natapov wrote:
> > On Fri, Oct 09, 2009 at 12:13:00PM +0200, Michael S. Tsirkin wrote:
> > > On Fri, Oct 09, 2009 at 09:00:49AM +0200, Gleb Natapov wrote:
> > > > On Fri, Oct 09, 2009 at 08:43:59AM +0200, Michael S. Tsirkin wrote:
> > > > > On Thu, Oct 08, 2009 at 07:40:12PM +0100, Jamie Lokier wrote:
> > > > > > Avi Kivity wrote:
> > > > > > > On 10/08/2009 06:06 PM, Michael S. Tsirkin wrote:
> > > > > > > >On Thu, Oct 08, 2009 at 05:29:29PM +0200, Avi Kivity wrote:
> > > > > > > >   
> > > > > > > >>On 10/08/2009 04:52 PM, Michael S. Tsirkin wrote:
> > > > > > > >>     
> > > > > > > >>>PCI memory should be disabled at reset, otherwise
> > > > > > > >>>we might claim transactions at address 0.
> > > > > > > >>>I/O should also be disabled, although for cirrus
> > > > > > > >>>it is harmless to enable it as we do not
> > > > > > > >>>have I/O bar.
> > > > > > > >>>
> > > > > > > >>>Note: need bios fix for this patch to work:
> > > > > > > >>>currently pc-bios incorrently assumes that it does not
> > > > > > > >>>need to enable i/o unless device has i/o bar.
> > > > > > > >>>
> > > > > > > >>>Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> > > > > > > >>
> > > > > > > >>This needs to be conditional on the machine type.  Old machines must
> > > > > > > >>retain this code for live migration to work (we need to live migrate the
> > > > > > > >>bios, so we can't assume the bios fix is in during live migration from
> > > > > > > >>older qemus).
> > > > > > > >>     
> > > > > > > >No, if you migrate from older qemu you will be fine as command
> > > > > > > >is enabled there on init.
> > > > > > > >   
> > > > > > > 
> > > > > > > Right.
> > > > > > 
> > > > > > No, I think Avi was right the first time.
> > > > > > 
> > > > > > Migrating from an older qemu will be fine at first, but at the next
> > > > > > reset _following_ migration, it'll be running the old BIOS on a new
> > > > > > qemu and fail.
> > > > > > 
> > > > > > -- Jamie
> > > > > 
> > > > > I think this just means that there's another bug.
> > > > > On reset BIOS should be re-read from flash.
> > > > > qemu instead keeps it in RAM, this means that if
> > > > > guest corrupts it, or BIOS itself runs self-modifying
> > > > > code (which is not uncommon btw), bad things happen.
> > > > > 
> > > > How do you know it is not uncommon (or happens at all?)
> > > 
> > > I know memory corruptions are not uncommon, from experience :)
> > > 
> > I don't see how from presence of memory corruptions you made a
> > conclusion that BIOSes run self-modifying code.
> > > > If BIOS is not shadowed it runs directly from ROM. Hard to use
> > > > self-modifying code there.
> > > 
> > > Sorry I don't really know how this is handled in qemu.
> > > Can it run BIOS from ROM? Is ROM content sent over for migration?
> > Yes it can. There is no much difference between ROM and RAM in qemu
> > though. I don't know if ROM content is sent over for migration, but
> > IMHO it shouldn't. This will solve the problem we currently discuss.
> > Shadowed BIOS should be copied of course. 
> 
> Interesting. OTOH this means that after migration,
> shadowed BIOS and ROM are different. In theory, this
> can create problems. In practice I doubt this.
> 
What kind of problems? After ROM BIOS is shadowed it is no
logger used until reset. Its RAM copy is used.

> > > I see this
> > >      ret = load_image(filename, qemu_get_ram_ptr(bios_offset));
> > > which seems to always load BIOS into RAM.
> > > Maybe I misunderstand. Could you enlighten me please?
> > Enlighten you about what? BIOS copies itself into shadow ram. At least
> > BOCHS bios does. I hope seabios does the same.
> 
> People were implying changing BIOS triggers migration/reset issues.
> Do you think so too?
> 
In this thread people seems to be implying that _not_ changing BIOS
trigger the problem on reset if destination no logger init VGA register
and relies on BIOS to do that instead.

Changing RAM copy of the BIOS at runtime may cause problems with TPR
patching for instance. Changing ROM copy shouldn't be the problem. It
will be the same as burning new ROM from running system. Until reset RAM
copy of a BIOS will be used, after reset new BIOS will run and will
shadow itself.

> > > 
> > > > > We should fix qemu to re-read bios from flash.
> > > > > Makes sense?
> > > > > 
> > > > We have option_rom_setup_reset() already.
> > > 
> > > We don't, anymore :)
> > We are fast! Well, we surely have something similar instead.
> > 
> > > 
> > > > Don't we use it for BIOSes
> > > > too?
> > > > 
> > > > > More long-term, we have duplication between reset and init
> > > > > routines. Maybe devices really should have init and cleanup,
> > > > > and on reset we'd cleanup all devices and then init them again?
> > > > > There are cases where state needs to be persistent across
> > > > > resets (VPD comes to mind) but these are rare,
> > > > > and probably need to be backed by writing to file anyway?
> > > > > 
> > > > > 
> > > > > -- 
> > > > > MST
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > 
> > > > --
> > > > 			Gleb.
> > 
> > --
> > 			Gleb.

--
			Gleb.