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

[Anthony Liguori]

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