[virtio-dev] Re: [PATCH v2 1/4] Add virtio Admin Virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Wed, Feb 09, 2022 at 10:27:25AM +0800, Jason Wang wrote:
> 
> 在 2022/1/30 下午11:30, Michael S. Tsirkin 写道:
> > On Sun, Jan 30, 2022 at 05:12:46PM +0200, Max Gurtovoy wrote:
> > > On 1/30/2022 4:41 PM, Michael S. Tsirkin wrote:
> > > > On Sun, Jan 30, 2022 at 11:56:30AM +0200, Max Gurtovoy wrote:
> > > > > On 1/30/2022 11:40 AM, Michael S. Tsirkin wrote:
> > > > > > On Sun, Jan 30, 2022 at 11:13:38AM +0200, Max Gurtovoy wrote:
> > > > > > > On 1/29/2022 5:53 AM, Jason Wang wrote:
> > > > > > > > On Fri, Jan 28, 2022 at 11:52 PM Michael S. Tsirkin 
> > > > > > > >  wrote:
> > > > > > > > > On Fri, Jan 28, 2022 at 04:49:34PM +0100, Cornelia Huck wrote:
> > > > > > > > > > On Fri, Jan 28 2022, "Michael S. Tsirkin"  
> > > > > > > > > > wrote:
> > > > > > > > > > 
> > > > > > > > > > > On Fri, Jan 28, 2022 at 01:14:14PM +0100, Cornelia Huck 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Mon, Jan 24 2022, Max Gurtovoy 
> > > > > > > > > > > >  wrote:
> > > > > > > > > > > > > +\section{Admin Virtqueues}\label{sec:Basic 
> > > > > > > > > > > > > Facilities of a Virtio Device / Admin Virtqueues}
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +Admin virtqueue is used to send administrative 
> > > > > > > > > > > > > commands to manipulate
> > > > > > > > > > > > > +various features of the device and/or to manipulate 
> > > > > > > > > > > > > various features,
> > > > > > > > > > > > > +if possible, of another device within the same group 
> > > > > > > > > > > > > (e.g. PCI VFs of
> > > > > > > > > > > > > +a parent PCI PF device are grouped together. These 
> > > > > > > > > > > > > devices can be
> > > > > > > > > > > > > +optionally managed by its parent PCI PF using its 
> > > > > > > > > > > > > admin virtqueue.).
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +Use of Admin virtqueue is negotiated by the 
> > > > > > > > > > > > > VIRTIO_F_ADMIN_VQ
> > > > > > > > > > > > > +feature bit.
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +Admin virtqueue index may vary among different 
> > > > > > > > > > > > > device types.
> > > > > > > > > > > > So, my understanding is:
> > > > > > > > > > > > - any device type may or may not support the admin vq
> > > > > > > > > > > > - if the device type wants to be able to accommodate 
> > > > > > > > > > > > the admin vq, it
> > > > > > > > > > > >   also needs to specify where it shows up when the 
> > > > > > > > > > > > feature is negotiated
> > > > > > > > > > > > 
> > > > > > > > > > > > Do we expect that eventually all device types will need 
> > > > > > > > > > > > to support the
> > > > > > > > > > > > admin vq (if some use case comes along that will 
> > > > > > > > > > > > require all devices to
> > > > > > > > > > > > participate, for example?)
> > > > > > > > > > > I suspect yes. And that's one of the reasons why I'd 
> > > > > > > > > > > rather we had a
> > > > > > > > > > > device independent way to locate the admin queue. There 
> > > > > > > > > > > are less
> > > > > > > > > > > transports than device types.
> > > > > > > > > > So, do we want to bite the bullet now and simply say that 
> > > > > > > > > > every device
> > > > > > > > > > type has the admin vq as the last vq if the feature is 
> > > > > > > > > > negotiated?
> > > > > > > > > > Should be straightforward for the device types that have a 
> > > > > > > > > > fixed number
> > > > > > > > > > of vqs, and doable for those that have a variable amount 
> > > > > > > > > > (two device
> > > > > > > > > > types are covered by this series anyway.) I think we need 
> > > > > > > > > > to put it with
> > > > > > > > > > the device types, as otherwise the numbering of virtqueues 
> > > > > > > > > > could change
> > > > > > > > > > in unpredictable ways with the admin vq off/on.
> > > > > > > > > Well that only works once. The next thing we'll need we won't 
> > > > > > > > > be able to
> > > > > > > > > make the last one ;) So I am inclined to add a per-transport 
> > > > > > > > > field that
> > > > > > > > > gives the admin queue number.
> > > > > > > > Technically, there's no need to use the same namespace for admin
> > > > > > > > virtqueue if it has a dedicated notification area. If we go 
> > > > > > > > this way,
> > > > > > > > we can simply use 0 as queue index for admin virtqueue.
> > > > > > > Or we can use index 0x for admin virtqueue for compatibility.
> > > > > > I think I'd prefer a register with the #. For example we might want
> > > > > > to limit the # of VQs in order to pass extra data with the kick 
> > > > > > write.
> > > > > So you are suggesting adding a new cfg_type (#define
> > > > > VIRTIO_PCI_CAP_ADMIN_CFG 10) ?
> > > > > 
> > > > > that will look something like:
> > > > > 
> > > > > struct virtio_pci_admin_cfg {
> > > > > 
> > > > >       le32 queue_index; /* read only for the driver */
> > > > > 
> > > > >       le16 queue_size; /* read-write */
> > > > >       le16 queue_msix_vector; /* read-write */
> > > > >       le16 

[virtio-dev] Re: [PATCH] virtio-gpio: offered -> negotiated

2022-02-08 Thread Viresh Kumar
On 25-01-22, 07:24, Michael S. Tsirkin wrote:
> virtqueues are only discovered after FEATURES_OK.
> As such it makes no sense to talk about virtqueues being affected by
> features which are offered but not negotiated, and doing so will confuse
> the reader.
> 
> Signed-off-by: Michael S. Tsirkin 
> ---
> 
> Viresh, others your thoughts on this? Looks like an obvious typo
> to me, and something that I think we can fix under the editorial
> changes rule.
> 
>  virtio-gpio.tex | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virtio-gpio.tex b/virtio-gpio.tex
> index 55c553f..8e5c7f0 100644
> --- a/virtio-gpio.tex
> +++ b/virtio-gpio.tex
> @@ -15,7 +15,7 @@ \subsection{Virtqueues}\label{sec:Device Types / GPIO 
> Device / Virtqueues}
>  \end{description}
>  
>  The \field{eventq} virtqueue is available only if the 
> \field{VIRTIO_GPIO_F_IRQ}
> -feature is offered by the device.
> +feature has been negotiated.
>  
>  \subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature 
> bits}

