netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
@ 2013-01-09  9:59 Stefan Assmann
  2013-01-09 17:09 ` Williams, Mitch A
  0 siblings, 1 reply; 13+ messages in thread
From: Stefan Assmann @ 2013-01-09  9:59 UTC (permalink / raw)
  To: netdev; +Cc: e1000-devel, sassmann

When the PF is up and igbvf is loaded the MAC address is not generated using
eth_hw_addr_random(). This results in addr_assign_type not to be set.
Make sure it gets set.

Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
---
 drivers/net/ethernet/intel/igbvf/netdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 53281ff..6a698c5 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -2746,6 +2746,7 @@ static int igbvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			dev_err(&pdev->dev, "Error reading MAC address\n");
 			goto err_hw_init;
 		}
+		netdev->addr_assign_type = NET_ADDR_RANDOM;
 		memcpy(netdev->dev_addr, adapter->hw.mac.addr,
 			netdev->addr_len);
 	}
-- 
1.8.0.2


------------------------------------------------------------------------------
Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
and much more. Keep your Java skills current with LearnJavaNow -
200+ hours of step-by-step video tutorials by Java experts.
SALE $49.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122612 
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-09  9:59 [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up Stefan Assmann
@ 2013-01-09 17:09 ` Williams, Mitch A
  2013-01-09 17:58   ` [E1000-devel] " Stefan Assmann
  0 siblings, 1 reply; 13+ messages in thread
From: Williams, Mitch A @ 2013-01-09 17:09 UTC (permalink / raw)
  To: Stefan Assmann, netdev@vger.kernel.org; +Cc: e1000-devel@lists.sourceforge.net

> -----Original Message-----
> From: Stefan Assmann [mailto:sassmann@kpanic.de]
> Sent: Wednesday, January 09, 2013 1:59 AM
> To: netdev@vger.kernel.org
> Cc: e1000-devel@lists.sourceforge.net; sassmann@kpanic.de
> Subject: [E1000-devel] [PATCH net-next] igbvf: fix setting
> addr_assign_type if PF is up
> 
> When the PF is up and igbvf is loaded the MAC address is not generated
> using eth_hw_addr_random(). This results in addr_assign_type not to be
> set.
> Make sure it gets set.
> 

NAK - In this case, the address may or may not be random. The user may
have (and should have!) explicitly set this address from the host to
ensure that the VF device receives the same address each time it boots.

-Mitch

> Signed-off-by: Stefan Assmann <sassmann@kpanic.de>
> ---
>  drivers/net/ethernet/intel/igbvf/netdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c
> b/drivers/net/ethernet/intel/igbvf/netdev.c
> index 53281ff..6a698c5 100644
> --- a/drivers/net/ethernet/intel/igbvf/netdev.c
> +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
> @@ -2746,6 +2746,7 @@ static int igbvf_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>  			dev_err(&pdev->dev, "Error reading MAC address\n");
>  			goto err_hw_init;
>  		}
> +		netdev->addr_assign_type = NET_ADDR_RANDOM;
>  		memcpy(netdev->dev_addr, adapter->hw.mac.addr,
>  			netdev->addr_len);
>  	}
> --
> 1.8.0.2
> 
> 
> ------------------------------------------------------------------------
> ------
> Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
> and much more. Keep your Java skills current with LearnJavaNow -
> 200+ hours of step-by-step video tutorials by Java experts.
> SALE $49.99 this month only -- learn more at:
> http://p.sf.net/sfu/learnmore_122612
> _______________________________________________
> E1000-devel mailing list
> E1000-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/e1000-devel
> To learn more about Intel&#174; Ethernet, visit
> http://communities.intel.com/community/wired

------------------------------------------------------------------------------
Master Java SE, Java EE, Eclipse, Spring, Hibernate, JavaScript, jQuery
and much more. Keep your Java skills current with LearnJavaNow -
200+ hours of step-by-step video tutorials by Java experts.
SALE $49.99 this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122612 
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-09 17:09 ` Williams, Mitch A
@ 2013-01-09 17:58   ` Stefan Assmann
  2013-01-09 18:56     ` Williams, Mitch A
  0 siblings, 1 reply; 13+ messages in thread
From: Stefan Assmann @ 2013-01-09 17:58 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: netdev@vger.kernel.org, e1000-devel@lists.sourceforge.net

On 09.01.2013 18:09, Williams, Mitch A wrote:
>> -----Original Message-----
>> From: Stefan Assmann [mailto:sassmann@kpanic.de]
>> Sent: Wednesday, January 09, 2013 1:59 AM
>> To: netdev@vger.kernel.org
>> Cc: e1000-devel@lists.sourceforge.net; sassmann@kpanic.de
>> Subject: [E1000-devel] [PATCH net-next] igbvf: fix setting
>> addr_assign_type if PF is up
>>
>> When the PF is up and igbvf is loaded the MAC address is not generated
>> using eth_hw_addr_random(). This results in addr_assign_type not to be
>> set.
>> Make sure it gets set.
>>
> 
> NAK - In this case, the address may or may not be random. The user may
> have (and should have!) explicitly set this address from the host to
> ensure that the VF device receives the same address each time it boots.

Maybe you can give me some advice on this then. Why is there different
behaviour depending on the PF being up or down? The problem I'm facing
is that if the user did not set a MAC address for the VF manually and
the PF is up during igbvf_probe it will not be labelled as random
although it is.
What about checking IGB_VF_FLAG_PF_SET_MAC and only set NET_ADDR_RANDOM
if the flag is cleared?

  Stefan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-09 17:58   ` [E1000-devel] " Stefan Assmann
@ 2013-01-09 18:56     ` Williams, Mitch A
  2013-01-09 19:53       ` Stefan Assmann
  2013-01-09 21:37       ` Greg Rose
  0 siblings, 2 replies; 13+ messages in thread
From: Williams, Mitch A @ 2013-01-09 18:56 UTC (permalink / raw)
  To: Stefan Assmann; +Cc: netdev@vger.kernel.org, e1000-devel@lists.sourceforge.net

> >> When the PF is up and igbvf is loaded the MAC address is not
> >> generated using eth_hw_addr_random(). This results in
> >> addr_assign_type not to be set.
> >> Make sure it gets set.
> >>
> >
> > NAK - In this case, the address may or may not be random. The user may
> > have (and should have!) explicitly set this address from the host to
> > ensure that the VF device receives the same address each time it
> boots.
> 
> Maybe you can give me some advice on this then. Why is there different
> behaviour depending on the PF being up or down? The problem I'm facing
> is that if the user did not set a MAC address for the VF manually and
> the PF is up during igbvf_probe it will not be labelled as random
> although it is.
> What about checking IGB_VF_FLAG_PF_SET_MAC and only set NET_ADDR_RANDOM
> if the flag is cleared?
> 

