Patchwork CMOS file support

login
register
mail settings
Submitter Mathias Krause
Date Sept. 16, 2010, 1:58 p.m.
Message ID <1284645517-32743-1-git-send-email-mathias.krause@secunet.com>
Download mbox | patch
Permalink /patch/64976/
State New
Headers show

Comments

Mathias Krause - Sept. 16, 2010, 1:58 p.m.
In contrast to the BIOS and Option ROMs the CMOS content cannot be
predefined by the user. Also the amount of useable CMOS ARM is pretty
limited, even though the amount of CMOS bytes emulated by qemu is
already twice as much as of the original MC146818. Nevertheless those
limitations are pretty annoying when testing different BIOS replacement
implementations like coreboot in combination with FILO that use CMOS
values above the basic RTC range for its own purpose to, e.g., control
the boot device order using a string containing the boot devices to
probe.

This patch adds support to specify a file to read the initial CMOS
content from. It also increases the CMOS size to 256 bytes and
implements access to the extended memory range via I/O ports 0x72 and
0x73.

Use it like: `qemu -global mc146818rtc.file=cmos.bin ...' where cmos.bin
is a binary file, sized 256 bytes containing the CMOS RAM.

Signed-off-by: Mathias Krause <mathias.krause@secunet.com>
---
 hw/mc146818rtc.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 56 insertions(+), 6 deletions(-)
Stefan Weil - Sept. 16, 2010, 4:49 p.m.
Am 16.09.2010 15:58, schrieb Mathias Krause:
> In contrast to the BIOS and Option ROMs the CMOS content cannot be
> predefined by the user. Also the amount of useable CMOS ARM is pretty
> limited, even though the amount of CMOS bytes emulated by qemu is
> already twice as much as of the original MC146818. Nevertheless those
> limitations are pretty annoying when testing different BIOS replacement
> implementations like coreboot in combination with FILO that use CMOS
> values above the basic RTC range for its own purpose to, e.g., control
> the boot device order using a string containing the boot devices to
> probe.
>
> This patch adds support to specify a file to read the initial CMOS
> content from. It also increases the CMOS size to 256 bytes and
> implements access to the extended memory range via I/O ports 0x72 and
> 0x73.
>
> Use it like: `qemu -global mc146818rtc.file=cmos.bin ...' where cmos.bin
> is a binary file, sized 256 bytes containing the CMOS RAM.
>
> Signed-off-by: Mathias Krause<mathias.krause@secunet.com>
> ---
>   hw/mc146818rtc.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>   1 files changed, 56 insertions(+), 6 deletions(-)
>
>    

[snip]

The intention of this patch is ok. Loading CMOS with initial data
is needed. I just want to add two questions / remarks how the
implementation might be improved.

Are there use cases where having a smaller CMOS size is better?
For example, when I want to emulate a system with 128 byte CMOS?
The size of CMOS could be derived from the size of the data
or specified by an additional parameter.

Using QEMU's block devices instead of a simple file would be
more consistent with the rest of QEMU and allow reading the
CMOS data not only from a file but also from an URL or other
sources.

