admin virtqueue fixes

PATCH In review Parav Pandit 2 participants 10 messages May 1, 2023 – May 2, 2023

lore

Share & tools

lore

Parav Pandit

author

2023-05-01 22:45 UTC #0 lore raw

cover admin virtqueue fixes

Hi Michael,

Please review these two small fixes for the admin virtqueue patches. They are on top of your work of v12 at [1].

Patch summary:
patch-1 removes PCI transport dependency on deprecated migration bit patch-2 fixes admin command command data type to be u8 similar to result

[1] https://lore.kernel.org/virtio-comment/[email protected]/T/#t

Parav Pandit (2):
admin: Remove dependency on the deprecated register
admin: Make optional command data type to u8

 admin.tex | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Signature · 6 lines

2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Parav Pandit

author

2023-05-01 22:44 UTC #1 lore raw

PATCH 1/2 admin: Remove dependency on the deprecated register

'VF Migration Capable' is already deprecated as per PCI base specification 6.0.1 section 9.3.3.2.1. It was long ago deprecated which was meant for MR-IOV.

Hence, remove virtio PCI specification to not depend on such deprecated bits.

This patch is on top of [1].

[1] https://lore.kernel.org/virtio-comment/[email protected]/T/#t

Signed-off-by Parav Pandit <[email protected]>

---
 admin.tex | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

admin.tex +5 -8

index 2efd4d7..037e2e6 100644
@@ -372,9 +372,8 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL and
 \field{status_qualifier} set to
 VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER;
-\field{NumVFs}, \field{VF Migration Capable}  and
-\field{VF Enable} refer to registers within the SR-IOV Extended
-Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
+\field{NumVFs} and \field{VF Enable} refer to registers within the
+SR-IOV Extended Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
 
 \drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}

@@ -423,11 +422,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 the driver specify a value for \field{group_member_id}
 between $1$ and \field{NumVFs} inclusive,
 the driver MUST also make sure that as long as any such command
-is outstanding, \field{VF Migration Capable} is clear and
-\field{VF Enable} is set;
-\field{NumVFs}, \field{VF Migration Capable}  and
-\field{VF Enable} refer to registers within the SR-IOV Extended
-Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
+is outstanding, is clear and \field{VF Enable} is set;
+\field{NumVFs} and \field{VF Enable} refer to registers within the
+SR-IOV Extended Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
 
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}

Signature · 18 lines

2.26.2


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Michael S. Tsirkin

2023-05-02 07:39 UTC #2 lore raw

[virtio-comment] Re: [PATCH 1/2] admin: Remove dependency on the deprecated register

Quoted 4 lines · Parav Pandit

'VF Migration Capable' is already deprecated as per PCI base specification 6.0.1 section 9.3.3.2.1. It was long ago deprecated which was meant for MR-IOV.

Quoted 8 lines

Hence, remove virtio PCI specification to not depend on such deprecated bits.
This patch is on top of [1].
[1] https://lore.kernel.org/virtio-comment/[email protected]/T/#t
Signed-off-by Parav Pandit <[email protected]>

OK you know best I guess. I will drop mentions of VF Migration Capable.

Quoted 37 lines

---
 admin.tex | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

admin.tex +5 -8

index 2efd4d7..037e2e6 100644
@@ -372,9 +372,8 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL and
 \field{status_qualifier} set to
 VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER;
-\field{NumVFs}, \field{VF Migration Capable}  and
-\field{VF Enable} refer to registers within the SR-IOV Extended
-Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
+\field{NumVFs} and \field{VF Enable} refer to registers within the
+SR-IOV Extended Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
 
 \drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}

@@ -423,11 +422,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 the driver specify a value for \field{group_member_id}
 between $1$ and \field{NumVFs} inclusive,
 the driver MUST also make sure that as long as any such command
