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

[Michael S. Tsirkin]

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<address@hidden>
> > > > > > >>
> > > > > > >>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.

> > 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?

> > 
> > > > 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 address@hidden
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > --
> > >                   Gleb.
> 
> --
>                       Gleb.