[Qemu-devel] [PATCH 0/2] Implement migration support for pflash_cfi01

[Qemu-devel] [PATCH 0/2] Implement migration support for pflash_cfi01

[Peter Maydell]

These patches implement migration support for pflash_cfi01.
The first patch just drops some useless state so we don't
have to think about it for migration.

NB that pflash_cfi01 is used in the x86 pc model. I think this
means that migration while the BIOS is accessing the flash
wouldn't have worked properly. Since migration from a device
with no vmstate to one with vmstate works OK this shouldn't
break cross-version migration. However x86 maintainers may
wish to review and confirm this for themselves...

Peter Maydell (2):
  pflash_cfi01: Drop unused 'bypass' field
  pflash_cfi01: Implement migration support

 hw/pflash_cfi01.c |   30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] pflash_cfi01: Drop unused 'bypass' field

[Peter Maydell]

For pflash_cfi01 the 'bypass' field is set to zero and never changes,
so remove it (it is a leftover from pflash_cfi02, where bypass is
implemented).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pflash_cfi01.c |   10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 5d57bab..20d10b3 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -68,7 +68,6 @@ struct pflash_t {
     uint8_t width;
     uint8_t be;
     int wcycle; /* if 0, the flash is read normally */
-    int bypass;
     int ro;
     uint8_t cmd;
     uint8_t status;
@@ -93,12 +92,8 @@ static void pflash_timer (void *opaque)
     DPRINTF("%s: command %02x done\n", __func__, pfl->cmd);
     /* Reset flash */
     pfl->status ^= 0x80;
-    if (pfl->bypass) {
-        pfl->wcycle = 2;
-    } else {
-        memory_region_rom_device_set_readable(&pfl->mem, true);
-        pfl->wcycle = 0;
-    }
+    memory_region_rom_device_set_readable(&pfl->mem, true);
+    pfl->wcycle = 0;
     pfl->cmd = 0;
 }
 
@@ -452,7 +447,6 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
  reset_flash:
     memory_region_rom_device_set_readable(&pfl->mem, true);
 
-    pfl->bypass = 0;
     pfl->wcycle = 0;
     pfl->cmd = 0;
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] pflash_cfi01: Implement migration support

[Peter Maydell]

Add a vmstate to pflash_cfi01, so that it can be live migrated.

