From patchwork Tue Nov 9 12:47:02 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Frank Heimes X-Patchwork-Id: 1552970 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=canonical.com header.i=@canonical.com header.a=rsa-sha256 header.s=20210705 header.b=aJ7HOmA5; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4HpSRs14Swz9sPf for ; Tue, 9 Nov 2021 23:47:20 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1mkQWs-0003Oh-5W; Tue, 09 Nov 2021 12:47:10 +0000 Received: from smtp-relay-canonical-1.internal ([10.131.114.174] helo=smtp-relay-canonical-1.canonical.com) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1mkQWq-0003OC-P8 for kernel-team@lists.ubuntu.com; Tue, 09 Nov 2021 12:47:08 +0000 Received: from T570.fritz.box (p5b1759c5.dip0.t-ipconnect.de [91.23.89.197]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-canonical-1.canonical.com (Postfix) with ESMTPSA id 7C2193F17A for ; Tue, 9 Nov 2021 12:47:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=canonical.com; s=20210705; t=1636462028; bh=QeTniO5MlLMtlhNoCnCjtq/VlszUqwC28pN0vPLuN3k=; h=From:To:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=aJ7HOmA5JI+V4hkKeG42yGQLVEpHhP/iBSm9BJm+ZRwSkGE3vpuBmQcyhKFigj/yk uk7NpGLN3J14Oen3ieo/6mLqLahy5goK0V9RVlMWuv3//SMojyqKMdZqkJFWil6CoU i8Ip1zvCmpbYuh/9HwXSfTuuSD/TxR6LnMx1uTwkNiPLg0eBjQzHE7F7zSLqREByz4 WLUYJyniEsvW/oHLb6pQBMOaegcjxIVUXex5SQl/c56rxfk4HzS2VGamfFZ9q3BhoN YDCjj5Sn7SuQfUEMyC2hCBBAGx3rfiMSohst7q6XQim8pHW+yYlkN8XcDv61pbfD1G q64VOUuWAVEnQ== From: frank.heimes@canonical.com To: kernel-team@lists.ubuntu.com Subject: [SRU][I][H][F][PATCH 1/1] virtio: write back F_VERSION_1 before validate Date: Tue, 9 Nov 2021 13:47:02 +0100 Message-Id: <20211109124702.2369200-2-frank.heimes@canonical.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20211109124702.2369200-1-frank.heimes@canonical.com> References: <20211109124702.2369200-1-frank.heimes@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Halil Pasic BugLink: https://bugs.launchpad.net/bugs/1950144 The virtio specification virtio-v1.1-cs01 states: "Transitional devices MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not been acknowledged by the driver." This is exactly what QEMU as of 6.1 has done relying solely on VIRTIO_F_VERSION_1 for detecting that. However, the specification also says: "... the driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device ..." before setting FEATURES_OK. In that case, any transitional device relying solely on VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in legacy format. In particular, this implies that it is in big endian format for big endian guests. This naturally confuses the driver which expects little endian in the modern mode. It is probably a good idea to amend the spec to clarify that VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation is complete. Before validate callback existed, config space was only read after FEATURES_OK. However, we already have two regressions, so let's address this here as well. The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio 1.0 is used on both sides. The latter renders virtio-blk unusable with DASD backing, because things simply don't work with the default. See Fixes tags for relevant commits. For QEMU, we can work around the issue by writing out the feature bits with VIRTIO_F_VERSION_1 bit set. We (ab)use the finalize_features config op for this. This isn't enough to address all vhost devices since these do not get the features until FEATURES_OK, however it looks like the affected devices actually never handled the endianness for legacy mode correctly, so at least that's not a regression. No devices except virtio net and virtio blk seem to be affected. Long term the right thing to do is to fix the hypervisors. Cc: #v4.11 Signed-off-by: Halil Pasic Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space") Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range") Reported-by: markver@us.ibm.com Reviewed-by: Cornelia Huck Link: https://lore.kernel.org/r/20211011053921.1198936-1-pasic@linux.ibm.com Signed-off-by: Michael S. Tsirkin (cherry picked from commit 2f9a174f918e29608564c7a4e8329893ab604fb4) Signed-off-by: Frank Heimes Acked-by: Krzysztof Kozlowski --- drivers/virtio/virtio.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 59a05f1b8105..91627e744326 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -225,6 +225,17 @@ static int virtio_dev_probe(struct device *_d) driver_features_legacy = driver_features; } + /* + * Some devices detect legacy solely via F_VERSION_1. Write + * F_VERSION_1 to force LE config space accesses before FEATURES_OK for + * these when needed. + */ + if (drv->validate && !virtio_legacy_is_little_endian() + && device_features & BIT_ULL(VIRTIO_F_VERSION_1)) { + dev->features = BIT_ULL(VIRTIO_F_VERSION_1); + dev->config->finalize_features(dev); + } + if (device_features & (1ULL << VIRTIO_F_VERSION_1)) dev->features = driver_features & device_features; else