| Message ID | 1429879153-23476-1-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Peter, On 04/27/2015 06:09 AM, Peter Crosthwaite wrote: > On Fri, Apr 24, 2015 at 5:39 AM, Eric Auger <eric.auger@linaro.org> wrote: >> Add a new irq_routing_notifier notifier in the SysBusDeviceClass. This >> notifier, if populated, is called after sysbus_connect_irq. >> >> This mechanism is used to setup VFIO signaling once VFIO platform >> devices get attached to their platform bus, on a machine init done >> notifier. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> >> v1 -> v2: >> - duly put the notifier in the class and not in the device >> --- >> hw/core/sysbus.c | 6 ++++++ >> include/hw/sysbus.h | 1 + >> 2 files changed, 7 insertions(+) >> >> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c >> index b53c351..8553a6f 100644 >> --- a/hw/core/sysbus.c >> +++ b/hw/core/sysbus.c >> @@ -109,7 +109,13 @@ qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n) >> >> void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq) >> { >> + SysBusDeviceClass *sbd = SYS_BUS_DEVICE_GET_CLASS(dev); >> + >> qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq); > > One of my long term goals is to try and get rid of sysbus IRQ > abstraction completely in favor of just qdev gpios. This means > features that apply to GPIOs automatically apply to IRQs and vice > versa. Can your notifier hook be pushed up to the qdev GPIO level to > make it more globally usable and avoid a new feature to sysbus IRQs? Yes sure, I am going to put the notifier in DeviceClass then. > >> + >> + if (sbd->irq_routing_notifier) { >> + sbd->irq_routing_notifier(dev, irq); >> + } >> } >> >> /* Check whether an MMIO region exists */ >> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h >> index d1f3f00..dbf3f0f 100644 >> --- a/include/hw/sysbus.h >> +++ b/include/hw/sysbus.h >> @@ -41,6 +41,7 @@ typedef struct SysBusDeviceClass { >> /*< public >*/ >> >> int (*init)(SysBusDevice *dev); >> + void (*irq_routing_notifier)(SysBusDevice *dev, qemu_irq irq); > > Is it better to make the name more matched to sysbus_connect_irq? > Perhaps connect_irq_notifier. But with the qdev approach this would be > connect_gpio_out_notifier or something along those lines. OK for that naming Thanks Eric > > Regards, > Peter > >> } SysBusDeviceClass; >> >> struct SysBusDevice { >> -- >> 1.8.3.2 >> >>
On 04/27/2015 12:39 PM, Paolo Bonzini wrote: > > > On 27/04/2015 10:26, Eric Auger wrote: >>>> One of my long term goals is to try and get rid of sysbus IRQ >>>> abstraction completely in favor of just qdev gpios. This means >>>> features that apply to GPIOs automatically apply to IRQs and vice >>>> versa. Can your notifier hook be pushed up to the qdev GPIO level to >>>> make it more globally usable and avoid a new feature to sysbus IRQs? >> Yes sure, I am going to put the notifier in DeviceClass then. > > I've thought too about this, and I'm not sure about it. > > It would mean you have to pass the gpio name (e.g. > SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this > would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of > the hook. Hi Paolo, Currently my notifier has the following proto: void (*connect_gpio_out_notifier)(DeviceState *dev, qemu_irq irq); It is sufficient for my need. is it really mandated to pass other qdev_connect_gpio_out_named args, ie. name & n? Best Regards Eric > > Paolo >
On 04/27/2015 04:39 PM, Peter Crosthwaite wrote: > On Mon, Apr 27, 2015 at 6:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 27/04/2015 14:20, Eric Auger wrote: >>> On 04/27/2015 12:39 PM, Paolo Bonzini wrote: >>>> >>>> >>>> On 27/04/2015 10:26, Eric Auger wrote: >>>>>>> One of my long term goals is to try and get rid of sysbus IRQ >>>>>>> abstraction completely in favor of just qdev gpios. This means >>>>>>> features that apply to GPIOs automatically apply to IRQs and vice >>>>>>> versa. Can your notifier hook be pushed up to the qdev GPIO level to >>>>>>> make it more globally usable and avoid a new feature to sysbus IRQs? >>>>> Yes sure, I am going to put the notifier in DeviceClass then. >>>> >>>> I've thought too about this, and I'm not sure about it. >>>> >>>> It would mean you have to pass the gpio name (e.g. >>>> SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this >>>> would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of >>>> the hook. > > That's OK IMO. SYSBUS_DEVICE_GPIO_IRQ was never intended to be > private. The semantics of it are something like "If you don't have > anything better to name your IRQ pin use this". > > This adds the requirement on machine level code that you can't > consistently use sysbus_connect_irq for intc connection. But machines > should be able to connect any wires between any cores without having > to special case interrupts or chip-selects, resets or whatever. So the > name of the GPIO has to be exposed to the callback hook registration > if we want to break down this GPIO special casing. Names of interrupt > pins are system-level knowledge so I think this is all OK. > >>> Hi Paolo, >>> >>> Currently my notifier has the following proto: >>> void (*connect_gpio_out_notifier)(DeviceState *dev, qemu_irq irq); >>> >>> It is sufficient for my need. >>> >>> is it really mandated to pass other qdev_connect_gpio_out_named args, >>> ie. name & n? >> >> It's an ugly situation. If you look at qdev_connect_gpio_out_named, it >> is really a thin wrapper around object_property_set_link. Just like >> Peter wasn't too happy with changing sysbus_connect_irq, the same >> objection would apply here. Callers of object_property_set_link should >> call the notifiers, not just those that use qdev_connect_gpio_out_named. >> >> This is why I originally asked you to look into using the check callback >> instead. >> > > Is this still feasible? Pushing it up to the higher again to the QOM > level? I think this would be an ideal backend to the problem even if > we still go with a code-friendly sysbus frontend. Peter, Paolo, After your feedbacks, I feel I need to spend some more time on the original check() track. I would prefer not to introduce any patch that will make issue in the future. Thanks Eric > > Regards, > Peter > >> This is why I think it's better to keep the sysbus patch. >> >> Paolo >>
Hi Peter, On 04/27/2015 07:43 PM, Peter Crosthwaite wrote: > On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 27/04/2015 16:56, Eric Auger wrote: >>> Peter, Paolo, >>> >>> After your feedbacks, I feel I need to spend some more time on the >>> original check() track. I would prefer not to introduce any patch that >>> will make issue in the future. >> >> Peter, see the other threads between me and Eric. See in particular >> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html >> starting at "The notifier actually is not even necessary" and the >> replies from there. >> > > Thanks, > > I see the problem with check. In this reply > > http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html > > Eric says that the problem with the check hook is it happens before > the setting. I think this can be solved with a RYO link setter for > GPIOs. We almost have an in-tree precedent with MemoryRegion and the > container property (memory.c): > > 960 op = object_property_add(OBJECT(mr), "container", > 961 "link<" TYPE_MEMORY_REGION ">", > 962 memory_region_get_container, > 963 NULL, /* memory_region_set_container */ > 964 NULL, NULL, &error_abort); > > Now in reality we could have done this link normal style as it is only > a trivial getter, but the reason the link was done this way in the > first place, is because I have a follow up patch to memory.c that adds > a customer Link setter: > > +static void memory_region_set_container(Object *obj, Visitor *v, void *opaque, > + const char *name, Error **errp) > +{ > + MemoryRegion *mr = MEMORY_REGION(obj); > + Error *local_err = NULL; > + MemoryRegion *old_container = mr->container; > + MemoryRegion *new_container = NULL; > + char *path = NULL; > + > + visit_type_str(v, &path, name, &local_err); > + > + if (!local_err && strcmp(path, "") != 0) { > + new_container = MEMORY_REGION(object_resolve_link(obj, name, path, > + &local_err)); > + while (new_container->alias) { > + new_container = new_container->alias; > + } > + } > + > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + object_ref(OBJECT(new_container)); > + > + memory_region_transaction_begin(); > + memory_region_ref(mr); > + if (old_container) { > + memory_region_del_subregion(old_container, mr); > + } > + mr->container = new_container; > + if (new_container) { > + memory_region_update_container_subregions(mr); > + } > + memory_region_unref(mr); > + memory_region_transaction_commit(); > + > + object_unref(OBJECT(old_container)); > +} > + > > op = object_property_add(OBJECT(mr), "container", > "link<" TYPE_MEMORY_REGION ">", > memory_region_get_container, > - NULL, /* memory_region_set_container */ > + memory_region_set_container, > NULL, NULL, &error_abort); > > > The function does the normal link setting - similar to > object_set_link_property (not to be confused with > object_property_set_link!) but is surrounded by class specific side > effects. Specifically in this case, it does > memory_region_transaction_begin/ref/unref/commit etc for the MR. > > For this GPIO case, you could create a custom setter that does the > normal set, then calls the DeviceClass installed hook (or you can > install the hook to the object and init it at qdev_init_gpio_out_named > time as suggested in the eariler thread). The callback will happen > after the link is populated. > > To reduce verbosity, I suggest making object_set_link_property() a > visible API, then RYO link setters can call it surrounded by custom > behavior e.g: > > foo_object_set_bar_property(...) > { > pre_set_link_side_effects(); > object_set_link_property(); > post_set_link_side_effects(); > } > > object_set_link_property() would need to be coreified and wrapped to > remove it's awareness of LinkProperty type (as that doesn't exist in > RYO properties) in this case. Thank you Peter for detailing this. Yesterday I re-worked on the solution based on the check() method where - check would take a Object **child as a 3d parameter - we would assign *child before the call and in case the check fails set the *child back to NULL in object_set_link_property. I need to do some more testing. I don't know if this solution would be acceptable too. If not I will implement according to your guidelines. Best Regards Eric > > Regards, > Peter > >> If you have any idea, please help. >> >> Paolo >>
On 04/28/2015 08:57 AM, Peter Crosthwaite wrote: > On Mon, Apr 27, 2015 at 11:46 PM, Eric Auger <eric.auger@linaro.org> wrote: >> Hi Peter, >> >> On 04/27/2015 07:43 PM, Peter Crosthwaite wrote: >>> On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> >>>> >>>> On 27/04/2015 16:56, Eric Auger wrote: >>>>> Peter, Paolo, >>>>> >>>>> After your feedbacks, I feel I need to spend some more time on the >>>>> original check() track. I would prefer not to introduce any patch that >>>>> will make issue in the future. >>>> >>>> Peter, see the other threads between me and Eric. See in particular >>>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html >>>> starting at "The notifier actually is not even necessary" and the >>>> replies from there. >>>> >>> >>> Thanks, >>> >>> I see the problem with check. In this reply >>> >>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html >>> >>> Eric says that the problem with the check hook is it happens before >>> the setting. I think this can be solved with a RYO link setter for >>> GPIOs. We almost have an in-tree precedent with MemoryRegion and the >>> container property (memory.c): >>> >>> 960 op = object_property_add(OBJECT(mr), "container", >>> 961 "link<" TYPE_MEMORY_REGION ">", >>> 962 memory_region_get_container, >>> 963 NULL, /* memory_region_set_container */ >>> 964 NULL, NULL, &error_abort); >>> >>> Now in reality we could have done this link normal style as it is only >>> a trivial getter, but the reason the link was done this way in the >>> first place, is because I have a follow up patch to memory.c that adds >>> a customer Link setter: >>> >>> +static void memory_region_set_container(Object *obj, Visitor *v, void *opaque, >>> + const char *name, Error **errp) >>> +{ >>> + MemoryRegion *mr = MEMORY_REGION(obj); >>> + Error *local_err = NULL; >>> + MemoryRegion *old_container = mr->container; >>> + MemoryRegion *new_container = NULL; >>> + char *path = NULL; >>> + >>> + visit_type_str(v, &path, name, &local_err); >>> + >>> + if (!local_err && strcmp(path, "") != 0) { >>> + new_container = MEMORY_REGION(object_resolve_link(obj, name, path, >>> + &local_err)); >>> + while (new_container->alias) { >>> + new_container = new_container->alias; >>> + } >>> + } >>> + >>> + if (local_err) { >>> + error_propagate(errp, local_err); >>> + return; >>> + } >>> + >>> + object_ref(OBJECT(new_container)); >>> + >>> + memory_region_transaction_begin(); >>> + memory_region_ref(mr); >>> + if (old_container) { >>> + memory_region_del_subregion(old_container, mr); >>> + } >>> + mr->container = new_container; >>> + if (new_container) { >>> + memory_region_update_container_subregions(mr); >>> + } >>> + memory_region_unref(mr); >>> + memory_region_transaction_commit(); >>> + >>> + object_unref(OBJECT(old_container)); >>> +} >>> + >>> >>> op = object_property_add(OBJECT(mr), "container", >>> "link<" TYPE_MEMORY_REGION ">", >>> memory_region_get_container, >>> - NULL, /* memory_region_set_container */ >>> + memory_region_set_container, >>> NULL, NULL, &error_abort); >>> >>> >>> The function does the normal link setting - similar to >>> object_set_link_property (not to be confused with >>> object_property_set_link!) but is surrounded by class specific side >>> effects. Specifically in this case, it does >>> memory_region_transaction_begin/ref/unref/commit etc for the MR. >>> >>> For this GPIO case, you could create a custom setter that does the >>> normal set, then calls the DeviceClass installed hook (or you can >>> install the hook to the object and init it at qdev_init_gpio_out_named >>> time as suggested in the eariler thread). The callback will happen >>> after the link is populated. >>> >>> To reduce verbosity, I suggest making object_set_link_property() a >>> visible API, then RYO link setters can call it surrounded by custom >>> behavior e.g: >>> >>> foo_object_set_bar_property(...) >>> { >>> pre_set_link_side_effects(); >>> object_set_link_property(); >>> post_set_link_side_effects(); >>> } >>> >>> object_set_link_property() would need to be coreified and wrapped to >>> remove it's awareness of LinkProperty type (as that doesn't exist in >>> RYO properties) in this case. >> >> Thank you Peter for detailing this. >> >> Yesterday I re-worked on the solution based on the check() method where >> - check would take a Object **child as a 3d parameter >> - we would assign *child before the call and in case the check fails set >> the *child back to NULL in object_set_link_property. >> > > I think this is starting to reach outside the design intent of the > check, letting it be an API that takes over the responsibility of > ->set. Ideally check should be boolean and side-effectless. I acknowledge ;-) > >> I need to do some more testing. >> >> I don't know if this solution would be acceptable too. If not I will >> implement according to your guidelines. >> > > Lets see what Paolo says before another rework. sure Thanks Eric > > Regards, > Peter > >> Best Regards >> >> Eric >>> >>> Regards, >>> Peter >>> >>>> If you have any idea, please help. >>>> >>>> Paolo >>>> >> >>
diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c index b53c351..8553a6f 100644 --- a/hw/core/sysbus.c +++ b/hw/core/sysbus.c @@ -109,7 +109,13 @@ qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n) void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq) { + SysBusDeviceClass *sbd = SYS_BUS_DEVICE_GET_CLASS(dev); + qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq); + + if (sbd->irq_routing_notifier) { + sbd->irq_routing_notifier(dev, irq); + } } /* Check whether an MMIO region exists */ diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h index d1f3f00..dbf3f0f 100644 --- a/include/hw/sysbus.h +++ b/include/hw/sysbus.h @@ -41,6 +41,7 @@ typedef struct SysBusDeviceClass { /*< public >*/ int (*init)(SysBusDevice *dev); + void (*irq_routing_notifier)(SysBusDevice *dev, qemu_irq irq); } SysBusDeviceClass; struct SysBusDevice {
Add a new irq_routing_notifier notifier in the SysBusDeviceClass. This notifier, if populated, is called after sysbus_connect_irq. This mechanism is used to setup VFIO signaling once VFIO platform devices get attached to their platform bus, on a machine init done notifier. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- v1 -> v2: - duly put the notifier in the class and not in the device --- hw/core/sysbus.c | 6 ++++++ include/hw/sysbus.h | 1 + 2 files changed, 7 insertions(+)