XXX this device is in pc, so does this break cross version
migration???

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/pflash_cfi01.c |   20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/pflash_cfi01.c b/hw/pflash_cfi01.c
index 20d10b3..646dc79 100644
--- a/hw/pflash_cfi01.c
+++ b/hw/pflash_cfi01.c
@@ -67,7 +67,7 @@ struct pflash_t {
     uint64_t sector_len;
     uint8_t width;
     uint8_t be;
-    int wcycle; /* if 0, the flash is read normally */
+    uint8_t wcycle; /* if 0, the flash is read normally */
     int ro;
     uint8_t cmd;
     uint8_t status;
@@ -77,7 +77,7 @@ struct pflash_t {
     uint16_t ident3;
     uint8_t cfi_len;
     uint8_t cfi_table[0x52];
-    hwaddr counter;
+    uint64_t counter;
     unsigned int writeblock_size;
     QEMUTimer *timer;
     MemoryRegion mem;
@@ -85,6 +85,19 @@ struct pflash_t {
     void *storage;
 };
 
+static const VMStateDescription vmstate_pflash = {
+    .name = "pflash_cfi01",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(wcycle, pflash_t),
+        VMSTATE_UINT8(cmd, pflash_t),
+        VMSTATE_UINT8(status, pflash_t),
+        VMSTATE_UINT64(counter, pflash_t),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void pflash_timer (void *opaque)
 {
     pflash_t *pfl = opaque;
@@ -223,7 +236,7 @@ static inline void pflash_data_write(pflash_t *pfl, hwaddr offset,
     uint8_t *p = pfl->storage;
 
     DPRINTF("%s: block write offset " TARGET_FMT_plx
-            " value %x counter " TARGET_FMT_plx "\n",
+            " value %x counter %016" PRIx64 "\n",
             __func__, offset, value, pfl->counter);
     switch (width) {
     case 1:
@@ -701,6 +714,7 @@ static void pflash_cfi01_class_init(ObjectClass *klass, void *data)
 
     k->init = pflash_cfi01_init;
     dc->props = pflash_cfi01_properties;
+    dc->vmsd = &vmstate_pflash;
 }
 
 
-- 
1.7.9.5

[Qemu-devel] Use of flash for x86 BIOS (was: [PATCH 0/2] Implement migration support for pflash_cfi01)

[Markus Armbruster]

[Note cc: Jordan, who added flash to x86 in commit bd183c79]

Peter Maydell <peter.maydell@linaro.org> writes:

> These patches implement migration support for pflash_cfi01.
> The first patch just drops some useless state so we don't
> have to think about it for migration.
>
> NB that pflash_cfi01 is used in the x86 pc model. I think this
> means that migration while the BIOS is accessing the flash
> wouldn't have worked properly. Since migration from a device
> with no vmstate to one with vmstate works OK this shouldn't
> break cross-version migration. However x86 maintainers may
> wish to review and confirm this for themselves...

x86 maintainers may wish to *switch it off* until it's done fully and
properly, by setting "pc-sysfw" property "rom_only" to 1.

x86 flash use was added in series "[PATCH v10 0/8] PC system flash
support", commit 4732dcaf..1b89fafe, v1.1.  Core code is
pc_system_firmware_init().  Works as follows, as far as I can tell:

* Create a "pc-sysfw" device with default properties.

  This device does nothing in particular, and is *not* guest-visible.
  It appears to be used just as a property container.

* If (default) property "rom_only" is non-zero, map the BIOS ROM the
  traditional way.  Get its contents from the file named by -bios,
  default "bios.bin".

  This is the case for isapc as well as pc-1.0 and older.

* Else, if KVM is enabled, putting BIOS in flash doesn't actually work
  (KVM restriction), so force "rom_only" to one, and map the BIOS ROM
  the traditional way, as above.

  Fatal error if the user defined drive if=pflash,bus=0,unit=0, because
  that's valid only when we're using flash for the BIOS ROM.

  I understand this case is meant to go away when the KVM restriction
  gets lifted.

* Else, map the BIOS ROM as flash device.

  Note: for now, this case applies only when KVM is disabled.

  - If the user did not define said pflash drive, create one with BIOS
    ROM contents (get it the same way as above).  Failure is fatal.

  - Create a "cfi.pflash01" block device backed by the pflash drive.

What does this mean?

1. The "pc-sysfw" device is mostly harmless.

2. Enabling/disabling KVM is guest-visible!  With KVM disabled, you get
   a flash memory device.  With KVM enabled, you get a ROM.  Not good;
   KVM should be as transparent as possible to the guest.

   I raised this issue last August, Jordan told me he's working on
   it[*], and I let the matter rest then.  That was a mistake.

[*] http://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg03178.html

Re: [Qemu-devel] Use of flash for x86 BIOS (was: [PATCH 0/2] Implement migration support for pflash_cfi01)

[Peter Maydell]

On 21 March 2013 07:45, Markus Armbruster <armbru@redhat.com> wrote:
> [Note cc: Jordan, who added flash to x86 in commit bd183c79]
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> These patches implement migration support for pflash_cfi01.
>> The first patch just drops some useless state so we don't
>> have to think about it for migration.
>>
>> NB that pflash_cfi01 is used in the x86 pc model. I think this
>> means that migration while the BIOS is accessing the flash
>> wouldn't have worked properly. Since migration from a device
>> with no vmstate to one with vmstate works OK this shouldn't
>> break cross-version migration. However x86 maintainers may
>> wish to review and confirm this for themselves...
>
> x86 maintainers may wish to *switch it off* until it's done fully and
> properly, by setting "pc-sysfw" property "rom_only" to 1.

So does that mean that these patches can't be applied until
the rom_only property is set, or is that a fix that can be made
independently?

thanks
-- PMM

Re: [Qemu-devel] Use of flash for x86 BIOS

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 21 March 2013 07:45, Markus Armbruster <armbru@redhat.com> wrote:
>> [Note cc: Jordan, who added flash to x86 in commit bd183c79]
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> These patches implement migration support for pflash_cfi01.
>>> The first patch just drops some useless state so we don't
>>> have to think about it for migration.
>>>
>>> NB that pflash_cfi01 is used in the x86 pc model. I think this
>>> means that migration while the BIOS is accessing the flash
>>> wouldn't have worked properly. Since migration from a device
>>> with no vmstate to one with vmstate works OK this shouldn't
>>> break cross-version migration. However x86 maintainers may
>>> wish to review and confirm this for themselves...
>>
>> x86 maintainers may wish to *switch it off* until it's done fully and
>> properly, by setting "pc-sysfw" property "rom_only" to 1.
>
> So does that mean that these patches can't be applied until
> the rom_only property is set, or is that a fix that can be made
> independently?

If your patches work, then applying them can't make things worse for x86
than they already are, can it?  Thus, independent, I guess.

Re: [Qemu-devel] Use of flash for x86 BIOS (was: [PATCH 0/2] Implement migration support for pflash_cfi01)

[Jordan Justen]

On Thu, Mar 21, 2013 at 12:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
> x86 maintainers may wish to *switch it off* until it's done fully and
> properly, by setting "pc-sysfw" property "rom_only" to 1.

This would completely disable the flash support. At the time this
feature was added, I think it was well understood that kvm would not
support the flash mode, while plain qemu could. If it was not a
show-stopper to integrating the feature originally, what has changed?

rom_only was added as part of the flash support enabling. I guess in a
way it would be amusing to use it to disabled that same feature.

> 1. The "pc-sysfw" device is mostly harmless.

Indeed, and it's only marginally of interest until kvm supports it.

Admittedly, I've been completely ineffectual in resolving the kvm
portion. More recently I tried to make use of KVM_MEM_READONLY to
address this. I was able to get an VM exit on writes to flash, but not
able to get the memory region to convert to full device mode so VM
exits would occur on reads as well. I am once again stalled...

-Jordan

Re: [Qemu-devel] Use of flash for x86 BIOS

[Markus Armbruster]

Jordan Justen <jljusten@gmail.com> writes:

> On Thu, Mar 21, 2013 at 12:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> x86 maintainers may wish to *switch it off* until it's done fully and
>> properly, by setting "pc-sysfw" property "rom_only" to 1.
>
> This would completely disable the flash support.

Unless the user enables it explicitly with something like "-global
pc-sysfw.rom_only=0".

>                                                  At the time this
> feature was added, I think it was well understood that kvm would not
> support the flash mode, while plain qemu could. If it was not a
> show-stopper to integrating the feature originally, what has changed?

Nothing changed, and that's the problem.

Merging the feature was okay, I think.  Defaulting it to "on" with TCG
and "off" with KVM was a mistake, because that made enabling/disabling
KVM guest-visible (see item 2. below).  The default needs to be the same
both with and without KVM.  Since the thing still doesn't work with KVM,
the default needs to be "off".

A possible explanation for making this mistake is that people assumed it
would soon work with KVM.  That turned out not to be the case.

> rom_only was added as part of the flash support enabling. I guess in a
> way it would be amusing to use it to disabled that same feature.
>
>> 1. The "pc-sysfw" device is mostly harmless.
>
> Indeed, and it's only marginally of interest until kvm supports it.
>
> Admittedly, I've been completely ineffectual in resolving the kvm
> portion. More recently I tried to make use of KVM_MEM_READONLY to
> address this. I was able to get an VM exit on writes to flash, but not
> able to get the memory region to convert to full device mode so VM
> exits would occur on reads as well. I am once again stalled...

Have you discussed your difficulties on kvm@vger.kernel.org?

>> 2. Enabling/disabling KVM is guest-visible!  With KVM disabled, you get
>>    a flash memory device.  With KVM enabled, you get a ROM.  Not good;
>>    KVM should be as transparent as possible to the guest.
>> 
>>    I raised this issue last August, Jordan told me he's working on
>>    it[*], and I let the matter rest then.  That was a mistake.
>> 
>> [*] http://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg03178.html

Re: [Qemu-devel] [PATCH 2/2] pflash_cfi01: Implement migration support

[Peter Maydell]

On 19 March 2013 18:24, Peter Maydell <peter.maydell@linaro.org> wrote:
> Add a vmstate to pflash_cfi01, so that it can be live migrated.
>
> XXX this device is in pc, so does this break cross version
> migration???

Oops. I checked that this was going to work ok, but forgot to
remove the XXX markers. I'm planning to put these patches
into the arm-devs queue, so I'll just remove this line
from the commit message at that point.

thanks
-- PMM

Re: [Qemu-devel] Use of flash for x86 BIOS

[Laszlo Ersek]

On 03/22/13 20:48, Markus Armbruster wrote:
> Jordan Justen <jljusten@gmail.com> writes:

>>> 1. The "pc-sysfw" device is mostly harmless.
>>
>> Indeed, and it's only marginally of interest until kvm supports it.
>>
>> Admittedly, I've been completely ineffectual in resolving the kvm
>> portion. More recently I tried to make use of KVM_MEM_READONLY to
>> address this. I was able to get an VM exit on writes to flash, but not
>> able to get the memory region to convert to full device mode so VM
>> exits would occur on reads as well. I am once again stalled...
> 
> Have you discussed your difficulties on kvm@vger.kernel.org?

The most recent discussion that I'm aware of was:

http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg05150.html
http://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg05236.html

(both are sub-threads under the same patch submission).

Laszlo

Re: [Qemu-devel] Use of flash for x86 BIOS

[Jordan Justen]

On Fri, Mar 22, 2013 at 12:48 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Jordan Justen <jljusten@gmail.com> writes:
>
>> On Thu, Mar 21, 2013 at 12:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> x86 maintainers may wish to *switch it off* until it's done fully and
>>> properly, by setting "pc-sysfw" property "rom_only" to 1.
>>
>> This would completely disable the flash support.
>
> Unless the user enables it explicitly with something like "-global
> pc-sysfw.rom_only=0".
>
>>                                                  At the time this
>> feature was added, I think it was well understood that kvm would not
>> support the flash mode, while plain qemu could. If it was not a
>> show-stopper to integrating the feature originally, what has changed?
>
> Nothing changed, and that's the problem.
>
> Merging the feature was okay, I think.  Defaulting it to "on" with TCG
> and "off" with KVM was a mistake, because that made enabling/disabling
> KVM guest-visible (see item 2. below).  The default needs to be the same
> both with and without KVM.  Since the thing still doesn't work with KVM,
> the default needs to be "off".
>
> A possible explanation for making this mistake is that people assumed it
> would soon work with KVM.  That turned out not to be the case.

Given this can be overridden on the command line it does seem
reasonable to disable it by default until KVM support is in place.

Slight off topic, does pc_q35 intend to support SPI flash at some
point? SPI programming is at a different MMIO address from the flash,
so it should be possible to support with qemu/kvm today.

Are there any plans for Q35 to become the default qemu PC machine?

-Jordan

Re: [Qemu-devel] Use of flash for x86 BIOS

[Xiao Guangrong]

On 03/23/2013 03:09 AM, Jordan Justen wrote:

> Admittedly, I've been completely ineffectual in resolving the kvm
> portion. More recently I tried to make use of KVM_MEM_READONLY to
> address this. I was able to get an VM exit on writes to flash, but not
> able to get the memory region to convert to full device mode so VM
> exits would occur on reads as well. I am once again stalled...

Hi Jordan,

What's memory region you want to get? I should admit that I do not
have enough background of flash, could you please explain this
requirement more detail?

Thanks!

Re: [Qemu-devel] Use of flash for x86 BIOS

[Jordan Justen]

On Sun, Apr 7, 2013 at 11:06 PM, Xiao Guangrong
<xiaoguangrong@linux.vnet.ibm.com> wrote:
> On 03/23/2013 03:09 AM, Jordan Justen wrote:
>
>> Admittedly, I've been completely ineffectual in resolving the kvm
>> portion. More recently I tried to make use of KVM_MEM_READONLY to
>> address this. I was able to get an VM exit on writes to flash, but not
>> able to get the memory region to convert to full device mode so VM
>> exits would occur on reads as well. I am once again stalled...
>
> Hi Jordan,
>
> What's memory region you want to get? I should admit that I do not
> have enough background of flash, could you please explain this
> requirement more detail?

The flash memory requires two modes.

Read/Execute mode:
* Initial state
* Writes will trap to QEMU, and may transition to Device mode based on
QEMU device emulation

Device mode:
* Region is not executable
* All reads & writes will trap to QEMU
* May transition back to Read/Execute mode based on QEMU device emulation

Using KVM_MEM_READONLY I was able to get Read/Execute mode to work,
but I was not able to get the memory region to transition to Device
mode. (I couldn't get reads to trap to QEMU.)

-Jordan

Re: [Qemu-devel] Use of flash for x86 BIOS

[Gleb Natapov]

On Mon, Apr 08, 2013 at 01:18:10AM -0700, Jordan Justen wrote:
> On Sun, Apr 7, 2013 at 11:06 PM, Xiao Guangrong
> <xiaoguangrong@linux.vnet.ibm.com> wrote:
> > On 03/23/2013 03:09 AM, Jordan Justen wrote:
> >
> >> Admittedly, I've been completely ineffectual in resolving the kvm
> >> portion. More recently I tried to make use of KVM_MEM_READONLY to
> >> address this. I was able to get an VM exit on writes to flash, but not
> >> able to get the memory region to convert to full device mode so VM
> >> exits would occur on reads as well. I am once again stalled...
> >
> > Hi Jordan,
> >
> > What's memory region you want to get? I should admit that I do not
> > have enough background of flash, could you please explain this
> > requirement more detail?
> 
> The flash memory requires two modes.
> 
> Read/Execute mode:
> * Initial state
> * Writes will trap to QEMU, and may transition to Device mode based on
> QEMU device emulation
> 
> Device mode:
> * Region is not executable
> * All reads & writes will trap to QEMU
> * May transition back to Read/Execute mode based on QEMU device emulation
> 
> Using KVM_MEM_READONLY I was able to get Read/Execute mode to work,
> but I was not able to get the memory region to transition to Device
> mode. (I couldn't get reads to trap to QEMU.)
> 
You need to drop memslot when you transition to Device mode.

--
			Gleb.

Re: [Qemu-devel] Use of flash for x86 BIOS

[Xiao Guangrong]

On 04/08/2013 04:43 PM, Gleb Natapov wrote:
> On Mon, Apr 08, 2013 at 01:18:10AM -0700, Jordan Justen wrote:
>> On Sun, Apr 7, 2013 at 11:06 PM, Xiao Guangrong
>> <xiaoguangrong@linux.vnet.ibm.com> wrote:
>>> On 03/23/2013 03:09 AM, Jordan Justen wrote:
>>>
>>>> Admittedly, I've been completely ineffectual in resolving the kvm
>>>> portion. More recently I tried to make use of KVM_MEM_READONLY to
>>>> address this. I was able to get an VM exit on writes to flash, but not
>>>> able to get the memory region to convert to full device mode so VM
>>>> exits would occur on reads as well. I am once again stalled...
>>>
>>> Hi Jordan,
>>>
>>> What's memory region you want to get? I should admit that I do not
>>> have enough background of flash, could you please explain this
>>> requirement more detail?
>>
>> The flash memory requires two modes.
>>
>> Read/Execute mode:
>> * Initial state
>> * Writes will trap to QEMU, and may transition to Device mode based on
>> QEMU device emulation
>>
>> Device mode:
>> * Region is not executable
>> * All reads & writes will trap to QEMU
>> * May transition back to Read/Execute mode based on QEMU device emulation
>>
>> Using KVM_MEM_READONLY I was able to get Read/Execute mode to work,
>> but I was not able to get the memory region to transition to Device
>> mode. (I couldn't get reads to trap to QEMU.)
>>
> You need to drop memslot when you transition to Device mode.

Yes. After delete the memslot, you can intercept both write and read. ;)

Re: [Qemu-devel] Use of flash for x86 BIOS

[Markus Armbruster]

Jordan Justen <jljusten@gmail.com> writes:

> On Fri, Mar 22, 2013 at 12:48 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Jordan Justen <jljusten@gmail.com> writes:
>>
>>> On Thu, Mar 21, 2013 at 12:45 AM, Markus Armbruster
>>> <armbru@redhat.com> wrote:
>>>> x86 maintainers may wish to *switch it off* until it's done fully and
>>>> properly, by setting "pc-sysfw" property "rom_only" to 1.
>>>
>>> This would completely disable the flash support.
>>
>> Unless the user enables it explicitly with something like "-global
>> pc-sysfw.rom_only=0".
>>
>>>                                                  At the time this
>>> feature was added, I think it was well understood that kvm would not
>>> support the flash mode, while plain qemu could. If it was not a
>>> show-stopper to integrating the feature originally, what has changed?
>>
>> Nothing changed, and that's the problem.
>>
>> Merging the feature was okay, I think.  Defaulting it to "on" with TCG
>> and "off" with KVM was a mistake, because that made enabling/disabling
>> KVM guest-visible (see item 2. below).  The default needs to be the same
>> both with and without KVM.  Since the thing still doesn't work with KVM,
>> the default needs to be "off".
>>
>> A possible explanation for making this mistake is that people assumed it
>> would soon work with KVM.  That turned out not to be the case.
>
> Given this can be overridden on the command line it does seem
> reasonable to disable it by default until KVM support is in place.

Sent, you're cc'ed.

> Slight off topic, does pc_q35 intend to support SPI flash at some
> point? SPI programming is at a different MMIO address from the flash,
> so it should be possible to support with qemu/kvm today.

No idea :)

> Are there any plans for Q35 to become the default qemu PC machine?

Unless I'm mistaken, that's the plan.