From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48971) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwlIG-0005nw-Tw for qemu-devel@nongnu.org; Wed, 01 Aug 2012 22:37:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwlIF-0006UZ-9n for qemu-devel@nongnu.org; Wed, 01 Aug 2012 22:37:44 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:65308) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwlIF-0006UV-3G for qemu-devel@nongnu.org; Wed, 01 Aug 2012 22:37:43 -0400 Received: by obbta14 with SMTP id ta14so12399177obb.4 for ; Wed, 01 Aug 2012 19:37:42 -0700 (PDT) From: Anthony Liguori In-Reply-To: <1343873409-8571-2-git-send-email-david@gibson.dropbear.id.au> References: <1343873409-8571-1-git-send-email-david@gibson.dropbear.id.au> <1343873409-8571-2-git-send-email-david@gibson.dropbear.id.au> Date: Wed, 01 Aug 2012 21:37:39 -0500 Message-ID: <87vch2nhn0.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 1/2] Allow QEMUMachine to override reset sequencing List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, agraf@suse.de David Gibson writes: > At present the qemu_system_reset() function always performs the same basic > actions on all machines. This includes running all the reset handler > hooks, however the order in which they run is not controlled by the board > logic. Let's be careful here in referring to "board logic". Reset should propagate--there's no argument there. Having all devices register in a list with no control over how reset is dispatched is wrong. It's workable because of qdev imposes per-bus reset functions and typically we have a small number of busses and ordering doesn't matter. > This is incorrect: any modern real hardware will generally have some sort > of reset controller, sometimes a full microcontroller or even service > processor which will control the order in which components on the board are > reset. I think this is true only with a loose definition of "modern", but okay. I don't think this is the right argument. The machine should serve as the entry point for reset. What components get reset in what order will need to be a machine hook for practical purposes (since not everything will be fully QOMized all at once). So I think this is the right approach for now. But all of the DT initialization stuff that is leading to this discussion in the first place is a gross hack to make a PV architecture work that took far too many short cuts. Building a DT in memory representing hardware instead of making things discoverable is not how hardware works. This sort of stuff is either (1) hard coded in a firmware/flashrom or (2) built dynamically in firmware. Let's not pretend like we're doing this because it's needed for real hardware. Regards, Anthony Liguori > For one specific example, the machine level reset handlers may need to > re-establish certain things in memory to meet the emulated platform's > specified entry conditions, before reexecuting the guest software. > re-establish the correct specified entry conditions for the emulated > platform. This must happen _after_ resetting peripheral devices, or they > could still be in the middle of DMA operations which would clobber the new > memory content. However with the normal ordering of reset hooks, the > machine is not able to register a hook which will run after the normal > qdev reset registered by generic code. > > This patch allows the machine to take control of the sequence of operations > during reset, by adding a new reset hook to the QEMUMachine. This hook is > optional - without it we just use the same reset sequence as always. That > logic is available in qemu_default_system_reset() to make it easy to > implement the case of the machine logic just needing to do some things > either before or after the normal reset. > > Signed-off-by: David Gibson > --- > hw/boards.h | 3 +++ > sysemu.h | 1 + > vl.c | 10 +++++++++- > 3 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/hw/boards.h b/hw/boards.h > index 59c01d0..f2bfd3e 100644 > --- a/hw/boards.h > +++ b/hw/boards.h > @@ -12,11 +12,14 @@ typedef void QEMUMachineInitFunc(ram_addr_t ram_size, > const char *initrd_filename, > const char *cpu_model); > > +typedef void QEMUMachineResetFunc(bool report); > + > typedef struct QEMUMachine { > const char *name; > const char *alias; > const char *desc; > QEMUMachineInitFunc *init; > + QEMUMachineResetFunc *reset; > int use_scsi; > int max_cpus; > unsigned int no_serial:1, > diff --git a/sysemu.h b/sysemu.h > index 6540c79..f74319b 100644 > --- a/sysemu.h > +++ b/sysemu.h > @@ -62,6 +62,7 @@ int qemu_powerdown_requested(void); > void qemu_system_killed(int signal, pid_t pid); > void qemu_kill_report(void); > extern qemu_irq qemu_system_powerdown; > +void qemu_default_system_reset(bool report); > void qemu_system_reset(bool report); > > void qemu_add_exit_notifier(Notifier *notify); > diff --git a/vl.c b/vl.c > index 9fea320..ac47a7c 100644 > --- a/vl.c > +++ b/vl.c > @@ -1396,7 +1396,7 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque) > } > } > > -void qemu_system_reset(bool report) > +void qemu_default_system_reset(bool report) > { > QEMUResetEntry *re, *nre; > > @@ -1410,6 +1410,14 @@ void qemu_system_reset(bool report) > cpu_synchronize_all_post_reset(); > } Not a huge fan of the naming here. How about qemu_devices_reset()? > +void qemu_system_reset(bool report) > +{ > + if (current_machine->reset) > + current_machine->reset(report); > + else > + qemu_default_system_reset(report); > +} Missing curly braces. Regards, Anthony Liguori > + > void qemu_system_reset_request(void) > { > if (no_reboot) { > -- > 1.7.10.4