[Qemu-devel] [PATCH 0/4] ahci: clean up signature generation

[Qemu-devel] [PATCH 0/4] ahci: clean up signature generation

[John Snow]

Ultimately, clean up the signature generation and as a result, tidy up
the port_reset and init_d2h functions.

________________________________________________________________________________

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

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

John Snow (4):
  ahci: remove dead reset code
  ahci: fix signature generation
  ahci: remove cmd_fis argument from write_fis_d2h
  ahci: clean up initial d2h semantics

 hw/ide/ahci.c | 53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/4] ahci: remove dead reset code

[John Snow]

This check is dead due to an earlier conditional.
AHCI does not currently support hotplugging, so
checks to see if devices are present or not are useless.

Remove it.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 48749c1..d04a161 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -604,10 +604,7 @@ static void ahci_reset_port(AHCIState *s, int port)
     }
 
     s->dev[port].port_state = STATE_RUN;
-    if (!ide_state->blk) {
-        pr->sig = 0;
-        ide_state->status = SEEK_STAT | WRERR_STAT;
-    } else if (ide_state->drive_kind == IDE_CD) {
+    if (ide_state->drive_kind == IDE_CD) {
         pr->sig = SATA_SIGNATURE_CDROM;
         ide_state->lcyl = 0x14;
         ide_state->hcyl = 0xeb;
-- 
2.4.3

[Qemu-devel] [PATCH 2/4] ahci: fix signature generation

[John Snow]

The initial register device-to-host FIS no longer needs to specially
set certain fields, as these can be handled generically by setting those
fields explicitly with the signatures we want at port reset time.

(1) Signatures are decomposed into their four component registers and
    set upon (AHCI) port reset.
(2) the signature cache register is no longer set manually per-each
    device type, but instead just once during ahci_init_d2h.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d04a161..3c50ccb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -542,20 +542,31 @@ static void ahci_init_d2h(AHCIDevice *ad)
 {
     uint8_t init_fis[20];
     IDEState *ide_state = &ad->port.ifs[0];
+    AHCIPortRegs *pr = &ad->port_regs;
 
     memset(init_fis, 0, sizeof(init_fis));
 
-    init_fis[4] = 1;
-    init_fis[12] = 1;
-
-    if (ide_state->drive_kind == IDE_CD) {
-        init_fis[5] = ide_state->lcyl;
-        init_fis[6] = ide_state->hcyl;
-    }
+    /* We're emulating receiving the first Reg H2D Fis from the device;
+     * Update the SIG register, but otherwise procede as normal. */
+    pr->sig = (ide_state->hcyl << 24) |
+        (ide_state->lcyl << 16) |
+        (ide_state->sector << 8) |
+        (ide_state->nsector & 0xFF);
 
     ahci_write_fis_d2h(ad, init_fis);
 }
 
+static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
+{
+    IDEState *s = &ad->port.ifs[0];
+    s->hcyl = sig >> 24 & 0xFF;
+    s->lcyl = sig >> 16 & 0xFF;
+    s->sector = sig >> 8 & 0xFF;
+    s->nsector = sig & 0xFF;
+
+    DPRINTF(ad->port_no, "set hcyl:lcyl:sect:nsect = 0x%08x\n", sig);
+}
+
 static void ahci_reset_port(AHCIState *s, int port)
 {
     AHCIDevice *d = &s->dev[port];
@@ -605,13 +616,10 @@ static void ahci_reset_port(AHCIState *s, int port)
 
     s->dev[port].port_state = STATE_RUN;
     if (ide_state->drive_kind == IDE_CD) {
-        pr->sig = SATA_SIGNATURE_CDROM;
-        ide_state->lcyl = 0x14;
-        ide_state->hcyl = 0xeb;
-        DPRINTF(port, "set lcyl = %d\n", ide_state->lcyl);
+        ahci_set_signature(d, SATA_SIGNATURE_CDROM);\
         ide_state->status = SEEK_STAT | WRERR_STAT | READY_STAT;
     } else {
-        pr->sig = SATA_SIGNATURE_DISK;
+        ahci_set_signature(d, SATA_SIGNATURE_DISK);
         ide_state->status = SEEK_STAT | WRERR_STAT;
     }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/4] ahci: fix signature generation

[Stefan Hajnoczi]

On Tue, Sep 01, 2015 at 04:50:39PM -0400, John Snow wrote:
> The initial register device-to-host FIS no longer needs to specially
> set certain fields, as these can be handled generically by setting those
> fields explicitly with the signatures we want at port reset time.
> 
> (1) Signatures are decomposed into their four component registers and
>     set upon (AHCI) port reset.
> (2) the signature cache register is no longer set manually per-each
>     device type, but instead just once during ahci_init_d2h.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index d04a161..3c50ccb 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -542,20 +542,31 @@ static void ahci_init_d2h(AHCIDevice *ad)
>  {
>      uint8_t init_fis[20];
>      IDEState *ide_state = &ad->port.ifs[0];
> +    AHCIPortRegs *pr = &ad->port_regs;
>  
>      memset(init_fis, 0, sizeof(init_fis));
>  
> -    init_fis[4] = 1;
> -    init_fis[12] = 1;
> -
> -    if (ide_state->drive_kind == IDE_CD) {
> -        init_fis[5] = ide_state->lcyl;
> -        init_fis[6] = ide_state->hcyl;
> -    }
> +    /* We're emulating receiving the first Reg H2D Fis from the device;
> +     * Update the SIG register, but otherwise procede as normal. */

s/procede/proceed/

[Qemu-devel] [PATCH 3/4] ahci: remove cmd_fis argument from write_fis_d2h

[John Snow]

It's no longer used. We used to generate a D2H FIS based
upon the command FIS that prompted the update, but in reality,
the D2H FIS is generated purely from register state.

cmd_fis is vestigial, so get rid of it.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3c50ccb..aa605ec 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -47,7 +47,7 @@ do { \
 static void check_cmd(AHCIState *s, int port);
 static int handle_cmd(AHCIState *s, int port, uint8_t slot);
 static void ahci_reset_port(AHCIState *s, int port);
-static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis);
+static void ahci_write_fis_d2h(AHCIDevice *ad);
 static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit);
 static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
@@ -540,12 +540,9 @@ static void ahci_check_cmd_bh(void *opaque)
 
 static void ahci_init_d2h(AHCIDevice *ad)
 {
-    uint8_t init_fis[20];
     IDEState *ide_state = &ad->port.ifs[0];
     AHCIPortRegs *pr = &ad->port_regs;
 
-    memset(init_fis, 0, sizeof(init_fis));
-
     /* We're emulating receiving the first Reg H2D Fis from the device;
      * Update the SIG register, but otherwise procede as normal. */
     pr->sig = (ide_state->hcyl << 24) |
@@ -553,7 +550,7 @@ static void ahci_init_d2h(AHCIDevice *ad)
         (ide_state->sector << 8) |
         (ide_state->nsector & 0xFF);
 
-    ahci_write_fis_d2h(ad, init_fis);
+    ahci_write_fis_d2h(ad);
 }
 
 static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
@@ -755,7 +752,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
     ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
 }
 
-static void ahci_write_fis_d2h(AHCIDevice *ad, uint8_t *cmd_fis)
+static void ahci_write_fis_d2h(AHCIDevice *ad)
 {
     AHCIPortRegs *pr = &ad->port_regs;
     uint8_t *d2h_fis;
@@ -1410,7 +1407,7 @@ static void ahci_cmd_done(IDEDMA *dma)
     DPRINTF(ad->port_no, "cmd done\n");
 
     /* update d2h status */
-    ahci_write_fis_d2h(ad, NULL);
+    ahci_write_fis_d2h(ad);
 
     if (!ad->check_bh) {
         /* maybe we still have something to process, check later */
-- 
2.4.3

[Qemu-devel] [PATCH 4/4] ahci: clean up initial d2h semantics

[John Snow]

with write_fis_d2h and signature generation tidied up,
let's adjust the initial d2h semantics to make more sense.

The initial d2h is considered delivered if there is guest
memory to save it to.

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

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index aa605ec..2186801 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -47,7 +47,7 @@ do { \
 static void check_cmd(AHCIState *s, int port);
 static int handle_cmd(AHCIState *s, int port, uint8_t slot);
 static void ahci_reset_port(AHCIState *s, int port);
-static void ahci_write_fis_d2h(AHCIDevice *ad);
+static bool ahci_write_fis_d2h(AHCIDevice *ad);
 static void ahci_init_d2h(AHCIDevice *ad);
 static int ahci_dma_prepare_buf(IDEDMA *dma, int32_t limit);
 static void ahci_commit_buf(IDEDMA *dma, uint32_t tx_bytes);
@@ -297,7 +297,6 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             if ((pr->cmd & PORT_CMD_FIS_ON) &&
                 !s->dev[port].init_d2h_sent) {
                 ahci_init_d2h(&s->dev[port]);
-                s->dev[port].init_d2h_sent = true;
             }
 
             check_cmd(s, port);
@@ -543,14 +542,19 @@ static void ahci_init_d2h(AHCIDevice *ad)
     IDEState *ide_state = &ad->port.ifs[0];
     AHCIPortRegs *pr = &ad->port_regs;
 
-    /* We're emulating receiving the first Reg H2D Fis from the device;
-     * Update the SIG register, but otherwise procede as normal. */
-    pr->sig = (ide_state->hcyl << 24) |
-        (ide_state->lcyl << 16) |
-        (ide_state->sector << 8) |
-        (ide_state->nsector & 0xFF);
+    if (ad->init_d2h_sent) {
+        return;
+    }
 
-    ahci_write_fis_d2h(ad);
+    if (ahci_write_fis_d2h(ad)) {
+        ad->init_d2h_sent = true;
+        /* We're emulating receiving the first Reg H2D Fis from the device;
+         * Update the SIG register, but otherwise procede as normal. */
+        pr->sig = (ide_state->hcyl << 24) |
+            (ide_state->lcyl << 16) |
+            (ide_state->sector << 8) |
+            (ide_state->nsector & 0xFF);
+    }
 }
 
 static void ahci_set_signature(AHCIDevice *ad, uint32_t sig)
@@ -752,7 +756,7 @@ static void ahci_write_fis_pio(AHCIDevice *ad, uint16_t len)
     ahci_trigger_irq(ad->hba, ad, PORT_IRQ_PIOS_FIS);
 }
 
-static void ahci_write_fis_d2h(AHCIDevice *ad)
+static bool ahci_write_fis_d2h(AHCIDevice *ad)
 {
     AHCIPortRegs *pr = &ad->port_regs;
     uint8_t *d2h_fis;
@@ -760,7 +764,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad)
     IDEState *s = &ad->port.ifs[0];
 
     if (!ad->res_fis || !(pr->cmd & PORT_CMD_FIS_RX)) {
-        return;
+        return false;
     }
 
     d2h_fis = &ad->res_fis[RES_FIS_RFIS];
@@ -793,6 +797,7 @@ static void ahci_write_fis_d2h(AHCIDevice *ad)
     }
 
     ahci_trigger_irq(ad->hba, ad, PORT_IRQ_D2H_REG_FIS);
+    return true;
 }
 
 static int prdt_tbl_entry_size(const AHCI_SG *tbl)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 0/4] ahci: clean up signature generation

