Re: [Qemu-devel] [PATCH 1/5] CAN bus simple messages transport implementation for QEMU

[Philippe Mathieu-Daudé]

Hi Pavel,

On 11/01/2017 10:00 PM, address@hidden wrote:
> From: Pavel Pisa <address@hidden>
> 
> The CanBusState state structure is created for each
> emulated CAN channel. Individual clients/emulated
> CAN interfaces or host interface connection registers
> to the bus by CanBusClientState structure.
> 
> Connection to the real host CAN bus network through
> SocketCAN network interface is available only for Linux
> host system. Mechanism is generic so more options
> can follow.
> 
> Implementation is as simple as possible, no migration,
> messages prioritization and queuing considered for now.
> But it is intended to be extended when need arises.
> 
> Development repository and more documentation at
> 
> https://gitlab.fel.cvut.cz/canbus/qemu-canbus
> 
> The work is based on Jin Yang GSoC 2013 work funded
> by Google and mentored in frame of RTEMS project GSoC
> slot donated to QEMU.
> 
> Rewritten for QEMU-2.0+ versions and architecture cleanup
> by Pavel Pisa (Czech Technical University in Prague).
> 
> Signed-off-by: Pavel Pisa <address@hidden>
> ---

Please document here (after the '---') the changes since your previous
series (this one being v2), i.e.:

Since v2:
- separate CAN bus emulation core functions and connection to Linux
SocketCAN host (as suggested by Frederic Konrad).

>  default-configs/pci.mak |   1 +
>  hw/Makefile.objs        |   1 +
>  hw/can/Makefile.objs    |   3 +
>  hw/can/can_core.c       | 374 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/can/can_emu.h   | 133 +++++++++++++++++
>  5 files changed, 512 insertions(+)
>  create mode 100644 hw/can/Makefile.objs
>  create mode 100644 hw/can/can_core.c
>  create mode 100644 include/can/can_emu.h
> 
> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> index e514bdef42..bbe11887a1 100644
> --- a/default-configs/pci.mak
> +++ b/default-configs/pci.mak
> @@ -31,6 +31,7 @@ CONFIG_ESP_PCI=y
>  CONFIG_SERIAL=y
>  CONFIG_SERIAL_ISA=y
>  CONFIG_SERIAL_PCI=y
> +CONFIG_CAN_CORE=y

CAN Bus is not PCI specific, your model (MF624) uses the PCI.

This looks weird to me here, but why not.

>  CONFIG_IPACK=y
>  CONFIG_WDT_IB6300ESB=y
>  CONFIG_PCI_TESTDEV=y
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index cf4cb2010b..9d84b8faaa 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -6,6 +6,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += block/
>  devices-dirs-$(CONFIG_SOFTMMU) += bt/
>  devices-dirs-$(CONFIG_SOFTMMU) += char/
>  devices-dirs-$(CONFIG_SOFTMMU) += cpu/
> +devices-dirs-$(CONFIG_SOFTMMU) += can/
>  devices-dirs-$(CONFIG_SOFTMMU) += display/
>  devices-dirs-$(CONFIG_SOFTMMU) += dma/
>  devices-dirs-$(CONFIG_SOFTMMU) += gpio/
> diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs
> new file mode 100644
> index 0000000000..9afb45679f
> --- /dev/null
> +++ b/hw/can/Makefile.objs
> @@ -0,0 +1,3 @@
> +# CAN bus interfaces emulation and infrastructure
> +
> +common-obj-$(CONFIG_CAN_CORE) += can_core.o
> diff --git a/hw/can/can_core.c b/hw/can/can_core.c
> new file mode 100644
> index 0000000000..783aa888db
> --- /dev/null
> +++ b/hw/can/can_core.c
> @@ -0,0 +1,374 @@
> +/*
> + * CAN common CAN bus emulation support
> + *
> + * Copyright (c) 2013-2014 Jin Yang
> + * Copyright (c) 2014 Pavel Pisa
> + *
> + * Initial development supported by Google GSoC 2013 from RTEMS project slot
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +#include "qemu/osdep.h"
> +#include "chardev/char.h"
> +#include "qemu/sockets.h"
> +#include "qemu/error-report.h"
> +#include "hw/hw.h"
> +#include "can/can_emu.h"
> +
> +#ifndef DEBUG_CAN
> +#define DEBUG_CAN 0
> +#endif /*DEBUG_CAN*/
> +
> +/*
> + * The option to connect individual CAN busses
> + * to the host CAN interface is operating system
> + * and CAN drivers infrastructure specific.
> + *
> + * Linux SocketCAN support is implemented for now.
> + * If more otions are added later, then they can be
> + * moved to separate files.
> + */

