RE: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated guests

-----Original Message-----
From: Andrea Parri <parri.andrea@gmail.com>
Sent: Wednesday, January 20, 2021 11:05 PM
To: Haiyang Zhang <haiyangz@microsoft.com>
Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
Stephen Hemminger [off-list ref]; Wei Liu
[off-list ref]; Michael Kelley [off-list ref]; Tianyu Lan
[off-list ref]; Saruhan Karademir
[off-list ref]; Juan Vazquez [off-list ref]; linux-
hyperv@vger.kernel.org; David S. Miller [off-list ref]; Jakub
Kicinski [off-list ref]; netdev@vger.kernel.org
Subject: Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated
guests

quoted

quoted

@@ -544,7 +545,8 @@ static int negotiate_nvsp_ver(struct hv_device

*device,
 	init_packet->msg.v2_msg.send_ndis_config.capability.ieee8021q = 1;

 	if (nvsp_ver >= NVSP_PROTOCOL_VERSION_5) {
-		init_packet->msg.v2_msg.send_ndis_config.capability.sriov =
1;
+		if (!hv_is_isolation_supported())
+			init_packet-

quoted

msg.v2_msg.send_ndis_config.capability.sriov = 1;

Please also add a log there stating we don't support sriov in this case.

Otherwise,

quoted

customers will ask why vf not showing up.

IIUC, you're suggesting that I append something like:

+		else
+			netdev_info(ndev, "SR-IOV not advertised: isolation
supported\n");

I've added this locally; please let me know if you had something else
/better in mind.

This message explains the failure reason better:
  "SR-IOV not advertised by guests on the host supporting isolation"

quoted

quoted

@@ -563,6 +565,13 @@ static int negotiate_nvsp_ver(struct hv_device

*device,
 	return ret;
 }

+static bool nvsp_is_valid_version(u32 version)
+{
+       if (hv_is_isolation_supported())
+               return version >= NVSP_PROTOCOL_VERSION_61;
+       return true;

Hosts support isolation should run nvsp 6.1+. This error is not expected.
Instead of fail silently, we should log an error to explain why it's failed, and

the current version and expected version.

Please see my next comment below.

quoted

quoted

+}
+
 static int netvsc_connect_vsp(struct hv_device *device,
 			      struct netvsc_device *net_device,
 			      const struct netvsc_device_info *device_info)

@@ -579,12 +588,17 @@ static int netvsc_connect_vsp(struct hv_device

*device,
 	init_packet = &net_device->channel_init_pkt;

 	/* Negotiate the latest NVSP protocol supported */
-	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--)
+	for (i = ARRAY_SIZE(ver_list) - 1; i >= 0; i--) {
+		if (!nvsp_is_valid_version(ver_list[i])) {
+			ret = -EPROTO;
+			goto cleanup;
+		}

This code can catch the invalid, but cannot get the current host nvsp

version.

quoted

I'd suggest move this check after version negotiation is done. So we can log

what's

quoted

the current host nvsp version, and why we fail it (the expected nvsp ver).

Mmh, invalid versions are not negotiated.  How about I simply add the
following logging right before the above 'ret = -EPROTO' say?

+			netdev_err(ndev, "Invalid NVSP version %x
(expected >= %x): isolation supported\n",
+				   ver_list[i], NVSP_PROTOCOL_VERSION_61);

(or something along these lines)

The negotiation process runs from the latest to oldest. If the host is 5, your code 
will fail before trying v6.0, and log:
	"Invalid NVSP version 60000  (expected >= 60001): isolation supported"
This will make user think the NVSP version is 6.0.

Since you will let the NIC fail and cleanup, there is no harm to check the version 
after negotiation. And this case is unexpected from a "normal" host. So I suggest 
move the check after negotiation is done, then we know the actual host nvsp 
version that causing this issue. And we can bring the accurate info to host team 
for better diagnosability.

Please point out this invalid version is caused by the host side, like this:
	"Invalid NVSP version 0x50000  (expected >= 0x60001) from the host with isolation support"
Also please use "0x%x" for hexadecimal numbers.

quoted

quoted

@@ -1357,7 +1371,8 @@ static void netvsc_receive_inband(struct

net_device *ndev,
 		break;

 	case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
-		netvsc_send_vf(ndev, nvmsg, msglen);
+		if (!hv_is_isolation_supported())
+			netvsc_send_vf(ndev, nvmsg, msglen);

When the driver doesn't advertise SRIOV, this message is not expected.
Instead of ignore silently, we should log an error.

I've appended:

+		else
+			netdev_err(ndev, "Unexpected VF message:
isolation supported\n");

Please log the msg type:
  "Ignore VF_ASSOCIATION msg from the host supporting isolation"

Thanks,
- Haiyang