Patchwork [V12,5/9] Revert "pci: don't export an internal function"

login
register
mail settings
Submitter Anthony PERARD
Date June 14, 2012, 2:39 p.m.
Message ID <4FD9F789.5070300@citrix.com>
Download mbox | patch
Permalink /patch/164944/
State New
Headers show

Comments

Anthony PERARD - June 14, 2012, 2:39 p.m.
On 13/06/12 13:48, Jan Kiszka wrote:
>>> >>  Will you be able to use an address parser via some device property?
>> >
>> >  Yes. Actually, the address is already a device property.
>> >
> Great. Then we should try to come up with a common pci-host-devaddr
> property everyone can use.

Is this patch would be a good common property?

It's probably close to the patch you send earlier Jan.

If it's good, I'll resend my passthrough patch series with this patch.
Jan Kiszka - June 14, 2012, 3:21 p.m.
On 2012-06-14 16:39, Anthony PERARD wrote:
> On 13/06/12 13:48, Jan Kiszka wrote:
>>>>>>  Will you be able to use an address parser via some device property?
>>>>
>>>>  Yes. Actually, the address is already a device property.
>>>>
>> Great. Then we should try to come up with a common pci-host-devaddr
>> property everyone can use.
> 
> Is this patch would be a good common property?
> 
> It's probably close to the patch you send earlier Jan.
> 
> If it's good, I'll resend my passthrough patch series with this patch.

Looks generally OK to me, let's try to move forward with this. Please
fix the coding style issues before sending.

> 
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index b7b5597..9c7a271 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -928,6 +928,104 @@ PropertyInfo qdev_prop_blocksize = {
>       .max   = 65024,
>   };
> 
> +/* --- pci host address --- */
> +
> +static void get_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> +    char buffer[4 + 2 + 2 + 2 + 1];

Buffer is too small.

> +    char *p = buffer;
> +
> +    snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%d",
> +             addr->domain, addr->bus, addr->slot, addr->function);
> +
> +    visit_type_str(v, &p, name, errp);
> +}
> +
> +/*
> + * Parse [<domain>:]<bus>:<slot>.<func>
> + *   if <domain> is not supplied, it's assumed to be 0.
> + */
> +static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    char *str, *p;
> +    char *e;
> +    unsigned long val;
> +    unsigned long dom = 0, bus = 0;
> +    unsigned int slot = 0, func = 0;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_str(v, &str, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    p = str;
> +    val = strtoul(p, &e, 16);
> +    if (e == p || *e != ':')
> +        goto inval;
> +    bus = val;
> +
> +    p = e + 1;
> +    val = strtoul(p, &e, 16);
> +    if (e == p)
> +        goto inval;
> +    if (*e == ':') {
> +        dom = bus;
> +        bus = val;
> +        p = e + 1;
> +        val = strtoul(p, &e, 16);
> +        if (e == p)
> +            goto inval;
> +    }
> +    slot = val;
> +
> +    if (*e != '.')
> +        goto inval;
> +    p = e + 1;
> +    val = strtoul(p, &e, 10);
> +    if (e == p)
> +        goto inval;
> +    func = val;
> +
> +    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
> +        goto inval;
> +
> +    if (*e)
> +        goto inval;
> +
> +    addr->domain = dom;
> +    addr->bus = bus;
> +    addr->slot = slot;
> +    addr->function = func;
> +
> +    g_free(str);
> +    return;
> +
> +inval:
> +    error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> +    g_free(str);
> +}
> +
> +PropertyInfo qdev_prop_pci_host_devaddr = {
> +    .name = "pci-host-devaddr",
> +    .get = get_pci_host_devaddr,
> +    .set = set_pci_host_devaddr,
> +};
> +
>   /* --- public helpers --- */
> 
>   static Property *qdev_prop_walk(Property *props, const char *name)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 4e90119..9a54ebe 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -225,6 +225,7 @@ extern PropertyInfo qdev_prop_netdev;
>   extern PropertyInfo qdev_prop_vlan;
>   extern PropertyInfo qdev_prop_pci_devfn;
>   extern PropertyInfo qdev_prop_blocksize;
> +extern PropertyInfo qdev_prop_pci_host_devaddr;
> 
>   #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>           .name      = (_name),                                    \
> @@ -288,6 +289,8 @@ extern PropertyInfo qdev_prop_blocksize;
>                           LostTickPolicy)
>   #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
>       DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
> +#define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, 
> PCIHostDeviceAddress)
> 
>   #define DEFINE_PROP_END_OF_LIST()               \
>       {}
> diff --git a/qemu-common.h b/qemu-common.h
> index 91e0562..0d6e51c 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -274,6 +274,13 @@ typedef enum LostTickPolicy {
>       LOST_TICK_MAX
>   } LostTickPolicy;
> 
> +typedef struct PCIHostDeviceAddress {
> +    unsigned int domain;
> +    unsigned int bus;
> +    unsigned int slot;
> +    unsigned int function;
> +} PCIHostDeviceAddress;
> +
>   void tcg_exec_init(unsigned long tb_size);
>   bool tcg_enabled(void);
> 
> 

