Re: [Qemu-devel] [PATCH v8 3/3] ich9: implement strap SPKR pin logic
On 02/07/2015 03:30, Paulo Alcantara wrote:
> No, the iTCO_wdt driver won't fail to start. It actually depends on
> whether RCBA BAR is set. However, ICH9 will just ignore whether
> NO_REBOOT is set or unset when SPKR pin is high.
This is the code I was referring to, in iTCO_wdt_start:
/* disable chipset's NO_REBOOT bit */
if (iTCO_wdt_unset_NO_REBOOT_bit()) {
spin_unlock(&iTCO_wdt_private.io_lock);
pr_err("failed to reset NO_REBOOT flag, reboot disabled by
hardware/BIOS\n");
return -EIO;
}
Paolo
Re: [Qemu-devel] [PATCH v8 3/3] ich9: implement strap SPKR pin logic
On Wed, 1 Jul 2015 15:31:31 +0200
"Michael S. Tsirkin" wrote:
> On Wed, Jul 01, 2015 at 03:18:41PM +0200, Paolo Bonzini wrote:
> >
> >
> > On 28/06/2015 19:58, Paulo Alcantara wrote:
> > > If the signal is sampled high, this indicates that the system is
> > > strapped to the "No Reboot" mode (ICH9 will disable the TCO Timer
> > > system reboot feature). The status of this strap is readable via
> > > the NO_REBOOT bit (CC: offset 0x3410:bit 5).
> > >
> > > The NO_REBOOT bit is set when SPKR pin on ICH9 is sampled high.
> > > This bit may be set or cleared by software if the strap is
> > > sampled low but may not override the strap when it indicates "No
> > > Reboot".
> > >
> > > This patch implements the logic where hardware has ability to set
> > > SPKR pin through a property named "noreboot" and it's sampled
> > > high by default.
> >
> > I know Michael suggested this, but I think default high is a worse
> > default. It does not allow recovering from a hard lockup where you
> > cannot process an NMI, unlike all other watchdogs implemented by
> > QEMU. In fact, the Linux driver fails to start if the strap is high.
No, the iTCO_wdt driver won't fail to start. It actually depends on
whether RCBA BAR is set. However, ICH9 will just ignore whether
NO_REBOOT is set or unset when SPKR pin is high.
I think you forgot to apply my SeaBIOS patch that sets RCBA BAR so it
failed to start. We should also make sure it's applied once TCO patches
get upstream.
Thanks,
Paulo
> >
> > My theory is that hardware manufacturers should only set the strap
> > high if they want the firmware to have total control of the
> > watchdog via SMIs (TCO_EN).
> >
> > If it is just a matter of being late in 2.4, just delay everything
> > to 2.5. It doesn't require any more work from Paulo, as you can
> > just flip the default yourself without adding a new machine type
> > (in fact I'm still not sure why machine types for Q35 are
> > versioned, since migration is not supported...).
> >
> > Paolo
>
> I don't think we should defer the whole series because of the argument
> about the default. I've merged these patches, I you like, pls send a
> one-line patch on top to flip the default with some info on how it was
> tested, and we can discuss it separately.
>
> Makes sense?
>
> BTW 2.4 makes qemu versioned because ahci finally supports migration
> so yes, we'll have to version from now on.
>
>
> > > Signed-off-by: Paulo Alcantara
> > > ---
> > > v7 -> v8:
> > > * change property name to "noreboot"
> > > * default "noreboot" property to high
> > > * define property in dc->props
> > > * update tco tests to support and exercise "noreboot" property
> > > ---
> > > hw/acpi/tco.c | 2 +-
> > > hw/isa/lpc_ich9.c | 6 ++
> > > include/hw/i386/ich9.h | 5 +
> > > tests/tco-test.c | 18 --
> > > 4 files changed, 28 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
> > > index 1794a54..7a026c2 100644
> > > --- a/hw/acpi/tco.c
> > > +++ b/hw/acpi/tco.c
> > > @@ -64,7 +64,7 @@ static void tco_timer_expired(void *opaque)
> > > tr->tco.sts2 |= TCO_BOOT_STS;
> > > tr->timeouts_no = 0;
> > >
> > > -if (!(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> > > +if (!lpc->pin_strap.spkr_hi && !(gcs &
> > > ICH9_CC_GCS_NO_REBOOT)) { watchdog_perform_action();
> > > tco_timer_stop(tr);
> > > return;
> > > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > > index b547002..3b460d4 100644
> > > --- a/hw/isa/lpc_ich9.c
> > > +++ b/hw/isa/lpc_ich9.c
> > > @@ -688,6 +688,11 @@ static const VMStateDescription
> > > vmstate_ich9_lpc = { }
> > > };
> > >
> > > +static Property ich9_lpc_properties[] = {
> > > +DEFINE_PROP_BOOL("noreboot", ICH9LPCState,
> > > pin_strap.spkr_hi, true),
> > > +DEFINE_PROP_END_OF_LIST(),
> > > +};
> > > +
> > > static void ich9_lpc_class_init(ObjectClass *klass, void *data)
> > > {
> > > DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -699,6 +704,7 @@ static void ich9_lpc_class_init(ObjectClass
> > > *klass, void *data) dc->reset = ich9_lpc_reset;
> > > k->init = ich9_lpc_init;
> > > dc->vmsd = &vmstate_ich9_lpc;
> > > +dc->props = ich9_lpc_properties;
> > > k->config_write = ich9_lpc_config_write;
> > > dc->desc = "ICH9 LPC bridge";
> > > k->vendor_id = PCI_VENDOR_ID_INTEL;
> > > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > > index f5681a3..63c5cd8 100644
> > > --- a/include/hw/i386/ich9.h
> > > +++ b/include/hw/i386/ich9.h
> > > @@ -46,6 +46,11 @@ typedef struct ICH9LPCState {
> > > ICH9LPCPMRegs pm;
> > > uint32_t sci_level; /* track sci level */
> > >
> > > +/* 2.24 Pin Straps */
> > > +struct {
> > > +bool spkr_hi;
> > > +} pin_strap;
> > > +
> > > /* 10.1 Chipset Configuration registers(Memory Space)
> > > which is pointed by RCBA */
> > > uint8_t chip_config[ICH9_C
Re: [Qemu-devel] [PATCH v8 3/3] ich9: implement strap SPKR pin logic
On 01/07/2015 15:31, Michael S. Tsirkin wrote: > I don't think we should defer the whole series because of the argument > about the default. I've merged these patches, I you like, pls send a > one-line patch on top to flip the default with some info on how it was > tested, and we can discuss it separately. > > Makes sense? Perfect. Paolo > BTW 2.4 makes qemu versioned because ahci finally supports migration > so yes, we'll have to version from now on. > >
Re: [Qemu-devel] [PATCH v8 3/3] ich9: implement strap SPKR pin logic
On Wed, Jul 01, 2015 at 03:18:41PM +0200, Paolo Bonzini wrote:
>
>
> On 28/06/2015 19:58, Paulo Alcantara wrote:
> > If the signal is sampled high, this indicates that the system is
> > strapped to the "No Reboot" mode (ICH9 will disable the TCO Timer system
> > reboot feature). The status of this strap is readable via the NO_REBOOT
> > bit (CC: offset 0x3410:bit 5).
> >
> > The NO_REBOOT bit is set when SPKR pin on ICH9 is sampled high. This bit
> > may be set or cleared by software if the strap is sampled low but may
> > not override the strap when it indicates "No Reboot".
> >
> > This patch implements the logic where hardware has ability to set SPKR
> > pin through a property named "noreboot" and it's sampled high by
> > default.
>
> I know Michael suggested this, but I think default high is a worse
> default. It does not allow recovering from a hard lockup where you
> cannot process an NMI, unlike all other watchdogs implemented by QEMU.
> In fact, the Linux driver fails to start if the strap is high.
>
> My theory is that hardware manufacturers should only set the strap high
> if they want the firmware to have total control of the watchdog via SMIs
> (TCO_EN).
>
> If it is just a matter of being late in 2.4, just delay everything to
> 2.5. It doesn't require any more work from Paulo, as you can just flip
> the default yourself without adding a new machine type (in fact I'm
> still not sure why machine types for Q35 are versioned, since migration
> is not supported...).
>
> Paolo
I don't think we should defer the whole series because of the argument
about the default. I've merged these patches, I you like, pls send a
one-line patch on top to flip the default with some info on how it was
tested, and we can discuss it separately.
Makes sense?
BTW 2.4 makes qemu versioned because ahci finally supports migration
so yes, we'll have to version from now on.
> > Signed-off-by: Paulo Alcantara
> > ---
> > v7 -> v8:
> > * change property name to "noreboot"
> > * default "noreboot" property to high
> > * define property in dc->props
> > * update tco tests to support and exercise "noreboot" property
> > ---
> > hw/acpi/tco.c | 2 +-
> > hw/isa/lpc_ich9.c | 6 ++
> > include/hw/i386/ich9.h | 5 +
> > tests/tco-test.c | 18 --
> > 4 files changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
> > index 1794a54..7a026c2 100644
> > --- a/hw/acpi/tco.c
> > +++ b/hw/acpi/tco.c
> > @@ -64,7 +64,7 @@ static void tco_timer_expired(void *opaque)
> > tr->tco.sts2 |= TCO_BOOT_STS;
> > tr->timeouts_no = 0;
> >
> > -if (!(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> > +if (!lpc->pin_strap.spkr_hi && !(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> > watchdog_perform_action();
> > tco_timer_stop(tr);
> > return;
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index b547002..3b460d4 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -688,6 +688,11 @@ static const VMStateDescription vmstate_ich9_lpc = {
> > }
> > };
> >
> > +static Property ich9_lpc_properties[] = {
> > +DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
> > +DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > static void ich9_lpc_class_init(ObjectClass *klass, void *data)
> > {
> > DeviceClass *dc = DEVICE_CLASS(klass);
> > @@ -699,6 +704,7 @@ static void ich9_lpc_class_init(ObjectClass *klass,
> > void *data)
> > dc->reset = ich9_lpc_reset;
> > k->init = ich9_lpc_init;
> > dc->vmsd = &vmstate_ich9_lpc;
> > +dc->props = ich9_lpc_properties;
> > k->config_write = ich9_lpc_config_write;
> > dc->desc = "ICH9 LPC bridge";
> > k->vendor_id = PCI_VENDOR_ID_INTEL;
> > diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> > index f5681a3..63c5cd8 100644
> > --- a/include/hw/i386/ich9.h
> > +++ b/include/hw/i386/ich9.h
> > @@ -46,6 +46,11 @@ typedef struct ICH9LPCState {
> > ICH9LPCPMRegs pm;
> > uint32_t sci_level; /* track sci level */
> >
> > +/* 2.24 Pin Straps */
> > +struct {
> > +bool spkr_hi;
> > +} pin_strap;
> > +
> > /* 10.1 Chipset Configuration registers(Memory Space)
> > which is pointed by RCBA */
> > uint8_t chip_config[ICH9_CC_SIZE];
> > diff --git a/tests/tco-test.c b/tests/tco-test.c
> > index 1a2fe3d..6a48188 100644
> > --- a/tests/tco-test.c
> > +++ b/tests/tco-test.c
> > @@ -42,6 +42,7 @@ enum {
> >
> > typedef struct {
> > const char *args;
> > +bool noreboot;
> > QPCIDevice *dev;
> > void *lpc_base;
> > void *tco_io_base;
> > @@ -53,7 +54,9 @@ static void test_init(TestData *d)
> > QTestState *qs;
> > char *s;
> >
> > -s = g_strdup_printf("-machine q35 %s", !d->args ? "" : d->args);
> > +s = g_strdup_printf("-machine q35 %s %s",
> > +d->noreboot ? "" : "-global
>
Re: [Qemu-devel] [PATCH v8 3/3] ich9: implement strap SPKR pin logic
On 28/06/2015 19:58, Paulo Alcantara wrote:
> If the signal is sampled high, this indicates that the system is
> strapped to the "No Reboot" mode (ICH9 will disable the TCO Timer system
> reboot feature). The status of this strap is readable via the NO_REBOOT
> bit (CC: offset 0x3410:bit 5).
>
> The NO_REBOOT bit is set when SPKR pin on ICH9 is sampled high. This bit
> may be set or cleared by software if the strap is sampled low but may
> not override the strap when it indicates "No Reboot".
>
> This patch implements the logic where hardware has ability to set SPKR
> pin through a property named "noreboot" and it's sampled high by
> default.
I know Michael suggested this, but I think default high is a worse
default. It does not allow recovering from a hard lockup where you
cannot process an NMI, unlike all other watchdogs implemented by QEMU.
In fact, the Linux driver fails to start if the strap is high.
My theory is that hardware manufacturers should only set the strap high
if they want the firmware to have total control of the watchdog via SMIs
(TCO_EN).
If it is just a matter of being late in 2.4, just delay everything to
2.5. It doesn't require any more work from Paulo, as you can just flip
the default yourself without adding a new machine type (in fact I'm
still not sure why machine types for Q35 are versioned, since migration
is not supported...).
Paolo
> Signed-off-by: Paulo Alcantara
> ---
> v7 -> v8:
> * change property name to "noreboot"
> * default "noreboot" property to high
> * define property in dc->props
> * update tco tests to support and exercise "noreboot" property
> ---
> hw/acpi/tco.c | 2 +-
> hw/isa/lpc_ich9.c | 6 ++
> include/hw/i386/ich9.h | 5 +
> tests/tco-test.c | 18 --
> 4 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
> index 1794a54..7a026c2 100644
> --- a/hw/acpi/tco.c
> +++ b/hw/acpi/tco.c
> @@ -64,7 +64,7 @@ static void tco_timer_expired(void *opaque)
> tr->tco.sts2 |= TCO_BOOT_STS;
> tr->timeouts_no = 0;
>
> -if (!(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> +if (!lpc->pin_strap.spkr_hi && !(gcs & ICH9_CC_GCS_NO_REBOOT)) {
> watchdog_perform_action();
> tco_timer_stop(tr);
> return;
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index b547002..3b460d4 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -688,6 +688,11 @@ static const VMStateDescription vmstate_ich9_lpc = {
> }
> };
>
> +static Property ich9_lpc_properties[] = {
> +DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
> +DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void ich9_lpc_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -699,6 +704,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void
> *data)
> dc->reset = ich9_lpc_reset;
> k->init = ich9_lpc_init;
> dc->vmsd = &vmstate_ich9_lpc;
> +dc->props = ich9_lpc_properties;
> k->config_write = ich9_lpc_config_write;
> dc->desc = "ICH9 LPC bridge";
> k->vendor_id = PCI_VENDOR_ID_INTEL;
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index f5681a3..63c5cd8 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -46,6 +46,11 @@ typedef struct ICH9LPCState {
> ICH9LPCPMRegs pm;
> uint32_t sci_level; /* track sci level */
>
> +/* 2.24 Pin Straps */
> +struct {
> +bool spkr_hi;
> +} pin_strap;
> +
> /* 10.1 Chipset Configuration registers(Memory Space)
> which is pointed by RCBA */
> uint8_t chip_config[ICH9_CC_SIZE];
> diff --git a/tests/tco-test.c b/tests/tco-test.c
> index 1a2fe3d..6a48188 100644
> --- a/tests/tco-test.c
> +++ b/tests/tco-test.c
> @@ -42,6 +42,7 @@ enum {
>
> typedef struct {
> const char *args;
> +bool noreboot;
> QPCIDevice *dev;
> void *lpc_base;
> void *tco_io_base;
> @@ -53,7 +54,9 @@ static void test_init(TestData *d)
> QTestState *qs;
> char *s;
>
> -s = g_strdup_printf("-machine q35 %s", !d->args ? "" : d->args);
> +s = g_strdup_printf("-machine q35 %s %s",
> +d->noreboot ? "" : "-global ICH9-LPC.noreboot=false",
> +!d->args ? "" : d->args);
> qs = qtest_start(s);
> qtest_irq_intercept_in(qs, "ioapic");
> g_free(s);
> @@ -135,6 +138,7 @@ static void test_tco_defaults(void)
> TestData d;
>
> d.args = NULL;
> +d.noreboot = true;
> test_init(&d);
> g_assert_cmpint(qpci_io_readw(d.dev, d.tco_io_base + TCO_RLD), ==,
> TCO_RLD_DEFAULT);
> @@ -167,6 +171,7 @@ static void test_tco_timeout(void)
> int ret;
>
> d.args = NULL;
> +d.noreboot = true;
> test_init(&d);
>
> stop_tco(&d);
> @@ -210,6 +215,7 @@ static void test_tco_max_timeout(void)
> int ret;
>
[Qemu-devel] [PATCH v8 3/3] ich9: implement strap SPKR pin logic
If the signal is sampled high, this indicates that the system is
strapped to the "No Reboot" mode (ICH9 will disable the TCO Timer system
reboot feature). The status of this strap is readable via the NO_REBOOT
bit (CC: offset 0x3410:bit 5).
The NO_REBOOT bit is set when SPKR pin on ICH9 is sampled high. This bit
may be set or cleared by software if the strap is sampled low but may
not override the strap when it indicates "No Reboot".
This patch implements the logic where hardware has ability to set SPKR
pin through a property named "noreboot" and it's sampled high by
default.
Signed-off-by: Paulo Alcantara
---
v7 -> v8:
* change property name to "noreboot"
* default "noreboot" property to high
* define property in dc->props
* update tco tests to support and exercise "noreboot" property
---
hw/acpi/tco.c | 2 +-
hw/isa/lpc_ich9.c | 6 ++
include/hw/i386/ich9.h | 5 +
tests/tco-test.c | 18 --
4 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
index 1794a54..7a026c2 100644
--- a/hw/acpi/tco.c
+++ b/hw/acpi/tco.c
@@ -64,7 +64,7 @@ static void tco_timer_expired(void *opaque)
tr->tco.sts2 |= TCO_BOOT_STS;
tr->timeouts_no = 0;
-if (!(gcs & ICH9_CC_GCS_NO_REBOOT)) {
+if (!lpc->pin_strap.spkr_hi && !(gcs & ICH9_CC_GCS_NO_REBOOT)) {
watchdog_perform_action();
tco_timer_stop(tr);
return;
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index b547002..3b460d4 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -688,6 +688,11 @@ static const VMStateDescription vmstate_ich9_lpc = {
}
};
+static Property ich9_lpc_properties[] = {
+DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
+DEFINE_PROP_END_OF_LIST(),
+};
+
static void ich9_lpc_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -699,6 +704,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void
*data)
dc->reset = ich9_lpc_reset;
k->init = ich9_lpc_init;
dc->vmsd = &vmstate_ich9_lpc;
+dc->props = ich9_lpc_properties;
k->config_write = ich9_lpc_config_write;
dc->desc = "ICH9 LPC bridge";
k->vendor_id = PCI_VENDOR_ID_INTEL;
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index f5681a3..63c5cd8 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -46,6 +46,11 @@ typedef struct ICH9LPCState {
ICH9LPCPMRegs pm;
uint32_t sci_level; /* track sci level */
+/* 2.24 Pin Straps */
+struct {
+bool spkr_hi;
+} pin_strap;
+
/* 10.1 Chipset Configuration registers(Memory Space)
which is pointed by RCBA */
uint8_t chip_config[ICH9_CC_SIZE];
diff --git a/tests/tco-test.c b/tests/tco-test.c
index 1a2fe3d..6a48188 100644
--- a/tests/tco-test.c
+++ b/tests/tco-test.c
@@ -42,6 +42,7 @@ enum {
typedef struct {
const char *args;
+bool noreboot;
QPCIDevice *dev;
void *lpc_base;
void *tco_io_base;
@@ -53,7 +54,9 @@ static void test_init(TestData *d)
QTestState *qs;
char *s;
-s = g_strdup_printf("-machine q35 %s", !d->args ? "" : d->args);
+s = g_strdup_printf("-machine q35 %s %s",
+d->noreboot ? "" : "-global ICH9-LPC.noreboot=false",
+!d->args ? "" : d->args);
qs = qtest_start(s);
qtest_irq_intercept_in(qs, "ioapic");
g_free(s);
@@ -135,6 +138,7 @@ static void test_tco_defaults(void)
TestData d;
d.args = NULL;
+d.noreboot = true;
test_init(&d);
g_assert_cmpint(qpci_io_readw(d.dev, d.tco_io_base + TCO_RLD), ==,
TCO_RLD_DEFAULT);
@@ -167,6 +171,7 @@ static void test_tco_timeout(void)
int ret;
d.args = NULL;
+d.noreboot = true;
test_init(&d);
stop_tco(&d);
@@ -210,6 +215,7 @@ static void test_tco_max_timeout(void)
int ret;
d.args = NULL;
+d.noreboot = true;
test_init(&d);
stop_tco(&d);
@@ -253,6 +259,7 @@ static void test_tco_second_timeout_pause(void)
QDict *ad;
td.args = "-watchdog-action pause";
+td.noreboot = false;
test_init(&td);
stop_tco(&td);
@@ -277,6 +284,7 @@ static void test_tco_second_timeout_reset(void)
QDict *ad;
td.args = "-watchdog-action reset";
+td.noreboot = false;
test_init(&td);
stop_tco(&td);
@@ -301,6 +309,7 @@ static void test_tco_second_timeout_shutdown(void)
QDict *ad;
td.args = "-watchdog-action shutdown";
+td.noreboot = false;
test_init(&td);
stop_tco(&td);
@@ -325,6 +334,7 @@ static void test_tco_second_timeout_none(void)
QDict *ad;
td.args = "-watchdog-action none";
+td.noreboot = false;
test_init(&td);
stop_tco(&td);
@@ -349,6 +359,7 @@ static void test_tco_ticks_counter(void)
uint16_t rld;
d.args = NULL;
+d.noreboot = true;
test_init(&d);