The difference in behavior is because we cannot get any MAC address at all
if the PF is down. The interface won't operate at all in this case, but if
the PF comes up sometime later, we can start working. The other alternative
is to leave the MAC address as all zeros and forcing the user to assign
an address manually. We chose to use a random address to at least give it
a chance of working once the PF woke up.

Currently, the PF has no way to communicate to the VF whether or not the
MAC address is random or assigned. The VF cannot check the
IGB_VF_FLAG_PF_SET_MAC flag because that only exists in the PF driver. To
propagate this flag down to the VF driver would require changes to the
PF/VF communication protocol.

In any case, I'm not sure that's the correct thing to do. From a policy
viewpoint, we don't want the VF to know what's happening in the PF. It
should not know how or why the MAC address was assigned, just like it
should not know whether or not the PF has placed it on a VLAN. VF devices
are not to be trusted and should not be given more information about the
state of the PF and host OS than is absolutely necessary to operate.

What's your use case here, Stefan? Why is this flag important to you?
As far as I can tell, nothing in the kernel ever looks at this flag.

-Mitch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-09 18:56     ` Williams, Mitch A
@ 2013-01-09 19:53       ` Stefan Assmann
  2013-01-09 21:37       ` Greg Rose
  1 sibling, 0 replies; 13+ messages in thread
From: Stefan Assmann @ 2013-01-09 19:53 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: netdev@vger.kernel.org, e1000-devel@lists.sourceforge.net

On 09.01.2013 19:56, Williams, Mitch A wrote:
>>>> When the PF is up and igbvf is loaded the MAC address is not
>>>> generated using eth_hw_addr_random(). This results in
>>>> addr_assign_type not to be set.
>>>> Make sure it gets set.
>>>>
>>>
>>> NAK - In this case, the address may or may not be random. The user may
>>> have (and should have!) explicitly set this address from the host to
>>> ensure that the VF device receives the same address each time it
>> boots.
>>
>> Maybe you can give me some advice on this then. Why is there different
>> behaviour depending on the PF being up or down? The problem I'm facing
>> is that if the user did not set a MAC address for the VF manually and
>> the PF is up during igbvf_probe it will not be labelled as random
>> although it is.
>> What about checking IGB_VF_FLAG_PF_SET_MAC and only set NET_ADDR_RANDOM
>> if the flag is cleared?
>>
> 
> The difference in behavior is because we cannot get any MAC address at all
> if the PF is down. The interface won't operate at all in this case, but if
> the PF comes up sometime later, we can start working. The other alternative
> is to leave the MAC address as all zeros and forcing the user to assign
> an address manually. We chose to use a random address to at least give it
> a chance of working once the PF woke up.
> 
> Currently, the PF has no way to communicate to the VF whether or not the
> MAC address is random or assigned. The VF cannot check the
> IGB_VF_FLAG_PF_SET_MAC flag because that only exists in the PF driver. To
> propagate this flag down to the VF driver would require changes to the
> PF/VF communication protocol.
> 
> In any case, I'm not sure that's the correct thing to do. From a policy
> viewpoint, we don't want the VF to know what's happening in the PF. It
> should not know how or why the MAC address was assigned, just like it
> should not know whether or not the PF has placed it on a VLAN. VF devices
> are not to be trusted and should not be given more information about the
> state of the PF and host OS than is absolutely necessary to operate.
> 
> What's your use case here, Stefan? Why is this flag important to you?
> As far as I can tell, nothing in the kernel ever looks at this flag.

It's about persistent device names.
You're right nothing in the kernel looks at the flag but udev uses it to
decide if the device should be identified by MAC address or PCI bus
topology.
If NET_ADDR_RANDOM is set udev will use the PCI bus information to
identify the device (instead of a changing MAC address, which would lead
to a new device name every reboot).

  Stefan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-09 18:56     ` Williams, Mitch A
  2013-01-09 19:53       ` Stefan Assmann
@ 2013-01-09 21:37       ` Greg Rose
  2013-01-14 22:25         ` Andy Gospodarek
  1 sibling, 1 reply; 13+ messages in thread
From: Greg Rose @ 2013-01-09 21:37 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: Stefan Assmann, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net

On Wed, 9 Jan 2013 18:56:36 +0000
"Williams, Mitch A" <mitch.a.williams@intel.com> wrote:

> > >> When the PF is up and igbvf is loaded the MAC address is not
> > >> generated using eth_hw_addr_random(). This results in
> > >> addr_assign_type not to be set.
> > >> Make sure it gets set.
> > >>
> > >
> > > NAK - In this case, the address may or may not be random. The
> > > user may have (and should have!) explicitly set this address from
> > > the host to ensure that the VF device receives the same address
> > > each time it
> > boots.
> > 
> > Maybe you can give me some advice on this then. Why is there
> > different behaviour depending on the PF being up or down? The
> > problem I'm facing is that if the user did not set a MAC address
> > for the VF manually and the PF is up during igbvf_probe it will not
> > be labelled as random although it is.
> > What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > NET_ADDR_RANDOM if the flag is cleared?
> > 
> 
> The difference in behavior is because we cannot get any MAC address
> at all if the PF is down. The interface won't operate at all in this
> case, but if the PF comes up sometime later, we can start working.
> The other alternative is to leave the MAC address as all zeros and
> forcing the user to assign an address manually. We chose to use a
> random address to at least give it a chance of working once the PF
> woke up.

Having been around at the inception of SR-IOV in Linux I recall that
the primary reason we used a random ethernet address was so
that the VF could at least work because there was no infrastructure
to allow the host administrator to set the MAC address of the VF.  This
hobbled testing and validation because the user would have to go to
each VM and use a command local to the VM to set the VF MAC address to
some LAA via ifconfig or ip.  When testing large numbers of VFs this was
a definite pain.

Now that has changed and I wonder if maybe we shouldn't back out the
random ethernet address assignment and go ahead with all zeros, leaving
the device non-functional until the user has intentionally set either
an LAA through the VF itself, or an administratively assigned MAC
through the ip tool via the PF.

Use of the random MAC address is not recommended by Intel's own best
known methods literature, it was used mostly so that we could get the
technology working and it should probably be at least considered for
deprecation or out right elimination.

My two cents...

- Greg

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-09 21:37       ` Greg Rose
@ 2013-01-14 22:25         ` Andy Gospodarek
  2013-01-15 18:31           ` Greg Rose
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Gospodarek @ 2013-01-14 22:25 UTC (permalink / raw)
  To: Greg Rose
  Cc: Williams, Mitch A, Stefan Assmann, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net

