Patchwork [2/3] qemu: make cirrus init value pci spec compliant

login
register
mail settings
Submitter Michael S. Tsirkin
Date Oct. 8, 2009, 2:52 p.m.
Message ID <20091008145241.GC12275@redhat.com>
Download mbox | patch
Permalink /patch/35454/
State New
Headers show

Comments

Michael S. Tsirkin - Oct. 8, 2009, 2:52 p.m.
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>
---
 hw/cirrus_vga.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
Avi Kivity - Oct. 8, 2009, 3:29 p.m.
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>
> ---
>   hw/cirrus_vga.c |    1 -
>   1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 1feb827..c54ac65 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -3248,7 +3248,6 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>        /* setup PCI */
>        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CIRRUS);
>        pci_config_set_device_id(pci_conf, device_id);
> -     pci_conf[0x04] = PCI_COMMAND_IOACCESS | PCI_COMMAND_MEMACCESS;
>        pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA);
>        pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
>    

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).
Michael S. Tsirkin - Oct. 8, 2009, 4:06 p.m.
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>
>> ---
>>   hw/cirrus_vga.c |    1 -
>>   1 files changed, 0 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>> index 1feb827..c54ac65 100644
>> --- a/hw/cirrus_vga.c
>> +++ b/hw/cirrus_vga.c
>> @@ -3248,7 +3248,6 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>>        /* setup PCI */
>>        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CIRRUS);
>>        pci_config_set_device_id(pci_conf, device_id);
>> -     pci_conf[0x04] = PCI_COMMAND_IOACCESS | PCI_COMMAND_MEMACCESS;
>>        pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA);
>>        pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
>>    
>
> 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.

Only migration *to* older qemu might create a problem and then only if
you manage to migrate after pci was reset but before bios set the
register, which is a small window.

Do we really want to update machine type for this?

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 8, 2009, 4:13 p.m.
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>
>>> ---
>>>    hw/cirrus_vga.c |    1 -
>>>    1 files changed, 0 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>>> index 1feb827..c54ac65 100644
>>> --- a/hw/cirrus_vga.c
>>> +++ b/hw/cirrus_vga.c
>>> @@ -3248,7 +3248,6 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>>>         /* setup PCI */
>>>         pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CIRRUS);
>>>         pci_config_set_device_id(pci_conf, device_id);
>>> -     pci_conf[0x04] = PCI_COMMAND_IOACCESS | PCI_COMMAND_MEMACCESS;
>>>         pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA);
>>>         pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
>>>
>>>        
>> 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.

> Only migration *to* older qemu might create a problem and then only if
> you manage to migrate after pci was reset but before bios set the
> register, which is a small window.
>
> Do we really want to update machine type for this?
>    

No, migration to older qemu is not supported.
Jamie Lokier - Oct. 8, 2009, 6:40 p.m.
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
Michael S. Tsirkin - Oct. 8, 2009, 7:39 p.m.
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.

OK but Avi was commenting on the patch that removed bit set in init,
ignoring reset behaviour.
So for old BIOS we want a flag not to clear I/O enable bit
on reset for VGA devices.


> -- Jamie
Michael S. Tsirkin - Oct. 9, 2009, 6:43 a.m.
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.

We should fix qemu to re-read bios from flash.
Makes sense?

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?
Gleb Natapov - Oct. 9, 2009, 7 a.m.
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?)
If BIOS is not shadowed it runs directly from ROM. Hard to use
self-modifying code there.

> We should fix qemu to re-read bios from flash.
> Makes sense?
> 
We have option_rom_setup_reset() already. 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.
Michael S. Tsirkin - Oct. 9, 2009, 10:13 a.m.
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 :)

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

> > We should fix qemu to re-read bios from flash.
> > Makes sense?
> > 
> We have option_rom_setup_reset() already.

We don't, anymore :)

> 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.
Jamie Lokier - Oct. 9, 2009, 11:37 a.m.
Michael S. Tsirkin wrote:
> 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?

It sounds look a good idea to me.  That is, after all, what hardware
reset often does :-)

-- Jamie
Gleb Natapov - Oct. 9, 2009, 11:49 a.m.
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. 

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

