| 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: <cde38f74-66da-7eb0-c933-d4848bd17bc1@amd.com>
Date: Wed, 15 Mar 2023 20:25:13 -0700
From: Shannon Nelson <shannon.nelson@....com>
To: Jason Wang <jasowang@...hat.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 3/15/23 12:05 AM, Jason Wang wrote:
> 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.
Yes, I agree this needs to be more clearly stated. The overlap is
because the original FW didn't have the virtio device as well modeled
and we had to go through adminq calls to get things done. Now that we
have a reasonable virtio emulation and can use the virtio_net_config, we
have a lot less need for the adminq calls.
>
>>
>> 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) ?
Ideally no difference.
>
>> + 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.
Sure.
>
>> +
>> + 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?)
It's just easier to hand the values to the FW in a single package and
let it sort things out as it needs, and it will complain with a handy
error code if necessary.
>
>> + 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?
Hmmm... this and hw_qindex are hardware specific values that I don't
thinkg we need any longer. I'll pull them out.
>
>> + 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),
Yes, it normally is going to be the same. The FW allows us to specify
it separate from the qid in order to allow flexibility in setting up
interrupts when we want to experiment with it. For now we just plug in
the number.
sln
>
> 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