[Qemu-devel] [PATCH 1/3] pc-bios: enable io/memory unconditionally

[Qemu-devel] [PATCH 1/3] pc-bios: enable io/memory unconditionally

[Michael S. Tsirkin]

VGA adapters need to claim memory and i/o
transactions even if they do not have any
i/o or memory bars. E.g. PCI spec, page 297,
gives an example of such a device:

	Programming interface 0000 0000b
	VGA-compatible controller. Memory
	addresses 0A 0000h through 0B
	FFFFh. I/O addresses 3B0h to 3BBh
	and 3C0h to 3DFh and all aliases of
	these addresses.

While we could check for these devices and special-case them, it is
easier to fix this by enabling i/o and memory space unconditionally:
devices that do not support it will just ignore this setting.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 kvm/bios/rombios32.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/kvm/bios/rombios32.c b/kvm/bios/rombios32.c
index 0b3556c..747a97b 100755
--- a/kvm/bios/rombios32.c
+++ b/kvm/bios/rombios32.c
@@ -700,6 +700,7 @@ void smp_probe(void)
 #define PCI_COMMAND		0x04	/* 16 bits */
 #define  PCI_COMMAND_IO		0x1	/* Enable response in I/O space */
 #define  PCI_COMMAND_MEMORY	0x2	/* Enable response in Memory space */
+#define PCI_CLASS_PROG		0x09	/* Reg. Level Programming Interface */
 #define PCI_CLASS_DEVICE        0x0a    /* Device class */
 #define PCI_INTERRUPT_LINE	0x3c	/* 8 bits */
 #define PCI_INTERRUPT_PIN	0x3d	/* 8 bits */