Hmm Id rather start having this part in hw/can/linux_socketcan.c but
this can indeed be moved later.

> +#ifdef __linux__
> +#include <sys/ioctl.h>
> +#include <net/if.h>
> +#include <linux/can.h>
> +#include <linux/can/raw.h>
> +
> +#define NUM_FILTER        4
> +#define CAN_READ_BUF_LEN  5
> +typedef struct {
> +    CanBusClientState  bus_client;
> +    qemu_can_filter    rfilter[NUM_FILTER];
> +    can_err_mask_t     err_mask;
> +
> +    qemu_can_frame     buf[CAN_READ_BUF_LEN];
> +    int                bufcnt;
> +    int                bufptr;
> +
> +    int                fd;
> +} CanBusHostConnectState;
> +
> +#endif /*__linux__*/
> +
> +static QTAILQ_HEAD(, CanBusState) can_buses =
> +    QTAILQ_HEAD_INITIALIZER(can_buses);
> +
> +CanBusState *can_bus_find_by_name(const char *name, bool create_missing)
> +{
> +    CanBusState *bus;
> +
> +    if (name == NULL) {
> +        name = "canbus0";
> +    }
> +
> +    QTAILQ_FOREACH(bus, &can_buses, next) {
> +        if (!strcmp(bus->name, name)) {
> +            return bus;
> +        }
> +    }
> +
> +    if (!create_missing) {
> +        return 0;
> +    }
> +
> +    bus = g_malloc0(sizeof(*bus));
> +    if (bus == NULL) {
> +        return NULL;
> +    }
> +
> +    QTAILQ_INIT(&bus->clients);
> +
> +    bus->name = g_strdup(name);
> +
> +    QTAILQ_INSERT_TAIL(&can_buses, bus, next);
> +    return bus;
> +}
> +
> +int can_bus_insert_client(CanBusState *bus, CanBusClientState *client)
> +{
> +    client->bus = bus;
> +    QTAILQ_INSERT_TAIL(&bus->clients, client, next);
> +    return 0;
> +}
> +
> +int can_bus_remove_client(CanBusClientState *client)
> +{
> +    CanBusState *bus = client->bus;
> +    if (bus == NULL) {
> +        return 0;
> +    }
> +
> +    QTAILQ_REMOVE(&bus->clients, client, next);
> +    client->bus = NULL;
> +    return 1;
> +}
> +
> +ssize_t can_bus_client_send(CanBusClientState *client,
> +             const struct qemu_can_frame *frames, size_t frames_cnt)
> +{
> +    int ret = 0;
> +    CanBusState *bus = client->bus;
> +    CanBusClientState *peer;
> +    if (bus == NULL) {
> +        return -1;
> +    }
> +
> +    QTAILQ_FOREACH(peer, &bus->clients, next) {
> +        if (peer->info->can_receive(peer)) {
> +            if (peer == client) {
> +                /* No loopback support for now */
> +                continue;
> +            }
> +            if (peer->info->receive(peer, frames, frames_cnt) > 0) {
> +                ret = 1;
> +            }
> +        }
> +    }
> +
> +    return ret;
> +}
> +
> +int can_bus_client_set_filters(CanBusClientState *client,
> +             const struct qemu_can_filter *filters, size_t filters_cnt)
> +{
> +    return 0;
> +}
> +

to avoid this error:

hw/can/can_core.c:153:13: error: 'can_display_msg' defined but not used
[-Werror=unused-function]
 static void can_display_msg(struct qemu_can_frame *msg)
             ^

please move here:

#ifdef __linux__

