[Qemu-devel] Re: [libvirt] [PATCH 3/9] Add new domain device: "controller"

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote:
> This augments virDomainDevice with a <controller> element
> that is used to represent disk controllers (e.g., scsi
> controllers). The XML format is given by
> 
> <controller type="scsi" id="<my_id>">
>    <bus addr="<Domain>:<Bus>:<Slot>">
> </controller>
> 
> where type denotes the disk interface (scsi, ide,...), id
> is an arbitrary string that identifies the controller for
> disk hotadd/remove, and bus addr denotes the controller address
> on the PCI bus.
> 
> The bus element can be omitted; in this case,
> an address will be assigned automatically. Currently,
> only hotplugging scsi controllers on a PCI bus
> is supported, and this only for qemu guests

As mentioned in the previous patch, I reckon 'id' is better
called 'name'. 

For PCI addresses, it is desirable to fully normalize the XML
format, by which I mean have separate attributes for domain,
bus and slot. We already have a syntax for PCI addresses used
for host device passthrough, so it'd make sense to use the
same syntax here for controllers. More broadly, we're probably
going to have to add a PCI address element to all our devices.
While it is unlikely we'll need non-PCI addresses, it doesn't
hurt to make it explicit by adding a type='pci' attribute

Thus I'd suggest

    <address type='pci' domain='0x0000' bus='0x06' slot='0x12'/>

Instead of

    <bus addr="<Domain>:<Bus>:<Slot>">

In the domain_conf.c/.h parser, we could have a datatype like

   enum {
      VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI,

      VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST
   };

   typedef struct _virDomainDevicePCIAddress virDomainDevicePCIAddress;
   struct _virDomainDevicePCIAddress {
        unsigned domain;
        unsigned bus;
        unsigned slot;
        unsigned function;
   };
   typedef struct _virDomainDeviceAddress virDomainDeviceAddress;
   struct _virDomainDeviceAddress {
      int type;
      union {
         virDomainDevicePCIAddress pci;
      } data;
   };


Which we then use in all the places in domain_conf.h wanting
address information. Obviously we'd not use the 'function'
field in most places, but doesn't hurt to have it.

And a pair of methods for parsing/formatting this address info
we can call from all the appropriate locations.


> diff --git a/src/domain_conf.h b/src/domain_conf.h
> index 898f6c9..6b3cb09 100644
> --- a/src/domain_conf.h
> +++ b/src/domain_conf.h
> @@ -111,6 +111,11 @@ struct _virDomainDiskDef {
>      char *src;
>      char *dst;
>      char *controller_id;
> +    struct {
> +        unsigned domain;
> +        unsigned bus;
> +        unsigned slot;
> +    } controller_pci_addr;

I think we should stick to just using the controller name
as the mandatory identifier for cross-referencing disks
to controllers.

>      char *driverName;
>      char *driverType;
>      char *serial;
> @@ -125,6 +130,19 @@ struct _virDomainDiskDef {
>      virStorageEncryptionPtr encryption;
>  };
>  
> +/* Stores the virtual disk controller configuration */
> +typedef struct _virDomainControllerDef virDomainControllerDef;
> +typedef virDomainControllerDef *virDomainControllerDefPtr;
> +struct _virDomainControllerDef {
> +    int type;
> +    char *id;
> +    struct {
> +        unsigned domain;
> +        unsigned bus;
> +        unsigned slot;
> +    } pci_addr;
> +};

With the generic address data type and s/id/name/, this would be just

  struct _virDomainControllerDef {
     int type;
     char *name;
     virDomainDeviceAddress addr;
  };


Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|