[virtio-dev] [PATCH 0/2] transport-pci: msix register desc improve

[virtio-dev] [PATCH 0/2] transport-pci: msix register desc improve

[Parav Pandit]

Problem:

Current wording queue_msix_vector and config_msix_vector says
"for MSI-X", which is bit confusing.

config_msix_vector and queue_msix_vector are the msix vector
number for configuration change and queue related interrupts.

Hence, reword it.

---
changelog:
v0->v1:
- added 'here'
- dropped 'receiving'
- dropped already merged patch for empty line

Parav Pandit (2):
  transport-pci: Improve config msix vector description
  transport-pci: Improve queue msix vector register desc

 transport-pci.tex | 5 +++--
 1 file changed, 3 insertions(+), 2 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] transport-pci: Improve config msix vector description

[Parav Pandit]

config_msix_vector is the register that holds the MSI-X vector number
for receiving configuration change related interrupts.

It is not "for MSI-X".

Hence, replace the confusing text with appropriate one.

Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Parav Pandit <parav@nvidia.com>

---
changelog:
v0->v1:
- added 'here'
- dropped 'receiving'
---
 transport-pci.tex | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/transport-pci.tex b/transport-pci.tex
index 42be072..cb9fadd 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -343,7 +343,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         Driver Feature Bits selected by \field{driver_feature_select}.
 
 \item[\field{config_msix_vector}]
-        The driver sets the Configuration Vector for MSI-X.
+        The driver writes an MSI-X vector number here for Configuration change
+        interrupts.
 
 \item[\field{num_queues}]
         The device specifies the maximum number of virtqueues supported here.
-- 
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] transport-pci: Improve config msix vector description

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 10:42:19PM +0300, Parav Pandit wrote:
> config_msix_vector is the register that holds the MSI-X vector number
> for receiving configuration change related interrupts.
> 
> It is not "for MSI-X".
> 
> Hence, replace the confusing text with appropriate one.
> 
> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v0->v1:
> - added 'here'
> - dropped 'receiving'
> ---
>  transport-pci.tex | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/transport-pci.tex b/transport-pci.tex
> index 42be072..cb9fadd 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -343,7 +343,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          Driver Feature Bits selected by \field{driver_feature_select}.
>  
>  \item[\field{config_msix_vector}]
> -        The driver sets the Configuration Vector for MSI-X.
> +        The driver writes an MSI-X vector number here for Configuration change
> +        interrupts.

Hmm I think it's "the vector", not an. And the order of words
is unusual. We also don't normally use "writes here". Would

	Used by the driver to set the MSI-X vector number for Configuration change interrupts.

be equally acceptable?


>  \item[\field{num_queues}]
>          The device specifies the maximum number of virtqueues supported here.
> -- 
> 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] transport-pci: Improve config msix vector description

[Michael S. Tsirkin]

On Wed, Apr 19, 2023 at 03:05:11AM -0400, Michael S. Tsirkin wrote:
> On Tue, Apr 11, 2023 at 10:42:19PM +0300, Parav Pandit wrote:
> > config_msix_vector is the register that holds the MSI-X vector number
> > for receiving configuration change related interrupts.
> > 
> > It is not "for MSI-X".
> > 
> > Hence, replace the confusing text with appropriate one.
> > 
> > Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > 
> > ---
> > changelog:
> > v0->v1:
> > - added 'here'
> > - dropped 'receiving'
> > ---
> >  transport-pci.tex | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/transport-pci.tex b/transport-pci.tex
> > index 42be072..cb9fadd 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -343,7 +343,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >          Driver Feature Bits selected by \field{driver_feature_select}.
> >  
> >  \item[\field{config_msix_vector}]
> > -        The driver sets the Configuration Vector for MSI-X.
> > +        The driver writes an MSI-X vector number here for Configuration change
> > +        interrupts.
> 
> Hmm I think it's "the vector", not an. And the order of words
> is unusual. We also don't normally use "writes here". Would
> 
> 	Used by the driver to set the MSI-X vector number for Configuration change interrupts.

or even Configuration change notification interrupts

this is what we call them elsewhere

> be equally acceptable?
> 
> 
> >  \item[\field{num_queues}]
> >          The device specifies the maximum number of virtqueues supported here.
> > -- 
> > 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

Re: [virtio-dev] Re: [PATCH 1/2] transport-pci: Improve config msix vector description

[Zhu, Lingshan]

