Same way as with the MAC, restore the expected number of queues at
device's start.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 1e0dbfcced..96fd3bc835 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
     return 0;
 }
 
+static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
+                                  const VirtIONet *n)
+{
+    uint64_t features = n->parent_obj.guest_features;
+    ssize_t dev_written;
+    void *cursor = s->cvq_cmd_out_buffer;
+    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
+        return 0;
+    }
+
+    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
+        .class = VIRTIO_NET_CTRL_MQ,
+        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
+    };
+    cursor += sizeof(struct virtio_net_ctrl_hdr);
+    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
+        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
+    };
+    cursor += sizeof(struct virtio_net_ctrl_mq);
+
+    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
+                                             sizeof(virtio_net_ctrl_ack));
+    if (unlikely(dev_written < 0)) {
+        return dev_written;
+    }
+
+    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
+}
+
 static int vhost_vdpa_net_load(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -409,6 +438,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
     if (unlikely(r < 0)) {
         return r;
     }
+    r = vhost_vdpa_net_load_mq(s, n);
+    if (unlikely(r)) {
+        return r;
+    }
 
     return 0;
 }
-- 
2.31.1

在 2022/8/20 01:13, Eugenio Pérez 写道:
> Same way as with the MAC, restore the expected number of queues at
> device's start.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 1e0dbfcced..96fd3bc835 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
>       return 0;
>   }
>   
> +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> +                                  const VirtIONet *n)
> +{
> +    uint64_t features = n->parent_obj.guest_features;
> +    ssize_t dev_written;
> +    void *cursor = s->cvq_cmd_out_buffer;
> +    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> +        return 0;
> +    }
> +
> +    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> +        .class = VIRTIO_NET_CTRL_MQ,
> +        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> +    };
> +    cursor += sizeof(struct virtio_net_ctrl_hdr);
> +    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> +        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> +    };


Such casting is not elegant, let's just prepare buffer and then do the 
copy inside vhost_vdpa_net_cvq_add()?


> +    cursor += sizeof(struct virtio_net_ctrl_mq);
> +
> +    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> +                                             sizeof(virtio_net_ctrl_ack));
> +    if (unlikely(dev_written < 0)) {
> +        return dev_written;
> +    }
> +
> +    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;


So I think we should have a dedicated buffer just for ack, then there's 
no need for such casting.

Thanks


> +}
> +
>   static int vhost_vdpa_net_load(NetClientState *nc)
>   {
>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -409,6 +438,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>       if (unlikely(r < 0)) {
>           return r;
>       }
> +    r = vhost_vdpa_net_load_mq(s, n);
> +    if (unlikely(r)) {
> +        return r;
> +    }
>   
>       return 0;
>   }

On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > Same way as with the MAC, restore the expected number of queues at
> > device's start.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 1e0dbfcced..96fd3bc835 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> >       return 0;
> >   }
> >
> > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > +                                  const VirtIONet *n)
> > +{
> > +    uint64_t features = n->parent_obj.guest_features;
> > +    ssize_t dev_written;
> > +    void *cursor = s->cvq_cmd_out_buffer;
> > +    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > +        return 0;
> > +    }
> > +
> > +    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > +        .class = VIRTIO_NET_CTRL_MQ,
> > +        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > +    };
> > +    cursor += sizeof(struct virtio_net_ctrl_hdr);
> > +    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > +        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > +    };
>
>
> Such casting is not elegant, let's just prepare buffer and then do the
> copy inside vhost_vdpa_net_cvq_add()?
>

I'm not sure what you propose here. I can pre-fill a buffer in the
stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
compiler should be able to optimize it, but I'm not sure if it
simplifies the code.

We can have a dedicated buffer for mac, another for mq, and one for
each different command, and map all of them at the device's start. But
this seems too much overhead to me.

Some alternatives that come to my mind:

* Declare a struct with both virtio_net_ctrl_hdr and each of the
control commands (using unions?), and cast s->cvq_cmd_out_buffer
accordingly.
* Declare a struct with all of the supported commands one after
another, and let qemu fill and send these accordingly.