Sorry for the delay, was out of office.

Acked-by: Viresh Kumar 

-- 
viresh

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



[virtio-dev] Re: [PATCH v2 1/4] Add virtio Admin Virtqueue

2022-02-08 Thread Jason Wang



在 2022/1/30 下午11:30, Michael S. Tsirkin 写道:

On Sun, Jan 30, 2022 at 05:12:46PM +0200, Max Gurtovoy wrote:

On 1/30/2022 4:41 PM, Michael S. Tsirkin wrote:

On Sun, Jan 30, 2022 at 11:56:30AM +0200, Max Gurtovoy wrote:

On 1/30/2022 11:40 AM, Michael S. Tsirkin wrote:

On Sun, Jan 30, 2022 at 11:13:38AM +0200, Max Gurtovoy wrote:

On 1/29/2022 5:53 AM, Jason Wang wrote:

On Fri, Jan 28, 2022 at 11:52 PM Michael S. Tsirkin  wrote:

On Fri, Jan 28, 2022 at 04:49:34PM +0100, Cornelia Huck wrote:

On Fri, Jan 28 2022, "Michael S. Tsirkin"  wrote:


On Fri, Jan 28, 2022 at 01:14:14PM +0100, Cornelia Huck wrote:

On Mon, Jan 24 2022, Max Gurtovoy  wrote:

+\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / 
Admin Virtqueues}
+
+Admin virtqueue is used to send administrative commands to manipulate
+various features of the device and/or to manipulate various features,
+if possible, of another device within the same group (e.g. PCI VFs of
+a parent PCI PF device are grouped together. These devices can be
+optionally managed by its parent PCI PF using its admin virtqueue.).
+
+Use of Admin virtqueue is negotiated by the VIRTIO_F_ADMIN_VQ
+feature bit.
+
+Admin virtqueue index may vary among different device types.

So, my understanding is:
- any device type may or may not support the admin vq
- if the device type wants to be able to accommodate the admin vq, it
  also needs to specify where it shows up when the feature is negotiated

Do we expect that eventually all device types will need to support the
admin vq (if some use case comes along that will require all devices to
participate, for example?)

I suspect yes. And that's one of the reasons why I'd rather we had a
device independent way to locate the admin queue. There are less
transports than device types.

So, do we want to bite the bullet now and simply say that every device
type has the admin vq as the last vq if the feature is negotiated?
Should be straightforward for the device types that have a fixed number
of vqs, and doable for those that have a variable amount (two device
types are covered by this series anyway.) I think we need to put it with
the device types, as otherwise the numbering of virtqueues could change
in unpredictable ways with the admin vq off/on.

Well that only works once. The next thing we'll need we won't be able to
make the last one ;) So I am inclined to add a per-transport field that
gives the admin queue number.

Technically, there's no need to use the same namespace for admin
virtqueue if it has a dedicated notification area. If we go this way,
we can simply use 0 as queue index for admin virtqueue.

Or we can use index 0x for admin virtqueue for compatibility.

I think I'd prefer a register with the #. For example we might want
to limit the # of VQs in order to pass extra data with the kick write.

So you are suggesting adding a new cfg_type (#define
VIRTIO_PCI_CAP_ADMIN_CFG 10) ?

that will look something like:

struct virtio_pci_admin_cfg {

      le32 queue_index; /* read only for the driver */

      le16 queue_size; /* read-write */
      le16 queue_msix_vector; /* read-write */
      le16 queue_enable; /* read-write */
      le16 queue_notify_off; /* read-only for driver */
      le64 queue_desc; /* read-write */
      le64 queue_driver; /* read-write */
      le64 queue_device; /* read-write */
      le16 queue_notify_data; /* read-only for driver */
      le16 queue_reset; /* read-write */

};

instead of re-using the struct virtio_pci_common_cfg ?


or do you prefer extending the struct virtio_pci_common_cfg with "le16
admin_queue_index; /* read only for the driver */ ?

The later. Other transports will need this too.


Cornelia has another idea which is that instead of
adding just the admin queue register to all transports,
we instead add a misc_config structure to all
transports. Working basically like device specific config,
but being device independent. For now it will only have
a single le16 admin_queue_index register.

For PCI we would thus add it with VIRTIO_PCI_CAP_MISC_CFG

The point here is that we are making it easier to add
more fields just like admin queue index in the future.

OK.

#define VIRTIO_PCI_CAP_MISC_CFG 10

and

struct virtio_pci_misc_cfg {
 le16 admin_queue_index; /* read-only for driver */
};

Is agreed by all for V3 ? instead of the net and blk AQ index definitions.

We need to add it to MMIO and CCW I guess too.



I wonder how much useful is this.

E.g for PCI we have an equation to calculate the queue notify address, 
if device choose to use dedicated notify for each queue it will probably 
end up with the last queue.


And I think the admin_queue_index should be stable regardless of the 
feature that has been negotiated?


Thanks




This is Cornelia's idea, we'll need her response.





Thanks


