Re: [Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/22/2010 10:17 AM, Michael S. Tsirkin wrote:
> On Mon, Mar 22, 2010 at 10:03:29AM -0500, Anthony Liguori wrote:
>    
>> On 03/22/2010 09:50 AM, Michael S. Tsirkin wrote:
>>      
>>> On Mon, Mar 22, 2010 at 09:49:03AM -0500, Anthony Liguori wrote:
>>>
>>>        
>>>> On 03/22/2010 08:30 AM, Paul Brook wrote:
>>>>
>>>>          
>>>>>> A VirtIOBlock device cannot be a VirtIODevice while being a
>>>>>> VirtIOPCIProxy (proxy is a poor name, btw).
>>>>>>
>>>>>> It really ought to be:
>>>>>>
>>>>>> DeviceState ->    VirtIODevice ->    VirtIOBlock
>>>>>>
>>>>>> and:
>>>>>>
>>>>>> PCIDevice ->    VirtIOPCI : implements VirtIOBus
>>>>>>
>>>>>> The interface between the VirtIODevice and VirtIOBus is the virtio
>>>>>> transport.
>>>>>>
>>>>>> The main reason a separate bus is needed is the same reason it's needed
>>>>>> in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to
>>>>>> the PCI bus without having it be part of the implementation detail.
>>>>>> Introducing another bus type fixes this (and it's what we do in the
>>>>>>     kernel).
>>>>>>
>>>>>>
>>>>>>              
>>>>> Why does virtio need a device state and bus at all?
>>>>>
>>>>>
>>>>>            
>>>> Because you need VirtIOBlock to have qdev properties that can be set.
>>>>
>>>> You also need VirtIOPCI to have separate qdev properties that can be set.
>>>>
>>>>
>>>>          
>>>>> Can't it just be an internal implementation interface, which happens to be
>>>>> used by all devices that happen to exposed a block device over a virtio
>>>>> transport?
>>>>>
>>>>>
>>>>>            
>>>> Theoretically, yes, but given the rest of the infrastructure's
>>>> interaction with qdev, making it a device makes the most sense IMHO.
>>>>
>>>>          
>>> Does this boil down to qdev wanting to be the 1st field
>>> in the structure, again? We can just lift that limitation.
>>>
>>>        
>> No, I don't think it's relevant at all.
>>
>> It's a classic OOP problem.
>>
>> VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
>>
>> VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
>>
>> But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be
>> an is-a relationship.  Initially, this was true and that's why the code
>> was structured that way.  Now that we have two type so of virtio
>> transports, we need to change the modelling.  It needs to get inverted
>> into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
>>
>> When one device has-a one or more other devices, we model that as a Bus.
>>      
> Hmm. Is anything wrong with VirtIOPCIBlock which would be both a VirtIOBlock
> and VirtIOPCI device?
>    

The problem is, VirtIODevice needs to interact with VirtIOPCI.  If you  
do this as:

VirtIOBlock         -> VirtIOPCIBlock
VirtIOPCIDevice ->

Then you need to redirect through the hierarchy.  It gets messy pretty 
quickly.  That's sort of what we do with VirtIOPCIProxy today and it's 
pretty ugly.

>> It's just like SCSI.  SCSIDisk is-a SCSIDevice, SCSIDevice is-a DeviceState.
>>
>> LSIState is-a PCIDevice, PCIDevice is-a DeviceState.
>>
>> LSIState has-a SCSIDevice because LSIState implements the SCSIBus interface.
>>      
> Yes but LSIState emulates a real HBI ...
>    

But look at the lguest virtio implement.  We would definitely model a 
VirtIOBus if we implemented something like that in qemu.  VirtIO really 
is designed to be a bus.

>>>> I can't envision any reason why we would ever want to have two MSI
>>>> vectors for a given queue.
>>>>
>>>>          
>>> No. OTOH whether we want a shared vector or a per-vq vector
>>> might depend on the device being used.
>>>
>>>        
>> Yup.  From a users perspective, we don't want them to create two
>> separate devices and manipulate parameters.  We definitely want one
>> interface where we can manipulate both VirtIODevice and VirtIOPCI
>> parameters.
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> Will a bus really help? Where would we put the # of vectors?
> I think this can't be a virtio-pci bus property as it might be different
> between different virtio pci devices.
>    

There would be a nvectors property of virtio-pci, you'd create a 
virtio-pci device, set nvectors, and add a VirtIODevice to it.  Sounds 
obtuse from a user's perspective so we'd want to simplify the syntax.  
But in terms of internal modelling, it really simplifies things 
tremendously.

Regards,

Anthony Liguori