> 
> > > 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 Natapov - Oct. 9, 2009, 11:54 a.m.
On Fri, Oct 09, 2009 at 12:37:38PM +0100, Jamie Lokier wrote:
> Michael S. Tsirkin wrote:
> > 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?
> 
> It sounds look a good idea to me.  That is, after all, what hardware
> reset often does :-)
> 
That is a two stage reset that was discussed already and deemed to be
the way to go.

--
			Gleb.
Michael S. Tsirkin - Oct. 9, 2009, 12:59 p.m.
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.

> > 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 majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > --
> > > 			Gleb.
> 
> --
> 			Gleb.
Gleb Natapov - Oct. 9, 2009, 3:08 p.m.
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.
Michael S. Tsirkin - Oct. 11, 2009, 1:36 p.m.
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

So after discussion with Gleb, it seems that what will happen
after migration to new qemu, old BIOS is shadowed, but memory
is already enabled.  After reset, new BIOS will be shadowed
and it will enable memory. Makes sense?
Gleb Natapov - Oct. 11, 2009, 1:45 p.m.
On Sun, Oct 11, 2009 at 03:36:26PM +0200, Michael S. Tsirkin wrote:
> > 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
> 
> So after discussion with Gleb, it seems that what will happen
> after migration to new qemu, old BIOS is shadowed, but memory
> is already enabled.  After reset, new BIOS will be shadowed
> and it will enable memory. Makes sense?
> 
I don't see how you made this conclusion after the discussion.
What should happen is after migration old bios should be used until reboot.
After reboot new bios should be used. What really happens today I don't
know.

--
			Gleb.
Michael S. Tsirkin - Oct. 11, 2009, 1:52 p.m.
On Sun, Oct 11, 2009 at 03:45:59PM +0200, Gleb Natapov wrote:
> On Sun, Oct 11, 2009 at 03:36:26PM +0200, Michael S. Tsirkin wrote:
> > > 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
> > 
> > So after discussion with Gleb, it seems that what will happen
> > after migration to new qemu, old BIOS is shadowed, but memory
> > is already enabled.  After reset, new BIOS will be shadowed
> > and it will enable memory. Makes sense?
> > 
> I don't see how you made this conclusion after the discussion.
> What should happen is after migration old bios should be used until reboot.
> After reboot new bios should be used.

I think that's what I said.

> What really happens today I don't know.
> --
> 			Gleb.
Gleb Natapov - Oct. 11, 2009, 1:57 p.m.
On Sun, Oct 11, 2009 at 03:52:40PM +0200, Michael S. Tsirkin wrote:
> On Sun, Oct 11, 2009 at 03:45:59PM +0200, Gleb Natapov wrote:
> > On Sun, Oct 11, 2009 at 03:36:26PM +0200, Michael S. Tsirkin wrote:
> > > > 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
> > > 
> > > So after discussion with Gleb, it seems that what will happen
> > > after migration to new qemu, old BIOS is shadowed, but memory
> > > is already enabled.  After reset, new BIOS will be shadowed
> > > and it will enable memory. Makes sense?
> > > 
> > I don't see how you made this conclusion after the discussion.
> > What should happen is after migration old bios should be used until reboot.
> > After reboot new bios should be used.
> 
> I think that's what I said.
> 
First shadowing bios has nothing to do with it. Second what do you mean
by "will happen"? That is how code works now or that is how it should
work?  I am not sure this is how it works.

--
			Gleb.
Michael S. Tsirkin - Oct. 11, 2009, 2:35 p.m.
On Sun, Oct 11, 2009 at 03:57:48PM +0200, Gleb Natapov wrote:
> On Sun, Oct 11, 2009 at 03:52:40PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Oct 11, 2009 at 03:45:59PM +0200, Gleb Natapov wrote:
> > > On Sun, Oct 11, 2009 at 03:36:26PM +0200, Michael S. Tsirkin wrote:
> > > > > 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
> > > > 
> > > > So after discussion with Gleb, it seems that what will happen
> > > > after migration to new qemu, old BIOS is shadowed, but memory
> > > > is already enabled.  After reset, new BIOS will be shadowed
> > > > and it will enable memory. Makes sense?
> > > > 
> > > I don't see how you made this conclusion after the discussion.
> > > What should happen is after migration old bios should be used until reboot.
> > > After reboot new bios should be used.
> > 
> > I think that's what I said.
> > 
> First shadowing bios has nothing to do with it. Second what do you mean
> by "will happen"? That is how code works now or that is how it should
> work?  I am not sure this is how it works.

