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

[Gerd Hoffmann]

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.

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.

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

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

cheers,
  Gerd