On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
> On Wed, 9 Jan 2013 18:56:36 +0000
> "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> 
> > > >> When the PF is up and igbvf is loaded the MAC address is not
> > > >> generated using eth_hw_addr_random(). This results in
> > > >> addr_assign_type not to be set.
> > > >> Make sure it gets set.
> > > >>
> > > >
> > > > NAK - In this case, the address may or may not be random. The
> > > > user may have (and should have!) explicitly set this address from
> > > > the host to ensure that the VF device receives the same address
> > > > each time it
> > > boots.
> > > 
> > > Maybe you can give me some advice on this then. Why is there
> > > different behaviour depending on the PF being up or down? The
> > > problem I'm facing is that if the user did not set a MAC address
> > > for the VF manually and the PF is up during igbvf_probe it will not
> > > be labelled as random although it is.
> > > What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > > NET_ADDR_RANDOM if the flag is cleared?
> > > 
> > 
> > The difference in behavior is because we cannot get any MAC address
> > at all if the PF is down. The interface won't operate at all in this
> > case, but if the PF comes up sometime later, we can start working.
> > The other alternative is to leave the MAC address as all zeros and
> > forcing the user to assign an address manually. We chose to use a
> > random address to at least give it a chance of working once the PF
> > woke up.
> 
> Having been around at the inception of SR-IOV in Linux I recall that
> the primary reason we used a random ethernet address was so
> that the VF could at least work because there was no infrastructure
> to allow the host administrator to set the MAC address of the VF.  This
> hobbled testing and validation because the user would have to go to
> each VM and use a command local to the VM to set the VF MAC address to
> some LAA via ifconfig or ip.  When testing large numbers of VFs this was
> a definite pain.
> 
> Now that has changed and I wonder if maybe we shouldn't back out the
> random ethernet address assignment and go ahead with all zeros, leaving
> the device non-functional until the user has intentionally set either
> an LAA through the VF itself, or an administratively assigned MAC
> through the ip tool via the PF.
> 
> Use of the random MAC address is not recommended by Intel's own best
> known methods literature, it was used mostly so that we could get the
> technology working and it should probably be at least considered for
> deprecation or out right elimination.
> 

It would be great to remove the bits that created random MAC addresses
for VFs, but wouldn't that break Linus' rule to "not break userspace" if
it was removed?

There are 2 options that immediately come to mind when looking to
resolve this: 

1.  Use some of the left-over bits in the mailbox messages to pass along
a flag with the E1000_VF_RESET messages to indicate whether the MAC was
randomly generated.  This would be pretty easy, but there could be
compatibility issues for a while.

2.  Default to a MAC address of all zeros, and as a device with
all-zeros for a MAC is brought up, randomly create one with
eth_hw_addr_random.  This may not immediately help cases where device
assignment are a problem, but it would ensure that any device with a
random MAC as assigned by the kernel, would have NET_ADDR_RANDOM set in
addr_assign_type.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-14 22:25         ` Andy Gospodarek
@ 2013-01-15 18:31           ` Greg Rose
  2013-01-17  0:42             ` Williams, Mitch A
  0 siblings, 1 reply; 13+ messages in thread
From: Greg Rose @ 2013-01-15 18:31 UTC (permalink / raw)
  To: Andy Gospodarek
  Cc: Williams, Mitch A, Stefan Assmann, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net

On Mon, 14 Jan 2013 17:25:42 -0500
Andy Gospodarek <andy@greyhouse.net> wrote:

> On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
> > On Wed, 9 Jan 2013 18:56:36 +0000
> > "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> > 
> > > > >> When the PF is up and igbvf is loaded the MAC address is not
> > > > >> generated using eth_hw_addr_random(). This results in
> > > > >> addr_assign_type not to be set.
> > > > >> Make sure it gets set.
> > > > >>
> > > > >
> > > > > NAK - In this case, the address may or may not be random. The
> > > > > user may have (and should have!) explicitly set this address
> > > > > from the host to ensure that the VF device receives the same
> > > > > address each time it
> > > > boots.
> > > > 
> > > > Maybe you can give me some advice on this then. Why is there
> > > > different behaviour depending on the PF being up or down? The
> > > > problem I'm facing is that if the user did not set a MAC address
> > > > for the VF manually and the PF is up during igbvf_probe it will
> > > > not be labelled as random although it is.
> > > > What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > > > NET_ADDR_RANDOM if the flag is cleared?
> > > > 
> > > 
> > > The difference in behavior is because we cannot get any MAC
> > > address at all if the PF is down. The interface won't operate at
> > > all in this case, but if the PF comes up sometime later, we can
> > > start working. The other alternative is to leave the MAC address
> > > as all zeros and forcing the user to assign an address manually.
> > > We chose to use a random address to at least give it a chance of
> > > working once the PF woke up.
> > 
> > Having been around at the inception of SR-IOV in Linux I recall that
> > the primary reason we used a random ethernet address was so
> > that the VF could at least work because there was no infrastructure
> > to allow the host administrator to set the MAC address of the VF.
> > This hobbled testing and validation because the user would have to
> > go to each VM and use a command local to the VM to set the VF MAC
> > address to some LAA via ifconfig or ip.  When testing large numbers
> > of VFs this was a definite pain.
> > 
> > Now that has changed and I wonder if maybe we shouldn't back out the
> > random ethernet address assignment and go ahead with all zeros,
> > leaving the device non-functional until the user has intentionally
> > set either an LAA through the VF itself, or an administratively
> > assigned MAC through the ip tool via the PF.
> > 
> > Use of the random MAC address is not recommended by Intel's own best
> > known methods literature, it was used mostly so that we could get
> > the technology working and it should probably be at least
> > considered for deprecation or out right elimination.
> > 
> 
> It would be great to remove the bits that created random MAC addresses
> for VFs, but wouldn't that break Linus' rule to "not break userspace"
> if it was removed?

It may, I'm not sure but before we make any changes we'd want to do our
due diligence.

> 
> There are 2 options that immediately come to mind when looking to
> resolve this: 
> 
> 1.  Use some of the left-over bits in the mailbox messages to pass
> along a flag with the E1000_VF_RESET messages to indicate whether the
> MAC was randomly generated.  This would be pretty easy, but there
> could be compatibility issues for a while.

We recently introduced the concept of mailbox message API versions in
our PF and VF drivers to handle this sort of thing.  We could probably
leverage that method to introduce a new API version that supports the
additional bits in the reset message.  It would only be used if the VF
could negotiate to the proper mailbox message API version with the PF.

> 
> 2.  Default to a MAC address of all zeros, and as a device with
> all-zeros for a MAC is brought up, randomly create one with
> eth_hw_addr_random.  This may not immediately help cases where device
> assignment are a problem, but it would ensure that any device with a
> random MAC as assigned by the kernel, would have NET_ADDR_RANDOM set
> in addr_assign_type.

Thanks for the suggestions.  We're considering some changes in this
area but we (Intel) need to give this a lot of thought and right now
we're just in a preliminary discussion mode about it.  Stay tuned.

- Greg

> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-15 18:31           ` Greg Rose
@ 2013-01-17  0:42             ` Williams, Mitch A
  2013-01-17  1:06               ` Andy Gospodarek
                                 ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Williams, Mitch A @ 2013-01-17  0:42 UTC (permalink / raw)
  To: Rose, Gregory V, Andy Gospodarek
  Cc: Stefan Assmann, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net