>
> > +    cursor += sizeof(struct virtio_net_ctrl_mq);
> > +
> > +    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > +                                             sizeof(virtio_net_ctrl_ack));
> > +    if (unlikely(dev_written < 0)) {
> > +        return dev_written;
> > +    }
> > +
> > +    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
>
>
> So I think we should have a dedicated buffer just for ack, then there's
> no need for such casting.
>

You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
directly and map it to the device?

Thanks!

On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > Same way as with the MAC, restore the expected number of queues at
> > > device's start.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > >   1 file changed, 33 insertions(+)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 1e0dbfcced..96fd3bc835 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > >       return 0;
> > >   }
> > >
> > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > +                                  const VirtIONet *n)
> > > +{
> > > +    uint64_t features = n->parent_obj.guest_features;
> > > +    ssize_t dev_written;
> > > +    void *cursor = s->cvq_cmd_out_buffer;
> > > +    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > +        .class = VIRTIO_NET_CTRL_MQ,
> > > +        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > +    };
> > > +    cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > +    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > +        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > +    };
> >
> >
> > Such casting is not elegant, let's just prepare buffer and then do the
> > copy inside vhost_vdpa_net_cvq_add()?
> >
>
> I'm not sure what you propose here. I can pre-fill a buffer in the
> stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> compiler should be able to optimize it, but I'm not sure if it
> simplifies the code.
>
> We can have a dedicated buffer for mac, another for mq, and one for
> each different command, and map all of them at the device's start. But
> this seems too much overhead to me.

Considering we may need to support and restore a lot of other fields,
this looks a little complicated.

I meant the caller can simply do:

struct virtio_net_ctrl_mq mq = { ...};

Then we do

vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);

Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
cmd_out_buffer etc from the caller.

>
> Some alternatives that come to my mind:
>
> * Declare a struct with both virtio_net_ctrl_hdr and each of the
> control commands (using unions?), and cast s->cvq_cmd_out_buffer
> accordingly.
> * Declare a struct with all of the supported commands one after
> another, and let qemu fill and send these accordingly.
>
> >
> > > +    cursor += sizeof(struct virtio_net_ctrl_mq);
> > > +
> > > +    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > +                                             sizeof(virtio_net_ctrl_ack));
> > > +    if (unlikely(dev_written < 0)) {
> > > +        return dev_written;
> > > +    }
> > > +
> > > +    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> >
> >
> > So I think we should have a dedicated buffer just for ack, then there's
> > no need for such casting.
> >
>
> You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> directly and map it to the device?

Kind of, considering the ack is the only kind of structure in the near
future, can we simply use the structure virtio_net_ctl_ack?

Thanks

>
> Thanks!
>

On Wed, Aug 24, 2022 at 10:52 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > >
> > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > Same way as with the MAC, restore the expected number of queues at
> > > > device's start.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > >   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > >   1 file changed, 33 insertions(+)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 1e0dbfcced..96fd3bc835 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > >       return 0;
> > > >   }
> > > >
> > > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > > +                                  const VirtIONet *n)
> > > > +{
> > > > +    uint64_t features = n->parent_obj.guest_features;
> > > > +    ssize_t dev_written;
> > > > +    void *cursor = s->cvq_cmd_out_buffer;
> > > > +    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > > +        .class = VIRTIO_NET_CTRL_MQ,
> > > > +        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > > +    };
> > > > +    cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > > +    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > > +        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > > +    };
> > >
> > >
> > > Such casting is not elegant, let's just prepare buffer and then do the
> > > copy inside vhost_vdpa_net_cvq_add()?
> > >
> >
> > I'm not sure what you propose here. I can pre-fill a buffer in the
> > stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> > compiler should be able to optimize it, but I'm not sure if it
> > simplifies the code.
> >
> > We can have a dedicated buffer for mac, another for mq, and one for
> > each different command, and map all of them at the device's start. But
> > this seems too much overhead to me.
>
> Considering we may need to support and restore a lot of other fields,
> this looks a little complicated.
>
> I meant the caller can simply do:
>
> struct virtio_net_ctrl_mq mq = { ...};
>
> Then we do
>
> vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
>
> Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
> cmd_out_buffer etc from the caller.
>

We need to add the ctrl header too. But yes, that is feasible, something like:

vhost_vdpa_net_cvq_add(&ctrl, &mq, sizeof(mq), ...);

