Re: [Qemu-devel] [RFC] Writeable files in fw_cfg

[Blue Swirl]

On Sun, Jan 27, 2013 at 4:38 PM, David Woodhouse <address@hidden> wrote:
> On Sun, 2013-01-27 at 16:02 +0000, Blue Swirl wrote:
>> On Sun, Jan 27, 2013 at 3:50 PM, David Woodhouse <address@hidden> wrote:
>> > On Sun, 2013-01-27 at 15:14 +0000, Blue Swirl wrote:
>> >> It looks like this duplicates rom_add_file() and fw_cfg_add_file(), so
>> >> I don't see the point.
>> >
>> > Both of those are read-only, surely? The firmware inside the guest can't
>> > use them for non-volatile storage.
>>
>> Right. But why do we need PV non-volatile storage, instead of normal flash?
>
> People have looked at adding normal flash to the PC target, but it has a
> number of problems. Firstly, it doesn't even *work* with KVM enabled.
> But even if we were to somehow fix that, the simple approach of
> emulating a BIOS flash is also very suboptimal; it puts the whole of the
> firmware *into* the flash and lets it store its non-volatile variables
> in the same emulated flash chip. So if you want to update the firmware
> (which would normally be just a case of updating the bios.bin file), you
> end up having to do each guest system individually *and* the update is
> painful because you have to preserve the non-volatile storage while
> updating the other parts.

That problem could be easily solved by allowing a combination of two
images with different RO/RW settings, for example -bios
bios.bin[,offset=0,ro] -bios flash.img, offset=0x8000,rw.

> It's much better to have a separate storage for the non-volatile
> variables. Yes, we *could* teach it to use a dedicated flash chip for
> that rather than using flash for the firmware *and* storage as we would
> on real hardware. But if we're going to do something different for the
> virtualised case, a writeable fw_cfg file seems a whole lot easier and
> saner.
>
>> > It doesn't duplicate fw_cfg_add_file(); it extends it to allow a
>> > writeable mode too. fw_cfg_add_file() becomes a simple wrapper around
>> > fw_cfg_add_file_writeable(), which actually contains most of the
>> > contents of the original function.
>>
>> OK, however most of the loading logic is now in loader.c and that
>> should not be duplicated either.
>
> I actually got file_get_size() from pc.c, and had marked it with a
> horrid C99 comment so we couldn't miss the duplication. I'll sort out
> that kind of detail later, if we can reach consensus on the basic
> approach.

So the duplication already exists. :-(

>
> --
> dwmw2
>