Regards
Stefan
Anthony Liguori - Sept. 16, 2010, 5:20 p.m.
On 09/16/2010 08:58 AM, Mathias Krause wrote:
> In contrast to the BIOS and Option ROMs the CMOS content cannot be
> predefined by the user. Also the amount of useable CMOS ARM is pretty
> limited, even though the amount of CMOS bytes emulated by qemu is
> already twice as much as of the original MC146818. Nevertheless those
> limitations are pretty annoying when testing different BIOS replacement
> implementations like coreboot in combination with FILO that use CMOS
> values above the basic RTC range for its own purpose to, e.g., control
> the boot device order using a string containing the boot devices to
> probe.
>
> This patch adds support to specify a file to read the initial CMOS
> content from. It also increases the CMOS size to 256 bytes and
> implements access to the extended memory range via I/O ports 0x72 and
> 0x73.
>
> Use it like: `qemu -global mc146818rtc.file=cmos.bin ...' where cmos.bin
> is a binary file, sized 256 bytes containing the CMOS RAM.
>
> Signed-off-by: Mathias Krause<mathias.krause@secunet.com>
>    

Instead of using FILE, I'd suggest using a BlockDriver to read and write 
the data.

I think it would be very nice to add write support too so that writes to 
CMOS were persisted across boots.

> ---
>   hw/mc146818rtc.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>   1 files changed, 56 insertions(+), 6 deletions(-)
>
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index 2b91fa8..9f215e0 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -78,12 +78,16 @@
>   #define REG_C_PF   0x40
>   #define REG_C_AF   0x20
>
> +#define BASE_PORT  0x70
> +#define CMOS_SIZE  256
> +
>   typedef struct RTCState {
>       ISADevice dev;
> -    uint8_t cmos_data[128];
> +    uint8_t cmos_data[CMOS_SIZE];
>       uint8_t cmos_index;
>       struct tm current_tm;
>       int32_t base_year;
> +    char *file;
>       qemu_irq irq;
>       qemu_irq sqw_irq;
>       int it_shift;
> @@ -206,7 +210,7 @@ static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data)
>       RTCState *s = opaque;
>
>       if ((addr&  1) == 0) {
> -        s->cmos_index = data&  0x7f;
> +        s->cmos_index = data&  (addr == BASE_PORT ? 0x7f : 0xff);
>       } else {
>           CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02x\n",
>                        s->cmos_index, data);
> @@ -581,7 +585,6 @@ static void rtc_reset(void *opaque)
>   static int rtc_initfn(ISADevice *dev)
>   {
>       RTCState *s = DO_UPCAST(RTCState, dev, dev);
> -    int base = 0x70;
>
>       s->cmos_data[RTC_REG_A] = 0x26;
>       s->cmos_data[RTC_REG_B] = 0x02;
> @@ -603,14 +606,57 @@ static int rtc_initfn(ISADevice *dev)
>           qemu_get_clock(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
>       qemu_mod_timer(s->second_timer2, s->next_second_time);
>
> -    register_ioport_write(base, 2, 1, cmos_ioport_write, s);
> -    register_ioport_read(base, 2, 1, cmos_ioport_read, s);
> +    register_ioport_write(BASE_PORT, 4, 1, cmos_ioport_write, s);
> +    register_ioport_read(BASE_PORT, 4, 1, cmos_ioport_read, s);
>
> -    qdev_set_legacy_instance_id(&dev->qdev, base, 2);
> +    qdev_set_legacy_instance_id(&dev->qdev, BASE_PORT, 2);
>       qemu_register_reset(rtc_reset, s);
>       return 0;
>   }
>
> +static long get_file_size(FILE *f)
> +{
> +    long where, size;
> +
> +    /* XXX: on Unix systems, using fstat() probably makes more sense */
> +
> +    where = ftell(f);
> +    fseek(f, 0, SEEK_END);
> +    size = ftell(f);
> +    fseek(f, where, SEEK_SET);
> +
> +    return size;
> +}
>    

BlockDrivers have a getlength() functions.

Regards,

Anthony Liguori
Mathias Krause - Sept. 17, 2010, 6:42 a.m.
Am 16.09.2010 18:49, Stefan Weil schrieb:
> The intention of this patch is ok. Loading CMOS with initial data
> is needed. I just want to add two questions / remarks how the
> implementation might be improved.
> 
> Are there use cases where having a smaller CMOS size is better?
> For example, when I want to emulate a system with 128 byte CMOS?
> The size of CMOS could be derived from the size of the data
> or specified by an additional parameter.

I cannot image cases where smaller sizes would be a benefit. The saved
disk space is negligible and you can always pad your CMOS configuration
uo to 256 bytes by filling the gap with zero bytes. If the system just
accesses the first 128 bytes the additional 128 zero bytes shouldn't hurt.

> Using QEMU's block devices instead of a simple file would be
> more consistent with the rest of QEMU and allow reading the
> CMOS data not only from a file but also from an URL or other
> sources.

Thanks for the hint. Since this is my first contribution to the project
I'm not that familiar with the code. Looking at other places, e.g. how
the -kernel option gets handled, I just see FILE everywhere. Can you
give me some pointers how to use this interface?

Thanks for the review!


Mathias
Mathias Krause - Sept. 17, 2010, 6:50 a.m.
Am 16.09.2010 19:20 schrieb Anthony Liguori:
> Instead of using FILE, I'd suggest using a BlockDriver to read and write
> the data.

I'll fix that as soon as I figured how to use this interface.

> I think it would be very nice to add write support too so that writes to
> CMOS were persisted across boots.

Indeed. Also I would like to have a command line interface like '-cmos
cmos.bin' instead of the ugly '-global mc146818rtc.file=cmos.bin'. But
I'm not aware how to create such an alias. Any pointers?

>> +static long get_file_size(FILE *f)
>> +{
>> +    long where, size;
>> +
>> +    /* XXX: on Unix systems, using fstat() probably makes more sense */
>> +
>> +    where = ftell(f);
>> +    fseek(f, 0, SEEK_END);
>> +    size = ftell(f);
>> +    fseek(f, where, SEEK_SET);
>> +
>> +    return size;
>> +}
>>    
> 
> BlockDrivers have a getlength() functions.

Would reduce the size of the patch which is always a good thing (tm).


Mathias
Kevin Wolf - Sept. 17, 2010, 10:44 a.m.
Hi Mathias,

Am 17.09.2010 08:42, schrieb Mathias Krause:
>> Using QEMU's block devices instead of a simple file would be
>> more consistent with the rest of QEMU and allow reading the
>> CMOS data not only from a file but also from an URL or other
>> sources.
> 
> Thanks for the hint. Since this is my first contribution to the project
> I'm not that familiar with the code. Looking at other places, e.g. how
> the -kernel option gets handled, I just see FILE everywhere. Can you
> give me some pointers how to use this interface?

Have a look at block.h which contains the prototypes for the public
block layer interface.

Basically, you need to create a BlockDriverState with bdrv_new() and
then open it with bdrv_open(). You'll want to specify the raw block
driver for opening the image, you get it with bdrv_find_format("raw").
bdrv_pread/pwrite are the right functions to access the file with byte
granularity (other functions work on 512 byte sectors). bdrv_delete
frees the the BlockDriverState when you're done.

HTH,
Kevin
Stefan Weil - Sept. 17, 2010, 10:58 a.m.
Am 17.09.2010 08:42, schrieb Mathias Krause:
> Am 16.09.2010 18:49, Stefan Weil schrieb:
>> The intention of this patch is ok. Loading CMOS with initial data
>> is needed. I just want to add two questions / remarks how the
>> implementation might be improved.
>>
>> Are there use cases where having a smaller CMOS size is better?
>> For example, when I want to emulate a system with 128 byte CMOS?
>> The size of CMOS could be derived from the size of the data
>> or specified by an additional parameter.
>
> I cannot image cases where smaller sizes would be a benefit. The saved
> disk space is negligible and you can always pad your CMOS configuration
> uo to 256 bytes by filling the gap with zero bytes. If the system just
> accesses the first 128 bytes the additional 128 zero bytes shouldn't hurt.
>

The benefit of smaller sizes is not saving RAM but precision
of the emulation.

A BIOS or an operating system might be designed to support
CMOS  with 128 bytes or 256 bytes and try guessing the size
by probing (many systems do something like this for RAM or
other memory types).

Testing the support of the small CMOS will be impossible
if the emulation always emulates 256 bytes. Filling the
additional 128 bytes with zero bytes won't help because
the system will access them successfully although writes
should fail.


>> Using QEMU's block devices instead of a simple file would be
>> more consistent with the rest of QEMU and allow reading the
>> CMOS data not only from a file but also from an URL or other
>> sources.
>
> Thanks for the hint. Since this is my first contribution to the project
> I'm not that familiar with the code. Looking at other places, e.g. how
> the -kernel option gets handled, I just see FILE everywhere. Can you
> give me some pointers how to use this interface?
>
> Thanks for the review!
>
>
> Mathias
>
Mathias Krause - Sept. 17, 2010, 11:16 a.m.
On 17.09.2010 12:58, Stefan Weil wrote:
> Am 17.09.2010 08:42, schrieb Mathias Krause:
>> Am 16.09.2010 18:49, Stefan Weil schrieb:
>>> Are there use cases where having a smaller CMOS size is better?
>>> For example, when I want to emulate a system with 128 byte CMOS?
>>> The size of CMOS could be derived from the size of the data
>>> or specified by an additional parameter.
>>
>> I cannot image cases where smaller sizes would be a benefit. The saved
>> disk space is negligible and you can always pad your CMOS configuration
>> uo to 256 bytes by filling the gap with zero bytes. If the system just
>> accesses the first 128 bytes the additional 128 zero bytes shouldn't
>> hurt.
>>
> 
> The benefit of smaller sizes is not saving RAM but precision
> of the emulation.

Well, as mentioned in my initial email the current emulation isn't
accurate as well. It is emulating 128 bytes of CMOS RAM instead of 64
bytes, the real MC146818 only has.

> A BIOS or an operating system might be designed to support
> CMOS  with 128 bytes or 256 bytes and try guessing the size
> by probing (many systems do something like this for RAM or
> other memory types).
> 
> Testing the support of the small CMOS will be impossible
> if the emulation always emulates 256 bytes. Filling the
> additional 128 bytes with zero bytes won't help because
> the system will access them successfully although writes
> should fail.

To access the upper 128 bytes of memory the operation system must be
aware that it needs to use different I/O ports to probe for those
addresses. The patch doesn't change the old behavior. Only the lower 128
bytes of memory will be accessible through the default RTC ports.
So if the operating system or BIOS in question already is aware of the
alternative method to access the upper 128 bytes, why should if fail then?
Mathias Krause - Sept. 17, 2010, 11:28 a.m.
Hi Kevin,

On 17.09.2010 12:44, Kevin Wolf wrote:
> Hi Mathias,
> 
> Am 17.09.2010 08:42, schrieb Mathias Krause:
>>> Using QEMU's block devices instead of a simple file would be
>>> more consistent with the rest of QEMU and allow reading the
>>> CMOS data not only from a file but also from an URL or other
>>> sources.
>> Thanks for the hint. Since this is my first contribution to the project
>> I'm not that familiar with the code. Looking at other places, e.g. how
>> the -kernel option gets handled, I just see FILE everywhere. Can you
>> give me some pointers how to use this interface?
> 
> Have a look at block.h which contains the prototypes for the public
> block layer interface.
> 
> Basically, you need to create a BlockDriverState with bdrv_new() and
> then open it with bdrv_open(). You'll want to specify the raw block
> driver for opening the image, you get it with bdrv_find_format("raw").
> bdrv_pread/pwrite are the right functions to access the file with byte
> granularity (other functions work on 512 byte sectors). bdrv_delete
> frees the the BlockDriverState when you're done.

Thank you for the detailed writeup. I think I should figure out how to
use it myself now. Albeit there seem to be not many users of this
interface right now. Looks like it's currently only used for storage
devices. So I'm questioning myself: What _real_ benefit would it bring
to use the QEMU block device layer for the CMOS file?

Cheers,
Mathias
Anthony Liguori - Sept. 17, 2010, 1:27 p.m.
On 09/17/2010 01:50 AM, Mathias Krause wrote:
> Am 16.09.2010 19:20 schrieb Anthony Liguori:
>    
>> Instead of using FILE, I'd suggest using a BlockDriver to read and write
>> the data.
>>      
> I'll fix that as soon as I figured how to use this interface.
>
>    
>> I think it would be very nice to add write support too so that writes to
>> CMOS were persisted across boots.
>>      
> Indeed. Also I would like to have a command line interface like '-cmos
> cmos.bin' instead of the ugly '-global mc146818rtc.file=cmos.bin'. But
> I'm not aware how to create such an alias. Any pointers?
>    

Unfortunately, it's a little complicated although it should get better 
in the future.   The right way to do this today would be:

   -drive file=cmos.bin,if=none,id=nvram -global mc146818rtc.drive=nvram

The use of -drive is historic.  We'll have a better option in the future 
that will look something like:

  -blockdev file=cmos.bin,id=nvram -global mc146818rtc.drive=nvram

But in either case, I'd suggest adding an -nvram option that was:

  -nvram <filename>

Which would do:

   drive_add(optarg, "if=none,id=nvram");

And then in the RTC code, default drive to nvram.

It gets a little tough to handle the case of in memory CMOS.

Regards,

Anthony Liguori

>    
>>> +static long get_file_size(FILE *f)
>>> +{
>>> +    long where, size;
>>> +
>>> +    /* XXX: on Unix systems, using fstat() probably makes more sense */
>>> +
>>> +    where = ftell(f);
>>> +    fseek(f, 0, SEEK_END);
>>> +    size = ftell(f);
>>> +    fseek(f, where, SEEK_SET);
>>> +
>>> +    return size;
>>> +}
>>>
>>>        
>> BlockDrivers have a getlength() functions.
>>      
> Would reduce the size of the patch which is always a good thing (tm).
>
>
> Mathias
>
Mathias Krause - Sept. 22, 2010, 7:43 p.m.
On 17.09.2010 15:27, Anthony Liguori wrote:
> On 09/17/2010 01:50 AM, Mathias Krause wrote:
>> Am 16.09.2010 19:20 schrieb Anthony Liguori:
>>   
>>> Instead of using FILE, I'd suggest using a BlockDriver to read and write
>>> the data.
>>>      
>> I'll fix that as soon as I figured how to use this interface.
>>
>>   
>>> I think it would be very nice to add write support too so that writes to
>>> CMOS were persisted across boots.
>>>      
>> Indeed. Also I would like to have a command line interface like '-cmos
>> cmos.bin' instead of the ugly '-global mc146818rtc.file=cmos.bin'. But
>> I'm not aware how to create such an alias. Any pointers?
>>    
> 
> Unfortunately, it's a little complicated although it should get better
> in the future.   The right way to do this today would be:
> 
>   -drive file=cmos.bin,if=none,id=nvram -global mc146818rtc.drive=nvram
> 
> The use of -drive is historic.  We'll have a better option in the future
> that will look something like:
> 
>  -blockdev file=cmos.bin,id=nvram -global mc146818rtc.drive=nvram

Well, I guess "better" lies in the eye of the beholder, then ;)


> But in either case, I'd suggest adding an -nvram option that was:
> 
>  -nvram <filename>
> 
> Which would do:
> 
>   drive_add(optarg, "if=none,id=nvram");
> 
> And then in the RTC code, default drive to nvram.

I managed to add the nvram option but how do I get a reference to the
drive back in the RTC code? Would I just loop with drive_get(IF_NONE, 0,
i) until the id of the returned drive is "nvram"? Doesn't sound right
but I've found no better solution due to the lack of an
drive_get_by_id() function.

> It gets a little tough to handle the case of in memory CMOS.

I don't quite get your point here. The content of the drive is copied in
the RTC init routine into a private char array. How could the type of
the drive may be a problem here?


Regards,

Mathias
Paolo Bonzini - Sept. 23, 2010, 12:12 p.m.
On 09/22/2010 09:43 PM, Mathias Krause wrote:
> I managed to add the nvram option but how do I get a reference to the
> drive back in the RTC code? Would I just loop with drive_get(IF_NONE, 0,
> i) until the id of the returned drive is "nvram"? Doesn't sound right
> but I've found no better solution due to the lack of an
> drive_get_by_id() function.

You can write one. :)  It's going to be very similar to 
drive_get_by_blockdev.

Paolo
Markus Armbruster - Sept. 24, 2010, 12:40 p.m.
Mathias Krause <mathias.krause@secunet.com> writes:

> Hi Kevin,
>
> On 17.09.2010 12:44, Kevin Wolf wrote:
>> Hi Mathias,
>> 
>> Am 17.09.2010 08:42, schrieb Mathias Krause:
>>>> Using QEMU's block devices instead of a simple file would be
>>>> more consistent with the rest of QEMU and allow reading the
>>>> CMOS data not only from a file but also from an URL or other
>>>> sources.
>>> Thanks for the hint. Since this is my first contribution to the project
>>> I'm not that familiar with the code. Looking at other places, e.g. how
>>> the -kernel option gets handled, I just see FILE everywhere. Can you
>>> give me some pointers how to use this interface?
>> 
>> Have a look at block.h which contains the prototypes for the public
>> block layer interface.
>> 
>> Basically, you need to create a BlockDriverState with bdrv_new() and
>> then open it with bdrv_open(). You'll want to specify the raw block
>> driver for opening the image, you get it with bdrv_find_format("raw").
>> bdrv_pread/pwrite are the right functions to access the file with byte
>> granularity (other functions work on 512 byte sectors). bdrv_delete
>> frees the the BlockDriverState when you're done.
>
> Thank you for the detailed writeup. I think I should figure out how to
> use it myself now. Albeit there seem to be not many users of this
> interface right now. Looks like it's currently only used for storage
> devices. So I'm questioning myself: What _real_ benefit would it bring
> to use the QEMU block device layer for the CMOS file?

I'd view initial CMOS contents as configuration.  We don't use the block
layer to access configuration files.
Markus Armbruster - Sept. 24, 2010, 12:42 p.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

> On 09/16/2010 08:58 AM, Mathias Krause wrote:
>> In contrast to the BIOS and Option ROMs the CMOS content cannot be
>> predefined by the user. Also the amount of useable CMOS ARM is pretty
>> limited, even though the amount of CMOS bytes emulated by qemu is
>> already twice as much as of the original MC146818. Nevertheless those
>> limitations are pretty annoying when testing different BIOS replacement
>> implementations like coreboot in combination with FILO that use CMOS
>> values above the basic RTC range for its own purpose to, e.g., control
>> the boot device order using a string containing the boot devices to
>> probe.
>>
>> This patch adds support to specify a file to read the initial CMOS
>> content from. It also increases the CMOS size to 256 bytes and
>> implements access to the extended memory range via I/O ports 0x72 and
>> 0x73.
>>
>> Use it like: `qemu -global mc146818rtc.file=cmos.bin ...' where cmos.bin
>> is a binary file, sized 256 bytes containing the CMOS RAM.
>>
>> Signed-off-by: Mathias Krause<mathias.krause@secunet.com>
>>    
>
> Instead of using FILE, I'd suggest using a BlockDriver to read and
> write the data.
>
> I think it would be very nice to add write support too so that writes
> to CMOS were persisted across boots.

