| Message ID | 1429879153-23476-1-git-send-email-eric.auger@linaro.org |
|---|---|
| State | New |
| Headers | show |
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? > + > + 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. Regards, Peter > } SysBusDeviceClass; > > struct SysBusDevice { > -- > 1.8.3.2 > >
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 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. Paolo
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 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. > 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. This is why I think it's better to keep the sysbus patch. Paolo
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. Regards, Peter > This is why I think it's better to keep the sysbus patch. > > 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 >>
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. If you have any idea, please help. Paolo
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. Regards, Peter > If you have any idea, please help. > > 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 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 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. Regards, Peter > 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 >>>> >> >>
On 27/04/2015 19:43, Peter Crosthwaite wrote: > 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. I agree with this, but I think it can be done separately from Eric's work. 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(+)