Patchwork [10/16] qdev: make the non-legacy pci address property accept an integer

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 2, 2012, 4:45 p.m.
Message ID <1328201142-26145-11-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/139177/
State New
Headers show

Comments

Paolo Bonzini - Feb. 2, 2012, 4:45 p.m.
PCI addresses are set with qdev_prop_uint32.  Thus we make the QOM
property accept a device and function encoded in an 8-bit integer,
instead of the magic dd.f hex string.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c |   25 +++++++------------------
 1 files changed, 7 insertions(+), 18 deletions(-)
Anthony Liguori - Feb. 2, 2012, 8:07 p.m.
On 02/02/2012 10:45 AM, Paolo Bonzini wrote:
> PCI addresses are set with qdev_prop_uint32.  Thus we make the QOM
> property accept a device and function encoded in an 8-bit integer,
> instead of the magic dd.f hex string.
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>

Isn't this a compatibility breaker?

Won't this break libvirt's usage of -device addr=0.2 ?

Regards,

Anthony Liguori

> ---
>   hw/qdev-properties.c |   25 +++++++------------------
>   1 files changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 4fb5cf8..e4bcc6d 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -950,30 +950,19 @@ static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
>       }
>   }
>
> -static void get_pci_devfn(Object *obj, Visitor *v, void *opaque,
> -                          const char *name, Error **errp)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    Property *prop = opaque;
> -    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> -    char buffer[32];
> -    char *p = buffer;
> -
> -    buffer[0] = 0;
> -    if (*ptr != -1) {
> -        snprintf(buffer, sizeof(buffer), "%02x.%x", *ptr>>  3, *ptr&  7);
> -    }
> -    visit_type_str(v,&p, name, errp);
> -}
> -
>   PropertyInfo qdev_prop_pci_devfn = {
>       .name  = "pci-devfn",
>       .type  = PROP_TYPE_UINT32,
>       .size  = sizeof(uint32_t),
>       .parse = parse_pci_devfn,
>       .print = print_pci_devfn,
> -    .get   = get_pci_devfn,
> -    .set   = set_generic,
> +    .get   = get_int32,
> +    .set   = set_int32,
> +    /* FIXME: this should be -1...255, but the address is stored
> +     * into an uint32_t rather than int32_t.
> +     */
> +    .min   = 0,
> +    .max   = 0xFFFFFFFFULL,
>   };
>
>   /* --- public helpers --- */
Paolo Bonzini - Feb. 2, 2012, 8:19 p.m.
On 02/02/2012 09:07 PM, Anthony Liguori wrote:
>>
>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>
> Isn't this a compatibility breaker?
>
> Won't this break libvirt's usage of -device addr=0.2 ?

Nope, the legacy property still keeps the dd.f format.  This is only for 
QOM (and internal use by qdev).

Paolo
Anthony Liguori - Feb. 3, 2012, 2:14 p.m.
On 02/02/2012 02:19 PM, Paolo Bonzini wrote:
> On 02/02/2012 09:07 PM, Anthony Liguori wrote:
>>>
>>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>>
>> Isn't this a compatibility breaker?
>>
>> Won't this break libvirt's usage of -device addr=0.2 ?
>
> Nope, the legacy property still keeps the dd.f format. This is only for QOM (and
> internal use by qdev).

Ah, I forgot we duplicate the properties here.

Since there is now a programmatic mapping between legacy properties types and 
non-legacy property types, could we remove the legacy properties that now have 
well behaved types and add some code to device_add to maintain compatibility?

Regards,

Anthony Liguori

> Paolo
>
Paolo Bonzini - Feb. 4, 2012, 12:21 a.m.
On 02/03/2012 03:14 PM, Anthony Liguori wrote:
>>>
>>
>> Nope, the legacy property still keeps the dd.f format. This is only
>> for QOM (and
>> internal use by qdev).
>
> Ah, I forgot we duplicate the properties here.
>
> Since there is now a programmatic mapping between legacy properties
> types and non-legacy property types, could we remove the legacy
> properties that now have well behaved types and add some code to
> device_add to maintain compatibility?

I'm not sure... we would trade removal of an ugly concept (the legacy 
properties) with addition of a layering violation (poking into the 
DeviceState subclasses).

We could hide them in qom-list/qom-get, but I'm not sure this would buy 
us anything, either.

