Re: [Qemu-devel] [PATCH v3] PIIX3: reset the VM when the Reset Control Register's RCPU bit gets set

[Stefan Hajnoczi]

On Thu, Jan 24, 2013 at 10:31 AM, Laszlo Ersek <address@hidden> 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 <address@hidden>
> ---
> 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 <address@hidden>