Patchwork [v3] PIIX3: reset the VM when the Reset Control Register's RCPU bit gets set

login
register
mail settings
Submitter Laszlo Ersek
Date Jan. 24, 2013, 9:31 a.m.
Message ID <1359019880-8693-1-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/215282/
State New
Headers show

Comments

Laszlo Ersek - Jan. 24, 2013, 9:31 a.m.
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 PIIX3 spec, IO port 0xcf9 is specified as the Reset Control
Register. Bit 1 (System Reset, SRST) would normally differentiate between
soft reset and hard reset, but we ignore the difference beyond allowing
the guest to read it back.

RHBZ reference: 890459

This patch introduces the following overlap between the preexistent
"pci-conf-idx" region and the "piix3-reset-control" region just being
added. Partial output from "info mtree":

  I/O
  0000000000000000-000000000000ffff (prio 0, RW): io
    0000000000000cf8-0000000000000cfb (prio 0, RW): pci-conf-idx
    0000000000000cf9-0000000000000cf9 (prio 1, RW): piix3-reset-control

I sanity-checked the patch by booting a RHEL-6.3 guest and found no
problems. I summoned gdb and set a breakpoint on rcr_write() in order to
gather a bit more confidence. Relevant frames of the stack:

  kvm_handle_io (port=3321, data=0x7f3f5f3de000, direction=1, size=1,
                 count=1)                                 [kvm-all.c:1422]
    cpu_outb (addr=3321, val=6 '\006')                      [ioport.c:289]
      ioport_write (index=0, address=3321, data=6)           [ioport.c:83]
        ioport_writeb_thunk (opaque=0x7f3f622c4680, addr=3321, data=6)
                                                            [ioport.c:212]
          memory_region_iorange_write (iorange=0x7f3f622c4680, offset=0,
                                       width=1, data=6)     [memory.c:439]
            access_with_adjusted_size (addr=0, value=0x7f3f531fbac0,
                                       size=1, access_size_min=1,
                                       access_size_max=4,
                                       access=0x7f3f5f6e0f90
                                           <memory_region_write_accessor>,
                                       opaque=0x7f3f6227b668)
                                                            [memory.c:364]
              memory_region_write_accessor (opaque=0x7f3f6227b668, addr=0,
                                            value=0x7f3f531fbac0, size=1,
                                            shift=0, mask=255)
                                                            [memory.c:334]
                rcr_write (opaque=0x7f3f6227afb0, addr=0, val=6, len=1)
                                                       [hw/piix_pci.c:498]

The dispatch happens in ioport_write(); "index=0" means byte-wide access:

    static void ioport_write(int index, uint32_t address, uint32_t data)
    {
        static IOPortWriteFunc * const default_func[3] = {
            default_ioport_writeb,
            default_ioport_writew,
            default_ioport_writel
        };
        IOPortWriteFunc *func = ioport_write_table[index][address];
        if (!func)
            func = default_func[index];
        func(ioport_opaque[address], address, data);
    }

The "ioport_write_table" and "ioport_opaque" arrays describe the flattened
IO port space. The first array is less interesting (it selects a thunk
function). The "ioport_opaque" array is interesting because it decides how
writing to the port is implemented ultimately.

4-byte wide access to 0xcf8 (pci-conf-idx):

  (gdb) print ioport_write_table[2][0xcf8]
  $1 = (IOPortWriteFunc *) 0x7f3f5f6d99ba <ioport_writel_thunk>

  (gdb) print \
        ((struct MemoryRegionIORange*)ioport_opaque[0xcf8])->mr->ops.write
  $2 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
       0x7f3f5f5575cb <pci_host_config_write>

1-byte wide access to 0xcf9 (piix3-reset-control):

  (gdb) print ioport_write_table[0][0xcf9]
  $3 = (IOPortWriteFunc *) 0x7f3f5f6d98d0 <ioport_writeb_thunk>

  (gdb) print \
        ((struct MemoryRegionIORange*)ioport_opaque[0xcf9])->mr->ops.write
  $4 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
       0x7f3f5f6b42f1 <rcr_write>

The higher priority of "piix3-reset-control" ensures that the 0xcf9
entries in ioport_write_table / ioport_opaque will always belong to it,
independently of its relative registration order versus "pci-conf-idx".

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
v2->v3:
- don't touch piix3_post_load(); take the RCR as it comes (Stefan).
  Diff against v2:

  diff --git a/hw/piix_pci.c b/hw/piix_pci.c
  index 38a1027..4c97a84 100644
  --- a/hw/piix_pci.c
  +++ b/hw/piix_pci.c
  @@ -462,7 +462,6 @@ static int piix3_post_load(void *opaque, int version_id)
   {
       PIIX3State *piix3 = opaque;
       piix3_update_irq_levels(piix3);
  -    piix3->rcr &= 2; /* keep System Reset type only */
       return 0;
   }

v1->v2:
- move the RCR into a subsection for migration (Paolo)

 hw/piix_pci.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)
