QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PULL 0/3] Ide patches
@ 2015-07-20 18:29 John Snow
  2015-07-20 18:29 ` [Qemu-devel] [PULL 1/3] qtest/ide: add another short PRDT test flavor John Snow
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: John Snow @ 2015-07-20 18:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, jsnow

The following changes since commit dcc8a3ab632d0f11a1bf3b08381cf0f93e616b9f:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2015-07-20 16:01:31 +0100)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 47c719964a8240c99d4b7a2b4695ae026c619b83:

  tests: Fix broken targets check-report-qtest-* (2015-07-20 14:26:41 -0400)

----------------------------------------------------------------

Notes:
 01: Tests changes made by the NCQ patchset, should be in 2.4.
 02: Bugfix.
 03: Test building fix.

----------------------------------------------------------------

Stefan Fritsch (1):
  ahci: Force ICC bits in PxCMD to zero

Stefan Hajnoczi (1):
  qtest/ide: add another short PRDT test flavor

Stefan Weil (1):
  tests: Fix broken targets check-report-qtest-*

 hw/ide/ahci.c    |  9 +++++++--
 tests/Makefile   |  1 +
 tests/ide-test.c | 27 +++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

-- 
2.1.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PULL 1/3] qtest/ide: add another short PRDT test flavor
  2015-07-20 18:29 [Qemu-devel] [PULL 0/3] Ide patches John Snow
@ 2015-07-20 18:29 ` John Snow
  2015-07-20 18:29 ` [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero John Snow
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2015-07-20 18:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, jsnow, Stefan Hajnoczi

From: Stefan Hajnoczi <stefanha@redhat.com>

The existing short PRDT test case does not transfer any data because the
first PRD is less than 1 sector.

This patch adds another short PRDT test case where the first sector can
be read but the PRDT is still smaller than the requested number of
sectors.  This exercises a different code path in ide_dma_cb().

Cc: John Snow <jsnow@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1435770571-9906-1-git-send-email-stefanha@redhat.com
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 78382e9..4a07e3a 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -339,6 +339,31 @@ static void test_bmdma_short_prdt(void)
     assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
 }
 
+static void test_bmdma_one_sector_short_prdt(void)
+{
+    uint8_t status;
+
+    /* Read 2 sectors but only give 1 sector in PRDT */
+    PrdtEntry prdt[] = {
+        {
+            .addr = 0,
+            .size = cpu_to_le32(0x200 | PRDT_EOT),
+        },
+    };
+
+    /* Normal request */
+    status = send_dma_request(CMD_READ_DMA, 0, 2,
+                              prdt, ARRAY_SIZE(prdt));
+    g_assert_cmphex(status, ==, 0);
+    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+
+    /* Abort the request before it completes */
+    status = send_dma_request(CMD_READ_DMA | CMDF_ABORT, 0, 2,
+                              prdt, ARRAY_SIZE(prdt));
+    g_assert_cmphex(status, ==, 0);
+    assert_bit_clear(inb(IDE_BASE + reg_status), DF | ERR);
+}
+
 static void test_bmdma_long_prdt(void)
 {
     uint8_t status;
@@ -592,6 +617,8 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/bmdma/setup", test_bmdma_setup);
     qtest_add_func("/ide/bmdma/simple_rw", test_bmdma_simple_rw);
     qtest_add_func("/ide/bmdma/short_prdt", test_bmdma_short_prdt);
+    qtest_add_func("/ide/bmdma/one_sector_short_prdt",
+                   test_bmdma_one_sector_short_prdt);
     qtest_add_func("/ide/bmdma/long_prdt", test_bmdma_long_prdt);
     qtest_add_func("/ide/bmdma/no_busmaster", test_bmdma_no_busmaster);
     qtest_add_func("/ide/bmdma/teardown", test_bmdma_teardown);
-- 
2.1.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero
  2015-07-20 18:29 [Qemu-devel] [PULL 0/3] Ide patches John Snow
  2015-07-20 18:29 ` [Qemu-devel] [PULL 1/3] qtest/ide: add another short PRDT test flavor John Snow
