* [Qemu-devel] [PATCH 0/4] ahci: unmap fixes
@ 2016-01-29 21:41 John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
To: qemu-block
Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, John Snow
As reported by Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>, there's a problem
you can expose in AHCI by rewriting the command list buffer and/or FIS
receive buffer addresses, then re-starting the AHCI device before bringing
it to a stop. Depending on the success of the remap operations, you may
be able to transition the device to a state where it thinks it is "running"
but no longer has a guest memory mapping.
When you try to transition it to the stopped state, QEMU crashes.
Tighten up the start/stop conditions, and pepper in a paranoia check inside
of the unmap function.
________________________________________________________________________________
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-unmap-fixes
https://github.com/jnsnow/qemu/tree/ahci-unmap-fixes
This version is tagged ahci-unmap-fixes-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-unmap-fixes-v1
John Snow (4):
ahci: Do not unmap NULL addresses
ahci: handle LIST_ON and FIS_ON in map helpers
ahci: explicitly reject bad engine states on post_load
ahci: prohibit "restarting" the FIS or CLB engines
hw/ide/ahci.c | 96 ++++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 59 insertions(+), 37 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
@ 2016-01-29 21:41 ` John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers John Snow
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
To: qemu-block
Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, John Snow
Definitely don't try to unmap a garbage address.
Reported-by: Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 17f1cbd..cdc9299 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -661,6 +661,10 @@ static bool ahci_map_fis_address(AHCIDevice *ad)
static void ahci_unmap_fis_address(AHCIDevice *ad)
{
+ if (ad->res_fis == NULL) {
+ DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n");
+ return;
+ }
dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
DMA_DIRECTION_FROM_DEVICE, 256);
ad->res_fis = NULL;
@@ -677,6 +681,10 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
static void ahci_unmap_clb_address(AHCIDevice *ad)
{
+ if (ad->lst == NULL) {
+ DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n");
+ return;
+ }
dma_memory_unmap(ad->hba->as, ad->lst, 1024,
DMA_DIRECTION_FROM_DEVICE, 1024);
ad->lst = NULL;
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
@ 2016-01-29 21:41 ` John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load John Snow
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
To: qemu-block
Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, John Snow
Instead of relying on ahci_cond_start_engines to maintain the
engine status indicators itself, have the lower-layer CLB and FIS mapper
helpers do it themselves.
This makes the cond_start routine slightly nicer to read, and makes sure
that the status indicators will always be correct.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index cdc9299..0653396 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -209,9 +209,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
AHCIPortRegs *pr = &ad->port_regs;
if (pr->cmd & PORT_CMD_START) {
- if (ahci_map_clb_address(ad)) {
- pr->cmd |= PORT_CMD_LIST_ON;
- } else {
+ if (!ahci_map_clb_address(ad)) {
error_report("AHCI: Failed to start DMA engine: "
"bad command list buffer address");
return -1;
@@ -219,7 +217,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
} else if (pr->cmd & PORT_CMD_LIST_ON) {
if (allow_stop) {
ahci_unmap_clb_address(ad);
- pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON);
} else {
error_report("AHCI: DMA engine should be off, "
"but appears to still be running");
@@ -228,9 +225,7 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
}
if (pr->cmd & PORT_CMD_FIS_RX) {
- if (ahci_map_fis_address(ad)) {
- pr->cmd |= PORT_CMD_FIS_ON;
- } else {
+ if (!ahci_map_fis_address(ad)) {
error_report("AHCI: Failed to start FIS receive engine: "
"bad FIS receive buffer address");
return -1;
@@ -238,7 +233,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
} else if (pr->cmd & PORT_CMD_FIS_ON) {
if (allow_stop) {
ahci_unmap_fis_address(ad);
- pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON);
} else {
error_report("AHCI: FIS receive engine should be off, "
"but appears to still be running");
@@ -656,7 +650,13 @@ static bool ahci_map_fis_address(AHCIDevice *ad)
AHCIPortRegs *pr = &ad->port_regs;
map_page(ad->hba->as, &ad->res_fis,
((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
- return ad->res_fis != NULL;
+ if (ad->res_fis != NULL) {
+ pr->cmd |= PORT_CMD_FIS_ON;
+ return true;
+ }
+
+ pr->cmd &= ~PORT_CMD_FIS_ON;
+ return false;
}
static void ahci_unmap_fis_address(AHCIDevice *ad)
@@ -665,6 +665,7 @@ static void ahci_unmap_fis_address(AHCIDevice *ad)
DPRINTF(ad->port_no, "Attempt to unmap NULL FIS address\n");
return;
}
+ ad->port_regs.cmd &= ~PORT_CMD_FIS_ON;
dma_memory_unmap(ad->hba->as, ad->res_fis, 256,
DMA_DIRECTION_FROM_DEVICE, 256);
ad->res_fis = NULL;
@@ -676,7 +677,13 @@ static bool ahci_map_clb_address(AHCIDevice *ad)
ad->cur_cmd = NULL;
map_page(ad->hba->as, &ad->lst,
((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
- return ad->lst != NULL;
+ if (ad->lst != NULL) {
+ pr->cmd |= PORT_CMD_LIST_ON;
+ return true;
+ }
+
+ pr->cmd &= ~PORT_CMD_LIST_ON;
+ return false;
}
static void ahci_unmap_clb_address(AHCIDevice *ad)
@@ -685,6 +692,7 @@ static void ahci_unmap_clb_address(AHCIDevice *ad)
DPRINTF(ad->port_no, "Attempt to unmap NULL CLB address\n");
return;
}
+ ad->port_regs.cmd &= ~PORT_CMD_LIST_ON;
dma_memory_unmap(ad->hba->as, ad->lst, 1024,
DMA_DIRECTION_FROM_DEVICE, 1024);
ad->lst = NULL;
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers John Snow
@ 2016-01-29 21:41 ` John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines John Snow
2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
To: qemu-block
Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, John Snow
Currently, we let ahci_cond_start_engines reject weird configurations
where either the DMA (CLB) or FIS engines are said to be started, but
their matching on/off control bit is toggled off.
There should be no way to achieve this, since any time you toggle the
control bit off, the status bit should always follow synchronously.
Preparing for a refactor in cond_start_engines, move the rejection logic
straight up into post_load.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 0653396..e856ad5 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -217,10 +217,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
} else if (pr->cmd & PORT_CMD_LIST_ON) {
if (allow_stop) {
ahci_unmap_clb_address(ad);
- } else {
- error_report("AHCI: DMA engine should be off, "
- "but appears to still be running");
- return -1;
}
}
@@ -233,10 +229,6 @@ static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
} else if (pr->cmd & PORT_CMD_FIS_ON) {
if (allow_stop) {
ahci_unmap_fis_address(ad);
- } else {
- error_report("AHCI: FIS receive engine should be off, "
- "but appears to still be running");
- return -1;
}
}
@@ -1567,10 +1559,23 @@ static int ahci_state_post_load(void *opaque, int version_id)
int i, j;
struct AHCIDevice *ad;
NCQTransferState *ncq_tfs;
+ AHCIPortRegs *pr;
AHCIState *s = opaque;
for (i = 0; i < s->ports; i++) {
ad = &s->dev[i];
+ pr = &ad->port_regs;
+
+ if (!(pr->cmd & PORT_CMD_START) && (pr->cmd & PORT_CMD_LIST_ON)) {
+ error_report("AHCI: DMA engine should be off, but status bit "
+ "indicates it is still running.");
+ return -1;
+ }
+ if (!(pr->cmd & PORT_CMD_FIS_RX) && (pr->cmd & PORT_CMD_FIS_ON)) {
+ error_report("AHCI: FIS RX engine should be off, but status bit "
+ "indicates it is still running.");
+ return -1;
+ }
/* Only remap the CLB address if appropriate, disallowing a state
* transition from 'on' to 'off' it should be consistent here. */
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
` (2 preceding siblings ...)
2016-01-29 21:41 ` [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load John Snow
@ 2016-01-29 21:41 ` John Snow
2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
4 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2016-01-29 21:41 UTC (permalink / raw)
To: qemu-block
Cc: peter.maydell, pjp, qemu-devel, zuozhi.fzz, pbonzini, John Snow
If the FIS or DMA engines are already started, do not allow them to be
"restarted." As a side-effect of this change, the migration post-load
routine must be modified to cope. If the engines are listed as "on"
in the migrated registers, they must be cleared to allow the startup
routine to see the transition from "off" to "on".
As a second side-effect, the extra argument to ahci_cond_engine_start
is removed in favor of consistent behavior.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/ide/ahci.c | 39 ++++++++++++++++++++-------------------
1 file changed, 20 insertions(+), 19 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index e856ad5..f0b8263 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -198,38 +198,38 @@ static void map_page(AddressSpace *as, uint8_t **ptr, uint64_t addr,
* Check the cmd register to see if we should start or stop
* the DMA or FIS RX engines.
*
- * @ad: Device to engage.
- * @allow_stop: Allow device to transition from started to stopped?
- * 'no' is useful for migration post_load, which does not expect a transition.
+ * @ad: Device to dis/engage.
*
* @return 0 on success, -1 on error.
*/
-static int ahci_cond_start_engines(AHCIDevice *ad, bool allow_stop)
+static int ahci_cond_start_engines(AHCIDevice *ad)
{
AHCIPortRegs *pr = &ad->port_regs;
+ bool cmd_start = pr->cmd & PORT_CMD_START;
+ bool cmd_on = pr->cmd & PORT_CMD_LIST_ON;
+ bool fis_start = pr->cmd & PORT_CMD_FIS_RX;
+ bool fis_on = pr->cmd & PORT_CMD_FIS_ON;
- if (pr->cmd & PORT_CMD_START) {
+ if (cmd_start && !cmd_on) {
if (!ahci_map_clb_address(ad)) {
+ pr->cmd &= ~PORT_CMD_START;
error_report("AHCI: Failed to start DMA engine: "
"bad command list buffer address");
return -1;
}
- } else if (pr->cmd & PORT_CMD_LIST_ON) {
- if (allow_stop) {
- ahci_unmap_clb_address(ad);
- }
+ } else if (!cmd_start && cmd_on) {
+ ahci_unmap_clb_address(ad);
}
- if (pr->cmd & PORT_CMD_FIS_RX) {
+ if (fis_start && !fis_on) {
if (!ahci_map_fis_address(ad)) {
+ pr->cmd &= ~PORT_CMD_FIS_RX;
error_report("AHCI: Failed to start FIS receive engine: "
"bad FIS receive buffer address");
return -1;
}
- } else if (pr->cmd & PORT_CMD_FIS_ON) {
- if (allow_stop) {
- ahci_unmap_fis_address(ad);
- }
+ } else if (!fis_start && fis_on) {
+ ahci_unmap_fis_address(ad);
}
return 0;
@@ -271,8 +271,8 @@ static void ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) |
(val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
- /* Check FIS RX and CLB engines, allow transition to false: */
- ahci_cond_start_engines(&s->dev[port], true);
+ /* Check FIS RX and CLB engines */
+ ahci_cond_start_engines(&s->dev[port]);
/* XXX usually the FIS would be pending on the bus here and
issuing deferred until the OS enables FIS receival.
@@ -1577,9 +1577,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
return -1;
}
- /* Only remap the CLB address if appropriate, disallowing a state
- * transition from 'on' to 'off' it should be consistent here. */
- if (ahci_cond_start_engines(ad, false) != 0) {
+ /* After a migrate, the DMA/FIS engines are "off" and
+ * need to be conditionally restarted */
+ pr->cmd &= ~(PORT_CMD_LIST_ON | PORT_CMD_FIS_ON);
+ if (ahci_cond_start_engines(ad) != 0) {
return -1;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] ahci: unmap fixes
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
` (3 preceding siblings ...)
2016-01-29 21:41 ` [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines John Snow
@ 2016-02-08 16:53 ` John Snow
2016-02-09 17:38 ` P J P
4 siblings, 1 reply; 7+ messages in thread
From: John Snow @ 2016-02-08 16:53 UTC (permalink / raw)
To: qemu-block; +Cc: peter.maydell, zuozhi.fzz, pbonzini, pjp, qemu-devel
PJP, ping? Look good?
On 01/29/2016 04:41 PM, John Snow wrote:
> As reported by Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>, there's a problem
> you can expose in AHCI by rewriting the command list buffer and/or FIS
> receive buffer addresses, then re-starting the AHCI device before bringing
> it to a stop. Depending on the success of the remap operations, you may
> be able to transition the device to a state where it thinks it is "running"
> but no longer has a guest memory mapping.
>
> When you try to transition it to the stopped state, QEMU crashes.
>
> Tighten up the start/stop conditions, and pepper in a paranoia check inside
> of the unmap function.
>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-unmap-fixes
> https://github.com/jnsnow/qemu/tree/ahci-unmap-fixes
>
> This version is tagged ahci-unmap-fixes-v1:
> https://github.com/jnsnow/qemu/releases/tag/ahci-unmap-fixes-v1
>
> John Snow (4):
> ahci: Do not unmap NULL addresses
> ahci: handle LIST_ON and FIS_ON in map helpers
> ahci: explicitly reject bad engine states on post_load
> ahci: prohibit "restarting" the FIS or CLB engines
>
> hw/ide/ahci.c | 96 ++++++++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 59 insertions(+), 37 deletions(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] ahci: unmap fixes
2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
@ 2016-02-09 17:38 ` P J P
0 siblings, 0 replies; 7+ messages in thread
From: P J P @ 2016-02-09 17:38 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: peter.maydell, pbonzini, qemu-devel, zuozhi.fzz
> On Monday, 8 February 2016 10:23 PM, John Snow <jsnow@redhat.com> wrote:
>> PJP, ping? Look good?
Oops, sorry!
> On 01/29/2016 04:41 PM, John Snow wrote:
>> As reported by Zuozhi fzz <zuozhi.fzz@alibaba-inc.com>, there's a problem
>> you can expose in AHCI by rewriting the command list buffer and/or FIS
>> receive buffer addresses, then re-starting the AHCI device before bringing
>> it to a stop. Depending on the success of the remap operations, you may
>> be able to transition the device to a state where it thinks it is
>> "running" but no longer has a guest memory mapping.
>>
>> When you try to transition it to the stopped state, QEMU crashes.
>>
>> Tighten up the start/stop conditions, and pepper in a paranoia check inside
>> of the unmap function.
>>
>> John Snow (4):
>> ahci: Do not unmap NULL addresses
>> ahci: handle LIST_ON and FIS_ON in map helpers
>> ahci: explicitly reject bad engine states on post_load
>> ahci: prohibit "restarting" the FIS or CLB engines
Yes, they look good.
Thank you.
--- -P J P
http://feedmug.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-09 17:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-29 21:41 [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 1/4] ahci: Do not unmap NULL addresses John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 2/4] ahci: handle LIST_ON and FIS_ON in map helpers John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 3/4] ahci: explicitly reject bad engine states on post_load John Snow
2016-01-29 21:41 ` [Qemu-devel] [PATCH 4/4] ahci: prohibit "restarting" the FIS or CLB engines John Snow
2016-02-08 16:53 ` [Qemu-devel] [PATCH 0/4] ahci: unmap fixes John Snow
2016-02-09 17:38 ` P J P
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).