| Message ID | 1422999999-25868-11-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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)); +} + unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t px) { unsigned i; diff --git a/tests/libqos/ahci.h b/tests/libqos/ahci.h index 0d12582..d43210f 100644 --- a/tests/libqos/ahci.h +++ b/tests/libqos/ahci.h @@ -515,6 +515,7 @@ void ahci_get_command_header(AHCIQState *ahci, uint8_t px, void ahci_set_command_header(AHCIQState *ahci, uint8_t px, uint8_t cx, AHCICommandHeader *cmd); void ahci_destroy_command(AHCIQState *ahci, uint8_t px, uint8_t cx); +void ahci_write_fis(AHCIQState *ahci, RegH2DFIS *fis, uint64_t addr); unsigned ahci_pick_cmd(AHCIQState *ahci, uint8_t px); #endif
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(-)