[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/3] ich9: add TCO interface emulation
|
From: |
Paulo Alcantara |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 1/3] ich9: add TCO interface emulation |
|
Date: |
Wed, 17 Jun 2015 23:10:43 -0300 |
On Wed, 17 Jun 2015 15:27:53 +0200
"Michael S. Tsirkin" <address@hidden> wrote:
> On Mon, Jun 01, 2015 at 08:48:39PM -0300, Paulo Alcantara wrote:
> > This interface provides some registers within a 32-byte range and
> > can be acessed through PCI-to-LPC bridge interface (PMBASE + 0x60).
> >
> > It's commonly used as a watchdog timer to detect system lockups
> > through SMIs that are generated -- if TCO_EN bit is set -- on every
> > timeout. If NO_REBOOT bit is not set in GCS (General Control and
> > Status register), the system will be resetted upon second timeout
> > if TCO_RLD register wasn't previously written to prevent timeout.
> >
> > This patch adds support to TCO watchdog logic and few other features
> > like mapping NMIs to SMIs (NMI2SMI_EN bit), system intruder
> > detection, etc. are not implemented yet.
> >
> > v1 -> v2:
> > * add migration support for TCO I/O device state
> > * wake up only when total time expired instead of every 0.6s
> > * some cleanup suggested by Paolo Bonzini
> > v2 -> v3:
> > * set SECOND_TO_STS and BOOT_STS bits in TCO2_STS instead
> > * improve handling of TCO_LOCK bit in TCO1_CNT register
>
> changelog must come after --- so that git am ignores it.
Ok.
>
> >
> > Signed-off-by: Paulo Alcantara <address@hidden>
>
> for some reason v3 was sent as reply to v2.
> Don't do that please.
Indeed. Sorry about that.
>
>
> > ---
> > hw/acpi/Makefile.objs | 2 +-
> > hw/acpi/ich9.c | 59 ++++++++++++
> > hw/acpi/tco.c | 254
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> > hw/isa/lpc_ich9.c | 10 ++ include/hw/acpi/ich9.h | 4 +
> > include/hw/acpi/tco.h | 98 +++++++++++++++++++
> > include/hw/i386/ich9.h | 8 ++
> > 7 files changed, 434 insertions(+), 1 deletion(-)
> > create mode 100644 hw/acpi/tco.c
> > create mode 100644 include/hw/acpi/tco.h
> >
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index 29d46d8..3db1f07 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,4 +1,4 @@
> > -common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o ich9.o pcihp.o
> > +common-obj-$(CONFIG_ACPI_X86) += core.o piix4.o ich9.o pcihp.o
> > tco.o common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> > common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> > common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 84e5bb8..10959fa 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -30,6 +30,7 @@
> > #include "qemu/timer.h"
> > #include "sysemu/sysemu.h"
> > #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/tco.h"
> > #include "sysemu/kvm.h"
> > #include "exec/address-spaces.h"
> >
> > @@ -92,8 +93,15 @@ static void ich9_smi_writel(void *opaque, hwaddr
> > addr, uint64_t val, unsigned width)
> > {
> > ICH9LPCPMRegs *pm = opaque;
> > + TCOIORegs *tr = &pm->tco_regs;
> > +
> > switch (addr) {
> > case 0:
> > + /* once TCO_LOCK bit is set, TCO_EN bit cannot be
> > overwritten */
> > + if (tr->tco.cnt1 & TCO_LOCK) {
> > + val &= ~ICH9_PMIO_SMI_EN_TCO_EN;
> > + val |= pm->smi_en & ICH9_PMIO_SMI_EN_TCO_EN;
> > + }
> > pm->smi_en = val;
> > break;
> > }
> > @@ -107,6 +115,29 @@ static const MemoryRegionOps ich9_smi_ops = {
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > };
> >
> > +static uint64_t ich9_tco_readw(void *opaque, hwaddr addr, unsigned
> > width) +{
> > + ICH9LPCPMRegs *pm = opaque;
> > + return acpi_pm_tco_ioport_readw(&pm->tco_regs, addr);
> > +}
> > +
> > +static void ich9_tco_writew(void *opaque, hwaddr addr, uint64_t
> > val,
> > + unsigned width)
> > +{
> > + ICH9LPCPMRegs *pm = opaque;
> > + acpi_pm_tco_ioport_writew(&pm->tco_regs, addr, val);
> > +}
> > +
> > +static const MemoryRegionOps ich9_tco_ops = {
> > + .read = ich9_tco_readw,
> > + .write = ich9_tco_writew,
> > + .valid.min_access_size = 1,
> > + .valid.max_access_size = 4,
> > + .impl.min_access_size = 1,
> > + .impl.max_access_size = 2,
> > + .endianness = DEVICE_LITTLE_ENDIAN,
> > +};
> > +
> > void ich9_pm_iospace_update(ICH9LPCPMRegs *pm, uint32_t pm_io_base)
> > {
> > ICH9_DEBUG("to 0x%x\n", pm_io_base);
> > @@ -157,6 +188,24 @@ static const VMStateDescription
> > vmstate_memhp_state = { }
> > };
> >
> > +static bool vmstate_test_use_tco(void *opaque)
> > +{
> > + ICH9LPCPMRegs *s = opaque;
> > + return s->tco_regs.use_tco;
> > +}
> > +
> > +static const VMStateDescription vmstate_tco_io_state = {
> > + .name = "ich9_pm/tco",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .minimum_version_id_old = 1,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_STRUCT(tco_regs, ICH9LPCPMRegs, 1,
> > vmstate_tco_io_sts,
> > + TCOIORegs),
> > + VMSTATE_END_OF_LIST()
> > + }
> > +};
> > +
> > const VMStateDescription vmstate_ich9_pm = {
> > .name = "ich9_pm",
> > .version_id = 1,
> > @@ -179,6 +228,10 @@ const VMStateDescription vmstate_ich9_pm = {
> > .vmsd = &vmstate_memhp_state,
> > .needed = vmstate_test_use_memhp,
> > },
> > + {
> > + .vmsd = &vmstate_tco_io_state,
> > + .needed = vmstate_test_use_tco,
> > + },
> > VMSTATE_END_OF_LIST()
> > }
> > };
> > @@ -230,6 +283,11 @@ void ich9_pm_init(PCIDevice *lpc_pci,
> > ICH9LPCPMRegs *pm, "acpi-smi", 8);
> > memory_region_add_subregion(&pm->io, ICH9_PMIO_SMI_EN,
> > &pm->io_smi);
> > + acpi_pm_tco_init(&pm->tco_regs);
> > + memory_region_init_io(&pm->io_tco, OBJECT(lpc_pci),
> > &ich9_tco_ops, pm,
> > + "sm-tco", ICH9_PMIO_TCO_LEN);
> > + memory_region_add_subregion(&pm->io, ICH9_PMIO_TCO_RLD,
> > &pm->io_tco); +
> > pm->irq = sci_irq;
> > qemu_register_reset(pm_reset, pm);
> > pm->powerdown_notifier.notify = pm_powerdown_req;
> > @@ -357,6 +415,7 @@ void ich9_pm_add_properties(Object *obj,
> > ICH9LPCPMRegs *pm, Error **errp) pm->disable_s3 = 0;
> > pm->disable_s4 = 0;
> > pm->s4_val = 2;
> > + pm->tco_regs.use_tco = true;
>
> Would be safer to add a property, and not to enable this for old
> machine types.
This use_tco field is *really* confusing. My initial idea of using this
field was to determine whether migrate TCO registers or not, but now
that doesn't make sense at all. On Q35 machine type all TCO registers
should be migrated and initialised.
Since this is ICH9-specific code, doesn't this mean other machine types
(e.g. i440FX) wouldn't be supported? would we need to add a property to
it yet?
>
> >
> > object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > &pm->pm_io_base, errp);
> > diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
> > new file mode 100644
> > index 0000000..c159281
> > --- /dev/null
> > +++ b/hw/acpi/tco.c
> > @@ -0,0 +1,254 @@
> > +/*
> > + * QEMU ICH9 TCO emulation
> > + *
> > + * Copyright (c) 2015 Paulo Alcantara <address@hidden>
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a copy
> > + * of this software and associated documentation files (the
> > "Software"), to deal
> > + * in the Software without restriction, including without
> > limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense,
> > and/or sell
> > + * copies of the Software, and to permit persons to whom the
> > Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +#include "qemu-common.h"
> > +#include "sysemu/watchdog.h"
> > +#include "hw/i386/ich9.h"
> > +
> > +#include "hw/acpi/tco.h"
> > +
> > +//#define DEBUG
> > +
> > +#ifdef DEBUG
> > +#define TCO_DEBUG(fmt, ...) \
> > + do { \
> > + fprintf(stderr, "%s "fmt, __func__, ## __VA_ARGS__); \
> > + } while (0)
> > +#else
> > +#define TCO_DEBUG(fmt, ...) do { } while (0)
> > +#endif
> > +
> > +enum {
> > + TCO_RLD_DEFAULT = 0x0000,
> > + TCO_DAT_IN_DEFAULT = 0x00,
> > + TCO_DAT_OUT_DEFAULT = 0x00,
> > + TCO1_STS_DEFAULT = 0x0000,
> > + TCO2_STS_DEFAULT = 0x0000,
> > + TCO1_CNT_DEFAULT = 0x0000,
> > + TCO2_CNT_DEFAULT = 0x0008,
> > + TCO_MESSAGE1_DEFAULT = 0x00,
> > + TCO_MESSAGE2_DEFAULT = 0x00,
> > + TCO_WDCNT_DEFAULT = 0x00,
> > + TCO_TMR_DEFAULT = 0x0004,
> > + SW_IRQ_GEN_DEFAULT = 0x03,
> > +};
> > +
> > +static inline void tco_timer_reload(TCOIORegs *tr)
> > +{
> > + tr->expire_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > + ((int64_t)(tr->tco.tmr & TCO_TMR_MASK) * TCO_TICK_NSEC);
> > + timer_mod(tr->tco_timer, tr->expire_time);
> > +}
> > +
> > +static inline void tco_timer_stop(TCOIORegs *tr)
> > +{
> > + tr->expire_time = -1;
> > +}
> > +
> > +static void tco_timer_expired(void *opaque)
> > +{
> > + TCOIORegs *tr = opaque;
> > + ICH9LPCPMRegs *pm = container_of(tr, ICH9LPCPMRegs, tco_regs);
> > + ICH9LPCState *lpc = container_of(pm, ICH9LPCState, pm);
> > + uint32_t gcs = pci_get_long(lpc->chip_config +
> > ICH9_LPC_RCBA_GCS); +
> > + tr->tco.rld = 0;
> > + tr->tco.sts1 |= TCO_TIMEOUT;
> > + if (++tr->timeouts_no == 2) {
> > + tr->tco.sts2 |= TCO_SECOND_TO_STS;
> > + tr->tco.sts2 |= TCO_BOOT_STS;
> > + tr->timeouts_no = 0;
> > +
> > + if (!(gcs & ICH9_LPC_RCBA_GCS_NO_REBOOT)) {
> > + watchdog_perform_action();
> > + tco_timer_stop(tr);
> > + return;
>
> So this is a virtual clock - not running when VM is not running.
> Doesn't this mean if you stop VM for a while, it's almost sure to
> fire when you resume?
Yes, it will.
>
> Maybe it's a useful feature, but maybe it's best to keep it off by
> default?
The timer is already halted by default. So you might ask me why it's
halted if TCO_TMR_HLT bit is 0 by default (e.g. timer running)? It will
only start counting down if someone reloads it through TCO_RLD register.
I would just keep it running when resuming VM because ICH9 spec says
that it will _only_ count down in the S0 state. Although, I think it's
worth mentioning that the firmware should be able to stop it (or
restore its default values) while in the S3 resume path.
>
> > + }
> > + }
> > +
> > + if (pm->smi_en & ICH9_PMIO_SMI_EN_TCO_EN) {
> > + ich9_generate_smi();
> > + } else {
> > + ich9_generate_nmi();
> > + }
> > + tr->tco.rld = tr->tco.tmr;
> > + tco_timer_reload(tr);
> > +}
> > +
> > +void acpi_pm_tco_init(TCOIORegs *tr)
> > +{
> > + *tr = (TCOIORegs) {
> > + .tco = {
> > + .rld = TCO_RLD_DEFAULT,
> > + .din = TCO_DAT_IN_DEFAULT,
> > + .dout = TCO_DAT_OUT_DEFAULT,
> > + .sts1 = TCO1_STS_DEFAULT,
> > + .sts2 = TCO2_STS_DEFAULT,
> > + .cnt1 = TCO1_CNT_DEFAULT,
> > + .cnt2 = TCO2_CNT_DEFAULT,
> > + .msg1 = TCO_MESSAGE1_DEFAULT,
> > + .msg2 = TCO_MESSAGE2_DEFAULT,
> > + .wdcnt = TCO_WDCNT_DEFAULT,
> > + .tmr = TCO_TMR_DEFAULT,
> > + },
> > + .sw_irq_gen = SW_IRQ_GEN_DEFAULT,
> > + .tco_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > tco_timer_expired, tr),
> > + .expire_time = -1,
> > + .timeouts_no = 0,
> > + };
> > +}
> > +
> > +/* NOTE: values of 0 or 1 will be ignored by ICH */
> > +static inline int can_start_tco_timer(TCOIORegs *tr)
> > +{
> > + return !(tr->tco.cnt1 & TCO_TMR_HLT) && tr->tco.tmr > 1;
> > +}
> > +
> > +uint32_t acpi_pm_tco_ioport_readw(TCOIORegs *tr, uint32_t addr)
> > +{
> > + uint16_t rld;
> > +
> > + switch (addr) {
> > + case TCO_RLD:
> > + if (tr->expire_time != -1) {
> > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + int64_t elapsed = (tr->expire_time - now) /
> > TCO_TICK_NSEC;
> > + rld = (uint16_t)elapsed | (tr->tco.rld &
> > ~TCO_RLD_MASK);
> > + } else {
> > + rld = tr->tco.rld;
> > + }
> > + return rld;
> > + case TCO_DAT_IN:
> > + return tr->tco.din;
> > + case TCO_DAT_OUT:
> > + return tr->tco.dout;
> > + case TCO1_STS:
> > + return tr->tco.sts1;
> > + case TCO2_STS:
> > + return tr->tco.sts2;
> > + case TCO1_CNT:
> > + return tr->tco.cnt1;
> > + case TCO2_CNT:
> > + return tr->tco.cnt2;
> > + case TCO_MESSAGE1:
> > + return tr->tco.msg1;
> > + case TCO_MESSAGE2:
> > + return tr->tco.msg2;
> > + case TCO_WDCNT:
> > + return tr->tco.wdcnt;
> > + case TCO_TMR:
> > + return tr->tco.tmr;
> > + case SW_IRQ_GEN:
> > + return tr->sw_irq_gen;
> > + }
> > + return 0;
> > +}
> > +
> > +void acpi_pm_tco_ioport_writew(TCOIORegs *tr, uint32_t addr,
> > uint32_t val) +{
> > + switch (addr) {
> > + case TCO_RLD:
> > + tr->timeouts_no = 0;
> > + if (can_start_tco_timer(tr)) {
> > + tr->tco.rld = tr->tco.tmr;
> > + tco_timer_reload(tr);
> > + } else {
> > + tr->tco.rld = val;
> > + }
> > + break;
> > + case TCO_DAT_IN:
> > + tr->tco.din = val;
> > + tr->tco.sts1 |= SW_TCO_SMI;
> > + ich9_generate_smi();
> > + break;
> > + case TCO_DAT_OUT:
> > + tr->tco.dout = val;
> > + tr->tco.sts1 |= TCO_INT_STS;
> > + /* TODO: cause an interrupt, as selected by the
> > TCO_INT_SEL bits */
> > + break;
> > + case TCO1_STS:
> > + tr->tco.sts1 = val & TCO1_STS_MASK;
> > + break;
> > + case TCO2_STS:
> > + tr->tco.sts2 = val & TCO2_STS_MASK;
> > + break;
> > + case TCO1_CNT:
> > + val &= TCO1_CNT_MASK;
> > + /*
> > + * once TCO_LOCK bit is set, it can not be cleared by
> > software. a reset
> > + * is required to change this bit from 1 to 0 -- it
> > defaults to 0.
> > + */
> > + tr->tco.cnt1 = val | (tr->tco.cnt1 & TCO_LOCK);
> > + if (can_start_tco_timer(tr)) {
> > + tr->tco.rld = tr->tco.tmr;
> > + tco_timer_reload(tr);
> > + } else {
> > + tco_timer_stop(tr);
> > + }
> > + break;
> > + case TCO2_CNT:
> > + tr->tco.cnt2 = val;
> > + break;
> > + case TCO_MESSAGE1:
> > + tr->tco.msg1 = val;
> > + break;
> > + case TCO_MESSAGE2:
> > + tr->tco.msg2 = val;
> > + break;
> > + case TCO_WDCNT:
> > + tr->tco.wdcnt = val;
> > + break;
> > + case TCO_TMR:
> > + tr->tco.tmr = val;
> > + break;
> > + case SW_IRQ_GEN:
> > + tr->sw_irq_gen = val;
> > + break;
> > + }
> > +}
> > +
> > +const VMStateDescription vmstate_tco_io_sts = {
> > + .name = "tco io device status",
> > + .version_id = 1,
> > + .minimum_version_id = 1,
> > + .minimum_version_id_old = 1,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_BOOL(use_tco, TCOIORegs),
>
> So this field is only migrated if it is true.
> Doesn't seem to make sense.
Yeah, indeed. I should remove this field and always migrate TCO
registers on Q35 machine type.
--
Paulo Alcantara, C.E.S.A.R
Speaking for myself only.