diff mbox

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

Message ID 4FD9F789.5070300@citrix.com
State New
Headers show

Commit Message

Anthony PERARD June 14, 2012, 2:39 p.m. UTC 
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.

Comments

Jan Kiszka June 14, 2012, 3:21 p.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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,
diff mbox

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);