-is outstanding, \field{VF Migration Capable} is clear and
-\field{VF Enable} is set;
-\field{NumVFs}, \field{VF Migration Capable}  and
-\field{VF Enable} refer to registers within the SR-IOV Extended
-Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
+is outstanding, is clear and \field{VF Enable} is set;
+\field{NumVFs} and \field{VF Enable} refer to registers within the
+SR-IOV Extended Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
 
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}

Signature · 1 line

2.26.2

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Subscribe: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Parav Pandit

author

2023-05-01 22:45 UTC #3 lore raw

PATCH 2/2 admin: Make optional command data type to u8

In reply to Parav Pandit

Command data content is specific to command opcode. This is similar to command result field command_specific_result.

Hence, make it u8.

This patch is on top of [1].

[1] https://lore.kernel.org/virtio-comment/[email protected]/T/#t

Signed-off-by Parav Pandit <[email protected]>

---
 admin.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

admin.tex +1 -1

index 037e2e6..648253c 100644
@@ -85,7 +85,7 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
         /* unused, reserved for future extensions */
         u8 reserved1[12];
         le64 group_member_id;
-        le64 command_specific_data[];
+        u8 command_specific_data[];
 
         /* Device-writable part */
         le16 status;

Signature · 6 lines

2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Michael S. Tsirkin

2023-05-02 07:38 UTC #4 lore raw

[virtio-comment] Re: [PATCH 2/2] admin: Make optional command data type to u8

Quoted 11 lines · Parav Pandit

Command data content is specific to command opcode. This is similar to command result field command_specific_result.
Hence, make it u8.
This patch is on top of [1].
[1] https://lore.kernel.org/virtio-comment/[email protected]/T/#t
Signed-off-by Parav Pandit <[email protected]>

Thanks for the patch. It was like this originally but readers were confused. The point of marking it le64 is to make it clear it's size is a multiple of 8 bytes, thus 64 bits. And format of fields inside it is generally LE.

Quoted 19 lines

---
 admin.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

admin.tex +1 -1

index 037e2e6..648253c 100644
@@ -85,7 +85,7 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
         /* unused, reserved for future extensions */
         u8 reserved1[12];
         le64 group_member_id;
-        le64 command_specific_data[];
+        u8 command_specific_data[];
 
         /* Device-writable part */
         le16 status;

Signature · 1 line

2.26.2

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Parav Pandit

author

2023-05-02 11:29 UTC #5 lore raw

[virtio-comment] RE: [PATCH 2/2] admin: Make optional command data type to u8

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, May 2, 2023 3:38 AM
Thanks for the patch. It was like this originally but readers were confused. The point of marking it le64 is to make it clear it's size is a multiple of 8 bytes, thus 64 bits. And format of fields inside it is generally LE.

If it is supposed to be le64, the result should be same data type too, isn't it?

Quoted 5 lines

---
 admin.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 16 lines

