[PATCH] igb: Indicate failure on vf reset for empty mac address

[PATCH] igb: Indicate failure on vf reset for empty mac address

[Alexander Graf]

Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
address to the VF on reset rather than a random one.

However, that behavioral change also requires igbvf driver changes
which can be hard especially when we want to talk to proprietary
guest OSs.

Looking at the code previous to the commit in Linux that made igbvf
work with empty mac addresses (8d56b6d), we can see that on reset
failure the driver will try to generate a new mac address with both
the old and the new code.

Furthermore, ixgbe does send reset failure when it detects an empty
mac address (35055928c).

So I think it's safe to make igb behave the same. With this patch I
can successfully run a Windows 8.1 guest with an empty mac address
and an assigned igbvf device that has no mac address set by the host.

If anyone is aware of a guest driver that chokes on NACK returns of
VF RESET commands, please speak up.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cb14bbd..e8c53b6 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6024,8 +6024,12 @@ static void igb_vf_reset_msg(struct igb_adapter *adapter, u32 vf)
 	adapter->vf_data[vf].flags |= IGB_VF_FLAG_CTS;
 
 	/* reply to reset with ack and vf mac address */
-	msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
-	memcpy(addr, vf_mac, ETH_ALEN);
+	if (!is_zero_ether_addr(vf_mac)) {
+		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
+		memcpy(addr, vf_mac, ETH_ALEN);
+	} else {
+		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_NACK;
+	}
 	igb_write_mbx(hw, msgbuf, 3, vf);
 }
 
-- 
1.8.1.4

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
> address to the VF on reset rather than a random one.
> 
> However, that behavioral change also requires igbvf driver changes
> which can be hard especially when we want to talk to proprietary
> guest OSs.
> 
> Looking at the code previous to the commit in Linux that made igbvf
> work with empty mac addresses (8d56b6d), we can see that on reset
> failure the driver will try to generate a new mac address with both
> the old and the new code.
> 
> Furthermore, ixgbe does send reset failure when it detects an empty
> mac address (35055928c).
> 
> So I think it's safe to make igb behave the same. With this patch I
> can successfully run a Windows 8.1 guest with an empty mac address
> and an assigned igbvf device that has no mac address set by the host.
> 
> If anyone is aware of a guest driver that chokes on NACK returns of
> VF RESET commands, please speak up.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Thanks Alexander, I will add your patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
> address to the VF on reset rather than a random one.
> 
> However, that behavioral change also requires igbvf driver changes
> which can be hard especially when we want to talk to proprietary
> guest OSs.
> 
> Looking at the code previous to the commit in Linux that made igbvf
> work with empty mac addresses (8d56b6d), we can see that on reset
> failure the driver will try to generate a new mac address with both
> the old and the new code.
> 
> Furthermore, ixgbe does send reset failure when it detects an empty
> mac address (35055928c).
> 
> So I think it's safe to make igb behave the same. With this patch I
> can successfully run a Windows 8.1 guest with an empty mac address
> and an assigned igbvf device that has no mac address set by the host.
> 
> If anyone is aware of a guest driver that chokes on NACK returns of
> VF RESET commands, please speak up.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index cb14bbd..e8c53b6 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6024,8 +6024,12 @@ static void igb_vf_reset_msg(struct igb_adapter *adapter, u32 vf)
>  	adapter->vf_data[vf].flags |= IGB_VF_FLAG_CTS;
>  
>  	/* reply to reset with ack and vf mac address */
> -	msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
> -	memcpy(addr, vf_mac, ETH_ALEN);
> +	if (!is_zero_ether_addr(vf_mac)) {
> +		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
> +		memcpy(addr, vf_mac, ETH_ALEN);

I know we were using memcpy() before, but shouldn't we convert this to
use the preferred ether_addr_copy()?

> +	} else {
> +		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_NACK;
> +	}
>  	igb_write_mbx(hw, msgbuf, 3, vf);
>  }
>  



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Alexander Graf]

