| Message ID | 1328201142-26145-11-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 --- */
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
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 >
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
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
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
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
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
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 --- */
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(-)