@ 2015-07-20 18:29 ` John Snow
  2015-07-21 11:38   ` Peter Maydell
  2015-07-20 18:29 ` [Qemu-devel] [PULL 3/3] tests: Fix broken targets check-report-qtest-* John Snow
  2015-07-21 10:18 ` [Qemu-devel] [PULL 0/3] Ide patches Peter Maydell
  3 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2015-07-20 18:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, Stefan Fritsch, jsnow

From: Stefan Fritsch <sf@sfritsch.de>

The AHCI spec requires that the HBA sets the ICC bits to zero after the
ICC change is done. Since we don't do any ICC change, force the bits to
zero all the time.

This fixes delays with some OSs (e.g. OpenBSD) waiting for the ICC bits
to change to 0.

Signed-off-by: Stefan Fritsch <sf@sfritsch.de>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: E1ZFpg7-00027N-HW@eru.sfritsch.de
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bb6a92f..48749c1 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -279,8 +279,13 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             break;
         case PORT_CMD:
             /* Block any Read-only fields from being set;
-             * including LIST_ON and FIS_ON. */
-            pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) | (val & ~PORT_CMD_RO_MASK);
+             * including LIST_ON and FIS_ON.
+             * The spec requires to set ICC bits to zero after the ICC change
+             * is done. We don't support ICC state changes, therefore always
+             * force the ICC bits to zero.
+             */
+            pr->cmd = (pr->cmd & PORT_CMD_RO_MASK) |
+                      (val & ~(PORT_CMD_RO_MASK|PORT_CMD_ICC_MASK));
 
             /* Check FIS RX and CLB engines, allow transition to false: */
             ahci_cond_start_engines(&s->dev[port], true);
-- 
2.1.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PULL 3/3] tests: Fix broken targets check-report-qtest-*
  2015-07-20 18:29 [Qemu-devel] [PULL 0/3] Ide patches John Snow
  2015-07-20 18:29 ` [Qemu-devel] [PULL 1/3] qtest/ide: add another short PRDT test flavor John Snow
  2015-07-20 18:29 ` [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero John Snow
@ 2015-07-20 18:29 ` John Snow
  2015-07-21 10:18 ` [Qemu-devel] [PULL 0/3] Ide patches Peter Maydell
  3 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2015-07-20 18:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: peter.maydell, jsnow, Stefan Weil

From: Stefan Weil <sw@weilnetz.de>

They need QTEST_QEMU_IMG. Without it, the tests raise an assertion:

$ make -C bin check-report-qtest-i386.xml
make: Entering directory 'bin'
GTESTER check-report-qtest-i386.xml
blkdebug: Suspended request 'A'
blkdebug: Resuming request 'A'
ahci-test: tests/libqos/libqos.c:162:
 mkimg: Assertion `qemu_img_path' failed.
main-loop: WARNING: I/O thread spun for 1000 iterations

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1437231284-17455-1-git-send-email-sw@weilnetz.de
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/Makefile b/tests/Makefile
index 2c4b8dc..8d26736 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -478,6 +478,7 @@ $(patsubst %, check-%, $(check-unit-y)): check-%: %
 
 $(patsubst %, check-report-qtest-%.xml, $(QTEST_TARGETS)): check-report-qtest-%.xml: $(check-qtest-y)
 	$(call quiet-command,QTEST_QEMU_BINARY=$*-softmmu/qemu-system-$* \
+		QTEST_QEMU_IMG=qemu-img$(EXESUF) \
 	  gtester -q $(GTESTER_OPTIONS) -o $@ -m=$(SPEED) $(check-qtest-$*-y),"GTESTER $@")
 
 check-report-unit.xml: $(check-unit-y)
-- 
2.1.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PULL 0/3] Ide patches
  2015-07-20 18:29 [Qemu-devel] [PULL 0/3] Ide patches John Snow
                   ` (2 preceding siblings ...)
  2015-07-20 18:29 ` [Qemu-devel] [PULL 3/3] tests: Fix broken targets check-report-qtest-* John Snow
@ 2015-07-21 10:18 ` Peter Maydell
  3 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2015-07-21 10:18 UTC (permalink / raw)
  To: John Snow; +Cc: QEMU Developers

On 20 July 2015 at 19:29, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit dcc8a3ab632d0f11a1bf3b08381cf0f93e616b9f:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2015-07-20 16:01:31 +0100)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 47c719964a8240c99d4b7a2b4695ae026c619b83:
>
>   tests: Fix broken targets check-report-qtest-* (2015-07-20 14:26:41 -0400)
>
> ----------------------------------------------------------------
>
> Notes:
>  01: Tests changes made by the NCQ patchset, should be in 2.4.
>  02: Bugfix.
>  03: Test building fix.
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero
  2015-07-20 18:29 ` [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero John Snow
@ 2015-07-21 11:38   ` Peter Maydell
  2015-07-21 12:55     ` Eric Blake
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2015-07-21 11:38 UTC (permalink / raw)
  To: John Snow; +Cc: Stefan Fritsch, QEMU Developers

On 20 July 2015 at 19:29, John Snow <jsnow@redhat.com> wrote:
> From: Stefan Fritsch <sf@sfritsch.de>
>
> The AHCI spec requires that the HBA sets the ICC bits to zero after the
> ICC change is done. Since we don't do any ICC change, force the bits to
> zero all the time.
>
> This fixes delays with some OSs (e.g. OpenBSD) waiting for the ICC bits
> to change to 0.

This change provokes a lot of clang sanitizer warnings:

/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:288:49: runtime
error: left shift of 15 by 28 places cannot be represented in type
'int'

PORT_CMD_ICC_MASK is defined as

 #define PORT_CMD_ICC_MASK         (0xf << 28) /* i/f ICC state mask */

which shifts into the sign bit of a signed integer.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero
  2015-07-21 11:38   ` Peter Maydell
