[Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Anthony Liguori]

Revision: 6609
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6609
Author:   aliguori
Date:     2009-02-11 15:21:48 +0000 (Wed, 11 Feb 2009)

Log Message:
-----------
Parse full PCI device addresses (Markus Armbruster)

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>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/hw/pci.c
    trunk/hw/pci.h

Modified: trunk/hw/pci.c
===================================================================
--- trunk/hw/pci.c	2009-02-11 15:21:41 UTC (rev 6608)
+++ trunk/hw/pci.c	2009-02-11 15:21:48 UTC (rev 6609)
@@ -26,6 +26,7 @@
 #include "console.h"
 #include "net.h"
 #include "virtio-net.h"
+#include "sysemu.h"
 
 //#define DEBUG_PCI
 
@@ -158,6 +159,82 @@
     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)
+	return -1;
+
+    slot = val;
+
+    if (*e)
+	return -1;
+
+    /* Note: QEMU doesn't implement domains other than 0 */
+    if (dom != 0 || pci_find_bus(bus) == NULL)
+	return -1;
+
+    *domp = dom;
+    *busp = bus;
+    *slotp = slot;
+    return 0;
+}
+
+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))
+        return -1;
+
+    return pci_parse_devaddr(devaddr, domp, busp, slotp);
+}
+
+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))
+	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);
+}
+
 /* -1 for devfn means auto assign */
 PCIDevice *pci_register_device(PCIBus *bus, const char *name,
                                int instance_size, int devfn,

Modified: trunk/hw/pci.h
===================================================================
--- trunk/hw/pci.h	2009-02-11 15:21:41 UTC (rev 6608)
+++ trunk/hw/pci.h	2009-02-11 15:21:48 UTC (rev 6609)
@@ -232,6 +232,9 @@
 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);

Re: [Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Paul Brook]

> This code parses full PCI device addresses.  It then rejects domains
> other than zero, because these are not supported in QEMU.
>...
> + * Parse [[<domain>:]<bus>:]<slot>, return -1 on error
> + */
> +static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
> unsigned *slotp) +{

If we're rejecting nonzero domains then having the parse routine return a 
domain value seems wrong. It's just going to make it harder to verify correct 
operation when domains are implemented.

Paul

Re: [Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Marcelo Tosatti]

On Thu, Feb 12, 2009 at 03:39:26PM +0000, Paul Brook wrote:
> > This code parses full PCI device addresses.  It then rejects domains
> > other than zero, because these are not supported in QEMU.
> >...
> > + * Parse [[<domain>:]<bus>:]<slot>, return -1 on error
> > + */
> > +static int pci_parse_devaddr(const char *addr, int *domp, int *busp,
> > unsigned *slotp) +{
> 
> If we're rejecting nonzero domains then having the parse routine return a 
> domain value seems wrong. It's just going to make it harder to verify correct 
> operation when domains are implemented.

+    /* 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;
+    }

+    if (!strcmp(devaddr, "auto")) {
+        *domp = *busp = 0;
+        *slotp = -1;
+        /* want to support dom/bus auto-assign at some point */
+        return 0;
+    }

We return domain 0. I considered domain 0 as implicit at the moment, is
that wrong?

I can't see where you're getting at.

Re: [Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Paul Brook]

> > If we're rejecting nonzero domains then having the parse routine return a
> > domain value seems wrong. It's just going to make it harder to verify
> > correct operation when domains are implemented.
>
> +    /* 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;
> +    }
>
> +    if (!strcmp(devaddr, "auto")) {
> +        *domp = *busp = 0;
> +        *slotp = -1;
> +        /* want to support dom/bus auto-assign at some point */
> +        return 0;
> +    }
>
> We return domain 0. I considered domain 0 as implicit at the moment, is
> that wrong?
>
> I can't see where you're getting at.

You're returning a value that's always known to be zero. IMHO That's worse 
than not returning a value at all.

This implies users of this function will either ignore the value or have 
redundant, untested (i.e. probably bitrotten) code to handle nonzero domains. 
Either way, it's liable the break horribly at runtime as soon as we start 
trying to support multiple domains.

If pci_parse_devaddr is defined to only handle zero domains, then it should 
not be returning a domain value. If/when we implement multiple PCI domains we 
can change the interface, and get nice compiler errors in all the other code 
we forgot to update.

Alternatively, have the parse routine return the full tuple, and enforce 
domain == 0 elsewhere.

Paul

Re: [Qemu-devel] [6609] Parse full PCI device addresses (Markus Armbruster)

[Marcelo Tosatti]

On Thu, Feb 12, 2009 at 04:18:16PM +0000, Paul Brook wrote:
> > > If we're rejecting nonzero domains then having the parse routine return a
> > > domain value seems wrong. It's just going to make it harder to verify
> > > correct operation when domains are implemented.
> >
> > +    /* 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;
> > +    }
> >
> > +    if (!strcmp(devaddr, "auto")) {
> > +        *domp = *busp = 0;
> > +        *slotp = -1;
> > +        /* want to support dom/bus auto-assign at some point */
> > +        return 0;
> > +    }
> >
> > We return domain 0. I considered domain 0 as implicit at the moment, is
> > that wrong?
> >
> > I can't see where you're getting at.
> 
> You're returning a value that's always known to be zero. IMHO That's worse 
> than not returning a value at all.
> 
> This implies users of this function will either ignore the value or have 
> redundant, untested (i.e. probably bitrotten) code to handle nonzero domains. 
> Either way, it's liable the break horribly at runtime as soon as we start 
> trying to support multiple domains.
> 
> If pci_parse_devaddr is defined to only handle zero domains, then it should 
> not be returning a domain value. If/when we implement multiple PCI domains we 
> can change the interface, and get nice compiler errors in all the other code 
> we forgot to update.

That makes sense.

> Alternatively, have the parse routine return the full tuple, and enforce 
> domain == 0 elsewhere.

OK, i'll submit something later in the week so you can ACK/NACK. We want
to pass <dom,bus,slot> all the way down to pci_register_device, and kill
the "-1" parameter that drivers use today (so one can statically assign
bus/slot and eventually domain).

But OK, will drop domain from the API for now.