> >
> > Some alternatives that come to my mind:
> >
> > * Declare a struct with both virtio_net_ctrl_hdr and each of the
> > control commands (using unions?), and cast s->cvq_cmd_out_buffer
> > accordingly.
> > * Declare a struct with all of the supported commands one after
> > another, and let qemu fill and send these accordingly.
> >
> > >
> > > > +    cursor += sizeof(struct virtio_net_ctrl_mq);
> > > > +
> > > > +    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > > +                                             sizeof(virtio_net_ctrl_ack));
> > > > +    if (unlikely(dev_written < 0)) {
> > > > +        return dev_written;
> > > > +    }
> > > > +
> > > > +    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> > >
> > >
> > > So I think we should have a dedicated buffer just for ack, then there's
> > > no need for such casting.
> > >
> >
> > You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> > directly and map it to the device?
>
> Kind of, considering the ack is the only kind of structure in the near
> future, can we simply use the structure virtio_net_ctl_ack?
>

Almost, but we need to map to the device in a page size. And I think
it's better to allocate a whole page for that, so it does not share
memory with qemu.

Other than that, yes, I think it can be declared as virtio_net_ctl_ack directly.

Thanks!

On Wed, Aug 24, 2022 at 5:06 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:52 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > > Same way as with the MAC, restore the expected number of queues at
> > > > > device's start.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > > >   1 file changed, 33 insertions(+)
> > > > >
> > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > index 1e0dbfcced..96fd3bc835 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > > >       return 0;
> > > > >   }
> > > > >
> > > > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > > > +                                  const VirtIONet *n)
> > > > > +{
> > > > > +    uint64_t features = n->parent_obj.guest_features;
> > > > > +    ssize_t dev_written;
> > > > > +    void *cursor = s->cvq_cmd_out_buffer;
> > > > > +    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > > > +        return 0;
> > > > > +    }
> > > > > +
> > > > > +    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > > > +        .class = VIRTIO_NET_CTRL_MQ,
> > > > > +        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > > > +    };
> > > > > +    cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > > > +    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > > > +        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > > > +    };
> > > >
> > > >
> > > > Such casting is not elegant, let's just prepare buffer and then do the
> > > > copy inside vhost_vdpa_net_cvq_add()?
> > > >
> > >
> > > I'm not sure what you propose here. I can pre-fill a buffer in the
> > > stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> > > compiler should be able to optimize it, but I'm not sure if it
> > > simplifies the code.
> > >
> > > We can have a dedicated buffer for mac, another for mq, and one for
> > > each different command, and map all of them at the device's start. But
> > > this seems too much overhead to me.
> >
> > Considering we may need to support and restore a lot of other fields,
> > this looks a little complicated.
> >
> > I meant the caller can simply do:
> >
> > struct virtio_net_ctrl_mq mq = { ...};
> >
> > Then we do
> >
> > vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
> >
> > Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
> > cmd_out_buffer etc from the caller.
> >
>
> We need to add the ctrl header too. But yes, that is feasible, something like:
>
> vhost_vdpa_net_cvq_add(&ctrl, &mq, sizeof(mq), ...);
>
> > >
> > > Some alternatives that come to my mind:
> > >
> > > * Declare a struct with both virtio_net_ctrl_hdr and each of the
> > > control commands (using unions?), and cast s->cvq_cmd_out_buffer
> > > accordingly.
> > > * Declare a struct with all of the supported commands one after
> > > another, and let qemu fill and send these accordingly.
> > >
> > > >
> > > > > +    cursor += sizeof(struct virtio_net_ctrl_mq);
> > > > > +
> > > > > +    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > > > +                                             sizeof(virtio_net_ctrl_ack));
> > > > > +    if (unlikely(dev_written < 0)) {
> > > > > +        return dev_written;
> > > > > +    }
> > > > > +
> > > > > +    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> > > >
> > > >
> > > > So I think we should have a dedicated buffer just for ack, then there's
> > > > no need for such casting.
> > > >
> > >
> > > You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> > > directly and map it to the device?
> >
> > Kind of, considering the ack is the only kind of structure in the near
> > future, can we simply use the structure virtio_net_ctl_ack?
> >
>
> Almost, but we need to map to the device in a page size. And I think
> it's better to allocate a whole page for that, so it does not share
> memory with qemu.