Stefan Hajnoczi - Jan. 25, 2013, 7:55 a.m.
On Thu, Jan 24, 2013 at 10:31 AM, 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 PIIX3 spec, IO port 0xcf9 is specified as the Reset Control
> Register. Bit 1 (System Reset, SRST) would normally differentiate between
> soft reset and hard reset, but we ignore the difference beyond allowing
> the guest to read it back.
>
> RHBZ reference: 890459
>
> This patch introduces the following overlap between the preexistent
> "pci-conf-idx" region and the "piix3-reset-control" region just being
> added. Partial output from "info mtree":
>
>   I/O
>   0000000000000000-000000000000ffff (prio 0, RW): io
>     0000000000000cf8-0000000000000cfb (prio 0, RW): pci-conf-idx
>     0000000000000cf9-0000000000000cf9 (prio 1, RW): piix3-reset-control
>
> I sanity-checked the patch by booting a RHEL-6.3 guest and found no
> problems. I summoned gdb and set a breakpoint on rcr_write() in order to
> gather a bit more confidence. Relevant frames of the stack:
>
>   kvm_handle_io (port=3321, data=0x7f3f5f3de000, direction=1, size=1,
>                  count=1)                                 [kvm-all.c:1422]
>     cpu_outb (addr=3321, val=6 '\006')                      [ioport.c:289]
>       ioport_write (index=0, address=3321, data=6)           [ioport.c:83]
>         ioport_writeb_thunk (opaque=0x7f3f622c4680, addr=3321, data=6)
>                                                             [ioport.c:212]
>           memory_region_iorange_write (iorange=0x7f3f622c4680, offset=0,
>                                        width=1, data=6)     [memory.c:439]
>             access_with_adjusted_size (addr=0, value=0x7f3f531fbac0,
>                                        size=1, access_size_min=1,
>                                        access_size_max=4,
>                                        access=0x7f3f5f6e0f90
>                                            <memory_region_write_accessor>,
>                                        opaque=0x7f3f6227b668)
>                                                             [memory.c:364]
>               memory_region_write_accessor (opaque=0x7f3f6227b668, addr=0,
>                                             value=0x7f3f531fbac0, size=1,
>                                             shift=0, mask=255)
>                                                             [memory.c:334]
>                 rcr_write (opaque=0x7f3f6227afb0, addr=0, val=6, len=1)
>                                                        [hw/piix_pci.c:498]
>
> The dispatch happens in ioport_write(); "index=0" means byte-wide access:
>
>     static void ioport_write(int index, uint32_t address, uint32_t data)
>     {
>         static IOPortWriteFunc * const default_func[3] = {
>             default_ioport_writeb,
>             default_ioport_writew,
>             default_ioport_writel
>         };
>         IOPortWriteFunc *func = ioport_write_table[index][address];
>         if (!func)
>             func = default_func[index];
>         func(ioport_opaque[address], address, data);
>     }
>
> The "ioport_write_table" and "ioport_opaque" arrays describe the flattened
> IO port space. The first array is less interesting (it selects a thunk
> function). The "ioport_opaque" array is interesting because it decides how
> writing to the port is implemented ultimately.
>
> 4-byte wide access to 0xcf8 (pci-conf-idx):
>
>   (gdb) print ioport_write_table[2][0xcf8]
>   $1 = (IOPortWriteFunc *) 0x7f3f5f6d99ba <ioport_writel_thunk>
>
>   (gdb) print \
>         ((struct MemoryRegionIORange*)ioport_opaque[0xcf8])->mr->ops.write
>   $2 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
>        0x7f3f5f5575cb <pci_host_config_write>
>
> 1-byte wide access to 0xcf9 (piix3-reset-control):
>
>   (gdb) print ioport_write_table[0][0xcf9]
>   $3 = (IOPortWriteFunc *) 0x7f3f5f6d98d0 <ioport_writeb_thunk>
>
>   (gdb) print \
>         ((struct MemoryRegionIORange*)ioport_opaque[0xcf9])->mr->ops.write
>   $4 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
>        0x7f3f5f6b42f1 <rcr_write>
>
> The higher priority of "piix3-reset-control" ensures that the 0xcf9
> entries in ioport_write_table / ioport_opaque will always belong to it,
> independently of its relative registration order versus "pci-conf-idx".
>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> v2->v3:
> - don't touch piix3_post_load(); take the RCR as it comes (Stefan).
>   Diff against v2:
>
>   diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>   index 38a1027..4c97a84 100644
>   --- a/hw/piix_pci.c
>   +++ b/hw/piix_pci.c
>   @@ -462,7 +462,6 @@ static int piix3_post_load(void *opaque, int version_id)
>    {
>        PIIX3State *piix3 = opaque;
>        piix3_update_irq_levels(piix3);
>   -    piix3->rcr &= 2; /* keep System Reset type only */
>        return 0;
>    }
>
> v1->v2:
> - move the RCR into a subsection for migration (Paolo)
>
>  hw/piix_pci.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)

Thanks Laszlo!

Reviewed-by: Stefan Hajnoczi <stefanha@gmail.com>
Anthony Liguori - Jan. 25, 2013, 2:18 p.m.
Hi,

Thank you for submitting your patch series.  checkpatch.pl has
detected that one or more of the patches in this series violate
the QEMU coding style.

If you believe this message was sent in error, please ignore it
or respond here with an explanation.

Otherwise, please correct the coding style issues and resubmit a
new version of the patch.

For more information about QEMU coding style, see:

http://git.qemu.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD

Here is the output from checkpatch.pl:

