[RFC] Distinguish between reset types

As discussed at length already, one potential 'workaround' for KVM
brokenness in old kernels (<3.9) with old CPUs (without 'unrestricted
guest' support), is to properly reset the PAM registers in the chipset.

I say 'workaround' but this would be a proper fix in its own right, and
as a side-effect would help to work around the problem we're actually
suffering.

To do it properly, we need to distinguish between a 'hard' reset
triggered by the PIIX3 RCR register, which resets the PAM configuration,
and a 'soft' reset triggered for example by the keyboard controller,
which doesn't.

This patch attempts to introduce a ResetType into the code logic. Rather
than propagating that ResetType through the entire stack of device reset
functions, I've added a 'qemu_last_reset_get()' function so that the
devices which *do* care can look for it.

Comments? There are a whole bunch more qemu_system_reset_request() calls
which will need a ResetType added, if I do it this way...

On 19 February 2013 11:40, David Woodhouse <dwmw2@infradead.org> wrote:
> To do it properly, we need to distinguish between a 'hard' reset
> triggered by the PIIX3 RCR register, which resets the PAM configuration,
> and a 'soft' reset triggered for example by the keyboard controller,
> which doesn't.
>
> This patch attempts to introduce a ResetType into the code logic. Rather
> than propagating that ResetType through the entire stack of device reset
> functions, I've added a 'qemu_last_reset_get()' function so that the
> devices which *do* care can look for it.
>
> Comments? There are a whole bunch more qemu_system_reset_request() calls
> which will need a ResetType added, if I do it this way...

I'm dubious about this. At the moment we have one set of reset
semantics, which is "this works as if you yanked the power to
the machine and plugged it back in again". We also have a number
of cases of device-local "software reset" where the device
internally resets just some of its state when the guest prods
an appropriate register.

If you want any other kind of reset, we have to start actually
really modelling the hardware with all the various reset lines
and so on. So a keyboard controller triggered reset should work
by asserting a gpio line which probably goes to some kind of power
controller which in turn causes various other devices to act in
whatever way the hardware acts for soft reset.

Basically, I don't think you can come up with a definition of
"soft reset" that makes sense across the range of architectures
and boards that we model in QEMU.

-- PMM

Il 19/02/2013 12:40, David Woodhouse ha scritto:
> As discussed at length already, one potential 'workaround' for KVM
> brokenness in old kernels (<3.9) with old CPUs (without 'unrestricted
> guest' support), is to properly reset the PAM registers in the chipset.
> 
> I say 'workaround' but this would be a proper fix in its own right, and
> as a side-effect would help to work around the problem we're actually
> suffering.
> 
> To do it properly, we need to distinguish between a 'hard' reset
> triggered by the PIIX3 RCR register, which resets the PAM configuration,
> and a 'soft' reset triggered for example by the keyboard controller,
> which doesn't.

I think the question is what happens in real hardware?  It is not very
far from the other patch you posted, actually.  The PCIRST# signal of
the PIIX3 must go to the i440FX.

So you can make the hard reset line a qemu_irq (output in PIIX, input in
i440FX) using qdev_init_gpio_in/out.  The input side in the i440FX then
can reset the PAM registers while the output side can pulse it before
calling qemu_system_reset_request() if RCR bit 1 is set.  Then you can
connect it using qdev_connect_gpio_out/qdev_get_gpio_in in
i440fx_common_init.

Paolo

Am 19.02.2013 12:40, schrieb David Woodhouse:
> As discussed at length already, one potential 'workaround' for KVM
> brokenness in old kernels (<3.9) with old CPUs (without 'unrestricted
> guest' support), is to properly reset the PAM registers in the chipset.
> 
> I say 'workaround' but this would be a proper fix in its own right, and
> as a side-effect would help to work around the problem we're actually
> suffering.
> 
> To do it properly, we need to distinguish between a 'hard' reset
> triggered by the PIIX3 RCR register, which resets the PAM configuration,
> and a 'soft' reset triggered for example by the keyboard controller,
> which doesn't.
> 
> This patch attempts to introduce a ResetType into the code logic. Rather
> than propagating that ResetType through the entire stack of device reset
> functions, I've added a 'qemu_last_reset_get()' function so that the
> devices which *do* care can look for it.
> 
> Comments? There are a whole bunch more qemu_system_reset_request() calls
> which will need a ResetType added, if I do it this way...

So far QEMU does not distinguish between reset types. Therefore I think
it is a bad idea to place QEMU_RESET_HARD in hw/cuda.c without reviewing
per case whether that is actually a hard or soft reset.

Better and much less intrusive would be introducing separate functions
with the new argument, leaving the existing code unchanged until
reviewed and decided by their maintainers (which may be in the range of
months). Alternatively a QEMU_RESET_DEFAULT aliased to QEMU_RESET_HARD
to raise awareness where decisions need to be taken.

Regards,
Andreas

On Tue, 2013-02-19 at 14:38 +0100, Andreas Färber wrote:
> So far QEMU does not distinguish between reset types. Therefore I think
> it is a bad idea to place QEMU_RESET_HARD in hw/cuda.c without reviewing
> per case whether that is actually a hard or soft reset.
> 
> Better and much less intrusive would be introducing separate functions
> with the new argument, leaving the existing code unchanged until
> reviewed and decided by their maintainers (which may be in the range of
> months). Alternatively a QEMU_RESET_DEFAULT aliased to QEMU_RESET_HARD
> to raise awareness where decisions need to be taken.

Yes, agreed. I'd want to set it up in such a fashion that each change
from the default to a specific QEMU_RESET_XXX type was in a separate
patch, for each device. And the implementation for i440fx would
obviously be a separate patch too.

On Tue, 2013-02-19 at 12:04 +0000, Peter Maydell wrote:
> I'm dubious about this. At the moment we have one set of reset
> semantics, which is "this works as if you yanked the power to
> the machine and plugged it back in again".

Except when it doesn't. Such as the PAM registers on the i440FX, which
*don't* reset themselves to the poweron state during a reset. And when I
posted the naïve patch to 'fix' that, it was pointed out to me that some
resets *shouldn't* do that.

>  We also have a number of cases of device-local "software reset" where
> the device internally resets just some of its state when the guest
> prods an appropriate register.

Yeah, when it's truly device-local that's different and fairly trivial
to handle.

> If you want any other kind of reset, we have to start actually
> really modelling the hardware with all the various reset lines
> and so on. So a keyboard controller triggered reset should work
> by asserting a gpio line which probably goes to some kind of power
> controller which in turn causes various other devices to act in
> whatever way the hardware acts for soft reset.

Maybe, although often these reset lines *are* system-wide. The 'soft'
vs. 'hard' distinction isn't something that the core code actually cares
about; its precise definition is a matter to be agreed between the
platform and the device drivers which run on that platform. It would be
trivial to extend the enum to handle additional values if a given
platform actually needed to.

> Basically, I don't think you can come up with a definition of
> "soft reset" that makes sense across the range of architectures
> and boards that we model in QEMU.

No, but (as discussed above) I don't think we *need* to have such a
universal definition, either.

But I'm not particularly wedded to this approach; we can do it with a
hack directly in piix_pci.c too. To a certain extent, this is just a
straw man, to show that the local hack isn't such a bad thing after
all :)

On 19 February 2013 13:58, David Woodhouse <dwmw2@infradead.org> wrote:
> On Tue, 2013-02-19 at 12:04 +0000, Peter Maydell wrote:
>> I'm dubious about this. At the moment we have one set of reset
>> semantics, which is "this works as if you yanked the power to
>> the machine and plugged it back in again".
>
> Except when it doesn't. Such as the PAM registers on the i440FX, which
> *don't* reset themselves to the poweron state during a reset. And when I
> posted the naïve patch to 'fix' that, it was pointed out to me that some
> resets *shouldn't* do that.

If they are reacting to QEMU reset by doing anything other than
"reset to poweron state" then that reset function is broken and
needs fixing. If there are other types of reset that need to
be modelled than power on reset then you need to actually
model the power controller, reset lines, etc, or at least a
sufficient subset of them for your purposes.

>> If you want any other kind of reset, we have to start actually
>> really modelling the hardware with all the various reset lines
>> and so on. So a keyboard controller triggered reset should work
>> by asserting a gpio line which probably goes to some kind of power
>> controller which in turn causes various other devices to act in
>> whatever way the hardware acts for soft reset.
>
> Maybe, although often these reset lines *are* system-wide.

And often they are not, which is my point. The only way
to actually model this stuff correctly is to model it
correctly. Mostly we get away with not doing so, because not
a lot of things truly care about the difference between a
programmed reset and powercycling the hardware.

> The 'soft'
> vs. 'hard' distinction isn't something that the core code actually cares
> about; its precise definition is a matter to be agreed between the
> platform and the device drivers which run on that platform.

This assumes that the devices are only ever used in a single
platform with a single way of doing reset, which is also not
always true.

>> Basically, I don't think you can come up with a definition of
>> "soft reset" that makes sense across the range of architectures
>> and boards that we model in QEMU.
>
> No, but (as discussed above) I don't think we *need* to have such a
> universal definition, either.

Your patch is effectively trying to introduce one (without
actually calling it one), which is why I'm arguing against it...

> But I'm not particularly wedded to this approach; we can do it with a
> hack directly in piix_pci.c too. To a certain extent, this is just a
> straw man, to show that the local hack isn't such a bad thing after
> all :)

It certainly sounds like it's closer to the right approach...

-- PMM

David Woodhouse <dwmw2@infradead.org> writes:

> As discussed at length already, one potential 'workaround' for KVM
> brokenness in old kernels (<3.9) with old CPUs (without 'unrestricted
> guest' support), is to properly reset the PAM registers in the chipset.
>
> I say 'workaround' but this would be a proper fix in its own right, and
> as a side-effect would help to work around the problem we're actually
> suffering.
>
> To do it properly, we need to distinguish between a 'hard' reset
> triggered by the PIIX3 RCR register, which resets the PAM configuration,
> and a 'soft' reset triggered for example by the keyboard controller,
> which doesn't.
>
> This patch attempts to introduce a ResetType into the code logic. Rather
> than propagating that ResetType through the entire stack of device reset
> functions, I've added a 'qemu_last_reset_get()' function so that the
> devices which *do* care can look for it.
>
> Comments? There are a whole bunch more qemu_system_reset_request() calls
> which will need a ResetType added, if I do it this way...

The existing API does suck, but there also isn't a universal "hard" and
"soft" reset.  If we're going to touch a bunch of code, we should try to
come up with the right interface.

qemu_system_reset_request() today does the following:

 1) indicate that a reset is pending
 2) break the vcpus out of their loops and wait for them to all quiesce
 3) invoke qemu_devices_reset() (or a machine specific reset handler)
   a) this also resets the cpus
 4) resume all vcpus

