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

[Haiyang Zhang]

> -----Original Message-----
> From: Andrea Parri <parri.andrea@xxxxxxxxx>
> Sent: Wednesday, January 20, 2021 11:05 PM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>; Wei Liu
> <wei.liu@xxxxxxxxxx>; Michael Kelley <mikelley@xxxxxxxxxxxxx>; Tianyu Lan
> <Tianyu.Lan@xxxxxxxxxxxxx>; Saruhan Karademir
> <skarade@xxxxxxxxxxxxx>; Juan Vazquez <juvazq@xxxxxxxxxxxxx>; linux-
> hyperv@xxxxxxxxxxxxxxx; David S. Miller <davem@xxxxxxxxxxxxx>; Jakub
> Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/4] hv_netvsc: Restrict configurations on isolated
> guests
> 
> > > @@ -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-
> > > >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,
> > 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"

> 
> 
> > > @@ -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.
> 
> 
> > > +}
> > > +
> > >  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.
> > I'd suggest move this check after version negotiation is done. So we can log
> what's
> > 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.

> 
> 
> > > @@ -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