Persisting CMOS makes it state instead of configuration.  Use of block
layer (treat it like a drive) makes more sense then.
Markus Armbruster - Sept. 24, 2010, 12:47 p.m.
Mathias Krause <mathias.krause@secunet.com> writes:

> On 17.09.2010 15:27, Anthony Liguori wrote:
>> On 09/17/2010 01:50 AM, Mathias Krause wrote:
>>> Am 16.09.2010 19:20 schrieb Anthony Liguori:
>>>   
>>>> Instead of using FILE, I'd suggest using a BlockDriver to read and write
>>>> the data.
>>>>      
>>> I'll fix that as soon as I figured how to use this interface.
>>>
>>>   
>>>> I think it would be very nice to add write support too so that writes to
>>>> CMOS were persisted across boots.
>>>>      
>>> Indeed. Also I would like to have a command line interface like '-cmos
>>> cmos.bin' instead of the ugly '-global mc146818rtc.file=cmos.bin'. But
>>> I'm not aware how to create such an alias. Any pointers?
>>>    
>> 
>> Unfortunately, it's a little complicated although it should get better
>> in the future.   The right way to do this today would be:
>> 
>>   -drive file=cmos.bin,if=none,id=nvram -global mc146818rtc.drive=nvram
>> 
>> The use of -drive is historic.  We'll have a better option in the future
>> that will look something like:
>> 
>>  -blockdev file=cmos.bin,id=nvram -global mc146818rtc.drive=nvram
>
> Well, I guess "better" lies in the eye of the beholder, then ;)
>
>
>> But in either case, I'd suggest adding an -nvram option that was:
>> 
>>  -nvram <filename>
>> 
>> Which would do:
>> 
>>   drive_add(optarg, "if=none,id=nvram");
>> 
>> And then in the RTC code, default drive to nvram.
>
> I managed to add the nvram option but how do I get a reference to the
> drive back in the RTC code? Would I just loop with drive_get(IF_NONE, 0,
> i) until the id of the returned drive is "nvram"? Doesn't sound right
> but I've found no better solution due to the lack of an
> drive_get_by_id() function.