No, this is not how it works, I think.
Currently BIOS ROM is loaded only on init, directly into qemu memory.
BTW, I don't think it's write-protected and it probably should be?

> 
> --
> 			Gleb.
Gleb Natapov - Oct. 11, 2009, 2:39 p.m.
On Sun, Oct 11, 2009 at 04:35:35PM +0200, Michael S. Tsirkin wrote:
> On Sun, Oct 11, 2009 at 03:57:48PM +0200, Gleb Natapov wrote:
> > On Sun, Oct 11, 2009 at 03:52:40PM +0200, Michael S. Tsirkin wrote:
> > > On Sun, Oct 11, 2009 at 03:45:59PM +0200, Gleb Natapov wrote:
> > > > On Sun, Oct 11, 2009 at 03:36:26PM +0200, Michael S. Tsirkin wrote:
> > > > > > 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
> > > > > 
> > > > > So after discussion with Gleb, it seems that what will happen
> > > > > after migration to new qemu, old BIOS is shadowed, but memory
> > > > > is already enabled.  After reset, new BIOS will be shadowed
> > > > > and it will enable memory. Makes sense?
> > > > > 
> > > > I don't see how you made this conclusion after the discussion.
> > > > What should happen is after migration old bios should be used until reboot.
> > > > After reboot new bios should be used.
> > > 
> > > I think that's what I said.
> > > 
> > First shadowing bios has nothing to do with it. Second what do you mean
> > by "will happen"? That is how code works now or that is how it should
> > work?  I am not sure this is how it works.
> 
> No, this is not how it works, I think.
> Currently BIOS ROM is loaded only on init, directly into qemu memory.
And have you checked if it is transfered to destination on migrate?

> BTW, I don't think it's write-protected and it probably should be?
> 
AFAIR it is not writable on plain qemu.

--
			Gleb.
Avi Kivity - Oct. 11, 2009, 2:42 p.m.
On 10/11/2009 04:35 PM, Michael S. Tsirkin wrote:
> No, this is not how it works, I think.
> Currently BIOS ROM is loaded only on init, directly into qemu memory.
>    

If we change it we break in-place upgrades - install qemu, run guest, 
upgrade qemu, reboot guest - now the old qemu runs with a new bios which 
may or may not work.
Michael S. Tsirkin - Oct. 11, 2009, 2:44 p.m.
On Sun, Oct 11, 2009 at 04:42:27PM +0200, Avi Kivity wrote:
> On 10/11/2009 04:35 PM, Michael S. Tsirkin wrote:
>> No, this is not how it works, I think.
>> Currently BIOS ROM is loaded only on init, directly into qemu memory.
>>    
>
> If we change it we break in-place upgrades - install qemu, run guest,  
> upgrade qemu, reboot guest - now the old qemu runs with a new bios which  
> may or may not work.

Sorry, could not parse this.
If you upgraded qemu, you are running with new qemu with new bios?

> -- 
> error compiling committee.c: too many arguments to function
Michael S. Tsirkin - Oct. 11, 2009, 2:45 p.m.
On Sun, Oct 11, 2009 at 04:39:46PM +0200, Gleb Natapov wrote:
> On Sun, Oct 11, 2009 at 04:35:35PM +0200, Michael S. Tsirkin wrote:
> > On Sun, Oct 11, 2009 at 03:57:48PM +0200, Gleb Natapov wrote:
> > > On Sun, Oct 11, 2009 at 03:52:40PM +0200, Michael S. Tsirkin wrote:
> > > > On Sun, Oct 11, 2009 at 03:45:59PM +0200, Gleb Natapov wrote:
> > > > > On Sun, Oct 11, 2009 at 03:36:26PM +0200, Michael S. Tsirkin wrote:
> > > > > > > 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
> > > > > > 
> > > > > > So after discussion with Gleb, it seems that what will happen
> > > > > > after migration to new qemu, old BIOS is shadowed, but memory
> > > > > > is already enabled.  After reset, new BIOS will be shadowed
> > > > > > and it will enable memory. Makes sense?
> > > > > > 
> > > > > I don't see how you made this conclusion after the discussion.
> > > > > What should happen is after migration old bios should be used until reboot.
> > > > > After reboot new bios should be used.
> > > > 
> > > > I think that's what I said.
> > > > 
> > > First shadowing bios has nothing to do with it. Second what do you mean
> > > by "will happen"? That is how code works now or that is how it should
> > > work?  I am not sure this is how it works.
> > 
> > No, this is not how it works, I think.
> > Currently BIOS ROM is loaded only on init, directly into qemu memory.
> And have you checked if it is transfered to destination on migrate?
> 
> > BTW, I don't think it's write-protected and it probably should be?
> > 
> AFAIR it is not writable on plain qemu.

