netdev - Re: [PATCH net-next v4 1/4] gve: Add basic driver framework for Compute Engine Virtual NIC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190701194736.6af1f2ed@cakuba.netronome.com>
Date:   Mon, 1 Jul 2019 19:47:36 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Catherine Sullivan <csully@...gle.com>
Cc:     netdev@...r.kernel.org, Sagi Shahar <sagis@...gle.com>,
        Jon Olson <jonolson@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Luigi Rizzo <lrizzo@...gle.com>
Subject: Re: [PATCH net-next v4 1/4] gve: Add basic driver framework for
 Compute Engine Virtual NIC

On Mon,  1 Jul 2019 15:57:52 -0700, Catherine Sullivan wrote:
> Add a driver framework for the Compute Engine Virtual NIC that will be
> available in the future.
> 
> At this point the only functionality is loading the driver.
> 
> Signed-off-by: Catherine Sullivan <csully@...gle.com>
> Signed-off-by: Sagi Shahar <sagis@...gle.com>
> Signed-off-by: Jon Olson <jonolson@...gle.com>
> Acked-by: Willem de Bruijn <willemb@...gle.com>
> Reviewed-by: Luigi Rizzo <lrizzo@...gle.com>

> +Admin Queue (AQ)
> +----------------
> +The Admin Queue is a PAGE_SIZE memory block, treated as an array of AQ
> +commands, used by the driver to issue commands to the device and set up
> +resources.The driver and the device maintain a count of how many commands
> +have been submitted and executed. To issue AQ commands, the driver must do
> +the following (with proper locking):
> +
> +1)  Copy new commands into next available slots in the AQ array
> +2)  Increment its counter by he number of new commands

s/he/the/

> +3)  Write the counter into the GVE_ADMIN_QUEUE_DOORBELL register
> +4)  Poll the ADMIN_QUEUE_EVENT_COUNTER register until it equals
> +    the value written to the doorbell, or until a timeout.
> +
> +The device will update the status field in each AQ command reported as
> +executed through the ADMIN_QUEUE_EVENT_COUNTER register.
> +

> +/* This function is not threadsafe - the caller is responsible for any
> + * necessary locks.
> + */
> +int gve_adminq_execute_cmd(struct gve_priv *priv,
> +			   union gve_adminq_command *cmd_orig)
> +{
> +	union gve_adminq_command *cmd;
> +	u32 status = 0;
> +	u32 prod_cnt;
> +
> +	cmd = &priv->adminq[priv->adminq_prod_cnt & priv->adminq_mask];
> +	priv->adminq_prod_cnt++;
> +	prod_cnt = priv->adminq_prod_cnt;
> +
> +	memcpy(cmd, cmd_orig, sizeof(*cmd_orig));

Eh, I guess you don't even need memory barriers around the data movement
to the DMA buffer because of the restriction to x86? :)

> +	gve_adminq_kick_cmd(priv, prod_cnt);
> +	if (!gve_adminq_wait_for_cmd(priv, prod_cnt)) {
> +		dev_err(&priv->pdev->dev, "AQ command timed out, need to reset AQ\n");
> +		return -ENOTRECOVERABLE;
> +	}

> +	memcpy(cmd_orig, cmd, sizeof(*cmd));
> +	status = be32_to_cpu(READ_ONCE(cmd->status));
> +	return gve_adminq_parse_err(&priv->pdev->dev, status);
> +}

> +int gve_adminq_describe_device(struct gve_priv *priv)
> +{
> +	struct gve_device_descriptor *descriptor;
> +	union gve_adminq_command cmd;
> +	dma_addr_t descriptor_bus;
> +	int err = 0;
> +	u8 *mac;
> +	u16 mtu;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	descriptor = dma_alloc_coherent(&priv->pdev->dev, PAGE_SIZE,
> +					&descriptor_bus, GFP_KERNEL);
> +	if (!descriptor)
> +		return -ENOMEM;
> +	cmd.opcode = cpu_to_be32(GVE_ADMINQ_DESCRIBE_DEVICE);
> +	cmd.describe_device.device_descriptor_addr =
> +						cpu_to_be64(descriptor_bus);
> +	cmd.describe_device.device_descriptor_version =
> +			cpu_to_be32(GVE_ADMINQ_DEVICE_DESCRIPTOR_VERSION);
> +	cmd.describe_device.available_length = cpu_to_be32(PAGE_SIZE);
> +
> +	err = gve_adminq_execute_cmd(priv, &cmd);
> +	if (err)
> +		goto free_device_descriptor;
> +
> +	mtu = be16_to_cpu(descriptor->mtu);
> +	if (mtu < ETH_MIN_MTU) {
> +		netif_err(priv, drv, priv->dev, "MTU %d below minimum MTU\n",
> +			  mtu);
> +		err = -EINVAL;
> +		goto free_device_descriptor;
> +	}
> +	priv->dev->max_mtu = mtu;
> +	priv->num_event_counters = be16_to_cpu(descriptor->counters);
> +	ether_addr_copy(priv->dev->dev_addr, descriptor->mac);

Since you check MTU you can check the address with
is_valid_ether_addr().

Also it's common practice to copy the provisioned MAC to
netdev->perm_addr as well as dev_addr for VFs.

> +	mac = descriptor->mac;
> +	netif_info(priv, drv, priv->dev, "MAC addr: %pM\n", mac);
> +
> +free_device_descriptor:
> +	dma_free_coherent(&priv->pdev->dev, sizeof(*descriptor), descriptor,
> +			  descriptor_bus);
> +	return err;
> +}

Oh, okay, David already applied..