From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsrI5-0002cw-Ks for qemu-devel@nongnu.org; Wed, 09 Jan 2013 03:45:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsrI4-00040f-AJ for qemu-devel@nongnu.org; Wed, 09 Jan 2013 03:45:41 -0500 Received: from greensocs.com ([87.106.252.221]:48262 helo=s15328186.onlinehome-server.info) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsrI3-00040E-W8 for qemu-devel@nongnu.org; Wed, 09 Jan 2013 03:45:40 -0500 Message-ID: <50ED2E2F.2090301@greensocs.com> Date: Wed, 09 Jan 2013 09:45:35 +0100 From: =?UTF-8?B?S09OUkFEIEZyw6lkw6lyaWM=?= MIME-Version: 1.0 References: <1357584074-10852-1-git-send-email-fred.konrad@greensocs.com> <1357584074-10852-5-git-send-email-fred.konrad@greensocs.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/61] virtio-pci : refactor virtio-pci device. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: kwolf@redhat.com, aliguori@us.ibm.com, e.voevodin@samsung.com, mst@redhat.com, mark.burton@greensocs.com, qemu-devel@nongnu.org, agraf@suse.de, cornelia.huck@de.ibm.com, aneesh.kumar@linux.vnet.ibm.com, stefanha@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com, afaerber@suse.de On 08/01/2013 18:54, Peter Maydell wrote: > On 7 January 2013 18:40, wrote: >> From: KONRAD Frederic >> >> Create the virtio-pci device. This transport device will create a >> virtio-pci-bus, so one VirtIODevice can be connected. >> >> Signed-off-by: KONRAD Frederic >> --- >> hw/virtio-pci.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/virtio-pci.h | 18 ++++++++ >> 2 files changed, 145 insertions(+) >> >> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >> index 5ff03e8..1f0ecbe 100644 >> --- a/hw/virtio-pci.c >> +++ b/hw/virtio-pci.c >> @@ -1163,6 +1163,130 @@ static TypeInfo virtio_scsi_info = { >> .class_init = virtio_scsi_class_init, >> }; >> >> +/* >> + * virtio-pci : This is the PCIDevice which have a virtio-pci-bus. > "has" > >> + */ >> + >> +/* This is called by virtio-bus just after the device is plugged. */ >> +static void virtio_pci_device_plugged(DeviceState *d) >> +{ >> + VirtIOPCIProxy *proxy = VIRTIO_PCI(d); >> + VirtioBusState *bus = proxy->bus; >> + uint8_t *config; >> + uint32_t size; >> + >> + /* Put the PCI IDs */ >> + switch (virtio_device_get_id(proxy->bus)) { >> + >> + >> + default: >> + error_report("unknown device id\n"); >> + break; >> + >> + } > This doesn't have any code in it yet (it gets added in later patches) but > the final result looks very repetitive. I think you'd be better off just having > some arrays: > > uint16_t virtio_pci_device_id[] = { > [VIRTIO_ID_BLOCK] = PCI_DEVICE_ID_VIRTIO_BLOCK, > [VIRTIO_ID_NET] = PCI_DEVICE_ID_VIRTIO_NET, > (etc) > }; > > similarly for the class. Then you can just drop the switch statement. > > In fact I think you might as well put in the array entries for all > the virtio devices in this patch rather than adding one in each of the > "add virtio-foo device" patches; it will do no harm for them to be > there early and it makes the later patches a little smaller. And what happen when it is a bad ID ? Fred > >> + >> + proxy->vdev = proxy->bus->vdev; >> + >> + config = proxy->pci_dev.config; >> + if (proxy->class_code) { >> + pci_config_set_class(config, proxy->class_code); >> + } >> + pci_set_word(config + PCI_SUBSYSTEM_VENDOR_ID, >> + pci_get_word(config + PCI_VENDOR_ID)); >> + pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_device_get_id(proxy->bus)); >> + config[PCI_INTERRUPT_PIN] = 1; >> --- a/hw/virtio-pci.h >> +++ b/hw/virtio-pci.h >> @@ -45,6 +45,22 @@ typedef struct { >> unsigned int users; >> } VirtIOIRQFD; >> >> +/* >> + * virtio-pci : This is the PCIDevice which have a virtio-pci-bus. > "has" > >> + */ > -- PMM >