[John Snow]

On 09/01/2015 04:50 PM, John Snow wrote:
> Ultimately, clean up the signature generation and as a result, tidy up
> the port_reset and init_d2h functions.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-sigfix
> https://github.com/jnsnow/qemu/tree/ahci-sigfix
> 
> This version is tagged ahci-sigfix-v1:
> https://github.com/jnsnow/qemu/releases/tag/ahci-sigfix-v1
> 
> John Snow (4):
>   ahci: remove dead reset code
>   ahci: fix signature generation
>   ahci: remove cmd_fis argument from write_fis_d2h
>   ahci: clean up initial d2h semantics
> 
>  hw/ide/ahci.c | 53 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 

Ping -- Stefan, you had some comments on the predecessor to this patch
for 2.4, does this look saner to you?

Re: [Qemu-devel] [PATCH 0/4] ahci: clean up signature generation

[Stefan Hajnoczi]

On Tue, Sep 01, 2015 at 04:50:37PM -0400, John Snow wrote:
> Ultimately, clean up the signature generation and as a result, tidy up
> the port_reset and init_d2h functions.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch ahci-sigfix
> https://github.com/jnsnow/qemu/tree/ahci-sigfix
> 
> This version is tagged ahci-sigfix-v1:
> https://github.com/jnsnow/qemu/releases/tag/ahci-sigfix-v1
> 
> John Snow (4):
>   ahci: remove dead reset code
>   ahci: fix signature generation
>   ahci: remove cmd_fis argument from write_fis_d2h
>   ahci: clean up initial d2h semantics
> 
>  hw/ide/ahci.c | 53 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> -- 
> 2.4.3
> 

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

Re: [Qemu-devel] [PATCH 0/4] ahci: clean up signature generation

[John Snow]

On 09/17/2015 04:40 AM, Stefan Hajnoczi wrote:
> On Tue, Sep 01, 2015 at 04:50:37PM -0400, John Snow wrote:
>> Ultimately, clean up the signature generation and as a result, tidy up
>> the port_reset and init_d2h functions.
>>
>> ________________________________________________________________________________
>>
>> For convenience, this branch is available at:
>> https://github.com/jnsnow/qemu.git branch ahci-sigfix
>> https://github.com/jnsnow/qemu/tree/ahci-sigfix
>>
>> This version is tagged ahci-sigfix-v1:
>> https://github.com/jnsnow/qemu/releases/tag/ahci-sigfix-v1
>>
>> John Snow (4):
>>   ahci: remove dead reset code
>>   ahci: fix signature generation
>>   ahci: remove cmd_fis argument from write_fis_d2h
>>   ahci: clean up initial d2h semantics
>>
>>  hw/ide/ahci.c | 53 ++++++++++++++++++++++++++++++-----------------------
>>  1 file changed, 30 insertions(+), 23 deletions(-)
>>
>> -- 
>> 2.4.3
>>
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 

With the comment in patch 2 fixed:

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js