@@ -767,7 +768,6 @@ static uint32_t pci_config_readb(PCIDevice *d, uint32_t addr)
 
 static void pci_set_io_region_addr(PCIDevice *d, int region_num, uint32_t addr)
 {
-    uint16_t cmd;
     uint32_t ofs, old_addr;
 
     if ( region_num == PCI_ROM_SLOT ) {
@@ -780,16 +780,6 @@ static void pci_set_io_region_addr(PCIDevice *d, int region_num, uint32_t addr)
 
     pci_config_writel(d, ofs, addr);
     BX_INFO("region %d: 0x%08x\n", region_num, addr);
-
-    /* enable memory mappings */
-    cmd = pci_config_readw(d, PCI_COMMAND);
-    if ( region_num == PCI_ROM_SLOT )
-        cmd |= 2;
-    else if (old_addr & PCI_ADDRESS_SPACE_IO)
-        cmd |= 1;
-    else
-        cmd |= 2;
-    pci_config_writew(d, PCI_COMMAND, cmd);
 }
 
 /* return the global irq number corresponding to a given device irq
@@ -946,6 +936,7 @@ static void pci_bios_init_device(PCIDevice *d)
 {
     int class;
     uint32_t *paddr;
+    uint16_t cmd;
     int i, pin, pic_irq, vendor_id, device_id;
 
     class = pci_config_readw(d, PCI_CLASS_DEVICE);
@@ -1024,6 +1015,11 @@ static void pci_bios_init_device(PCIDevice *d)
         break;
     }
 
+    /* enable memory mappings */
+    cmd = pci_config_readw(d, PCI_COMMAND);
+    cmd |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY;
+    pci_config_writew(d, PCI_COMMAND, cmd);
+
     /* map the interrupt */
     pin = pci_config_readb(d, PCI_INTERRUPT_PIN);
     if (pin != 0) {
-- 
1.6.5.rc2

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

[Michael S. Tsirkin]

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;
 
-- 
1.6.5.rc2

[Qemu-devel] [PATCH 3/3] qemu: cleanup unused macros in cirrus

[Michael S. Tsirkin]

Cirrus vga has a copy of many PCI macros,
and it doesn't even use them. Clean up.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/cirrus_vga.c |   37 +------------------------------------
 1 files changed, 1 insertions(+), 36 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index c54ac65..ff90e0b 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -172,41 +172,6 @@
 #define CIRRUS_MMIO_LINEDRAW_MODE     0x39	// byte
 #define CIRRUS_MMIO_BLTSTATUS         0x40	// byte
 
-// PCI 0x04: command(word), 0x06(word): status
-#define PCI_COMMAND_IOACCESS                0x0001
-#define PCI_COMMAND_MEMACCESS               0x0002
-#define PCI_COMMAND_BUSMASTER               0x0004
-#define PCI_COMMAND_SPECIALCYCLE            0x0008
-#define PCI_COMMAND_MEMWRITEINVALID         0x0010
-#define PCI_COMMAND_PALETTESNOOPING         0x0020
-#define PCI_COMMAND_PARITYDETECTION         0x0040
-#define PCI_COMMAND_ADDRESSDATASTEPPING     0x0080
-#define PCI_COMMAND_SERR                    0x0100
-#define PCI_COMMAND_BACKTOBACKTRANS         0x0200
-// PCI 0x08, 0xff000000 (0x09-0x0b:class,0x08:rev)
-#define PCI_CLASS_BASE_DISPLAY        0x03
-// PCI 0x08, 0x00ff0000
-#define PCI_CLASS_SUB_VGA             0x00
-// PCI 0x0c, 0x00ff0000 (0x0c:cacheline,0x0d:latency,0x0e:headertype,0x0f:Built-in self test)
-// 0x10-0x3f (headertype 00h)
-// PCI 0x10,0x14,0x18,0x1c,0x20,0x24: base address mapping registers
-//   0x10: MEMBASE, 0x14: IOBASE(hard-coded in XFree86 3.x)
-#define PCI_MAP_MEM                 0x0
-#define PCI_MAP_IO                  0x1
-#define PCI_MAP_MEM_ADDR_MASK       (~0xf)
-#define PCI_MAP_IO_ADDR_MASK        (~0x3)
-#define PCI_MAP_MEMFLAGS_32BIT      0x0
-#define PCI_MAP_MEMFLAGS_32BIT_1M   0x1
-#define PCI_MAP_MEMFLAGS_64BIT      0x4
-#define PCI_MAP_MEMFLAGS_CACHEABLE  0x8
-// PCI 0x28: cardbus CIS pointer
-// PCI 0x2c: subsystem vendor id, 0x2e: subsystem id
-// PCI 0x30: expansion ROM base address
-#define PCI_ROMBIOS_ENABLED         0x1
-// PCI 0x34: 0xffffff00=reserved, 0x000000ff=capabilities pointer
-// PCI 0x38: reserved
-// PCI 0x3c: 0x3c=int-line, 0x3d=int-pin, 0x3e=min-gnt, 0x3f=maax-lat
-
 #define CIRRUS_PNPMMIO_SIZE         0x1000
 
 #define ABS(a) ((signed)(a) > 0 ? a : -a)
@@ -3248,8 +3213,8 @@ 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;
 
      /* setup memory space */
      /* memory #0 LFB */
-- 
1.6.5.rc2

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

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

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

[Michael S. Tsirkin]

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

[Qemu-devel] Re: [PATCH 3/3] qemu: cleanup unused macros in cirrus

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 05:24:35PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Cirrus vga has a copy of many PCI macros,
> > and it doesn't even use them. Clean up.
> > @@ -3248,8 +3213,8 @@ 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;
> 
> If it is wrong as you claim in the other patch, just remove it, or at
> least put a comment.

Of course. sorry.

> >       pci_config_set_class(pci_conf, PCI_CLASS_DISPLAY_VGA);
> > -     pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL;
> 
> Did yo;u wanted to remove this one also?

Yes, this is the default anyway.

> >  
> >       /* setup memory space */
> >       /* memory #0 LFB */
> 
> Nice cleanup for the rest :)
> 
> Later, Juan.

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Jamie Lokier]

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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

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?


-- 
MST

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

[Gleb Natapov]

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.

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

[Michael S. Tsirkin]

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.

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

[Jamie Lokier]

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

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

[Gleb Natapov]

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.

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

[Gleb Natapov]

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.

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

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

[Gleb Natapov]

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.

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

[Michael S. Tsirkin]

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?

-- 
MST

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

[Gleb Natapov]

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.

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

[Michael S. Tsirkin]

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.

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

[Gleb Natapov]

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.

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

[Michael S. Tsirkin]

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.

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

[Gleb Natapov]

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.

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

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.

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

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

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

[Gleb Natapov]

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.

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

[Michael S. Tsirkin]

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

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Michael S. Tsirkin]

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

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

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

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

[Michael S. Tsirkin]

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

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

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

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

[Michael S. Tsirkin]

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

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

[Avi Kivity]

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Avi Kivity]

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Michael S. Tsirkin]

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.

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

[Avi Kivity]

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.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

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

[Anthony Liguori]

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.

-- 
Regards,

Anthony Liguori

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

[Gerd Hoffmann]

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

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

[Gleb Natapov]

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.

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

[Gleb Natapov]

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.

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

[Anthony Liguori]

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

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

[Gerd Hoffmann]

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

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

[Gleb Natapov]

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.

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

[Kevin O'Connor]

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

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

[Jamie Lokier]

Gleb Natapov wrote:
> But KVM doesn't support it (memory is always writable).

That seems like something which could be fixed.

-- Jamie

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

[Gerd Hoffmann]

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

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

[Michael S. Tsirkin]

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