Thanks,
Jan
Michael S. Tsirkin - June 14, 2012, 3:30 p.m.
On Thu, Jun 14, 2012 at 03:39:05PM +0100, Anthony PERARD wrote:
> On 13/06/12 13:48, Jan Kiszka wrote:
> >>>>>  Will you be able to use an address parser via some device property?
> >>>
> >>>  Yes. Actually, the address is already a device property.
> >>>
> >Great. Then we should try to come up with a common pci-host-devaddr
> >property everyone can use.
> 
> Is this patch would be a good common property?
> 
> It's probably close to the patch you send earlier Jan.
> 
> If it's good, I'll resend my passthrough patch series with this patch.
> 
> 
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index b7b5597..9c7a271 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -928,6 +928,104 @@ PropertyInfo qdev_prop_blocksize = {
>      .max   = 65024,
>  };
> 
> +/* --- pci host address --- */
> +
> +static void get_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> +    char buffer[4 + 2 + 2 + 2 + 1];

One trick is this:
    char buffer[] = "XXXX:XX:XX.X";

And comparing with the above, your buffer looks too short, doesn't it?


> +    char *p = buffer;
> +
> +    snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%d",
> +             addr->domain, addr->bus, addr->slot, addr->function);

A good idea might be to check the return code. assert(ret <= sizeof buf).

> +
> +    visit_type_str(v, &p, name, errp);
> +}
> +
> +/*
> + * Parse [<domain>:]<bus>:<slot>.<func>
> + *   if <domain> is not supplied, it's assumed to be 0.
> + */
> +static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
> +                                 const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    char *str, *p;
> +    char *e;
> +    unsigned long val;
> +    unsigned long dom = 0, bus = 0;
> +    unsigned int slot = 0, func = 0;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_str(v, &str, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    p = str;
> +    val = strtoul(p, &e, 16);
> +    if (e == p || *e != ':')
> +        goto inval;
> +    bus = val;
> +
> +    p = e + 1;
> +    val = strtoul(p, &e, 16);
> +    if (e == p)
> +        goto inval;
> +    if (*e == ':') {
> +        dom = bus;
> +        bus = val;
> +        p = e + 1;
> +        val = strtoul(p, &e, 16);
> +        if (e == p)
> +            goto inval;
> +    }
> +    slot = val;
> +
> +    if (*e != '.')
> +        goto inval;
> +    p = e + 1;
> +    val = strtoul(p, &e, 10);
> +    if (e == p)
> +        goto inval;
> +    func = val;
> +
> +    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
> +        goto inval;
> +
> +    if (*e)
> +        goto inval;
> +
> +    addr->domain = dom;
> +    addr->bus = bus;
> +    addr->slot = slot;
> +    addr->function = func;
> +
> +    g_free(str);
> +    return;
> +
> +inval:
> +    error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
> +    g_free(str);
> +}
> +
> +PropertyInfo qdev_prop_pci_host_devaddr = {
> +    .name = "pci-host-devaddr",
> +    .get = get_pci_host_devaddr,
> +    .set = set_pci_host_devaddr,
> +};
> +
>  /* --- public helpers --- */
> 
>  static Property *qdev_prop_walk(Property *props, const char *name)
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 4e90119..9a54ebe 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -225,6 +225,7 @@ extern PropertyInfo qdev_prop_netdev;
>  extern PropertyInfo qdev_prop_vlan;
>  extern PropertyInfo qdev_prop_pci_devfn;
>  extern PropertyInfo qdev_prop_blocksize;
> +extern PropertyInfo qdev_prop_pci_host_devaddr;
> 
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>          .name      = (_name),                                    \
> @@ -288,6 +289,8 @@ extern PropertyInfo qdev_prop_blocksize;
>                          LostTickPolicy)
>  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
>      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
> +#define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
> +    DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr,
> PCIHostDeviceAddress)
> 
>  #define DEFINE_PROP_END_OF_LIST()               \
>      {}
> diff --git a/qemu-common.h b/qemu-common.h
> index 91e0562..0d6e51c 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -274,6 +274,13 @@ typedef enum LostTickPolicy {
>      LOST_TICK_MAX
>  } LostTickPolicy;
> 
> +typedef struct PCIHostDeviceAddress {
> +    unsigned int domain;
> +    unsigned int bus;
> +    unsigned int slot;
> +    unsigned int function;
> +} PCIHostDeviceAddress;
> +
>  void tcg_exec_init(unsigned long tb_size);
>  bool tcg_enabled(void);
> 
> 
> -- 
> Anthony PERARD
Anthony PERARD - June 14, 2012, 3:59 p.m.
On 14/06/12 16:30, Michael S. Tsirkin wrote:
>> >  Is this patch would be a good common property?
>> >
>> >  It's probably close to the patch you send earlier Jan.
>> >
>> >  If it's good, I'll resend my passthrough patch series with this patch.
>> >
>> >
>> >  diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> >  index b7b5597..9c7a271 100644
>> >  --- a/hw/qdev-properties.c
>> >  +++ b/hw/qdev-properties.c
>> >  @@ -928,6 +928,104 @@ PropertyInfo qdev_prop_blocksize = {
>> >        .max   = 65024,
>> >    };
>> >
>> >  +/* --- pci host address --- */
>> >  +
>> >  +static void get_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
>> >  +                                 const char *name, Error **errp)
>> >  +{
>> >  +    DeviceState *dev = DEVICE(obj);
>> >  +    Property *prop = opaque;
>> >  +    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
>> >  +    char buffer[4 + 2 + 2 + 2 + 1];
> One trick is this:
>      char buffer[] = "XXXX:XX:XX.X";

I love this trick, even if it use a bit of space in the binary, I think. 
I will use it.

> And comparing with the above, your buffer looks too short, doesn't it?

Yes, I think I forgot to count the ':'s and '.'.

>> >  +    char *p = buffer;
>> >  +
>> >  +    snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%d",
>> >  +             addr->domain, addr->bus, addr->slot, addr->function);
> A good idea might be to check the return code. assert(ret<= sizeof buf).

