Re: [PATCH v2 4/4] Add support for MSI-X vectors configuration for PCI VFs

[Jason Wang <jasowang@redhat.com>]

On Thu, Jan 27, 2022 at 1:22 PM Parav Pandit <parav@nvidia.com> wrote:
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Thursday, January 27, 2022 9:06 AM
>
> [...]
>
> >
> > And I think it's better to explicit split the commands then we can have
> > dedicated commands for MSI instead of trying to mix all SRIOV related
> > attributes into the same command.
>
> It is not mixed. Its clubbed together so that driver-device communication can be minimal and still flexible.
>
> For example in [1] it has, command like,
> ip link add vx0 type vxlan id 100 local 1.1.1.1 remote 2.2.2.2 dev eth0 dstport 4789
>
> it would be inefficient do it as,
> $ ip link add vx0 type vxlan id 100
> $ ip link vx0 local 1.1.1.1
> $ ip link vx0 remote 2.2.2.2
> $ ip link vx0 dev eth0
> $ ip link vx0 dstport 4789
>
> [1] https://developers.redhat.com/blog/2018/10/22/introduction-to-linux-interfaces-for-virtual-networking#macvlan

vxlan part actually.

>
> Similarly, we prefer the ability and flexibility to set more fields in one command.

I understood this motivation. But what I want to say is:

struct virtio_cmd {
    u64 attr_mask;
    field_X;
    field_Y;
    field_Z;
}

What's the default value if one of the X,Y, Z is not specified? How to
get those default values?

And suppose we add a new field

struct virtio_cmd {
    u64 attr_mask;
    field_X;
    field_Y;
    field_Z;
    field_M;
}

How can the driver know it can use field_M? And if field_M is
supported in src but not dst, how can we keep the migration
compatibility?

Would it be simple to just mandate all the fields in this case?

Thanks


>
> > I think it's better to have a dedicated command to set the msix_vectors, then
> > there's no need for the attrs_mask.
> >
> Main design principle in having attr_mask is to avoid issuing multiple commands for related/adjacent attributes.
> Lesser the number of commands serviced, more efficient the overall system is.
> Hence the attribute mask gives the flexibility to set one or more.