From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mqlw4-0007rV-Cj for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:52:28 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mqlvz-0007pq-OU for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:52:27 -0400 Received: from [199.232.76.173] (port=59559 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mqlvz-0007pk-2f for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:52:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10374) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mqlvy-0005o0-9I for qemu-devel@nongnu.org; Thu, 24 Sep 2009 06:52:22 -0400 Date: Thu, 24 Sep 2009 11:52:14 +0100 From: "Daniel P. Berrange" Message-ID: <20090924105214.GC21831@redhat.com> References: <1253287576-12875-1-git-send-email-wolfgang.mauerer@siemens.com> <1253287576-12875-4-git-send-email-wolfgang.mauerer@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1253287576-12875-4-git-send-email-wolfgang.mauerer@siemens.com> Subject: [Qemu-devel] Re: [libvirt] [PATCH 3/9] Add new domain device: "controller" Reply-To: "Daniel P. Berrange" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wolfgang Mauerer Cc: libvir-list@redhat.com, jan.kiszka@siemens.com, qemu-devel@nongnu.org On Fri, Sep 18, 2009 at 05:26:10PM +0200, Wolfgang Mauerer wrote: > This augments virDomainDevice with a element > that is used to represent disk controllers (e.g., scsi > controllers). The XML format is given by > > > > > > 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
Instead of 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 :|