> Am 08.10.2014 um 23:32 schrieb Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> 
>> On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
>> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
>> address to the VF on reset rather than a random one.
>> 
>> However, that behavioral change also requires igbvf driver changes
>> which can be hard especially when we want to talk to proprietary
>> guest OSs.
>> 
>> Looking at the code previous to the commit in Linux that made igbvf
>> work with empty mac addresses (8d56b6d), we can see that on reset
>> failure the driver will try to generate a new mac address with both
>> the old and the new code.
>> 
>> Furthermore, ixgbe does send reset failure when it detects an empty
>> mac address (35055928c).
>> 
>> So I think it's safe to make igb behave the same. With this patch I
>> can successfully run a Windows 8.1 guest with an empty mac address
>> and an assigned igbvf device that has no mac address set by the host.
>> 
>> If anyone is aware of a guest driver that chokes on NACK returns of
>> VF RESET commands, please speak up.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index cb14bbd..e8c53b6 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -6024,8 +6024,12 @@ static void igb_vf_reset_msg(struct igb_adapter *adapter, u32 vf)
>>    adapter->vf_data[vf].flags |= IGB_VF_FLAG_CTS;
>> 
>>    /* reply to reset with ack and vf mac address */
>> -    msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
>> -    memcpy(addr, vf_mac, ETH_ALEN);
>> +    if (!is_zero_ether_addr(vf_mac)) {
>> +        msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
>> +        memcpy(addr, vf_mac, ETH_ALEN);
> 
> I know we were using memcpy() before, but shouldn't we convert this to
> use the preferred ether_addr_copy()?

Sure, but don't you think a coccinelle script across the intel drivers would be better than single spot changes? ;)

Alex

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
> address to the VF on reset rather than a random one.
> 
> However, that behavioral change also requires igbvf driver changes
> which can be hard especially when we want to talk to proprietary
> guest OSs.
> 
> Looking at the code previous to the commit in Linux that made igbvf
> work with empty mac addresses (8d56b6d), we can see that on reset
> failure the driver will try to generate a new mac address with both
> the old and the new code.
> 
> Furthermore, ixgbe does send reset failure when it detects an empty
> mac address (35055928c).
> 
> So I think it's safe to make igb behave the same. With this patch I
> can successfully run a Windows 8.1 guest with an empty mac address
> and an assigned igbvf device that has no mac address set by the host.
> 
> If anyone is aware of a guest driver that chokes on NACK returns of
> VF RESET commands, please speak up.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Actually after further review of this patch and the reported bug in
SuSE's bugzilla, we are NACK'ing this patch.

If the reset has not failed, why are we indicating that it has?  We
originally supplied the VF with a NULL MAC, so we should supply it
again.  That way, the VF can choose to either regenerate a new random
MAC or keep using the one that it had.

The current method was a fix that was requested by the community in the
first place, also we cannot take into account "proprietary guest OS's".

> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index cb14bbd..e8c53b6 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6024,8 +6024,12 @@ static void igb_vf_reset_msg(struct igb_adapter *adapter, u32 vf)
>  	adapter->vf_data[vf].flags |= IGB_VF_FLAG_CTS;
>  
>  	/* reply to reset with ack and vf mac address */
> -	msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
> -	memcpy(addr, vf_mac, ETH_ALEN);
> +	if (!is_zero_ether_addr(vf_mac)) {
> +		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
> +		memcpy(addr, vf_mac, ETH_ALEN);
> +	} else {
> +		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_NACK;
> +	}
>  	igb_write_mbx(hw, msgbuf, 3, vf);
>  }
>  



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Alexander Graf]

> Am 09.10.2014 um 00:29 schrieb Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> 
>> On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
>> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
>> address to the VF on reset rather than a random one.
>> 
>> However, that behavioral change also requires igbvf driver changes
>> which can be hard especially when we want to talk to proprietary
>> guest OSs.
>> 
>> Looking at the code previous to the commit in Linux that made igbvf
>> work with empty mac addresses (8d56b6d), we can see that on reset
>> failure the driver will try to generate a new mac address with both
>> the old and the new code.
>> 
>> Furthermore, ixgbe does send reset failure when it detects an empty
>> mac address (35055928c).
>> 
>> So I think it's safe to make igb behave the same. With this patch I
>> can successfully run a Windows 8.1 guest with an empty mac address
>> and an assigned igbvf device that has no mac address set by the host.
>> 
>> If anyone is aware of a guest driver that chokes on NACK returns of
>> VF RESET commands, please speak up.
>> 
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>> drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> Actually after further review of this patch and the reported bug in
> SuSE's bugzilla, we are NACK'ing this patch.
> 
> If the reset has not failed, why are we indicating that it has?