> -----Original Message-----
> From: Rose, Gregory V
> Sent: Tuesday, January 15, 2013 10:32 AM
> To: Andy Gospodarek
> Cc: Williams, Mitch A; Stefan Assmann; netdev@vger.kernel.org; e1000-
> devel@lists.sourceforge.net
> Subject: Re: [E1000-devel] [PATCH net-next] igbvf: fix setting
> addr_assign_type if PF is up
> 
> On Mon, 14 Jan 2013 17:25:42 -0500
> Andy Gospodarek <andy@greyhouse.net> wrote:
> 
> > On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
> > > On Wed, 9 Jan 2013 18:56:36 +0000
> > > "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> > >
> > > > > >> When the PF is up and igbvf is loaded the MAC address is not
> > > > > >> generated using eth_hw_addr_random(). This results in
> > > > > >> addr_assign_type not to be set.
> > > > > >> Make sure it gets set.
> > > > > >>
> > > > > >
> > > > > > NAK - In this case, the address may or may not be random. The
> > > > > > user may have (and should have!) explicitly set this address
> > > > > > from the host to ensure that the VF device receives the same
> > > > > > address each time it
> > > > > boots.
> > > > >
> > > > > Maybe you can give me some advice on this then. Why is there
> > > > > different behaviour depending on the PF being up or down? The
> > > > > problem I'm facing is that if the user did not set a MAC address
> > > > > for the VF manually and the PF is up during igbvf_probe it will
> > > > > not be labelled as random although it is.
> > > > > What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > > > > NET_ADDR_RANDOM if the flag is cleared?
> > > > >
> > > >
> > > > The difference in behavior is because we cannot get any MAC
> > > > address at all if the PF is down. The interface won't operate at
> > > > all in this case, but if the PF comes up sometime later, we can
> > > > start working. The other alternative is to leave the MAC address
> > > > as all zeros and forcing the user to assign an address manually.
> > > > We chose to use a random address to at least give it a chance of
> > > > working once the PF woke up.
> > >
> > > Having been around at the inception of SR-IOV in Linux I recall that
> > > the primary reason we used a random ethernet address was so that the
> > > VF could at least work because there was no infrastructure to allow
> > > the host administrator to set the MAC address of the VF.
> > > This hobbled testing and validation because the user would have to
> > > go to each VM and use a command local to the VM to set the VF MAC
> > > address to some LAA via ifconfig or ip.  When testing large numbers
> > > of VFs this was a definite pain.
> > >
> > > Now that has changed and I wonder if maybe we shouldn't back out the
> > > random ethernet address assignment and go ahead with all zeros,
> > > leaving the device non-functional until the user has intentionally
> > > set either an LAA through the VF itself, or an administratively
> > > assigned MAC through the ip tool via the PF.
> > >
> > > Use of the random MAC address is not recommended by Intel's own best
> > > known methods literature, it was used mostly so that we could get
> > > the technology working and it should probably be at least considered
> > > for deprecation or out right elimination.
> > >
> >
> > It would be great to remove the bits that created random MAC addresses
> > for VFs, but wouldn't that break Linus' rule to "not break userspace"
> > if it was removed?
> 
> It may, I'm not sure but before we make any changes we'd want to do our
> due diligence.
> 
> >
> > There are 2 options that immediately come to mind when looking to
> > resolve this:
> >
> > 1.  Use some of the left-over bits in the mailbox messages to pass
> > along a flag with the E1000_VF_RESET messages to indicate whether the
> > MAC was randomly generated.  This would be pretty easy, but there
> > could be compatibility issues for a while.
> 
> We recently introduced the concept of mailbox message API versions in
> our PF and VF drivers to handle this sort of thing.  We could probably
> leverage that method to introduce a new API version that supports the
> additional bits in the reset message.  It would only be used if the VF
> could negotiate to the proper mailbox message API version with the PF.
> 
> >
> > 2.  Default to a MAC address of all zeros, and as a device with
> > all-zeros for a MAC is brought up, randomly create one with
> > eth_hw_addr_random.  This may not immediately help cases where device
> > assignment are a problem, but it would ensure that any device with a
> > random MAC as assigned by the kernel, would have NET_ADDR_RANDOM set
> > in addr_assign_type.
> 
> Thanks for the suggestions.  We're considering some changes in this area
> but we (Intel) need to give this a lot of thought and right now we're
> just in a preliminary discussion mode about it.  Stay tuned.
> 
> - Greg

OK, here's what I'm thinking. We don't need to change the communications
protocol for this, and it shouldn't break userspace.

First, have the PF driver quit assigning random addresses. It will either
give the VF the address assigned by the administrator, or it will give
all zeros.

Second, modify the VF driver init sequence slightly. If it gets all
zeros from the PF driver, then it should give itself a random address
and set NET_ADDR_RANDOM.

If we do it this way, the VF will still come up with a random address if
one has not been assigned, and it will always know whether or not the
address that it is using is random.

