| Message ID | 0a460e01cca4fa24f446c7a715fe6df17d0be9ed.1283152674.git.yamahata@valinux.co.jp |
|---|---|
| State | New |
| Headers | show |
On 08/30/2010 09:49 AM, Isaku Yamahata wrote: > +/* those two functions are obsoleted by cold/warm reset API. */ > [qemu_register_reset/qemu_unregister_reset] Are they? They have a _lot_ of callers and most of the time you do not really care about cold vs. warm reset. So, I think either you define a new API where you can request cold reset/warm reset/both, or qemu_register_reset is here to stay forever. In general, I don't like the duplication you introduce between cold reset, warm reset, shutdown, powerdown, etc. Maybe you can introduce a new "VMEvent" abstraction with functions like "request", "is requested", "register handler"? It could also be interesting to convert everything to the Notifier API, if someone wants to play with Coccinelle... Paolo
On 08/30/2010 11:38 AM, Isaku Yamahata wrote: > Sounds good idea. I'll give it a try. > Before touching the code, I'd like to split out those related functions > and main_loop() from vl.c into a new file, main-loop.c or something like that. > Any objection for splitting out vl.c for that? That would be good. Most if not all of cpus.c could go in the new file too. Thanks! Paolo
Thank you for comments. On Mon, Aug 30, 2010 at 10:50:47AM +0200, Paolo Bonzini wrote: > On 08/30/2010 09:49 AM, Isaku Yamahata wrote: >> +/* those two functions are obsoleted by cold/warm reset API. */ >> [qemu_register_reset/qemu_unregister_reset] > > Are they? > > They have a _lot_ of callers and most of the time you do not really care > about cold vs. warm reset. So, I think either you define a new API > where you can request cold reset/warm reset/both, or qemu_register_reset > is here to stay forever. Then, let's keep qemu_register_reset() as for both cold/warm with documentation/comments. > In general, I don't like the duplication you introduce between cold > reset, warm reset, shutdown, powerdown, etc. Maybe you can introduce a > new "VMEvent" abstraction with functions like "request", "is requested", > "register handler"? Sounds good idea. I'll give it a try. Before touching the code, I'd like to split out those related functions and main_loop() from vl.c into a new file, main-loop.c or something like that. Any objection for splitting out vl.c for that? > It could also be interesting to convert everything to the Notifier API, > if someone wants to play with Coccinelle... > > Paolo >
On 08/30/2010 02:49 AM, Isaku Yamahata wrote: > Distinguish warm reset from cold reset by introducing > cold/warm reset helper function instead of single reset routines. > > Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> > --- > hw/hw.h | 7 +++++ > sysemu.h | 4 +++ > vl.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++---------- > 3 files changed, 85 insertions(+), 15 deletions(-) > > diff --git a/hw/hw.h b/hw/hw.h > index 4405092..6fb844e 100644 > --- a/hw/hw.h > +++ b/hw/hw.h > @@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr, > > typedef void QEMUResetHandler(void *opaque); > > + > +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque); > +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque); > +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque); > +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque); > I was thinking that we should stick entirely within the qdev abstraction. The patchset I sent out introduced a cold reset as a qdev property on the devices. For warm reset, if I understand correctly, we need two things. We need to 1) control propagation order and we need to 2) differentiate per-device between cold reset and warm reset. For (2), I don't know that we truly do need it. For something like PCI AER, wouldn't we just move the AER initialization to the qdev init function and then never change the AER registers during reset? IOW, the only way to do a cold reset would be to destroy and recreate the device. For (1), I still don't fully understand what's needed here. Do we just care about doing a certain transversal order (like depth-first) or do we actually care about withholding the reset signal for devices if as Avi said, we SCSI devices don't get reset. Changing transversal order is trivial. I think we should be very clear though if we're going to introduce bus-specific mechanisms to modify transversal though. Regards, Anthony Liguori > +/* those two functions are obsoleted by cold/warm reset API. */ > void qemu_register_reset(QEMUResetHandler *func, void *opaque); > void qemu_unregister_reset(QEMUResetHandler *func, void *opaque); > > diff --git a/sysemu.h b/sysemu.h > index 7fc4e20..927892a 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void); > void cpu_enable_ticks(void); > void cpu_disable_ticks(void); > > +/* transitional compat = qemu_system_warm_reset_request() */ > void qemu_system_reset_request(void); > + > +void qemu_system_cold_reset_request(void); > +void qemu_system_warm_reset_request(void); > void qemu_system_shutdown_request(void); > void qemu_system_powerdown_request(void); > extern qemu_irq qemu_system_powerdown; > diff --git a/vl.c b/vl.c > index a919a32..609d74c 100644 > --- a/vl.c > +++ b/vl.c > @@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry { > } QEMUResetEntry; > > QTAILQ_HEAD(reset_handlers, QEMUResetEntry); > -static struct reset_handlers reset_handlers = > - QTAILQ_HEAD_INITIALIZER(reset_handlers); > -static int reset_requested; > + > +static struct reset_handlers cold_reset_handlers = > + QTAILQ_HEAD_INITIALIZER(cold_reset_handlers); > +static struct reset_handlers warm_reset_handlers = > + QTAILQ_HEAD_INITIALIZER(warm_reset_handlers); > +static int cold_reset_requested; > +static int warm_reset_requested; > static int shutdown_requested; > static int powerdown_requested; > int debug_requested; > @@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void) > return qemu_requested(&shutdown_requested); > } > > -static int qemu_reset_requested(void) > +static int qemu_cold_reset_requested(void) > +{ > + return qemu_requested(&cold_reset_requested); > +} > + > +static int qemu_warm_reset_requested(void) > { > - return qemu_requested(&reset_requested); > + return qemu_requested(&warm_reset_requested); > } > > static int qemu_powerdown_requested(void) > @@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers) > } > } > > +/* obsolated by qemu_register_cold/warm_reset() */ > void qemu_register_reset(QEMUResetHandler *func, void *opaque) > { > - qemu_register_reset_handler(func, opaque,&reset_handlers); > + qemu_register_cold_reset(func, opaque); > + qemu_register_warm_reset(func, opaque); > } > > +/* obsolated by qemu_unregister_cold/warm_reset() */ > void qemu_unregister_reset(QEMUResetHandler *func, void *opaque) > { > - qemu_unregister_reset_handler(func, opaque,&reset_handlers); > + qemu_unregister_cold_reset(func, opaque); > + qemu_unregister_warm_reset(func, opaque); > +} > + > +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque) > +{ > + qemu_register_reset_handler(func, opaque,&cold_reset_handlers); > +} > + > +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque) > +{ > + qemu_unregister_reset_handler(func, opaque,&cold_reset_handlers); > +} > + > +static void qemu_system_cold_reset(void) > +{ > + qemu_system_reset_handler(&cold_reset_handlers); > + monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */ > + cpu_synchronize_all_post_reset(); > +} > + > +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque) > +{ > + qemu_register_reset_handler(func, opaque,&warm_reset_handlers); > +} > + > +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque) > +{ > + qemu_unregister_reset_handler(func, opaque,&warm_reset_handlers); > } > > -static void qemu_system_reset(void) > +static void qemu_system_warm_reset(void) > { > - qemu_system_reset_handler(&reset_handlers); > - monitor_protocol_event(QEVENT_RESET, NULL); > + qemu_system_reset_handler(&warm_reset_handlers); > + monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */ > cpu_synchronize_all_post_reset(); > } > > @@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested) > qemu_system_request(requested); > } > > +void qemu_system_cold_reset_request(void) > +{ > + qemu_system_request_reboot_check(&cold_reset_requested); > +} > + > +void qemu_system_warm_reset_request(void) > +{ > + qemu_system_request_reboot_check(&warm_reset_requested); > +} > + > +/* trantitional compat */ > void qemu_system_reset_request(void) > { > - qemu_system_request_reboot_check(&reset_requested); > + qemu_system_request_reboot_check(&warm_reset_requested); > } > > void qemu_system_shutdown_request(void) > @@ -1322,7 +1373,9 @@ static int vm_can_run(void) > { > if (powerdown_requested) > return 0; > - if (reset_requested) > + if (cold_reset_requested) > + return 0; > + if (warm_reset_requested) > return 0; > if (shutdown_requested) > return 0; > @@ -1368,9 +1421,15 @@ static void main_loop(void) > } else > break; > } > - if (qemu_reset_requested()) { > + if (qemu_warm_reset_requested()) { > + pause_all_vcpus(); > + qemu_system_warm_reset(); > + resume_all_vcpus(); > + } > + if (qemu_cold_reset_requested()) { > + /* power cycle */ > pause_all_vcpus(); > - qemu_system_reset(); > + qemu_system_cold_reset(); > resume_all_vcpus(); > } > if (qemu_powerdown_requested()) { > @@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp) > exit(1); > } > > - qemu_system_reset(); > + qemu_system_cold_reset(); > if (loadvm) { > if (load_vmstate(loadvm)< 0) { > autostart = 0; >
On 08/30/2010 03:50 AM, Paolo Bonzini wrote: > On 08/30/2010 09:49 AM, Isaku Yamahata wrote: >> +/* those two functions are obsoleted by cold/warm reset API. */ >> [qemu_register_reset/qemu_unregister_reset] > > Are they? Yes, but introduce more reset functions isn't the right approach. Reset should be a method of the device tree, not a stand alone function. Regards, Anthony Liguori > > They have a _lot_ of callers and most of the time you do not really > care about cold vs. warm reset. So, I think either you define a new > API where you can request cold reset/warm reset/both, or > qemu_register_reset is here to stay forever. > > In general, I don't like the duplication you introduce between cold > reset, warm reset, shutdown, powerdown, etc. Maybe you can introduce > a new "VMEvent" abstraction with functions like "request", "is > requested", "register handler"? > > It could also be interesting to convert everything to the Notifier > API, if someone wants to play with Coccinelle... > > Paolo >
On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 08/30/2010 03:50 AM, Paolo Bonzini wrote: >> >> On 08/30/2010 09:49 AM, Isaku Yamahata wrote: >>> >>> +/* those two functions are obsoleted by cold/warm reset API. */ >>> [qemu_register_reset/qemu_unregister_reset] >> >> Are they? > > Yes, but introduce more reset functions isn't the right approach. > > Reset should be a method of the device tree, not a stand alone function. In theory the reset tree may be very different from device tree. In practice the reset tree is probably very flat (global reset signal, a few bus reset signals) so device tree approach may get the same results. IIRC on some HW ages ago the CPU could initiate an external device reset (without resetting the CPU) but in that case the board caused also CPU to be reset so it was useless. One way to model the disjoint reset trees would be to use an explicit qemu_irq signal for reset. It's a bit complex to set up compared to the almost flat tree we want though.
On 08/30/2010 02:16 PM, Blue Swirl wrote: > On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 08/30/2010 03:50 AM, Paolo Bonzini wrote: >> >>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote: >>> >>>> +/* those two functions are obsoleted by cold/warm reset API. */ >>>> [qemu_register_reset/qemu_unregister_reset] >>>> >>> Are they? >>> >> Yes, but introduce more reset functions isn't the right approach. >> >> Reset should be a method of the device tree, not a stand alone function. >> > In theory the reset tree may be very different from device tree. In > practice the reset tree is probably very flat (global reset signal, a > few bus reset signals) so device tree approach may get the same > results. > Well the device tree doesn't really have to be a tree :-) My thinking if we need to support custom reset propagation is that we have the current reset() handler return 0 to propagate to children, < 0 on error, and > 0 to not propagate to direct children just as we do with the walkers. In the case of > 0, the device can choose to propagate to any device that it knows about independent of the default walking order. This makes the device tree a directed graph whereas the transversal path can be arbitrarily custom. The only questions in my mind are, do we truly need this and do we need more than a single type of reset. We could make this almost arbitrarily complicated if we wanted to but we should try to keep things simply unless there's a compelling reason not to. Regards, Anthon Liguori > IIRC on some HW ages ago the CPU could initiate an external device > reset (without resetting the CPU) but in that case the board caused > also CPU to be reset so it was useless. > > One way to model the disjoint reset trees would be to use an explicit > qemu_irq signal for reset. It's a bit complex to set up compared to > the almost flat tree we want though. > >
On Mon, Aug 30, 2010 at 7:25 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 08/30/2010 02:16 PM, Blue Swirl wrote: >> >> On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori<anthony@codemonkey.ws> >> wrote: >> >>> >>> On 08/30/2010 03:50 AM, Paolo Bonzini wrote: >>> >>>> >>>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote: >>>> >>>>> >>>>> +/* those two functions are obsoleted by cold/warm reset API. */ >>>>> [qemu_register_reset/qemu_unregister_reset] >>>>> >>>> >>>> Are they? >>>> >>> >>> Yes, but introduce more reset functions isn't the right approach. >>> >>> Reset should be a method of the device tree, not a stand alone function. >>> >> >> In theory the reset tree may be very different from device tree. In >> practice the reset tree is probably very flat (global reset signal, a >> few bus reset signals) so device tree approach may get the same >> results. >> > > Well the device tree doesn't really have to be a tree :-) True, but is the non-tree still always useful for other things besides reset? Again, in theory. > My thinking if we need to support custom reset propagation is that we have > the current reset() handler return 0 to propagate to children, < 0 on error, > and > 0 to not propagate to direct children just as we do with the walkers. > > In the case of > 0, the device can choose to propagate to any device that it > knows about independent of the default walking order. This makes the device > tree a directed graph whereas the transversal path can be arbitrarily > custom. I'd rather not have that much knowledge about the reset tree in the devices. > The only questions in my mind are, do we truly need this and do we need more > than a single type of reset. We could make this almost arbitrarily > complicated if we wanted to but we should try to keep things simply unless > there's a compelling reason not to. Fully agreed, I think current model works. But I'm not opposed to a more generic approach, like VM events, combining also power control with reset. Though events would not help with the disjoint tree problem. With qemu_irq approach, each event would be replaced by a signal type with a few instances. The devices would be as simple as now, but wiring in the board level would be bloated.
On 08/30/2010 02:36 PM, Blue Swirl wrote: > On Mon, Aug 30, 2010 at 7:25 PM, Anthony Liguori<anthony@codemonkey.ws> wrote: > >> On 08/30/2010 02:16 PM, Blue Swirl wrote: >> >>> On Mon, Aug 30, 2010 at 1:03 PM, Anthony Liguori<anthony@codemonkey.ws> >>> wrote: >>> >>> >>>> On 08/30/2010 03:50 AM, Paolo Bonzini wrote: >>>> >>>> >>>>> On 08/30/2010 09:49 AM, Isaku Yamahata wrote: >>>>> >>>>> >>>>>> +/* those two functions are obsoleted by cold/warm reset API. */ >>>>>> [qemu_register_reset/qemu_unregister_reset] >>>>>> >>>>>> >>>>> Are they? >>>>> >>>>> >>>> Yes, but introduce more reset functions isn't the right approach. >>>> >>>> Reset should be a method of the device tree, not a stand alone function. >>>> >>>> >>> In theory the reset tree may be very different from device tree. In >>> practice the reset tree is probably very flat (global reset signal, a >>> few bus reset signals) so device tree approach may get the same >>> results. >>> >>> >> Well the device tree doesn't really have to be a tree :-) >> > True, but is the non-tree still always useful for other things besides > reset? Again, in theory. > I think non-tree transversal are not the norm. But I guess my point is, being a tree verses a directed graph is really just a state of mind :-) If we use a visitor pattern and then allow a visitor to redirect the recursion, then assuming the nodes have links to other nodes that don't fall in the normal tree hierarchy, it should just work. OTOH, if we really designed it as a graph we would need to keep a visited history which generally is a pain in the butt. I don't want to do that but I think what we have now is good enough. >> My thinking if we need to support custom reset propagation is that we have >> the current reset() handler return 0 to propagate to children,< 0 on error, >> and> 0 to not propagate to direct children just as we do with the walkers. >> >> In the case of> 0, the device can choose to propagate to any device that it >> knows about independent of the default walking order. This makes the device >> tree a directed graph whereas the transversal path can be arbitrarily >> custom. >> > I'd rather not have that much knowledge about the reset tree in the devices. > Aren't the devices the precise place where that knowledge should reside? Is the reset tree really every implemented entirely based on complex wiring that exists outside of any type of bus topology? >> The only questions in my mind are, do we truly need this and do we need more >> than a single type of reset. We could make this almost arbitrarily >> complicated if we wanted to but we should try to keep things simply unless >> there's a compelling reason not to. >> > Fully agreed, I think current model works. But I'm not opposed to a > more generic approach, like VM events, combining also power control > with reset. Though events would not help with the disjoint tree > problem. > > With qemu_irq approach, each event would be replaced by a signal type > with a few instances. The devices would be as simple as now, but > wiring in the board level would be bloated. > A signal/slot infrastructure would be a better way to express this sort of thing because at the wire level, you'll quickly encounter line repeaters, muxers, and demuxers. But I think we're far ahead of ourselves. I think we'll solve the current problem merely by taking the approach in my previous patch and simply making something like PCI AER initialization part of the init() routine and not part of the reset() routine. Regards, Anthony Liguori
On Mon, Aug 30, 2010 at 07:59:22AM -0500, Anthony Liguori wrote: > On 08/30/2010 02:49 AM, Isaku Yamahata wrote: >> Distinguish warm reset from cold reset by introducing >> cold/warm reset helper function instead of single reset routines. >> >> Signed-off-by: Isaku Yamahata<yamahata@valinux.co.jp> >> --- >> hw/hw.h | 7 +++++ >> sysemu.h | 4 +++ >> vl.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++---------- >> 3 files changed, 85 insertions(+), 15 deletions(-) >> >> diff --git a/hw/hw.h b/hw/hw.h >> index 4405092..6fb844e 100644 >> --- a/hw/hw.h >> +++ b/hw/hw.h >> @@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr, >> >> typedef void QEMUResetHandler(void *opaque); >> >> + >> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque); >> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque); >> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque); >> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque); >> > > I was thinking that we should stick entirely within the qdev abstraction. > > The patchset I sent out introduced a cold reset as a qdev property on > the devices. > > For warm reset, if I understand correctly, we need two things. We need > to 1) control propagation order and we need to 2) differentiate > per-device between cold reset and warm reset. > > For (2), I don't know that we truly do need it. For something like PCI > AER, wouldn't we just move the AER initialization to the qdev init > function and then never change the AER registers during reset? > > IOW, the only way to do a cold reset would be to destroy and recreate > the device. I'm lost here. Then, what should qdev_reset() do? So far you have strongly claimed that qdev_reset() should be cold reset. (the current implementation is imperfect though) So I had to create the patch that distinguishes warm reset from cold reset. But here you suggest that qdev_reset() be warm reset and only way to cold-reset be destroy/re-create. Can you elaborate on what qdev_reset() API should do? If qdev_reset() is cold reset, something like qdev_warm_reset() is necessary. If qdev_reset() is warm reset and the only way to cold-reset is destroy/re-create, I'm fine with it. > For (1), I still don't fully understand what's needed here. Do we just > care about doing a certain transversal order (like depth-first) or do we > actually care about withholding the reset signal for devices if as Avi > said, we SCSI devices don't get reset. > > Changing transversal order is trivial. I think we should be very clear > though if we're going to introduce bus-specific mechanisms to modify > transversal though. > > Regards, > > Anthony Liguori > >> +/* those two functions are obsoleted by cold/warm reset API. */ >> void qemu_register_reset(QEMUResetHandler *func, void *opaque); >> void qemu_unregister_reset(QEMUResetHandler *func, void *opaque); >> >> diff --git a/sysemu.h b/sysemu.h >> index 7fc4e20..927892a 100644 >> --- a/sysemu.h >> +++ b/sysemu.h >> @@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void); >> void cpu_enable_ticks(void); >> void cpu_disable_ticks(void); >> >> +/* transitional compat = qemu_system_warm_reset_request() */ >> void qemu_system_reset_request(void); >> + >> +void qemu_system_cold_reset_request(void); >> +void qemu_system_warm_reset_request(void); >> void qemu_system_shutdown_request(void); >> void qemu_system_powerdown_request(void); >> extern qemu_irq qemu_system_powerdown; >> diff --git a/vl.c b/vl.c >> index a919a32..609d74c 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry { >> } QEMUResetEntry; >> >> QTAILQ_HEAD(reset_handlers, QEMUResetEntry); >> -static struct reset_handlers reset_handlers = >> - QTAILQ_HEAD_INITIALIZER(reset_handlers); >> -static int reset_requested; >> + >> +static struct reset_handlers cold_reset_handlers = >> + QTAILQ_HEAD_INITIALIZER(cold_reset_handlers); >> +static struct reset_handlers warm_reset_handlers = >> + QTAILQ_HEAD_INITIALIZER(warm_reset_handlers); >> +static int cold_reset_requested; >> +static int warm_reset_requested; >> static int shutdown_requested; >> static int powerdown_requested; >> int debug_requested; >> @@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void) >> return qemu_requested(&shutdown_requested); >> } >> >> -static int qemu_reset_requested(void) >> +static int qemu_cold_reset_requested(void) >> +{ >> + return qemu_requested(&cold_reset_requested); >> +} >> + >> +static int qemu_warm_reset_requested(void) >> { >> - return qemu_requested(&reset_requested); >> + return qemu_requested(&warm_reset_requested); >> } >> >> static int qemu_powerdown_requested(void) >> @@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers) >> } >> } >> >> +/* obsolated by qemu_register_cold/warm_reset() */ >> void qemu_register_reset(QEMUResetHandler *func, void *opaque) >> { >> - qemu_register_reset_handler(func, opaque,&reset_handlers); >> + qemu_register_cold_reset(func, opaque); >> + qemu_register_warm_reset(func, opaque); >> } >> >> +/* obsolated by qemu_unregister_cold/warm_reset() */ >> void qemu_unregister_reset(QEMUResetHandler *func, void *opaque) >> { >> - qemu_unregister_reset_handler(func, opaque,&reset_handlers); >> + qemu_unregister_cold_reset(func, opaque); >> + qemu_unregister_warm_reset(func, opaque); >> +} >> + >> +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque) >> +{ >> + qemu_register_reset_handler(func, opaque,&cold_reset_handlers); >> +} >> + >> +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque) >> +{ >> + qemu_unregister_reset_handler(func, opaque,&cold_reset_handlers); >> +} >> + >> +static void qemu_system_cold_reset(void) >> +{ >> + qemu_system_reset_handler(&cold_reset_handlers); >> + monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */ >> + cpu_synchronize_all_post_reset(); >> +} >> + >> +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque) >> +{ >> + qemu_register_reset_handler(func, opaque,&warm_reset_handlers); >> +} >> + >> +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque) >> +{ >> + qemu_unregister_reset_handler(func, opaque,&warm_reset_handlers); >> } >> >> -static void qemu_system_reset(void) >> +static void qemu_system_warm_reset(void) >> { >> - qemu_system_reset_handler(&reset_handlers); >> - monitor_protocol_event(QEVENT_RESET, NULL); >> + qemu_system_reset_handler(&warm_reset_handlers); >> + monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */ >> cpu_synchronize_all_post_reset(); >> } >> >> @@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested) >> qemu_system_request(requested); >> } >> >> +void qemu_system_cold_reset_request(void) >> +{ >> + qemu_system_request_reboot_check(&cold_reset_requested); >> +} >> + >> +void qemu_system_warm_reset_request(void) >> +{ >> + qemu_system_request_reboot_check(&warm_reset_requested); >> +} >> + >> +/* trantitional compat */ >> void qemu_system_reset_request(void) >> { >> - qemu_system_request_reboot_check(&reset_requested); >> + qemu_system_request_reboot_check(&warm_reset_requested); >> } >> >> void qemu_system_shutdown_request(void) >> @@ -1322,7 +1373,9 @@ static int vm_can_run(void) >> { >> if (powerdown_requested) >> return 0; >> - if (reset_requested) >> + if (cold_reset_requested) >> + return 0; >> + if (warm_reset_requested) >> return 0; >> if (shutdown_requested) >> return 0; >> @@ -1368,9 +1421,15 @@ static void main_loop(void) >> } else >> break; >> } >> - if (qemu_reset_requested()) { >> + if (qemu_warm_reset_requested()) { >> + pause_all_vcpus(); >> + qemu_system_warm_reset(); >> + resume_all_vcpus(); >> + } >> + if (qemu_cold_reset_requested()) { >> + /* power cycle */ >> pause_all_vcpus(); >> - qemu_system_reset(); >> + qemu_system_cold_reset(); >> resume_all_vcpus(); >> } >> if (qemu_powerdown_requested()) { >> @@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp) >> exit(1); >> } >> >> - qemu_system_reset(); >> + qemu_system_cold_reset(); >> if (loadvm) { >> if (load_vmstate(loadvm)< 0) { >> autostart = 0; >> > >
On 08/30/2010 09:58 PM, Isaku Yamahata wrote: >> I was thinking that we should stick entirely within the qdev abstraction. >> >> The patchset I sent out introduced a cold reset as a qdev property on >> the devices. >> >> For warm reset, if I understand correctly, we need two things. We need >> to 1) control propagation order and we need to 2) differentiate >> per-device between cold reset and warm reset. >> >> For (2), I don't know that we truly do need it. For something like PCI >> AER, wouldn't we just move the AER initialization to the qdev init >> function and then never change the AER registers during reset? >> >> IOW, the only way to do a cold reset would be to destroy and recreate >> the device. >> > I'm lost here. Then, what should qdev_reset() do? > I don't know, that's what I'm trying to understand. As of this moment, you've convinced me that it should be a warm reset. However, I'm not yet convinced that we need to allow buses to change the propagation path of the warm reset. Regards, Anthony Liguori
On Tue, Aug 31, 2010 at 08:08:17AM -0500, Anthony Liguori wrote: > On 08/30/2010 09:58 PM, Isaku Yamahata wrote: > >>I was thinking that we should stick entirely within the qdev abstraction. > >> > >>The patchset I sent out introduced a cold reset as a qdev property on > >>the devices. > >> > >>For warm reset, if I understand correctly, we need two things. We need > >>to 1) control propagation order and we need to 2) differentiate > >>per-device between cold reset and warm reset. > >> > >>For (2), I don't know that we truly do need it. For something like PCI > >>AER, wouldn't we just move the AER initialization to the qdev init > >>function and then never change the AER registers during reset? > >> > >>IOW, the only way to do a cold reset would be to destroy and recreate > >>the device. > >I'm lost here. Then, what should qdev_reset() do? > > I don't know, that's what I'm trying to understand. > > As of this moment, you've convinced me that it should be a warm > reset. However, I'm not yet convinced that we need to allow buses > to change the propagation path of the warm reset. > System_reset should do cold reset like it does now. -- Gleb.
On 08/31/2010 08:14 AM, Gleb Natapov wrote: > System_reset should do cold reset like it does now. Why? Regards, Anthony Liguori > > -- > Gleb. >
On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote: > On 08/31/2010 08:14 AM, Gleb Natapov wrote: > >System_reset should do cold reset like it does now. > > Why? > Because I should not be forced to restart qemu to bring devices to initial state. -- Gleb.
On 08/31/2010 08:21 AM, Gleb Natapov wrote: > On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote: > >> On 08/31/2010 08:14 AM, Gleb Natapov wrote: >> >>> System_reset should do cold reset like it does now. >>> >> Why? >> >> > Because I should not be forced to restart qemu to bring devices to > initial state. > IOW, you use system_reset for debugging purposes to reset the device model. Point taken but functionally speaking, system_reset should map to a RESET signal and from what I can tell in this thread, that's a warm reset. Regards, Anthony Liguori > -- > Gleb. >
On 08/31/2010 04:26 PM, Anthony Liguori wrote: > On 08/31/2010 08:21 AM, Gleb Natapov wrote: >> On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote: >>> On 08/31/2010 08:14 AM, Gleb Natapov wrote: >>>> System_reset should do cold reset like it does now. >>> Why? >>> >> Because I should not be forced to restart qemu to bring devices to >> initial state. > > IOW, you use system_reset for debugging purposes to reset the device > model. > > Point taken but functionally speaking, system_reset should map to a > RESET signal and from what I can tell in this thread, that's a warm > reset. > Note, for most devices there's no difference. x86 has INIT and RESET, with the keyboard controller RESET signal sometimes wired to INIT, and RAM doesn't have RESET. Otherwise most devices don't see a difference.
On 08/31/2010 08:29 AM, Avi Kivity wrote: > Note, for most devices there's no difference. x86 has INIT and RESET, > with the keyboard controller RESET signal sometimes wired to INIT, and > RAM doesn't have RESET. Otherwise most devices don't see a difference. Yes, that's why I'm wondering if we can just get away with using a simple reset() callback and for the handful of devices that don't do a full reset, they can just move the state unaffected by warm reset to ->init(). For cold reset, I'd rather approach it as a device destroy + create. This means that given a DeviceState, we need to collect enough information to recreate the device. I'm not 100% sure we have that today but if we solve that problem, it means we can migrate the device tree during migration which is a feature I'd really like to see. Regards, Anthony Liguori
On Tue, Aug 31, 2010 at 04:29:43PM +0300, Avi Kivity wrote: > On 08/31/2010 04:26 PM, Anthony Liguori wrote: > >On 08/31/2010 08:21 AM, Gleb Natapov wrote: > >>On Tue, Aug 31, 2010 at 08:20:51AM -0500, Anthony Liguori wrote: > >>>On 08/31/2010 08:14 AM, Gleb Natapov wrote: > >>>>System_reset should do cold reset like it does now. > >>>Why? > >>> > >>Because I should not be forced to restart qemu to bring devices to > >>initial state. > > > >IOW, you use system_reset for debugging purposes to reset the > >device model. > > > >Point taken but functionally speaking, system_reset should map to > >a RESET signal and from what I can tell in this thread, that's a > >warm reset. > > > > Note, for most devices there's no difference. x86 has INIT and > RESET, with the keyboard controller RESET signal sometimes wired to > INIT, and RAM doesn't have RESET. Otherwise most devices don't see > a difference. > Actually soft reset is defined as INIT in PIIX4 spec: Bits 1 and 2 in this register are used by PIIX4 to generate a hard reset or a soft reset. During a hard reset, PIIX4 asserts CPURST, PCIRST#, and RSTDRV, as well as reset its core and suspend well logic. During a soft reset, PIIX4 asserts INIT. -- Gleb.
On 08/31/2010 04:34 PM, Anthony Liguori wrote: > On 08/31/2010 08:29 AM, Avi Kivity wrote: >> Note, for most devices there's no difference. x86 has INIT and >> RESET, with the keyboard controller RESET signal sometimes wired to >> INIT, and RAM doesn't have RESET. Otherwise most devices don't see a >> difference. > > Yes, that's why I'm wondering if we can just get away with using a > simple reset() callback and for the handful of devices that don't do a > full reset, they can just move the state unaffected by warm reset to > ->init(). > This seems reasonable. > For cold reset, I'd rather approach it as a device destroy + create. > This means that given a DeviceState, we need to collect enough > information to recreate the device. I'm not 100% sure we have that > today but if we solve that problem, it means we can migrate the device > tree during migration which is a feature I'd really like to see. Why do we need a cold reset at all? it doesn't map to anything.
On 08/31/2010 08:46 AM, Avi Kivity wrote: > On 08/31/2010 04:34 PM, Anthony Liguori wrote: >> On 08/31/2010 08:29 AM, Avi Kivity wrote: >>> Note, for most devices there's no difference. x86 has INIT and >>> RESET, with the keyboard controller RESET signal sometimes wired to >>> INIT, and RAM doesn't have RESET. Otherwise most devices don't see >>> a difference. >> >> Yes, that's why I'm wondering if we can just get away with using a >> simple reset() callback and for the handful of devices that don't do >> a full reset, they can just move the state unaffected by warm reset >> to ->init(). >> > > This seems reasonable. But I'm still not sure whether the reset signal can be deliver based on a pre-order transversal or whether a custom transversal was required that each bus participates in. >> For cold reset, I'd rather approach it as a device destroy + create. >> This means that given a DeviceState, we need to collect enough >> information to recreate the device. I'm not 100% sure we have that >> today but if we solve that problem, it means we can migrate the >> device tree during migration which is a feature I'd really like to see. > > Why do we need a cold reset at all? it doesn't map to anything. That's why I'm suggesting a second-class approach to implementing it if someone really wants it. Regards, Anthony Liguori
On Tue, Aug 31, 2010 at 08:58:06AM -0500, Anthony Liguori wrote: > On 08/31/2010 08:46 AM, Avi Kivity wrote: > > On 08/31/2010 04:34 PM, Anthony Liguori wrote: > >>On 08/31/2010 08:29 AM, Avi Kivity wrote: > >>>Note, for most devices there's no difference. x86 has INIT > >>>and RESET, with the keyboard controller RESET signal sometimes > >>>wired to INIT, and RAM doesn't have RESET. Otherwise most > >>>devices don't see a difference. > >> > >>Yes, that's why I'm wondering if we can just get away with using > >>a simple reset() callback and for the handful of devices that > >>don't do a full reset, they can just move the state unaffected > >>by warm reset to ->init(). > >> > > > >This seems reasonable. > > But I'm still not sure whether the reset signal can be deliver based > on a pre-order transversal or whether a custom transversal was > required that each bus participates in. > The thing is in qemu reset of one device can affect state of other device. Think about device that updates its interrupt line during reset and this affects pic/ioapic/apic. Real HW does not have a problem that we have here. -- Gleb.
On 08/31/2010 04:58 PM, Anthony Liguori wrote: > On 08/31/2010 08:46 AM, Avi Kivity wrote: >> On 08/31/2010 04:34 PM, Anthony Liguori wrote: >>> On 08/31/2010 08:29 AM, Avi Kivity wrote: >>>> Note, for most devices there's no difference. x86 has INIT and >>>> RESET, with the keyboard controller RESET signal sometimes wired to >>>> INIT, and RAM doesn't have RESET. Otherwise most devices don't see >>>> a difference. >>> >>> Yes, that's why I'm wondering if we can just get away with using a >>> simple reset() callback and for the handful of devices that don't do >>> a full reset, they can just move the state unaffected by warm reset >>> to ->init(). >>> >> >> This seems reasonable. > > But I'm still not sure whether the reset signal can be deliver based > on a pre-order transversal or whether a custom transversal was > required that each bus participates in. There's no such thing as a global reset. There's a trace on the board that's called RESET that's connected to many chip's RESET input (but perhaps not all). A PCI bus controller will likely propagate its RESET input into the PCI reset signal, however it's called. So individual RESET traces are connected by chips, which have different conditions for asserting them. So yes, we need a custom traversal. Some bus controllers will block off RESET completely, some will pass it through, some can reset the devices on their bus independently of the controller's RESET input, if it has one. (but I think we're drifting off into pointlessness here) > >>> For cold reset, I'd rather approach it as a device destroy + >>> create. This means that given a DeviceState, we need to collect >>> enough information to recreate the device. I'm not 100% sure we >>> have that today but if we solve that problem, it means we can >>> migrate the device tree during migration which is a feature I'd >>> really like to see. >> >> Why do we need a cold reset at all? it doesn't map to anything. > > That's why I'm suggesting a second-class approach to implementing it > if someone really wants it. Sure.
On 08/31/2010 09:03 AM, Avi Kivity wrote: > There's no such thing as a global reset. There's a trace on the board > that's called RESET that's connected to many chip's RESET input (but > perhaps not all). A PCI bus controller will likely propagate its > RESET input into the PCI reset signal, however it's called. So > individual RESET traces are connected by chips, which have different > conditions for asserting them. > > So yes, we need a custom traversal. Some bus controllers will block > off RESET completely, some will pass it through, some can reset the > devices on their bus independently of the controller's RESET input, if > it has one. > > (but I think we're drifting off into pointlessness here) That's my source of confusion. I know hardware does a custom transversal but from a functional perspective, do we care? So far, no one has answered this with, "yes, because device XX propagates in a specific ordering of child a, b, c, d..." which I take to mean that the answer is no. Regards, Anthony Liguori
On 08/31/2010 06:00 PM, Anthony Liguori wrote: > On 08/31/2010 09:03 AM, Avi Kivity wrote: >> There's no such thing as a global reset. There's a trace on the >> board that's called RESET that's connected to many chip's RESET input >> (but perhaps not all). A PCI bus controller will likely propagate >> its RESET input into the PCI reset signal, however it's called. So >> individual RESET traces are connected by chips, which have different >> conditions for asserting them. >> >> So yes, we need a custom traversal. Some bus controllers will block >> off RESET completely, some will pass it through, some can reset the >> devices on their bus independently of the controller's RESET input, >> if it has one. >> >> (but I think we're drifting off into pointlessness here) > > That's my source of confusion. I know hardware does a custom > transversal but from a functional perspective, do we care? > > So far, no one has answered this with, "yes, because device XX > propagates in a specific ordering of child a, b, c, d..." which I take > to mean that the answer is no. > I don't think there's an issue with the order of propagation, just whether propagation takes place or not. I'd make all bus models explicitly propagate reset if they want to.
diff --git a/hw/hw.h b/hw/hw.h index 4405092..6fb844e 100644 --- a/hw/hw.h +++ b/hw/hw.h @@ -269,6 +269,13 @@ void register_device_unmigratable(DeviceState *dev, const char *idstr, typedef void QEMUResetHandler(void *opaque); + +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque); +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque); +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque); +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque); + +/* those two functions are obsoleted by cold/warm reset API. */ void qemu_register_reset(QEMUResetHandler *func, void *opaque); void qemu_unregister_reset(QEMUResetHandler *func, void *opaque); diff --git a/sysemu.h b/sysemu.h index 7fc4e20..927892a 100644 --- a/sysemu.h +++ b/sysemu.h @@ -48,7 +48,11 @@ int64_t cpu_get_ticks(void); void cpu_enable_ticks(void); void cpu_disable_ticks(void); +/* transitional compat = qemu_system_warm_reset_request() */ void qemu_system_reset_request(void); + +void qemu_system_cold_reset_request(void); +void qemu_system_warm_reset_request(void); void qemu_system_shutdown_request(void); void qemu_system_powerdown_request(void); extern qemu_irq qemu_system_powerdown; diff --git a/vl.c b/vl.c index a919a32..609d74c 100644 --- a/vl.c +++ b/vl.c @@ -1122,9 +1122,13 @@ typedef struct QEMUResetEntry { } QEMUResetEntry; QTAILQ_HEAD(reset_handlers, QEMUResetEntry); -static struct reset_handlers reset_handlers = - QTAILQ_HEAD_INITIALIZER(reset_handlers); -static int reset_requested; + +static struct reset_handlers cold_reset_handlers = + QTAILQ_HEAD_INITIALIZER(cold_reset_handlers); +static struct reset_handlers warm_reset_handlers = + QTAILQ_HEAD_INITIALIZER(warm_reset_handlers); +static int cold_reset_requested; +static int warm_reset_requested; static int shutdown_requested; static int powerdown_requested; int debug_requested; @@ -1142,9 +1146,14 @@ static int qemu_shutdown_requested(void) return qemu_requested(&shutdown_requested); } -static int qemu_reset_requested(void) +static int qemu_cold_reset_requested(void) +{ + return qemu_requested(&cold_reset_requested); +} + +static int qemu_warm_reset_requested(void) { - return qemu_requested(&reset_requested); + return qemu_requested(&warm_reset_requested); } static int qemu_powerdown_requested(void) @@ -1196,20 +1205,51 @@ static void qemu_system_reset_handler(struct reset_handlers *handlers) } } +/* obsolated by qemu_register_cold/warm_reset() */ void qemu_register_reset(QEMUResetHandler *func, void *opaque) { - qemu_register_reset_handler(func, opaque, &reset_handlers); + qemu_register_cold_reset(func, opaque); + qemu_register_warm_reset(func, opaque); } +/* obsolated by qemu_unregister_cold/warm_reset() */ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque) { - qemu_unregister_reset_handler(func, opaque, &reset_handlers); + qemu_unregister_cold_reset(func, opaque); + qemu_unregister_warm_reset(func, opaque); +} + +void qemu_register_cold_reset(QEMUResetHandler *func, void *opaque) +{ + qemu_register_reset_handler(func, opaque, &cold_reset_handlers); +} + +void qemu_unregister_cold_reset(QEMUResetHandler *func, void *opaque) +{ + qemu_unregister_reset_handler(func, opaque, &cold_reset_handlers); +} + +static void qemu_system_cold_reset(void) +{ + qemu_system_reset_handler(&cold_reset_handlers); + monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_COLD? */ + cpu_synchronize_all_post_reset(); +} + +void qemu_register_warm_reset(QEMUResetHandler *func, void *opaque) +{ + qemu_register_reset_handler(func, opaque, &warm_reset_handlers); +} + +void qemu_unregister_warm_reset(QEMUResetHandler *func, void *opaque) +{ + qemu_unregister_reset_handler(func, opaque, &warm_reset_handlers); } -static void qemu_system_reset(void) +static void qemu_system_warm_reset(void) { - qemu_system_reset_handler(&reset_handlers); - monitor_protocol_event(QEVENT_RESET, NULL); + qemu_system_reset_handler(&warm_reset_handlers); + monitor_protocol_event(QEVENT_RESET, NULL); /* QEVENT_RESET_WARM? */ cpu_synchronize_all_post_reset(); } @@ -1227,9 +1267,20 @@ static void qemu_system_request_reboot_check(int *requested) qemu_system_request(requested); } +void qemu_system_cold_reset_request(void) +{ + qemu_system_request_reboot_check(&cold_reset_requested); +} + +void qemu_system_warm_reset_request(void) +{ + qemu_system_request_reboot_check(&warm_reset_requested); +} + +/* trantitional compat */ void qemu_system_reset_request(void) { - qemu_system_request_reboot_check(&reset_requested); + qemu_system_request_reboot_check(&warm_reset_requested); } void qemu_system_shutdown_request(void) @@ -1322,7 +1373,9 @@ static int vm_can_run(void) { if (powerdown_requested) return 0; - if (reset_requested) + if (cold_reset_requested) + return 0; + if (warm_reset_requested) return 0; if (shutdown_requested) return 0; @@ -1368,9 +1421,15 @@ static void main_loop(void) } else break; } - if (qemu_reset_requested()) { + if (qemu_warm_reset_requested()) { + pause_all_vcpus(); + qemu_system_warm_reset(); + resume_all_vcpus(); + } + if (qemu_cold_reset_requested()) { + /* power cycle */ pause_all_vcpus(); - qemu_system_reset(); + qemu_system_cold_reset(); resume_all_vcpus(); } if (qemu_powerdown_requested()) { @@ -2992,7 +3051,7 @@ int main(int argc, char **argv, char **envp) exit(1); } - qemu_system_reset(); + qemu_system_cold_reset(); if (loadvm) { if (load_vmstate(loadvm) < 0) { autostart = 0;
Distinguish warm reset from cold reset by introducing cold/warm reset helper function instead of single reset routines. Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> --- hw/hw.h | 7 +++++ sysemu.h | 4 +++ vl.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 85 insertions(+), 15 deletions(-)