Re: [PATCH v2 4/8] qemu:virtio-net: Add a virtqueue for control commands from the guest

[Anthony Liguori <anthony@codemonkey.ws>]

Alex Williamson wrote:
> On Tue, 2009-02-03 at 14:56 -0600, Anthony Liguori wrote:
>   
>> Alex Williamson wrote:
>>     
>>> +        ctrl = elem.out_sg[0].iov_base;
>>> +        status = elem.in_sg[elem.in_num - 1].iov_base;
>>>   
>>>       
>> This works only because we're dealing with uint8_ts but it's broken 
>> elsewhere in virtio-net.  iov_base is guest memory which means it may 
>> have different endianness.  We really should be using ldub_p() here.
>>
>>     
>>> +        *status = VIRTIO_NET_ERR;
>>>   
>>>       
>> And stb_p() here.
>>     
>
> Thanks Anthony.  So before I patch bomb again, I think this turns into
> something like below.  This makes the struct definitions less useful on
> the backend side, but I'm not sure we can do anything about that.  I
> assume for u16s and u32s used in other parts of the interface, we'll
> define those as little endian, which means the guest side driver
> eventually needs a follow-up patch for big endian archs.  Thanks,
>
> Alex
>
>
> qemu:virtio-net: Add a virtqueue for control commands from the guest
>
> This will be used for RX mode, MAC table, VLAN table control, etc...
>
> The control transaction consists of one or more "out" sg entries and
> one or more "in" sg entries.  The first out entry contains a header
> defining the class and command.  Additional out entries may provide
> data for the command.  A response via the ack entry is required
> and the guest will typically be waiting for it.
>
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
>
>  hw/virtio-net.c |   34 +++++++++++++++++++++++++++++++++-
>  hw/virtio-net.h |   18 ++++++++++++++++++
>  2 files changed, 51 insertions(+), 1 deletions(-)
>
>
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index 073f23a..7971f95 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -25,6 +25,7 @@ typedef struct VirtIONet
>      uint16_t status;
>      VirtQueue *rx_vq;
>      VirtQueue *tx_vq;
> +    VirtQueue *ctrl_vq;
>      VLANClientState *vc;
>      QEMUTimer *tx_timer;
>      int tx_timer_active;
> @@ -79,7 +80,9 @@ static void virtio_net_set_link_status(VLANClientState *vc)
>  
>  static uint32_t virtio_net_get_features(VirtIODevice *vdev)
>  {
> -    uint32_t features = (1 << VIRTIO_NET_F_MAC) | (1 << VIRTIO_NET_F_STATUS);
> +    uint32_t features = (1 << VIRTIO_NET_F_MAC) |
> +                        (1 << VIRTIO_NET_F_STATUS) |
> +                        (1 << VIRTIO_NET_F_CTRL_VQ);
>  
>      return features;
>  }
> @@ -91,6 +94,34 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>      n->mergeable_rx_bufs = !!(features & (1 << VIRTIO_NET_F_MRG_RXBUF));
>  }
>  
> +static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    struct virtio_net_ctrl_hdr ctrl;
> +    virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
> +    VirtQueueElement elem;
> +
> +    while (virtqueue_pop(vq, &elem)) {
> +        if ((elem.in_num < 1) | (elem.out_num < 1)) {
> +            fprintf(stderr, "virtio-net ctrl missing headers\n");
> +            exit(1);
> +        }
> +
> +        if (elem.out_sg[0].iov_len < sizeof(ctrl) ||
> +            elem.out_sg[elem.in_num - 1].iov_len < sizeof(status)) {
> +            fprintf(stderr, "virtio-net ctrl header not in correct element\n");
> +            exit(1);
> +        }
> +
> +        ctrl.class = ldub_p(elem.out_sg[0].iov_base);
> +        ctrl.cmd = ldub_p(elem.out_sg[0].iov_base + sizeof(ctrl.class));
> +
> +        stb_p(elem.in_sg[elem.in_num - 1].iov_base, status);
> +
> +        virtqueue_push(vq, &elem, sizeof(status));
> +        virtio_notify(vdev, vq);
> +    }
> +}
>
>   

Yup, this looks right to me.

Regards,

Anthony Liguori