Re: [Qemu-devel] [PATCH 7/7] [RFC] pci bus: preliminary for multi pci bus support.

[Markus Armbruster <armbru@redhat.com>]

Isaku Yamahata <yamahata@valinux.co.jp> writes:

> This patch is preliminary for multi pci bus support to
> add -pci option.
>
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
>
> ---
[...]
> 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 [[<domain>:]<bus>:]<slot>.<func>, 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 ".<func>" 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 = [[<domain>:]<bus>:]<slot>.<func>\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;