* [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
@ 2018-02-20 13:26 Hugo Landau
2018-02-20 13:57 ` Cédric Le Goater
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Hugo Landau @ 2018-02-20 13:26 UTC (permalink / raw
To: qemu-devel, clg; +Cc: Hugo Landau
Some register blocks of the ast2500 are protected by protection key
registers which require the right magic value to be written to those
registers to allow those registers to be mutated.
Register manuals indicate that writing the correct magic value to these
registers should cause subsequent reads from those values to return 1,
and writing any other value should cause subsequent reads to return 0.
Previously, qemu implemented these registers incorrectly: the registers
were handled as simple memory, meaning that writing some value x to a
protection key register would result in subsequent reads from that
register returning the same value x. The protection was implemented by
ensuring that the current value of that register equaled the magic
value.
This modifies qemu to have the correct behaviour: attempts to write to a
ast2500 protection register results in a transition to 1 or 0 depending
on whether the written value is the correct magic. The protection logic
is updated to ensure that the value of the register is nonzero.
This bug caused deadlocks with u-boot HEAD: when u-boot is done with a
protectable register block, it attempts to lock it by writing the
bitwise inverse of the correct magic value, and then spinning forever
until the register reads as zero. Since qemu implemented writes to these
registers as ordinary memory writes, writing the inverse of the magic
value resulted in subsequent reads returning that value, leading to
u-boot spinning forever.
Signed-off-by: Hugo Landau <hlandau@devever.net>
---
hw/misc/aspeed_scu.c | 6 +++++-
hw/misc/aspeed_sdmc.c | 8 +++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 74537ce975..5e6d5744ee 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -191,7 +191,7 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
}
if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
- s->regs[PROT_KEY] != ASPEED_SCU_PROT_KEY) {
+ !s->regs[PROT_KEY]) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);
return;
}
@@ -199,6 +199,10 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
trace_aspeed_scu_write(offset, size, data);
switch (reg) {
+ case PROT_KEY:
+ s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
+ return;
+
case FREQ_CNTR_EVAL:
case VGA_SCRATCH1 ... VGA_SCRATCH8:
case RNG_DATA:
diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index f0b3053fae..265171ee42 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -110,7 +110,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
return;
}
- if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
+ if (addr == R_PROT) {
+ s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
+ return;
+ }
+
+ if (!s->regs[R_PROT]) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
return;
}
@@ -123,6 +128,7 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
data &= ~ASPEED_SDMC_READONLY_MASK;
break;
case AST2500_A0_SILICON_REV:
+ case AST2500_A1_SILICON_REV:
data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
break;
default:
--
2.15.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 13:26 [Qemu-devel] [PATCH] Fix ast2500 protection register emulation Hugo Landau
@ 2018-02-20 13:57 ` Cédric Le Goater
2018-02-20 14:19 ` Hugo Landau
2018-02-20 23:01 ` Andrew Jeffery
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2018-02-20 13:57 UTC (permalink / raw
To: Hugo Landau, qemu-devel; +Cc: qemu-arm, Peter Maydell, Andrew Jeffery
On 02/20/2018 02:26 PM, Hugo Landau wrote:
> Some register blocks of the ast2500 are protected by protection key
> registers which require the right magic value to be written to those
> registers to allow those registers to be mutated.
>
> Register manuals indicate that writing the correct magic value to these
> registers should cause subsequent reads from those values to return 1,
> and writing any other value should cause subsequent reads to return 0.
Yes indeed.
OpenBMC has a similar patch in its QEMU tree :
https://github.com/openbmc/qemu/commit/0529fa5e5947
but this one is better.
> Previously, qemu implemented these registers incorrectly: the registers
> were handled as simple memory, meaning that writing some value x to a
> protection key register would result in subsequent reads from that
> register returning the same value x. The protection was implemented by
> ensuring that the current value of that register equaled the magic
> value.
>
> This modifies qemu to have the correct behaviour: attempts to write to a
> ast2500 protection register results in a transition to 1 or 0 depending
> on whether the written value is the correct magic. The protection logic
> is updated to ensure that the value of the register is nonzero.
>
> This bug caused deadlocks with u-boot HEAD: when u-boot is done with a
> protectable register block, it attempts to lock it by writing the
> bitwise inverse of the correct magic value, and then spinning forever
> until the register reads as zero. Since qemu implemented writes to these
> registers as ordinary memory writes, writing the inverse of the magic
> value resulted in subsequent reads returning that value, leading to
> u-boot spinning forever.
>
> Signed-off-by: Hugo Landau <hlandau@devever.net>
With a minor comment below,
Reviewed-by: Cédric Le Goater <clg@kaod.org>
I also gave it a test on an OpenBMC romulus image. Looks fine, but that's
an old custom U-Boot. Which defconfig did you use for U-Boot HEAD ?
> ---
> hw/misc/aspeed_scu.c | 6 +++++-
> hw/misc/aspeed_sdmc.c | 8 +++++++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 74537ce975..5e6d5744ee 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -191,7 +191,7 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
> }
>
> if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
> - s->regs[PROT_KEY] != ASPEED_SCU_PROT_KEY) {
> + !s->regs[PROT_KEY]) {
> qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);
> return;
> }
> @@ -199,6 +199,10 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
> trace_aspeed_scu_write(offset, size, data);
>
> switch (reg) {
> + case PROT_KEY:
> + s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
> + return;
> +
> case FREQ_CNTR_EVAL:
> case VGA_SCRATCH1 ... VGA_SCRATCH8:
> case RNG_DATA:
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index f0b3053fae..265171ee42 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -110,7 +110,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
> return;
> }
>
> - if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
> + if (addr == R_PROT) {
> + s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
> + return;
> + }
> +
> + if (!s->regs[R_PROT]) {
> qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n", __func__);
> return;
> }
> @@ -123,6 +128,7 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
> data &= ~ASPEED_SDMC_READONLY_MASK;
> break;
> case AST2500_A0_SILICON_REV:
> + case AST2500_A1_SILICON_REV:
That's unrelated to the commit log, but I don't think you need to
resend for that.
Thanks,
C.
> data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> break;
> default:
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 13:57 ` Cédric Le Goater
@ 2018-02-20 14:19 ` Hugo Landau
2018-02-21 14:27 ` Cédric Le Goater
0 siblings, 1 reply; 8+ messages in thread
From: Hugo Landau @ 2018-02-20 14:19 UTC (permalink / raw
To: Cédric Le Goater; +Cc: qemu-devel
> I also gave it a test on an OpenBMC romulus image. Looks fine, but that's
> an old custom U-Boot. Which defconfig did you use for U-Boot HEAD ?
evb-ast2500_defconfig.
FYI, these changes are necessary, but not sufficient to get u-boot HEAD
(or for that matter u-boot 2017.11, another version tested) running.
The other issues were
- the tests
while (!(readl(®s->ecc_test_ctrl) & SDRAM_TEST_DONE));
and
while (!(readl(&info->regs->config) & SDRAM_CONF_CACHE_INIT_DONE));
which appear in various places in the u-boot source and which spin
forever. I made u-boot work by commenting these out in u-boot rather
than patching qemu, not familiar enough with qemu to implement this.
- the call to reset_assert in ast2500_sdrammc_probe seems to actually
reset the machine rather than just initialize SDRAM as it is
apparently supposed to, leading to an infinite cycle of resets.
Couldn't quite figure out how it was supposed to work, so I
commented this out, since obviously qemu doesn't actually have SDRAM
initialization requirements.
The above changes plus this patch allowed u-boot to get to the u-boot
CLI. Haven't tried booting anything with it yet though.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 13:26 [Qemu-devel] [PATCH] Fix ast2500 protection register emulation Hugo Landau
2018-02-20 13:57 ` Cédric Le Goater
@ 2018-02-20 23:01 ` Andrew Jeffery
2018-02-22 11:22 ` Peter Maydell
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Andrew Jeffery @ 2018-02-20 23:01 UTC (permalink / raw
To: qemu-devel
On Tue, 20 Feb 2018, at 23:56, Hugo Landau wrote:
> Some register blocks of the ast2500 are protected by protection key
> registers which require the right magic value to be written to those
> registers to allow those registers to be mutated.
>
> Register manuals indicate that writing the correct magic value to these
> registers should cause subsequent reads from those values to return 1,
> and writing any other value should cause subsequent reads to return 0.
>
> Previously, qemu implemented these registers incorrectly: the registers
> were handled as simple memory, meaning that writing some value x to a
> protection key register would result in subsequent reads from that
> register returning the same value x. The protection was implemented by
> ensuring that the current value of that register equaled the magic
> value.
>
> This modifies qemu to have the correct behaviour: attempts to write to a
> ast2500 protection register results in a transition to 1 or 0 depending
> on whether the written value is the correct magic. The protection logic
> is updated to ensure that the value of the register is nonzero.
>
> This bug caused deadlocks with u-boot HEAD: when u-boot is done with a
> protectable register block, it attempts to lock it by writing the
> bitwise inverse of the correct magic value, and then spinning forever
> until the register reads as zero. Since qemu implemented writes to these
> registers as ordinary memory writes, writing the inverse of the magic
> value resulted in subsequent reads returning that value, leading to
> u-boot spinning forever.
>
> Signed-off-by: Hugo Landau <hlandau@devever.net>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> hw/misc/aspeed_scu.c | 6 +++++-
> hw/misc/aspeed_sdmc.c | 8 +++++++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 74537ce975..5e6d5744ee 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -191,7 +191,7 @@ static void aspeed_scu_write(void *opaque, hwaddr
> offset, uint64_t data,
> }
>
> if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
> - s->regs[PROT_KEY] != ASPEED_SCU_PROT_KEY) {
> + !s->regs[PROT_KEY]) {
> qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n",
> __func__);
> return;
> }
> @@ -199,6 +199,10 @@ static void aspeed_scu_write(void *opaque, hwaddr
> offset, uint64_t data,
> trace_aspeed_scu_write(offset, size, data);
>
> switch (reg) {
> + case PROT_KEY:
> + s->regs[reg] = (data == ASPEED_SCU_PROT_KEY) ? 1 : 0;
> + return;
> +
> case FREQ_CNTR_EVAL:
> case VGA_SCRATCH1 ... VGA_SCRATCH8:
> case RNG_DATA:
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index f0b3053fae..265171ee42 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -110,7 +110,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr
> addr, uint64_t data,
> return;
> }
>
> - if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
> + if (addr == R_PROT) {
> + s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
> + return;
> + }
> +
> + if (!s->regs[R_PROT]) {
> qemu_log_mask(LOG_GUEST_ERROR, "%s: SDMC is locked!\n",
> __func__);
> return;
> }
> @@ -123,6 +128,7 @@ static void aspeed_sdmc_write(void *opaque, hwaddr
> addr, uint64_t data,
> data &= ~ASPEED_SDMC_READONLY_MASK;
> break;
> case AST2500_A0_SILICON_REV:
> + case AST2500_A1_SILICON_REV:
> data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
> break;
> default:
> --
> 2.15.0
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 14:19 ` Hugo Landau
@ 2018-02-21 14:27 ` Cédric Le Goater
0 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2018-02-21 14:27 UTC (permalink / raw
To: Hugo Landau; +Cc: qemu-devel
On 02/20/2018 03:19 PM, Hugo Landau wrote:
>> I also gave it a test on an OpenBMC romulus image. Looks fine, but that's
>> an old custom U-Boot. Which defconfig did you use for U-Boot HEAD ?
> evb-ast2500_defconfig.
ok
> FYI, these changes are necessary, but not sufficient to get u-boot HEAD
> (or for that matter u-boot 2017.11, another version tested) running.
>
> The other issues were
> - the tests
> while (!(readl(®s->ecc_test_ctrl) & SDRAM_TEST_DONE));
> and
> while (!(readl(&info->regs->config) & SDRAM_CONF_CACHE_INIT_DONE));
> which appear in various places in the u-boot source and which spin
> forever. I made u-boot work by commenting these out in u-boot rather
> than patching qemu, not familiar enough with qemu to implement this.
This patch :
https://github.com/openbmc/qemu/commit/4fb98fffd3115d8d3d0a16a1033f5335b5c0fd9b
fakes some more SDMC registers to let the SDRAM initialization run. you
might want to take a look at it.
Thanks,
C.
> - the call to reset_assert in ast2500_sdrammc_probe seems to actually
> reset the machine rather than just initialize SDRAM as it is
> apparently supposed to, leading to an infinite cycle of resets.
> Couldn't quite figure out how it was supposed to work, so I
> commented this out, since obviously qemu doesn't actually have SDRAM
> initialization requirements.
>
> The above changes plus this patch allowed u-boot to get to the u-boot
> CLI. Haven't tried booting anything with it yet though.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 13:26 [Qemu-devel] [PATCH] Fix ast2500 protection register emulation Hugo Landau
2018-02-20 13:57 ` Cédric Le Goater
2018-02-20 23:01 ` Andrew Jeffery
@ 2018-02-22 11:22 ` Peter Maydell
2018-02-22 12:46 ` no-reply
2018-02-24 0:53 ` no-reply
4 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2018-02-22 11:22 UTC (permalink / raw
To: Hugo Landau; +Cc: QEMU Developers, Cédric Le Goater
On 20 February 2018 at 13:26, Hugo Landau <hlandau@devever.net> wrote:
> Some register blocks of the ast2500 are protected by protection key
> registers which require the right magic value to be written to those
> registers to allow those registers to be mutated.
>
> Register manuals indicate that writing the correct magic value to these
> registers should cause subsequent reads from those values to return 1,
> and writing any other value should cause subsequent reads to return 0.
>
> Previously, qemu implemented these registers incorrectly: the registers
> were handled as simple memory, meaning that writing some value x to a
> protection key register would result in subsequent reads from that
> register returning the same value x. The protection was implemented by
> ensuring that the current value of that register equaled the magic
> value.
>
> This modifies qemu to have the correct behaviour: attempts to write to a
> ast2500 protection register results in a transition to 1 or 0 depending
> on whether the written value is the correct magic. The protection logic
> is updated to ensure that the value of the register is nonzero.
>
> This bug caused deadlocks with u-boot HEAD: when u-boot is done with a
> protectable register block, it attempts to lock it by writing the
> bitwise inverse of the correct magic value, and then spinning forever
> until the register reads as zero. Since qemu implemented writes to these
> registers as ordinary memory writes, writing the inverse of the magic
> value resulted in subsequent reads returning that value, leading to
> u-boot spinning forever.
>
> Signed-off-by: Hugo Landau <hlandau@devever.net>
> - if (addr != R_PROT && s->regs[R_PROT] != PROT_KEY_UNLOCK) {
> + if (addr == R_PROT) {
> + s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
> + return;
> + }
Applied to target-arm.next, thanks. I fixed up the incorrect indentation
in this part which checkpatch complains about.
-- PMM
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 13:26 [Qemu-devel] [PATCH] Fix ast2500 protection register emulation Hugo Landau
` (2 preceding siblings ...)
2018-02-22 11:22 ` Peter Maydell
@ 2018-02-22 12:46 ` no-reply
2018-02-24 0:53 ` no-reply
4 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2018-02-22 12:46 UTC (permalink / raw
To: hlandau; +Cc: famz, qemu-devel, clg
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180220132627.4163-1-hlandau@devever.net
Subject: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
8b610e4ed2 Fix ast2500 protection register emulation
=== OUTPUT BEGIN ===
Checking PATCH 1/1: Fix ast2500 protection register emulation...
ERROR: suspect code indent for conditional statements (4, 6)
#75: FILE: hw/misc/aspeed_sdmc.c:113:
+ if (addr == R_PROT) {
+ s->regs[addr] = (data == PROT_KEY_UNLOCK) ? 1 : 0;
total: 1 errors, 0 warnings, 38 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.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix ast2500 protection register emulation
2018-02-20 13:26 [Qemu-devel] [PATCH] Fix ast2500 protection register emulation Hugo Landau
` (3 preceding siblings ...)
2018-02-22 12:46 ` no-reply
@ 2018-02-24 0:53 ` no-reply
4 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2018-02-24 0:53 UTC (permalink / raw
To: hlandau; +Cc: famz, qemu-devel, clg
Hi,
This series failed build test on s390x host. Please find the details below.
N/A. Internal error while reading log file
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-02-24 12:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-20 13:26 [Qemu-devel] [PATCH] Fix ast2500 protection register emulation Hugo Landau
2018-02-20 13:57 ` Cédric Le Goater
2018-02-20 14:19 ` Hugo Landau
2018-02-21 14:27 ` Cédric Le Goater
2018-02-20 23:01 ` Andrew Jeffery
2018-02-22 11:22 ` Peter Maydell
2018-02-22 12:46 ` no-reply
2018-02-24 0:53 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.