Re: [Qemu-devel] Defaults for qdev properties inherited from bus

[Markus Armbruster]

Gerd Hoffmann <address@hidden> writes:

> On 09/03/09 23:49, Markus Armbruster wrote:
>> This isn't always a problem.  For instance, PCI addr defaulting to "pick
>> one" is exactly right for most devices.  Not so for the ISA iobases:
>> devices have different default I/O addresses.
>
> Hmm.  Maybe it was a bas idea to make the iobase a bus property in the
> first place.
>
> We have basically two groups of isa devices:
>
>  (1) standard isa devices which are present in every machine at a fixed
>      address.  Even at non-pc hardware.  Floppy controller, PS/2
>      keyboard, ...
>  (2) devices which can be optionally plugged in, potentially multiple
>      instances with multiple iobases (ne2k_isa, sb16, serial ports,
>      ...).
>
> The first group is hard-coded in machine->init() right now, and some
> day they are probaby listed in pc.dtc.  They will never ever be added
> via -device.  And making iobase configurable doesn't make sense at
> all.

Yes.  Changing iobases of such core devices amounts to defining a new
machine type.

> The second group has instance-dependent defaults (see ${device}_io
> arrays in pc.c and elsewhere), i.e. first ne2k_isa at 0x300, second at
> 0x320, etc.  Property defaults can cover instance #1 only.

The instance-dependent defaults are occasionally convenient, but I'm not
sure the functionality worth replicating for -device.

>> One way to deal with this is to code the true default values in the
>> device's init() callback: check whether the value is still the
>> inherited, unwanted default, and if yes, set it to the true default.
>> Like this:
>>
>>      if (dev->iobase[0] == -1)
>>          dev->iobase[0] = 0x441;
>>      if (dev->iobase[1] == -1)
>>          dev->iobase[1] = 0x443;
>
> We can move the io properties from the bus to the devices which
> actually need it.  Then this would probably become something like
> this:
>
>       if (ne2k_isa->iobase == -1)
>           ne2c_isa->iobase = default_base[instance];

Not sure I get what you mean.  If your proposal has the above code in
the init() callback, then it's not much of an improvement.

The cleanest way to implement defaults is to get the job done in
qdev_create().  No second-guessing in init() whether the default has
been overridden by the user.

A general mechanism to define device defaults for inherited properties
would solve the problem.  Overkill?

>> A related problem is validation of property vales.  Devices may accept
>> only a subset of the values the property inherited from the bus accepts.
>> For instance, ISA devices can typically use only to a fixed set of I/O
>> addresses.  The bus's PropertyInfo doesn't know them, so this doesn't
>> get checked.
>>
>> One way to deal with this is to validate in the device's init()
>> callback: fail it when the values aren't acceptable.  Like this:
>>
>>      if (dev->iobase[0] != 0x441 || dev->iobase[1] != 0x443)
>>          return -1;
>>
>> Detects bad configuration relatively late.  Is this okay?
>
> We have to live with that anyway, so I don't see this as a big
> issue. We can try to model some common checks into properties (stuff
> like valid value ranges).  But there will always be cases which can't
> be checked this way ...

We could split device configuration validation and device initialization
into separate callbacks.