From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51875) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1apkAj-0000wP-Ft for qemu-devel@nongnu.org; Mon, 11 Apr 2016 18:19:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1apkAf-0000I2-5p for qemu-devel@nongnu.org; Mon, 11 Apr 2016 18:19:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56666) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1apkAe-0000HR-Ts for qemu-devel@nongnu.org; Mon, 11 Apr 2016 18:19:01 -0400 References: <1459924806-306-1-git-send-email-den@openvz.org> <1459924806-306-4-git-send-email-den@openvz.org> From: Eric Blake Message-ID: <570C22D2.8060401@redhat.com> Date: Mon, 11 Apr 2016 16:18:58 -0600 MIME-Version: 1.0 In-Reply-To: <1459924806-306-4-git-send-email-den@openvz.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1jUEw5goSvsvgpbOFHcissDWoDNFjrKC9" Subject: Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" , qemu-devel@nongnu.org Cc: Pavel Butsykin This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --1jUEw5goSvsvgpbOFHcissDWoDNFjrKC9 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/06/2016 12:40 AM, Denis V. Lunev wrote: > From: Pavel Butsykin >=20 > Restart of ATAPI DMA used to be unreachable, because the request to do > so wasn't indicated in bus->error_status due to the lack of spare bits,= and > ide_restart_bh() would return early doing nothing. >=20 > This patch makes use of the observation that not all bit combinations w= ere > possible in ->error_status. In particular, IDE_RETRY_READ only made sen= se > together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use > IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request. >=20 > To makes things more uniform, ATAPI DMA gets its own value for ->dma_cm= d. > As a means against confusion, macros are added to test the state of > ->error_status. >=20 > The patch fixes the restart of both in-flight and pending ATAPI DMA, > following the scheme similar to that of IDE DMA. >=20 > Signed-off-by: Pavel Butsykin > Signed-off-by: Denis V. Lunev > --- I'll leave the technical feasibility of this to others, but have some coding style comments: > @@ -783,8 +782,10 @@ static int ide_handle_rw_error(IDEState *s, int er= ror, int op) > s->bus->error_status =3D op; > } else if (action =3D=3D BLOCK_ERROR_ACTION_REPORT) { > block_acct_failed(blk_get_stats(s->blk), &s->acct); > - if (op & IDE_RETRY_DMA) { > + if (IS_IDE_RETRY_DMA(op)) { > ide_dma_error(s); I'd probably have split this into two patches; one adding the accessor macros for existing access, and the other adding the new bit pattern (mixing a conversion along with new code is a bit trickier to review in one patch). > +++ b/hw/ide/internal.h > @@ -338,6 +338,7 @@ enum ide_dma_cmd { > IDE_DMA_READ, > IDE_DMA_WRITE, > IDE_DMA_TRIM, > + IDE_DMA_ATAPI Please keep the trailing comma, so that the next addition to this enum won't have to adjust an existing line. > }; > =20 > #define ide_cmd_is_read(s) \ > @@ -508,11 +509,27 @@ struct IDEDevice { > /* These are used for the error_status field of IDEBus */ > #define IDE_RETRY_DMA 0x08 > #define IDE_RETRY_PIO 0x10 > +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */ > #define IDE_RETRY_READ 0x20 > #define IDE_RETRY_FLUSH 0x40 > #define IDE_RETRY_TRIM 0x80 > #define IDE_RETRY_HBA 0x100 Seems rather sparse on the comments about which bit patterns are valid together. If IDE_RETRY_READ is always used with at least one other bit, it might make more sense to have an IDE_RETRY_MASK that selects the set of bits being multiplexed, and/or macros that define the bits used in combination. Something along the lines of: #define IDE_RETRY_MASK 0x38 #define IDE_RETRY_READ_DMA 0x28 #define IDE_RETRY_READ_PIO 0x30 #define IDE_RETRY_ATAPI 0x20 > =20 > +#define IS_IDE_RETRY_DMA(_status) \ > + ((_status) & IDE_RETRY_DMA) > + > +#define IS_IDE_RETRY_PIO(_status) \ > + ((_status) & IDE_RETRY_PIO) and these seem prone to false positives; where it might be better to do: #define IS_IDE_RETRY_DMA(_status) \ (((_status) & IDE_RETRY_MASK) =3D=3D IDE_RETRY_READ_DMA) > + > +/* > + * The method of the IDE_RETRY_ATAPI determination is to use a previou= sly > + * impossible bit combination as a new status value. > + */ > +#define IS_IDE_RETRY_ATAPI(_status) \ > + (((_status) & IDE_RETRY_ATAPI) && \ > + !IS_IDE_RETRY_DMA(_status) && \ > + !IS_IDE_RETRY_PIO(_status)) > + And this evaluates _status more than once, compared to: #define IS_IDE_RETRY_ATAPI(_status) \ (((_status) & IDE_RETRY_MASK) =3D=3D IDE_RETRY_ATAPI) --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --1jUEw5goSvsvgpbOFHcissDWoDNFjrKC9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXDCLSAAoJEKeha0olJ0NqvfwH/jFxUq5oN1AF1pnMjad2XXn9 POsEad7Wg/vm+7WsVqzkwR//j4K7OmBwhs0a9az3BsoxnaX0cYTQChHCNJJa3fO3 vh9w/LLN+rBCAUw8UXfIfgaM5nU8JNSClAWP0pqnxBzCemEGaRdd4puPLdAoQzXc 9HhFCtoXkjzZiOJFI9wLRmSSF+0s26uoWTbnFFqg7fU25JTxBuD4cpG3XLwJC4Y+ +ARDjdWZDPJrFTsLs0s5N+RYuS8eeCEhuhEtBJZsRdUW7RhwpZ+E5agETiuAtp+W TY/iBpUEITW9auG12Kgz3xHfavmhXRhSAxDCT+/zXxcD/pHKxsQ42VoI4bUOVZg= =gp+r -----END PGP SIGNATURE----- --1jUEw5goSvsvgpbOFHcissDWoDNFjrKC9--