On 4/19/2023 3:05 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 11, 2023 at 10:42:19PM +0300, Parav Pandit wrote:
>> config_msix_vector is the register that holds the MSI-X vector number
>> for receiving configuration change related interrupts.
>>
>> It is not "for MSI-X".
>>
>> Hence, replace the confusing text with appropriate one.
>>
>> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> Signed-off-by: Parav Pandit <parav@nvidia.com>
>>
>> ---
>> changelog:
>> v0->v1:
>> - added 'here'
>> - dropped 'receiving'
>> ---
>>   transport-pci.tex | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/transport-pci.tex b/transport-pci.tex
>> index 42be072..cb9fadd 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -343,7 +343,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           Driver Feature Bits selected by \field{driver_feature_select}.
>>   
>>   \item[\field{config_msix_vector}]
>> -        The driver sets the Configuration Vector for MSI-X.
>> +        The driver writes an MSI-X vector number here for Configuration change
>> +        interrupts.
> Hmm I think it's "the vector", not an. And the order of words
> is unusual. We also don't normally use "writes here". Would
>
> 	Used by the driver to set the MSI-X vector number for Configuration change interrupts.
>
> be equally acceptable?
Is it better also explain what is a MSI-X vector number here? Like 
saying the offset in the table or the slot number?

Thanks,
Zhu Lingshan
>
>
>>   \item[\field{num_queues}]
>>           The device specifies the maximum number of virtqueues supported here.
>> -- 
>> 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
>


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

Re: [virtio-dev] Re: [PATCH 1/2] transport-pci: Improve config msix vector description

[Michael S. Tsirkin]

On Wed, Apr 19, 2023 at 04:25:13PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 4/19/2023 3:05 PM, Michael S. Tsirkin wrote:
> > On Tue, Apr 11, 2023 at 10:42:19PM +0300, Parav Pandit wrote:
> > > config_msix_vector is the register that holds the MSI-X vector number
> > > for receiving configuration change related interrupts.
> > > 
> > > It is not "for MSI-X".
> > > 
> > > Hence, replace the confusing text with appropriate one.
> > > 
> > > Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > 
> > > ---
> > > changelog:
> > > v0->v1:
> > > - added 'here'
> > > - dropped 'receiving'
> > > ---
> > >   transport-pci.tex | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/transport-pci.tex b/transport-pci.tex
> > > index 42be072..cb9fadd 100644
> > > --- a/transport-pci.tex
> > > +++ b/transport-pci.tex
> > > @@ -343,7 +343,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> > >           Driver Feature Bits selected by \field{driver_feature_select}.
> > >   \item[\field{config_msix_vector}]
> > > -        The driver sets the Configuration Vector for MSI-X.
> > > +        The driver writes an MSI-X vector number here for Configuration change
> > > +        interrupts.
> > Hmm I think it's "the vector", not an. And the order of words
> > is unusual. We also don't normally use "writes here". Would
> > 
> > 	Used by the driver to set the MSI-X vector number for Configuration change interrupts.
> > 
> > be equally acceptable?
> Is it better also explain what is a MSI-X vector number here? Like saying
> the offset in the table or the slot number?
> 
> Thanks,
> Zhu Lingshan

It does, in fact, refer to the MSI-X entry. Vector is in fact the
contents of the entry.  The mixup between vector and entry dates back to
PCI spec which has text like
	While more accurately termed “per-entry masking”, masking an MSI-X Table entry is still
	referred to as “vector masking” so similar descriptions can be used for both MSI and MSI-
	X.


> > 
> > 
> > >   \item[\field{num_queues}]
> > >           The device specifies the maximum number of virtqueues supported here.
> > > -- 
> > > 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
> > 


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

RE: [virtio-dev] Re: [PATCH 1/2] transport-pci: Improve config msix vector description

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, April 19, 2023 7:27 AM
> 
> On Wed, Apr 19, 2023 at 04:25:13PM +0800, Zhu, Lingshan wrote:

> > > > +        The driver writes an MSI-X vector number here for Configuration
> change
> > > > +        interrupts.
> > > Hmm I think it's "the vector", not an. And the order of words is
> > > unusual. We also don't normally use "writes here". Would
> > >
> > > 	Used by the driver to set the MSI-X vector number for Configuration
> change interrupts.
> > >

To keep the text consistent with rest of the fields and to address below comment about MSI-X table entry reference,

As:
The driver writes the MSI-X table entry index in this field for Configuration change notification interrupts.

Similar to,
The driver writes this to accept feature bits offered by ...