Very good question. It's what the ixgbe driver does and older Linux, newer Linux as well as Windows seem to interpret it as "I need to generate a mac address".

I have no idea whether that's the intended meaning of a reset nack, but if it isn't, ixgbe probably shouldn't nack it either then.

Is the mbox message protocol documented anywhere? I could only find references to register layouts so far, but nothing about the comminucation on top of them.

> We
> originally supplied the VF with a NULL MAC, so we should supply it
> again.  That way, the VF can choose to either regenerate a new random
> MAC or keep using the one that it had.

Where is this documented? And why doesn't ixgbe adhere to this?

> The current method was a fix that was requested by the community in the
> first place, also we cannot take into account "proprietary guest OS's".

I don't think I understand this comment. "The community" is a very broad term in Linux ;). Also, this patch merely moves igb to behave identically to ixgbe in how it tells an igbvf driver that the mac address is empty. Nothing changes wrt the original change to not generate random mac addresses. That stays identical.


Alex

RE: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Williams, Mitch A]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, October 08, 2014 3:39 PM
> To: Kirsher, Jeffrey T
> Cc: David S. Miller; netdev@vger.kernel.org; Williams, Mitch A; Andy
> Gospodarek; Stefan Assmann; Brown, Aaron F; Rose, Gregory V; Ronciak, John
> Subject: Re: [PATCH] igb: Indicate failure on vf reset for empty mac address
> 
> 
> 
> 
> > Am 09.10.2014 um 00:29 schrieb Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> >
> >> On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
> >> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
> >> address to the VF on reset rather than a random one.
> >>
> >> However, that behavioral change also requires igbvf driver changes
> >> which can be hard especially when we want to talk to proprietary
> >> guest OSs.
> >>
> >> Looking at the code previous to the commit in Linux that made igbvf
> >> work with empty mac addresses (8d56b6d), we can see that on reset
> >> failure the driver will try to generate a new mac address with both
> >> the old and the new code.
> >>
> >> Furthermore, ixgbe does send reset failure when it detects an empty
> >> mac address (35055928c).
> >>
> >> So I think it's safe to make igb behave the same. With this patch I
> >> can successfully run a Windows 8.1 guest with an empty mac address
> >> and an assigned igbvf device that has no mac address set by the host.
> >>
> >> If anyone is aware of a guest driver that chokes on NACK returns of
> >> VF RESET commands, please speak up.
> >>
> >> Signed-off-by: Alexander Graf <agraf@suse.de>
> >> ---
> >> drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > Actually after further review of this patch and the reported bug in
> > SuSE's bugzilla, we are NACK'ing this patch.
> >
> > If the reset has not failed, why are we indicating that it has?
> 
> Very good question. It's what the ixgbe driver does and older Linux, newer
> Linux as well as Windows seem to interpret it as "I need to generate a mac
> address".
> 
> I have no idea whether that's the intended meaning of a reset nack, but if
> it isn't, ixgbe probably shouldn't nack it either then.
> 
> Is the mbox message protocol documented anywhere? I could only find
> references to register layouts so far, but nothing about the comminucation
> on top of them.
> 
> > We
> > originally supplied the VF with a NULL MAC, so we should supply it
> > again.  That way, the VF can choose to either regenerate a new random
> > MAC or keep using the one that it had.
> 
> Where is this documented? And why doesn't ixgbe adhere to this?
> 
> > The current method was a fix that was requested by the community in the
> > first place, also we cannot take into account "proprietary guest OS's".
> 
> I don't think I understand this comment. "The community" is a very broad
> term in Linux ;). Also, this patch merely moves igb to behave identically to
> ixgbe in how it tells an igbvf driver that the mac address is empty. Nothing
> changes wrt the original change to not generate random mac addresses. That
> stays identical.
> 
> 
> Alex

