Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542339 > unrolled thread

Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial  numbers Greg KH <gregkh@linuxfoundation.org> - 2016-12-15 00:30 +0100
    Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial  numbers Stephen Hemminger <stephen@networkplumber.org> - 2016-12-15 01:00 +0100
      RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial  numbers KY Srinivasan <kys@microsoft.com> - 2016-12-16 02:20 +0100
        RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial  numbers Haiyang Zhang <haiyangz@microsoft.com> - 2016-12-16 17:00 +0100
          RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial  numbers KY Srinivasan <kys@microsoft.com> - 2016-12-16 20:00 +0100
      Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial  numbers Greg KH <gregkh@linuxfoundation.org> - 2016-12-16 17:50 +0100

#1542339 — Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial numbers

On Wed, Dec 14, 2016 at 11:18:59PM +0000, Haiyang Zhang wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Saturday, December 10, 2016 7:21 AM
> > To: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>; olaf@aepfle.de;
> > jasowang@redhat.com; linux-kernel@vger.kernel.org;
> > bjorn.helgaas@gmail.com; apw@canonical.com; devel@linuxdriverproject.org;
> > leann.ogasawara@canonical.com
> > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> > serial numbers
> > 
> > On Fri, Dec 09, 2016 at 04:21:48PM -0800, Stephen Hemminger wrote:
> > > On Fri, 9 Dec 2016 22:35:05 +0000
> > > Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > >
> > > > > > >
> > > > > > > Emulated NIC is already excluded in start of netvc notifier
> > handler.
> > > > > > >
> > > > > > > static int netvsc_netdev_event(struct notifier_block *this,
> > > > > > > 			       unsigned long event, void *ptr)
> > > > > > > {
> > > > > > > 	struct net_device *event_dev =
> > netdev_notifier_info_to_dev(ptr);
> > > > > > >
> > > > > > > 	/* Skip our own events */
> > > > > > > 	if (event_dev->netdev_ops == &device_ops)
> > > > > > > 		return NOTIFY_DONE;
> > > > > > >
> > > > > >
> > > > > > Emulated device is not based on netvsc. It's the native Linux
> > > > > (dec100M?)
> > > > > > Driver. So this line doesn't exclude it. And how about other NIC
> > type
> > > > > > may be added in the future?
> > > > >
> > > > > Sorry, forgot about that haven't used emulated device in years.
> > > > > The emulated device should appear to be on a PCI bus, but the
> > serial
> > > > > would not match??
> > > >
> > > > It's not a vmbus device, not a hv_pci device either. Hv_PCI is a
> > subset
> > > > of vmbus devices. So emulated NIC won't have hv_pci serial number.
> > > >
> > > > In my patch, the following code ensure, we only try to get serial
> > number
> > > > after confirming it's vmbus and hv_pci device:
> > > >
> > > > +               if (!dev_is_vmbus(dev))
> > > > +                       continue;
> > > > +
> > > > +               hdev = device_to_hv_device(dev);
> > > > +               if (hdev->device_id != HV_PCIE)
> > > > +                       continue;
> > >
> > > Ok, the walk back up the device tree is logically ok, but I don't
> > > know enough about PCI device tree to be assured that it is safe.
> > > Also, you could short circuit away most of the unwanted devices
> > > by making sure the vf_netdev->dev.parent is a PCI device.
> > 
> > Ugh, this seems really really messy.  Can't we just have the
> > netdev_event interface pass back a pointer to something that we "know"
> > what it is?  This walking the device tree is a mess, and not good.
> > 
> > I'd even argue that dev_is_pci() needs to be removed from the tree too,
> > as it shouldn't be needed either.  We did a lot of work on the driver
> > model to prevent the need for having to declare the "type" of 'struct
> > device' at all, and by doing this type of thing it goes against the
> > basic design of the model.
> > 
> > Yes, it makes things a bit "tougher" in places, but you don't do crazy
> > things like walk device trees to try to find random devices and then
> > think it's safe to actually use them :(
> > 
> 
> We register a notifier_block with:
> 	register_netdevice_notifier(struct notifier_block *nb)
> 
> The "struct notifier_block" basically contains a callback function:
> struct notifier_block {
>         notifier_fn_t notifier_call;
>         struct notifier_block __rcu *next;
>         int priority;
> };
> 
> It doesn't specify which device we want, so all net devices can trigger
> this event. Seems we can't have this notifier return VF device only.

Ok, I dug in the kernel and it looks like people check the netdev_ops
structure to see if it matches up with their function pointers to "know"
if this is their device or not.  Why not do that here?  Or compare the
"string" of the driver name?  Or any other such trick that the drivers
that call register_netdevice_notifier do?

All of which are more sane than walking the device tree...

And why am I having to do network driver development, ick ick ick :)

