Re: [Qemu-devel] [PATCHv2 1/8] Introduce deriver_name field to DeviceInfo structure.

[Markus Armbruster <armbru@redhat.com>]

Gleb Natapov <gleb@redhat.com> writes:

> On Sat, Nov 06, 2010 at 10:01:25AM +0100, Markus Armbruster wrote:
> [skip]
>> > Why should Seabios match against three (or even more) different type of
>> > devices to detect ata interface? Why require Seabios changes when this
>> > can be avoided (if new device that provide ata is added)? OpenBIOS also
>> > supports qemu BTW (this is Open Firmware implementation for pc, you can
>> > run and see what kind of device paths it generates). 
>> 
>> I think we've finally cut through the confusion caused by the
>> unfortunate choice of driver_name for this new device attribute.
>> 
>> The fact that you choose values of your driver_name in a way that is
>> inspired by the syntactic conventions of IEEE 1275 is not its
>> distinguishing characteristic.  The values of existing member name are
>> inspired by that as well.  driver_name's distinguishing characteristic
>> is its purpose: communication with SeaBIOS.
>> 
> My understanding of this name in IEEE 1275 is that it specifies what
> driver in FW handles a device.
>
>> I'm fine with you choosing its values however it's convenient for that
>> purpose, as long as you give it a name reflecting that purpose.  What
>> about fw_name and qdev_fw_name()?
>> 
> I am not attached to the name. Can "alias" be used for that purpose?

alias needs to be an unambigous name of the device, just like name.  If
I understand you correctly, you want to use the same string for
different, yet sufficiently compatible devices, so alias won't do.

>> Next, I'm worried about overloading of method get_dev_path().  It's
>> being used for a very specific purpose: savevm/loadvm.  
>> 
> This part of the patch is not completed yet. I intend to change the code
> in savevm/loadvm to call qdev_get_dev_path() to get full device path
> there.
>
>> * It's currently defined only for PCI devices.  Your PATCH 7/8 changes
>>   its value there, from DOMAIN:BUS:SLOT.FUNCTION to FW_NAME@SLOT.
>> 
> Old definition is buggy BTW. BUS part is controlled by a guest and may
> be different from default value at destination.

Yes, it's problematic for anything domain:bus 0:0.  Which happens to be
the only one that can occur here currently, as far as I know.

>>   - The old value identifies the qdev.  The new value does not
>>     (remember, we have a separate qdev per PCI function).  Why is this
>>     okay?
>> 
> No no. New value is FW_NAME@SLOT,FUNC. Spec says that if FUNC is zero it
> can be omitted.

Okay.

>>   - Is the value saved with the VM?  If yes, this is an incompatible
>>     change.
> Don't understand that remark.

If the value somehow makes it into the savevm data, then changing values
could render the data incompatible: old qemu can't load new data, and
vice versa.

>> * You extend it for ISA and System bus (PATCH 5,6/8).  How does this
>>   affect savevm?
>> 
> We should ask savevm experts. As far as I can see it affects id
> creation. As long as id is unique we should be OK. We may send more info
> on migration after the patches.

We definitely need review and testing there.