What makes it non-writeable?
It goes:
    bios_offset = qemu_ram_alloc(bios_size);
    ret = load_image(filename, qemu_get_ram_ptr(bios_offset));
Does qemu_ram_alloc allocate read-only memory then?

> --
> 			Gleb.
Avi Kivity - Oct. 11, 2009, 2:53 p.m.
On 10/11/2009 04:44 PM, Michael S. Tsirkin wrote:
> On Sun, Oct 11, 2009 at 04:42:27PM +0200, Avi Kivity wrote:
>    
>> On 10/11/2009 04:35 PM, Michael S. Tsirkin wrote:
>>      
>>> No, this is not how it works, I think.
>>> Currently BIOS ROM is loaded only on init, directly into qemu memory.
>>>
>>>        
>> If we change it we break in-place upgrades - install qemu, run guest,
>> upgrade qemu, reboot guest - now the old qemu runs with a new bios which
>> may or may not work.
>>      
> Sorry, could not parse this.
> If you upgraded qemu, you are running with new qemu with new bios?
>    

The guest is running during the upgrade, so the qemu executable is still 
from the old install (but deleted).
Gleb Natapov - Oct. 11, 2009, 3 p.m.
On Sun, Oct 11, 2009 at 04:45:46PM +0200, Michael S. Tsirkin wrote:
> > AFAIR it is not writable on plain qemu.
> 
> What makes it non-writeable?
> It goes:
>     bios_offset = qemu_ram_alloc(bios_size);
>     ret = load_image(filename, qemu_get_ram_ptr(bios_offset));
> Does qemu_ram_alloc allocate read-only memory then?
> 
Look closer.

--
			Gleb.
Michael S. Tsirkin - Oct. 11, 2009, 4:12 p.m.
On Sun, Oct 11, 2009 at 04:53:39PM +0200, Avi Kivity wrote:
> On 10/11/2009 04:44 PM, Michael S. Tsirkin wrote:
>> On Sun, Oct 11, 2009 at 04:42:27PM +0200, Avi Kivity wrote:
>>    
>>> On 10/11/2009 04:35 PM, Michael S. Tsirkin wrote:
>>>      
>>>> No, this is not how it works, I think.
>>>> Currently BIOS ROM is loaded only on init, directly into qemu memory.
>>>>
>>>>        
>>> If we change it we break in-place upgrades - install qemu, run guest,
>>> upgrade qemu, reboot guest - now the old qemu runs with a new bios which
>>> may or may not work.
>>>      
>> Sorry, could not parse this.
>> If you upgraded qemu, you are running with new qemu with new bios?
>>    
>
> The guest is running during the upgrade, so the qemu executable is still  
> from the old install (but deleted).

Oh, I don't think we should re-read it from file each time.
We could read it at startup, keep a copy around for reboots.

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 11, 2009, 4:24 p.m.
On 10/11/2009 06:12 PM, Michael S. Tsirkin wrote:
>
>> The guest is running during the upgrade, so the qemu executable is still
>> from the old install (but deleted).
>>      
> Oh, I don't think we should re-read it from file each time.
> We could read it at startup, keep a copy around for reboots.
>    

We do that, see hw/loader.c.
Michael S. Tsirkin - Oct. 11, 2009, 4:33 p.m.
On Sun, Oct 11, 2009 at 06:24:30PM +0200, Avi Kivity wrote:
> On 10/11/2009 06:12 PM, Michael S. Tsirkin wrote:
>>
>>> The guest is running during the upgrade, so the qemu executable is still
>>> from the old install (but deleted).
>>>      
>> Oh, I don't think we should re-read it from file each time.
>> We could read it at startup, keep a copy around for reboots.
>>    
>
> We do that, see hw/loader.c.