Alex, I can't speak for ixgbe - I've never worked on that driver. However, for igb and igbvf, I was the one who changed the MAC address generation code, at the request of the Linux networking community. In general, the PF driver provides a valid MAC to the VF driver if and only if the user has assigned a fixed MAC address for that VF on the command line. Otherwise, the VF driver gets all zeros, and it knows to generate its own random MAC address. This was done so that the VF would know when it was using a random MAC address and be able to inform the stack (and, eventually, udev). This kept udev from creating a new interface name each time the VF was booted.

On a reset, the PF should not generate a new MAC address for the VF driver. It shouldn't do anything with the VF MAC, except report what it knows to the VF driver - either all zeros, or the address assigned by the user. It's up to the VF driver to know what to do.

And the PF should not lie to the VF driver and tell it that reset failed. That's just incorrect.

Yes, there are some older Linux and Windows VF drivers that don't know what to do in this situation. But we made this change almost two years ago, and all of our current drivers (both Windows and Linux) do the right thing.

If you have a bug to report against a current Windows or Linux VF driver, please let us know. We will be happy to help. 

If you have another proprietary OS that doesn't work, then you need to fix that VF driver, not the PF driver. The Linux kernel community will not accept a change that is only made to support a non-Linux guest.

In any case, this patch is not correct, so we have responded with a NAK.

-Mitch Williams

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Alexander Graf]

On 09.10.14 01:33, Williams, Mitch A wrote:
> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, October 08, 2014 3:39 PM
>> To: Kirsher, Jeffrey T
>> Cc: David S. Miller; netdev@vger.kernel.org; Williams, Mitch A; Andy
>> Gospodarek; Stefan Assmann; Brown, Aaron F; Rose, Gregory V; Ronciak, John
>> Subject: Re: [PATCH] igb: Indicate failure on vf reset for empty mac address
>>
>>
>>
>>
>>> Am 09.10.2014 um 00:29 schrieb Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
>>>
>>>> On Wed, 2014-10-08 at 23:23 +0200, Alexander Graf wrote:
>>>> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac
>>>> address to the VF on reset rather than a random one.
>>>>
>>>> However, that behavioral change also requires igbvf driver changes
>>>> which can be hard especially when we want to talk to proprietary
>>>> guest OSs.
>>>>
>>>> Looking at the code previous to the commit in Linux that made igbvf
>>>> work with empty mac addresses (8d56b6d), we can see that on reset
>>>> failure the driver will try to generate a new mac address with both
>>>> the old and the new code.
>>>>
>>>> Furthermore, ixgbe does send reset failure when it detects an empty
>>>> mac address (35055928c).
>>>>
>>>> So I think it's safe to make igb behave the same. With this patch I
>>>> can successfully run a Windows 8.1 guest with an empty mac address
>>>> and an assigned igbvf device that has no mac address set by the host.
>>>>
>>>> If anyone is aware of a guest driver that chokes on NACK returns of
>>>> VF RESET commands, please speak up.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>> drivers/net/ethernet/intel/igb/igb_main.c | 8 ++++++--
>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> Actually after further review of this patch and the reported bug in
>>> SuSE's bugzilla, we are NACK'ing this patch.
>>>
>>> If the reset has not failed, why are we indicating that it has?
>>
>> Very good question. It's what the ixgbe driver does and older Linux, newer
>> Linux as well as Windows seem to interpret it as "I need to generate a mac
>> address".
>>
>> I have no idea whether that's the intended meaning of a reset nack, but if
>> it isn't, ixgbe probably shouldn't nack it either then.
>>
>> Is the mbox message protocol documented anywhere? I could only find
>> references to register layouts so far, but nothing about the comminucation
>> on top of them.
>>
>>> We
>>> originally supplied the VF with a NULL MAC, so we should supply it
>>> again.  That way, the VF can choose to either regenerate a new random
>>> MAC or keep using the one that it had.
>>
>> Where is this documented? And why doesn't ixgbe adhere to this?
>>
>>> The current method was a fix that was requested by the community in the
>>> first place, also we cannot take into account "proprietary guest OS's".
>>
>> I don't think I understand this comment. "The community" is a very broad
>> term in Linux ;). Also, this patch merely moves igb to behave identically to
>> ixgbe in how it tells an igbvf driver that the mac address is empty. Nothing
>> changes wrt the original change to not generate random mac addresses. That
>> stays identical.
>>
>>
>> Alex
> 
> Alex, I can't speak for ixgbe - I've never worked on that driver. However, for igb and igbvf, I was the one who changed the MAC address generation code, at the request of the Linux networking community. In general, the PF driver provides a valid MAC to the VF driver if and only if the user has assigned a fixed MAC address for that VF on the command line. Otherwise, the VF driver gets all zeros, and it knows to generate its own random MAC address. This was done so that the VF would know when it was using a random MAC address and be able to inform the stack (and, eventually, udev). This kept udev from creating a new interface name each time the VF was booted.