Use DEFINE_PROP_DRIVE() to define mc146818rtc's property drive, and it's
automatic: qdev assigns a pointer to the BlockDriverState.

To access DriveInfo, use drive_get_by_blockdev().  I doubt you need
that.

[...]
Mathias Krause - Sept. 26, 2010, 8:44 p.m.
On 24.09.2010 14:47, Markus Armbruster wrote:
> Mathias Krause <mathias.krause@secunet.com> writes:
> 
>> On 17.09.2010 15:27, Anthony Liguori wrote:
>>> On 09/17/2010 01:50 AM, Mathias Krause wrote:
>>>> Am 16.09.2010 19:20 schrieb Anthony Liguori:
>>>>   
>>>>> Instead of using FILE, I'd suggest using a BlockDriver to read and write
>>>>> the data.
>>>>>      
>>>> I'll fix that as soon as I figured how to use this interface.
>>>>
>>>>   
>>>>> I think it would be very nice to add write support too so that writes to
>>>>> CMOS were persisted across boots.
>>>>>      
>>>> Indeed. Also I would like to have a command line interface like '-cmos
>>>> cmos.bin' instead of the ugly '-global mc146818rtc.file=cmos.bin'. But
>>>> I'm not aware how to create such an alias. Any pointers?
>>>>    
>>> Unfortunately, it's a little complicated although it should get better
>>> in the future.   The right way to do this today would be:
>>>
>>>   -drive file=cmos.bin,if=none,id=nvram -global mc146818rtc.drive=nvram
>>>
>>> The use of -drive is historic.  We'll have a better option in the future
>>> that will look something like:
>>>
>>>  -blockdev file=cmos.bin,id=nvram -global mc146818rtc.drive=nvram
>> Well, I guess "better" lies in the eye of the beholder, then ;)
>>
>>
>>> But in either case, I'd suggest adding an -nvram option that was:
>>>
>>>  -nvram <filename>
>>>
>>> Which would do:
>>>
>>>   drive_add(optarg, "if=none,id=nvram");
>>>
>>> And then in the RTC code, default drive to nvram.
>> I managed to add the nvram option but how do I get a reference to the
>> drive back in the RTC code? Would I just loop with drive_get(IF_NONE, 0,
>> i) until the id of the returned drive is "nvram"? Doesn't sound right
>> but I've found no better solution due to the lack of an
>> drive_get_by_id() function.
> 
> Use DEFINE_PROP_DRIVE() to define mc146818rtc's property drive, and it's
> automatic: qdev assigns a pointer to the BlockDriverState.