If there are no objections, I'll try to get some patches done in the next
few days and get them into our internal test queue. These would then 
escape into the real world in a few weeks.

-Mitch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-17  0:42             ` Williams, Mitch A
@ 2013-01-17  1:06               ` Andy Gospodarek
  2013-01-17  1:10               ` [E1000-devel] " Andy Gospodarek
                                 ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Andy Gospodarek @ 2013-01-17  1:06 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: e1000-devel@lists.sourceforge.net, Stefan Assmann,
	netdev@vger.kernel.org


[-- Attachment #1.1: Type: text/plain, Size: 6045 bytes --]

On Jan 16, 2013 7:42 PM, "Williams, Mitch A" <mitch.a.williams@intel.com>
wrote:
>
> > -----Original Message-----
> > From: Rose, Gregory V
> > Sent: Tuesday, January 15, 2013 10:32 AM
> > To: Andy Gospodarek
> > Cc: Williams, Mitch A; Stefan Assmann; netdev@vger.kernel.org; e1000-
> > devel@lists.sourceforge.net
> > Subject: Re: [E1000-devel] [PATCH net-next] igbvf: fix setting
> > addr_assign_type if PF is up
> >
> > On Mon, 14 Jan 2013 17:25:42 -0500
> > Andy Gospodarek <andy@greyhouse.net> wrote:
> >
> > > On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
> > > > On Wed, 9 Jan 2013 18:56:36 +0000
> > > > "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> > > >
> > > > > > >> When the PF is up and igbvf is loaded the MAC address is not
> > > > > > >> generated using eth_hw_addr_random(). This results in
> > > > > > >> addr_assign_type not to be set.
> > > > > > >> Make sure it gets set.
> > > > > > >>
> > > > > > >
> > > > > > > NAK - In this case, the address may or may not be random. The
> > > > > > > user may have (and should have!) explicitly set this address
> > > > > > > from the host to ensure that the VF device receives the same
> > > > > > > address each time it
> > > > > > boots.
> > > > > >
> > > > > > Maybe you can give me some advice on this then. Why is there
> > > > > > different behaviour depending on the PF being up or down? The
> > > > > > problem I'm facing is that if the user did not set a MAC address
> > > > > > for the VF manually and the PF is up during igbvf_probe it will
> > > > > > not be labelled as random although it is.
> > > > > > What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > > > > > NET_ADDR_RANDOM if the flag is cleared?
> > > > > >
> > > > >
> > > > > The difference in behavior is because we cannot get any MAC
> > > > > address at all if the PF is down. The interface won't operate at
> > > > > all in this case, but if the PF comes up sometime later, we can
> > > > > start working. The other alternative is to leave the MAC address
> > > > > as all zeros and forcing the user to assign an address manually.
> > > > > We chose to use a random address to at least give it a chance of
> > > > > working once the PF woke up.
> > > >
> > > > Having been around at the inception of SR-IOV in Linux I recall that
> > > > the primary reason we used a random ethernet address was so that the
> > > > VF could at least work because there was no infrastructure to allow
> > > > the host administrator to set the MAC address of the VF.
> > > > This hobbled testing and validation because the user would have to
> > > > go to each VM and use a command local to the VM to set the VF MAC
> > > > address to some LAA via ifconfig or ip.  When testing large numbers
> > > > of VFs this was a definite pain.
> > > >
> > > > Now that has changed and I wonder if maybe we shouldn't back out the
> > > > random ethernet address assignment and go ahead with all zeros,
> > > > leaving the device non-functional until the user has intentionally
> > > > set either an LAA through the VF itself, or an administratively
> > > > assigned MAC through the ip tool via the PF.
> > > >
> > > > Use of the random MAC address is not recommended by Intel's own best
> > > > known methods literature, it was used mostly so that we could get
> > > > the technology working and it should probably be at least considered
> > > > for deprecation or out right elimination.
> > > >
> > >
> > > It would be great to remove the bits that created random MAC addresses
> > > for VFs, but wouldn't that break Linus' rule to "not break userspace"
> > > if it was removed?
> >
> > It may, I'm not sure but before we make any changes we'd want to do our
> > due diligence.
> >
> > >
> > > There are 2 options that immediately come to mind when looking to
> > > resolve this:
> > >
> > > 1.  Use some of the left-over bits in the mailbox messages to pass
> > > along a flag with the E1000_VF_RESET messages to indicate whether the
> > > MAC was randomly generated.  This would be pretty easy, but there
> > > could be compatibility issues for a while.
> >
> > We recently introduced the concept of mailbox message API versions in
> > our PF and VF drivers to handle this sort of thing.  We could probably
> > leverage that method to introduce a new API version that supports the
> > additional bits in the reset message.  It would only be used if the VF
> > could negotiate to the proper mailbox message API version with the PF.
> >
> > >
> > > 2.  Default to a MAC address of all zeros, and as a device with
> > > all-zeros for a MAC is brought up, randomly create one with
> > > eth_hw_addr_random.  This may not immediately help cases where device
> > > assignment are a problem, but it would ensure that any device with a
> > > random MAC as assigned by the kernel, would have NET_ADDR_RANDOM set
> > > in addr_assign_type.
> >
> > Thanks for the suggestions.  We're considering some changes in this area
> > but we (Intel) need to give this a lot of thought and right now we're
> > just in a preliminary discussion mode about it.  Stay tuned.
> >
> > - Greg
>
> OK, here's what I'm thinking. We don't need to change the communications
> protocol for this, and it shouldn't break userspace.
>
> First, have the PF driver quit assigning random addresses. It will either
> give the VF the address assigned by the administrator, or it will give
> all zeros.
>
> Second, modify the VF driver init sequence slightly. If it gets all
> zeros from the PF driver, then it should give itself a random address
> and set NET_ADDR_RANDOM.
>
> If we do it this way, the VF will still come up with a random address if
> one has not been assigned, and it will always know whether or not the
> address that it is using is random.
>
> If there are no objections, I'll try to get some patches done in the next
> few days and get them into our internal test queue. These would then
> escape into the real world in a few weeks.
>
> -Mitch

Oh, I like this even better than my proposed option 2.  I say, go for it.

[-- Attachment #2: Type: text/plain, Size: 383 bytes --]

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122712

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-17  0:42             ` Williams, Mitch A
  2013-01-17  1:06               ` Andy Gospodarek
@ 2013-01-17  1:10               ` Andy Gospodarek
  2013-01-17 11:39               ` Stefan Assmann
  2013-01-17 17:08               ` Greg Rose
  3 siblings, 0 replies; 13+ messages in thread
