From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJP9J-0000tk-E0 for qemu-devel@nongnu.org; Thu, 05 Feb 2015 11:19:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJP9E-0007Vb-CK for qemu-devel@nongnu.org; Thu, 05 Feb 2015 11:19:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59708) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJP9E-0007Ul-5P for qemu-devel@nongnu.org; Thu, 05 Feb 2015 11:19:20 -0500 Message-ID: <54D39804.2040503@redhat.com> Date: Thu, 05 Feb 2015 11:19:16 -0500 From: John Snow MIME-Version: 1.0 References: <1422999999-25868-1-git-send-email-jsnow@redhat.com> <1422999999-25868-11-git-send-email-jsnow@redhat.com> <20150205132927.GA19277@stefanha-thinkpad.redhat.com> In-Reply-To: <20150205132927.GA19277@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 10/19] qtest/ahci: add ahci_write_fis List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: famz@redhat.com, mst@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com 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 >> --- >> 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.