Re: [Qemu-devel] [PATCH v2 10/19] qtest/ahci: add ahci_write_fis

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Feb 05, 2015 at 11:19:16AM -0500, John Snow wrote:
> 
> 
> On 02/05/2015 08:29 AM, Stefan Hajnoczi wrote:
> >On Tue, Feb 03, 2015 at 04:46:30PM -0500, John Snow wrote:
> >>Similar to ahci_set_command_header, add a helper that takes an
> >>in-memory representation of a command FIS and writes it to guest
> >>memory, handling endianness as-needed.
> >>
> >>Signed-off-by: John Snow <jsnow@redhat.com>
> >>---
> >>  tests/ahci-test.c   |  2 +-
> >>  tests/libqos/ahci.c | 10 ++++++++++
> >>  tests/libqos/ahci.h |  1 +
> >>  3 files changed, 12 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/tests/ahci-test.c b/tests/ahci-test.c
> >>index 211274e..658956d 100644
> >>--- a/tests/ahci-test.c
> >>+++ b/tests/ahci-test.c
> >>@@ -728,7 +728,7 @@ static void ahci_test_identify(AHCIQState *ahci)
> >>      g_assert_cmphex(ahci_px_rreg(ahci, i, AHCI_PX_IS), ==, 0);
> >>
> >>      /* Commit the Command FIS to the Command Table */
> >>-    memwrite(table, &fis, sizeof(fis));
> >>+    ahci_write_fis(ahci, &fis, table);
> >>
> >>      /* Commit the PRD entry to the Command Table */
> >>      memwrite(table + 0x80, &prd, sizeof(prd));
> >>diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> >>index ec72627..7336781 100644
> >>--- a/tests/libqos/ahci.c
> >>+++ b/tests/libqos/ahci.c
> >>@@ -464,6 +464,16 @@ void ahci_destroy_command(AHCIQState *ahci, uint8_t px, uint8_t cx)
> >>      ahci->port[px].prdtl[cx] = 0;
> >>  }
> >>
> >>+void ahci_write_fis(AHCIQState *ahci, RegH2DFIS *fis, uint64_t addr)
> >>+{
> >>+    RegH2DFIS tmp = *fis;
> >>+
> >>+    /* All other FIS fields are 8 bit and do not need to be flipped. */
> >>+    tmp.count = cpu_to_le16(tmp.count);
> >>+
> >>+    memwrite(addr, &tmp, sizeof(tmp));
> >>+}
> >
> >This patch looks wrong because tmp.count is byteswapped now but not
> >before.  It actually works because the value is 0 so we never bothered
> >to assign it explicitly.
> >
> >I do wonder about the 'aux' field in the FIS struct.  It's uint32_t.
> >Although the tests never access it, should that field be byteswapped?
> >
> >Stefan
> >
> 
> The Aux field(s) is/are used for some NCQ subcommands, and the formatting
> varies per-command, so it's not (at the moment) possible to byte swap it
> automatically ahead of time.
> 
> So the answer is "sometimes, maybe, but we're not using it right now."
> 
> Also, yes, count /was/ wrong before. It's right now :) since the IDENTIFY
> test as it currently stands is very literal and script-ish, there was no
> need to swap the bits there before. The DMA test requires this, though.
> 
> If you'd like, I can add a comment or a note that the AUX fields are
> currently ignored.

We discussed the aux field on IRC.  Since its structure depends on the
command, it would be more appropraite to make it uint8_t aux[4] and
perhaps introduce a union if we actually start using that field.  That
way it's clear that this is not a uint32_t that needs byteswapping.

Stefan

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