[virtio-dev] [PATCH 2/2] transport-pci: Improve queue msix vector register desc

[Parav Pandit]

queue_msix_vector register is for receiving interrupts from the device
for the virtqueue.

"for MSI-X" is confusing term.

Also it is the register that driver "writes" to, similar to
many other registers such as queue_desc, queue_driver etc.

Hence, replace the verb from use to write.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
changelog:
v0->v1:
- added 'here'
- dropped 'receiving'
---
 transport-pci.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-pci.tex b/transport-pci.tex
index cb9fadd..00669b5 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -367,7 +367,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         A 0 means the queue is unavailable.
 
 \item[\field{queue_msix_vector}]
-        The driver uses this to specify the queue vector for MSI-X.
+        The driver writes an MSI-X vector number here for virtqueue interrupts.
 
 \item[\field{queue_enable}]
         The driver uses this to selectively prevent the device from executing requests from this virtqueue.
-- 
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] transport-pci: Improve queue msix vector register desc

[Michael S. Tsirkin]

On Tue, Apr 11, 2023 at 10:42:20PM +0300, Parav Pandit wrote:
> queue_msix_vector register is for receiving interrupts from the device
> for the virtqueue.
> 
> "for MSI-X" is confusing term.
> 
> Also it is the register that driver "writes" to, similar to
> many other registers such as queue_desc, queue_driver etc.
> 
> Hence, replace the verb from use to write.
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
> changelog:
> v0->v1:
> - added 'here'
> - dropped 'receiving'
> ---
>  transport-pci.tex | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/transport-pci.tex b/transport-pci.tex
> index cb9fadd..00669b5 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -367,7 +367,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          A 0 means the queue is unavailable.
>  
>  \item[\field{queue_msix_vector}]
> -        The driver uses this to specify the queue vector for MSI-X.
> +        The driver writes an MSI-X vector number here for virtqueue interrupts.

Here it's *the* specific queue not just some queue. And in fact
we call them virtqueue notifications, not interrupts elsewhere.
So how about:

        Used by the driver to set the MSI-X vector number for the virtqueue notifications.


>  \item[\field{queue_enable}]
>          The driver uses this to selectively prevent the device from executing requests from this virtqueue.
> -- 
> 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] transport-pci: Improve queue msix vector register desc

[Michael S. Tsirkin]

On Wed, Apr 19, 2023 at 03:08:30AM -0400, Michael S. Tsirkin wrote:
> On Tue, Apr 11, 2023 at 10:42:20PM +0300, Parav Pandit wrote:
> > queue_msix_vector register is for receiving interrupts from the device
> > for the virtqueue.
> > 
> > "for MSI-X" is confusing term.
> > 
> > Also it is the register that driver "writes" to, similar to
> > many other registers such as queue_desc, queue_driver etc.
> > 
> > Hence, replace the verb from use to write.
> > 
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > Reviewed-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > ---
> > changelog:
> > v0->v1:
> > - added 'here'
> > - dropped 'receiving'
> > ---
> >  transport-pci.tex | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/transport-pci.tex b/transport-pci.tex
> > index cb9fadd..00669b5 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -367,7 +367,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> >          A 0 means the queue is unavailable.
> >  
> >  \item[\field{queue_msix_vector}]
> > -        The driver uses this to specify the queue vector for MSI-X.
> > +        The driver writes an MSI-X vector number here for virtqueue interrupts.
> 
> Here it's *the* specific queue not just some queue. And in fact
> we call them virtqueue notifications, not interrupts elsewhere.
> So how about:
> 
>         Used by the driver to set the MSI-X vector number for the virtqueue notifications.

or even "the virtqueue notification interrupts".

> 
> >  \item[\field{queue_enable}]
> >          The driver uses this to selectively prevent the device from executing requests from this virtqueue.
> > -- 
> > 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] transport-pci: msix register desc improve

[Parav Pandit]

Hi Michael,

> From: Parav Pandit <parav@nvidia.com>
> Sent: Tuesday, April 11, 2023 3:42 PM
> 
> Problem:
> 
> Current wording queue_msix_vector and config_msix_vector says "for MSI-X",
> which is bit confusing.
> 
> config_msix_vector and queue_msix_vector are the msix vector number for
> configuration change and queue related interrupts.
> 
> Hence, reword it.
> 
> ---
> changelog:
> v0->v1:
> - added 'here'
> - dropped 'receiving'
> - dropped already merged patch for empty line

If you do not have further comments, can you please take this rewording change forward?

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