RE: [PATCH net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: Leon Romanovsky <leon@xxxxxxxxxx>
> Sent: Wednesday, April 7, 2021 8:51 AM
> To: Dexuan Cui <decui@xxxxxxxxxxxxx>
> Cc: davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; KY Srinivasan
> <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Wei Liu
> <liuwe@xxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> On Wed, Apr 07, 2021 at 08:40:13AM +0000, Dexuan Cui wrote:
> > > From: Leon Romanovsky <leon@xxxxxxxxxx>
> > > Sent: Wednesday, April 7, 2021 1:10 AM
> > >
> > > <...>
> > >
> > > > +int gdma_verify_vf_version(struct pci_dev *pdev)
> > > > +{
> > > > +	struct gdma_context *gc = pci_get_drvdata(pdev);
> > > > +	struct gdma_verify_ver_req req = { 0 };
> > > > +	struct gdma_verify_ver_resp resp = { 0 };
> > > > +	int err;
> > > > +
> > > > +	gdma_init_req_hdr(&req.hdr, GDMA_VERIFY_VF_DRIVER_VERSION,
> > > > +			  sizeof(req), sizeof(resp));
> > > > +
> > > > +	req.protocol_ver_min = GDMA_PROTOCOL_FIRST;
> > > > +	req.protocol_ver_max = GDMA_PROTOCOL_LAST;
> > > > +
> > > > +	err = gdma_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
> > > > +	if (err || resp.hdr.status) {
> > > > +		pr_err("VfVerifyVersionOutput: %d, status=0x%x\n", err,
> > > > +		       resp.hdr.status);
> > > > +		return -EPROTO;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > >
> > > <...>
> > > > +	err = gdma_verify_vf_version(pdev);
> > > > +	if (err)
> > > > +		goto remove_irq;
> > >
> > > Will this VF driver be used in the guest VM? What will prevent from users
> to
> > > change it?
> > > I think that such version negotiation scheme is not allowed.
> >
> > Yes, the VF driver is expected to run in a Linux VM that runs on Azure.
> >
> > Currently gdma_verify_vf_version() just tells the PF driver that the VF
> driver
> > is only able to support GDMA_PROTOCOL_V1, and want to use
> > GDMA_PROTOCOL_V1's message formats to talk to the PF driver later.
> >
> > enum {
> >         GDMA_PROTOCOL_UNDEFINED = 0,
> >         GDMA_PROTOCOL_V1 = 1,
> >         GDMA_PROTOCOL_FIRST = GDMA_PROTOCOL_V1,
> >         GDMA_PROTOCOL_LAST = GDMA_PROTOCOL_V1,
> >         GDMA_PROTOCOL_VALUE_MAX
> > };
> >
> > The PF driver is supposed to always support GDMA_PROTOCOL_V1, so I
> expect
> > here gdma_verify_vf_version() should succeed. If a user changes the Linux
> VF
> > driver and try to use a protocol version not supported by the PF driver,
> then
> > gdma_verify_vf_version() will fail; later, if the VF driver tries to talk to the
> PF
> > driver using an unsupported message format, the PF driver will return a
> failure.
> 
> The worry is not for the current code, but for the future one when you will
> support v2, v3 e.t.c. First, your code will look like a spaghetti and
> second, users will try and mix vX with "unsupported" commands just for the
> fun.

In the future, if the protocol version updated on the host side, guests need 
to support different host versions because not all hosts are updated 
(simultaneously). So this negotiation is necessary to know the supported 
version, and decide the proper command version to use. 

If any user try "unsupported commands just for the fun", the host will deny 
and return an error.

Thanks,
- Haiyang