> +static void can_display_msg(struct qemu_can_frame *msg)
> +{
> +    int i;
> +
> +    fprintf(stderr, "%03X [%01d]:", (msg->can_id & 0x1fffffff), 
> msg->can_dlc);
> +    for (i = 0; i < msg->can_dlc; i++) {
> +        fprintf(stderr, "  %02X", msg->data[i]);
> +    }
> +    fprintf(stderr, "\n");
> +}
> +
> +#ifdef __linux__
> +
> +static void can_bus_host_read(void *opaque)
> +{
> +    CanBusHostConnectState *c;
> +    c = (CanBusHostConnectState *)opaque;
> +
> +    /* CAN_READ_BUF_LEN for multiple messages syscall is possible for future 
> */
> +    c->bufcnt = read(c->fd, c->buf, sizeof(qemu_can_frame));
> +    if (c->bufcnt < 0) {
> +        perror("CAN bus host read");
> +        return;
> +    }
> +
> +    can_bus_client_send(&c->bus_client, c->buf, 1);
> +
> +    if (DEBUG_CAN) {
> +        can_display_msg(c->buf); /* Just display the first one. */
> +    }
> +}
> +
> +static int can_bus_host_can_receive(CanBusClientState *client)
> +{
> +    CanBusHostConnectState *c;
> +    c = container_of(client, CanBusHostConnectState, bus_client);
> +
> +    if (c->fd < 0) {
> +        return -1;
> +    }
> +
> +    return 1;
> +}
> +
> +static ssize_t can_bus_host_receive(CanBusClientState *client,
> +                            const qemu_can_frame *frames, size_t frames_cnt)
> +{
> +    CanBusHostConnectState *c;
> +    c = container_of(client, CanBusHostConnectState, bus_client);
> +    size_t len = sizeof(qemu_can_frame);
> +    int res;
> +
> +    if (c->fd < 0) {
> +        return -1;
> +    }
> +
> +    res = write(c->fd, frames, len);
> +
> +    if (!res) {
> +        fprintf(stderr, "CAN bus write to host device zero length\n");
> +        return -1;
> +    }
> +
> +    /* send frame */
> +    if (res != len) {
> +        perror("CAN bus write to host device error");
> +        return -1;
> +    }
> +
> +    return 1;
> +}
> +
> +static void can_bus_host_cleanup(CanBusClientState *client)
> +{
> +    CanBusHostConnectState *c;
> +    c = container_of(client, CanBusHostConnectState, bus_client);
> +
> +    if (c->fd >= 0) {
> +        qemu_set_fd_handler(c->fd, NULL, NULL, c);
> +        close(c->fd);
> +        c->fd = -1;
> +    }
> +}
> +
> +static int can_bus_host_set_filters(CanBusClientState *client,
> +                   const struct qemu_can_filter *filters, size_t filters_cnt)
> +{
> +    CanBusHostConnectState *c;
> +    c = container_of(client, CanBusHostConnectState, bus_client);
> +
> +    int i;
> +
> +    if (filters_cnt > 4) {
> +        return -1;
> +    }
> +
> +    if (DEBUG_CAN) {
> +        for (i = 0; i < filters_cnt; i++) {
> +            fprintf(stderr, "[%i]  id=0x%08x maks=0x%08x\n",
> +                   i, filters[i].can_id, filters[i].can_mask);
> +        }
> +    }
> +
> +    setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER,
> +               filters, filters_cnt * sizeof(qemu_can_filter));
> +
> +    return 0;
> +}
> +
> +static void can_bus_host_update_read_handler(CanBusHostConnectState *c)
> +{
> +    if (c->fd >= 0) {
> +        qemu_set_fd_handler(c->fd, can_bus_host_read, NULL, c);
> +    }
> +}
> +
> +static CanBusClientInfo can_bus_host_bus_client_info = {
> +    .can_receive = can_bus_host_can_receive,
> +    .receive = can_bus_host_receive,
> +    .cleanup = can_bus_host_cleanup,
> +    .poll = NULL
> +};
> +
> +static
> +CanBusHostConnectState *can_bus_host_connect_new(const char *host_dev_name)
> +{
> +    int s; /* can raw socket */
> +    CanBusHostConnectState    *c;
> +    struct sockaddr_can addr;
> +    struct ifreq ifr;
> +
> +    c = g_malloc0(sizeof(CanBusHostConnectState));
> +    if (c == NULL) {
> +        goto fail1;
> +    }
> +
> +    c->fd = -1;
> +
> +    /* open socket */
> +    s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
> +    if (s < 0) {
> +        perror("socket");
> +        goto fail;
> +    }
> +
> +    addr.can_family = AF_CAN;
> +    memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
> +    strcpy(ifr.ifr_name, host_dev_name);
> +    if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
> +        perror("SIOCGIFINDEX");
> +        goto fail;
> +    }
> +    addr.can_ifindex = ifr.ifr_ifindex;
> +
> +    c->err_mask = 0xffffffff; /* Receive error frame. */
> +    setsockopt(s, SOL_CAN_RAW, CAN_RAW_ERR_FILTER,
> +                   &c->err_mask, sizeof(c->err_mask));
> +
> +    /* Receive all data frame. If |= CAN_INV_FILTER no data. */
> +    c->rfilter[0].can_id = 0;
> +    c->rfilter[0].can_mask = 0;
> +    c->rfilter[0].can_mask &= ~CAN_ERR_FLAG;
> +    setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER,
> +                   c->rfilter, sizeof(struct qemu_can_filter));
> +
> +    if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
> +        perror("bind");
> +        goto fail;
> +    }
> +
> +    c->fd = s;
> +
> +    c->bus_client.info = &can_bus_host_bus_client_info;
> +
> +    can_bus_host_update_read_handler(c);
> +
> +    return c;
> +
> +fail:
> +    can_bus_host_cleanup(&c->bus_client);
> +    g_free(c);
> +fail1:
> +
> +    return NULL;
> +}
> +
> +int can_bus_connect_to_host_device(CanBusState *bus, const char 
> *host_dev_name)
> +{
> +    CanBusHostConnectState *c;
> +
> +    c = can_bus_host_connect_new(host_dev_name);
> +    if (c == NULL) {
> +        error_report("CAN bus setup of host connect to \"%s\" failed",
> +                      host_dev_name);
> +        exit(1);
> +    }
> +
> +    if (can_bus_insert_client(bus, &c->bus_client) < 0) {
> +        error_report("CAN host device \"%s\" connect to bus \"%s\" failed",
> +                      host_dev_name, bus->name);
> +        exit(1);
> +    }
> +
> +    if (0) {
> +        /*
> +         * Not used there or as a CanBusHostConnectState method
> +         * for now but included there for documentation purposes
> +         * and to suppress warning.
> +         */
> +        can_bus_host_set_filters(&c->bus_client, NULL, 0);
> +    }
> +
> +    return 0;
> +}
> +
> +#else /*__linux__*/
> +int can_bus_connect_to_host_device(CanBusState *bus, const char *name)
> +{
> +    error_report("CAN bus connect to host device not supported on this 
> system");
> +    exit(1);
> +}

This would be normally be in 'can-stubs.c'

> +#endif /*__linux__*/
> diff --git a/include/can/can_emu.h b/include/can/can_emu.h
> new file mode 100644
> index 0000000000..86b35aef32
> --- /dev/null
> +++ b/include/can/can_emu.h
> @@ -0,0 +1,133 @@
> +/*
> + * CAN common CAN bus emulation support
> + *
> + * Copyright (c) 2013-2014 Jin Yang
> + * Copyright (c) 2014 Pavel Pisa
> + *
> + * Initial development supported by Google GSoC 2013 from RTEMS project slot
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef NET_CAN_EMU_H
> +#define NET_CAN_EMU_H
> +
> +#include "qemu/queue.h"
> +
> +/* NOTE: the following two structures is copied from <linux/can.h>. */
> +
> +/*
> + * Controller Area Network Identifier structure
> + *
> + * bit 0-28    : CAN identifier (11/29 bit)
> + * bit 29      : error frame flag (0 = data frame, 1 = error frame)
> + * bit 30      : remote transmission request flag (1 = rtr frame)
> + * bit 31      : frame format flag (0 = standard 11 bit, 1 = extended 29 bit)
> + */
> +typedef uint32_t qemu_canid_t;
> +
> +#if defined(__GNUC__) || defined(__linux__)
> +    #define QEMU_CAN_FRAME_DATA_ALIGN __attribute__((aligned(8)))
> +#else
> +    #define QEMU_CAN_FRAME_DATA_ALIGN
> +#endif

drop ...

> +
> +typedef struct qemu_can_frame {
> +    qemu_canid_t    can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
> +    uint8_t         can_dlc; /* data length code: 0 .. 8 */
> +    uint8_t         data[8] QEMU_CAN_FRAME_DATA_ALIGN;

... and use QEMU_ALIGNED(8) instead

> +} qemu_can_frame;
> +
> +/**
> + * struct qemu_can_filter - CAN ID based filter in can_register().
> + * @can_id:   relevant bits of CAN ID which are not masked out.
> + * @can_mask: CAN mask (see description)
> + *
> + * Description:
> + * A filter matches, when
> + *
> + *          <received_can_id> & mask == can_id & mask
> + *
> + * The filter can be inverted (CAN_INV_FILTER bit set in can_id) or it can
> + * filter for error message frames (CAN_ERR_FLAG bit set in mask).
> + */
> +typedef struct qemu_can_filter {
> +    qemu_canid_t    can_id;
> +    qemu_canid_t    can_mask;
> +} qemu_can_filter;
> +
> +#define CAN_INV_FILTER 0x20000000U /* to be set in qemu_can_filter.can_id */
> +
> +typedef struct CanBusClientState CanBusClientState;
> +typedef struct CanBusState CanBusState;
> +
> +typedef void (CanBusClientPoll)(CanBusClientState *, bool enable);
> +typedef int (CanBusClientCanReceive)(CanBusClientState *);
> +typedef ssize_t (CanBusClientReceive)(CanBusClientState *,
> +                 const struct qemu_can_frame *frames, size_t frames_cnt);
> +typedef void (CanBusClientCleanup) (CanBusClientState *);
> +typedef void (CanBusClientDestructor)(CanBusClientState *);

I'd keep it simple without typedef'ing.

> +
> +typedef struct CanBusClientInfo {
> +    /*CanBusClientOptionsKind type;*/
> +    size_t size;
> +    CanBusClientCanReceive *can_receive;
> +    CanBusClientReceive *receive;
> +    CanBusClientCleanup *cleanup;
> +    CanBusClientPoll *poll;
> +} CanBusClientInfo;
> +
> +struct CanBusClientState {
> +    CanBusClientInfo *info;
> +    CanBusState *bus;
> +    int link_down;
> +    QTAILQ_ENTRY(CanBusClientState) next;
> +    CanBusClientState *peer;
> +    /*CanBusQueue *incoming_queue;*/
> +    char *model;
> +    char *name;
> +    /*unsigned receive_disabled : 1;*/
> +    CanBusClientDestructor *destructor;
> +    /*unsigned int queue_index;*/
> +    /*unsigned rxfilter_notify_enabled:1;*/
> +};
> +
> +struct CanBusState {
> +    char *name;
> +    QTAILQ_HEAD(, CanBusClientState) clients;
> +    QTAILQ_ENTRY(CanBusState) next;
> +};
> +
> +CanBusState *can_bus_find_by_name(const char *name, bool create_missing);
> +
> +int can_bus_insert_client(CanBusState *bus, CanBusClientState *client);
> +
> +int can_bus_remove_client(CanBusClientState *client);
> +
> +ssize_t can_bus_client_send(CanBusClientState *,
> +                            const struct qemu_can_frame *frames,
> +                            size_t frames_cnt);
> +
> +int can_bus_client_set_filters(CanBusClientState *,
> +                               const struct qemu_can_filter *filters,
> +                               size_t filters_cnt);
> +
> +int can_bus_connect_to_host_device(CanBusState *bus, const char 
> *host_dev_name);
> +
> +#endif

Regards,

Phil.