@ 2015-07-21 12:55     ` Eric Blake
  2015-07-21 13:02       ` Peter Maydell
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2015-07-21 12:55 UTC (permalink / raw)
  To: Peter Maydell, John Snow; +Cc: Stefan Fritsch, QEMU Developers

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

On 07/21/2015 05:38 AM, Peter Maydell wrote:
> On 20 July 2015 at 19:29, John Snow <jsnow@redhat.com> wrote:
>> From: Stefan Fritsch <sf@sfritsch.de>
>>
>> The AHCI spec requires that the HBA sets the ICC bits to zero after the
>> ICC change is done. Since we don't do any ICC change, force the bits to
>> zero all the time.
>>
>> This fixes delays with some OSs (e.g. OpenBSD) waiting for the ICC bits
>> to change to 0.
> 
> This change provokes a lot of clang sanitizer warnings:
> 
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:288:49: runtime
> error: left shift of 15 by 28 places cannot be represented in type
> 'int'
> 
> PORT_CMD_ICC_MASK is defined as
> 
>  #define PORT_CMD_ICC_MASK         (0xf << 28) /* i/f ICC state mask */
> 
> which shifts into the sign bit of a signed integer.

Should be fixable by using (0xfU << 28), right?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero
  2015-07-21 12:55     ` Eric Blake
@ 2015-07-21 13:02       ` Peter Maydell
  2015-07-21 17:41         ` John Snow
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2015-07-21 13:02 UTC (permalink / raw)
  To: Eric Blake; +Cc: Stefan Fritsch, John Snow, QEMU Developers

On 21 July 2015 at 13:55, Eric Blake <eblake@redhat.com> wrote:
> On 07/21/2015 05:38 AM, Peter Maydell wrote:
>> On 20 July 2015 at 19:29, John Snow <jsnow@redhat.com> wrote:
>>> From: Stefan Fritsch <sf@sfritsch.de>
>>>
>>> The AHCI spec requires that the HBA sets the ICC bits to zero after the
>>> ICC change is done. Since we don't do any ICC change, force the bits to
>>> zero all the time.
>>>
>>> This fixes delays with some OSs (e.g. OpenBSD) waiting for the ICC bits
>>> to change to 0.
>>
>> This change provokes a lot of clang sanitizer warnings:
>>
>> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:288:49: runtime
>> error: left shift of 15 by 28 places cannot be represented in type
>> 'int'
>>
>> PORT_CMD_ICC_MASK is defined as
>>
>>  #define PORT_CMD_ICC_MASK         (0xf << 28) /* i/f ICC state mask */
>>
>> which shifts into the sign bit of a signed integer.
>
> Should be fixable by using (0xfU << 28), right?

Yes, though it assumes that if you say "~PORT_CMD_ICC_MASK"
you're happy to only get a 32-bit mask. 0xfULL would avoid
that (see discussion on the other thread with Paolo about
the PPC similar issue.)

-- PMM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero
  2015-07-21 13:02       ` Peter Maydell
