From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MBTZe-0003WG-S1 for qemu-devel@nongnu.org; Tue, 02 Jun 2009 08:58:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MBTZa-0003TA-2q for qemu-devel@nongnu.org; Tue, 02 Jun 2009 08:58:38 -0400 Received: from [199.232.76.173] (port=35030 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MBTZZ-0003T3-Sw for qemu-devel@nongnu.org; Tue, 02 Jun 2009 08:58:33 -0400 Received: from mx20.gnu.org ([199.232.41.8]:32297) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1MBTZZ-0004fF-39 for qemu-devel@nongnu.org; Tue, 02 Jun 2009 08:58:33 -0400 Received: from mx2.redhat.com ([66.187.237.31]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MBTZX-0005kW-Os for qemu-devel@nongnu.org; Tue, 02 Jun 2009 08:58:32 -0400 Subject: Re: [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support. References: <1243924970-17545-1-git-send-email-yamahata@valinux.co.jp> <1243924970-17545-8-git-send-email-yamahata@valinux.co.jp> From: Markus Armbruster Date: Tue, 02 Jun 2009 14:56:20 +0200 In-Reply-To: <1243924970-17545-8-git-send-email-yamahata@valinux.co.jp> (Isaku Yamahata's message of "Tue\, 2 Jun 2009 15\:42\:50 +0900") Message-ID: <8763febxqj.fsf@pike.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: paul@codesourcery.com, mtosatti@redhat.com, avi@redhat.com, qemu-devel@nongnu.org, mst@redhat.com Isaku Yamahata writes: > This patch is preliminary for multi pci bus support to > add -pci option. > > Signed-off-by: Isaku Yamahata > > --- [...] > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c > new file mode 100644 > index 0000000..3aedaa1 > --- /dev/null > +++ b/hw/pci_bridge.c > @@ -0,0 +1,232 @@ [...] > +/* > + * Parse [[:]:]., return -1 on error > + */ > +static int pci_parse_devfn(const char *addr, > + int *domp, int *busp, > + unsigned int *slotp, unsigned int *funcp) > +{ > + const char *p; > + char *e; > + unsigned long val; > + unsigned long dom = 0; > + unsigned long bus = 0; > + unsigned int slot = 0; > + unsigned int func = 0; > + > + p = addr; > + val = strtoul(p, &e, 16); > + if (e == p) > + return -1; > + if (*e == ':') { > + bus = val; > + p = e + 1; > + val = strtoul(p, &e, 16); > + if (e == p) > + return -1; > + if (*e == ':') { > + dom = bus; > + bus = val; > + p = e + 1; > + val = strtoul(p, &e, 16); > + if (e == p) > + return -1; > + } > + } > + slot = val; > + > + if (*e != '.') > + return -1; > + p = e + 1; > + val = strtoul(p, &e, 16); > + if (e == p) > + return -1; > + func = val; > + > + if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7) > + return -1; > + > + /* For now, only 0 domain is supported */ > + if (dom != 0) > + return -1; > + > + *domp = dom; > + *busp = bus; > + *slotp = slot; > + *funcp = func; > + return 0; > +} This duplicates much of pci_parse_devaddr() from hw/pci.c. I suggest to extend that function instead: make it take a funcp parameter, and parse the "." part iff funcp != NULL. > + > +int pci_device_parse(const char* options) Style nitpick: char *options > +{ > + int ret = -1; > + char buf[1024]; > + char *e; > + struct PCIDeviceInfo *pd = &pd_table[nb_pci_devices]; > + > + if (nb_pci_devices >= MAX_PCI_DEVS) { > + /* XXX:WARN */ > + goto out; > + } > + > + > + if (get_param_value(buf, sizeof(buf), "pci_addr", options) < 0) { > + /* XXX error */ > + goto out; > + } > + if (pci_parse_devfn(buf, &pd->dom, &pd->bus, &pd->slot, &pd->func) < 0) { > + goto out; > + } > + > + if (get_param_value(buf, sizeof(buf), "secondary", options) < 0) { > + goto out; > + } > + pd->secondary_bus = strtoul(buf, &e, 16); > + if (buf == e) { > + goto out; > + } > + if (pd->secondary_bus > 0xff) { > + goto out; > + } > + > + if (get_param_value(buf, sizeof(buf), "model", options) < 0) { > + goto out; > + } > + pd->model = strdup(buf); > + if (pd->model == NULL) { > + goto out; > + } > + > + /* save for later use */ > + pd->options = strdup(options); > + if (pd->options == NULL) { > + goto out; > + } > + > + nb_pci_devices++; > + ret = 0; > + > +out: > + return ret; > +} > + > +static int pci_device_init_one(struct PCIDeviceInfo *pd) > +{ > + PCIBus *parent_bus; > + PCIBus *bus; > + > + if (pci_find_device(pd->bus, pd->slot, pd->func) != NULL) { > + return -1; > + } > + > + parent_bus = pci_find_bus(pd->bus); > + if (parent_bus == NULL) { > + return -1; > + } > + > + bus = pci_bridge_create_simple(parent_bus, PCI_DEVFN(pd->slot, pd->func), > + PCI_VENDOR_ID_INTEL, > + PCI_DEVICE_ID_INTEL_82801BA_11, > + &i82801ba11_map_irq, "PCI to PCI Bridge", > + pd->secondary_bus); > + return 0; > +} > + > +int pci_device_init(void) > +{ > + int i; > + int ret = 0; > + > + for (i = 0; i < nb_pci_devices; i++) { > + struct PCIDeviceInfo *pd = &pd_table[i]; > + const char *model = pd->model; > + > + if (!strcmp(model, "bridge")) { > + ret = pci_device_init_one(pd); > + } else { > + /* unknown model */ > + ret = -1; I think we better print suitable diagnostics here. > + } > + > + if (ret < 0) > + break; > + } > + return ret; > +} > + > +/* > + * Local variables: > + * c-indent-level: 4 > + * c-basic-offset: 4 > + * tab-width: 8 > + * indent-tab-mode: nil > + * End: > + */ [...] > diff --git a/qemu-options.hx b/qemu-options.hx > index 87af798..c5be603 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -886,6 +886,16 @@ override the default configuration (@option{-net nic -net user}) which > is activated if no @option{-net} options are provided. > ETEXI > > +DEF("pci", HAS_ARG, QEMU_OPTION_pci, \ > + "-pci pci_addr=pci_addr,model=type[,model specific options]\n" > + "pci_addr = [[:]:].\n" > + "model = bridge\n") > +STEXI > +@item -pci pci_addr=@var{pci_addr},model=@var{type}[,@var{model specific options}] > +connect pci device of model @var{type} at pci bus address of @var{pci_addr} > +with model specific options. > +ETEXI > + I find pci_addr a bit unfortunate (which of the various addresses associated with a PCI device is it?), but it follows the precedent set by monitor commands drive_add, pci_add, pci_del. > #ifdef CONFIG_SLIRP > DEF("tftp", HAS_ARG, QEMU_OPTION_tftp, \ > "-tftp dir allow tftp access to files in dir [-net user]\n") > diff --git a/vl.c b/vl.c > index f8c0d00..0082250 100644 > --- a/vl.c > +++ b/vl.c > @@ -134,6 +134,7 @@ int main(int argc, char **argv) > #include "hw/usb.h" > #include "hw/pcmcia.h" > #include "hw/pc.h" > +#include "hw/pci_bridge.h" > #include "hw/audiodev.h" > #include "hw/isa.h" > #include "hw/baum.h" > @@ -5139,6 +5140,10 @@ int main(int argc, char **argv, char **envp) > net_clients[nb_net_clients] = optarg; > nb_net_clients++; > break; > + case QEMU_OPTION_pci: > + if (pci_device_parse(optarg) < 0) > + exit(1); > + break; > #ifdef CONFIG_SLIRP > case QEMU_OPTION_tftp: > tftp_prefix = optarg;