From: Andy Gospodarek @ 2013-01-17  1:10 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: Rose, Gregory V, Stefan Assmann, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net

On Wed, Jan 16, 2013 at 7:42 PM, Williams, Mitch A
<mitch.a.williams@intel.com> wrote:
>
> > -----Original Message-----
> > From: Rose, Gregory V
> > Sent: Tuesday, January 15, 2013 10:32 AM
> > To: Andy Gospodarek
> > Cc: Williams, Mitch A; Stefan Assmann; netdev@vger.kernel.org; e1000-
> > devel@lists.sourceforge.net
> > Subject: Re: [E1000-devel] [PATCH net-next] igbvf: fix setting
> > addr_assign_type if PF is up
> >
> > On Mon, 14 Jan 2013 17:25:42 -0500
> > Andy Gospodarek <andy@greyhouse.net> wrote:
> >
> > > On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
> > > > On Wed, 9 Jan 2013 18:56:36 +0000
> > > > "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> > > >
> > > > > > >> When the PF is up and igbvf is loaded the MAC address is not
> > > > > > >> generated using eth_hw_addr_random(). This results in
> > > > > > >> addr_assign_type not to be set.
> > > > > > >> Make sure it gets set.
> > > > > > >>
> > > > > > >
> > > > > > > NAK - In this case, the address may or may not be random. The
> > > > > > > user may have (and should have!) explicitly set this address
> > > > > > > from the host to ensure that the VF device receives the same
> > > > > > > address each time it
> > > > > > boots.
> > > > > >
> > > > > > Maybe you can give me some advice on this then. Why is there
> > > > > > different behaviour depending on the PF being up or down? The
> > > > > > problem I'm facing is that if the user did not set a MAC address
> > > > > > for the VF manually and the PF is up during igbvf_probe it will
> > > > > > not be labelled as random although it is.
> > > > > > What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > > > > > NET_ADDR_RANDOM if the flag is cleared?
> > > > > >
> > > > >
> > > > > The difference in behavior is because we cannot get any MAC
> > > > > address at all if the PF is down. The interface won't operate at
> > > > > all in this case, but if the PF comes up sometime later, we can
> > > > > start working. The other alternative is to leave the MAC address
> > > > > as all zeros and forcing the user to assign an address manually.
> > > > > We chose to use a random address to at least give it a chance of
> > > > > working once the PF woke up.
> > > >
> > > > Having been around at the inception of SR-IOV in Linux I recall that
> > > > the primary reason we used a random ethernet address was so that the
> > > > VF could at least work because there was no infrastructure to allow
> > > > the host administrator to set the MAC address of the VF.
> > > > This hobbled testing and validation because the user would have to
> > > > go to each VM and use a command local to the VM to set the VF MAC
> > > > address to some LAA via ifconfig or ip.  When testing large numbers
> > > > of VFs this was a definite pain.
> > > >
> > > > Now that has changed and I wonder if maybe we shouldn't back out the
> > > > random ethernet address assignment and go ahead with all zeros,
> > > > leaving the device non-functional until the user has intentionally
> > > > set either an LAA through the VF itself, or an administratively
> > > > assigned MAC through the ip tool via the PF.
> > > >
> > > > Use of the random MAC address is not recommended by Intel's own best
> > > > known methods literature, it was used mostly so that we could get
> > > > the technology working and it should probably be at least considered
> > > > for deprecation or out right elimination.
> > > >
> > >
> > > It would be great to remove the bits that created random MAC addresses
> > > for VFs, but wouldn't that break Linus' rule to "not break userspace"
> > > if it was removed?
> >
> > It may, I'm not sure but before we make any changes we'd want to do our
> > due diligence.
> >
> > >
> > > There are 2 options that immediately come to mind when looking to
> > > resolve this:
> > >
> > > 1.  Use some of the left-over bits in the mailbox messages to pass
> > > along a flag with the E1000_VF_RESET messages to indicate whether the
> > > MAC was randomly generated.  This would be pretty easy, but there
> > > could be compatibility issues for a while.
> >
> > We recently introduced the concept of mailbox message API versions in
> > our PF and VF drivers to handle this sort of thing.  We could probably
> > leverage that method to introduce a new API version that supports the
> > additional bits in the reset message.  It would only be used if the VF
> > could negotiate to the proper mailbox message API version with the PF.
> >
> > >
> > > 2.  Default to a MAC address of all zeros, and as a device with
> > > all-zeros for a MAC is brought up, randomly create one with
> > > eth_hw_addr_random.  This may not immediately help cases where device
> > > assignment are a problem, but it would ensure that any device with a
> > > random MAC as assigned by the kernel, would have NET_ADDR_RANDOM set
> > > in addr_assign_type.
> >
> > Thanks for the suggestions.  We're considering some changes in this area
> > but we (Intel) need to give this a lot of thought and right now we're
> > just in a preliminary discussion mode about it.  Stay tuned.
> >
> > - Greg
>
> OK, here's what I'm thinking. We don't need to change the communications
> protocol for this, and it shouldn't break userspace.
>
> First, have the PF driver quit assigning random addresses. It will either
> give the VF the address assigned by the administrator, or it will give
> all zeros.
>
> Second, modify the VF driver init sequence slightly. If it gets all
> zeros from the PF driver, then it should give itself a random address
> and set NET_ADDR_RANDOM.
>
> If we do it this way, the VF will still come up with a random address if
> one has not been assigned, and it will always know whether or not the
> address that it is using is random.
>
> If there are no objections, I'll try to get some patches done in the next
> few days and get them into our internal test queue. These would then
> escape into the real world in a few weeks.
>
> -Mitch