@ 2015-07-21 17:41         ` John Snow
  0 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2015-07-21 17:41 UTC (permalink / raw)
  To: Peter Maydell, Eric Blake; +Cc: Stefan Fritsch, QEMU Developers



On 07/21/2015 09:02 AM, Peter Maydell wrote:
> On 21 July 2015 at 13:55, Eric Blake <eblake@redhat.com> wrote:
>> On 07/21/2015 05:38 AM, Peter Maydell wrote:
>>> On 20 July 2015 at 19:29, John Snow <jsnow@redhat.com> wrote:
>>>> From: Stefan Fritsch <sf@sfritsch.de>
>>>>
>>>> The AHCI spec requires that the HBA sets the ICC bits to zero after the
>>>> ICC change is done. Since we don't do any ICC change, force the bits to
>>>> zero all the time.
>>>>
>>>> This fixes delays with some OSs (e.g. OpenBSD) waiting for the ICC bits
>>>> to change to 0.
>>>
>>> This change provokes a lot of clang sanitizer warnings:
>>>
>>> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:288:49: runtime
>>> error: left shift of 15 by 28 places cannot be represented in type
>>> 'int'
>>>
>>> PORT_CMD_ICC_MASK is defined as
>>>
>>>  #define PORT_CMD_ICC_MASK         (0xf << 28) /* i/f ICC state mask */
>>>
>>> which shifts into the sign bit of a signed integer.
>>
>> Should be fixable by using (0xfU << 28), right?
> 
> Yes, though it assumes that if you say "~PORT_CMD_ICC_MASK"
> you're happy to only get a 32-bit mask. 0xfULL would avoid
> that (see discussion on the other thread with Paolo about
> the PPC similar issue.)
> 

I think we're happy to admit it's a simple 32bit mask, since it's just a
32bit field and I can't imagine us needing it for any other purpose
right now.

I'd be worried that ~(0xfULL) would be pretty much the wrong thing in
nearly all cases. Same for ~..UL.

I'll send a quick patch for Eric's suggestion.

> -- PMM
> 

Thanks,
--js

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-20 18:29 [Qemu-devel] [PULL 0/3] Ide patches John Snow
2015-07-20 18:29 ` [Qemu-devel] [PULL 1/3] qtest/ide: add another short PRDT test flavor John Snow
2015-07-20 18:29 ` [Qemu-devel] [PULL 2/3] ahci: Force ICC bits in PxCMD to zero John Snow
2015-07-21 11:38   ` Peter Maydell
2015-07-21 12:55     ` Eric Blake
2015-07-21 13:02       ` Peter Maydell
2015-07-21 17:41         ` John Snow
2015-07-20 18:29 ` [Qemu-devel] [PULL 3/3] tests: Fix broken targets check-report-qtest-* John Snow
2015-07-21 10:18 ` [Qemu-devel] [PULL 0/3] Ide patches Peter Maydell

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git