Come on, 'git grep' is your friend.  Even better yet, use a good tool
like 'vgrep' which makes git grep work really really well.

thanks,

greg k-h

[toc] | [next] | [standalone]

#1542363

On Wed, 14 Dec 2016 15:27:58 -0800
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Dec 14, 2016 at 11:18:59PM +0000, Haiyang Zhang wrote:
> > 
> >   
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > Sent: Saturday, December 10, 2016 7:21 AM
> > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > Cc: Haiyang Zhang <haiyangz@microsoft.com>; olaf@aepfle.de;
> > > jasowang@redhat.com; linux-kernel@vger.kernel.org;
> > > bjorn.helgaas@gmail.com; apw@canonical.com; devel@linuxdriverproject.org;
> > > leann.ogasawara@canonical.com
> > > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> > > serial numbers
> > > 
> > > On Fri, Dec 09, 2016 at 04:21:48PM -0800, Stephen Hemminger wrote:  
> > > > On Fri, 9 Dec 2016 22:35:05 +0000
> > > > Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > > >  
> > > > > > > >
> > > > > > > > Emulated NIC is already excluded in start of netvc notifier  
> > > handler.  
> > > > > > > >
> > > > > > > > static int netvsc_netdev_event(struct notifier_block *this,
> > > > > > > > 			       unsigned long event, void *ptr)
> > > > > > > > {
> > > > > > > > 	struct net_device *event_dev =  
> > > netdev_notifier_info_to_dev(ptr);  
> > > > > > > >
> > > > > > > > 	/* Skip our own events */
> > > > > > > > 	if (event_dev->netdev_ops == &device_ops)
> > > > > > > > 		return NOTIFY_DONE;
> > > > > > > >  
> > > > > > >
> > > > > > > Emulated device is not based on netvsc. It's the native Linux  
> > > > > > (dec100M?)  
> > > > > > > Driver. So this line doesn't exclude it. And how about other NIC  
> > > type  
> > > > > > > may be added in the future?  
> > > > > >
> > > > > > Sorry, forgot about that haven't used emulated device in years.
> > > > > > The emulated device should appear to be on a PCI bus, but the  
> > > serial  
> > > > > > would not match??  
> > > > >
> > > > > It's not a vmbus device, not a hv_pci device either. Hv_PCI is a  
> > > subset  
> > > > > of vmbus devices. So emulated NIC won't have hv_pci serial number.
> > > > >
> > > > > In my patch, the following code ensure, we only try to get serial  
> > > number  
> > > > > after confirming it's vmbus and hv_pci device:
> > > > >
> > > > > +               if (!dev_is_vmbus(dev))
> > > > > +                       continue;
> > > > > +
> > > > > +               hdev = device_to_hv_device(dev);
> > > > > +               if (hdev->device_id != HV_PCIE)
> > > > > +                       continue;  
> > > >
> > > > Ok, the walk back up the device tree is logically ok, but I don't
> > > > know enough about PCI device tree to be assured that it is safe.
> > > > Also, you could short circuit away most of the unwanted devices
> > > > by making sure the vf_netdev->dev.parent is a PCI device.  
> > > 
> > > Ugh, this seems really really messy.  Can't we just have the
> > > netdev_event interface pass back a pointer to something that we "know"
> > > what it is?  This walking the device tree is a mess, and not good.
> > > 
> > > I'd even argue that dev_is_pci() needs to be removed from the tree too,
> > > as it shouldn't be needed either.  We did a lot of work on the driver
> > > model to prevent the need for having to declare the "type" of 'struct
> > > device' at all, and by doing this type of thing it goes against the
> > > basic design of the model.
> > > 
> > > Yes, it makes things a bit "tougher" in places, but you don't do crazy
> > > things like walk device trees to try to find random devices and then
> > > think it's safe to actually use them :(
> > >   
> > 
> > We register a notifier_block with:
> > 	register_netdevice_notifier(struct notifier_block *nb)
> > 
> > The "struct notifier_block" basically contains a callback function:
> > struct notifier_block {
> >         notifier_fn_t notifier_call;
> >         struct notifier_block __rcu *next;
> >         int priority;
> > };
> > 
> > It doesn't specify which device we want, so all net devices can trigger
> > this event. Seems we can't have this notifier return VF device only.  
> 
> Ok, I dug in the kernel and it looks like people check the netdev_ops
> structure to see if it matches up with their function pointers to "know"
> if this is their device or not.  Why not do that here?  Or compare the
> "string" of the driver name?  Or any other such trick that the drivers
> that call register_netdevice_notifier do?
> 
> All of which are more sane than walking the device tree...
> 
> And why am I having to do network driver development, ick ick ick :)
> 
> Come on, 'git grep' is your friend.  Even better yet, use a good tool
> like 'vgrep' which makes git grep work really really well.