I think what we really need is a way to have finer control over what
happens in step (3).  Instead of passing a ResetType, I think we should
introduce a new function that takes a function pointer/opaque that gets
invoked for step (3).

The existing qemu_system_reset_request() would call this function with a
function pointer that invokes qemu_devices_reset().

But you could also invoke this new function with your own callback that
let you control the behavior of reset.

It's a bit ugly, but you would still need to set some sort of global
flag to tell the PIIX to do a soft reset.  However, over time, as we
eliminate the list of reset notifiers, we could call a different reset
method.

Plus, with this approach, you don't have to touch a bunch of different
devices...

Regards,

Anthony Liguori

>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index 7eb0c2b..79d7466 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -135,9 +135,9 @@ static void apb_config_writel (void *opaque, hwaddr addr,
>              s->reset_control |= val & RESET_WMASK;
>              if (val & SOFT_POR) {
>                  s->nr_resets = 0;
> -                qemu_system_reset_request();
> +                qemu_system_reset_request(QEMU_RESET_SOFT);
>              } else if (val & SOFT_XIR) {
> -                qemu_system_reset_request();
> +                qemu_system_reset_request(QEMU_RESET_SOFT);
>              }
>          }
>          break;
> diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
> index 7ecb7da..b553539 100644
> --- a/hw/arm_sysctl.c
> +++ b/hw/arm_sysctl.c
> @@ -239,7 +239,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
>              if (s->lockval == LOCK_VALUE) {
>                  s->resetlevel = val;
>                  if (val & 0x100) {
> -                    qemu_system_reset_request();
> +                    qemu_system_reset_request(QEMU_RESET_HARD);
>                  }
>              }
>              break;
> @@ -248,7 +248,7 @@ static void arm_sysctl_write(void *opaque, hwaddr offset,
>              if (s->lockval == LOCK_VALUE) {
>                  s->resetlevel = val;
>                  if (val & 0x04) {
> -                    qemu_system_reset_request();
> +                    qemu_system_reset_request(QEMU_RESET_HARD);
>                  }
>              }
>              break;
> diff --git a/hw/cuda.c b/hw/cuda.c
> index b36c535..20c0894 100644
> --- a/hw/cuda.c
> +++ b/hw/cuda.c
> @@ -529,7 +529,7 @@ static void cuda_receive_packet(CUDAState *s,
>          obuf[0] = CUDA_PACKET;
>          obuf[1] = 0;
>          cuda_send_packet_to_host(s, obuf, 2);
> -        qemu_system_reset_request();
> +        qemu_system_reset_request(QEMU_RESET_HARD);
>          break;
>      default:
>          break;
> diff --git a/hw/m48t59.c b/hw/m48t59.c
> index 427d95b..5a21701 100644
> --- a/hw/m48t59.c
> +++ b/hw/m48t59.c
> @@ -166,7 +166,7 @@ static void watchdog_cb (void *opaque)
>  	NVRAM->buffer[0x1FF7] = 0x00;
>  	NVRAM->buffer[0x1FFC] &= ~0x40;
>          /* May it be a hw CPU Reset instead ? */
> -        qemu_system_reset_request();
> +        qemu_system_reset_request(QEMU_RESET_HARD);
>      } else {
>  	qemu_set_irq(NVRAM->IRQ, 1);
>  	qemu_set_irq(NVRAM->IRQ, 0);
> diff --git a/hw/pc.c b/hw/pc.c
> index 53cc173..d138a92 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -440,7 +440,7 @@ static void port92_write(void *opaque, hwaddr addr, uint64_t val,
>      s->outport = val;
>      qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      if (val & 1) {
> -        qemu_system_reset_request();
> +        qemu_system_reset_request(QEMU_RESET_SOFT);
>      }
>  }
>  
> diff --git a/hw/pckbd.c b/hw/pckbd.c
> index 3bad09b..b55d61e 100644
> --- a/hw/pckbd.c
> +++ b/hw/pckbd.c
> @@ -220,7 +220,8 @@ static void outport_write(KBDState *s, uint32_t val)
>          qemu_set_irq(*s->a20_out, (val >> 1) & 1);
>      }
>      if (!(val & 1)) {
> -        qemu_system_reset_request();
> +        /* This should be a soft reset, not full chipset reset. */
> +        qemu_system_reset_request(QEMU_RESET_SOFT);
>      }
>  }
>  
> @@ -299,7 +300,7 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>          s->outport &= ~KBD_OUT_A20;
>          break;
>      case KBD_CCMD_RESET:
> -        qemu_system_reset_request();
> +        qemu_system_reset_request(QEMU_RESET_SOFT);
>          break;
>      case KBD_CCMD_NO_OP:
>          /* ignore that */
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 6c77e49..55654c0 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -171,6 +171,27 @@ static int i440fx_load_old(QEMUFile* f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +static void i440fx_reset(DeviceState *ds)
> +{
> +    PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, ds);
> +    PCII440FXState *d = DO_UPCAST(PCII440FXState, dev, dev);
> +    uint8_t *pci_conf = d->dev.config;
> +
> +    if (qemu_last_reset_get() < QEMU_RESET_HARD)
> +        return;
> +
> +    pci_conf[0x59] = 0x00; // Reset PAM setup
> +    pci_conf[0x5a] = 0x00;
> +    pci_conf[0x5b] = 0x00;
> +    pci_conf[0x5c] = 0x00;
> +    pci_conf[0x5d] = 0x00;
> +    pci_conf[0x5e] = 0x00;
> +    pci_conf[0x5f] = 0x00;
> +    pci_conf[0x72] = 0x02; // And SMM
> +
> +    i440fx_update_memory_mappings(d);
> +}
> +
>  static int i440fx_post_load(void *opaque, int version_id)
>  {
>      PCII440FXState *d = opaque;
> @@ -521,7 +542,10 @@ static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
>      PIIX3State *d = opaque;
>  
>      if (val & 4) {
> -        qemu_system_reset_request();
> +        if (val & 2)
> +            qemu_system_reset_request(QEMU_RESET_HARD);
> +        else
> +            qemu_system_reset_request(QEMU_RESET_SOFT);
>          return;
>      }
>      d->rcr = val & 2; /* keep System Reset type only */
> @@ -615,6 +639,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
>      dc->desc = "Host bridge";
>      dc->no_user = 1;
>      dc->vmsd = &vmstate_i440fx;
> +    dc->reset = i440fx_reset;
>  }
>  
>  static const TypeInfo i440fx_info = {
> diff --git a/hw/watchdog.c b/hw/watchdog.c
> index 072d256..ba5aa7e 100644
> --- a/hw/watchdog.c
> +++ b/hw/watchdog.c
> @@ -117,7 +117,7 @@ void watchdog_perform_action(void)
>      switch(watchdog_action) {
>      case WDT_RESET:             /* same as 'system_reset' in monitor */
>          watchdog_mon_event("reset");
> -        qemu_system_reset_request();
> +        qemu_system_reset_request(QEMU_RESET_HARD);
>          break;
>  
>      case WDT_SHUTDOWN:          /* same as 'system_powerdown' in monitor */
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 1d9599e..90635a5 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -33,6 +33,14 @@ void vm_state_notify(int running, RunState state);
>  #define VMRESET_SILENT   false
>  #define VMRESET_REPORT   true
>  
> +typedef enum ResetType {
> +    QEMU_RESET_NONE = 0,
> +    QEMU_RESET_WAKEUP,
> +    QEMU_RESET_SOFT,
> +    QEMU_RESET_HARD,
> +    QEMU_RESET_POWERON,
> +} ResetType;
> +    
>  void vm_start(void);
>  void vm_stop(RunState state);
>  void vm_stop_force_state(RunState state);
> @@ -43,7 +51,7 @@ typedef enum WakeupReason {
>      QEMU_WAKEUP_REASON_PMTIMER,
>  } WakeupReason;
>  
> -void qemu_system_reset_request(void);
> +void qemu_system_reset_request(ResetType type);
>  void qemu_system_suspend_request(void);
>  void qemu_register_suspend_notifier(Notifier *notifier);
>  void qemu_system_wakeup_request(WakeupReason reason);
> @@ -55,10 +63,11 @@ void qemu_register_powerdown_notifier(Notifier *notifier);
>  void qemu_system_debug_request(void);
>  void qemu_system_vmstop_request(RunState reason);
>  int qemu_shutdown_requested_get(void);
> -int qemu_reset_requested_get(void);
> +ResetType qemu_reset_requested_get(void);
> +ResetType qemu_last_reset_get(void);
>  void qemu_system_killed(int signal, pid_t pid);
>  void qemu_devices_reset(void);
> -void qemu_system_reset(bool report);
> +void qemu_system_reset(ResetType type, bool report);
>  
>  void qemu_add_exit_notifier(Notifier *notify);
>  void qemu_remove_exit_notifier(Notifier *notify);
> diff --git a/kvm-all.c b/kvm-all.c
> index 04ec2d5..1177ca1 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1599,7 +1599,7 @@ int kvm_cpu_exec(CPUArchState *env)
>              break;
>          case KVM_EXIT_SHUTDOWN:
>              DPRINTF("shutdown\n");
> -            qemu_system_reset_request();
> +            qemu_system_reset_request(QEMU_RESET_HARD);
>              ret = EXCP_INTERRUPT;
>              break;
>          case KVM_EXIT_UNKNOWN:
> diff --git a/qmp.c b/qmp.c
> index 55b056b..34c0429 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -95,7 +95,7 @@ void qmp_stop(Error **errp)
>  
>  void qmp_system_reset(Error **errp)
>  {
> -    qemu_system_reset_request();
> +    qemu_system_reset_request(QEMU_RESET_HARD);
>  }
>  
>  void qmp_system_powerdown(Error **erp)
> diff --git a/qtest.c b/qtest.c
> index 4663a38..61190f4 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -391,7 +391,7 @@ static void qtest_event(void *opaque, int event)
>  
>      switch (event) {
>      case CHR_EVENT_OPENED:
> -        qemu_system_reset(false);
> +        qemu_system_reset(QEMU_RESET_POWERON, false);
>          for (i = 0; i < ARRAY_SIZE(irq_levels); i++) {
>              irq_levels[i] = 0;
>          }
> diff --git a/savevm.c b/savevm.c
> index a8a53ef..035901b 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2299,7 +2299,7 @@ int load_vmstate(const char *name)
>          return -EINVAL;
>      }
>  
> -    qemu_system_reset(VMRESET_SILENT);
> +    qemu_system_reset(QEMU_RESET_WAKEUP, VMRESET_SILENT);
>      ret = qemu_loadvm_state(f);
>  
>      qemu_fclose(f);
> diff --git a/target-i386/excp_helper.c b/target-i386/excp_helper.c
> index 179ea82..2faf53f 100644
> --- a/target-i386/excp_helper.c
> +++ b/target-i386/excp_helper.c
> @@ -64,7 +64,7 @@ static int check_exception(CPUX86State *env, int intno, int *error_code)
>  
>          qemu_log_mask(CPU_LOG_RESET, "Triple fault\n");
>  
> -        qemu_system_reset_request();
> +        qemu_system_reset_request(QEMU_RESET_SOFT);
>          return EXCP_HLT;
>      }
>  #endif
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index d1cb4e2..cc0de0e 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1147,7 +1147,7 @@ static void do_inject_x86_mce(void *data)
>                             " triple fault\n",
>                             cpu->cpu_index);
>              qemu_log_mask(CPU_LOG_RESET, "Triple fault\n");
> -            qemu_system_reset_request();
> +            qemu_system_reset_request(QEMU_RESET_SOFT);
>              return;
>          }
>          if (banks[1] & MCI_STATUS_VAL) {
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 9ebf181..f52a3d6 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1846,7 +1846,7 @@ int kvm_arch_process_async_events(CPUState *cs)
>  
>          if (env->exception_injected == EXCP08_DBLE) {
>              /* this means triple fault */
> -            qemu_system_reset_request();
> +            qemu_system_reset_request(QEMU_RESET_SOFT);
>              env->exit_request = 1;
>              return 0;
>          }
> diff --git a/vl.c b/vl.c
> index c5b0eea..948eb99 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1696,7 +1696,8 @@ typedef struct QEMUResetEntry {
>  
>  static QTAILQ_HEAD(reset_handlers, QEMUResetEntry) reset_handlers =
>      QTAILQ_HEAD_INITIALIZER(reset_handlers);
> -static int reset_requested;
> +static ResetType reset_requested;
> +static ResetType last_reset = QEMU_RESET_POWERON;
>  static int shutdown_requested, shutdown_signal = -1;
>  static pid_t shutdown_pid;
>  static int powerdown_requested;
> @@ -1717,11 +1718,16 @@ int qemu_shutdown_requested_get(void)
>      return shutdown_requested;
>  }
>  
> -int qemu_reset_requested_get(void)
> +ResetType qemu_reset_requested_get(void)
>  {
>      return reset_requested;
>  }
>  
> +ResetType qemu_last_reset_get(void)
> +{
> +    return last_reset;
> +}
> +
>  static int qemu_shutdown_requested(void)
>  {
>      int r = shutdown_requested;
> @@ -1745,10 +1751,10 @@ static void qemu_kill_report(void)
>      }
>  }
>  
> -static int qemu_reset_requested(void)
> +static ResetType qemu_reset_requested(void)
>  {
> -    int r = reset_requested;
> -    reset_requested = 0;
> +    ResetType r = reset_requested;
> +    reset_requested = QEMU_RESET_NONE;
>      return r;
>  }
>  
> @@ -1824,8 +1830,9 @@ void qemu_devices_reset(void)
>      }
>  }
>  
> -void qemu_system_reset(bool report)
> +void qemu_system_reset(ResetType type, bool report)
>  {
> +    last_reset = type;
>      if (current_machine && current_machine->reset) {
>          current_machine->reset();
>      } else {
> @@ -1837,12 +1844,12 @@ void qemu_system_reset(bool report)
>      cpu_synchronize_all_post_reset();
>  }
>  
> -void qemu_system_reset_request(void)
> +void qemu_system_reset_request(ResetType type)
>  {
>      if (no_reboot) {
>          shutdown_requested = 1;
>      } else {
> -        reset_requested = 1;
> +        reset_requested = type;
>      }
>      cpu_stop_current();
>      qemu_notify_event();
> @@ -1945,6 +1952,7 @@ void qemu_system_vmstop_request(RunState state)
>  static bool main_loop_should_exit(void)
>  {
>      RunState r;
> +    ResetType rt;
>      if (qemu_debug_requested()) {
>          vm_stop(RUN_STATE_DEBUG);
>      }
> @@ -1960,10 +1968,11 @@ static bool main_loop_should_exit(void)
>              return true;
>          }
>      }
> -    if (qemu_reset_requested()) {
> +    rt = qemu_reset_requested();
> +    if (rt) {
>          pause_all_vcpus();
>          cpu_synchronize_all_states();
> -        qemu_system_reset(VMRESET_REPORT);
> +        qemu_system_reset(rt, VMRESET_REPORT);
>          resume_all_vcpus();
>          if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>              runstate_check(RUN_STATE_SHUTDOWN)) {
> @@ -1973,7 +1982,7 @@ static bool main_loop_should_exit(void)
>      if (qemu_wakeup_requested()) {
>          pause_all_vcpus();
>          cpu_synchronize_all_states();
> -        qemu_system_reset(VMRESET_SILENT);
> +        qemu_system_reset(QEMU_RESET_WAKEUP, VMRESET_SILENT);
>          resume_all_vcpus();
>          monitor_protocol_event(QEVENT_WAKEUP, NULL);
>      }
> @@ -4308,7 +4317,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
>      qemu_run_machine_init_done_notifiers();
>  
> -    qemu_system_reset(VMRESET_SILENT);
> +    qemu_system_reset(QEMU_RESET_POWERON, VMRESET_SILENT);
>      if (loadvm) {
>          if (load_vmstate(loadvm) < 0) {
>              autostart = 0;
> diff --git a/xen-all.c b/xen-all.c
> index 110f958..4be465e 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -883,11 +883,13 @@ static void cpu_handle_ioreq(void *opaque)
>           * causes Xen to powerdown the domain.
>           */
>          if (runstate_is_running()) {
> +            ResetType rt;
>              if (qemu_shutdown_requested_get()) {
>                  destroy_hvm_domain(false);
>              }
> -            if (qemu_reset_requested_get()) {
> -                qemu_system_reset(VMRESET_REPORT);
> +            rt = qemu_reset_requested_get();
> +            if (rt) {
> +                qemu_system_reset(rt, VMRESET_REPORT);
>                  destroy_hvm_domain(true);
>              }
>          }
>
>
> -- 
> David Woodhouse                            Open Source Technology Centre
> David.Woodhouse@intel.com                              Intel Corporation

On Tue, 2013-02-19 at 14:08 +0000, Peter Maydell wrote:
> On 19 February 2013 13:58, David Woodhouse <dwmw2@infradead.org> wrote:
> > On Tue, 2013-02-19 at 12:04 +0000, Peter Maydell wrote:
> >> I'm dubious about this. At the moment we have one set of reset
> >> semantics, which is "this works as if you yanked the power to
> >> the machine and plugged it back in again".
> >
> > Except when it doesn't....
> 
> If they are reacting to QEMU reset by doing anything other than
> "reset to poweron state" then that reset function is broken and
> needs fixing. 

Be careful what you wish for :)

By this logic, anything that calls qemu_system_reset_request() when it
isn't asking for a full power-cycle reset is broken, and that includes
everything from triple-fault handling to keyboard controller to...
fairly much everything else.

So yes, I could do the easy thing and just go ahead and
*unconditionally* reset the PAM registers in i440fx_init().

We could stick a reset handler in place to wipe the whole of the guest's
physical memory too, just to make the point :)

> If there are other types of reset that need to
> be modelled than power on reset then you need to actually
> model the power controller, reset lines, etc, or at least a
> sufficient subset of them for your purposes.

Excellent. Well, *mine* is a full power-on reset so I'm fine, thanks :)

But we've just declared fairly much every other reset trigger to be
broken.