[Qemu-devel] [PATCH 0/1] ahci: do not remap clb/fis unconditionally

[Qemu-devel] [PATCH 0/1] ahci: do not remap clb/fis unconditionally

[John Snow]

This continues the IOMMU fix from 2.3, where we should not attempt
to remap the CLB or FIS RX buffers if the AHCI device is currently
running.

The same applies to migration: keep our mitts off these registers
unless the device is supposed to be on.

Does not impact backwards compatibility for the AHCI device.

==
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch ahci-iommu
https://github.com/jnsnow/qemu/tree/ahci-iommu

This version is tagged ahci-iommu-v1:
https://github.com/jnsnow/qemu/releases/tag/ahci-iommu-v1
==

John Snow (1):
  ahci: do not remap clb/fis unconditionally

 hw/ide/ahci.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 25 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/1] ahci: do not remap clb/fis unconditionally

[John Snow]

This continues the IOMMU fix from 2.3, where we should not attempt
to remap the CLB or FIS RX buffers if the AHCI device is currently
running.

The same applies to migration: keep our mitts off these registers
unless the device is supposed to be on.

Does not impact backwards compatibility for the AHCI device.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 25 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 8e36dec..9e5d862 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -198,6 +198,61 @@ 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.
+ *
+ * @return 0 on success, -1 on error.
+ */
+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 {
+            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);
+            pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON);
+        } else {
+            error_report("AHCI: DMA engine should be off, "
+                         "but appears to still be running");
+            return -1;
+        }
+    }
+
+    if (pr->cmd & PORT_CMD_FIS_RX) {
+        if (ahci_map_fis_address(ad)) {
+            pr->cmd |= PORT_CMD_FIS_ON;
+        } else {
+            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);
+            pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON);
+        } else {
+            error_report("AHCI: FIS receive engine should be off, "
+                         "but appears to still be running");
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
 static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
 {
     AHCIPortRegs *pr = &s->dev[port].port_regs;
@@ -229,29 +284,8 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
              * including LIST_ON and FIS_ON. */
             pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) | (val & ~PORT_CMD_RO_MASK);
 
-            if (pr->cmd & PORT_CMD_START) {
-                if (ahci_map_clb_address(&s->dev[port])) {
-                    pr->cmd |= PORT_CMD_LIST_ON;
-                } else {
-                    error_report("AHCI: Failed to start DMA engine: "
-                                 "bad command list buffer address");
-                }
-            } else if (pr->cmd & PORT_CMD_LIST_ON) {
-                ahci_unmap_clb_address(&s->dev[port]);
-                pr->cmd = pr->cmd & ~(PORT_CMD_LIST_ON);
-            }
-
-            if (pr->cmd & PORT_CMD_FIS_RX) {
-                if (ahci_map_fis_address(&s->dev[port])) {
-                    pr->cmd |= PORT_CMD_FIS_ON;
-                } else {
-                    error_report("AHCI: Failed to start FIS receive engine: "
-                                 "bad FIS receive buffer address");
-                }
-            } else if (pr->cmd & PORT_CMD_FIS_ON) {
-                ahci_unmap_fis_address(&s->dev[port]);
-                pr->cmd = pr->cmd & ~(PORT_CMD_FIS_ON);
-            }
+            /* Check FIS RX and CLB engines, allow transition to false: */
+            ahci_cond_start_engines(&s->dev[port], true);
 
             /* XXX usually the FIS would be pending on the bus here and
                    issuing deferred until the OS enables FIS receival.
@@ -1404,8 +1438,12 @@ static int ahci_state_post_load(void *opaque, int version_id)
     for (i = 0; i < s->ports; i++) {
         ad = &s->dev[i];
 
-        ahci_map_clb_address(ad);
-        ahci_map_fis_address(ad);
+        /* 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) {
+            return -1;
+        }
+
         /*
          * If an error is present, ad->busy_slot will be valid and not -1.
          * In this case, an operation is waiting to resume and will re-check
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/1] ahci: do not remap clb/fis unconditionally

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

On Tue, May 12, 2015 at 06:36:12PM -0400, John Snow wrote:
> This continues the IOMMU fix from 2.3, where we should not attempt
> to remap the CLB or FIS RX buffers if the AHCI device is currently
> running.
> 
> The same applies to migration: keep our mitts off these registers
> unless the device is supposed to be on.
> 
> Does not impact backwards compatibility for the AHCI device.
> 
> ==
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-iommu
> https://github.com/jnsnow/qemu/tree/ahci-iommu
> 
> This version is tagged ahci-iommu-v1:
> https://github.com/jnsnow/qemu/releases/tag/ahci-iommu-v1
> ==
> 
> John Snow (1):
>   ahci: do not remap clb/fis unconditionally
> 
>  hw/ide/ahci.c | 88 ++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 63 insertions(+), 25 deletions(-)
> 
> -- 
> 2.1.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/1] ahci: do not remap clb/fis unconditionally

[John Snow]

On 05/18/2015 05:57 AM, Stefan Hajnoczi wrote:
> On Tue, May 12, 2015 at 06:36:12PM -0400, John Snow wrote:
>> This continues the IOMMU fix from 2.3, where we should not
>> attempt to remap the CLB or FIS RX buffers if the AHCI device is
>> currently running.
>> 
>> The same applies to migration: keep our mitts off these
>> registers unless the device is supposed to be on.
>> 
>> Does not impact backwards compatibility for the AHCI device.
>> 
>> == For convenience, this branch is available at: 
>> https://github.com/jnsnow/qemu.git branch ahci-iommu 
>> https://github.com/jnsnow/qemu/tree/ahci-iommu
>> 
>> This version is tagged ahci-iommu-v1: 
>> https://github.com/jnsnow/qemu/releases/tag/ahci-iommu-v1 ==
>> 
>> John Snow (1): ahci: do not remap clb/fis unconditionally
>> 
>> hw/ide/ahci.c | 88
>> ++++++++++++++++++++++++++++++++++++++++++----------------- 1
>> file changed, 63 insertions(+), 25 deletions(-)
>> 
>> -- 2.1.0
>> 
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

Thanks, staged in my IDE branch:
https://github.com/jnsnow/qemu/commits/ide

--js