| 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
| ||
|
Message-ID: <CACGkMEtcm+VeTUKw_DF=bHFpYRUyqOkhh+UEfc+ppUp5zuNVkw@mail.gmail.com>
Date: Wed, 15 Mar 2023 15:05:34 +0800
From: Jason Wang <jasowang@...hat.com>
To: Shannon Nelson <shannon.nelson@....com>
Cc: mst@...hat.com, virtualization@...ts.linux-foundation.org,
brett.creeley@....com, davem@...emloft.net, netdev@...r.kernel.org,
kuba@...nel.org, drivers@...sando.io
Subject: Re: [PATCH RFC v2 virtio 4/7] pds_vdpa: add vdpa config client commands
On Thu, Mar 9, 2023 at 9:31 AM Shannon Nelson <shannon.nelson@....com> wrote:
>
> These are the adminq commands that will be needed for
> setting up and using the vDPA device.
It's better to explain under which case the driver should use adminq,
I see some functions overlap with common configuration capability.
More below.
>
> Signed-off-by: Shannon Nelson <shannon.nelson@....com>
> ---
> drivers/vdpa/pds/Makefile | 1 +
> drivers/vdpa/pds/cmds.c | 207 +++++++++++++++++++++++++++++++++++
> drivers/vdpa/pds/cmds.h | 16 +++
> drivers/vdpa/pds/vdpa_dev.h | 36 +++++-
> include/linux/pds/pds_vdpa.h | 175 +++++++++++++++++++++++++++++
> 5 files changed, 434 insertions(+), 1 deletion(-)
> create mode 100644 drivers/vdpa/pds/cmds.c
> create mode 100644 drivers/vdpa/pds/cmds.h
>
> diff --git a/drivers/vdpa/pds/Makefile b/drivers/vdpa/pds/Makefile
> index ca2efa8c6eb5..7211eba3d942 100644
> --- a/drivers/vdpa/pds/Makefile
> +++ b/drivers/vdpa/pds/Makefile
> @@ -4,6 +4,7 @@
> obj-$(CONFIG_PDS_VDPA) := pds_vdpa.o
>
> pds_vdpa-y := aux_drv.o \
> + cmds.o \
> virtio_pci.o \
> vdpa_dev.o
>
> diff --git a/drivers/vdpa/pds/cmds.c b/drivers/vdpa/pds/cmds.c
> new file mode 100644
> index 000000000000..45410739107c
> --- /dev/null
> +++ b/drivers/vdpa/pds/cmds.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc */
> +
> +#include <linux/vdpa.h>
> +#include <linux/virtio_pci_modern.h>
> +
> +#include <linux/pds/pds_core_if.h>
> +#include <linux/pds/pds_adminq.h>
> +#include <linux/pds/pds_auxbus.h>
> +#include <linux/pds/pds_vdpa.h>
> +
> +#include "vdpa_dev.h"
> +#include "aux_drv.h"
> +#include "cmds.h"
> +
> +int pds_vdpa_init_hw(struct pds_vdpa_device *pdsv)
> +{
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_init_cmd init_cmd = {
> + .opcode = PDS_VDPA_CMD_INIT,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + .len = cpu_to_le32(sizeof(struct virtio_net_config)),
> + .config_pa = 0, /* we use the PCI space, not an alternate space */
> + };
> + struct pds_vdpa_comp init_comp = {0};
> + int err;
> +
> + /* Initialize the vdpa/virtio device */
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&init_cmd,
> + sizeof(init_cmd),
> + (union pds_core_adminq_comp *)&init_comp,
> + 0);
> + if (err)
> + dev_err(dev, "Failed to init hw, status %d: %pe\n",
> + init_comp.status, ERR_PTR(err));
> +
> + return err;
> +}
> +
> +int pds_vdpa_cmd_reset(struct pds_vdpa_device *pdsv)
> +{
This function is not used.
And I wonder what's the difference between reset via adminq and reset
via pds_vdpa_set_status(0) ?
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_cmd cmd = {
> + .opcode = PDS_VDPA_CMD_RESET,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + };
> + struct pds_vdpa_comp comp = {0};
> + int err;
> +
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&cmd,
> + sizeof(cmd),
> + (union pds_core_adminq_comp *)&comp,
> + 0);
> + if (err)
> + dev_err(dev, "Failed to reset hw, status %d: %pe\n",
> + comp.status, ERR_PTR(err));
It might be better to use deb_dbg() here since it can be triggered by the guest.
> +
> + return err;
> +}
> +
> +int pds_vdpa_cmd_set_mac(struct pds_vdpa_device *pdsv, u8 *mac)
> +{
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_setattr_cmd cmd = {
> + .opcode = PDS_VDPA_CMD_SET_ATTR,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + .attr = PDS_VDPA_ATTR_MAC,
> + };
> + struct pds_vdpa_comp comp = {0};
> + int err;
> +
> + ether_addr_copy(cmd.mac, mac);
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&cmd,
> + sizeof(cmd),
> + (union pds_core_adminq_comp *)&comp,
> + 0);
> + if (err)
> + dev_err(dev, "Failed to set mac address %pM, status %d: %pe\n",
> + mac, comp.status, ERR_PTR(err));
> +
> + return err;
> +}
> +
> +int pds_vdpa_cmd_set_max_vq_pairs(struct pds_vdpa_device *pdsv, u16 max_vqp)
> +{
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_setattr_cmd cmd = {
> + .opcode = PDS_VDPA_CMD_SET_ATTR,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + .attr = PDS_VDPA_ATTR_MAX_VQ_PAIRS,
> + .max_vq_pairs = cpu_to_le16(max_vqp),
> + };
> + struct pds_vdpa_comp comp = {0};
> + int err;
> +
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&cmd,
> + sizeof(cmd),
> + (union pds_core_adminq_comp *)&comp,
> + 0);
> + if (err)
> + dev_err(dev, "Failed to set max vq pairs %u, status %d: %pe\n",
> + max_vqp, comp.status, ERR_PTR(err));
> +
> + return err;
> +}
> +
> +int pds_vdpa_cmd_init_vq(struct pds_vdpa_device *pdsv, u16 qid,
> + struct pds_vdpa_vq_info *vq_info)
> +{
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_vq_init_comp comp = {0};
> + struct pds_vdpa_vq_init_cmd cmd = {
> + .opcode = PDS_VDPA_CMD_VQ_INIT,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + .qid = cpu_to_le16(qid),
> + .len = cpu_to_le16(ilog2(vq_info->q_len)),
> + .desc_addr = cpu_to_le64(vq_info->desc_addr),
> + .avail_addr = cpu_to_le64(vq_info->avail_addr),
> + .used_addr = cpu_to_le64(vq_info->used_addr),
> + .intr_index = cpu_to_le16(qid),
> + };
> + int err;
> +
> + dev_dbg(dev, "%s: qid %d len %d desc_addr %#llx avail_addr %#llx used_addr %#llx\n",
> + __func__, qid, ilog2(vq_info->q_len),
> + vq_info->desc_addr, vq_info->avail_addr, vq_info->used_addr);
> +
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&cmd,
> + sizeof(cmd),
> + (union pds_core_adminq_comp *)&comp,
> + 0);
We map common cfg in pds_vdpa_probe_virtio, any reason for using
adminq here? (I guess it might be faster?)
> + if (err) {
> + dev_err(dev, "Failed to init vq %d, status %d: %pe\n",
> + qid, comp.status, ERR_PTR(err));
> + return err;
> + }
> +
> + vq_info->hw_qtype = comp.hw_qtype;
What does hw_qtype mean?
> + vq_info->hw_qindex = le16_to_cpu(comp.hw_qindex);
> +
> + return 0;
> +}
> +
> +int pds_vdpa_cmd_reset_vq(struct pds_vdpa_device *pdsv, u16 qid)
> +{
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_vq_reset_cmd cmd = {
> + .opcode = PDS_VDPA_CMD_VQ_RESET,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + .qid = cpu_to_le16(qid),
> + };
> + struct pds_vdpa_comp comp = {0};
> + int err;
> +
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&cmd,
> + sizeof(cmd),
> + (union pds_core_adminq_comp *)&comp,
> + 0);
> + if (err)
> + dev_err(dev, "Failed to reset vq %d, status %d: %pe\n",
> + qid, comp.status, ERR_PTR(err));
> +
> + return err;
> +}
> +
> +int pds_vdpa_cmd_set_features(struct pds_vdpa_device *pdsv, u64 features)
> +{
> + struct pds_auxiliary_dev *padev = pdsv->vdpa_aux->padev;
> + struct device *dev = &padev->aux_dev.dev;
> + struct pds_vdpa_set_features_cmd cmd = {
> + .opcode = PDS_VDPA_CMD_SET_FEATURES,
> + .vdpa_index = pdsv->vdpa_index,
> + .vf_id = cpu_to_le16(pdsv->vdpa_aux->vf_id),
> + .features = cpu_to_le64(features),
> + };
> + struct pds_vdpa_comp comp = {0};
> + int err;
> +
> + err = padev->ops->adminq_cmd(padev,
> + (union pds_core_adminq_cmd *)&cmd,
> + sizeof(cmd),
> + (union pds_core_adminq_comp *)&comp,
> + 0);
> + if (err)
> + dev_err(dev, "Failed to set features %#llx, status %d: %pe\n",
> + features, comp.status, ERR_PTR(err));
> +
> + return err;
> +}
> diff --git a/drivers/vdpa/pds/cmds.h b/drivers/vdpa/pds/cmds.h
> new file mode 100644
> index 000000000000..72e19f4efde6
> --- /dev/null
> +++ b/drivers/vdpa/pds/cmds.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2023 Advanced Micro Devices, Inc */
> +
> +#ifndef _VDPA_CMDS_H_
> +#define _VDPA_CMDS_H_
> +
> +int pds_vdpa_init_hw(struct pds_vdpa_device *pdsv);
> +
> +int pds_vdpa_cmd_reset(struct pds_vdpa_device *pdsv);
> +int pds_vdpa_cmd_set_mac(struct pds_vdpa_device *pdsv, u8 *mac);
> +int pds_vdpa_cmd_set_max_vq_pairs(struct pds_vdpa_device *pdsv, u16 max_vqp);
> +int pds_vdpa_cmd_init_vq(struct pds_vdpa_device *pdsv, u16 qid,
> + struct pds_vdpa_vq_info *vq_info);
> +int pds_vdpa_cmd_reset_vq(struct pds_vdpa_device *pdsv, u16 qid);
> +int pds_vdpa_cmd_set_features(struct pds_vdpa_device *pdsv, u64 features);
> +#endif /* _VDPA_CMDS_H_ */
> diff --git a/drivers/vdpa/pds/vdpa_dev.h b/drivers/vdpa/pds/vdpa_dev.h
> index 97fab833a0aa..33284ebe538c 100644
> --- a/drivers/vdpa/pds/vdpa_dev.h
> +++ b/drivers/vdpa/pds/vdpa_dev.h
> @@ -4,11 +4,45 @@
> #ifndef _VDPA_DEV_H_
> #define _VDPA_DEV_H_
>
> -#define PDS_VDPA_MAX_QUEUES 65
> +#include <linux/pci.h>
> +#include <linux/vdpa.h>
> +
> +struct pds_vdpa_vq_info {
> + bool ready;
> + u64 desc_addr;
> + u64 avail_addr;
> + u64 used_addr;
> + u32 q_len;
> + u16 qid;
> + int irq;
> + char irq_name[32];
> +
> + void __iomem *notify;
> + dma_addr_t notify_pa;
> +
> + u64 doorbell;
> + u16 avail_idx;
> + u16 used_idx;
> +
> + u8 hw_qtype;
> + u16 hw_qindex;
>
> + struct vdpa_callback event_cb;
> + struct pds_vdpa_device *pdsv;
> +};
> +
> +#define PDS_VDPA_MAX_QUEUES 65
> +#define PDS_VDPA_MAX_QLEN 32768
> struct pds_vdpa_device {
> struct vdpa_device vdpa_dev;
> struct pds_vdpa_aux *vdpa_aux;
> +
> + struct pds_vdpa_vq_info vqs[PDS_VDPA_MAX_QUEUES];
> + u64 req_features; /* features requested by vdpa */
> + u64 actual_features; /* features negotiated and in use */
> + u8 vdpa_index; /* rsvd for future subdevice use */
> + u8 num_vqs; /* num vqs in use */
> + struct vdpa_callback config_cb;
> };
>
> int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux);
> diff --git a/include/linux/pds/pds_vdpa.h b/include/linux/pds/pds_vdpa.h
> index 3f7c08551163..b6a4cb4d3c6b 100644
> --- a/include/linux/pds/pds_vdpa.h
> +++ b/include/linux/pds/pds_vdpa.h
> @@ -101,4 +101,179 @@ struct pds_vdpa_ident_cmd {
> __le32 len;
> __le64 ident_pa;
> };
> +
> +/**
> + * struct pds_vdpa_status_cmd - STATUS_UPDATE command
> + * @opcode: Opcode PDS_VDPA_CMD_STATUS_UPDATE
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @status: new status bits
> + */
> +struct pds_vdpa_status_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + u8 status;
> +};
> +
> +/**
> + * enum pds_vdpa_attr - List of VDPA device attributes
> + * @PDS_VDPA_ATTR_MAC: MAC address
> + * @PDS_VDPA_ATTR_MAX_VQ_PAIRS: Max virtqueue pairs
> + */
> +enum pds_vdpa_attr {
> + PDS_VDPA_ATTR_MAC = 1,
> + PDS_VDPA_ATTR_MAX_VQ_PAIRS = 2,
> +};
> +
> +/**
> + * struct pds_vdpa_setattr_cmd - SET_ATTR command
> + * @opcode: Opcode PDS_VDPA_CMD_SET_ATTR
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @attr: attribute to be changed (enum pds_vdpa_attr)
> + * @pad: Word boundary padding
> + * @mac: new mac address to be assigned as vdpa device address
> + * @max_vq_pairs: new limit of virtqueue pairs
> + */
> +struct pds_vdpa_setattr_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + u8 attr;
> + u8 pad[3];
> + union {
> + u8 mac[6];
> + __le16 max_vq_pairs;
> + } __packed;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_init_cmd - queue init command
> + * @opcode: Opcode PDS_VDPA_CMD_VQ_INIT
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @qid: Queue id (bit0 clear = rx, bit0 set = tx, qid=N is ctrlq)
> + * @len: log(2) of max descriptor count
> + * @desc_addr: DMA address of descriptor area
> + * @avail_addr: DMA address of available descriptors (aka driver area)
> + * @used_addr: DMA address of used descriptors (aka device area)
> + * @intr_index: interrupt index
> + */
> +struct pds_vdpa_vq_init_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le16 qid;
> + __le16 len;
> + __le64 desc_addr;
> + __le64 avail_addr;
> + __le64 used_addr;
> + __le16 intr_index;
Just wonder in which case intr_index can be different from qid, in
pds_vdpa_cmd_init_vq() we had:
.intr_index = cpu_to_le16(qid),
Thanks
> +};
> +
> +/**
> + * struct pds_vdpa_vq_init_comp - queue init completion
> + * @status: Status of the command (enum pds_core_status_code)
> + * @hw_qtype: HW queue type, used in doorbell selection
> + * @hw_qindex: HW queue index, used in doorbell selection
> + * @rsvd: Word boundary padding
> + * @color: Color bit
> + */
> +struct pds_vdpa_vq_init_comp {
> + u8 status;
> + u8 hw_qtype;
> + __le16 hw_qindex;
> + u8 rsvd[11];
> + u8 color;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_reset_cmd - queue reset command
> + * @opcode: Opcode PDS_VDPA_CMD_VQ_RESET
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @qid: Queue id
> + */
> +struct pds_vdpa_vq_reset_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le16 qid;
> +};
> +
> +/**
> + * struct pds_vdpa_set_features_cmd - set hw features
> + * @opcode: Opcode PDS_VDPA_CMD_SET_FEATURES
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @rsvd: Word boundary padding
> + * @features: Feature bit mask
> + */
> +struct pds_vdpa_set_features_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le32 rsvd;
> + __le64 features;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_set_state_cmd - set vq state
> + * @opcode: Opcode PDS_VDPA_CMD_VQ_SET_STATE
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @qid: Queue id
> + * @avail: Device avail index.
> + * @used: Device used index.
> + *
> + * If the virtqueue uses packed descriptor format, then the avail and used
> + * index must have a wrap count. The bits should be arranged like the upper
> + * 16 bits in the device available notification data: 15 bit index, 1 bit wrap.
> + */
> +struct pds_vdpa_vq_set_state_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le16 qid;
> + __le16 avail;
> + __le16 used;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_get_state_cmd - get vq state
> + * @opcode: Opcode PDS_VDPA_CMD_VQ_GET_STATE
> + * @vdpa_index: Index for vdpa subdevice
> + * @vf_id: VF id
> + * @qid: Queue id
> + */
> +struct pds_vdpa_vq_get_state_cmd {
> + u8 opcode;
> + u8 vdpa_index;
> + __le16 vf_id;
> + __le16 qid;
> +};
> +
> +/**
> + * struct pds_vdpa_vq_get_state_comp - get vq state completion
> + * @status: Status of the command (enum pds_core_status_code)
> + * @rsvd0: Word boundary padding
> + * @avail: Device avail index.
> + * @used: Device used index.
> + * @rsvd: Word boundary padding
> + * @color: Color bit
> + *
> + * If the virtqueue uses packed descriptor format, then the avail and used
> + * index will have a wrap count. The bits will be arranged like the "next"
> + * part of device available notification data: 15 bit index, 1 bit wrap.
> + */
> +struct pds_vdpa_vq_get_state_comp {
> + u8 status;
> + u8 rsvd0;
> + __le16 avail;
> + __le16 used;
> + u8 rsvd[9];
> + u8 color;
> +};
> +
> #endif /* _PDS_VDPA_IF_H_ */
> --
> 2.17.1
>
Powered by blists - more mailing lists