From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33595 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PXgmt-0005nb-EB for qemu-devel@nongnu.org; Tue, 28 Dec 2010 16:08:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PXgmr-0001Nz-PU for qemu-devel@nongnu.org; Tue, 28 Dec 2010 16:08:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36888) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PXgmr-0001Nl-Id for qemu-devel@nongnu.org; Tue, 28 Dec 2010 16:08:53 -0500 Date: Tue, 28 Dec 2010 23:08:31 +0200 From: "Michael S. Tsirkin" Message-ID: <20101228210831.GA4046@redhat.com> References: <1292879881$2997@local> <20101227140136.GA27055@redhat.com> <5C6885C942B1469F90662F3DC1C2D8A1@FSCPC> <20101228171955.GA2850@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH] piix: use pci_config_set_prog_interface() List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sebastian Herbszt Cc: qemu-devel@nongnu.org On Tue, Dec 28, 2010 at 07:48:23PM +0100, Sebastian Herbszt wrote: > Michael S. Tsirkin wrote: > >On Tue, Dec 28, 2010 at 05:24:06PM +0100, Sebastian Herbszt wrote: > >>Michael S. Tsirkin wrote: > >>>On Mon, Dec 20, 2010 at 10:18:01PM +0100, Sebastian Herbszt wrote: > >>>>Use pci_config_set_prog_interface(). > >>>> > >>>>Signed-off-by: Sebastian Herbszt > >>> > >>>Since I was asked explicitly - I don't have a problem > >>>with these changes: both class and prog interface. > >>>However, they aren't all that useful in themselves. > >>> > >>>For class, what I would like to see is a system where > >>>the device class is put in the qdev info table, > >>>and where -device ? > >>>(and hopefully the legacy -help, -nic etc as well) > >>>use this information. > >> > >>I am not sure if you mean something like the patch below. > > > >Not exactly > > > >- I'd like to keep the pci_config_set_class in the devices, > > just make it do an assert. > > Assert on which condition? That PCI class matches device type defined in qdev. This will serve to verify that all devices are converted properly. > >- Nics already have DEFINE_NIC_PROPERTIES - can this be used somehow? > > Same for other devices ... > > We got DEFINE_NIC_PROPERTIES and DEFINE_BLOCK_PROPERTIES. > Something like DEFINE_PCI_PROPERTIES could be introduced, but i am not sure This shouldn't have to do anything with PCI. We should define the device type as NIC, PCI class can be derived from that. > which device properties it should hold (vendor_id, device_id, class, etc?). > Those will then be user-modifiable with e.g. -device e1000,pci_class=1234. > > Sebastian Why is this helpful? - tweaking class will just break guests - binary representation is unfriendly, let's not require users to read pci spec just to run qemu. I can imagine some useful features implemented by sticking the type in qdev: - -device ? would sort devices by type and print the type, - something like this for qtree etc - -help would get all e.g. nic models from qdev, - allow easy to remember aliases with per-type scope (e.g. -device virtio,type=nic to be equivalent to -device virtio-net-pci) - filter (for device ?, qtree etc) system devices that user has no business tweaking (could be type=system or something like that) > diff --git a/hw/e1000.c b/hw/e1000.c > index af101bd..e302703 100644 > --- a/hw/e1000.c > +++ b/hw/e1000.c > @@ -1171,6 +1171,7 @@ static PCIDeviceInfo e1000_info = { > .romfile = "pxe-e1000.bin", > .qdev.props = (Property[]) { > DEFINE_NIC_PROPERTIES(E1000State, conf), > + DEFINE_PROP_UINT16("pci_class", E1000State, dev.class, PCI_CLASS_NETWORK_ETHERNET), > DEFINE_PROP_END_OF_LIST(), > } > }; > diff --git a/hw/pci.c b/hw/pci.c > index ef00d20..06bbf04 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1620,6 +1620,9 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > info->is_bridge); > if (pci_dev == NULL) > return -1; > + > + pci_config_set_class(pci_dev->config, pci_dev->class); > + > rc = info->init(pci_dev); > if (rc != 0) { > do_pci_unregister_device(pci_dev); > diff --git a/hw/pci.h b/hw/pci.h > index 17744dc..5dc9053 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -189,6 +189,9 @@ struct PCIDevice { > char *romfile; > ram_addr_t rom_offset; > uint32_t rom_bar; > + > + /* class */ Please don't copy code into comments verbatim. > + uint16_t class; Ugh. We have class in config already. > }; > > PCIDevice *pci_register_device(PCIBus *bus, const char *name, >