Re: [PATCH v2 02/20] piix4: Add the Reset Control Register - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Li Qiang <liq3ea@gmail.com>
Cc: "Stefano Stabellini" <sstabellini@kernel.org>,
	xen-devel@lists.xenproject.org, "Paul Durrant" <paul@xen.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Qemu Developers" <qemu-devel@nongnu.org>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Hervé Poussineau" <hpoussin@reactos.org>,
	"Aleksandar Markovic" <amarkovic@wavecomp.com>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Anthony Perard" <anthony.perard@citrix.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Aleksandar Rikalo" <aleksandar.rikalo@rt-rk.com>,
	"Aurelien Jarno" <aurelien@aurel32.net>,
	"Richard Henderson" <rth@twiddle.net>
Subject: Re: [PATCH v2 02/20] piix4: Add the Reset Control Register
Date: Mon, 21 Oct 2019 10:37:38 +0200	[thread overview]
Message-ID: <3235e49f-fdd6-5e5c-f60b-44538ff05d76@redhat.com> (raw)
In-Reply-To: <CAKXe6SKQGYj+N04An+t5sP4jVkpEpmmGGfpZxb5VAV+w3owsBw@mail.gmail.com>

On 10/21/19 3:25 AM, Li Qiang wrote:
> 
> 
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 
> 2019年10月18日周五 下午9:50写道：
> 
>     From: Hervé Poussineau <hpoussin@reactos.org
>     <mailto:hpoussin@reactos.org>>
> 
>     The RCR I/O port (0xcf9) is used to generate a hard reset or a soft
>     reset.
> 
>     Acked-by: Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>>
>     Acked-by: Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>>
>     Signed-off-by: Hervé Poussineau <hpoussin@reactos.org
>     <mailto:hpoussin@reactos.org>>
>     Message-Id: <20171216090228.28505-7-hpoussin@reactos.org
>     <mailto:20171216090228.28505-7-hpoussin@reactos.org>>
>     Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com
>     <mailto:amarkovic@wavecomp.com>>
>     [PMD: rebased, updated includes]
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>       hw/isa/piix4.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++---
>       1 file changed, 46 insertions(+), 3 deletions(-)
> 
>     diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
>     index 890d999abf..d0b18e0586 100644
>     --- a/hw/isa/piix4.c
>     +++ b/hw/isa/piix4.c
>     @@ -2,6 +2,7 @@
>        * QEMU PIIX4 PCI Bridge Emulation
>        *
>        * Copyright (c) 2006 Fabrice Bellard
>     + * Copyright (c) 2018 Hervé Poussineau
>        *
>        * Permission is hereby granted, free of charge, to any person
>     obtaining a copy
>        * of this software and associated documentation files (the
>     "Software"), to deal
>     @@ -28,11 +29,17 @@
>       #include "hw/isa/isa.h"
>       #include "hw/sysbus.h"
>       #include "migration/vmstate.h"
>     +#include "sysemu/reset.h"
>     +#include "sysemu/runstate.h"
> 
>       PCIDevice *piix4_dev;
> 
>       typedef struct PIIX4State {
>           PCIDevice dev;
>     +
>     +    /* Reset Control Register */
>     +    MemoryRegion rcr_mem;
>     +    uint8_t rcr;
>       } PIIX4State;
> 
>       #define TYPE_PIIX4_PCI_DEVICE "PIIX4"
>     @@ -87,15 +94,51 @@ static const VMStateDescription vmstate_piix4 = {
>           }
>       };
> 
>     +static void piix4_rcr_write(void *opaque, hwaddr addr, uint64_t val,
>     +                            unsigned int len)
>     +{
>     +    PIIX4State *s = opaque;
>     +
>     +    if (val & 4) {
>     +        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>     +        return;
>     +    }
>     +
>     +    s->rcr = val & 2; /* keep System Reset type only */
>     +}
>     +
>     +static uint64_t piix4_rcr_read(void *opaque, hwaddr addr, unsigned
>     int len)
>     +{
>     +    PIIX4State *s = opaque;
>     +
>     +    return s->rcr;
>     +}
>     +
>     +static const MemoryRegionOps piix4_rcr_ops = {
>     +    .read = piix4_rcr_read,
>     +    .write = piix4_rcr_write,
>     +    .endianness = DEVICE_LITTLE_ENDIAN,
>     +    .impl = {
>     +        .min_access_size = 1,
>     +        .max_access_size = 1,
>     +    },
>     +};
>     +
>       static void piix4_realize(PCIDevice *dev, Error **errp)
>       {
>     -    PIIX4State *d = PIIX4_PCI_DEVICE(dev);
>     +    PIIX4State *s = PIIX4_PCI_DEVICE(dev);
> 
>     -    if (!isa_bus_new(DEVICE(d), pci_address_space(dev),
>     +    if (!isa_bus_new(DEVICE(dev), pci_address_space(dev),
>                            pci_address_space_io(dev), errp)) {
>               return;
>           }
>     -    piix4_dev = &d->dev;
>     +
>     +    memory_region_init_io(&s->rcr_mem, OBJECT(dev), &piix4_rcr_ops, s,
>     +                          "reset-control", 1);
>     +    memory_region_add_subregion_overlap(pci_address_space_io(dev),
>     0xcf9,
> 
> 
> 
> Can we use 'RCR_IOPORT' instead of constant value here? Also don't see 
> this change
> in later patches of this seirals.

Good idea, I missed this one :)

> Anyway
> 
> Reviewed-by: Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>>

Thanks!

> 
> Thanks,
> Li Qiang
> 
>     +                                        &s->rcr_mem, 1);
>     +
>     +    piix4_dev = dev;
>       }
> 
>       int piix4_init(PCIBus *bus, ISABus **isa_bus, int devfn)
>     -- 
>     2.21.0
> 
> 