Another advantage to this approach is that
we can make sure admin queue gets a page by itself (which can be good if
we want to allow access to regular vqs 

[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 03:48:42PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Tuesday, February 8, 2022 9:08 PM
> 
> 
> > On Tue, Feb 08, 2022 at 03:35:58PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Tuesday, February 8, 2022 8:56 PM
> > >
> > > > > I might have been a bit too vague about what I had been thinking
> > > > > about. Let's do a sketch (intentionally without concrete sizes):
> > > > >
> > > > > +---+
> > > > > | command   |
> > > > > +---+
> > > > > | target type (0 - self, 1 - dev id, 2 - group id, ...  |
> > > > > +---+
> > > > > | dev id|
> > > > > +---+
> > > > > | group id  |
> > > > > +---+
> > > > > | command-specific data |
> > > > > +---+
> > > > > | response part |
> > > > > +---+
> > > > >
> > > > > 'dev id' would be valid for 'target type' == 1, 'group id' would
> > > > > be valid for 'target type' == 2. Alternatively, 'dev id' and 'group 
> > > > > id'
> > > > > could be a single 'target id' field; if there's nothing better to
> > > > > use, it can simply contain a uuid.
> > > >
> > > > I am not sure why do we have both dev id and group id.
> > > > They are never used together right?
> > > > Maybe just have an id length field if we can't agree on how much
> > > > space to reserve.
> > > This is what I propose in a previous email.
> > > A device id can be duplicate in different groups.
> > > So to build hierarchy group id will be desired.
> > >
> > > So id[] array can contain nested one or multiple fields.
> > 
> > I guess nesting is for when there's like an SF within a VF?
> That is one case, but I imagine that if SF is on top of VF, than VF will 
> directly manage it. PF should not get involved in nested management.
> 
> Nesting is for the case, where there is explicit group. A simple example is,
> A NIC has two PCI functions, both support SR-IOV and VFs.
> But only one PF is managing the VFs for msix, mac etc configuration.

Or maybe that's the case of "it hurts when I do this - then don't do that".


> In this case there are two groups g0, g1.
> grp0 belongs to PF0.
> grp1 belongs to PF1.
> 
> So a id definition looks like below.
> 
> struct pf_identifier {
>   u8 device; /* pci device of bdf */
>   u8 function;/* pci function of bdf */
> } __attribute__(packed);
> 
> struct pci_vf_grouped_id {
>   struct pf_identifier pf_id;
>   u16 vf_num;
> };
> 
> struct device_identfier {
>   u8 id_type; /* below id type enum */
>   u16 id_len;
>   union {
>   u8 raw[0];
>   struct pci_vf_grouped_id gvf_id;
>   } id_data;  
> };
> 
> enum id_type {
>   VIRTIO_PCI_VF = 0,  /* implicitly grouped VF */
>   VIRTIO_PCI_REMOTE_VF = 1, /* part of hirechical group */
>   VIRTIO_SF,
>   VIRTIO_PCI_REMOTE_SF, /* part of hirechical group */
>   VIRTIO_MMIO,
>   /* more */
> };


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 06:52:09PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Tuesday, February 8, 2022 9:10 PM
> > 
> > On Tue, Feb 08, 2022 at 03:06:16PM +, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin 
> > > > Sent: Tuesday, February 8, 2022 7:29 PM
> > > >
> > > > > Do we have a concrete example of a command that can be targeted
> > > > > for same
> > > > device and a target device, which requires differentiating their
> > > > destination? If so, lets discuss and then it make sense to add for the 
> > > > well-
> > defined use case.
> > > >
> > > > So e.g. things like controlling NIC's MAC can reasonably be part of
> > > > the same device.
> > > A mac address of NIC can be programmed via the existing control VQ for the
> > self.
> > 
> > Not if it's disabled for the guest.
> > 
> Its unrelated.
> The idea was to issue same command in same way by two devices = primary and 
> secondary.
> And in case of primary, it will refer to secondary device.
> And in second case secondary tells to self.
> 
> So if its disabled by guest, it doesn't matter how guest transports it, 
> either via CVQ or AQ.
> Its disabled.
> Why to re-invent command that exists on CVQ to AQ?

I can go into it but it's beside the point.  I was just trying to help
you come up with use-cases.  Don't like it - come up with your own ones.


> > OK, and Cornelia also said she thinks 64 is necessary.
> > 
> > > So if we really want to cover variety of cases like [1] and some more
> > > complex nested cases, we better define, Device identifier as below,
> > > struct device_identifier {
> > >   u8 id_length;
> > >   u8 id[];/* variable length field
> > > };
> > > For implicitly grouped VFs of a PF, id can be 2 bytes.
> > > For more advance cases it can be a structure consist one or more
> > combination of (a) host id or controller id (b) PF BDF, (c) sf id (d) PASID 
> > and
> > more.
> > >
> > > [1]
> > > https://www.kernel.org/doc/Documentation/networking/devlink/devlink-po
> > > rt.rst
> > 
> > I'm fine with this too.
> >
> Since we aim for future proofing and flexibility, variable length id is 
> better than constant u64. It covers the u64 case anyway.
>  
> > --
> > MST


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 03:06:16PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Tuesday, February 8, 2022 7:29 PM
> > 
> > > Do we have a concrete example of a command that can be targeted for same
> > device and a target device, which requires differentiating their 
> > destination? If
> > so, lets discuss and then it make sense to add for the well-defined use 
> > case.
> > 
> > So e.g. things like controlling NIC's MAC can reasonably be part of the same
> > device.
> A mac address of NIC can be programmed via the existing control VQ for the 
> self.

Not if it's disabled for the guest.

> Lets come up with some other example.

Go ahead.

> > > So better to first come up with a valid use case and a device that 
> > > supports it,
> > which needs a group.
> > > Otherwise a target id can be a long string of PCI device = :03:00.0 
> > > or a
> > BDF or a VF number or a VF of a different PCI PF or a SF number 32-bit or SF
> > UUID or a VF on a remote DPU system or PCI device on transparent bridge or
> > something else.
> > 
> > Well PASID is IIRC just 20 bit on express. 
> There are off line discussion and some on the mailing list too (I don't have 
> a link to few months/year old email),
> That PASID is being overloaded to identify the SF and process both. And I 
> tend to agree to it.
> 
> So I won't be surprised if a new SF function id takes different route than 
> PASID.
> More below.
> 
> > I find it unlikely that we'll need more
> > than 64 bit. Yes, it's hard to predict the future but just doing 16 bit 
> > here seems
> > frankly like a premature optimization. UUID for a transient thing such as 
> > SF just
> > seems unnecessary. 32 or 64 bit seem both acceptable.
> > 
> > > Without knowing the grouping and a nonexistence device we shouldn't
> > complicate the commands.
> > >
> > > There are enough opcodes (64K) that can define new structure for more
> > complex devices.
> > 
> > I think you are asking for a bit much frankly, it's up to you to build the 
> > interface.
> I likely didn't understand above point.
> 
> > Just like with code, if the design does not feel robust the bar is much 
> > higher
> > even if one can not prove there's a locking problem. 
> 
> > Same here, this interface
> > design does not yet feel very robust yet - so either we build it in a way 
> > that
> > seems robust based basically on our gut feeling, or actually spend time
> > predicting and addressing future use-cases to prove they can be addressed.
> 
> > I dare say we've developed some intuition about what makes an extensible
> > interface and where things are likely to go here at the TC, so I wouldn't 
> > discard
> > all feedback as unnecessary complication even if it does not always come 
> > with
> > concrete use-case examples.
> I do not doubt your intuition. 
> I am open to feedback, but we haven't really established at least single 
> explicit grouping example and ask is to add some arbitrary reserved bytes for 
> it.
> 
> >From my DPU experience, over last 3 years, we have built SF and VF device on 
> >parent PF-A, and their management interface on parent PF-B.
> On top of that it is expected to be uniquely indefinable in multi-host env, 
> that brings the notion of multiple controllers by a single management device.
> And also make it work uniformly with same interface when parent and 
> management device are same.
> You can see an extendible interface at [1].
> 
> With this base line of [1], I do not agree that defining a u32 
> device_identifier (to contain 20-bit PASID) will be sufficient in future.
> 

OK, and Cornelia also said she thinks 64 is necessary.

> So if we really want to cover variety of cases like [1] and some more complex 
> nested cases, we better define,
> Device identifier as below,
> struct device_identifier {
>   u8 id_length;
>   u8 id[];/* variable length field
> };
> For implicitly grouped VFs of a PF, id can be 2 bytes.
> For more advance cases it can be a structure consist one or more combination 
> of (a) host id or controller id (b) PF BDF, (c) sf id (d) PASID and more.
> 
> [1] 
> https://www.kernel.org/doc/Documentation/networking/devlink/devlink-port.rst

I'm fine with this too.

-- 
MST


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 03:35:58PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Tuesday, February 8, 2022 8:56 PM
> 
> > > I might have been a bit too vague about what I had been thinking
> > > about. Let's do a sketch (intentionally without concrete sizes):
> > >
> > > +---+
> > > | command   |
> > > +---+
> > > | target type (0 - self, 1 - dev id, 2 - group id, ...  |
> > > +---+
> > > | dev id|
> > > +---+
> > > | group id  |
> > > +---+
> > > | command-specific data |
> > > +---+
> > > | response part |
> > > +---+
> > >
> > > 'dev id' would be valid for 'target type' == 1, 'group id' would be
> > > valid for 'target type' == 2. Alternatively, 'dev id' and 'group id'
> > > could be a single 'target id' field; if there's nothing better to use,
> > > it can simply contain a uuid.
> > 
> > I am not sure why do we have both dev id and group id.
> > They are never used together right?
> > Maybe just have an id length field if we can't agree on how much space to
> > reserve.
> This is what I propose in a previous email.
> A device id can be duplicate in different groups.
> So to build hierarchy group id will be desired.
> 
> So id[] array can contain nested one or multiple fields.

I guess nesting is for when there's like an SF within a VF?

-- 
MST


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 03:33:36PM +, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin 
> > Sent: Tuesday, February 8, 2022 8:58 PM
> > On Tue, Feb 08, 2022 at 03:11:28PM +, Parav Pandit wrote:
> > >
> > > > From: Cornelia Huck 
> > > > Sent: Tuesday, February 8, 2022 8:29 PM
> > >
> > > > very specific (essentially PCI-specific), we need to at least answer
> > > > the question "how could this work for things that are not PCI?"
> > > Why would one want to execute PCI VF MSI-X configuration command for a
> > non PCI device?
> > 
> > E.g. there were proposals to add msi support to virtio-mmio.
> 
> What prevents mmio specific command say mmio_dev_interrupt_config()?
> Which fields do overlap with msix config of pci vf?
> How is the mmio device identified?

if mmio starts supporting msi-x then # of vectors can reasonably be
specified in exactly the same way.

-- 
MST


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Cornelia Huck
On Tue, Feb 08 2022, "Michael S. Tsirkin"  wrote:

> On Tue, Feb 08, 2022 at 03:59:13PM +0100, Cornelia Huck wrote:
>> On Tue, Feb 08 2022, "Michael S. Tsirkin"  wrote:
>> 
>> > On Tue, Feb 08, 2022 at 01:32:12PM +, Parav Pandit wrote:
>> >> 
>> >> > From: Cornelia Huck 
>> >> > Sent: Tuesday, February 8, 2022 6:50 PM
>> >> > 
>> >> > On Tue, Feb 08 2022, Parav Pandit  wrote:
>> >> > 
>> >> > >> From: Michael S. Tsirkin 
>> >> > >> Sent: Tuesday, February 8, 2022 12:13 PM
>> >> > >
>> >> > >> On Tue, Feb 08, 2022 at 06:25:41AM +, Parav Pandit wrote:
>> >> > >> >
>> >> > >> > > From: Michael S. Tsirkin 
>> >> > >> > > Sent: Monday, February 7, 2022 4:09 PM
>> >> > >> > >
>> >> > >> > > Next, trying to think about scalable iov extensions. So we will
>> >> > >> > > have groups of VFs and then SFs as the next level.
>> >> > >> > > How does one differentiate between the two?
>> >> > >> > > Maybe reserve a field for "destination type"?
>> >> > >> > >
>> >> > >> > We already discussed this in v2.
>> >> > >> > SF will have different identification than 16-bits. And no one
>> >> > >> > knows what
>> >> > >> that would be.
>> >> > >> > We just cannot reserve some arbitrary bytes for unknown.
>> >> > >> > You suggested in v2 to reserve 4 bytes for sf_id, and I explained
>> >> > >> > you that 4
>> >> > >> bytes may not be enough.
>> >> > >> >
>> >> > >> > Whether SFs are on top of VFs or SFs are on top of PFs or both is
>> >> > >> > completely
>> >> > >> different spec.
>> >> > >> > Whether PF will manage SFs of the VFs or it will be done nested
>> >> > >> > manner by
>> >> > >> VF etc is a completely different discussion than what is being 
>> >> > >> proposed here.
>> >> > >> > Whether PF will manage the SF is yet another big question. We work
>> >> > >> > with
>> >> > >> users and they dislike this.
>> >> > >> > To address it, some OS has a different management interface (not
>> >> > >> > visible to
>> >> > >> PF) for SF life cycle even though SFs are anchored on a PF.
>> >> > >> >
>> >> > >> > So SF/iov extension discussion has long way to go for community to
>> >> > >> > first
>> >> > >> understand the use cases before crafting some extension.
>> >> > >> >
>> >> > >> > So lets not complicate and mix things here for a blurry definition
>> >> > >> > of scalable
>> >> > >> iov/sf extension.
>> >> > >>
>> >> > >> Some reserved bytes won't hurt. 2 bytes for type gives us 64k types,
>> >> > >> sounds like that should be enough.
>> >> > > It doesn't stop there.
>> >> > > Mentioning some destination type, interrupt type, etc also requires 
>> >> > > reserving
>> >> > bytes for different device id type, interrupt type and more.
>> >> > > We past this stage long ago after discussing this in v1 at [1].
>> >> > > It is just better and cleaner to define a different structure to 
>> >> > > describe SF/iov
>> >> > and its configuration.
>> >> > 
>> >> > I have the feeling that we might be overcomplicating this. We have some
>> >> > groups of targets (a device, a group, that more complicated SF thingy), 
>> >> > and we
>> >> > want to distinguish between them. That's easy enough to cover via some 
>> >> > kind of
>> >> > enum-equivalent (0 == same dev, 1 == target a dev id, 2 == target a 
>> >> > group id, 3
>> >> > == multi-layer target) and some spec how 1 and 2 should look like (as 
>> >> > I'd expect
>> >> > them to be common for many different things). 
>> >> Do we have a concrete example of a command that can be targeted for same 
>> >> device and a target device, which requires differentiating their 
>> >> destination? If so, lets discuss and then it make sense to add for the 
>> >> well-defined use case.
>> >
>> > So e.g. things like controlling NIC's MAC can reasonably be part of
>> > the same device.
>> 
>> Yes, that would be an example.
>> 
>> I might have been a bit too vague about what I had been thinking
>> about. Let's do a sketch (intentionally without concrete sizes):
>> 
>> +---+
>> | command   |
>> +---+
>> | target type (0 - self, 1 - dev id, 2 - group id, ...  |
>> +---+
>> | dev id|
>> +---+
>> | group id  |
>> +---+
>> | command-specific data |
>> +---+
>> | response part |
>> +---+
>> 
>> 'dev id' would be valid for 'target type' == 1, 'group id' would be
>> valid for 'target type' == 2. Alternatively, 'dev id' and 'group id'
>> could be a single 'target id' field; if there's nothing better to use,
>> it can simply contain a uuid.
>
> 

[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 03:11:28PM +, Parav Pandit wrote:
> 
> > From: Cornelia Huck 
> > Sent: Tuesday, February 8, 2022 8:29 PM
> 
> > very specific (essentially PCI-specific), we need to at least answer the 
> > question
> > "how could this work for things that are not PCI?"
> Why would one want to execute PCI VF MSI-X configuration command for a non 
> PCI device?

E.g. there were proposals to add msi support to virtio-mmio.

-- 
MST


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 03:59:13PM +0100, Cornelia Huck wrote:
> On Tue, Feb 08 2022, "Michael S. Tsirkin"  wrote:
> 
> > On Tue, Feb 08, 2022 at 01:32:12PM +, Parav Pandit wrote:
> >> 
> >> > From: Cornelia Huck 
> >> > Sent: Tuesday, February 8, 2022 6:50 PM
> >> > 
> >> > On Tue, Feb 08 2022, Parav Pandit  wrote:
> >> > 
> >> > >> From: Michael S. Tsirkin 
> >> > >> Sent: Tuesday, February 8, 2022 12:13 PM
> >> > >
> >> > >> On Tue, Feb 08, 2022 at 06:25:41AM +, Parav Pandit wrote:
> >> > >> >
> >> > >> > > From: Michael S. Tsirkin 
> >> > >> > > Sent: Monday, February 7, 2022 4:09 PM
> >> > >> > >
> >> > >> > > Next, trying to think about scalable iov extensions. So we will
> >> > >> > > have groups of VFs and then SFs as the next level.
> >> > >> > > How does one differentiate between the two?
> >> > >> > > Maybe reserve a field for "destination type"?
> >> > >> > >
> >> > >> > We already discussed this in v2.
> >> > >> > SF will have different identification than 16-bits. And no one
> >> > >> > knows what
> >> > >> that would be.
> >> > >> > We just cannot reserve some arbitrary bytes for unknown.
> >> > >> > You suggested in v2 to reserve 4 bytes for sf_id, and I explained
> >> > >> > you that 4
> >> > >> bytes may not be enough.
> >> > >> >
> >> > >> > Whether SFs are on top of VFs or SFs are on top of PFs or both is
> >> > >> > completely
> >> > >> different spec.
> >> > >> > Whether PF will manage SFs of the VFs or it will be done nested
> >> > >> > manner by
> >> > >> VF etc is a completely different discussion than what is being 
> >> > >> proposed here.
> >> > >> > Whether PF will manage the SF is yet another big question. We work
> >> > >> > with
> >> > >> users and they dislike this.
> >> > >> > To address it, some OS has a different management interface (not
> >> > >> > visible to
> >> > >> PF) for SF life cycle even though SFs are anchored on a PF.
> >> > >> >
> >> > >> > So SF/iov extension discussion has long way to go for community to
> >> > >> > first
> >> > >> understand the use cases before crafting some extension.
> >> > >> >
> >> > >> > So lets not complicate and mix things here for a blurry definition
> >> > >> > of scalable
> >> > >> iov/sf extension.
> >> > >>
> >> > >> Some reserved bytes won't hurt. 2 bytes for type gives us 64k types,
> >> > >> sounds like that should be enough.
> >> > > It doesn't stop there.
> >> > > Mentioning some destination type, interrupt type, etc also requires 
> >> > > reserving
> >> > bytes for different device id type, interrupt type and more.
> >> > > We past this stage long ago after discussing this in v1 at [1].
> >> > > It is just better and cleaner to define a different structure to 
> >> > > describe SF/iov
> >> > and its configuration.
> >> > 
> >> > I have the feeling that we might be overcomplicating this. We have some
> >> > groups of targets (a device, a group, that more complicated SF thingy), 
> >> > and we
> >> > want to distinguish between them. That's easy enough to cover via some 
> >> > kind of
> >> > enum-equivalent (0 == same dev, 1 == target a dev id, 2 == target a 
> >> > group id, 3
> >> > == multi-layer target) and some spec how 1 and 2 should look like (as 
> >> > I'd expect
> >> > them to be common for many different things). 
> >> Do we have a concrete example of a command that can be targeted for same 
> >> device and a target device, which requires differentiating their 
> >> destination? If so, lets discuss and then it make sense to add for the 
> >> well-defined use case.
> >
> > So e.g. things like controlling NIC's MAC can reasonably be part of
> > the same device.
> 
> Yes, that would be an example.
> 
> I might have been a bit too vague about what I had been thinking
> about. Let's do a sketch (intentionally without concrete sizes):
> 
> +---+
> | command   |
> +---+
> | target type (0 - self, 1 - dev id, 2 - group id, ...  |
> +---+
> | dev id|
> +---+
> | group id  |
> +---+
> | command-specific data |
> +---+
> | response part |
> +---+
> 
> 'dev id' would be valid for 'target type' == 1, 'group id' would be
> valid for 'target type' == 2. Alternatively, 'dev id' and 'group id'
> could be a single 'target id' field; if there's nothing better to use,
> it can simply contain a uuid.

I am not sure why do we have both dev id and group id.
They are never used together right?
Maybe just have an id length field if we can't agree on
how much space 

Re: [virtio-dev] RE: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Cornelia Huck
On Tue, Feb 08 2022, Parav Pandit  wrote:

>> From: Cornelia Huck 
>> Sent: Tuesday, February 8, 2022 8:29 PM
>
>> very specific (essentially PCI-specific), we need to at least answer the 
>> question
>> "how could this work for things that are not PCI?"
> Why would one want to execute PCI VF MSI-X configuration command for a non 
> PCI device?

Well, that obviously does not make sense! But we also obviously still
want to be able to target admin commands in general to non-PCI devices.


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Cornelia Huck
On Tue, Feb 08 2022, "Michael S. Tsirkin"  wrote:

> On Tue, Feb 08, 2022 at 01:32:12PM +, Parav Pandit wrote:
>> 
>> > From: Cornelia Huck 
>> > Sent: Tuesday, February 8, 2022 6:50 PM
>> > 
>> > On Tue, Feb 08 2022, Parav Pandit  wrote:
>> > 
>> > >> From: Michael S. Tsirkin 
>> > >> Sent: Tuesday, February 8, 2022 12:13 PM
>> > >
>> > >> On Tue, Feb 08, 2022 at 06:25:41AM +, Parav Pandit wrote:
>> > >> >
>> > >> > > From: Michael S. Tsirkin 
>> > >> > > Sent: Monday, February 7, 2022 4:09 PM
>> > >> > >
>> > >> > > Next, trying to think about scalable iov extensions. So we will
>> > >> > > have groups of VFs and then SFs as the next level.
>> > >> > > How does one differentiate between the two?
>> > >> > > Maybe reserve a field for "destination type"?
>> > >> > >
>> > >> > We already discussed this in v2.
>> > >> > SF will have different identification than 16-bits. And no one
>> > >> > knows what
>> > >> that would be.
>> > >> > We just cannot reserve some arbitrary bytes for unknown.
>> > >> > You suggested in v2 to reserve 4 bytes for sf_id, and I explained
>> > >> > you that 4
>> > >> bytes may not be enough.
>> > >> >
>> > >> > Whether SFs are on top of VFs or SFs are on top of PFs or both is
>> > >> > completely
>> > >> different spec.
>> > >> > Whether PF will manage SFs of the VFs or it will be done nested
>> > >> > manner by
>> > >> VF etc is a completely different discussion than what is being proposed 
>> > >> here.
>> > >> > Whether PF will manage the SF is yet another big question. We work
>> > >> > with
>> > >> users and they dislike this.
>> > >> > To address it, some OS has a different management interface (not
>> > >> > visible to
>> > >> PF) for SF life cycle even though SFs are anchored on a PF.
>> > >> >
>> > >> > So SF/iov extension discussion has long way to go for community to
>> > >> > first
>> > >> understand the use cases before crafting some extension.
>> > >> >
>> > >> > So lets not complicate and mix things here for a blurry definition
>> > >> > of scalable
>> > >> iov/sf extension.
>> > >>
>> > >> Some reserved bytes won't hurt. 2 bytes for type gives us 64k types,
>> > >> sounds like that should be enough.
>> > > It doesn't stop there.
>> > > Mentioning some destination type, interrupt type, etc also requires 
>> > > reserving
>> > bytes for different device id type, interrupt type and more.
>> > > We past this stage long ago after discussing this in v1 at [1].
>> > > It is just better and cleaner to define a different structure to 
>> > > describe SF/iov
>> > and its configuration.
>> > 
>> > I have the feeling that we might be overcomplicating this. We have some
>> > groups of targets (a device, a group, that more complicated SF thingy), 
>> > and we
>> > want to distinguish between them. That's easy enough to cover via some 
>> > kind of
>> > enum-equivalent (0 == same dev, 1 == target a dev id, 2 == target a group 
>> > id, 3
>> > == multi-layer target) and some spec how 1 and 2 should look like (as I'd 
>> > expect
>> > them to be common for many different things). 
>> Do we have a concrete example of a command that can be targeted for same 
>> device and a target device, which requires differentiating their 
>> destination? If so, lets discuss and then it make sense to add for the 
>> well-defined use case.
>
> So e.g. things like controlling NIC's MAC can reasonably be part of
> the same device.

Yes, that would be an example.

I might have been a bit too vague about what I had been thinking
about. Let's do a sketch (intentionally without concrete sizes):

+---+
| command   |
+---+
| target type (0 - self, 1 - dev id, 2 - group id, ...  |
+---+
| dev id|
+---+
| group id  |
+---+
| command-specific data |
+---+
| response part |
+---+

'dev id' would be valid for 'target type' == 1, 'group id' would be
valid for 'target type' == 2. Alternatively, 'dev id' and 'group id'
could be a single 'target id' field; if there's nothing better to use,
it can simply contain a uuid.

>
>> > The SF thingy can be covered
>> > by 3, and we'll probably want to make that one command-specific, as the 
>> > whole
>> > setup does not have enough things we can standardize on.
>> This comment has underlying assumption that there are nested layers. And 
>> that assumption may not be true at all.
>> At least for iov extension of intel and SF of Mellanox (already open source 
>> for 1+ year) doesn't have the nested layers 

[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 10:34:24AM +0200, Max Gurtovoy wrote:
> "Admin virtqueue is one of the management interface that used to send 
> administrative
> commands to manipulate various features of the *device* and/or to manipulate
> various features, if possible, of *another device* within the same group"
> 
> It's not only for groups.

First this sentence is unclear what does "to manipulate" refer to?
What is done to manipulate these things?
I think the vq is to send commands. The commands are to manipulate etc.
So pls cut this up and use shorter sentences please.

And I think the vq is useful mostly for groups. But yes, thinkably if
admin commands are submitted via e.g. MMIO then they can refer to the
same device where they are sent, not to a group.  And this is exactly
why Cornelia's suggesting a way to differentiate.  We should not need a
completely separate command just because it's going to the same device
and not to another one.

-- 
MST


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Michael S. Tsirkin
On Tue, Feb 08, 2022 at 01:32:12PM +, Parav Pandit wrote:
> 
> > From: Cornelia Huck 
> > Sent: Tuesday, February 8, 2022 6:50 PM
> > 
> > On Tue, Feb 08 2022, Parav Pandit  wrote:
> > 
> > >> From: Michael S. Tsirkin 
> > >> Sent: Tuesday, February 8, 2022 12:13 PM
> > >
> > >> On Tue, Feb 08, 2022 at 06:25:41AM +, Parav Pandit wrote:
> > >> >
> > >> > > From: Michael S. Tsirkin 
> > >> > > Sent: Monday, February 7, 2022 4:09 PM
> > >> > >
> > >> > > Next, trying to think about scalable iov extensions. So we will
> > >> > > have groups of VFs and then SFs as the next level.
> > >> > > How does one differentiate between the two?
> > >> > > Maybe reserve a field for "destination type"?
> > >> > >
> > >> > We already discussed this in v2.
> > >> > SF will have different identification than 16-bits. And no one
> > >> > knows what
> > >> that would be.
> > >> > We just cannot reserve some arbitrary bytes for unknown.
> > >> > You suggested in v2 to reserve 4 bytes for sf_id, and I explained
> > >> > you that 4
> > >> bytes may not be enough.
> > >> >
> > >> > Whether SFs are on top of VFs or SFs are on top of PFs or both is
> > >> > completely
> > >> different spec.
> > >> > Whether PF will manage SFs of the VFs or it will be done nested
> > >> > manner by
> > >> VF etc is a completely different discussion than what is being proposed 
> > >> here.
> > >> > Whether PF will manage the SF is yet another big question. We work
> > >> > with
> > >> users and they dislike this.
> > >> > To address it, some OS has a different management interface (not
> > >> > visible to
> > >> PF) for SF life cycle even though SFs are anchored on a PF.
> > >> >
> > >> > So SF/iov extension discussion has long way to go for community to
> > >> > first
> > >> understand the use cases before crafting some extension.
> > >> >
> > >> > So lets not complicate and mix things here for a blurry definition
> > >> > of scalable
> > >> iov/sf extension.
> > >>
> > >> Some reserved bytes won't hurt. 2 bytes for type gives us 64k types,
> > >> sounds like that should be enough.
> > > It doesn't stop there.
> > > Mentioning some destination type, interrupt type, etc also requires 
> > > reserving
> > bytes for different device id type, interrupt type and more.
> > > We past this stage long ago after discussing this in v1 at [1].
> > > It is just better and cleaner to define a different structure to describe 
> > > SF/iov
> > and its configuration.
> > 
> > I have the feeling that we might be overcomplicating this. We have some
> > groups of targets (a device, a group, that more complicated SF thingy), and 
> > we
> > want to distinguish between them. That's easy enough to cover via some kind 
> > of
> > enum-equivalent (0 == same dev, 1 == target a dev id, 2 == target a group 
> > id, 3
> > == multi-layer target) and some spec how 1 and 2 should look like (as I'd 
> > expect
> > them to be common for many different things). 
> Do we have a concrete example of a command that can be targeted for same 
> device and a target device, which requires differentiating their destination? 
> If so, lets discuss and then it make sense to add for the well-defined use 
> case.

So e.g. things like controlling NIC's MAC can reasonably be part of
the same device.

> > The SF thingy can be covered
> > by 3, and we'll probably want to make that one command-specific, as the 
> > whole
> > setup does not have enough things we can standardize on.
> This comment has underlying assumption that there are nested layers. And that 
> assumption may not be true at all.
> At least for iov extension of intel and SF of Mellanox (already open source 
> for 1+ year) doesn't have the nested layers as today as far as I understand.
> > 
> > Does that make sense? This standardizes the simpler setups, and gives enough
> > flexibility for the more complicated ones. If we have another simpler setup 
> > in
> > the future, it can become type 4, 5, etc.
> I feel that without a use case we are over complicating the commands by 
> introducing group/target id etc.
> 
> So better to first come up with a valid use case and a device that supports 
> it, which needs a group.
> Otherwise a target id can be a long string of PCI device = :03:00.0 or a 
> BDF or a VF number or a VF of a different PCI PF or a SF number 32-bit or SF 
> UUID or a VF on a remote DPU system or PCI device on transparent bridge or 
> something else.

Well PASID is IIRC just 20 bit on express. I find it unlikely that we'll
need more than 64 bit. Yes, it's hard to predict the future but just
doing 16 bit here seems frankly like a premature optimization. UUID for
a transient thing such as SF just seems unnecessary. 32 or 64 bit seem
both acceptable.

> Without knowing the grouping and a nonexistence device we shouldn't 
> complicate the commands.
> 
> There are enough opcodes (64K) that can define new structure for more complex 
> devices.

I think you are asking for a bit much frankly, it's up to you to build
the 

[virtio-dev] RE: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Cornelia Huck
On Tue, Feb 08 2022, Parav Pandit  wrote:

>> From: Michael S. Tsirkin 
>> Sent: Tuesday, February 8, 2022 12:13 PM
>
>> On Tue, Feb 08, 2022 at 06:25:41AM +, Parav Pandit wrote:
>> >
>> > > From: Michael S. Tsirkin 
>> > > Sent: Monday, February 7, 2022 4:09 PM
>> > >
>> > > Next, trying to think about scalable iov extensions. So we will have
>> > > groups of VFs and then SFs as the next level.
>> > > How does one differentiate between the two?
>> > > Maybe reserve a field for "destination type"?
>> > >
>> > We already discussed this in v2.
>> > SF will have different identification than 16-bits. And no one knows what
>> that would be.
>> > We just cannot reserve some arbitrary bytes for unknown.
>> > You suggested in v2 to reserve 4 bytes for sf_id, and I explained you that 
>> > 4
>> bytes may not be enough.
>> >
>> > Whether SFs are on top of VFs or SFs are on top of PFs or both is 
>> > completely
>> different spec.
>> > Whether PF will manage SFs of the VFs or it will be done nested manner by
>> VF etc is a completely different discussion than what is being proposed here.
>> > Whether PF will manage the SF is yet another big question. We work with
>> users and they dislike this.
>> > To address it, some OS has a different management interface (not visible to
>> PF) for SF life cycle even though SFs are anchored on a PF.
>> >
>> > So SF/iov extension discussion has long way to go for community to first
>> understand the use cases before crafting some extension.
>> >
>> > So lets not complicate and mix things here for a blurry definition of 
>> > scalable
>> iov/sf extension.
>> 
>> Some reserved bytes won't hurt. 2 bytes for type gives us 64k types, sounds 
>> like
>> that should be enough.
> It doesn't stop there.
> Mentioning some destination type, interrupt type, etc also requires reserving 
> bytes for different device id type, interrupt type and more.
> We past this stage long ago after discussing this in v1 at [1].
> It is just better and cleaner to define a different structure to describe 
> SF/iov and its configuration.

I have the feeling that we might be overcomplicating this. We have some
groups of targets (a device, a group, that more complicated SF thingy),
and we want to distinguish between them. That's easy enough to cover via
some kind of enum-equivalent (0 == same dev, 1 == target a dev id, 2
== target a group id, 3 == multi-layer target) and some spec how 1 and 2
should look like (as I'd expect them to be common for many different
things). The SF thingy can be covered by 3, and we'll probably want to
make that one command-specific, as the whole setup does not have enough
things we can standardize on.

Does that make sense? This standardizes the simpler setups, and gives
enough flexibility for the more complicated ones. If we have another
simpler setup in the future, it can become type 4, 5, etc.


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



[virtio-dev] Re: [PATCH v3 1/4] Add virtio Admin virtqueue

2022-02-08 Thread Cornelia Huck
On Tue, Feb 08 2022, Max Gurtovoy  wrote:

> On 2/8/2022 8:45 AM, Michael S. Tsirkin wrote:
>> On Tue, Feb 08, 2022 at 02:41:44AM +0200, Max Gurtovoy wrote:
>>> On 2/7/2022 6:18 PM, Michael S. Tsirkin wrote:
 On Mon, Feb 07, 2022 at 04:58:19PM +0200, Max Gurtovoy wrote:
> On 2/7/2022 12:39 PM, Michael S. Tsirkin wrote:
>> On Thu, Feb 03, 2022 at 09:57:13AM +0200, Max Gurtovoy wrote:
>>> +\begin{lstlisting}
>>> +struct virtio_admin_cmd {
>>> +/* Device-readable part */
>>> +u16 command;
>>> +u8 command_specific_data[];
>>> +
>>> +/* Device-writable part */
>>> +u8 status;
>>> +u8 command_specific_error;
>>> +u8 command_specific_result[];
>>> +};
>>> +\end{lstlisting}
>> ok this abstraction is an improvement, thanks!
>>
>> What I'd like to see is moving a bit more format to this generic 
>> structure.
>>
>>From what I could gather, some commands affect a group as a whole, and
>> some commands just a single member of the group. We could have a
>> "destination" field for that, and a special "all of the group"
>> destination for commands affecting the whole group.
>>
>>
>> Next, trying to think about scalable iov extensions. So we
>> will have groups of VFs and then SFs as the next level.
>> How does one differentiate between the two?
>> Maybe reserve a field for "destination type"?
> For now we have only a PCI group that composed of VFs and the PF.
>
> What you suggest, IMO is a definition of a generic virtio group/subsystem
> that I've mentioned in the discussion of V1.
>
> Once we have virtio group - it should have a group id and them the admin
> command can have a field calld group_id for commands that are targeted to
> the whole group.
>
> Some commands are referring to a specific device in the group so only a
> vdev_id is needed.
>
> Some commands are even targeted to the same device to query some info (we
> have examples in this series for that), so in this case there is no need 
> for
> vdev_id nor group_id.
>
> So I'm sure sure we can improve common virtio_admin_cmd structure to have
> these attributes since they are not mandatory because of the reasons I've
> mentioned.
 I'm not sure I understand 100%, but try to address in the next
 revision and we'll discuss.
>>> I meant to say that I'm *not* sure we can improve the common structure...
>>>
>>> It was a typo.
>>>
>>> And I don't understand why this info can't be in the command_specific_data
>>> because of all the reasons I mentioned above.
>> It can, but as declared admin commands are there to handle
>> groups of VFs, so let's standardize how they refer to groups.
>
> "Admin virtqueue is one of the management interface that used to send 
> administrative
> commands to manipulate various features of the *device* and/or to manipulate
> various features, if possible, of *another device* within the same group"
>
> It's not only for groups.

What I don't understand: Why can't we simply add target information to
the common definition? If a certain command doesn't need that target
information, it is simply ignored.

The benefit of reserving a field for target information and specifying
how groups and devices are supposed to be addressed is that not every
command will have to roll their own.


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