Normally, that would work but in this case we have one driver (netvsc)
which is managing another driver which is unaware of Hyper-V or netvsc
drivers existence.  The callback is happening in netvsc driver and it
needs to say "hey I know that SR-IOV device, it is associated with my
network device". This problem is how to know that N is associated with
V? The V device has to be a network device, that is easy. But then it
also has to be a PCI device, not to bad. But then the netvsc code
is matching based on hyper-V only PCI bus metadata (the serial #).

The Microsoft developers made the rational decision not to go modifying
all the possible SR-IOV network devices from Intel and Mellanox to add
the functionality there. That would have been much worse.

Maybe, rather than trying to do the management in the kernel it
could have been done better in user space. Unfortunately, this would
only move the problem.  The PCI-hyperv host driver could expose serial
value through sysfs (with some pain). But the problem would be how
to make a new API to  join the two V and N device.  Doing a private
ioctl is worse than the notifier.

[toc] | [prev] | [next] | [standalone]

#1543158

> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of Stephen Hemminger
> Sent: Wednesday, December 14, 2016 3:52 PM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: olaf@aepfle.de; jasowang@redhat.com; linux-kernel@vger.kernel.org;
> bjorn.helgaas@gmail.com; apw@canonical.com;
> devel@linuxdriverproject.org; leann.ogasawara@canonical.com; Haiyang
> Zhang <haiyangz@microsoft.com>
> Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial
> numbers
> 
> On Wed, 14 Dec 2016 15:27:58 -0800
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Dec 14, 2016 at 11:18:59PM +0000, Haiyang Zhang wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Saturday, December 10, 2016 7:21 AM
> > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > Cc: Haiyang Zhang <haiyangz@microsoft.com>; olaf@aepfle.de;
> > > > jasowang@redhat.com; linux-kernel@vger.kernel.org;
> > > > bjorn.helgaas@gmail.com; apw@canonical.com;
> devel@linuxdriverproject.org;
> > > > leann.ogasawara@canonical.com
> > > > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> > > > serial numbers
> > > >
> > > > On Fri, Dec 09, 2016 at 04:21:48PM -0800, Stephen Hemminger wrote:
> > > > > On Fri, 9 Dec 2016 22:35:05 +0000
> > > > > Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > > > >
> > > > > > > > >
> > > > > > > > > Emulated NIC is already excluded in start of netvc notifier
> > > > handler.
> > > > > > > > >
> > > > > > > > > static int netvsc_netdev_event(struct notifier_block *this,
> > > > > > > > > 			       unsigned long event, void *ptr)
> > > > > > > > > {
> > > > > > > > > 	struct net_device *event_dev =
> > > > netdev_notifier_info_to_dev(ptr);
> > > > > > > > >
> > > > > > > > > 	/* Skip our own events */
> > > > > > > > > 	if (event_dev->netdev_ops == &device_ops)
> > > > > > > > > 		return NOTIFY_DONE;
> > > > > > > > >
> > > > > > > >
> > > > > > > > Emulated device is not based on netvsc. It's the native Linux
> > > > > > > (dec100M?)
> > > > > > > > Driver. So this line doesn't exclude it. And how about other NIC
> > > > type
> > > > > > > > may be added in the future?
> > > > > > >
> > > > > > > Sorry, forgot about that haven't used emulated device in years.
> > > > > > > The emulated device should appear to be on a PCI bus, but the
> > > > serial
> > > > > > > would not match??
> > > > > >
> > > > > > It's not a vmbus device, not a hv_pci device either. Hv_PCI is a
> > > > subset
> > > > > > of vmbus devices. So emulated NIC won't have hv_pci serial
> number.
> > > > > >
> > > > > > In my patch, the following code ensure, we only try to get serial
> > > > number
> > > > > > after confirming it's vmbus and hv_pci device:
> > > > > >
> > > > > > +               if (!dev_is_vmbus(dev))
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               hdev = device_to_hv_device(dev);
> > > > > > +               if (hdev->device_id != HV_PCIE)
> > > > > > +                       continue;
> > > > >
> > > > > Ok, the walk back up the device tree is logically ok, but I don't
> > > > > know enough about PCI device tree to be assured that it is safe.
> > > > > Also, you could short circuit away most of the unwanted devices
> > > > > by making sure the vf_netdev->dev.parent is a PCI device.
> > > >
> > > > Ugh, this seems really really messy.  Can't we just have the
> > > > netdev_event interface pass back a pointer to something that we
> "know"
> > > > what it is?  This walking the device tree is a mess, and not good.
> > > >
> > > > I'd even argue that dev_is_pci() needs to be removed from the tree
> too,
> > > > as it shouldn't be needed either.  We did a lot of work on the driver
> > > > model to prevent the need for having to declare the "type" of 'struct
> > > > device' at all, and by doing this type of thing it goes against the
> > > > basic design of the model.
> > > >
> > > > Yes, it makes things a bit "tougher" in places, but you don't do crazy
> > > > things like walk device trees to try to find random devices and then
> > > > think it's safe to actually use them :(
> > > >
> > >
> > > We register a notifier_block with:
> > > 	register_netdevice_notifier(struct notifier_block *nb)
> > >
> > > The "struct notifier_block" basically contains a callback function:
> > > struct notifier_block {
> > >         notifier_fn_t notifier_call;
> > >         struct notifier_block __rcu *next;
> > >         int priority;
> > > };
> > >
> > > It doesn't specify which device we want, so all net devices can trigger
> > > this event. Seems we can't have this notifier return VF device only.
> >
> > Ok, I dug in the kernel and it looks like people check the netdev_ops
> > structure to see if it matches up with their function pointers to "know"
> > if this is their device or not.  Why not do that here?  Or compare the
> > "string" of the driver name?  Or any other such trick that the drivers
> > that call register_netdevice_notifier do?
> >
> > All of which are more sane than walking the device tree...
> >
> > And why am I having to do network driver development, ick ick ick :)
> >
> > Come on, 'git grep' is your friend.  Even better yet, use a good tool
> > like 'vgrep' which makes git grep work really really well.
> 
> Normally, that would work but in this case we have one driver (netvsc)
> which is managing another driver which is unaware of Hyper-V or netvsc
> drivers existence.  The callback is happening in netvsc driver and it
> needs to say "hey I know that SR-IOV device, it is associated with my
> network device". This problem is how to know that N is associated with
> V? The V device has to be a network device, that is easy. But then it
> also has to be a PCI device, not to bad. But then the netvsc code
> is matching based on hyper-V only PCI bus metadata (the serial #).
> 
> The Microsoft developers made the rational decision not to go modifying
> all the possible SR-IOV network devices from Intel and Mellanox to add
> the functionality there. That would have been much worse.
> 
> Maybe, rather than trying to do the management in the kernel it
> could have been done better in user space. Unfortunately, this would
> only move the problem.  The PCI-hyperv host driver could expose serial
> value through sysfs (with some pain). But the problem would be how
> to make a new API to  join the two V and N device.  Doing a private
> ioctl is worse than the notifier.

All this has been discussed earlier in the thread. I think I have a solution
to the problem:
The only PCI (non-VF) NIC that may be present in the VM is the emulated NIC and
we know exactly the device ID and vendor ID of this NIC. Furthermore,
as a platform we are not going to be emulating additional NICs. So,
if the PCI NIC is not the emulated NIC, it must be a VF and we can extract the
serial number.

Regards,

K. Y
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdriverd
> ev.linuxdriverproject.org%2Fmailman%2Flistinfo%2Fdriverdev-
> devel&data=02%7C01%7Ckys%40microsoft.com%7C77c2c8a38fe2431945e408
> d4247c2c7d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63617356
> 3122444667&sdata=u5C0v7ixzRu%2Btw51tTzHNpbsNqCDQTpigzUtwahIPvE%
> 3D&reserved=0

[toc] | [prev] | [next] | [standalone]

#1543547

> -----Original Message-----
> From: KY Srinivasan
> Sent: Thursday, December 15, 2016 8:11 PM
> To: Stephen Hemminger <stephen@networkplumber.org>; Greg KH
> <gregkh@linuxfoundation.org>
> Cc: olaf@aepfle.de; jasowang@redhat.com; linux-kernel@vger.kernel.org;
> bjorn.helgaas@gmail.com; apw@canonical.com; devel@linuxdriverproject.org;
> leann.ogasawara@canonical.com; Haiyang Zhang <haiyangz@microsoft.com>
> Subject: RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> serial numbers
> 
> 
> 
> > -----Original Message-----
> > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> > Behalf Of Stephen Hemminger
> > Sent: Wednesday, December 14, 2016 3:52 PM
> > To: Greg KH <gregkh@linuxfoundation.org>
> > Cc: olaf@aepfle.de; jasowang@redhat.com; linux-kernel@vger.kernel.org;
> > bjorn.helgaas@gmail.com; apw@canonical.com;
> > devel@linuxdriverproject.org; leann.ogasawara@canonical.com; Haiyang
> > Zhang <haiyangz@microsoft.com>
> > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> serial
> > numbers
> >
> > Normally, that would work but in this case we have one driver (netvsc)
> > which is managing another driver which is unaware of Hyper-V or netvsc
> > drivers existence.  The callback is happening in netvsc driver and it
> > needs to say "hey I know that SR-IOV device, it is associated with my
> > network device". This problem is how to know that N is associated with
> > V? The V device has to be a network device, that is easy. But then it
> > also has to be a PCI device, not to bad. But then the netvsc code
> > is matching based on hyper-V only PCI bus metadata (the serial #).
> >
> > The Microsoft developers made the rational decision not to go
> modifying
> > all the possible SR-IOV network devices from Intel and Mellanox to add
> > the functionality there. That would have been much worse.
> >
> > Maybe, rather than trying to do the management in the kernel it
> > could have been done better in user space. Unfortunately, this would
> > only move the problem.  The PCI-hyperv host driver could expose serial
> > value through sysfs (with some pain). But the problem would be how
> > to make a new API to  join the two V and N device.  Doing a private
> > ioctl is worse than the notifier.
> 
> All this has been discussed earlier in the thread. I think I have a
> solution
> to the problem:
> The only PCI (non-VF) NIC that may be present in the VM is the emulated
> NIC and
> we know exactly the device ID and vendor ID of this NIC. Furthermore,
> as a platform we are not going to be emulating additional NICs. So,
> if the PCI NIC is not the emulated NIC, it must be a VF and we can
> extract the
> serial number.

How about direct pass-through NIC devices. Do they have vPCI serial number?
And, the numbers should be different from VF NIC?

Thanks,
- Haiyang

[toc] | [prev] | [next] | [standalone]

#1543694

> -----Original Message-----
> From: Haiyang Zhang
> Sent: Friday, December 16, 2016 7:21 AM
> To: KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Greg KH <gregkh@linuxfoundation.org>
> Cc: olaf@aepfle.de; jasowang@redhat.com; linux-kernel@vger.kernel.org;
> bjorn.helgaas@gmail.com; apw@canonical.com;
> devel@linuxdriverproject.org; leann.ogasawara@canonical.com
> Subject: RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on serial
> numbers
> 
> 
> 
> > -----Original Message-----
> > From: KY Srinivasan
> > Sent: Thursday, December 15, 2016 8:11 PM
> > To: Stephen Hemminger <stephen@networkplumber.org>; Greg KH
> > <gregkh@linuxfoundation.org>
> > Cc: olaf@aepfle.de; jasowang@redhat.com; linux-kernel@vger.kernel.org;
> > bjorn.helgaas@gmail.com; apw@canonical.com;
> devel@linuxdriverproject.org;
> > leann.ogasawara@canonical.com; Haiyang Zhang
> <haiyangz@microsoft.com>
> > Subject: RE: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> > serial numbers
> >
> >
> >
> > > -----Original Message-----
> > > From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org]
> On
> > > Behalf Of Stephen Hemminger
> > > Sent: Wednesday, December 14, 2016 3:52 PM
> > > To: Greg KH <gregkh@linuxfoundation.org>
> > > Cc: olaf@aepfle.de; jasowang@redhat.com; linux-
> kernel@vger.kernel.org;
> > > bjorn.helgaas@gmail.com; apw@canonical.com;
> > > devel@linuxdriverproject.org; leann.ogasawara@canonical.com; Haiyang
> > > Zhang <haiyangz@microsoft.com>
> > > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> > serial
> > > numbers
> > >
> > > Normally, that would work but in this case we have one driver (netvsc)
> > > which is managing another driver which is unaware of Hyper-V or netvsc
> > > drivers existence.  The callback is happening in netvsc driver and it
> > > needs to say "hey I know that SR-IOV device, it is associated with my
> > > network device". This problem is how to know that N is associated with
> > > V? The V device has to be a network device, that is easy. But then it
> > > also has to be a PCI device, not to bad. But then the netvsc code
> > > is matching based on hyper-V only PCI bus metadata (the serial #).
> > >
> > > The Microsoft developers made the rational decision not to go
> > modifying
> > > all the possible SR-IOV network devices from Intel and Mellanox to add
> > > the functionality there. That would have been much worse.
> > >
> > > Maybe, rather than trying to do the management in the kernel it
> > > could have been done better in user space. Unfortunately, this would
> > > only move the problem.  The PCI-hyperv host driver could expose serial
> > > value through sysfs (with some pain). But the problem would be how
> > > to make a new API to  join the two V and N device.  Doing a private
> > > ioctl is worse than the notifier.
> >
> > All this has been discussed earlier in the thread. I think I have a
> > solution
> > to the problem:
> > The only PCI (non-VF) NIC that may be present in the VM is the emulated
> > NIC and
> > we know exactly the device ID and vendor ID of this NIC. Furthermore,
> > as a platform we are not going to be emulating additional NICs. So,
> > if the PCI NIC is not the emulated NIC, it must be a VF and we can
> > extract the
> > serial number.
> 
> How about direct pass-through NIC devices. Do they have vPCI serial
> number?
> And, the numbers should be different from VF NIC?

This may not have a valid serial number; but is still a descendent of vmbus.

K. Y  
> 
> Thanks,
> - Haiyang

[toc] | [prev] | [next] | [standalone]

#1543585

On Wed, Dec 14, 2016 at 03:51:34PM -0800, Stephen Hemminger wrote:
> On Wed, 14 Dec 2016 15:27:58 -0800
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Wed, Dec 14, 2016 at 11:18:59PM +0000, Haiyang Zhang wrote:
> > > 
> > >   
> > > > -----Original Message-----
> > > > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > > > Sent: Saturday, December 10, 2016 7:21 AM
> > > > To: Stephen Hemminger <stephen@networkplumber.org>
> > > > Cc: Haiyang Zhang <haiyangz@microsoft.com>; olaf@aepfle.de;
> > > > jasowang@redhat.com; linux-kernel@vger.kernel.org;
> > > > bjorn.helgaas@gmail.com; apw@canonical.com; devel@linuxdriverproject.org;
> > > > leann.ogasawara@canonical.com
> > > > Subject: Re: [PATCH 3/3] hv_netvsc: Implement VF matching based on
> > > > serial numbers
> > > > 
> > > > On Fri, Dec 09, 2016 at 04:21:48PM -0800, Stephen Hemminger wrote:  
> > > > > On Fri, 9 Dec 2016 22:35:05 +0000
> > > > > Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > > > >  
> > > > > > > > >
> > > > > > > > > Emulated NIC is already excluded in start of netvc notifier  
> > > > handler.  
> > > > > > > > >
> > > > > > > > > static int netvsc_netdev_event(struct notifier_block *this,
> > > > > > > > > 			       unsigned long event, void *ptr)
> > > > > > > > > {
> > > > > > > > > 	struct net_device *event_dev =  
> > > > netdev_notifier_info_to_dev(ptr);  
> > > > > > > > >
> > > > > > > > > 	/* Skip our own events */
> > > > > > > > > 	if (event_dev->netdev_ops == &device_ops)
> > > > > > > > > 		return NOTIFY_DONE;
> > > > > > > > >  
> > > > > > > >
> > > > > > > > Emulated device is not based on netvsc. It's the native Linux  
> > > > > > > (dec100M?)  
> > > > > > > > Driver. So this line doesn't exclude it. And how about other NIC  
> > > > type  
> > > > > > > > may be added in the future?  
> > > > > > >
> > > > > > > Sorry, forgot about that haven't used emulated device in years.
> > > > > > > The emulated device should appear to be on a PCI bus, but the  
> > > > serial  
> > > > > > > would not match??  
> > > > > >
> > > > > > It's not a vmbus device, not a hv_pci device either. Hv_PCI is a  
> > > > subset  
> > > > > > of vmbus devices. So emulated NIC won't have hv_pci serial number.
> > > > > >
> > > > > > In my patch, the following code ensure, we only try to get serial  
> > > > number  
> > > > > > after confirming it's vmbus and hv_pci device:
> > > > > >
> > > > > > +               if (!dev_is_vmbus(dev))
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               hdev = device_to_hv_device(dev);
> > > > > > +               if (hdev->device_id != HV_PCIE)
> > > > > > +                       continue;  
> > > > >
> > > > > Ok, the walk back up the device tree is logically ok, but I don't
> > > > > know enough about PCI device tree to be assured that it is safe.
> > > > > Also, you could short circuit away most of the unwanted devices
> > > > > by making sure the vf_netdev->dev.parent is a PCI device.  
> > > > 
> > > > Ugh, this seems really really messy.  Can't we just have the
> > > > netdev_event interface pass back a pointer to something that we "know"
> > > > what it is?  This walking the device tree is a mess, and not good.
> > > > 
> > > > I'd even argue that dev_is_pci() needs to be removed from the tree too,
> > > > as it shouldn't be needed either.  We did a lot of work on the driver
> > > > model to prevent the need for having to declare the "type" of 'struct
> > > > device' at all, and by doing this type of thing it goes against the
> > > > basic design of the model.
> > > > 
> > > > Yes, it makes things a bit "tougher" in places, but you don't do crazy
> > > > things like walk device trees to try to find random devices and then
> > > > think it's safe to actually use them :(
> > > >   
> > > 
> > > We register a notifier_block with:
> > > 	register_netdevice_notifier(struct notifier_block *nb)
> > > 
> > > The "struct notifier_block" basically contains a callback function:
> > > struct notifier_block {
> > >         notifier_fn_t notifier_call;
> > >         struct notifier_block __rcu *next;
> > >         int priority;
> > > };
> > > 
> > > It doesn't specify which device we want, so all net devices can trigger
> > > this event. Seems we can't have this notifier return VF device only.  
> > 
> > Ok, I dug in the kernel and it looks like people check the netdev_ops
> > structure to see if it matches up with their function pointers to "know"
> > if this is their device or not.  Why not do that here?  Or compare the
> > "string" of the driver name?  Or any other such trick that the drivers
> > that call register_netdevice_notifier do?
> > 
> > All of which are more sane than walking the device tree...
> > 
> > And why am I having to do network driver development, ick ick ick :)
> > 
> > Come on, 'git grep' is your friend.  Even better yet, use a good tool
> > like 'vgrep' which makes git grep work really really well.
> 
> Normally, that would work but in this case we have one driver (netvsc)
> which is managing another driver which is unaware of Hyper-V or netvsc
> drivers existence.

That's the root problem here :)

> The callback is happening in netvsc driver and it
> needs to say "hey I know that SR-IOV device, it is associated with my
> network device". This problem is how to know that N is associated with
> V? The V device has to be a network device, that is easy. But then it
> also has to be a PCI device, not to bad.

I'd argue that it is just as bad, as it shouldn't be poking around with
a random 'struct device' like that, but I'll let it slide...

> But then the netvsc code
> is matching based on hyper-V only PCI bus metadata (the serial #).

Which is a mess.

Again, walking the device tree like this is racy, broken, and shouldn't
be done anywhere.  You are crossing bus boundries and lots of bad things
could happen if a device was removed at the same time.  Just don't do
that.

> The Microsoft developers made the rational decision not to go modifying
> all the possible SR-IOV network devices from Intel and Mellanox to add
> the functionality there. That would have been much worse.

Why is that worse?  How many lines of code would that be?

> Maybe, rather than trying to do the management in the kernel it
> could have been done better in user space. Unfortunately, this would
> only move the problem.  The PCI-hyperv host driver could expose serial
> value through sysfs (with some pain). But the problem would be how
> to make a new API to  join the two V and N device.  Doing a private
> ioctl is worse than the notifier.

I still don't really understand the relationship between V and N, but it
feels like it is very tenous and sketchy and you should work to make it
more explicit.  We have the source to these drivers, do it correctly,
or, do something in the network bus layer to be able to properly
represent this heirachy so that it is "obvious" what is going on.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web