I will.

Thanks,
Anthony PERARD - June 14, 2012, 4:01 p.m.
On 14/06/12 16:21, Jan Kiszka wrote:
>> Is this patch would be a good common property?
>>
>> It's probably close to the patch you send earlier Jan.
>>
>> If it's good, I'll resend my passthrough patch series with this patch.
> Looks generally OK to me, let's try to move forward with this. Please
> fix the coding style issues before sending.

I fix the patch a resend the series.

Thanks,

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index b7b5597..9c7a271 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -928,6 +928,104 @@  PropertyInfo qdev_prop_blocksize = {
      .max   = 65024,
  };

+/* --- pci host address --- */
+
+static void get_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
+    char buffer[4 + 2 + 2 + 2 + 1];
+    char *p = buffer;
+
+    snprintf(buffer, sizeof(buffer), "%04x:%02x:%02x.%d",
+             addr->domain, addr->bus, addr->slot, addr->function);
+
+    visit_type_str(v, &p, name, errp);
+}
+
+/*
+ * Parse [<domain>:]<bus>:<slot>.<func>
+ *   if <domain> is not supplied, it's assumed to be 0.
+ */
+static void set_pci_host_devaddr(Object *obj, Visitor *v, void *opaque,
+                                 const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    PCIHostDeviceAddress *addr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    char *str, *p;
+    char *e;
+    unsigned long val;
+    unsigned long dom = 0, bus = 0;
+    unsigned int slot = 0, func = 0;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_str(v, &str, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    p = str;
+    val = strtoul(p, &e, 16);
+    if (e == p || *e != ':')
+        goto inval;
+    bus = val;
+
+    p = e + 1;
+    val = strtoul(p, &e, 16);
+    if (e == p)
+        goto inval;
+    if (*e == ':') {
+        dom = bus;
+        bus = val;
+        p = e + 1;
+        val = strtoul(p, &e, 16);
+        if (e == p)
+            goto inval;
+    }
+    slot = val;
+
+    if (*e != '.')
+        goto inval;
+    p = e + 1;
+    val = strtoul(p, &e, 10);
+    if (e == p)
+        goto inval;
+    func = val;
+
+    if (dom > 0xffff || bus > 0xff || slot > 0x1f || func > 7)
+        goto inval;
+
+    if (*e)
+        goto inval;
+
+    addr->domain = dom;
+    addr->bus = bus;
+    addr->slot = slot;
+    addr->function = func;
+
+    g_free(str);
+    return;
+
+inval:
+    error_set_from_qdev_prop_error(errp, EINVAL, dev, prop, str);
+    g_free(str);
+}
+
+PropertyInfo qdev_prop_pci_host_devaddr = {
+    .name = "pci-host-devaddr",
+    .get = get_pci_host_devaddr,
+    .set = set_pci_host_devaddr,
+};
+
  /* --- public helpers --- */

  static Property *qdev_prop_walk(Property *props, const char *name)
diff --git a/hw/qdev.h b/hw/qdev.h
index 4e90119..9a54ebe 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -225,6 +225,7 @@  extern PropertyInfo qdev_prop_netdev;
  extern PropertyInfo qdev_prop_vlan;
  extern PropertyInfo qdev_prop_pci_devfn;
  extern PropertyInfo qdev_prop_blocksize;
+extern PropertyInfo qdev_prop_pci_host_devaddr;

  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
          .name      = (_name),                                    \
@@ -288,6 +289,8 @@  extern PropertyInfo qdev_prop_blocksize;
                          LostTickPolicy)
  #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f, _d) \
      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blocksize, uint16_t)
+#define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, 
PCIHostDeviceAddress)

  #define DEFINE_PROP_END_OF_LIST()               \
      {}
diff --git a/qemu-common.h b/qemu-common.h
index 91e0562..0d6e51c 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -274,6 +274,13 @@  typedef enum LostTickPolicy {
      LOST_TICK_MAX
  } LostTickPolicy;

+typedef struct PCIHostDeviceAddress {
+    unsigned int domain;
+    unsigned int bus;
+    unsigned int slot;
+    unsigned int function;
+} PCIHostDeviceAddress;
+
  void tcg_exec_init(unsigned long tb_size);
  bool tcg_enabled(void);