Which function specifically?

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 11, 2009, 4:37 p.m.
On 10/11/2009 06:33 PM, Michael S. Tsirkin wrote:
> On Sun, Oct 11, 2009 at 06:24:30PM +0200, Avi Kivity wrote:
>    
>> On 10/11/2009 06:12 PM, Michael S. Tsirkin wrote:
>>      
>>>        
>>>> The guest is running during the upgrade, so the qemu executable is still
>>>> from the old install (but deleted).
>>>>
>>>>          
>>> Oh, I don't think we should re-read it from file each time.
>>> We could read it at startup, keep a copy around for reboots.
>>>
>>>        
>> We do that, see hw/loader.c.
>>      
> Which function specifically?
>    

rom_add_file().
Michael S. Tsirkin - Oct. 11, 2009, 4:39 p.m.
On Sun, Oct 11, 2009 at 06:37:17PM +0200, Avi Kivity wrote:
> On 10/11/2009 06:33 PM, Michael S. Tsirkin wrote:
>> On Sun, Oct 11, 2009 at 06:24:30PM +0200, Avi Kivity wrote:
>>    
>>> On 10/11/2009 06:12 PM, Michael S. Tsirkin wrote:
>>>      
>>>>        
>>>>> The guest is running during the upgrade, so the qemu executable is still
>>>>> from the old install (but deleted).
>>>>>
>>>>>          
>>>> Oh, I don't think we should re-read it from file each time.
>>>> We could read it at startup, keep a copy around for reboots.
>>>>
>>>>        
>>> We do that, see hw/loader.c.
>>>      
>> Which function specifically?
>>    
>
> rom_add_file().

Is it used for BIOS? pc.c seems to call this for initrd only?

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 11, 2009, 4:48 p.m.
On 10/11/2009 06:39 PM, Michael S. Tsirkin wrote:
>> rom_add_file().
>>      
> Is it used for BIOS? pc.c seems to call this for initrd only?
>    

It's used indirectly for everything.
Michael S. Tsirkin - Oct. 11, 2009, 5:18 p.m.
On Sun, Oct 11, 2009 at 06:48:34PM +0200, Avi Kivity wrote:
> On 10/11/2009 06:39 PM, Michael S. Tsirkin wrote:
>>> rom_add_file().
>>>      
>> Is it used for BIOS? pc.c seems to call this for initrd only?
>>    
>
> It's used indirectly for everything.

So we are fine then? After reset new BIOS will get used?

> -- 
> error compiling committee.c: too many arguments to function
Avi Kivity - Oct. 11, 2009, 5:27 p.m.
On 10/11/2009 07:18 PM, Michael S. Tsirkin wrote:
> On Sun, Oct 11, 2009 at 06:48:34PM +0200, Avi Kivity wrote:
>    
>> On 10/11/2009 06:39 PM, Michael S. Tsirkin wrote:
>>      
>>>> rom_add_file().
>>>>
>>>>          
>>> Is it used for BIOS? pc.c seems to call this for initrd only?
>>>
>>>        
>> It's used indirectly for everything.
>>      
> So we are fine then? After reset new BIOS will get used?
>    

Yes.
Avi Kivity - Oct. 11, 2009, 5:28 p.m.
On 10/11/2009 07:27 PM, Avi Kivity wrote:
> So we are fine then? After reset new BIOS will get used?
>
> Yes.
>

I meant, we are fine for upgrade, and no, the new BIOS will not be 
used.  The file is only read once.
Michael S. Tsirkin - Oct. 11, 2009, 6:10 p.m.
On Sun, Oct 11, 2009 at 07:28:13PM +0200, Avi Kivity wrote:
> On 10/11/2009 07:27 PM, Avi Kivity wrote:
>> So we are fine then? After reset new BIOS will get used?
>>
>> Yes.
>>
>
> I meant, we are fine for upgrade, and no, the new BIOS will not be used.  
> The file is only read once.

So this is what happens:
- after migration from old qemu to new old BIOS is used until reset
- after reset new BIOS is used
- after upgrade from old version of qemu upgrade does not have any
  effect until qemu is restarted

If this is a fair summary, the only issue with fixing BIOS bug
is migration from new qemu to old qemu, and we do not care much.


> -- 
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.
Avi Kivity - Oct. 11, 2009, 8:41 p.m.
On 10/11/2009 08:10 PM, Michael S. Tsirkin wrote:
>
> So this is what happens:
> - after migration from old qemu to new old BIOS is used until reset
> - after reset new BIOS is used
> - after upgrade from old version of qemu upgrade does not have any
>    effect until qemu is restarted
>
> If this is a fair summary, the only issue with fixing BIOS bug
> is migration from new qemu to old qemu, and we do not care much.
>    