Yup, makes a lot of sense.

> 
> On a reset, the PF should not generate a new MAC address for the VF driver. It shouldn't do anything with the VF MAC, except report what it knows to the VF driver - either all zeros, or the address assigned by the user. It's up to the VF driver to know what to do.

Well, semantically you want to tell the guest "I don't have a mac
address" or "this is your mac address". Whether the former is encoded in
an empty mac address is really an implementation detail I suppose.

> 
> And the PF should not lie to the VF driver and tell it that reset failed. That's just incorrect.
> 
> Yes, there are some older Linux and Windows VF drivers that don't know what to do in this situation. But we made this change almost two years ago, and all of our current drivers (both Windows and Linux) do the right thing.
> 
> If you have a bug to report against a current Windows or Linux VF driver, please let us know. We will be happy to help. 

Sure! So what I did was that I picked a recent Windows 8.1 installation
and ran it in a VM. Then I assigned the VF to it and it just borked at
me ;).

Apparently 2 years weren't enough for the driver to trickle into the
latest released version of Windows.

> 
> If you have another proprietary OS that doesn't work, then you need to fix that VF driver, not the PF driver. The Linux kernel community will not accept a change that is only made to support a non-Linux guest.

The interface in question is a guest interface. Breaking it in the first
place was probably not a very good decision. Instead, it would've been
better to enhance the protocol so that instead of sending a "1" command,
you would send another command that indicates to the PF driver that the
guest knows how to react to an empty mac address. For the old "1"
command you could've returned randomly generated mac addresses still, or
failed the request altogether (which coincidentally would've made it
work too). But it's probably too late for that :).

> In any case, this patch is not correct, so we have responded with a NAK.

So we're stuck in a nasty situation here. There are 2 problems that I
can see:

  1) User experience is horrible. There is no way mortals will find out
why their VFs don't work in VMs with Windows. It's good for our business
model of paid support, but probably not in Intel's best interest ;)

  2) Igb and ixgbe implement an incredibly similar protocol between VF
and PF, yet are inconsistent in how they react to the reset function. I
dislike inconsistencies :)

I agree that failing the reset sounds odd, but why does the ixgbe driver
do it? And why do the Linux and Windows drivers recover properly from
that failure? Someone must've thought of that case, no?


Alex

Re: [PATCH] igb: Indicate failure on vf reset for empty mac address

[Sergei Shtylyov]

Hello.

On 10/9/2014 1:23 AM, Alexander Graf wrote:

> Commit 5ac6f91d changed the igb driver to expose a zero (empty) mac

   Please also specify that commit's summary line in parens.

> address to the VF on reset rather than a random one.

> However, that behavioral change also requires igbvf driver changes
> which can be hard especially when we want to talk to proprietary
> guest OSs.

> Looking at the code previous to the commit in Linux that made igbvf
> work with empty mac addresses (8d56b6d), we can see that on reset

    And that one's

> failure the driver will try to generate a new mac address with both
> the old and the new code.

> Furthermore, ixgbe does send reset failure when it detects an empty
> mac address (35055928c).

    And that one's too.

> So I think it's safe to make igb behave the same. With this patch I
> can successfully run a Windows 8.1 guest with an empty mac address
> and an assigned igbvf device that has no mac address set by the host.

> If anyone is aware of a guest driver that chokes on NACK returns of
> VF RESET commands, please speak up.

> Signed-off-by: Alexander Graf <agraf@suse.de>

WBR, Sergei