Re: [Qemu-devel] Selecting device variant types based on bdrv size

[Peter Crosthwaite]

Hi Andreas, Jan,

On Mon, May 27, 2013 at 11:40 PM, Andreas Färber <address@hidden> wrote:
> Hi,
>
> Am 27.05.2013 09:50, schrieb Peter Crosthwaite:
>> I have a bit of a chicken and egg problem trying to refactor Jans AT24
>> I2C EEPROM model. I'm trying to migrate static class properties up to
>> the class level rather than down on the device property level (as we
>> did for EHCI in the sysbusification a while back). Problem is the
>> device model has part autodetection logic based on the size of the
>> backing image - you can instantiate the "abstract" class and selection
>> of what part it is depends on the backing file size. And so if we go
>> for one class for each separate part, we don't actually know what
>> concrete class to instantiate until we have a handle on the bdrv at
>> realize time which is way to late. Any Ideas?
>
> I think the practical question to ask is: What's the difference between
> those subclasses? Then maybe you can initialize YourAutoType::field from
> DerivedTypeClass::template_field or the like. I.e., the class is

So with the work im trying to do, the difference is actually up in the
parent classes. Inheritance goes something like this:

TYPE_I2C_DEVICE -> TYPE_AT24 -> { TYPE_AT_24_08, TYPE_AT_24_16, ..... }

The issue is, each of the I2C devices has a different number of
"device bits" (the new property) that the machine model may set for
the I2C address. Jans implementation has this as a property on the
TYPE_AT24 level, however I have other devices wanting this property
and its a common feature amongst I2C devices, so im trying to push it
up to TYPE_I2C_DEVICE. But I feel like num_device_bits belongs as a
class property in I2CSlaveClass so I'm not sure I want to push it into
I2CSlaveState for the sake of at24s late auto detection.

When inheritance fails you though, there's always composition. Add a
TYPE_AT_24_GENERIC concrete that just instantiates the desired type as
its own child and passes through all the I2C API callbacks.

Jan, does that work for you? All it means is you see an extra level in
the device heirachy. Id almost say "its a feature not a bug" as it
means the auto-detected child device type will appear in the device
heirachy now so you can inspect which specific device was
auto-instantiated.

> supposed to exist only once, so you can't modify it beyond class_init,
> but you can modify the instance including field values and per-instance
> callback hooks.
>
> For a vaguely related example, you may want to look at the history of
> target-ppc/kvm.c for how I previously mutated the host-ppc-cpu type -
> depending on the host, not user parameters (today it uses inheritance
> from a dynamically chosen base type instead; reason was not a technical
> one but that target-ppc/kvm.c does not get compile-tested on x86 if
> someone changes/adds ppc-cpu fields - no concern for regular devices).
>
> FWIW bdrv not fitting well into the realize scheme was the main reason
> behind going for DeviceState rather than Object for realize. ;)
>

I'm not sure Object::Realise is going to help me. Its more fundamental
isn't it? realize on any level is going to be too late as I want to do
class selection based on a property. I guess worded like that I am
asking the impossible :)

> BTW do we have any guidance of when to use properties vs. subclasses?
> Might be a good addition to the QOMConventions page since it recently
> came up for CAN as well.
>

Yes please! I'll have a search through CAN discussion.

>> Can you safely change a devices type at realize time?
>>
>> realize() {
>>    ...
>>    OBJECT(dev)->class = the_now_known_correct_child_class;
>>    ...
>> }
>>
>> Obviously this would need an API call in QOM to sanity check it.
>
> Short answer: No, such a mutation is generally unsafe.
>
> Instance sizes can differ between types - could be sanity-checked.

Yep. Was thinking that this only works if the type doesn't grow.
Assert otherwise.

> A type can expect to get access to its final class on instance_init.

Yes this is a curly one that I dont have any good answers too.

> instance_init may init fields that you can only get by instantiating.
> A type mutation would change child<> or link<> properties at runtime.

Odd. Any good reason why these are type aware?

Thanks for the input.

Regards,
Peter

> Realize will be too late to tweak the resulting instance further.
> Real OO languages don't support it, causing QOM lock-in.
>
> So I think this is rather hinting into the direction of a three-stage
> construction - instance_init, open, realize - as discussed by
> Kevin/Markus some time ago.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>