Subject: PIIX3: reset the VM when the Reset Control Register's RCPU bit gets set
ERROR: space prohibited before open square bracket '['
#164: FILE: hw/piix_pci.c:490:
+    .fields = (VMStateField []) {

ERROR: space prohibited before open square bracket '['
#178: FILE: hw/piix_pci.c:509:
+    .subsections = (VMStateSubsection []) {

total: 2 errors, 0 warnings, 112 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.



Regards,

Anthony Liguori
Laszlo Ersek - Jan. 25, 2013, 5:20 p.m.
Hi,

On 01/25/13 15:18, Anthony Liguori wrote:

> If you believe this message was sent in error, please ignore it
> or respond here with an explanation.
> 
> Otherwise, please correct the coding style issues and resubmit a
> new version of the patch.
> 
> For more information about QEMU coding style, see:
> 
> http://git.qemu.org/?p=qemu.git;a=blob_plain;f=CODING_STYLE;hb=HEAD
> 
> Here is the output from checkpatch.pl:
> 
> Subject: PIIX3: reset the VM when the Reset Control Register's RCPU bit gets set
> ERROR: space prohibited before open square bracket '['
> #164: FILE: hw/piix_pci.c:490:
> +    .fields = (VMStateField []) {
> 
> ERROR: space prohibited before open square bracket '['
> #178: FILE: hw/piix_pci.c:509:
> +    .subsections = (VMStateSubsection []) {
> 
> total: 2 errors, 0 warnings, 112 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.

The subscript operator [] is used here in the type name of a compound
literal which is probably not what the script intends to catch.

  1945  # check for spacing round square brackets; allowed:
  1946  #  1. with a type on the left -- int [] a;

The code in question was copied from "docs/migration.txt" and seems to
match existing practice:

  git grep -E '(VMStateField|VMStateSubsection) \[]' \
  | wc -l

  139

Thanks
Laszlo
Peter Maydell - Jan. 25, 2013, 5:23 p.m.
On 25 January 2013 17:20, Laszlo Ersek <lersek@redhat.com> wrote:
> The subscript operator [] is used here in the type name of a compound
> literal which is probably not what the script intends to catch.
>
>   1945  # check for spacing round square brackets; allowed:
>   1946  #  1. with a type on the left -- int [] a;
>
> The code in question was copied from "docs/migration.txt" and seems to
> match existing practice:
>
>   git grep -E '(VMStateField|VMStateSubsection) \[]' \
>   | wc -l
>
>   139

On the other hand the usage without the space is in the
majority:

git grep -E '(VMStateField|VMStateSubsection)\[]' | wc -l
     218

-- PMM
Laszlo Ersek - Jan. 25, 2013, 9:38 p.m.
On 01/25/13 18:23, Peter Maydell wrote:
> On 25 January 2013 17:20, Laszlo Ersek <lersek@redhat.com> wrote:
>> The subscript operator [] is used here in the type name of a compound
>> literal which is probably not what the script intends to catch.
>>
>>   1945  # check for spacing round square brackets; allowed:
>>   1946  #  1. with a type on the left -- int [] a;
>>
>> The code in question was copied from "docs/migration.txt" and seems to
>> match existing practice:
>>
>>   git grep -E '(VMStateField|VMStateSubsection) \[]' \
>>   | wc -l
>>
>>   139
> 
> On the other hand the usage without the space is in the
> majority:
> 
> git grep -E '(VMStateField|VMStateSubsection)\[]' | wc -l
>      218

OK. I propose to take v3 as-is, and in accordance with
<https://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg04116.html>, point
(3), I'll post a patch that eradicates the space from code & docs alike
(those 139  occurrences, plus the two new one in my v3).

(Although I know Stefan doesn't like reformatting patches because they
shadow "real" commits in git blame.)

Thanks
Laszlo
Michael S. Tsirkin - Jan. 28, 2013, 2:59 p.m.
On Thu, Jan 24, 2013 at 10:31:20AM +0100, Laszlo Ersek 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 PIIX3 spec, IO port 0xcf9 is specified as the Reset Control
> Register. Bit 1 (System Reset, SRST) would normally differentiate between
> soft reset and hard reset, but we ignore the difference beyond allowing
> the guest to read it back.
> 
> RHBZ reference: 890459
> 
> This patch introduces the following overlap between the preexistent
> "pci-conf-idx" region and the "piix3-reset-control" region just being
> added. Partial output from "info mtree":
> 
>   I/O
>   0000000000000000-000000000000ffff (prio 0, RW): io
>     0000000000000cf8-0000000000000cfb (prio 0, RW): pci-conf-idx
>     0000000000000cf9-0000000000000cf9 (prio 1, RW): piix3-reset-control
> 
> I sanity-checked the patch by booting a RHEL-6.3 guest and found no
> problems. I summoned gdb and set a breakpoint on rcr_write() in order to
> gather a bit more confidence. Relevant frames of the stack:
> 
>   kvm_handle_io (port=3321, data=0x7f3f5f3de000, direction=1, size=1,
>                  count=1)                                 [kvm-all.c:1422]
>     cpu_outb (addr=3321, val=6 '\006')                      [ioport.c:289]
>       ioport_write (index=0, address=3321, data=6)           [ioport.c:83]
>         ioport_writeb_thunk (opaque=0x7f3f622c4680, addr=3321, data=6)
>                                                             [ioport.c:212]
>           memory_region_iorange_write (iorange=0x7f3f622c4680, offset=0,
>                                        width=1, data=6)     [memory.c:439]
>             access_with_adjusted_size (addr=0, value=0x7f3f531fbac0,
>                                        size=1, access_size_min=1,
>                                        access_size_max=4,
>                                        access=0x7f3f5f6e0f90
>                                            <memory_region_write_accessor>,
>                                        opaque=0x7f3f6227b668)
>                                                             [memory.c:364]
>               memory_region_write_accessor (opaque=0x7f3f6227b668, addr=0,
>                                             value=0x7f3f531fbac0, size=1,
>                                             shift=0, mask=255)
>                                                             [memory.c:334]
>                 rcr_write (opaque=0x7f3f6227afb0, addr=0, val=6, len=1)
>                                                        [hw/piix_pci.c:498]
> 
> The dispatch happens in ioport_write(); "index=0" means byte-wide access:
> 
>     static void ioport_write(int index, uint32_t address, uint32_t data)
>     {
>         static IOPortWriteFunc * const default_func[3] = {
>             default_ioport_writeb,
>             default_ioport_writew,
>             default_ioport_writel
>         };
>         IOPortWriteFunc *func = ioport_write_table[index][address];
>         if (!func)
>             func = default_func[index];
>         func(ioport_opaque[address], address, data);
>     }
> 
> The "ioport_write_table" and "ioport_opaque" arrays describe the flattened
> IO port space. The first array is less interesting (it selects a thunk
> function). The "ioport_opaque" array is interesting because it decides how
> writing to the port is implemented ultimately.
> 
> 4-byte wide access to 0xcf8 (pci-conf-idx):
> 
>   (gdb) print ioport_write_table[2][0xcf8]
>   $1 = (IOPortWriteFunc *) 0x7f3f5f6d99ba <ioport_writel_thunk>
> 
>   (gdb) print \
>         ((struct MemoryRegionIORange*)ioport_opaque[0xcf8])->mr->ops.write
>   $2 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
>        0x7f3f5f5575cb <pci_host_config_write>
> 
> 1-byte wide access to 0xcf9 (piix3-reset-control):
> 
>   (gdb) print ioport_write_table[0][0xcf9]
>   $3 = (IOPortWriteFunc *) 0x7f3f5f6d98d0 <ioport_writeb_thunk>
> 
>   (gdb) print \
>         ((struct MemoryRegionIORange*)ioport_opaque[0xcf9])->mr->ops.write
>   $4 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
>        0x7f3f5f6b42f1 <rcr_write>
> 
> The higher priority of "piix3-reset-control" ensures that the 0xcf9
> entries in ioport_write_table / ioport_opaque will always belong to it,
> independently of its relative registration order versus "pci-conf-idx".
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

I have tweaked whitespace a bit and applied it in my tree.
Thanks!

> ---
> v2->v3:
> - don't touch piix3_post_load(); take the RCR as it comes (Stefan).
>   Diff against v2:
> 
>   diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>   index 38a1027..4c97a84 100644
>   --- a/hw/piix_pci.c
>   +++ b/hw/piix_pci.c
>   @@ -462,7 +462,6 @@ static int piix3_post_load(void *opaque, int version_id)
>    {
>        PIIX3State *piix3 = opaque;
>        piix3_update_irq_levels(piix3);
>   -    piix3->rcr &= 2; /* keep System Reset type only */
>        return 0;
>    }
> 
> v1->v2:
> - move the RCR into a subsection for migration (Paolo)
> 
>  hw/piix_pci.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 3d79c73..4c97a84 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.
> @@ -46,6 +47,12 @@ typedef struct I440FXState {
>  #define XEN_PIIX_NUM_PIRQS      128ULL
>  #define PIIX_PIRQC              0x60
>  
> +/*
> + * Reset Control Register: PCI-accessible ISA-Compatible Register at address
> + * 0xcf9, provided by the PCI/ISA bridge (PIIX3 PCI function 0, 8086:7000).
> + */
> +#define RCR_IOPORT 0xcf9
> +
>  typedef struct PIIX3State {
>      PCIDevice dev;
>  
> @@ -67,6 +74,12 @@ typedef struct PIIX3State {
>  
>      /* This member isn't used. Just for save/load compatibility */
>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
> +
> +    /* Reset Control Register contents */
> +    uint8_t rcr;
> +
> +    /* IO memory region for Reset Control Register (RCR_IOPORT) */
> +    MemoryRegion rcr_mem;
>  } PIIX3State;
>  
>  struct PCII440FXState {
> @@ -442,6 +455,7 @@ static void piix3_reset(void *opaque)
>      pci_conf[0xae] = 0x00;
>  
>      d->pic_levels = 0;
> +    d->rcr = 0;
>  }
>  
>  static int piix3_post_load(void *opaque, int version_id)
> @@ -462,6 +476,23 @@ static void piix3_pre_save(void *opaque)
>      }
>  }
>  
> +static bool piix3_rcr_needed(void *opaque)
> +{
> +    PIIX3State *piix3 = opaque;
> +
> +    return (piix3->rcr != 0);
> +}
> +
> +static const VMStateDescription vmstate_piix3_rcr = {
> +    .name = "PIIX3/rcr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_UINT8(rcr, PIIX3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_piix3 = {
>      .name = "PIIX3",
>      .version_id = 3,
> @@ -474,14 +505,51 @@ static const VMStateDescription vmstate_piix3 = {
>          VMSTATE_INT32_ARRAY_V(pci_irq_levels_vmstate, PIIX3State,
>                                PIIX_NUM_PIRQS, 3),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection []) {
> +        {
> +            .vmsd = &vmstate_piix3_rcr,
> +            .needed = piix3_rcr_needed,
> +        },
> +        { 0 }
>      }
>  };
>  
> +
> +static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
> +{
> +    PIIX3State *d = opaque;
> +
> +    if (val & 4) {
> +        qemu_system_reset_request();
> +        return;
> +    }
> +    d->rcr = val & 2; /* keep System Reset type only */
> +}
> +
> +static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned len)
> +{
> +    PIIX3State *d = opaque;
> +
> +    return d->rcr;
> +}
> +
> +static const MemoryRegionOps rcr_ops = {
> +    .read = rcr_read,
> +    .write = rcr_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN
> +};
> +
>  static int piix3_initfn(PCIDevice *dev)
>  {
>      PIIX3State *d = DO_UPCAST(PIIX3State, dev, dev);
>  
>      isa_bus_new(&d->dev.qdev, pci_address_space_io(dev));
> +
> +    memory_region_init_io(&d->rcr_mem, &rcr_ops, d, "piix3-reset-control", 1);
> +    memory_region_add_subregion_overlap(pci_address_space_io(dev), RCR_IOPORT,
> +                                        &d->rcr_mem, 1);
> +
>      qemu_register_reset(piix3_reset, d);
>      return 0;
>  }
> -- 
> 1.7.1
>
Kevin Wolf - Jan. 28, 2013, 3:04 p.m.
Am 25.01.2013 18:23, schrieb Peter Maydell:
> On 25 January 2013 17:20, Laszlo Ersek <lersek@redhat.com> wrote:
>> The subscript operator [] is used here in the type name of a compound
>> literal which is probably not what the script intends to catch.
>>
>>   1945  # check for spacing round square brackets; allowed:
>>   1946  #  1. with a type on the left -- int [] a;
>>
>> The code in question was copied from "docs/migration.txt" and seems to
>> match existing practice:
>>
>>   git grep -E '(VMStateField|VMStateSubsection) \[]' \
>>   | wc -l
>>
>>   139
> 
> On the other hand the usage without the space is in the
> majority:
> 
> git grep -E '(VMStateField|VMStateSubsection)\[]' | wc -l
>      218

Means that it's about 60:40. Shouldn't we simply allow both variants in
such cases?

Kevin
Paolo Bonzini - Feb. 19, 2013, 12:46 p.m.
Il 24/01/2013 10:31, Laszlo Ersek ha scritto:
> 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 PIIX3 spec, IO port 0xcf9 is specified as the Reset Control
> Register. Bit 1 (System Reset, SRST) would normally differentiate between
> soft reset and hard reset, but we ignore the difference beyond allowing
> the guest to read it back.

Could you implement it for lpc_ich9.c too?

You can find it in the ICH9 spec on page 486
(http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-9-datasheet.pdf).

Paolo

> RHBZ reference: 890459
> 
> This patch introduces the following overlap between the preexistent
> "pci-conf-idx" region and the "piix3-reset-control" region just being
> added. Partial output from "info mtree":
> 
>   I/O
>   0000000000000000-000000000000ffff (prio 0, RW): io
>     0000000000000cf8-0000000000000cfb (prio 0, RW): pci-conf-idx
>     0000000000000cf9-0000000000000cf9 (prio 1, RW): piix3-reset-control
> 
> I sanity-checked the patch by booting a RHEL-6.3 guest and found no
> problems. I summoned gdb and set a breakpoint on rcr_write() in order to
> gather a bit more confidence. Relevant frames of the stack:
> 
>   kvm_handle_io (port=3321, data=0x7f3f5f3de000, direction=1, size=1,
>                  count=1)                                 [kvm-all.c:1422]
>     cpu_outb (addr=3321, val=6 '\006')                      [ioport.c:289]
>       ioport_write (index=0, address=3321, data=6)           [ioport.c:83]
>         ioport_writeb_thunk (opaque=0x7f3f622c4680, addr=3321, data=6)
>                                                             [ioport.c:212]
>           memory_region_iorange_write (iorange=0x7f3f622c4680, offset=0,
>                                        width=1, data=6)     [memory.c:439]
>             access_with_adjusted_size (addr=0, value=0x7f3f531fbac0,
>                                        size=1, access_size_min=1,
>                                        access_size_max=4,
>                                        access=0x7f3f5f6e0f90
>                                            <memory_region_write_accessor>,
>                                        opaque=0x7f3f6227b668)
>                                                             [memory.c:364]
>               memory_region_write_accessor (opaque=0x7f3f6227b668, addr=0,
>                                             value=0x7f3f531fbac0, size=1,
>                                             shift=0, mask=255)
>                                                             [memory.c:334]
>                 rcr_write (opaque=0x7f3f6227afb0, addr=0, val=6, len=1)
>                                                        [hw/piix_pci.c:498]
> 
> The dispatch happens in ioport_write(); "index=0" means byte-wide access:
> 
>     static void ioport_write(int index, uint32_t address, uint32_t data)
>     {
>         static IOPortWriteFunc * const default_func[3] = {
>             default_ioport_writeb,
>             default_ioport_writew,
>             default_ioport_writel
>         };
>         IOPortWriteFunc *func = ioport_write_table[index][address];
>         if (!func)
>             func = default_func[index];
>         func(ioport_opaque[address], address, data);
>     }
> 
> The "ioport_write_table" and "ioport_opaque" arrays describe the flattened
> IO port space. The first array is less interesting (it selects a thunk
> function). The "ioport_opaque" array is interesting because it decides how
> writing to the port is implemented ultimately.
> 
> 4-byte wide access to 0xcf8 (pci-conf-idx):
> 
>   (gdb) print ioport_write_table[2][0xcf8]
>   $1 = (IOPortWriteFunc *) 0x7f3f5f6d99ba <ioport_writel_thunk>
> 
>   (gdb) print \
>         ((struct MemoryRegionIORange*)ioport_opaque[0xcf8])->mr->ops.write
>   $2 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
>        0x7f3f5f5575cb <pci_host_config_write>
> 
> 1-byte wide access to 0xcf9 (piix3-reset-control):
> 
>   (gdb) print ioport_write_table[0][0xcf9]
>   $3 = (IOPortWriteFunc *) 0x7f3f5f6d98d0 <ioport_writeb_thunk>
> 
>   (gdb) print \
>         ((struct MemoryRegionIORange*)ioport_opaque[0xcf9])->mr->ops.write
>   $4 = (void (*)(void *, hwaddr, uint64_t, unsigned int))
>        0x7f3f5f6b42f1 <rcr_write>
> 
> The higher priority of "piix3-reset-control" ensures that the 0xcf9
> entries in ioport_write_table / ioport_opaque will always belong to it,
> independently of its relative registration order versus "pci-conf-idx".
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> v2->v3:
> - don't touch piix3_post_load(); take the RCR as it comes (Stefan).
>   Diff against v2:
> 
>   diff --git a/hw/piix_pci.c b/hw/piix_pci.c
>   index 38a1027..4c97a84 100644
>   --- a/hw/piix_pci.c
>   +++ b/hw/piix_pci.c
>   @@ -462,7 +462,6 @@ static int piix3_post_load(void *opaque, int version_id)
>    {
>        PIIX3State *piix3 = opaque;
>        piix3_update_irq_levels(piix3);
>   -    piix3->rcr &= 2; /* keep System Reset type only */
>        return 0;
>    }
> 
> v1->v2:
> - move the RCR into a subsection for migration (Paolo)
> 
>  hw/piix_pci.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/piix_pci.c b/hw/piix_pci.c
> index 3d79c73..4c97a84 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.
> @@ -46,6 +47,12 @@ typedef struct I440FXState {
>  #define XEN_PIIX_NUM_PIRQS      128ULL
>  #define PIIX_PIRQC              0x60
>  
> +/*
> + * Reset Control Register: PCI-accessible ISA-Compatible Register at address
> + * 0xcf9, provided by the PCI/ISA bridge (PIIX3 PCI function 0, 8086:7000).
> + */
> +#define RCR_IOPORT 0xcf9
> +
>  typedef struct PIIX3State {
>      PCIDevice dev;
>  
> @@ -67,6 +74,12 @@ typedef struct PIIX3State {
>  
>      /* This member isn't used. Just for save/load compatibility */
>      int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
> +
> +    /* Reset Control Register contents */
> +    uint8_t rcr;
> +
> +    /* IO memory region for Reset Control Register (RCR_IOPORT) */
> +    MemoryRegion rcr_mem;
>  } PIIX3State;
>  
>  struct PCII440FXState {
> @@ -442,6 +455,7 @@ static void piix3_reset(void *opaque)
>      pci_conf[0xae] = 0x00;
>  
>      d->pic_levels = 0;
> +    d->rcr = 0;
>  }
>  
>  static int piix3_post_load(void *opaque, int version_id)
> @@ -462,6 +476,23 @@ static void piix3_pre_save(void *opaque)
>      }
>  }
>  
> +static bool piix3_rcr_needed(void *opaque)
> +{
> +    PIIX3State *piix3 = opaque;
> +
> +    return (piix3->rcr != 0);
> +}
> +
> +static const VMStateDescription vmstate_piix3_rcr = {
> +    .name = "PIIX3/rcr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField []) {
> +        VMSTATE_UINT8(rcr, PIIX3State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_piix3 = {
>      .name = "PIIX3",
>      .version_id = 3,
> @@ -474,14 +505,51 @@ static const VMStateDescription vmstate_piix3 = {
>          VMSTATE_INT32_ARRAY_V(pci_irq_levels_vmstate, PIIX3State,
>                                PIIX_NUM_PIRQS, 3),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection []) {
> +        {
> +            .vmsd = &vmstate_piix3_rcr,
> +            .needed = piix3_rcr_needed,
> +        },
> +        { 0 }
>      }
>  };
>  
> +
> +static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
> +{
> +    PIIX3State *d = opaque;
> +
> +    if (val & 4) {
> +        qemu_system_reset_request();
> +        return;
> +    }
> +    d->rcr = val & 2; /* keep System Reset type only */
> +}
> +
> +static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned len)
> +{
> +    PIIX3State *d = opaque;
> +
> +    return d->rcr;
> +}
> +
> +static const MemoryRegionOps rcr_ops = {
> +    .read = rcr_read,
> +    .write = rcr_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN
> +};
> +
>  static int piix3_initfn(PCIDevice *dev)
>  {
>      PIIX3State *d = DO_UPCAST(PIIX3State, dev, dev);
>  
>      isa_bus_new(&d->dev.qdev, pci_address_space_io(dev));
> +
> +    memory_region_init_io(&d->rcr_mem, &rcr_ops, d, "piix3-reset-control", 1);
> +    memory_region_add_subregion_overlap(pci_address_space_io(dev), RCR_IOPORT,
> +                                        &d->rcr_mem, 1);
> +
>      qemu_register_reset(piix3_reset, d);
>      return 0;
>  }
>
Laszlo Ersek - Feb. 19, 2013, 2:57 p.m.
On 02/19/13 13:46, Paolo Bonzini wrote:
> Il 24/01/2013 10:31, Laszlo Ersek ha scritto:
>> 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 PIIX3 spec, IO port 0xcf9 is specified as the Reset Control
>> Register. Bit 1 (System Reset, SRST) would normally differentiate between
>> soft reset and hard reset, but we ignore the difference beyond allowing
>> the guest to read it back.
> 
> Could you implement it for lpc_ich9.c too?

Yes, I'd like to try, but...

> 
> You can find it in the ICH9 spec on page 486
> (http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-9-datasheet.pdf).

(Thanks for the link. Interestingly, "wget" got "403 Forbidden". I
googled the filename and then found it under the exact same URL. Intel's
webserver probably insists on a cookie or some Referer.)

... earlier Michael pointed out to me that a shared handler for the
[0xcf8, 0xcfc) range would be preferred over the overlapping regions.
(Which makes me recall my RFC version of the patch.) Since RST_CNT on
the ICH9 is also at 0xcf9, I assume I should fix up the PIIX3 first and
then follow the PIIX3 impl. in ICH9.

I could even attempt adding the "hard reset out" thing you mention in
<http://thread.gmane.org/gmane.comp.emulators.qemu/195351/focus=195358>.
(Full emulation of PCIRST# seems "slightly" complex... :))

Michael, what do you think? :)

Thanks
Laszlo
Paolo Bonzini - Feb. 19, 2013, 3:02 p.m.
Il 19/02/2013 15:57, Laszlo Ersek ha scritto:
>> > 
>> > You can find it in the ICH9 spec on page 486
>> > (http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-9-datasheet.pdf).
> (Thanks for the link. Interestingly, "wget" got "403 Forbidden". I
> googled the filename and then found it under the exact same URL. Intel's
> webserver probably insists on a cookie or some Referer.)
> 
> ... earlier Michael pointed out to me that a shared handler for the
> [0xcf8, 0xcfc) range would be preferred over the overlapping regions.
> (Which makes me recall my RFC version of the patch.) Since RST_CNT on
> the ICH9 is also at 0xcf9, I assume I should fix up the PIIX3 first and
> then follow the PIIX3 impl. in ICH9.

I actually thought the same, but it is really weird how it works in real
hardware.

0xCF8 and 0xCFC are handled by the PCI host (part of the i440FX aka
NorthBridge), while 0xCF9 are handled by PIIX3.  You cannot find 0xCF8
and 0xCFC in the PIIX3 and ICH9 datasheets, you cannot find 0xCF9 in the
i440FX datasheet (didn't check Q35).

So it looks like these are _really_ overlapping regions in hardware.

> I could even attempt adding the "hard reset out" thing you mention in
> <http://thread.gmane.org/gmane.comp.emulators.qemu/195351/focus=195358>.
> (Full emulation of PCIRST# seems "slightly" complex... :))

David beat you to that. :)

Paolo

> Michael, what do you think? :)
Michael S. Tsirkin - Feb. 19, 2013, 3:15 p.m.
On Tue, Feb 19, 2013 at 03:57:19PM +0100, Laszlo Ersek wrote:
> On 02/19/13 13:46, Paolo Bonzini wrote:
> > Il 24/01/2013 10:31, Laszlo Ersek ha scritto:
> >> 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 PIIX3 spec, IO port 0xcf9 is specified as the Reset Control
> >> Register. Bit 1 (System Reset, SRST) would normally differentiate between
> >> soft reset and hard reset, but we ignore the difference beyond allowing
> >> the guest to read it back.
> > 
> > Could you implement it for lpc_ich9.c too?
> 
> Yes, I'd like to try, but...
> 
> > 
> > You can find it in the ICH9 spec on page 486
> > (http://www.intel.com/content/dam/doc/datasheet/io-controller-hub-9-datasheet.pdf).
> 
> (Thanks for the link. Interestingly, "wget" got "403 Forbidden". I
> googled the filename and then found it under the exact same URL. Intel's
> webserver probably insists on a cookie or some Referer.)
> 
> ... earlier Michael pointed out to me that a shared handler for the
> [0xcf8, 0xcfc) range would be preferred over the overlapping regions.
> (Which makes me recall my RFC version of the patch.) Since RST_CNT on
> the ICH9 is also at 0xcf9, I assume I should fix up the PIIX3 first and
> then follow the PIIX3 impl. in ICH9.
> 
> I could even attempt adding the "hard reset out" thing you mention in
> <http://thread.gmane.org/gmane.comp.emulators.qemu/195351/focus=195358>.
> (Full emulation of PCIRST# seems "slightly" complex... :))
> 
> Michael, what do you think? :)
> 
> Thanks
> Laszlo

Yes I think it's the cleanest way to do this.
Though, it was Blue Swirl that pushed for the separate handlers right?
I'm not too picky, separate handlers seem to work ATM
even though I think it's more a bug than a feature.
Laszlo Ersek - Feb. 19, 2013, 3:45 p.m.
On 02/19/13 16:15, Michael S. Tsirkin wrote:
> On Tue, Feb 19, 2013 at 03:57:19PM +0100, Laszlo Ersek wrote:

>> ... earlier Michael pointed out to me that a shared handler for the
>> [0xcf8, 0xcfc) range would be preferred over the overlapping regions.
>> (Which makes me recall my RFC version of the patch.) Since RST_CNT on
>> the ICH9 is also at 0xcf9, I assume I should fix up the PIIX3 first and
>> then follow the PIIX3 impl. in ICH9.

> Yes I think it's the cleanest way to do this.
> Though, it was Blue Swirl that pushed for the separate handlers right?

Yes I think it was his recommendation:
http://thread.gmane.org/gmane.comp.emulators.qemu/187502/focus=187736
http://thread.gmane.org/gmane.comp.emulators.qemu/187502/focus=188318

> I'm not too picky, separate handlers seem to work ATM
> even though I think it's more a bug than a feature.

OK, then I'll assume you're not going to NAK the ICH9 patch (to be
written) just because of its use of overlapping regions (if it works at
all of course).

Thanks!
Laszlo
Michael S. Tsirkin - Feb. 19, 2013, 4:44 p.m.
On Tue, Feb 19, 2013 at 04:45:52PM +0100, Laszlo Ersek wrote:
> On 02/19/13 16:15, Michael S. Tsirkin wrote:
> > On Tue, Feb 19, 2013 at 03:57:19PM +0100, Laszlo Ersek wrote:
> 
> >> ... earlier Michael pointed out to me that a shared handler for the
> >> [0xcf8, 0xcfc) range would be preferred over the overlapping regions.
> >> (Which makes me recall my RFC version of the patch.) Since RST_CNT on
> >> the ICH9 is also at 0xcf9, I assume I should fix up the PIIX3 first and
> >> then follow the PIIX3 impl. in ICH9.
> 
> > Yes I think it's the cleanest way to do this.
> > Though, it was Blue Swirl that pushed for the separate handlers right?
> 
> Yes I think it was his recommendation:
> http://thread.gmane.org/gmane.comp.emulators.qemu/187502/focus=187736
> http://thread.gmane.org/gmane.comp.emulators.qemu/187502/focus=188318
> 
> > I'm not too picky, separate handlers seem to work ATM
> > even though I think it's more a bug than a feature.
> 
> OK, then I'll assume you're not going to NAK the ICH9 patch (to be
> written) just because of its use of overlapping regions (if it works at
> all of course).
> 
> Thanks!
> Laszlo

Yea, I don't mind.

Patch

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index 3d79c73..4c97a84 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.
@@ -46,6 +47,12 @@  typedef struct I440FXState {
 #define XEN_PIIX_NUM_PIRQS      128ULL
 #define PIIX_PIRQC              0x60
 
+/*
+ * Reset Control Register: PCI-accessible ISA-Compatible Register at address
+ * 0xcf9, provided by the PCI/ISA bridge (PIIX3 PCI function 0, 8086:7000).
+ */
+#define RCR_IOPORT 0xcf9
+
 typedef struct PIIX3State {
     PCIDevice dev;
 
@@ -67,6 +74,12 @@  typedef struct PIIX3State {
 
     /* This member isn't used. Just for save/load compatibility */
     int32_t pci_irq_levels_vmstate[PIIX_NUM_PIRQS];
+
+    /* Reset Control Register contents */
+    uint8_t rcr;
+
+    /* IO memory region for Reset Control Register (RCR_IOPORT) */
+    MemoryRegion rcr_mem;
 } PIIX3State;
 
 struct PCII440FXState {
@@ -442,6 +455,7 @@  static void piix3_reset(void *opaque)
     pci_conf[0xae] = 0x00;
 
     d->pic_levels = 0;
+    d->rcr = 0;
 }
 
 static int piix3_post_load(void *opaque, int version_id)
@@ -462,6 +476,23 @@  static void piix3_pre_save(void *opaque)
     }
 }
 
+static bool piix3_rcr_needed(void *opaque)
+{
+    PIIX3State *piix3 = opaque;
+
+    return (piix3->rcr != 0);
+}
+
+static const VMStateDescription vmstate_piix3_rcr = {
+    .name = "PIIX3/rcr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_UINT8(rcr, PIIX3State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_piix3 = {
     .name = "PIIX3",
     .version_id = 3,
@@ -474,14 +505,51 @@  static const VMStateDescription vmstate_piix3 = {
         VMSTATE_INT32_ARRAY_V(pci_irq_levels_vmstate, PIIX3State,
                               PIIX_NUM_PIRQS, 3),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_piix3_rcr,
+            .needed = piix3_rcr_needed,
+        },
+        { 0 }
     }
 };
 
+
+static void rcr_write(void *opaque, hwaddr addr, uint64_t val, unsigned len)
+{
+    PIIX3State *d = opaque;
+
+    if (val & 4) {
+        qemu_system_reset_request();
+        return;
+    }
+    d->rcr = val & 2; /* keep System Reset type only */
+}
+
+static uint64_t rcr_read(void *opaque, hwaddr addr, unsigned len)
+{
+    PIIX3State *d = opaque;
+
+    return d->rcr;
+}
+
+static const MemoryRegionOps rcr_ops = {
+    .read = rcr_read,
+    .write = rcr_write,
+    .endianness = DEVICE_LITTLE_ENDIAN
+};
+
 static int piix3_initfn(PCIDevice *dev)
 {
     PIIX3State *d = DO_UPCAST(PIIX3State, dev, dev);
 
     isa_bus_new(&d->dev.qdev, pci_address_space_io(dev));
+
+    memory_region_init_io(&d->rcr_mem, &rcr_ops, d, "piix3-reset-control", 1);
+    memory_region_add_subregion_overlap(pci_address_space_io(dev), RCR_IOPORT,
+                                        &d->rcr_mem, 1);
+
     qemu_register_reset(piix3_reset, d);
     return 0;
 }