Re: [Qemu-devel] [patch 1/2] PCI: full device address parsing

[Markus Armbruster <armbru@redhat.com>]

Marcelo Tosatti <marcelo@hera.kernel.org> writes:

> From: Markus Armbruster <armbru@pond.sub.org>
>
> This code parses full PCI device addresses.  It then rejects domains
> other than zero, because these are not supported in QEMU.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

Patch has issues with reporting errors.

> Index: trunk/hw/pci.c
> ===================================================================
> --- trunk.orig/hw/pci.c
> +++ trunk/hw/pci.c
> @@ -26,6 +26,7 @@
>  #include "console.h"
>  #include "net.h"
>  #include "virtio-net.h"
> +#include "sysemu.h"
>  
>  //#define DEBUG_PCI
>  
> @@ -158,6 +159,89 @@ static int pci_set_default_subsystem_id(
>      return 0;
>  }
>  
> +/*
> + * Parse [[<domain>:]<bus>:]<slot>, return -1 on error
> + */
> +static int pci_parse_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp)
> +{
> +    const char *p;
> +    char *e;
> +    unsigned long val;
> +    unsigned long dom = 0, bus = 0;
> +    unsigned slot = 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;
> +	}
> +    }
> +
> +    if (dom > 0xffff || bus > 0xff || val > 0x1f) {
> +        fprintf(stderr, "PCI device address %s out of range", addr);
> +	 return -1;

Funny indentation.

> +    }
> +    slot = val;
> +
> +    if (*e)
> +	return -1;
> +
> +    /* Note: QEMU doesn't implement domains other than 0 */
> +    if (dom != 0 || pci_find_bus(bus) == NULL) {
> +	fprintf(stderr, "PCI device address %s not supported", addr);
> +	return -1;
> +    }
> +
> +    *domp = dom;
> +    *busp = bus;
> +    *slotp = slot;
> +    return 0;
> +}

Some failures print to stderr, some are silent.

> +
> +int pci_read_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp)
> +{
> +    char devaddr[32];
> +
> +    if (!get_param_value(devaddr, sizeof(devaddr), "pci_addr", addr)) {
> +        term_printf("Missing pci_addr=\n");
> +        return -1;
> +    }
> +
> +    return pci_parse_devaddr(devaddr, domp, busp, slotp);
> +}

Some failures print to stderr, some are silent, and some print to the
monitor.

> +
> +int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp)
> +{
> +    char devaddr[32];
> +
> +    if (!get_param_value(devaddr, sizeof(devaddr), "pci_addr", addr)) {
> +        term_printf("Missing pci_addr=\n");
> +        return -1;
> +    }
> +
> +    if (!strcmp(devaddr, "auto")) {
> +        *domp = *busp = 0;
> +        *slotp = -1;
> +        /* want to support dom/bus auto-assign at some point */
> +        return 0;
> +    }
> +
> +    return pci_parse_devaddr(devaddr, domp, busp, slotp);
> +}
> +

Same here.

>  /* -1 for devfn means auto assign */
>  PCIDevice *pci_register_device(PCIBus *bus, const char *name,
>                                 int instance_size, int devfn,
> Index: trunk/hw/pci.h
> ===================================================================
> --- trunk.orig/hw/pci.h
> +++ trunk/hw/pci.h
> @@ -232,6 +232,9 @@ void pci_for_each_device(int bus_num, vo
>  PCIBus *pci_find_bus(int bus_num);
>  PCIDevice *pci_find_device(int bus_num, int slot, int function);
>  
> +int pci_read_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp);
> +int pci_assign_devaddr(const char *addr, int *domp, int *busp, unsigned *slotp);
> +
>  void pci_info(void);
>  PCIBus *pci_bridge_init(PCIBus *bus, int devfn, uint16_t vid, uint16_t did,
>                          pci_map_irq_fn map_irq, const char *name);