| Message ID | 425e38f28bba536cfb1ae389ffa963984990f306.1515960078.git.pisa@cmp.felk.cvut.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pavel, I'm CC'ing the QEMU Sockets maintainer to ask them a quick review of the socket part. On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote: > From: Pavel Pisa <pisa@cmp.felk.cvut.cz> > > Connection to the real host CAN bus network through > SocketCAN network interface is available only for Linux > host system. Mechanism is generic, support for another > CAN API and operating systems can be implemented in future. > > Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> > --- > hw/can/Makefile.objs | 4 + > hw/can/can_socketcan.c | 314 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 318 insertions(+) > create mode 100644 hw/can/can_socketcan.c > > diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs > index 1028d7c455..f999085f7a 100644 > --- a/hw/can/Makefile.objs > +++ b/hw/can/Makefile.objs > @@ -2,5 +2,9 @@ > > ifeq ($(CONFIG_CAN_CORE),y) > common-obj-y += can_core.o > +ifeq ($(CONFIG_LINUX),y) > +common-obj-y += can_socketcan.o > +else > common-obj-y += can_host_stub.o > endif > +endif > diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c > new file mode 100644 > index 0000000000..f6df747c5a > --- /dev/null > +++ b/hw/can/can_socketcan.c > @@ -0,0 +1,314 @@ > +/* > + * CAN socketcan support to connect to the Linux host SocketCAN interfaces > + * > + * Copyright (c) 2013-2014 Jin Yang > + * Copyright (c) 2014-2018 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 "qemu/log.h" > +#include "qemu/error-report.h" > +#include "chardev/char.h" > +#include "qemu/sockets.h" > +#include "qemu/error-report.h" > +#include "hw/hw.h" > +#include "can/can_emu.h" > + > +#include <sys/ioctl.h> > +#include <net/if.h> > +#include <linux/can.h> > +#include <linux/can/raw.h> > + > +#ifndef DEBUG_CAN > +#define DEBUG_CAN 0 > +#endif /*DEBUG_CAN*/ > + > +#define CAN_READ_BUF_LEN 5 > +typedef struct { > + CanBusClientState bus_client; > + qemu_can_filter *rfilter; > + int rfilter_num; > + can_err_mask_t err_mask; > + > + qemu_can_frame buf[CAN_READ_BUF_LEN]; > + int bufcnt; > + int bufptr; > + > + int fd; > +} CanBusSocketcanConnectState; > + Please move those checks out of the function, to call them once at build time and not at runtime. /* Check that QEMU and Linux kernel flags encoding matches */ QEMU_BUILD_BUG_ON(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); QEMU_BUILD_BUG_ON(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); QEMU_BUILD_BUG_ON(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); QEMU_BUILD_BUG_ON(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); QEMU_BUILD_BUG_ON(offsetof(qemu_can_frame, data) == offsetof(struct can_frame, data)); > +static void can_bus_socketcan_display_msg(struct qemu_can_frame *msg) > +{ > + int i; > + > + /* Check that QEMU and Linux kernel flags encoding matches */ > + assert(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); > + assert(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); > + assert(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); > + > + assert(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); > + > + assert(offsetof(qemu_can_frame, data) == offsetof(struct can_frame, data)); ^ those > + > + qemu_log_lock(); > + qemu_log("[cansocketcan]: %03X [%01d] %s %s", > + msg->can_id & QEMU_CAN_EFF_MASK, > + msg->can_dlc, > + msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF", > + msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT"); > + > + for (i = 0; i < msg->can_dlc; i++) { > + qemu_log(" %02X", msg->data[i]); > + } > + qemu_log("\n"); I'd rather use tracepoints, but since this is restricted by DEBUG_CAN this doesn't bother the user console, so ok. > + qemu_log_flush(); > + qemu_log_unlock(); > +} > + > +static void can_bus_socketcan_read(void *opaque) > +{ > + CanBusSocketcanConnectState *c; > + c = (CanBusSocketcanConnectState *)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) { > + warn_report("CAN bus host read failed (%s)", strerror(errno)); > + return; > + } > + > + can_bus_client_send(&c->bus_client, c->buf, 1); > + > + if (DEBUG_CAN) { > + can_bus_socketcan_display_msg(c->buf); > + } > +} > + > +static int can_bus_socketcan_can_receive(CanBusClientState *client) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, bus_client); > + > + if (c->fd < 0) { > + return -1; > + } > + > + return 1; > +} > + > +static ssize_t can_bus_socketcan_receive(CanBusClientState *client, > + const qemu_can_frame *frames, size_t frames_cnt) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, 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) { > + warn_report("[cansocketcan]: write message to host returns zero"); > + return -1; > + } > + > + if (res != len) { > + if (res < 0) { > + warn_report("[cansocketcan]: write to host failed (%s)", > + strerror(errno)); > + } else { > + warn_report("[cansocketcan]: write to host truncated"); > + } > + return -1; > + } > + > + return 1; > +} > + > +static void can_bus_socketcan_cleanup(CanBusClientState *client) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, bus_client); > + > + if (c->fd >= 0) { > + qemu_set_fd_handler(c->fd, NULL, NULL, c); > + close(c->fd); > + c->fd = -1; > + } > + > + c->rfilter_num = 0; > + if (c->rfilter != NULL) { > + g_free(c->rfilter); > + } > +} > + > +static int can_bus_socketcan_set_filters(CanBusClientState *client, > + const struct qemu_can_filter *filters, size_t filters_cnt) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, bus_client); > + > + int i; > + > + if (DEBUG_CAN) { > + qemu_log_lock(); > + qemu_log("[cansocketcan]: filters set for channel\n"); > + 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); > + } > + qemu_log("\n"); > + qemu_log_flush(); > + qemu_log_unlock(); > + } > + > + setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER, > + filters, filters_cnt * sizeof(qemu_can_filter)); > + > + return 0; > +} > + > +static > +void can_bus_socketcan_update_read_handler(CanBusSocketcanConnectState *c) > +{ > + if (c->fd >= 0) { > + qemu_set_fd_handler(c->fd, can_bus_socketcan_read, NULL, c); > + } > +} > + > +static CanBusClientInfo can_bus_socketcan_bus_client_info = { > + .can_receive = can_bus_socketcan_can_receive, > + .receive = can_bus_socketcan_receive, > + .cleanup = can_bus_socketcan_cleanup, > + .poll = NULL > +}; > + > +static CanBusSocketcanConnectState * > + can_bus_socketcan_connect_new(const char *host_dev_name) > +{ > + int s; /* can raw socket */ > + CanBusSocketcanConnectState *c; > + struct sockaddr_can addr; > + struct ifreq ifr; > + > + c = g_malloc0(sizeof(CanBusSocketcanConnectState)); > + if (c == NULL) { > + goto fail1; > + } > + > + c->fd = -1; > + > + /* open socket */ > + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); I never used it, but I think QEMU uses his socket API: "qemu/sockets.h" > + if (s < 0) { > + error_report("[cansocketcan]: CAN_RAW socket create failed (%s)", > + strerror(errno)); > + 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) { > + error_report("[cansocketcan]: host interface %s not available (%s)", > + host_dev_name, strerror(errno)); > + 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)); > + > + c->rfilter_num = 1; > + c->rfilter = g_malloc0(c->rfilter_num * sizeof(struct qemu_can_filter)); > + if (c->rfilter == NULL) { > + goto fail; > + } > + > + /* 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, > + c->rfilter_num * sizeof(struct qemu_can_filter)); > + > + if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) { > + error_report("[cansocketcan]: bind to host interface %s failed (%s)", > + host_dev_name, strerror(errno)); > + goto fail; > + } > + > + c->fd = s; > + > + c->bus_client.info = &can_bus_socketcan_bus_client_info; > + > + can_bus_socketcan_update_read_handler(c); > + > + return c; > + > +fail: > + can_bus_socketcan_cleanup(&c->bus_client); > + g_free(c); > +fail1: > + > + return NULL; > +} > + > +static int can_bus_connect_to_host_socketcan(CanBusState *bus, > + const char *host_dev_name) > +{ > + CanBusSocketcanConnectState *c; > + > + c = can_bus_socketcan_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 CanBusSocketcanConnectState method > + * for now but included there for documentation purposes > + * and to suppress warning. > + */ > + can_bus_socketcan_set_filters(&c->bus_client, NULL, 0); > + } > + > + return 0; > +} > + > +int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = > + can_bus_connect_to_host_socketcan; >
Hello Philippe, thanks for review. I have updated patch series in can-pci branch in https://gitlab.fel.cvut.cz/canbus/qemu-canbus I would wait some day if there is some remark from other developers and socket ones especially. On Monday 15 of January 2018 03:55:00 Philippe Mathieu-Daudé wrote: > Hi Pavel, > > I'm CC'ing the QEMU Sockets maintainer to ask them a quick review of the > socket part. > > On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote: > > From: Pavel Pisa <pisa@cmp.felk.cvut.cz> > Please move those checks out of the function, to call them once at build > time and not at runtime. > > /* Check that QEMU and Linux kernel flags encoding matches */ > QEMU_BUILD_BUG_ON(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); > QEMU_BUILD_BUG_ON(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); > QEMU_BUILD_BUG_ON(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); > QEMU_BUILD_BUG_ON(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); > QEMU_BUILD_BUG_ON(offsetof(qemu_can_frame, data) > == offsetof(struct can_frame, data)); moved > > + > > + qemu_log_lock(); > > + qemu_log("[cansocketcan]: %03X [%01d] %s %s", > > + msg->can_id & QEMU_CAN_EFF_MASK, > > + msg->can_dlc, > > + msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF", > > + msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT"); > > + > > + for (i = 0; i < msg->can_dlc; i++) { > > + qemu_log(" %02X", msg->data[i]); > > + } > > + qemu_log("\n"); > > I'd rather use tracepoints, but since this is restricted by DEBUG_CAN > this doesn't bother the user console, so ok. > > > + qemu_log_flush(); > > + qemu_log_unlock(); > > +} Trace events seems as nice feature. But simple text printf like output has been enough till now for CAN testing. There is no debugging output in production build. So I would add tracing infrastructure later if needed. > > + /* open socket */ > > + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); > > I never used it, but I think QEMU uses his socket API: "qemu/sockets.h" The SocketCAN host connection code is Linux specific, but I can switch to qemu_socket() if it is preferred. But address family has to be from Linux header file anyway. Best wishes, Pavel
On 01/15/2018 06:29 PM, Pavel Pisa wrote: > Hello Philippe, > > thanks for review. > > I have updated patch series in can-pci branch in > > https://gitlab.fel.cvut.cz/canbus/qemu-canbus > > I would wait some day if there is some remark > from other developers and socket ones especially. I'll have a look. > > On Monday 15 of January 2018 03:55:00 Philippe Mathieu-Daudé wrote: >> Hi Pavel, >> >> I'm CC'ing the QEMU Sockets maintainer to ask them a quick review of the >> socket part. >> >> On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote: >>> From: Pavel Pisa <pisa@cmp.felk.cvut.cz> > >> Please move those checks out of the function, to call them once at build >> time and not at runtime. >> >> /* Check that QEMU and Linux kernel flags encoding matches */ >> QEMU_BUILD_BUG_ON(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); >> QEMU_BUILD_BUG_ON(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); >> QEMU_BUILD_BUG_ON(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); >> QEMU_BUILD_BUG_ON(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); >> QEMU_BUILD_BUG_ON(offsetof(qemu_can_frame, data) >> == offsetof(struct can_frame, data)); > > moved > >>> + >>> + qemu_log_lock(); >>> + qemu_log("[cansocketcan]: %03X [%01d] %s %s", >>> + msg->can_id & QEMU_CAN_EFF_MASK, >>> + msg->can_dlc, >>> + msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF", >>> + msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT"); >>> + >>> + for (i = 0; i < msg->can_dlc; i++) { >>> + qemu_log(" %02X", msg->data[i]); >>> + } >>> + qemu_log("\n"); >> >> I'd rather use tracepoints, but since this is restricted by DEBUG_CAN >> this doesn't bother the user console, so ok. >> >>> + qemu_log_flush(); >>> + qemu_log_unlock(); >>> +} > > Trace events seems as nice feature. But simple text printf > like output has been enough till now for CAN testing. > There is no debugging output in production build. > So I would add tracing infrastructure later if needed. They are as useful as console printf, but less invasive and more powerful: you can use a much precise timing, select which set of events to display without having to recompile. The default backend behaves as console printf. You should try them :) >>> + /* open socket */ >>> + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); >> >> I never used it, but I think QEMU uses his socket API: "qemu/sockets.h" > > The SocketCAN host connection code is Linux specific, > but I can switch to qemu_socket() if it is preferred. > But address family has to be from Linux header file anyway. qemu_socket() sockets are heavily tested and already solve many things, like async I/O and error handling. Regards, Phil.
Hello Philippe, On Tuesday 16 of January 2018 01:12:09 Philippe Mathieu-Daudé wrote: > On 01/15/2018 06:29 PM, Pavel Pisa wrote: > > Hello Philippe, > > > > thanks for review. > > > > I have updated patch series in can-pci branch in > > > > https://gitlab.fel.cvut.cz/canbus/qemu-canbus > > > > I would wait some day if there is some remark > > from other developers and socket ones especially. > > I'll have a look. if there are no more comments, I prepare version 5 of the patches at start of the next week. Already implemented changes for version 5: Assertions to check match to Linux struct can_frame has been moved out of the function. The case statements use ranges. min_access_size=1 removed. "for loops" are used to process operations for the first and the second chip where appropriate in PCM-3680I and MIOe-3680 patches. Deniz Eren reported that updated version works correctly. I have not proceed with next two remarks yet: Inlining in can_sja_single_filter and can_sja_dual_filter functions. There is only single group of repeated four lines of code which could be inlined, others are groups of two lines each. > if (extended) { vvvvvvvvvvvvvvv > filter->can_id = (uint32_t)acr[0] << 21; > filter->can_id |= (uint32_t)acr[1] << 13; > filter->can_id |= (uint32_t)acr[2] << 5; > filter->can_id |= (uint32_t)acr[3] >> 3; ^^^^^^^^^^^^^^^ > if (acr[3] & 4) { > filter->can_id |= QEMU_CAN_RTR_FLAG; > } > vvvvvvvvvvvvvvv > filter->can_mask = (uint32_t)amr[0] << 21; > filter->can_mask |= (uint32_t)amr[1] << 13; > filter->can_mask |= (uint32_t)amr[2] << 5; > filter->can_mask |= (uint32_t)amr[3] >> 3; ^^^^^^^^^^^^^^^ > filter->can_mask = ~filter->can_mask & QEMU_CAN_EFF_MASK; > if (!(amr[3] & 4)) { > filter->can_mask |= QEMU_CAN_RTR_FLAG; > } Only these marked parts repeats. Rest is similar but not material for simple inline. Same for others two line sequences. All these bit level manipulations are moved from frame processing and another more top level functions into these short can_sja_{single,dual}_filter functions. > /* PeliCAN mode */ > enum SJA1000_PeliCAN_regs { > SJA_MOD = 0x00, > /* Command register */ > SJA_CMR = 0x01, .... > /* TX Error Counter */ > SJA_TXERR0 = 0x0e, > SJA_TXERR1 = 0x0f, > /* Rx Message Counter (number of msgs. in RX FIFO */ > SJA_RMC = 0x1d, > /* Rx Buffer Start Addr. (address of current MSG) */ > SJA_RBSA = 0x1e, > /* Transmit Buffer (write) Receive Buffer (read) Frame Information */ > SJA_FRM = 0x10, > /* > * ID bytes (11 bits in 0 and 1 for standard message or > * 16 bits in 0,1 and 13 bits in 2,3 for extended message) > */ > SJA_ID0 = 0x11, SJA_ID1 = 0x12, > /* ID cont. for extended frames */ > SJA_ID2 = 0x13, SJA_ID3 = 0x14, Proposed formating SJA_MOD = 0x00, /* Command */ SJA_CMR = 0x01, /* Status */ this is really better for short comments. But how format long ones then to keep lines max 80 characters SJA_FRM = 0x10, /* Transmit Buffer (write) Receive Buffer * (read) Frame Information */ The SJA_ID0 is would look even worse. But if the second format is preferred then I update the patch. > > Trace events seems as nice feature. But simple text printf > > like output has been enough till now for CAN testing. > > There is no debugging output in production build. > > So I would add tracing infrastructure later if needed. > > They are as useful as console printf, but less invasive and more > powerful: you can use a much precise timing, select which set of events > to display without having to recompile. > The default backend behaves as console printf. > > You should try them :) I have tried them on pci_update_mappings_del pci_update_mappings_add pci_cfg_write and they work great. They would be nice for SJA1000 register accesses, PCI boards configuration etc. I am not sure how to use them for CAN messages which has a variable length data field. Any of debugging outputs is active (all is optimized out) in regular build. > >>> + /* open socket */ > >>> + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); > >> > >> I never used it, but I think QEMU uses his socket API: "qemu/sockets.h" > > > > The SocketCAN host connection code is Linux specific, > > but I can switch to qemu_socket() if it is preferred. > > But address family has to be from Linux header file anyway. > > qemu_socket() sockets are heavily tested and already solve many things, > like async I/O and error handling. The CAN socket works with RAW packets read and written as 16 bytes. So some higher level of optimizations used for stream or larger packets have no significant effect there and SocketCAN API is Linux kernel specific so generic abstraction layers has no effect there as well. I have question about reporting fatal error. Our code uses error_report() and exit(1) for now. error_report("CAN host device \"%s\" connect to bus \"%s\" failed", host_dev_name, bus->name); exit(1); Is that correct or there is better solution/function instead of exit? I have seen this combination in many other places of QEMU code in past but may it be there is some proposed change already. Best wishes, Pavel Pisa
Cc'ing Paolo and Marc-André, the "Character device backends" maintainers. On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote: > From: Pavel Pisa <pisa@cmp.felk.cvut.cz> > > Connection to the real host CAN bus network through > SocketCAN network interface is available only for Linux > host system. Mechanism is generic, support for another > CAN API and operating systems can be implemented in future. > > Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> > --- > hw/can/Makefile.objs | 4 + > hw/can/can_socketcan.c | 314 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 318 insertions(+) > create mode 100644 hw/can/can_socketcan.c > > diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs > index 1028d7c455..f999085f7a 100644 > --- a/hw/can/Makefile.objs > +++ b/hw/can/Makefile.objs > @@ -2,5 +2,9 @@ > > ifeq ($(CONFIG_CAN_CORE),y) > common-obj-y += can_core.o > +ifeq ($(CONFIG_LINUX),y) > +common-obj-y += can_socketcan.o > +else > common-obj-y += can_host_stub.o I'd just throw this stub in stubs/can_host.c > endif > +endif > diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c > new file mode 100644 > index 0000000000..f6df747c5a > --- /dev/null > +++ b/hw/can/can_socketcan.c I think this is not the correct place to put this file, this is not a modeled hardware, but a chardev backend. This would be chardev/can-socketcan.c. > @@ -0,0 +1,314 @@ > +/* > + * CAN socketcan support to connect to the Linux host SocketCAN interfaces > + * > + * Copyright (c) 2013-2014 Jin Yang > + * Copyright (c) 2014-2018 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 "qemu/log.h" > +#include "qemu/error-report.h" > +#include "chardev/char.h" > +#include "qemu/sockets.h" > +#include "qemu/error-report.h" > +#include "hw/hw.h" > +#include "can/can_emu.h" > + > +#include <sys/ioctl.h> > +#include <net/if.h> > +#include <linux/can.h> > +#include <linux/can/raw.h> > + > +#ifndef DEBUG_CAN > +#define DEBUG_CAN 0 > +#endif /*DEBUG_CAN*/ > + > +#define CAN_READ_BUF_LEN 5 > +typedef struct { > + CanBusClientState bus_client; > + qemu_can_filter *rfilter; > + int rfilter_num; > + can_err_mask_t err_mask; > + > + qemu_can_frame buf[CAN_READ_BUF_LEN]; > + int bufcnt; > + int bufptr; > + > + int fd; > +} CanBusSocketcanConnectState; > + > +static void can_bus_socketcan_display_msg(struct qemu_can_frame *msg) > +{ > + int i; > + > + /* Check that QEMU and Linux kernel flags encoding matches */ > + assert(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); > + assert(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); > + assert(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); > + > + assert(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); > + > + assert(offsetof(qemu_can_frame, data) == offsetof(struct can_frame, data)); > + > + qemu_log_lock(); > + qemu_log("[cansocketcan]: %03X [%01d] %s %s", > + msg->can_id & QEMU_CAN_EFF_MASK, > + msg->can_dlc, > + msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF", > + msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT"); > + > + for (i = 0; i < msg->can_dlc; i++) { > + qemu_log(" %02X", msg->data[i]); > + } > + qemu_log("\n"); > + qemu_log_flush(); > + qemu_log_unlock(); > +} > + > +static void can_bus_socketcan_read(void *opaque) > +{ > + CanBusSocketcanConnectState *c; > + c = (CanBusSocketcanConnectState *)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) { > + warn_report("CAN bus host read failed (%s)", strerror(errno)); > + return; > + } > + > + can_bus_client_send(&c->bus_client, c->buf, 1); > + > + if (DEBUG_CAN) { > + can_bus_socketcan_display_msg(c->buf); > + } > +} > + > +static int can_bus_socketcan_can_receive(CanBusClientState *client) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, bus_client); > + > + if (c->fd < 0) { > + return -1; > + } > + > + return 1; > +} > + > +static ssize_t can_bus_socketcan_receive(CanBusClientState *client, > + const qemu_can_frame *frames, size_t frames_cnt) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, 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) { > + warn_report("[cansocketcan]: write message to host returns zero"); > + return -1; > + } > + > + if (res != len) { > + if (res < 0) { > + warn_report("[cansocketcan]: write to host failed (%s)", > + strerror(errno)); > + } else { > + warn_report("[cansocketcan]: write to host truncated"); > + } > + return -1; > + } > + > + return 1; > +} > + > +static void can_bus_socketcan_cleanup(CanBusClientState *client) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, bus_client); > + > + if (c->fd >= 0) { > + qemu_set_fd_handler(c->fd, NULL, NULL, c); > + close(c->fd); > + c->fd = -1; > + } > + > + c->rfilter_num = 0; > + if (c->rfilter != NULL) { > + g_free(c->rfilter); > + } > +} > + > +static int can_bus_socketcan_set_filters(CanBusClientState *client, > + const struct qemu_can_filter *filters, size_t filters_cnt) > +{ > + CanBusSocketcanConnectState *c; > + c = container_of(client, CanBusSocketcanConnectState, bus_client); > + > + int i; > + > + if (DEBUG_CAN) { > + qemu_log_lock(); > + qemu_log("[cansocketcan]: filters set for channel\n"); > + 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); > + } > + qemu_log("\n"); > + qemu_log_flush(); > + qemu_log_unlock(); > + } > + > + setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER, > + filters, filters_cnt * sizeof(qemu_can_filter)); Good than you don't do the filtering in userland :) > + > + return 0; > +} > + > +static > +void can_bus_socketcan_update_read_handler(CanBusSocketcanConnectState *c) > +{ > + if (c->fd >= 0) { > + qemu_set_fd_handler(c->fd, can_bus_socketcan_read, NULL, c); > + } > +} > + > +static CanBusClientInfo can_bus_socketcan_bus_client_info = { > + .can_receive = can_bus_socketcan_can_receive, > + .receive = can_bus_socketcan_receive, > + .cleanup = can_bus_socketcan_cleanup, > + .poll = NULL > +}; > + > +static CanBusSocketcanConnectState * > + can_bus_socketcan_connect_new(const char *host_dev_name) > +{ > + int s; /* can raw socket */ > + CanBusSocketcanConnectState *c; > + struct sockaddr_can addr; > + struct ifreq ifr; > + > + c = g_malloc0(sizeof(CanBusSocketcanConnectState)); > + if (c == NULL) { > + goto fail1; > + } > + > + c->fd = -1; > + > + /* open socket */ > + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); > + if (s < 0) { > + error_report("[cansocketcan]: CAN_RAW socket create failed (%s)", > + strerror(errno)); > + 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) { > + error_report("[cansocketcan]: host interface %s not available (%s)", > + host_dev_name, strerror(errno)); > + 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)); > + > + c->rfilter_num = 1; > + c->rfilter = g_malloc0(c->rfilter_num * sizeof(struct qemu_can_filter)); > + if (c->rfilter == NULL) { > + goto fail; > + } > + > + /* 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; > + I'd extract this in a more explicit explicit function like promisc_filter() and use can_bus_socketcan_set_filters(c, promisc_filter); maybe clever would be to use NULL for promisc? #define CANBUS_PROMISC_FILTER NULL /* why... */ can_bus_socketcan_set_filters(c, CANBUS_PROMISC_FILTER); can_bus_socketcan_set_filters( ..., *filters, rfilter_num ) { static const qemu_can_filter promisc_filter = { .can_id = 0, .can_mask = 0, .can_mask &= ~CAN_ERR_FLAG }; if (!filters) { filters = &promisc_filter; rfilter_num = 1; } setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER, filters, rfilter_num * sizeof(struct qemu_can_filter)); ... > + setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, c->rfilter, > + c-> don't forget to g_free(c->rfilter); > + > + if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) { > + error_report("[cansocketcan]: bind to host interface %s failed (%s)", > + host_dev_name, strerror(errno)); > + goto fail; > + } > + > + c->fd = s; > + > + c->bus_client.info = &can_bus_socketcan_bus_client_info; > + > + can_bus_socketcan_update_read_handler(c); > + > + return c; > + > +fail: > + can_bus_socketcan_cleanup(&c->bus_client); > + g_free(c); > +fail1: > + > + return NULL; > +} > + > +static int can_bus_connect_to_host_socketcan(CanBusState *bus, > + const char *host_dev_name) > +{ > + CanBusSocketcanConnectState *c; > + > + c = can_bus_socketcan_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 CanBusSocketcanConnectState method > + * for now but included there for documentation purposes > + * and to suppress warning. So this can be avoided using PROMISC = NULL as suggested. > + */ > + can_bus_socketcan_set_filters(&c->bus_client, NULL, 0); > + } > + > + return 0; > +} > + > +int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = > + can_bus_connect_to_host_socketcan; >
On 01/19/2018 09:57 AM, Philippe Mathieu-Daudé wrote: > Cc'ing Paolo and Marc-André, the "Character device backends" maintainers. > > On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote: >> From: Pavel Pisa <pisa@cmp.felk.cvut.cz> >> >> Connection to the real host CAN bus network through >> SocketCAN network interface is available only for Linux >> host system. Mechanism is generic, support for another >> CAN API and operating systems can be implemented in future. >> >> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz> >> --- >> hw/can/Makefile.objs | 4 + >> hw/can/can_socketcan.c | 314 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 318 insertions(+) >> create mode 100644 hw/can/can_socketcan.c >> >> diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs >> index 1028d7c455..f999085f7a 100644 >> --- a/hw/can/Makefile.objs >> +++ b/hw/can/Makefile.objs >> @@ -2,5 +2,9 @@ >> >> ifeq ($(CONFIG_CAN_CORE),y) >> common-obj-y += can_core.o >> +ifeq ($(CONFIG_LINUX),y) >> +common-obj-y += can_socketcan.o >> +else >> common-obj-y += can_host_stub.o > > I'd just throw this stub in stubs/can_host.c > >> endif >> +endif >> diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c >> new file mode 100644 >> index 0000000000..f6df747c5a >> --- /dev/null >> +++ b/hw/can/can_socketcan.c > > I think this is not the correct place to put this file, this is not a > modeled hardware, but a chardev backend. I meant 'socketcan is a frontend for the canbus char backend'. > > This would be chardev/can-socketcan.c. > >> @@ -0,0 +1,314 @@ >> +/* >> + * CAN socketcan support to connect to the Linux host SocketCAN interfaces >> + * >> + * Copyright (c) 2013-2014 Jin Yang >> + * Copyright (c) 2014-2018 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 "qemu/log.h" >> +#include "qemu/error-report.h" >> +#include "chardev/char.h" >> +#include "qemu/sockets.h" >> +#include "qemu/error-report.h" >> +#include "hw/hw.h" >> +#include "can/can_emu.h" >> + >> +#include <sys/ioctl.h> >> +#include <net/if.h> >> +#include <linux/can.h> >> +#include <linux/can/raw.h> >> + >> +#ifndef DEBUG_CAN >> +#define DEBUG_CAN 0 >> +#endif /*DEBUG_CAN*/ >> + >> +#define CAN_READ_BUF_LEN 5 >> +typedef struct { >> + CanBusClientState bus_client; >> + qemu_can_filter *rfilter; >> + int rfilter_num; >> + can_err_mask_t err_mask; >> + >> + qemu_can_frame buf[CAN_READ_BUF_LEN]; >> + int bufcnt; >> + int bufptr; >> + >> + int fd; >> +} CanBusSocketcanConnectState; >> + >> +static void can_bus_socketcan_display_msg(struct qemu_can_frame *msg) >> +{ >> + int i; >> + >> + /* Check that QEMU and Linux kernel flags encoding matches */ >> + assert(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); >> + assert(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); >> + assert(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); >> + >> + assert(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); >> + >> + assert(offsetof(qemu_can_frame, data) == offsetof(struct can_frame, data)); >> + >> + qemu_log_lock(); >> + qemu_log("[cansocketcan]: %03X [%01d] %s %s", >> + msg->can_id & QEMU_CAN_EFF_MASK, >> + msg->can_dlc, >> + msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF", >> + msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT"); >> + >> + for (i = 0; i < msg->can_dlc; i++) { >> + qemu_log(" %02X", msg->data[i]); >> + } >> + qemu_log("\n"); >> + qemu_log_flush(); >> + qemu_log_unlock(); >> +} >> + >> +static void can_bus_socketcan_read(void *opaque) >> +{ >> + CanBusSocketcanConnectState *c; >> + c = (CanBusSocketcanConnectState *)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) { >> + warn_report("CAN bus host read failed (%s)", strerror(errno)); >> + return; >> + } >> + >> + can_bus_client_send(&c->bus_client, c->buf, 1); >> + >> + if (DEBUG_CAN) { >> + can_bus_socketcan_display_msg(c->buf); >> + } >> +} >> + >> +static int can_bus_socketcan_can_receive(CanBusClientState *client) >> +{ >> + CanBusSocketcanConnectState *c; >> + c = container_of(client, CanBusSocketcanConnectState, bus_client); >> + >> + if (c->fd < 0) { >> + return -1; >> + } >> + >> + return 1; >> +} >> + >> +static ssize_t can_bus_socketcan_receive(CanBusClientState *client, >> + const qemu_can_frame *frames, size_t frames_cnt) >> +{ >> + CanBusSocketcanConnectState *c; >> + c = container_of(client, CanBusSocketcanConnectState, 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) { >> + warn_report("[cansocketcan]: write message to host returns zero"); >> + return -1; >> + } >> + >> + if (res != len) { >> + if (res < 0) { >> + warn_report("[cansocketcan]: write to host failed (%s)", >> + strerror(errno)); >> + } else { >> + warn_report("[cansocketcan]: write to host truncated"); >> + } >> + return -1; >> + } >> + >> + return 1; >> +} >> + >> +static void can_bus_socketcan_cleanup(CanBusClientState *client) >> +{ >> + CanBusSocketcanConnectState *c; >> + c = container_of(client, CanBusSocketcanConnectState, bus_client); >> + >> + if (c->fd >= 0) { >> + qemu_set_fd_handler(c->fd, NULL, NULL, c); >> + close(c->fd); >> + c->fd = -1; >> + } >> + >> + c->rfilter_num = 0; >> + if (c->rfilter != NULL) { >> + g_free(c->rfilter); >> + } >> +} >> + >> +static int can_bus_socketcan_set_filters(CanBusClientState *client, >> + const struct qemu_can_filter *filters, size_t filters_cnt) >> +{ >> + CanBusSocketcanConnectState *c; >> + c = container_of(client, CanBusSocketcanConnectState, bus_client); >> + >> + int i; >> + >> + if (DEBUG_CAN) { >> + qemu_log_lock(); >> + qemu_log("[cansocketcan]: filters set for channel\n"); >> + 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); >> + } >> + qemu_log("\n"); >> + qemu_log_flush(); >> + qemu_log_unlock(); >> + } >> + >> + setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER, >> + filters, filters_cnt * sizeof(qemu_can_filter)); > > Good than you don't do the filtering in userland :) > >> + >> + return 0; >> +} >> + >> +static >> +void can_bus_socketcan_update_read_handler(CanBusSocketcanConnectState *c) >> +{ >> + if (c->fd >= 0) { >> + qemu_set_fd_handler(c->fd, can_bus_socketcan_read, NULL, c); >> + } >> +} >> + >> +static CanBusClientInfo can_bus_socketcan_bus_client_info = { >> + .can_receive = can_bus_socketcan_can_receive, >> + .receive = can_bus_socketcan_receive, >> + .cleanup = can_bus_socketcan_cleanup, >> + .poll = NULL >> +}; >> + >> +static CanBusSocketcanConnectState * >> + can_bus_socketcan_connect_new(const char *host_dev_name) >> +{ >> + int s; /* can raw socket */ >> + CanBusSocketcanConnectState *c; >> + struct sockaddr_can addr; >> + struct ifreq ifr; >> + >> + c = g_malloc0(sizeof(CanBusSocketcanConnectState)); >> + if (c == NULL) { >> + goto fail1; >> + } >> + >> + c->fd = -1; >> + >> + /* open socket */ >> + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); >> + if (s < 0) { >> + error_report("[cansocketcan]: CAN_RAW socket create failed (%s)", >> + strerror(errno)); >> + 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) { >> + error_report("[cansocketcan]: host interface %s not available (%s)", >> + host_dev_name, strerror(errno)); >> + 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)); >> + >> + c->rfilter_num = 1; >> + c->rfilter = g_malloc0(c->rfilter_num * sizeof(struct qemu_can_filter)); >> + if (c->rfilter == NULL) { >> + goto fail; >> + } >> + >> + /* 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; >> + > > I'd extract this in a more explicit explicit function like > promisc_filter() and use > > can_bus_socketcan_set_filters(c, promisc_filter); > > maybe clever would be to use NULL for promisc? > > #define CANBUS_PROMISC_FILTER NULL /* why... */ > > can_bus_socketcan_set_filters(c, CANBUS_PROMISC_FILTER); > > can_bus_socketcan_set_filters( ..., *filters, rfilter_num ) > { > static const qemu_can_filter promisc_filter = { > .can_id = 0, > .can_mask = 0, > .can_mask &= ~CAN_ERR_FLAG > }; > > if (!filters) { > filters = &promisc_filter; > rfilter_num = 1; > } > setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER, > filters, rfilter_num * sizeof(struct qemu_can_filter)); > ... > >> + setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER, c->rfilter, >> + c-> > > don't forget to g_free(c->rfilter); > >> + >> + if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) { >> + error_report("[cansocketcan]: bind to host interface %s failed (%s)", >> + host_dev_name, strerror(errno)); >> + goto fail; >> + } >> + >> + c->fd = s; >> + >> + c->bus_client.info = &can_bus_socketcan_bus_client_info; >> + >> + can_bus_socketcan_update_read_handler(c); >> + >> + return c; >> + >> +fail: >> + can_bus_socketcan_cleanup(&c->bus_client); >> + g_free(c); >> +fail1: >> + >> + return NULL; >> +} >> + >> +static int can_bus_connect_to_host_socketcan(CanBusState *bus, >> + const char *host_dev_name) >> +{ >> + CanBusSocketcanConnectState *c; >> + >> + c = can_bus_socketcan_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 CanBusSocketcanConnectState method >> + * for now but included there for documentation purposes >> + * and to suppress warning. > > So this can be avoided using PROMISC = NULL as suggested. > >> + */ >> + can_bus_socketcan_set_filters(&c->bus_client, NULL, 0); >> + } >> + >> + return 0; >> +} >> + >> +int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = >> + can_bus_connect_to_host_socketcan; >>
On Mon, Jan 15, 2018 at 09:12:09PM -0300, Philippe Mathieu-Daudé wrote: > On 01/15/2018 06:29 PM, Pavel Pisa wrote: > >>> + /* open socket */ > >>> + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); > >> > >> I never used it, but I think QEMU uses his socket API: "qemu/sockets.h" > > > > The SocketCAN host connection code is Linux specific, > > but I can switch to qemu_socket() if it is preferred. > > But address family has to be from Linux header file anyway. > > qemu_socket() sockets are heavily tested and already solve many things, > like async I/O and error handling. NB that's just the low level system call wrapper. All it really does is ensure O_CLOSEXEC is set for all sockets. It should defintely be used just for that reason alone though. Regards, Daniel
On 01/19/2018 05:51 AM, Pavel Pisa wrote: > Hello Philippe, > > On Tuesday 16 of January 2018 01:12:09 Philippe Mathieu-Daudé wrote: >> On 01/15/2018 06:29 PM, Pavel Pisa wrote: >>> Hello Philippe, >>> >>> thanks for review. >>> >>> I have updated patch series in can-pci branch in >>> >>> https://gitlab.fel.cvut.cz/canbus/qemu-canbus >>> >>> I would wait some day if there is some remark >>> from other developers and socket ones especially. >> >> I'll have a look. > > if there are no more comments, I prepare version 5 > of the patches at start of the next week. > > Already implemented changes for version 5: > > Assertions to check match to Linux struct can_frame > has been moved out of the function. > The case statements use ranges. > min_access_size=1 removed. > "for loops" are used to process operations > for the first and the second chip where > appropriate in PCM-3680I and MIOe-3680 patches. > Deniz Eren reported that updated version > works correctly. > > I have not proceed with next two remarks yet: > > Inlining in can_sja_single_filter and can_sja_dual_filter > functions. There is only single group of repeated four lines > of code which could be inlined, others are groups of two > lines each. > >> if (extended) { > vvvvvvvvvvvvvvv >> filter->can_id = (uint32_t)acr[0] << 21; >> filter->can_id |= (uint32_t)acr[1] << 13; >> filter->can_id |= (uint32_t)acr[2] << 5; >> filter->can_id |= (uint32_t)acr[3] >> 3; > ^^^^^^^^^^^^^^^ >> if (acr[3] & 4) { >> filter->can_id |= QEMU_CAN_RTR_FLAG; >> } >> > vvvvvvvvvvvvvvv >> filter->can_mask = (uint32_t)amr[0] << 21; >> filter->can_mask |= (uint32_t)amr[1] << 13; >> filter->can_mask |= (uint32_t)amr[2] << 5; >> filter->can_mask |= (uint32_t)amr[3] >> 3; > ^^^^^^^^^^^^^^^ >> filter->can_mask = ~filter->can_mask & QEMU_CAN_EFF_MASK; >> if (!(amr[3] & 4)) { >> filter->can_mask |= QEMU_CAN_RTR_FLAG; >> } > > Only these marked parts repeats. > Rest is similar but not material for simple inline. > Same for others two line sequences. > All these bit level manipulations are moved from frame > processing and another more top level functions into these > short can_sja_{single,dual}_filter functions. fine. > >> /* PeliCAN mode */ >> enum SJA1000_PeliCAN_regs { >> SJA_MOD = 0x00, >> /* Command register */ >> SJA_CMR = 0x01, > .... >> /* TX Error Counter */ >> SJA_TXERR0 = 0x0e, >> SJA_TXERR1 = 0x0f, >> /* Rx Message Counter (number of msgs. in RX FIFO */ >> SJA_RMC = 0x1d, >> /* Rx Buffer Start Addr. (address of current MSG) */ >> SJA_RBSA = 0x1e, >> /* Transmit Buffer (write) Receive Buffer (read) Frame Information */ >> SJA_FRM = 0x10, >> /* >> * ID bytes (11 bits in 0 and 1 for standard message or >> * 16 bits in 0,1 and 13 bits in 2,3 for extended message) >> */ >> SJA_ID0 = 0x11, SJA_ID1 = 0x12, >> /* ID cont. for extended frames */ >> SJA_ID2 = 0x13, SJA_ID3 = 0x14, > > Proposed formating > > SJA_MOD = 0x00, /* Command */ > SJA_CMR = 0x01, /* Status */ > > this is really better for short comments. > But how format long ones then to keep lines max 80 characters > > SJA_FRM = 0x10, /* Transmit Buffer (write) Receive Buffer > * (read) Frame Information > */ SJA_FRM = 0x10, /* Frame Information */ /* on write: Transmit Buffer used */ /* on read: Receive Buffer used */ but I'd keep it as: SJA_FRM = 0x10, /* Frame Information */ > > The SJA_ID0 is would look even worse. As you prefer, comments are good if useful, too much redundant comments are counter productive IMHO. > > But if the second format is preferred then I update the patch. > >>> Trace events seems as nice feature. But simple text printf >>> like output has been enough till now for CAN testing. >>> There is no debugging output in production build. >>> So I would add tracing infrastructure later if needed. >> >> They are as useful as console printf, but less invasive and more >> powerful: you can use a much precise timing, select which set of events >> to display without having to recompile. >> The default backend behaves as console printf. >> >> You should try them :) > > I have tried them on > > pci_update_mappings_del > pci_update_mappings_add > pci_cfg_write > > and they work great. They would be nice for SJA1000 > register accesses, PCI boards configuration etc. > I am not sure how to use them for CAN messages > which has a variable length data field. Yes, I hit this problem with variable length data in the SD Bus. Let's ask Stefan what is the best approach (he said "Tracing is not great for dumping large amounts of data") > Any of debugging outputs is active (all is optimized out) > in regular build. > >>>>> + /* open socket */ >>>>> + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); >>>> >>>> I never used it, but I think QEMU uses his socket API: "qemu/sockets.h" >>> >>> The SocketCAN host connection code is Linux specific, >>> but I can switch to qemu_socket() if it is preferred. >>> But address family has to be from Linux header file anyway. I'll let the Chardev maintainers suggest what's best. >> >> qemu_socket() sockets are heavily tested and already solve many things, >> like async I/O and error handling. > > The CAN socket works with RAW packets read and written as 16 bytes. > So some higher level of optimizations used for stream or larger packets > have no significant effect there and SocketCAN API is Linux kernel > specific so generic abstraction layers has no effect there as well. > > I have question about reporting fatal error. > > Our code uses error_report() and exit(1) for now. > > error_report("CAN host device \"%s\" connect to bus \"%s\" failed", > host_dev_name, bus->name); > exit(1); > > Is that correct or there is better solution/function instead of exit? > I have seen this combination in many other places of QEMU code > in past but may it be there is some proposed change already. I think QEMU codebase now try to unify this using "include/qapi/error.h", so your can_bus_connect_to_host_socketcan() function would take an extra 'Error *errp' parameter. > > Best wishes, > > Pavel Pisa > >
On Fri, Jan 19, 2018 at 10:37:22AM -0300, Philippe Mathieu-Daudé wrote: > On 01/19/2018 05:51 AM, Pavel Pisa wrote: > > On Tuesday 16 of January 2018 01:12:09 Philippe Mathieu-Daudé wrote: > >> On 01/15/2018 06:29 PM, Pavel Pisa wrote: > > But if the second format is preferred then I update the patch. > > > >>> Trace events seems as nice feature. But simple text printf > >>> like output has been enough till now for CAN testing. > >>> There is no debugging output in production build. > >>> So I would add tracing infrastructure later if needed. > >> > >> They are as useful as console printf, but less invasive and more > >> powerful: you can use a much precise timing, select which set of events > >> to display without having to recompile. > >> The default backend behaves as console printf. > >> > >> You should try them :) > > > > I have tried them on > > > > pci_update_mappings_del > > pci_update_mappings_add > > pci_cfg_write > > > > and they work great. They would be nice for SJA1000 > > register accesses, PCI boards configuration etc. > > I am not sure how to use them for CAN messages > > which has a variable length data field. > > Yes, I hit this problem with variable length data in the SD Bus. > > Let's ask Stefan what is the best approach (he said "Tracing is not > great for dumping large amounts of data") Yes, tracing isn't pcap. It's not designed for dumping payloads - especially when the length is more than 64 bytes or so. I suggest writing out a pcap file so wireshark or tcpdump can be used to analyze it. The code is pretty simple, see net/dump.c. Stefan
diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs index 1028d7c455..f999085f7a 100644 --- a/hw/can/Makefile.objs +++ b/hw/can/Makefile.objs @@ -2,5 +2,9 @@ ifeq ($(CONFIG_CAN_CORE),y) common-obj-y += can_core.o +ifeq ($(CONFIG_LINUX),y) +common-obj-y += can_socketcan.o +else common-obj-y += can_host_stub.o endif +endif diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c new file mode 100644 index 0000000000..f6df747c5a --- /dev/null +++ b/hw/can/can_socketcan.c @@ -0,0 +1,314 @@ +/* + * CAN socketcan support to connect to the Linux host SocketCAN interfaces + * + * Copyright (c) 2013-2014 Jin Yang + * Copyright (c) 2014-2018 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 "qemu/log.h" +#include "qemu/error-report.h" +#include "chardev/char.h" +#include "qemu/sockets.h" +#include "qemu/error-report.h" +#include "hw/hw.h" +#include "can/can_emu.h" + +#include <sys/ioctl.h> +#include <net/if.h> +#include <linux/can.h> +#include <linux/can/raw.h> + +#ifndef DEBUG_CAN +#define DEBUG_CAN 0 +#endif /*DEBUG_CAN*/ + +#define CAN_READ_BUF_LEN 5 +typedef struct { + CanBusClientState bus_client; + qemu_can_filter *rfilter; + int rfilter_num; + can_err_mask_t err_mask; + + qemu_can_frame buf[CAN_READ_BUF_LEN]; + int bufcnt; + int bufptr; + + int fd; +} CanBusSocketcanConnectState; + +static void can_bus_socketcan_display_msg(struct qemu_can_frame *msg) +{ + int i; + + /* Check that QEMU and Linux kernel flags encoding matches */ + assert(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG); + assert(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG); + assert(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG); + + assert(QEMU_CAN_INV_FILTER == CAN_INV_FILTER); + + assert(offsetof(qemu_can_frame, data) == offsetof(struct can_frame, data)); + + qemu_log_lock(); + qemu_log("[cansocketcan]: %03X [%01d] %s %s", + msg->can_id & QEMU_CAN_EFF_MASK, + msg->can_dlc, + msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF", + msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT"); + + for (i = 0; i < msg->can_dlc; i++) { + qemu_log(" %02X", msg->data[i]); + } + qemu_log("\n"); + qemu_log_flush(); + qemu_log_unlock(); +} + +static void can_bus_socketcan_read(void *opaque) +{ + CanBusSocketcanConnectState *c; + c = (CanBusSocketcanConnectState *)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) { + warn_report("CAN bus host read failed (%s)", strerror(errno)); + return; + } + + can_bus_client_send(&c->bus_client, c->buf, 1); + + if (DEBUG_CAN) { + can_bus_socketcan_display_msg(c->buf); + } +} + +static int can_bus_socketcan_can_receive(CanBusClientState *client) +{ + CanBusSocketcanConnectState *c; + c = container_of(client, CanBusSocketcanConnectState, bus_client); + + if (c->fd < 0) { + return -1; + } + + return 1; +} + +static ssize_t can_bus_socketcan_receive(CanBusClientState *client, + const qemu_can_frame *frames, size_t frames_cnt) +{ + CanBusSocketcanConnectState *c; + c = container_of(client, CanBusSocketcanConnectState, 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) { + warn_report("[cansocketcan]: write message to host returns zero"); + return -1; + } + + if (res != len) { + if (res < 0) { + warn_report("[cansocketcan]: write to host failed (%s)", + strerror(errno)); + } else { + warn_report("[cansocketcan]: write to host truncated"); + } + return -1; + } + + return 1; +} + +static void can_bus_socketcan_cleanup(CanBusClientState *client) +{ + CanBusSocketcanConnectState *c; + c = container_of(client, CanBusSocketcanConnectState, bus_client); + + if (c->fd >= 0) { + qemu_set_fd_handler(c->fd, NULL, NULL, c); + close(c->fd); + c->fd = -1; + } + + c->rfilter_num = 0; + if (c->rfilter != NULL) { + g_free(c->rfilter); + } +} + +static int can_bus_socketcan_set_filters(CanBusClientState *client, + const struct qemu_can_filter *filters, size_t filters_cnt) +{ + CanBusSocketcanConnectState *c; + c = container_of(client, CanBusSocketcanConnectState, bus_client); + + int i; + + if (DEBUG_CAN) { + qemu_log_lock(); + qemu_log("[cansocketcan]: filters set for channel\n"); + 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); + } + qemu_log("\n"); + qemu_log_flush(); + qemu_log_unlock(); + } + + setsockopt(c->fd, SOL_CAN_RAW, CAN_RAW_FILTER, + filters, filters_cnt * sizeof(qemu_can_filter)); + + return 0; +} + +static +void can_bus_socketcan_update_read_handler(CanBusSocketcanConnectState *c) +{ + if (c->fd >= 0) { + qemu_set_fd_handler(c->fd, can_bus_socketcan_read, NULL, c); + } +} + +static CanBusClientInfo can_bus_socketcan_bus_client_info = { + .can_receive = can_bus_socketcan_can_receive, + .receive = can_bus_socketcan_receive, + .cleanup = can_bus_socketcan_cleanup, + .poll = NULL +}; + +static CanBusSocketcanConnectState * + can_bus_socketcan_connect_new(const char *host_dev_name) +{ + int s; /* can raw socket */ + CanBusSocketcanConnectState *c; + struct sockaddr_can addr; + struct ifreq ifr; + + c = g_malloc0(sizeof(CanBusSocketcanConnectState)); + if (c == NULL) { + goto fail1; + } + + c->fd = -1; + + /* open socket */ + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); + if (s < 0) { + error_report("[cansocketcan]: CAN_RAW socket create failed (%s)", + strerror(errno)); + 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) { + error_report("[cansocketcan]: host interface %s not available (%s)", + host_dev_name, strerror(errno)); + 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)); + + c->rfilter_num = 1; + c->rfilter = g_malloc0(c->rfilter_num * sizeof(struct qemu_can_filter)); + if (c->rfilter == NULL) { + goto fail; + } + + /* 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, + c->rfilter_num * sizeof(struct qemu_can_filter)); + + if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) { + error_report("[cansocketcan]: bind to host interface %s failed (%s)", + host_dev_name, strerror(errno)); + goto fail; + } + + c->fd = s; + + c->bus_client.info = &can_bus_socketcan_bus_client_info; + + can_bus_socketcan_update_read_handler(c); + + return c; + +fail: + can_bus_socketcan_cleanup(&c->bus_client); + g_free(c); +fail1: + + return NULL; +} + +static int can_bus_connect_to_host_socketcan(CanBusState *bus, + const char *host_dev_name) +{ + CanBusSocketcanConnectState *c; + + c = can_bus_socketcan_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 CanBusSocketcanConnectState method + * for now but included there for documentation purposes + * and to suppress warning. + */ + can_bus_socketcan_set_filters(&c->bus_client, NULL, 0); + } + + return 0; +} + +int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) = + can_bus_connect_to_host_socketcan;