Oh, I like this even better than my proposed option 2.  I say, go for it.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [E1000-devel] [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-17  0:42             ` Williams, Mitch A
  2013-01-17  1:06               ` Andy Gospodarek
  2013-01-17  1:10               ` [E1000-devel] " Andy Gospodarek
@ 2013-01-17 11:39               ` Stefan Assmann
  2013-01-17 17:08               ` Greg Rose
  3 siblings, 0 replies; 13+ messages in thread
From: Stefan Assmann @ 2013-01-17 11:39 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: Rose, Gregory V, Andy Gospodarek, netdev@vger.kernel.org,
	e1000-devel@lists.sourceforge.net

On 17.01.2013 01:42, Williams, Mitch A wrote:
>> -----Original Message-----
>> From: Rose, Gregory V
>> Sent: Tuesday, January 15, 2013 10:32 AM
>> To: Andy Gospodarek
>> Cc: Williams, Mitch A; Stefan Assmann; netdev@vger.kernel.org; e1000-
>> devel@lists.sourceforge.net
>> Subject: Re: [E1000-devel] [PATCH net-next] igbvf: fix setting
>> addr_assign_type if PF is up
>>
>> On Mon, 14 Jan 2013 17:25:42 -0500
>> Andy Gospodarek <andy@greyhouse.net> wrote:
>>
>>> On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
>>>> On Wed, 9 Jan 2013 18:56:36 +0000
>>>> "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
>>>>
>>>>>>>> When the PF is up and igbvf is loaded the MAC address is not
>>>>>>>> generated using eth_hw_addr_random(). This results in
>>>>>>>> addr_assign_type not to be set.
>>>>>>>> Make sure it gets set.
>>>>>>>>
>>>>>>>
>>>>>>> NAK - In this case, the address may or may not be random. The
>>>>>>> user may have (and should have!) explicitly set this address
>>>>>>> from the host to ensure that the VF device receives the same
>>>>>>> address each time it
>>>>>> boots.
>>>>>>
>>>>>> Maybe you can give me some advice on this then. Why is there
>>>>>> different behaviour depending on the PF being up or down? The
>>>>>> problem I'm facing is that if the user did not set a MAC address
>>>>>> for the VF manually and the PF is up during igbvf_probe it will
>>>>>> not be labelled as random although it is.
>>>>>> What about checking IGB_VF_FLAG_PF_SET_MAC and only set
>>>>>> NET_ADDR_RANDOM if the flag is cleared?
>>>>>>
>>>>>
>>>>> The difference in behavior is because we cannot get any MAC
>>>>> address at all if the PF is down. The interface won't operate at
>>>>> all in this case, but if the PF comes up sometime later, we can
>>>>> start working. The other alternative is to leave the MAC address
>>>>> as all zeros and forcing the user to assign an address manually.
>>>>> We chose to use a random address to at least give it a chance of
>>>>> working once the PF woke up.
>>>>
>>>> Having been around at the inception of SR-IOV in Linux I recall that
>>>> the primary reason we used a random ethernet address was so that the
>>>> VF could at least work because there was no infrastructure to allow
>>>> the host administrator to set the MAC address of the VF.
>>>> This hobbled testing and validation because the user would have to
>>>> go to each VM and use a command local to the VM to set the VF MAC
>>>> address to some LAA via ifconfig or ip.  When testing large numbers
>>>> of VFs this was a definite pain.
>>>>
>>>> Now that has changed and I wonder if maybe we shouldn't back out the
>>>> random ethernet address assignment and go ahead with all zeros,
>>>> leaving the device non-functional until the user has intentionally
>>>> set either an LAA through the VF itself, or an administratively
>>>> assigned MAC through the ip tool via the PF.
>>>>
>>>> Use of the random MAC address is not recommended by Intel's own best
>>>> known methods literature, it was used mostly so that we could get
>>>> the technology working and it should probably be at least considered
>>>> for deprecation or out right elimination.
>>>>
>>>
>>> It would be great to remove the bits that created random MAC addresses
>>> for VFs, but wouldn't that break Linus' rule to "not break userspace"
>>> if it was removed?
>>
>> It may, I'm not sure but before we make any changes we'd want to do our
>> due diligence.
>>
>>>
>>> There are 2 options that immediately come to mind when looking to
>>> resolve this:
>>>
>>> 1.  Use some of the left-over bits in the mailbox messages to pass
>>> along a flag with the E1000_VF_RESET messages to indicate whether the
>>> MAC was randomly generated.  This would be pretty easy, but there
>>> could be compatibility issues for a while.
>>
>> We recently introduced the concept of mailbox message API versions in
>> our PF and VF drivers to handle this sort of thing.  We could probably
>> leverage that method to introduce a new API version that supports the
>> additional bits in the reset message.  It would only be used if the VF
>> could negotiate to the proper mailbox message API version with the PF.
>>
>>>
>>> 2.  Default to a MAC address of all zeros, and as a device with
>>> all-zeros for a MAC is brought up, randomly create one with
>>> eth_hw_addr_random.  This may not immediately help cases where device
>>> assignment are a problem, but it would ensure that any device with a
>>> random MAC as assigned by the kernel, would have NET_ADDR_RANDOM set
>>> in addr_assign_type.
>>
>> Thanks for the suggestions.  We're considering some changes in this area
>> but we (Intel) need to give this a lot of thought and right now we're
>> just in a preliminary discussion mode about it.  Stay tuned.
>>
>> - Greg
> 
> OK, here's what I'm thinking. We don't need to change the communications
> protocol for this, and it shouldn't break userspace.
> 
> First, have the PF driver quit assigning random addresses. It will either
> give the VF the address assigned by the administrator, or it will give
> all zeros.
> 
> Second, modify the VF driver init sequence slightly. If it gets all
> zeros from the PF driver, then it should give itself a random address
> and set NET_ADDR_RANDOM.
> 
> If we do it this way, the VF will still come up with a random address if
> one has not been assigned, and it will always know whether or not the
> address that it is using is random.
> 
> If there are no objections, I'll try to get some patches done in the next
> few days and get them into our internal test queue. These would then 
> escape into the real world in a few weeks.

Thanks Mitch! That sounds like a good idea. Let me know when you've got
something testable as I'd like to give it a try.

  Stefan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up
  2013-01-17  0:42             ` Williams, Mitch A
                                 ` (2 preceding siblings ...)
  2013-01-17 11:39               ` Stefan Assmann
@ 2013-01-17 17:08               ` Greg Rose
  3 siblings, 0 replies; 13+ messages in thread
From: Greg Rose @ 2013-01-17 17:08 UTC (permalink / raw)
  To: Williams, Mitch A
  Cc: e1000-devel@lists.sourceforge.net, Stefan Assmann,
	Andy Gospodarek, netdev@vger.kernel.org

On Wed, 16 Jan 2013 16:42:30 -0800
"Williams, Mitch A" <mitch.a.williams@intel.com> wrote:

> > -----Original Message-----
> > From: Rose, Gregory V
> > Sent: Tuesday, January 15, 2013 10:32 AM
> > To: Andy Gospodarek
> > Cc: Williams, Mitch A; Stefan Assmann; netdev@vger.kernel.org;
> > e1000- devel@lists.sourceforge.net
> > Subject: Re: [E1000-devel] [PATCH net-next] igbvf: fix setting
> > addr_assign_type if PF is up
> > 
> > On Mon, 14 Jan 2013 17:25:42 -0500
> > Andy Gospodarek <andy@greyhouse.net> wrote:
> > 
> > > On Wed, Jan 09, 2013 at 01:37:45PM -0800, Greg Rose wrote:
> > > > On Wed, 9 Jan 2013 18:56:36 +0000
> > > > "Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> > > >
> > > > > > >> When the PF is up and igbvf is loaded the MAC address is
> > > > > > >> not generated using eth_hw_addr_random(). This results in
> > > > > > >> addr_assign_type not to be set.
> > > > > > >> Make sure it gets set.
> > > > > > >>
> > > > > > >
> > > > > > > NAK - In this case, the address may or may not be random.
> > > > > > > The user may have (and should have!) explicitly set this
> > > > > > > address from the host to ensure that the VF device
> > > > > > > receives the same address each time it
> > > > > > boots.
> > > > > >
> > > > > > Maybe you can give me some advice on this then. Why is there
> > > > > > different behaviour depending on the PF being up or down?
> > > > > > The problem I'm facing is that if the user did not set a
> > > > > > MAC address for the VF manually and the PF is up during
> > > > > > igbvf_probe it will not be labelled as random although it
> > > > > > is. What about checking IGB_VF_FLAG_PF_SET_MAC and only set
> > > > > > NET_ADDR_RANDOM if the flag is cleared?
> > > > > >
> > > > >
> > > > > The difference in behavior is because we cannot get any MAC
> > > > > address at all if the PF is down. The interface won't operate
> > > > > at all in this case, but if the PF comes up sometime later,
> > > > > we can start working. The other alternative is to leave the
> > > > > MAC address as all zeros and forcing the user to assign an
> > > > > address manually. We chose to use a random address to at
> > > > > least give it a chance of working once the PF woke up.
> > > >
> > > > Having been around at the inception of SR-IOV in Linux I recall
> > > > that the primary reason we used a random ethernet address was
> > > > so that the VF could at least work because there was no
> > > > infrastructure to allow the host administrator to set the MAC
> > > > address of the VF. This hobbled testing and validation because
> > > > the user would have to go to each VM and use a command local to
> > > > the VM to set the VF MAC address to some LAA via ifconfig or
> > > > ip.  When testing large numbers of VFs this was a definite pain.
> > > >
> > > > Now that has changed and I wonder if maybe we shouldn't back
> > > > out the random ethernet address assignment and go ahead with
> > > > all zeros, leaving the device non-functional until the user has
> > > > intentionally set either an LAA through the VF itself, or an
> > > > administratively assigned MAC through the ip tool via the PF.
> > > >
> > > > Use of the random MAC address is not recommended by Intel's own
> > > > best known methods literature, it was used mostly so that we
> > > > could get the technology working and it should probably be at
> > > > least considered for deprecation or out right elimination.
> > > >
> > >
> > > It would be great to remove the bits that created random MAC
> > > addresses for VFs, but wouldn't that break Linus' rule to "not
> > > break userspace" if it was removed?
> > 
> > It may, I'm not sure but before we make any changes we'd want to do
> > our due diligence.
> > 
> > >
> > > There are 2 options that immediately come to mind when looking to
> > > resolve this:
> > >
> > > 1.  Use some of the left-over bits in the mailbox messages to pass
> > > along a flag with the E1000_VF_RESET messages to indicate whether
> > > the MAC was randomly generated.  This would be pretty easy, but
> > > there could be compatibility issues for a while.
> > 
> > We recently introduced the concept of mailbox message API versions
> > in our PF and VF drivers to handle this sort of thing.  We could
> > probably leverage that method to introduce a new API version that
> > supports the additional bits in the reset message.  It would only
> > be used if the VF could negotiate to the proper mailbox message API
> > version with the PF.
> > 
> > >
> > > 2.  Default to a MAC address of all zeros, and as a device with
> > > all-zeros for a MAC is brought up, randomly create one with
> > > eth_hw_addr_random.  This may not immediately help cases where
> > > device assignment are a problem, but it would ensure that any
> > > device with a random MAC as assigned by the kernel, would have
> > > NET_ADDR_RANDOM set in addr_assign_type.
> > 
> > Thanks for the suggestions.  We're considering some changes in this
> > area but we (Intel) need to give this a lot of thought and right
> > now we're just in a preliminary discussion mode about it.  Stay
> > tuned.
> > 
> > - Greg
> 
> OK, here's what I'm thinking. We don't need to change the
> communications protocol for this, and it shouldn't break userspace.
> 
> First, have the PF driver quit assigning random addresses. It will
> either give the VF the address assigned by the administrator, or it
> will give all zeros.
> 
> Second, modify the VF driver init sequence slightly. If it gets all
> zeros from the PF driver, then it should give itself a random address
> and set NET_ADDR_RANDOM.
> 
> If we do it this way, the VF will still come up with a random address
> if one has not been assigned, and it will always know whether or not
> the address that it is using is random.
> 
> If there are no objections, I'll try to get some patches done in the
> next few days and get them into our internal test queue. These would
> then escape into the real world in a few weeks.
> 
> -Mitch

I'll second and third Andy and Stefan and say go for it.  I'll look
into making equivalent changes for the 82599 and X540 10G drivers.

thanks Mitch,

- Greg

------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnmore_122712
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-01-17 17:08 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-09  9:59 [PATCH net-next] igbvf: fix setting addr_assign_type if PF is up Stefan Assmann
2013-01-09 17:09 ` Williams, Mitch A
2013-01-09 17:58   ` [E1000-devel] " Stefan Assmann
2013-01-09 18:56     ` Williams, Mitch A
2013-01-09 19:53       ` Stefan Assmann
2013-01-09 21:37       ` Greg Rose
2013-01-14 22:25         ` Andy Gospodarek
2013-01-15 18:31           ` Greg Rose
2013-01-17  0:42             ` Williams, Mitch A
2013-01-17  1:06               ` Andy Gospodarek
2013-01-17  1:10               ` [E1000-devel] " Andy Gospodarek
2013-01-17 11:39               ` Stefan Assmann
2013-01-17 17:08               ` Greg Rose

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).