Yes.
Anthony Liguori - Oct. 12, 2009, 1:44 p.m.
Gleb Natapov wrote
>> BTW, I don't think it's write-protected and it probably should be?
>>
>>     
> AFAIR it is not writable on plain qemu.
>   

KVM wants it to be writable for the TPR optimization.  Historically, it 
was read-only in QEMU but it changed to read-write in order to fake 
coreboot into thinking that the bios implemented PMM which it doesn't.
Gerd Hoffmann - Oct. 12, 2009, 1:45 p.m.
>>>> 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.

Sure.  Try 'info roms'.  Everything listed there will be reloaded on 
reset (unless mem=rom, in which case the guest should not be able to 
modify the bits in the first place).  That includes vgabios, optionroms, 
-kernel images, ...

cheers,
   Gerd
Gleb Natapov - Oct. 12, 2009, 1:53 p.m.
On Mon, Oct 12, 2009 at 03:45:47PM +0200, Gerd Hoffmann wrote:
> >>>>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.
> 
> Sure.  Try 'info roms'.  Everything listed there will be reloaded on
> reset (unless mem=rom, in which case the guest should not be able to
> modify the bits in the first place).  That includes vgabios,
> optionroms, -kernel images, ...
> 
And what about bios? I don't see it there.

--
			Gleb.
Gleb Natapov - Oct. 12, 2009, 2:21 p.m.
On Mon, Oct 12, 2009 at 08:44:56AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote
> >>BTW, I don't think it's write-protected and it probably should be?
> >>
> >AFAIR it is not writable on plain qemu.
> 
> KVM wants it to be writable for the TPR optimization.  Historically,
I don't think so. TPR will work after BIOS will be shadowed. This is simply
KVM shortcoming.

> it was read-only in QEMU but it changed to read-write in order to
I just checked. It is still read-only in QEMU _before_ BIOS is shadowed.

> fake coreboot into thinking that the bios implemented PMM which it
> doesn't.
What is PMM? Post memory manager? How is it related?

--
			Gleb.
Anthony Liguori - Oct. 12, 2009, 2:31 p.m.
Gleb Natapov wrote:
> On Mon, Oct 12, 2009 at 08:44:56AM -0500, Anthony Liguori wrote:
>   
>> Gleb Natapov wrote
>>     
>>>> BTW, I don't think it's write-protected and it probably should be?
>>>>
>>>>         
>>> AFAIR it is not writable on plain qemu.
>>>       
>> KVM wants it to be writable for the TPR optimization.  Historically,
>>     
> I don't think so. TPR will work after BIOS will be shadowed. This is simply
> KVM shortcoming.
>
>   
>> it was read-only in QEMU but it changed to read-write in order to
>>     
> I just checked. It is still read-only in QEMU _before_ BIOS is shadowed.
>
>   
>> fake coreboot into thinking that the bios implemented PMM which it
>> doesn't.
>>     
> What is PMM? Post memory manager? How is it related?
>   

I was thinking of option rom memory.  BIOS memory is still read-only.

Regards,

Anthony Liguori
Gerd Hoffmann - Oct. 12, 2009, 2:50 p.m.
On 10/12/09 15:53, Gleb Natapov wrote:
> On Mon, Oct 12, 2009 at 03:45:47PM +0200, Gerd Hoffmann wrote:
>>>>>> 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.
>>
>> Sure.  Try 'info roms'.  Everything listed there will be reloaded on
>> reset (unless mem=rom, in which case the guest should not be able to
>> modify the bits in the first place).  That includes vgabios,
>> optionroms, -kernel images, ...
>>
> And what about bios? I don't see it there.

pc.c does some magic here to map the complete bios into high memory and 
the legacy part at 0xe0000, thats why it is loaded in a different way. 
It gets loaded to IO_MEM_ROM memory though, so the guest should not be 
able to corrupt it.

cheers,
   Gerd