That works quite well but now, by using the QEMU block layer, the file
for the CMOS data must be a multiple of 512 bytes. Otherwise
bdrv_pread() will fail because the driver only handles full blocks so
truncates the 256 bytes to a block device with 0 blocks. Sadly the block
size of 512 seems to be hardcoded or can it be changed on a per drive basis?


Regards,
Mathias
Markus Armbruster - Oct. 11, 2010, 1:25 p.m.
Maybe Kevin or Christoph (cc'ed) can help.

Mathias Krause <mathias.krause@secunet.com> writes:

> On 24.09.2010 14:47, Markus Armbruster wrote:
>> Mathias Krause <mathias.krause@secunet.com> writes:
>> 
>>> On 17.09.2010 15:27, Anthony Liguori wrote:
>>>> On 09/17/2010 01:50 AM, Mathias Krause wrote:
>>>>> Am 16.09.2010 19:20 schrieb Anthony Liguori:
>>>>>   
>>>>>> Instead of using FILE, I'd suggest using a BlockDriver to read and write
>>>>>> the data.
>>>>>>      
>>>>> I'll fix that as soon as I figured how to use this interface.
>>>>>
>>>>>   
>>>>>> I think it would be very nice to add write support too so that writes to
>>>>>> CMOS were persisted across boots.
>>>>>>      
>>>>> Indeed. Also I would like to have a command line interface like '-cmos
>>>>> cmos.bin' instead of the ugly '-global mc146818rtc.file=cmos.bin'. But
>>>>> I'm not aware how to create such an alias. Any pointers?
>>>>>    
>>>> Unfortunately, it's a little complicated although it should get better
>>>> in the future.   The right way to do this today would be:
>>>>
>>>>   -drive file=cmos.bin,if=none,id=nvram -global mc146818rtc.drive=nvram
>>>>
>>>> The use of -drive is historic.  We'll have a better option in the future
>>>> that will look something like:
>>>>
>>>>  -blockdev file=cmos.bin,id=nvram -global mc146818rtc.drive=nvram
>>> Well, I guess "better" lies in the eye of the beholder, then ;)
>>>
>>>
>>>> But in either case, I'd suggest adding an -nvram option that was:
>>>>
>>>>  -nvram <filename>
>>>>
>>>> Which would do:
>>>>
>>>>   drive_add(optarg, "if=none,id=nvram");
>>>>
>>>> And then in the RTC code, default drive to nvram.
>>> I managed to add the nvram option but how do I get a reference to the
>>> drive back in the RTC code? Would I just loop with drive_get(IF_NONE, 0,
>>> i) until the id of the returned drive is "nvram"? Doesn't sound right
>>> but I've found no better solution due to the lack of an
>>> drive_get_by_id() function.
>> 
>> Use DEFINE_PROP_DRIVE() to define mc146818rtc's property drive, and it's
>> automatic: qdev assigns a pointer to the BlockDriverState.
>
> That works quite well but now, by using the QEMU block layer, the file
> for the CMOS data must be a multiple of 512 bytes. Otherwise
> bdrv_pread() will fail because the driver only handles full blocks so
> truncates the 256 bytes to a block device with 0 blocks. Sadly the block
> size of 512 seems to be hardcoded or can it be changed on a per drive basis?