I guess using a union will solve the problem?

Thanks

>
> Other than that, yes, I think it can be declared as virtio_net_ctl_ack directly.
>
> Thanks!
>

On Wed, Aug 24, 2022 at 11:08 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 5:06 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Aug 24, 2022 at 10:52 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > >
> > > > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > > > Same way as with the MAC, restore the expected number of queues at
> > > > > > device's start.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > >   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > > > >   1 file changed, 33 insertions(+)
> > > > > >
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index 1e0dbfcced..96fd3bc835 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > > > >       return 0;
> > > > > >   }
> > > > > >
> > > > > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > > > > +                                  const VirtIONet *n)
> > > > > > +{
> > > > > > +    uint64_t features = n->parent_obj.guest_features;
> > > > > > +    ssize_t dev_written;
> > > > > > +    void *cursor = s->cvq_cmd_out_buffer;
> > > > > > +    if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > > > > +        return 0;
> > > > > > +    }
> > > > > > +
> > > > > > +    *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > > > > +        .class = VIRTIO_NET_CTRL_MQ,
> > > > > > +        .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > > > > +    };
> > > > > > +    cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > > > > +    *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > > > > +        .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > > > > +    };
> > > > >
> > > > >
> > > > > Such casting is not elegant, let's just prepare buffer and then do the
> > > > > copy inside vhost_vdpa_net_cvq_add()?
> > > > >
> > > >
> > > > I'm not sure what you propose here. I can pre-fill a buffer in the
> > > > stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> > > > compiler should be able to optimize it, but I'm not sure if it
> > > > simplifies the code.
> > > >
> > > > We can have a dedicated buffer for mac, another for mq, and one for
> > > > each different command, and map all of them at the device's start. But
> > > > this seems too much overhead to me.
> > >
> > > Considering we may need to support and restore a lot of other fields,
> > > this looks a little complicated.
> > >
> > > I meant the caller can simply do:
> > >
> > > struct virtio_net_ctrl_mq mq = { ...};
> > >
> > > Then we do
> > >
> > > vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
> > >
> > > Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
> > > cmd_out_buffer etc from the caller.
> > >
> >
> > We need to add the ctrl header too. But yes, that is feasible, something like:
> >
> > vhost_vdpa_net_cvq_add(&ctrl, &mq, sizeof(mq), ...);
> >
> > > >
> > > > Some alternatives that come to my mind:
> > > >
> > > > * Declare a struct with both virtio_net_ctrl_hdr and each of the
> > > > control commands (using unions?), and cast s->cvq_cmd_out_buffer
> > > > accordingly.
> > > > * Declare a struct with all of the supported commands one after
> > > > another, and let qemu fill and send these accordingly.
> > > >
> > > > >
> > > > > > +    cursor += sizeof(struct virtio_net_ctrl_mq);
> > > > > > +
> > > > > > +    dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > > > > +                                             sizeof(virtio_net_ctrl_ack));
> > > > > > +    if (unlikely(dev_written < 0)) {
> > > > > > +        return dev_written;
> > > > > > +    }
> > > > > > +
> > > > > > +    return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> > > > >
> > > > >
> > > > > So I think we should have a dedicated buffer just for ack, then there's
> > > > > no need for such casting.
> > > > >
> > > >
> > > > You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> > > > directly and map it to the device?
> > >
> > > Kind of, considering the ack is the only kind of structure in the near
> > > future, can we simply use the structure virtio_net_ctl_ack?
> > >
> >
> > Almost, but we need to map to the device in a page size. And I think
> > it's better to allocate a whole page for that, so it does not share
> > memory with qemu.
>
> I guess using a union will solve the problem?
>

It was more a nitpick than a problem, pointing out the need to
allocate a whole page casting it or not to virtio_net_ctrl_ack. In
other words, we must init status as "status =
g_malloc0(real_host_page_size())", not "g_malloc0(sizeof(*status))".

But I think the union is a good idea. The problem is that
qemu_real_host_page_size is not a constant so we cannot declare a type
with that.

> Thanks
>
> >
> > Other than that, yes, I think it can be declared as virtio_net_ctl_ack directly.
> >
> > Thanks!
> >
>