Gleb Natapov - Oct. 12, 2009, 2:52 p.m.
On Mon, Oct 12, 2009 at 04:50:35PM +0200, Gerd Hoffmann wrote:
> On 10/12/09 15:53, Gleb Natapov wrote:
> >On Mon, Oct 12, 2009 at 03:45:47PM +0200, Gerd Hoffmann wrote:
> >>>>>>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.
> >>
> >>Sure.  Try 'info roms'.  Everything listed there will be reloaded on
> >>reset (unless mem=rom, in which case the guest should not be able to
> >>modify the bits in the first place).  That includes vgabios,
> >>optionroms, -kernel images, ...
> >>
> >And what about bios? I don't see it there.
> 
> pc.c does some magic here to map the complete bios into high memory
> and the legacy part at 0xe0000, thats why it is loaded in a
> different way. It gets loaded to IO_MEM_ROM memory though, so the
> guest should not be able to corrupt it.
> 
But KVM doesn't support it (memory is always writable). And we want to reload
BIOS on reboot for migration purposes.

--
			Gleb.
Kevin O'Connor - Oct. 12, 2009, 3 p.m.
On Mon, Oct 12, 2009 at 08:44:56AM -0500, Anthony Liguori wrote:
> Gleb Natapov wrote
>>> BTW, I don't think it's write-protected and it probably should be?
>>>
>>>     
>> AFAIR it is not writable on plain qemu.
>>   
>
> KVM wants it to be writable for the TPR optimization.  Historically, it  
> was read-only in QEMU but it changed to read-write in order to fake  
> coreboot into thinking that the bios implemented PMM which it doesn't.

During init, SeaBIOS will enable writes to all memory between
0xc0000-0x100000 - shadowing the ram as needed.  Just prior to
starting the boot phase, SeaBIOS will make the bios f-segment and all
option roms read only.  (Note, this differs from bochs-bios which
doesn't make optionroms writable - seabios makes them writable because
some optionroms need it.)

As an aside, it would be nice if qemu would just start off with this
memory as read/writable, as the shadoing process is ugly, it's qemu
specific, and it always ends up as read/writable anyway.

I'm not sure where coreboot or PMM comes into this.  SeaBIOS does
implement the Post Memory Manager calls.

-Kevin
Jamie Lokier - Oct. 12, 2009, 3:21 p.m.
Gleb Natapov wrote:
> But KVM doesn't support it (memory is always writable).

That seems like something which could be fixed.

-- Jamie
Gerd Hoffmann - Oct. 12, 2009, 3:43 p.m.
On 10/12/09 16:52, Gleb Natapov wrote:
> On Mon, Oct 12, 2009 at 04:50:35PM +0200, Gerd Hoffmann wrote:
>> pc.c does some magic here to map the complete bios into high memory
>> and the legacy part at 0xe0000, thats why it is loaded in a
>> different way. It gets loaded to IO_MEM_ROM memory though, so the
>> guest should not be able to corrupt it.
>>
> But KVM doesn't support it (memory is always writable). And we want to reload
> BIOS on reboot for migration purposes.

Then it should be switched over to the new rom loader.  Also the logic 
which does *not* reload stuff which goes to IO_MEM_ROM must be turned 
off for kvm them.

cheers,
   Gerd
Michael S. Tsirkin - Oct. 12, 2009, 4:57 p.m.
On Mon, Oct 12, 2009 at 05:43:39PM +0200, Gerd Hoffmann wrote:
> On 10/12/09 16:52, Gleb Natapov wrote:
>> On Mon, Oct 12, 2009 at 04:50:35PM +0200, Gerd Hoffmann wrote:
>>> pc.c does some magic here to map the complete bios into high memory
>>> and the legacy part at 0xe0000, thats why it is loaded in a
>>> different way. It gets loaded to IO_MEM_ROM memory though, so the
>>> guest should not be able to corrupt it.
>>>
>> But KVM doesn't support it (memory is always writable). And we want to reload
>> BIOS on reboot for migration purposes.
>
> Then it should be switched over to the new rom loader.  Also the logic  
> which does *not* reload stuff which goes to IO_MEM_ROM must be turned  
> off for kvm them.

I think it needs to be turned off anyway.  Since ROM is sent over for
migration, it might get out of sync with file even if it is read-only.

> cheers,
>   Gerd

Patch

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 1feb827..c54ac65 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -3248,7 +3248,6 @@  static int pci_cirrus_vga_initfn(PCIDevice *dev)
      /* setup PCI */
      pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CIRRUS);
      pci_config_set_device_id(pci_conf, device_id);
-     pci_conf[0x04] = PCI_COMMAND_IOACCESS | PCI_COMMAND_MEMACCESS;
      pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA);
      pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;