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

From: Leon Romanovsky <leon@kernel.org>
To: Dexuan Cui <decui@microsoft.com>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	Wei Liu <liuwe@microsoft.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>
Subject: Re: [PATCH net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
Date: Wed, 7 Apr 2021 15:51:18 +0300	[thread overview]
Message-ID: <YG2qxjPJ4ruas1dI@unreal> (raw)
In-Reply-To: <MW2PR2101MB08923D4417E44C5750BFB964BF759@MW2PR2101MB0892.namprd21.prod.outlook.com>

On Wed, Apr 07, 2021 at 08:40:13AM +0000, Dexuan Cui wrote:
> > From: Leon Romanovsky <leon@kernel.org>
> > 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.

Thanks

> 
> Thanks,
> -- Dexuan