admin.tex \subsection{Group administration

Quoted 5 lines

admin.tex +0 -0

index 037e2e6..648253c 100644
@@ -85,7 +85,7 @@ \subsection{Group administration

commands}\label{sec:Basic Facilities of a Virti

Quoted 10 lines

admin.tex +1 -2

         /* unused, reserved for future extensions */
         u8 reserved1[12];
         le64 group_member_id;
-        le64 command_specific_data[];
+        u8 command_specific_data[];
 
         /* Device-writable part */
         le16 status;
--

2.26.2

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Michael S. Tsirkin

2023-05-02 11:36 UTC #6 lore raw

[virtio-comment] Re: [PATCH 2/2] admin: Make optional command data type to u8

Quoted 9 lines · Parav Pandit

Quoted 7 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, May 2, 2023 3:38 AM
Thanks for the patch. It was like this originally but readers were confused. The point of marking it le64 is to make it clear it's size is a multiple of 8 bytes, thus 64 bits. And format of fields inside it is generally LE.
If it is supposed to be le64, the result should be same data type too, isn't it?

Oh. Good point. Will fix.

Quoted 17 lines

admin.tex \subsection{Group administration

Quoted 10 lines

---
 admin.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

admin.tex +0 -0

index 037e2e6..648253c 100644
@@ -85,7 +85,7 @@ \subsection{Group administration

commands}\label{sec:Basic Facilities of a Virti

Quoted 6 lines

admin.tex +1 -1

         /* unused, reserved for future extensions */
         u8 reserved1[12];
         le64 group_member_id;
-        le64 command_specific_data[];
+        u8 command_specific_data[];

Quoted 4 lines

admin.tex +0 -1

         /* Device-writable part */
         le16 status;
--

2.26.2

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Michael S. Tsirkin

2023-05-02 07:41 UTC #7 lore raw

In reply to Parav Pandit

Quoted 11 lines · Parav Pandit

Hi Michael,
Please review these two small fixes for the admin virtqueue patches. They are on top of your work of v12 at [1].
Patch summary:
patch-1 removes PCI transport dependency on deprecated migration bit patch-2 fixes admin command command data type to be u8 similar to result
[1] https://lore.kernel.org/virtio-comment/[email protected]/T/#t

I can take 1/2 since that's just removing text. Taking 2/2 will mean more review time before vote - besides me not really liking that change, are you sure it's worth it?
Was going to start voting today but if we are still tweaking command format then I can't ...

Quoted 9 lines

Parav Pandit (2):
admin: Remove dependency on the deprecated register
admin: Make optional command data type to u8
 admin.tex | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
Signature · 1 line
2.26.2

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Parav Pandit

author

2023-05-02 11:35 UTC #8 lore raw

Quoted 21 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, May 2, 2023 3:42 AM
Quoted 6 lines · Parav Pandit
Hi Michael,
Please review these two small fixes for the admin virtqueue patches. They are on top of your work of v12 at [1].
Quoted 8 lines
Patch summary:
patch-1 removes PCI transport dependency on deprecated migration bit patch-2 fixes admin command command data type to be u8 similar to result
[1]
https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat
.com/T/#t

Quoted 6 lines

I can take 1/2 since that's just removing text. Taking 2/2 will mean more review time before vote - besides me not really liking that change, are you sure it's worth it?
Was going to start voting today but if we are still tweaking command format then I can't ...

There were 4 editorial fixes without which PDF is not readable for v12 and commit message needs update the AQ usage. It needs rebase also.
And these two changes also.

Given we have 6+ changes, better to make them and ask for vote after that.

Vote includes spec and commit message content.
So please roll v13 addressing above to make things easier to vote.

You said posting too fast is not good. Now one week has passed, so its fine to repost v13 and ask for the vote.

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Michael S. Tsirkin

2023-05-02 11:40 UTC #9 lore raw

Quoted 31 lines · Parav Pandit

Quoted 21 lines
From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, May 2, 2023 3:42 AM
Quoted 6 lines · Parav Pandit
Hi Michael,
Please review these two small fixes for the admin virtqueue patches. They are on top of your work of v12 at [1].
Quoted 8 lines
Patch summary:
patch-1 removes PCI transport dependency on deprecated migration bit patch-2 fixes admin command command data type to be u8 similar to result
[1]
https://lore.kernel.org/virtio-comment/cover.1682354275.git.mst@redhat
.com/T/#t
Quoted 6 lines
I can take 1/2 since that's just removing text. Taking 2/2 will mean more review time before vote - besides me not really liking that change, are you sure it's worth it?
Was going to start voting today but if we are still tweaking command format then I can't ...
There were 4 editorial fixes without which PDF is not readable for v12 and commit message needs update the AQ usage. It needs rebase also.
And these two changes also.

Quoted 6 lines

Given we have 6+ changes, better to make them and ask for vote after that.
Vote includes spec and commit message content.
So please roll v13 addressing above to make things easier to vote.
You said posting too fast is not good. Now one week has passed, so its fine to repost v13 and ask for the vote.

Yes absolutely. I am still debating with myself whether the change in result from u8 to le64 means I should wait another week before vote ...

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]