RE: [PATCH linux-next v3 6/6] vdpa_sim_net: Add support for user supported devices

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, January 15, 2021 11:09 AM
> 
> 
> On 2021/1/14 下午3:58, Parav Pandit wrote:
> >
> >> From: Jason Wang <jasowang@redhat.com>
> >> Sent: Thursday, January 14, 2021 9:48 AM
> >>
> >> On 2021/1/7 上午11:48, Parav Pandit wrote:
> >>>> From: Michael S. Tsirkin <mst@redhat.com>
> >>>> Sent: Tuesday, January 5, 2021 6:53 PM
> >>>>
> >>>> On Tue, Jan 05, 2021 at 12:30:15PM +0000, Parav Pandit wrote:
> >>>>>> From: Michael S. Tsirkin <mst@redhat.com>
> >>>>>> Sent: Tuesday, January 5, 2021 5:45 PM
> >>>>>>
> >>>>>> On Tue, Jan 05, 2021 at 12:02:33PM +0000, Parav Pandit wrote:
> >>>>>>>> From: Michael S. Tsirkin <mst@redhat.com>
> >>>>>>>> Sent: Tuesday, January 5, 2021 5:19 PM
> >>>>>>>>
> >>>>>>>> On Tue, Jan 05, 2021 at 12:32:03PM +0200, Parav Pandit wrote:
> >>>>>>>>> Enable user to create vdpasim net simulate devices.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> $ vdpa dev add mgmtdev vdpasim_net name foo2
> >>>>>>>>>
> >>>>>>>>> Show the newly created vdpa device by its name:
> >>>>>>>>> $ vdpa dev show foo2
> >>>>>>>>> foo2: type network mgmtdev vdpasim_net vendor_id 0
> max_vqs 2
> >>>>>>>>> max_vq_size 256
> >>>>>>>>>
> >>>>>>>>> $ vdpa dev show foo2 -jp
> >>>>>>>>> {
> >>>>>>>>>       "dev": {
> >>>>>>>>>           "foo2": {
> >>>>>>>>>               "type": "network",
> >>>>>>>>>               "mgmtdev": "vdpasim_net",
> >>>>>>>>>               "vendor_id": 0,
> >>>>>>>>>               "max_vqs": 2,
> >>>>>>>>>               "max_vq_size": 256
> >>>>>>>>>           }
> >>>>>>>>>       }
> >>>>>>>>> }
> >>>>>>>> I'd like an example of how do device specific (e.g. net
> >>>>>>>> specific) interfaces tie in to this.
> >>>>>>> Not sure I follow your question.
> >>>>>>> Do you mean how to set mac address or mtu of this vdpa device of
> >>>>>>> type
> >>>>>> net?
> >>>>>>> If so, dev add command will be extended shortly in subsequent
> >>>>>>> series to
> >>>>>> set this net specific attributes.
> >>>>>>> (I did mention in the next steps in cover letter).
> >>>>>>>
> >>>>>>>>> +static int __init vdpasim_net_init(void) {
> >>>>>>>>> +	int ret;
> >>>>>>>>> +
> >>>>>>>>> +	if (macaddr) {
> >>>>>>>>> +		mac_pton(macaddr, macaddr_buf);
> >>>>>>>>> +		if (!is_valid_ether_addr(macaddr_buf))
> >>>>>>>>> +			return -EADDRNOTAVAIL;
> >>>>>>>>> +	} else {
> >>>>>>>>> +		eth_random_addr(macaddr_buf);
> >>>>>>>>>    	}
> >>>>>>>> Hmm so all devices start out with the same MAC until changed?
> >>>>>>>> And how is the change effected?
> >>>>>>> Post this patchset and post we have iproute2 vdpa in the tree,
> >>>>>>> will add the
> >>>>>> mac address as the input attribute during "vdpa dev add" command.
> >>>>>>> So that each different vdpa device can have user specified
> >>>>>>> (different) mac
> >>>>>> address.
> >>>>>>
> >>>>>> For now maybe just avoid VIRTIO_NET_F_MAC then for new devices
> >>>> then?
> >>>>> That would require book keeping existing net vdpa_sim devices
> >>>>> created to
> >>>> avoid setting VIRTIO_NET_F_MAC.
> >>>>> Such book keeping code will be short lived anyway.
> >>>>> Not sure if its worth it.
> >>>>> Until now only one device was created. So not sure two vdpa
> >>>>> devices with
> >>>> same mac address will be a real issue.
> >>>>> When we add mac address attribute in add command, at that point
> >>>>> also
> >>>> remove the module parameter macaddr.
> >>>>
> >>>> Will that be mandatory? I'm not to happy with a UAPI we intend to
> >>>> break straight away ...
> >>> No. Specifying mac address shouldn't be mandatory. UAPI wont' be
> >> broken.
> >>
> >>
> >> If it's not mandatory. Does it mean the vDPA parent need to use its
> >> own logic to generate a validate mac? I'm not sure this is what
> >> management (libvirt want).
> >>
> > There are few use cases that I see with PFs, VFs and SFs supporting vdpa
> devices.
> >
> > 1. User wants to use the VF only for vdpa purpose. Here user got the VF
> which was pre-setup by the sysadmin.
> > In this case whatever MAC assigned to the VF can be used by its vdpa
> device.
> > Here, user doesn't need to pass the mac address during vdpa device
> creation time.
> > This is done as the same MAC has been setup in the ACL rules on the switch
> side.
> > Non VDPA users of a VF typically use the VF this way for Netdev and rdma
> functionality.
> > They might continue same way for vdpa application as well.
> > Here VF mac is either set using
> > (a) devlink port function set hw_addr command or using
> > (b) ip link set vf mac
> > So vdpa tool didn't pass the mac. (optional).
> > Though VIRTIO_NET_F_MAC is still valid.
> >
> > 2. User may want to create one or more vdpa device out of the mgmt.
> device.
> > Here user wants to more/full control of all features, overriding what
> sysadmin has setup as MAC of the VF/SF.
> > In this case user will specify the MAC via mgmt tool.
> > (a) This is also used by those vdpa devices which doesn't have eswitch
> offloads.
> > (b) This will work with eswitch offloads as well who does source learning.
> > (c) User chose to use the vdpa device of a VF while VF Netdev and rdma
> device are used by hypervisor for something else as well.
> > VIRTIO_NET_F_MAC remains valid in all 2.{a,b,c}.
> >
> > 3. A  vendor mgmt. device always expects it user to provide mac for its
> vdpa devices.
> > So when it is not provided, it can fail with error message string in extack or
> clear the VIRTIO_NET_F_MAC and let it work using virtio spec's 5.1.5 point 5
> to proceed.
> >
> > As common denominator of all above cases, if QEMU or user pass the MAC
> during creation, it will almost always work.
> > Advance user and QEMU with switchdev mode support who has done
> 1.a/1.b, will omit it.
> > I do not know how deep integration of QEMU exist with the switchdev
> mode support.
> >
> > With that mac, mtu as optional input fields provide the necessary flexibility
> for different stacks to take appropriate shape as they desire.
> 
> 
> Thanks for the clarification. I think we'd better document the above in the
> patch that introduces the mac setting from management API.

Yes. Will do.
Thanks.