[virtio-dev] [PATCH 0/2] admin virtqueue fixes

[virtio-dev] [PATCH 0/2] admin virtqueue fixes

[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/cover.1682354275.git.mst@redhat.com/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(-)

-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[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.

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/cover.1682354275.git.mst@redhat.com/T/#t

Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 admin.tex | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/admin.tex b/admin.tex
index 2efd4d7..037e2e6 100644
--- a/admin.tex
+++ b/admin.tex
@@ -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}
 
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[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/cover.1682354275.git.mst@redhat.com/T/#t

Signed-off-by: Parav Pandit <parav@nvidia.com>
---
 admin.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/admin.tex b/admin.tex
index 037e2e6..648253c 100644
--- a/admin.tex
+++ b/admin.tex
@@ -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;
-- 
2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Tue, May 02, 2023 at 01:44:30AM +0300, Parav Pandit wrote:
> 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/cover.1682354275.git.mst@redhat.com/T/#t
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>

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.

> ---
>  admin.tex | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/admin.tex b/admin.tex
> index 037e2e6..648253c 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -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;
> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Tue, May 02, 2023 at 01:44:29AM +0300, Parav Pandit wrote:
> '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/cover.1682354275.git.mst@redhat.com/T/#t
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>

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

> ---
>  admin.tex | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/admin.tex b/admin.tex
> index 2efd4d7..037e2e6 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -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}
>  
> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH 0/2] admin virtqueue fixes

[Michael S. Tsirkin]

On Tue, May 02, 2023 at 01:44:28AM +0300, Parav Pandit wrote:
> 
> 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/cover.1682354275.git.mst@redhat.com/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 ...


> 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(-)
> 
> -- 
> 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> 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?

> 
> > ---
> >  admin.tex | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 037e2e6..648253c 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -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;
> > --
> > 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] RE: [PATCH 0/2] admin virtqueue fixes

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, May 2, 2023 3:42 AM
> 
> On Tue, May 02, 2023 at 01:44:28AM +0300, Parav Pandit wrote:
> >
> > 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/cover.1682354275.git.mst@redhat
> > .com/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 ...
>
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.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

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

[Michael S. Tsirkin]

On Tue, May 02, 2023 at 11:29:03AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > 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.

> > 
> > > ---
> > >  admin.tex | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/admin.tex b/admin.tex
> > > index 037e2e6..648253c 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -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;
> > > --
> > > 2.26.2


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH 0/2] admin virtqueue fixes

[Michael S. Tsirkin]

On Tue, May 02, 2023 at 11:34:10AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, May 2, 2023 3:42 AM
> > 
> > On Tue, May 02, 2023 at 01:44:28AM +0300, Parav Pandit wrote:
> > >
> > > 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/cover.1682354275.git.mst@redhat
> > > .com/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 ...
> >
> 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.

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: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org