From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0467A1CB526 for ; Thu, 10 Oct 2024 14:16:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=140.211.166.136 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728569772; cv=none; b=fRZrbKTpksclZ0c0JcTESgXpC7awQtKkMVwpxCs9PlJh1CoIKWNeLZ8HlNJuNr7KOOs+fk6jzfJhc5gWa/jzX6KJ8ll0Q6cxeWSBzvFpodBc+zevguwVLNGY4HGYZa2T+CgIklvnvzO6sFiFh46DSBDWee17Qd9Z++9Sxep7Rr0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728569772; c=relaxed/simple; bh=H7OVQVkmlp50arWBH4CMVe4fVD2e0/VM1ZzNEAmm3Yo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: In-Reply-To:Content-Type:Content-Disposition; b=oeeBx0XTGSiCdtO33ZEwqo+utF8e3VCUm4uyteRj1OhagujpxASlP5Z9IddgNY7ZiaTbBjgT0edJXIpAr94zTcQ+H1qfNeo1Lwb5QMcyja42TQsYyL+wnrghZOgQOl2P64HVnkwVNE7O8aiKAi7+upthUn4qRnWcqzhKl6xj5c4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=PlbIRHFo; arc=none smtp.client-ip=140.211.166.136 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="PlbIRHFo" Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id A314460B4B for ; Thu, 10 Oct 2024 14:16:10 +0000 (UTC) X-Virus-Scanned: amavis at osuosl.org X-Spam-Flag: NO X-Spam-Score: -5.79 X-Spam-Level: Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavis, port 10024) with ESMTP id LS7JkgoDGeP7 for ; Thu, 10 Oct 2024 14:16:08 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=170.10.129.124; helo=us-smtp-delivery-124.mimecast.com; envelope-from=mst@redhat.com; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp3.osuosl.org 4B0BF60A69 Authentication-Results: smtp3.osuosl.org; dmarc=pass (p=none dis=none) header.from=redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 4B0BF60A69 Authentication-Results: smtp3.osuosl.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=PlbIRHFo Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by smtp3.osuosl.org (Postfix) with ESMTPS id 4B0BF60A69 for ; Thu, 10 Oct 2024 14:16:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1728569766; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=hnXbQWY2la7bHGRN3tmpJWIXjnTy7RCcsDoClnyV/ZM=; b=PlbIRHFoM0RTUbmKu9ZSJuYJfs9vUzDSBFRgbDJ6tyQGRfy+c0RdWdO+PnEDKuHehIYvzK vWjlm7VDcWT1zgQHIRqJTNeUmokynyl7PknRtpjsMm6XoILI98QZNPb/yS8JGAXlrabHg/ c8k1oFh2WjQ3/Q68S4Dcbe3QectJDnA= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-367-HixqfPIiNUaCfd2vsiqBAg-1; Thu, 10 Oct 2024 10:16:05 -0400 X-MC-Unique: HixqfPIiNUaCfd2vsiqBAg-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-43113dab986so8538025e9.2 for ; Thu, 10 Oct 2024 07:16:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728569762; x=1729174562; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=hnXbQWY2la7bHGRN3tmpJWIXjnTy7RCcsDoClnyV/ZM=; b=ok/0imsKfqxuNnwRCl6UG4EcH/uCU4MCAXwuUfLNn4lfeG5PwP8A1Bv5wDCEKzLAwG QdMhU8lPr2NKoyUDqIJonC+QY2CL6sI8IJI+/d9RTyroro/EHo6NwgfTrUXfBZpxgwvj 7LeVX3xAd00GN0qWMUgdMDGeGtjyYDuqD+7ZsGotLClzrgAKtgbzoMjIM90Y6po/4bJS KSCRwsIx/mnkzJfMASkweBrJCxRtCUpsT3Nl2Ir8hCVtpFvO8ApjileL9z3/8jKH3rkx sdglxcDZDdQtgmmiEiR/5vfQ+HvS03lx1sw5f39muZdKPmbSMIaSumQ9eC3jHQujJBmC CoyA== X-Forwarded-Encrypted: i=1; AJvYcCUAqsQ56HuCGsnGz8X7MkyQX0UOtjNv2F3YioJB29UjhQqHMekUkTzJKneloaHaJZ2b9GWFelGMHpjZl4fL7Q==@lists.linux-foundation.org X-Gm-Message-State: AOJu0YyF94pR+mBqjs4CS2Od90qJC/9C9kNtu0jDTVtAIJrQQHU4VfIv 4gpKJjnTXKkM1f8DwqDWlmQU6oZe1H9ocoJJPZe2OGviriglhX63VPdP917U8j23IEVqqaMva+1 PT5MlAbxVCUZHBgctKzdSckdTREy4sgfmHiZP/ptvFfGM4DhE4hoVIP/0wki2+PDmzUYtUjd+mz FxG+M= X-Received: by 2002:a05:600c:458a:b0:42c:b62c:9f0d with SMTP id 5b1f17b1804b1-430ccf432aamr77059335e9.17.1728569762180; Thu, 10 Oct 2024 07:16:02 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEbc7ZgEO6oTw7aFDJ8bb8UXfHnaMLSJbOPjwcffSHpNJiFT4jfQ32Fn/4ykgSL8BZMmS6Qfw== X-Received: by 2002:a05:600c:458a:b0:42c:b62c:9f0d with SMTP id 5b1f17b1804b1-430ccf432aamr77058895e9.17.1728569761762; Thu, 10 Oct 2024 07:16:01 -0700 (PDT) Received: from redhat.com ([2a02:14f:17d:fb13:fd72:22f:64e2:b824]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-37d4b79f69dsm1662112f8f.81.2024.10.10.07.15.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Oct 2024 07:16:00 -0700 (PDT) Date: Thu, 10 Oct 2024 10:15:57 -0400 From: "Michael S. Tsirkin" To: Max Gurtovoy Cc: Matias Ezequiel Vara Larsen , dverkamp@chromium.org, virtualization@lists.linux-foundation.org, stefanha@redhat.com, virtio-dev@lists.linux.dev, oren@nvidia.com, parav@nvidia.com, nitzanc@nvidia.com, benwalker@nvidia.com Subject: Re: [PATCH v3 1/1] virtio-blk: Add description for blk_size field Message-ID: <20241010101235-mutt-send-email-mst@kernel.org> References: <20241009160512.8390-1-mgurtovoy@nvidia.com> Precedence: bulk X-Mailing-List: virtualization@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Oct 10, 2024 at 05:04:55PM +0300, Max Gurtovoy wrote: > > On 10/10/2024 13:24, Matias Ezequiel Vara Larsen wrote: > > Hello, > > > > I wrote some minor comments below. > > > > On Wed, Oct 09, 2024 at 07:05:12PM +0300, Max Gurtovoy wrote: > > > This field is only valid when the VIRTIO_BLK_F_BLK_SIZE feature bit is > > > offered by the device. > > > > > > The blk_size field actually represents the logical block size of the > > > device. It is always a power of two and typically ranges from 512 bytes > > > to larger values such as 4 KB. > > > > > > Add description for this field to provide clarity on its constraints. > > > > > > Reviewed-by: Daniel Verkamp > > > Reviewed-by: Stefan Hajnoczi > > > Signed-off-by: Max Gurtovoy > > > --- > > > device-types/blk/description.tex | 34 ++++++++++++++++++++++++++++++++ > > > 1 file changed, 34 insertions(+) > > > > > > diff --git a/device-types/blk/description.tex b/device-types/blk/description.tex > > > index 2712ada..caa5d13 100644 > > > --- a/device-types/blk/description.tex > > > +++ b/device-types/blk/description.tex > > > @@ -135,6 +135,9 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device / > > > present. The availability of the others all depend on various feature > > > bits as indicated above. > > > +The field \field{blk_size} exists only if VIRTIO_BLK_F_BLK_SIZE is offered by the device. > > > +This field reports the block size of the device, expressed in bytes. > > > + > > > The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies > > > the number of queues. > > > @@ -282,6 +285,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic > > > \drivernormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization} > > > +Drivers SHOULD negotiate VIRTIO_BLK_F_BLK_SIZE if the feature is offered by the > > > +device. When negotiated, drivers SHOULD interpret the \field{blk_size} as the > > > +logical block size. > > > + > > > +If the VIRTIO_BLK_F_BLK_SIZE feature is not offered by the device, then drivers > > > +MAY assume that the logical block size is 512 bytes. > > > + > > > Drivers SHOULD NOT negotiate VIRTIO_BLK_F_FLUSH if they are incapable of > > > sending VIRTIO_BLK_T_FLUSH commands. > > > @@ -319,6 +329,10 @@ \subsection{Device Initialization}\label{sec:Device Types / Block Device / Devic > > > \devicenormative{\subsubsection}{Device Initialization}{Device Types / Block Device / Device Initialization} > > > +Devices SHOULD always offer VIRTIO_BLK_F_BLK_SIZE feature. When this feature is > > > +offered, devices MUST initialize \field{blk_size} to a power of two greater > > > +than or equal to 512. > > > + > > > Devices SHOULD always offer VIRTIO_BLK_F_FLUSH, and MUST offer it > > > if they offer VIRTIO_BLK_F_CONFIG_WCE. > > > @@ -879,6 +893,14 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope > > > The length of \field{data} MUST be a multiple of 512 bytes for VIRTIO_BLK_T_IN > > > and VIRTIO_BLK_T_OUT requests. > > > +The length of \field{data} SHOULD be a multiple of \field{blk_size} for VIRTIO_BLK_T_IN > > > +and VIRTIO_BLK_T_OUT requests, when the VIRTIO_BLK_F_BLK_SIZE feature has been > > > +offered by the device. > > > + > > I would rewrite it as: > > > > `When the VIRTIO_BLK_F_BLK_SIZE feature has been offered by the device, > > the length of \field{data} SHOULD be a multiple of \field{blk_size} for > > VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests` > > > > Just because people tend to read from more important to less important > > within a sentence but I do not have an strong opinion about it. > > Apologies if I may sound bit picky ;). > > I don't have a strong opinion on it as well. > > I'm open to making changes, but I'd appreciate getting input from MST/Stefan > first. > > This way, we can ensure we're aligning with their perspective and avoid > potential back-and-forth revisions. Please put the condition first, then the result, this is standard english. You can also combine multiple things then. Also, it should be "is offered". > > > > > +The value of \field{sector} (multiplied by 512) SHOULD be a multiple of > > > +\field{blk_size} for VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests, when the > > > +VIRTIO_BLK_F_BLK_SIZE feature has offered by the device. > > > + > > For the same reason, I would rewrite it as: > > > > `When the VIRTIO_BLK_F_BLK_SIZE feature has offered by the device, the has offered is agrammatical. > > value of \field{sector} (multiplied by 512) SHOULD be a multiple of > > \field{blk_size} for VIRTIO_BLK_T_IN and VIRTIO_BLK_T_OUT requests`. > > > > I think I would follow this pattern in the text below too. > > > > > The length of \field{data} MUST be a multiple of the size of struct > > > virtio_blk_discard_write_zeroes for VIRTIO_BLK_T_DISCARD, > > > VIRTIO_BLK_T_SECURE_ERASE and VIRTIO_BLK_T_WRITE_ZEROES requests. > > > @@ -966,6 +988,18 @@ \subsection{Device Operation}\label{sec:Device Types / Block Device / Device Ope > > > for a write request if the VIRTIO_BLK_F_RO feature if offered, and MUST NOT > > > write any data. > > > +A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for > > > +VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the requested > > > +\field{sector} (multiplied by 512) is not an integer multiple of the device's > > > +\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by > > > +the device. > > > + > > > +A device MAY set the \field{status} to VIRTIO_BLK_S_IOERR for > > > +VIRTIO_BLK_T_IN or VIRTIO_BLK_T_OUT requests if the length of the > > > +requested \field{data} is not an integer multiple of the device's > > > +\field{blk_size}, when the VIRTIO_BLK_F_BLK_SIZE feature has been offered by > > > +the device. > > > + > > > The device MUST set the \field{status} byte to VIRTIO_BLK_S_UNSUPP for > > > discard, secure erase and write zeroes commands if any unknown flag is set. > > > Furthermore, the device MUST set the \field{status} byte to > > > -- > > > 2.18.1 > > > > > >