Patch

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 2b91fa8..9f215e0 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -78,12 +78,16 @@ 
 #define REG_C_PF   0x40
 #define REG_C_AF   0x20
 
+#define BASE_PORT  0x70
+#define CMOS_SIZE  256
+
 typedef struct RTCState {
     ISADevice dev;
-    uint8_t cmos_data[128];
+    uint8_t cmos_data[CMOS_SIZE];
     uint8_t cmos_index;
     struct tm current_tm;
     int32_t base_year;
+    char *file;
     qemu_irq irq;
     qemu_irq sqw_irq;
     int it_shift;
@@ -206,7 +210,7 @@  static void cmos_ioport_write(void *opaque, uint32_t addr, uint32_t data)
     RTCState *s = opaque;
 
     if ((addr & 1) == 0) {
-        s->cmos_index = data & 0x7f;
+        s->cmos_index = data & (addr == BASE_PORT ? 0x7f : 0xff);
     } else {
         CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02x\n",
                      s->cmos_index, data);
@@ -581,7 +585,6 @@  static void rtc_reset(void *opaque)
 static int rtc_initfn(ISADevice *dev)
 {
     RTCState *s = DO_UPCAST(RTCState, dev, dev);
-    int base = 0x70;
 
     s->cmos_data[RTC_REG_A] = 0x26;
     s->cmos_data[RTC_REG_B] = 0x02;
@@ -603,14 +606,57 @@  static int rtc_initfn(ISADevice *dev)
         qemu_get_clock(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
     qemu_mod_timer(s->second_timer2, s->next_second_time);
 
-    register_ioport_write(base, 2, 1, cmos_ioport_write, s);
-    register_ioport_read(base, 2, 1, cmos_ioport_read, s);
+    register_ioport_write(BASE_PORT, 4, 1, cmos_ioport_write, s);
+    register_ioport_read(BASE_PORT, 4, 1, cmos_ioport_read, s);
 
-    qdev_set_legacy_instance_id(&dev->qdev, base, 2);
+    qdev_set_legacy_instance_id(&dev->qdev, BASE_PORT, 2);
     qemu_register_reset(rtc_reset, s);
     return 0;
 }
 
+static long get_file_size(FILE *f)
+{
+    long where, size;
+
+    /* XXX: on Unix systems, using fstat() probably makes more sense */
+
+    where = ftell(f);
+    fseek(f, 0, SEEK_END);
+    size = ftell(f);
+    fseek(f, where, SEEK_SET);
+
+    return size;
+}
+
+static void cmos_load_from_file(RTCState *s, const char *file)
+{
+    const char *err_msg = NULL;
+    FILE *f;
+
+    if (!(f = fopen(file, "r"))) {
+        err_msg = strerror(errno);
+        goto err;
+    }
+
+    if (get_file_size(f) != CMOS_SIZE) {
+        err_msg = "invalid file size";
+        goto err;
+    }
+
+    if (fread(s->cmos_data, 1, CMOS_SIZE, f) != CMOS_SIZE) {
+        err_msg = strerror(errno);
+        goto err;
+    }
+    CMOS_DPRINTF("cmos: initialized from '%s'\n", file);
+
+    fclose(f);
+
+    return;
+err:
+    fprintf(stderr, "qemu: could not load CMOS '%s': %s\n", file, err_msg);
+    exit(1);
+}
+
 ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
 {
     ISADevice *dev;
@@ -618,6 +664,9 @@  ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
 
     dev = isa_create("mc146818rtc");
     s = DO_UPCAST(RTCState, dev, dev);
+    if (s->file != NULL) {
+        cmos_load_from_file(s, s->file);
+    }
     qdev_prop_set_int32(&dev->qdev, "base_year", base_year);
     qdev_init_nofail(&dev->qdev);
     if (intercept_irq) {
@@ -636,6 +685,7 @@  static ISADeviceInfo mc146818rtc_info = {
     .init          = rtc_initfn,
     .qdev.props    = (Property[]) {
         DEFINE_PROP_INT32("base_year", RTCState, base_year, 1980),
+        DEFINE_PROP_STRING("file", RTCState, file),
         DEFINE_PROP_END_OF_LIST(),
     }
 };