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

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

[Markus Armbruster]

Devices inherit some properties from their bus.  For instance, ISA
devices inherit iobase, iobase2, and PCI devices inherit addr.

Properties have a default value.  For inherited properties, the same
default value has to do for all devices on the same kind of bus, because
the default value is inherited from the bus along with the property.

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.

Note that isa_create_simple() mask this problem: it takes iobases as
arguments, and the property default value is not used.  The problem
becomes visible when devices are created with -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;

Workable, if not exactly elegant.

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?

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

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

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> 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.

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

[Gerd Hoffmann]

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

For -serial (and maybe -parallel) you probably want to have that because 
there are standard i/o port for serial lines 1 -> 4.  More than one 
serial line isn't that uncommon too, and even in a pci machine these are 
isa devices.

For sb16, ne2k_isa, ... we can probably simply go with default values 
(so -device $name just works for a single card) and expect the user to 
override iobase in case he wants to plug multiple cards into his isapc.

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

Only needed for devices which want to have different default values per 
instance (i.e. serial, see above).  Otherwise property defaults (or no 
iobase property at all) will do just fine.

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

Sure.  Might be overkill for just one or two use cases though.

cheers,
   Gerd