Paolo
Paolo Bonzini - Feb. 4, 2012, 12:43 a.m.
On 02/04/2012 01:21 AM, Paolo Bonzini wrote:
> I'm not sure... we would trade removal of an ugly concept (the legacy
> properties) with addition of a layering violation (poking into the
> DeviceState subclasses).

The main problem here is that you said no to a hierarchy of property 
classes.  This is what would be good here: being able to say "does this 
property have legacy print/parse methods?" and call them if available 
from device_add.

So, you can choose your poison. :)  For now I think the idea in this 
patch series is good enough for its purpose (which is to actually _use_ 
QOM), we can tweak the design and really eliminate the legacy properties 
later.  I don't mind going through multiple iterations as long as the 
state after each iteration is clearly better than before.
f
Paolo
Anthony Liguori - Feb. 4, 2012, 3 a.m.
On 02/03/2012 06:43 PM, Paolo Bonzini wrote:
> On 02/04/2012 01:21 AM, Paolo Bonzini wrote:
>> I'm not sure... we would trade removal of an ugly concept (the legacy
>> properties) with addition of a layering violation (poking into the
>> DeviceState subclasses).
>
> The main problem here is that you said no to a hierarchy of property classes.
> This is what would be good here: being able to say "does this property have
> legacy print/parse methods?" and call them if available from device_add.
>
> So, you can choose your poison. :) For now I think the idea in this patch series
> is good enough for its purpose (which is to actually _use_ QOM),

Yeah, I was just thinking out loud.  My plan is to pull your series into my 
qom-rebase branch.

The last few commits on https://github.com/aliguori/qemu/tree/qom-rebase.12 have 
a variant visitor and accessors that use it.

Regards,

Anthony Liguori

> we can tweak
> the design and really eliminate the legacy properties later. I don't mind going
> through multiple iterations as long as the state after each iteration is clearly
> better than before.
> f
> Paolo
Paolo Bonzini - Feb. 4, 2012, 6:42 a.m.
On 02/04/2012 04:00 AM, Anthony Liguori wrote:
>>
>
> Yeah, I was just thinking out loud.  My plan is to pull your series into
> my qom-rebase branch.
>
> The last few commits on
> https://github.com/aliguori/qemu/tree/qom-rebase.12 have a variant
> visitor and accessors that use it.

Ok, I'll cherry-pick from there and send out an updated patch series 
later today.  I have some more bugfixes and documentation improvements.

I started looking at PROP_PTR.  It's divided in 4:

- things that do not need PROP_PTR (smbus_eeprom, vmmouse);

- OMAP, which can be done now;

- things that need your 4/4 to set up links to devices;

- things that need CPU objects;

Paolo
Paolo Bonzini - Feb. 4, 2012, 7:13 a.m.
On 02/04/2012 07:42 AM, Paolo Bonzini wrote:
> Ok, I'll cherry-pick from there and send out an updated patch series
> later today.

... that won't work unfortunately.  Enums are broken (not your fault 
really, I sent this before losttickpolicy went in); I really dislike 
having a single object that can magically act both as input and output 
visitor; and there is no documentation in the header for the new functions.

I'll stick with the QObject version for now, but move it to a separate file.

Paolo

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 4fb5cf8..e4bcc6d 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -950,30 +950,19 @@  static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, size_t
     }
 }
 
-static void get_pci_devfn(Object *obj, Visitor *v, void *opaque,
-                          const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
-    char buffer[32];
-    char *p = buffer;
-
-    buffer[0] = 0;
-    if (*ptr != -1) {
-        snprintf(buffer, sizeof(buffer), "%02x.%x", *ptr >> 3, *ptr & 7);
-    }
-    visit_type_str(v, &p, name, errp);
-}
-
 PropertyInfo qdev_prop_pci_devfn = {
     .name  = "pci-devfn",
     .type  = PROP_TYPE_UINT32,
     .size  = sizeof(uint32_t),
     .parse = parse_pci_devfn,
     .print = print_pci_devfn,
-    .get   = get_pci_devfn,
-    .set   = set_generic,
+    .get   = get_int32,
+    .set   = set_int32,
+    /* FIXME: this should be -1...255, but the address is stored
+     * into an uint32_t rather than int32_t.
+     */
+    .min   = 0,
+    .max   = 0xFFFFFFFFULL,
 };
 
 /* --- public helpers --- */