Linux Virtualization - Apr 2023 - [PATCH resend 1/2] vdpa/snet: support getting and setting VQ state

On Sun, Apr 2, 2023 at 8:52?PM Alvaro Karsz <alvaro.karsz at
solid-run.com> wrote:
>
> This patch adds the get_vq_state and set_vq_state vDPA callbacks.
>
> In order to get the VQ state, the state needs to be read from the DPU.
> In order to allow that, the old messaging mechanism is replaced with a new,
> flexible control mechanism.
> This mechanism allows to read data from the DPU.
>
> The mechanism can be used if the negotiated config version is 2 or
> higher.
>
> If the new mechanism is used when the config version is 1, it will call
> snet_send_ctrl_msg_old, which is config 1 compatible.
>
> Signed-off-by: Alvaro Karsz <alvaro.karsz at solid-run.com>
> ---
>  drivers/vdpa/solidrun/Makefile     |   1 +
>  drivers/vdpa/solidrun/snet_ctrl.c  | 318 +++++++++++++++++++++++++++++
>  drivers/vdpa/solidrun/snet_hwmon.c |   2 +-
>  drivers/vdpa/solidrun/snet_main.c  | 111 ++++------
>  drivers/vdpa/solidrun/snet_vdpa.h  |  17 +-
>  5 files changed, 378 insertions(+), 71 deletions(-)
>  create mode 100644 drivers/vdpa/solidrun/snet_ctrl.c
>
> diff --git a/drivers/vdpa/solidrun/Makefile
b/drivers/vdpa/solidrun/Makefile
> index c0aa3415bf7..9116252cd5f 100644
> --- a/drivers/vdpa/solidrun/Makefile
> +++ b/drivers/vdpa/solidrun/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_SNET_VDPA) += snet_vdpa.o
>  snet_vdpa-$(CONFIG_SNET_VDPA) += snet_main.o
> +snet_vdpa-$(CONFIG_SNET_VDPA) += snet_ctrl.o
>  ifdef CONFIG_HWMON
>  snet_vdpa-$(CONFIG_SNET_VDPA) += snet_hwmon.o
>  endif
> diff --git a/drivers/vdpa/solidrun/snet_ctrl.c
b/drivers/vdpa/solidrun/snet_ctrl.c
> new file mode 100644
> index 00000000000..54909549a00
> --- /dev/null
> +++ b/drivers/vdpa/solidrun/snet_ctrl.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * SolidRun DPU driver for control plane
> + *
> + * Copyright (C) 2022-2023 SolidRun
> + *
> + * Author: Alvaro Karsz <alvaro.karsz at solid-run.com>
> + *
> + */
> +
> +#include <linux/iopoll.h>
> +
> +#include "snet_vdpa.h"
> +
> +enum snet_ctrl_opcodes {
> +       SNET_CTRL_OP_DESTROY = 1,
> +       SNET_CTRL_OP_READ_VQ_STATE,
> +};
> +
> +#define SNET_CTRL_TIMEOUT              2000000
> +
> +#define SNET_CTRL_DATA_SIZE_MASK       0x0000FFFF
> +#define SNET_CTRL_IN_PROCESS_MASK      0x00010000
> +#define SNET_CTRL_CHUNK_RDY_MASK       0x00020000
> +#define SNET_CTRL_ERROR_MASK           0x0FFC0000
> +
> +#define SNET_VAL_TO_ERR(val)           (-(((val) &
SNET_CTRL_ERROR_MASK) >> 18))
> +#define SNET_EMPTY_CTRL(val)           (((val) & SNET_CTRL_ERROR_MASK)
|| \
> +                                               !((val) &
SNET_CTRL_IN_PROCESS_MASK))
> +#define SNET_DATA_READY(val)           ((val) & (SNET_CTRL_ERROR_MASK
| SNET_CTRL_CHUNK_RDY_MASK))
> +
> +/* Control register used to read data from the DPU */
> +struct snet_ctrl_reg_ctrl {
> +       /* Chunk size in 4B words */
> +       u16 data_size;
> +       /* We are in the middle of a command */
> +       u16 in_process:1;
> +       /* A data chunk is ready and can be consumed */
> +       u16 chunk_ready:1;
> +       /* Error code */
> +       u16 error:10;
> +       /* Saved for future usage */
> +       u16 rsvd:4;
> +};
> +
> +/* Opcode register */
> +struct snet_ctrl_reg_op {
> +       u16 opcode;
> +       /* Only if VQ index is relevant for the command */
> +       u16 vq_idx;
> +};
> +
> +struct snet_ctrl_regs {
> +       struct snet_ctrl_reg_op op;
> +       struct snet_ctrl_reg_ctrl ctrl;
> +       u32 rsvd;
> +       u32 data[];
> +};
> +
> +static struct snet_ctrl_regs __iomem *snet_get_ctrl(struct snet *snet)
> +{
> +       return snet->bar + snet->psnet->cfg.ctrl_off;
> +}
> +
> +static int snet_wait_for_empty_ctrl(struct snet_ctrl_regs __iomem *regs)
> +{
> +       u32 val;
> +
> +       return readx_poll_timeout(ioread32, &regs->ctrl, val,
SNET_EMPTY_CTRL(val), 10,
> +                                 SNET_CTRL_TIMEOUT);
> +}
> +
> +static int snet_wait_for_empty_op(struct snet_ctrl_regs __iomem *regs)
> +{
> +       u32 val;
> +
> +       return readx_poll_timeout(ioread32, &regs->op, val, !val,
10, SNET_CTRL_TIMEOUT);
> +}
> +
> +static int snet_wait_for_data(struct snet_ctrl_regs __iomem *regs)
> +{
> +       u32 val;
> +
> +       return readx_poll_timeout(ioread32, &regs->ctrl, val,
SNET_DATA_READY(val), 10,
> +                                 SNET_CTRL_TIMEOUT);
> +}
> +
> +static u32 snet_read32_word(struct snet_ctrl_regs __iomem *ctrl_regs, u16
word_idx)
> +{
> +       return ioread32(&ctrl_regs->data[word_idx]);
> +}
> +
> +static u32 snet_read_ctrl(struct snet_ctrl_regs __iomem *ctrl_regs)
> +{
> +       return ioread32(&ctrl_regs->ctrl);
> +}
> +
> +static void snet_write_ctrl(struct snet_ctrl_regs __iomem *ctrl_regs, u32
val)
> +{
> +       iowrite32(val, &ctrl_regs->ctrl);
> +}
> +
> +static void snet_write_op(struct snet_ctrl_regs __iomem *ctrl_regs, u32
val)
> +{
> +       iowrite32(val, &ctrl_regs->op);
> +}
> +
> +static int snet_wait_for_dpu_completion(struct snet_ctrl_regs __iomem
*ctrl_regs)
> +{
> +       /* Wait until the DPU finishes completely.
> +        * It will clear the opcode register.
> +        */
> +       return snet_wait_for_empty_op(ctrl_regs);
> +}
> +
> +/* Reading ctrl from the DPU:
> + * buf_size must be 4B aligned
> + *
> + * Steps:
> + *
> + * (1) Verify that the DPU is not in the middle of another operation by
> + *     reading the in_process and error bits in the control register.
> + * (2) Write the request opcode and the VQ idx in the opcode register
> + *     and write the buffer size in the control register.
> + * (3) Start readind chunks of data, chunk_ready bit indicates that a
> + *     data chunk is available, we signal that we read the data by
clearing the bit.
> + * (4) Detect that the transfer is completed when the in_process bit
> + *     in the control register is cleared or when the an error appears.
> + */
> +static int snet_ctrl_read_from_dpu(struct snet *snet, u16 opcode, u16
vq_idx, void *buffer,
> +                                  u32 buf_size)
> +{
> +       struct pci_dev *pdev = snet->pdev;
> +       struct snet_ctrl_regs __iomem *regs = snet_get_ctrl(snet);
> +       u32 *bfr_ptr = (u32 *)buffer;
> +       u32 val;
> +       u16 buf_words;
> +       int ret;
> +       u16 words, i, tot_words = 0;
> +
> +       /* Supported for config 2+ */
> +       if (!SNET_CFG_VER(snet, 2))
> +               return -EOPNOTSUPP;
> +
> +       if (!IS_ALIGNED(buf_size, 4))
> +               return -EINVAL;
> +
> +       mutex_lock(&snet->ctrl_lock);
> +
> +       buf_words = buf_size / 4;
> +
> +       /* Make sure control register is empty */
> +       ret = snet_wait_for_empty_ctrl(regs);
> +       if (ret) {
> +               SNET_WARN(pdev, "Timeout waiting for previous control
data to be consumed\n");
> +               goto exit;
> +       }
> +
> +       /* Overwrite the control register with the new buffer size (in 4B
words) */
> +       snet_write_ctrl(regs, buf_words);
> +       /* Use a memory barrier, this must be written before the opcode
register. */
> +       wmb();

At least you need to use smp_wmb() but if you want to serialize MMIO
writes you can simply use spinlocks after the removal of mmiowb
work[1].

Note that Documentation/memory-barriers.txt said:

        1. All readX() and writeX() accesses to the same peripheral are ordered
           with respect to each other. This ensures that MMIO register accesses
           by the same CPU thread to a particular device will arrive in program
           order.

[1] https://lwn.net/Articles/780710/

> +
> +       /* Write opcode and VQ idx */
> +       val = opcode | (vq_idx << 16);
> +       snet_write_op(regs, val);
> +
> +       while (buf_words != tot_words) {
> +               ret = snet_wait_for_data(regs);
> +               if (ret) {
> +                       SNET_WARN(pdev, "Timeout waiting for control
data\n");
> +                       goto exit;
> +               }
> +
> +               val = snet_read_ctrl(regs);
> +
> +               /* Error? */
> +               if (val & SNET_CTRL_ERROR_MASK) {
> +                       ret = SNET_VAL_TO_ERR(val);
> +                       SNET_WARN(pdev, "Error while reading control
data from DPU, err %d\n", ret);
> +                       goto exit;
> +               }
> +
> +               words = min_t(u16, val & SNET_CTRL_DATA_SIZE_MASK,
buf_words - tot_words);
> +
> +               for (i = 0; i < words; i++) {
> +                       *bfr_ptr = snet_read32_word(regs, i);
> +                       bfr_ptr++;
> +               }
> +
> +               tot_words += words;
> +
> +               /* Is the job completed? */
> +               if (!(val & SNET_CTRL_IN_PROCESS_MASK))
> +                       break;
> +
> +               /* Clear the chunk ready bit and continue */
> +               val &= ~SNET_CTRL_CHUNK_RDY_MASK;
> +               snet_write_ctrl(regs, val);
> +       }
> +
> +       ret = snet_wait_for_dpu_completion(regs);
> +       if (ret)
> +               SNET_WARN(pdev, "Timeout waiting for the DPU to
complete a control command\n");
> +
> +exit:
> +       mutex_unlock(&snet->ctrl_lock);
> +       return ret;
> +}
> +
> +/* Send a control message to the DPU using the old mechanism
> + * used with config version 1.
> + */
> +static int snet_send_ctrl_msg_old(struct snet *snet, u32 opcode)
> +{
> +       struct pci_dev *pdev = snet->pdev;
> +       struct snet_ctrl_regs __iomem *regs = snet_get_ctrl(snet);
> +       int ret;
> +
> +       mutex_lock(&snet->ctrl_lock);
> +
> +       /* Old mechanism uses just 1 register, the opcode register.
> +        * Make sure that the opcode register is empty, and that the DPU
isn't
> +        * processing an old message.
> +        */
> +       ret = snet_wait_for_empty_op(regs);
> +       if (ret) {
> +               SNET_WARN(pdev, "Timeout waiting for previous control
message to be ACKed\n");
> +               goto exit;
> +       }
> +
> +       /* Write the message */
> +       snet_write_op(regs, opcode);
> +
> +       /* DPU ACKs the message by clearing the opcode register */
> +       ret = snet_wait_for_empty_op(regs);
> +       if (ret)
> +               SNET_WARN(pdev, "Timeout waiting for a control message
to be ACKed\n");
> +
> +exit:
> +       mutex_unlock(&snet->ctrl_lock);
> +       return ret;
> +}
> +
> +/* Send a control message to the DPU.
> + * A control message is a message without payload.
> + */
> +static int snet_send_ctrl_msg(struct snet *snet, u16 opcode, u16 vq_idx)
> +{
> +       struct pci_dev *pdev = snet->pdev;
> +       struct snet_ctrl_regs __iomem *regs = snet_get_ctrl(snet);
> +       u32 val;
> +       int ret;
> +
> +       /* If config version is not 2+, use the old mechanism */
> +       if (!SNET_CFG_VER(snet, 2))
> +               return snet_send_ctrl_msg_old(snet, opcode);
> +
> +       mutex_lock(&snet->ctrl_lock);
> +
> +       /* Make sure control register is empty */
> +       ret = snet_wait_for_empty_ctrl(regs);
> +       if (ret) {
> +               SNET_WARN(pdev, "Timeout waiting for previous control
data to be consumed\n");
> +               goto exit;
> +       }
> +
> +       /* Clear the control register - clear the error code if previous
control operation failed */
> +       snet_write_ctrl(regs, 0);
> +
> +       /* Write opcode and VQ idx */
> +       val = opcode | (vq_idx << 16);
> +       snet_write_op(regs, val);
I guess we need to serialize two writes here as well.

Thanks

Hi Jason,
> > +       /* Overwrite the control register with the new buffer size (in
4B words) */
> > +       snet_write_ctrl(regs, buf_words);
> > +       /* Use a memory barrier, this must be written before the
opcode register. */
> > +       wmb();
> 
> 
> At least you need to use smp_wmb() but if you want to serialize MMIO
> writes you can simply use spinlocks after the removal of mmiowb
> work[1].
> 
I'm not sure how a spinlock can help in this case.
The entire control mechanism is protected by the spinlock snet->ctrl_lock, so
multiple CPUs won't use it simultaneously.
> Note that Documentation/memory-barriers.txt said:
> 
>         1. All readX() and writeX() accesses to the same peripheral are
ordered
>            with respect to each other. This ensures that MMIO register
accesses
>            by the same CPU thread to a particular device will arrive in
program
>            order.
> 
So it will arrive to the pci subsystem in program order, but the pci subsystem
may reorder, right?
I can just issue a read after the writes, something like:

write_control
read_control
write op
read op

What do you think?
> > +       /* Clear the control register - clear the error code if
previous control operation failed */
> > +       snet_write_ctrl(regs, 0);
> > +
> > +       /* Write opcode and VQ idx */
> > +       val = opcode | (vq_idx << 16);
> > +       snet_write_op(regs, val);
> 
> I guess we need to serialize two writes here as well.
Same here.

Thanks