[RFC] PIIX: reset the VM when the Reset Control Register's RCPU bit gets set

From <http://mjg59.dreamwidth.org/3561.html>:

  Traditional PCI config space access is achieved by writing a 32 bit
  value to io port 0xcf8 to identify the bus, device, function and config
  register. Port 0xcfc then contains the register in question. But if you
  write the appropriate pair of magic values to 0xcf9, the machine will
  reboot. Spectacular! And not standardised in any way (certainly not part
  of the PCI spec), so different chipsets may have different requirements.
  Booo.

In the PIIX4 spec, IO port 0xcf9 is specified as the Reset Control
Register. Therefore let's handle single byte writes to offset 1 in the
[0xcf8, 0xcfb] range separately, and interpret the RCPU bit in the value.
(Based on "docs/memory.txt", overlapping regions seem to serve a different
purpose.)

The SRST bit alone could be stateful. However we don't distinguish between
soft & hard reset, hence the SRST bit is neither checked nor saved.

RHBZ reference: 890459

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/piix_pci.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> From <http://mjg59.dreamwidth.org/3561.html>:
>
>   Traditional PCI config space access is achieved by writing a 32 bit
>   value to io port 0xcf8 to identify the bus, device, function and config
>   register. Port 0xcfc then contains the register in question. But if you
>   write the appropriate pair of magic values to 0xcf9, the machine will
>   reboot. Spectacular! And not standardised in any way (certainly not part
>   of the PCI spec), so different chipsets may have different requirements.
>   Booo.
>
> In the PIIX4 spec, IO port 0xcf9 is specified as the Reset Control
> Register. Therefore let's handle single byte writes to offset 1 in the
> [0xcf8, 0xcfb] range separately, and interpret the RCPU bit in the value.
> (Based on "docs/memory.txt", overlapping regions seem to serve a different
> purpose.)
>
> The SRST bit alone could be stateful. However we don't distinguish between
> soft & hard reset, hence the SRST bit is neither checked nor saved.
>
> RHBZ reference: 890459
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/piix_pci.c |   22 +++++++++++++++++++++-
>  1 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 3d79c73..89d694c 100644
> --- a/hw/piix_pci.c
> +++ b/hw/piix_pci.c
> @@ -31,6 +31,7 @@
>  #include "qemu/range.h"
>  #include "xen.h"
>  #include "pam.h"
> +#include "sysemu/sysemu.h"
>
>  /*
>   * I440FX chipset data sheet.
> @@ -180,11 +181,30 @@ static const VMStateDescription vmstate_i440fx = {
>      }
>  };
>
> +static void i440fx_host_config_write(void *opaque, hwaddr addr,
> +                                     uint64_t val, unsigned len)
> +{
> +    if (addr == 1 && len == 1) {
> +        if (val & 4) {
> +            qemu_system_reset_request();
> +        }
> +        return;
> +    }
> +    pci_host_conf_le_ops.write(opaque, addr, val, len);
> +}
> +
> +static MemoryRegionOps i440fx_host_conf_ops = {
> +    .read       = NULL,
> +    .write      = i440fx_host_config_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN
> +};
> +
>  static int i440fx_pcihost_initfn(SysBusDevice *dev)
>  {
>      PCIHostState *s = PCI_HOST_BRIDGE(dev);
>
> -    memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
> +    i440fx_host_conf_ops.read = pci_host_conf_le_ops.read;

It would be cleaner to introduce a new memory region (without this
copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches
accesses to 0xcf9. This may mean that pci_host_config_{read,write}
will need to be exposed.

> +    memory_region_init_io(&s->conf_mem, &i440fx_host_conf_ops, s,
>                            "pci-conf-idx", 4);
>      sysbus_add_io(dev, 0xcf8, &s->conf_mem);
>      sysbus_init_ioports(&s->busdev, 0xcf8, 4);
> --
> 1.7.1
>
>

Hi,

On 01/09/13 22:01, Blue Swirl wrote:
> On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <lersek@redhat.com> wrote:

>> +static void i440fx_host_config_write(void *opaque, hwaddr addr,
>> +                                     uint64_t val, unsigned len)
>> +{
>> +    if (addr == 1 && len == 1) {
>> +        if (val & 4) {
>> +            qemu_system_reset_request();
>> +        }
>> +        return;
>> +    }
>> +    pci_host_conf_le_ops.write(opaque, addr, val, len);
>> +}
>> +
>> +static MemoryRegionOps i440fx_host_conf_ops = {
>> +    .read       = NULL,
>> +    .write      = i440fx_host_config_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN
>> +};
>> +
>>  static int i440fx_pcihost_initfn(SysBusDevice *dev)
>>  {
>>      PCIHostState *s = PCI_HOST_BRIDGE(dev);
>>
>> -    memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
>> +    i440fx_host_conf_ops.read = pci_host_conf_le_ops.read;
> 
> It would be cleaner to introduce a new memory region (without this
> copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches
> accesses to 0xcf9. This may mean that pci_host_config_{read,write}
> will need to be exposed.

Do you mean:

(1) introducing the new "i440fx_host_conf_ops" struct-of-funcptrs with
detached functions (that is, duplicating the guts of
pci_host_config_{read,write} and modifying them, and then registering
s->conf_mem with this "i440fx_host_conf_ops"; or

(2) leaving s->conf_mem as-is, and introducing a sub-region just for
port 0xcf9, with higher visibility priority?

(I don't feel confident about (2), and based on "docs/memory.txt" I
thought that overlapping regions had not been invented for this purpose.)

IOW, are you OK with the explicit offset + access-width based check,
just organized differently, or are you proposing a one-byte-wide subregion?

Thanks!
Laszlo

Hi,

Am 11.01.2013 10:30, schrieb Laszlo Ersek:
> On 01/09/13 22:01, Blue Swirl wrote:
>> On Tue, Jan 8, 2013 at 9:44 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>>> +static void i440fx_host_config_write(void *opaque, hwaddr addr,
>>> +                                     uint64_t val, unsigned len)
>>> +{
>>> +    if (addr == 1 && len == 1) {
>>> +        if (val & 4) {
>>> +            qemu_system_reset_request();
>>> +        }
>>> +        return;
>>> +    }
>>> +    pci_host_conf_le_ops.write(opaque, addr, val, len);
>>> +}
>>> +
>>> +static MemoryRegionOps i440fx_host_conf_ops = {
>>> +    .read       = NULL,
>>> +    .write      = i440fx_host_config_write,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN
>>> +};
>>> +
>>>  static int i440fx_pcihost_initfn(SysBusDevice *dev)
>>>  {
>>>      PCIHostState *s = PCI_HOST_BRIDGE(dev);
>>>
>>> -    memory_region_init_io(&s->conf_mem, &pci_host_conf_le_ops, s,
>>> +    i440fx_host_conf_ops.read = pci_host_conf_le_ops.read;
>>
>> It would be cleaner to introduce a new memory region (without this
>> copying) which passes 0xcf8 and 0xcfc to standard PCI host but catches
>> accesses to 0xcf9. This may mean that pci_host_config_{read,write}
>> will need to be exposed.
> 
> Do you mean:
> 
> (1) introducing the new "i440fx_host_conf_ops" struct-of-funcptrs with
> detached functions (that is, duplicating the guts of
> pci_host_config_{read,write} and modifying them, and then registering
> s->conf_mem with this "i440fx_host_conf_ops"; or
> 
> (2) leaving s->conf_mem as-is, and introducing a sub-region just for
> port 0xcf9, with higher visibility priority?
> 
> (I don't feel confident about (2), and based on "docs/memory.txt" I
> thought that overlapping regions had not been invented for this purpose.)
> 
> IOW, are you OK with the explicit offset + access-width based check,
> just organized differently, or are you proposing a one-byte-wide subregion?

Another option:

(3) leaving s->conf_mem as-is but implementing your own read function as
well that forwards to pci_host_conf_le_ops.read() to avoid this unusual
non-const MemoryRegionOps construct

But I guess Blue meant (2), which should be slightly more performant.

Regards,
Andreas