From: Pavel Pisa <pisa@cmp.felk.cvut.cz>

Basic emulation of CAN bus controller and interconnection for QEMU.

Patches version 4:
Resolve comments longer than 80 characters to suppress
all warnings reported by scripts/checkpatch.pl.
Follow all suggestions from Frederic Konrad review.
Replace all printf and perror calls by QEMU equivalents.
Include Deniz Eren signed-off confimation.

Patches version 3:
Support to connect to host SocketCAN interface has been
separated from the core bus implementation. Only simple
statically initialize pointer to the connection function
is used, no QOM concept for now.
SJA1000 message filters redone and code unified where
possible.
Basic documentation added.
QEMU_ALIGNED used in definition of CAN frame structure,
structure and defines are separated from Linux/SocketCAN
API defined ones to allow to keep QEMU message format
independed from host system one. Check for correspondence
to socketcan defines added.

Patches version 2:
The bus emulation and the SJA1000 chip emulation introduced
by individual patches as suggested by Frederic Konrad.
Simple example board to test SJA1000 as single memory-mapped BAR
has been omitted in a new series because emulation of real
existing boards can provide same functions now.
Conditionalized debug printfs changed to be exposed to compiler
syntax check as suggested in review.

The work has been started by Jin Yang in the frame of GSoC 2013 slot
contributed by RTEMS project which has been looking for environment
to allow develop and test CAN drivers for multiple CPU architectures.

I have menthored the project and then done substantial code cleanup
and update to QOM. Deniz Eren then used emulation for SJA1000 base card
driver development for other operating system and contributed
PCM-3680I and MIOe-3680 support.

Some page about the project

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/wikis/home

FEE CTU GitLab repository with can-pci branch for 2.3, 2.4, 2.7, 2.8, 2.10
and 2.11 QEMU version is available in the repository

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci

mirror at GitHub

  https://github.com/CTU-IIG/qemu

There are many areas for improvement and extension of the code still
(for example freeze and migration is not implemented. CAN controllers
use proper QOM model but bus/interconnection emulation uses simple broadcast
connection which is required for CAN, but it is not based on QEMU bus model).
I have tried to look into QEMU VLANs implementation but it
does not map straightforward to CAN and I would need some help/opinion
from more advanced developers to decide what is their right
mapping to CAN.

CAN-FD support would be interesting requires other developers/
companies contributions or setup of some project to allow invite
some students and colleagues from my university into project.

But I believe that (even in its actual state) provided solution
is great help for embedded systems developers when they can connect
SocketCAN from one or more embedded systems running in virtual
environment together or with Linux host SocketCAN virtual
or real bus interfaces.

We have even tested our generic CANopen device configured
for CANopen 401 profile for generic I/O running in the virtual
system which can control GPIO inputs/outputs through virtual
industrial I/O card.

Generally QEMU can be interesting setup which allows
to test complete industrial and automotive applications
in virtual environment even before real hardware is availabe.

Deniz Eren (2):
  CAN bus PCM-3680I PCI (dual SJA1000 channel) emulation added.
  CAN bus MIOe-3680 PCI (dual SJA1000 channel) emulation added.

Pavel Pisa (5):
  CAN bus simple messages transport implementation for QEMU
  CAN bus support to connect bust to Linux host SocketCAN interface.
  CAN bus SJA1000 chip register level emulation for QEMU
  CAN bus Kvaser PCI CAN-S (single SJA1000 channel) emulation added.
  QEMU CAN bus emulation documentation

 default-configs/pci.mak   |    3 +
 docs/can.txt              |   78 ++++
 hw/Makefile.objs          |    1 +
 hw/can/Makefile.objs      |   14 +
 hw/can/can_core.c         |  136 ++++++
 hw/can/can_host_stub.c    |   36 ++
 hw/can/can_kvaser_pci.c   |  375 +++++++++++++++++
 hw/can/can_mioe3680_pci.c |  336 +++++++++++++++
 hw/can/can_pcm3680_pci.c  |  336 +++++++++++++++
 hw/can/can_sja1000.c      | 1013 +++++++++++++++++++++++++++++++++++++++++++++
 hw/can/can_sja1000.h      |  167 ++++++++
 hw/can/can_socketcan.c    |  314 ++++++++++++++
 include/can/can_emu.h     |  131 ++++++
 13 files changed, 2940 insertions(+)
 create mode 100644 docs/can.txt
 create mode 100644 hw/can/Makefile.objs
 create mode 100644 hw/can/can_core.c
 create mode 100644 hw/can/can_host_stub.c
 create mode 100644 hw/can/can_kvaser_pci.c
 create mode 100644 hw/can/can_mioe3680_pci.c
 create mode 100644 hw/can/can_pcm3680_pci.c
 create mode 100644 hw/can/can_sja1000.c
 create mode 100644 hw/can/can_sja1000.h
 create mode 100644 hw/can/can_socketcan.c
 create mode 100644 include/can/can_emu.h

-- 
2.11.0

Hi Pavel,

On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote:
> From: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> 
> Basic emulation of CAN bus controller and interconnection for QEMU.
> 
> Patches version 4:
> Resolve comments longer than 80 characters to suppress
> all warnings reported by scripts/checkpatch.pl.
> Follow all suggestions from Frederic Konrad review.
> Replace all printf and perror calls by QEMU equivalents.
> Include Deniz Eren signed-off confimation.
> 
> Patches version 3:
> Support to connect to host SocketCAN interface has been
> separated from the core bus implementation. Only simple
> statically initialize pointer to the connection function
> is used, no QOM concept for now.
> SJA1000 message filters redone and code unified where
> possible.
> Basic documentation added.
> QEMU_ALIGNED used in definition of CAN frame structure,
> structure and defines are separated from Linux/SocketCAN
> API defined ones to allow to keep QEMU message format
> independed from host system one. Check for correspondence
> to socketcan defines added.
> 
> Patches version 2:
> The bus emulation and the SJA1000 chip emulation introduced
> by individual patches as suggested by Frederic Konrad.
> Simple example board to test SJA1000 as single memory-mapped BAR
> has been omitted in a new series because emulation of real
> existing boards can provide same functions now.
> Conditionalized debug printfs changed to be exposed to compiler
> syntax check as suggested in review.
> 
> The work has been started by Jin Yang in the frame of GSoC 2013 slot
> contributed by RTEMS project which has been looking for environment
> to allow develop and test CAN drivers for multiple CPU architectures.
> 
> I have menthored the project and then done substantial code cleanup
> and update to QOM. Deniz Eren then used emulation for SJA1000 base card
> driver development for other operating system and contributed
> PCM-3680I and MIOe-3680 support.
> 
> Some page about the project
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus/wikis/home
> 
> FEE CTU GitLab repository with can-pci branch for 2.3, 2.4, 2.7, 2.8, 2.10
> and 2.11 QEMU version is available in the repository
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci
> 
> mirror at GitHub
> 
>   https://github.com/CTU-IIG/qemu
> 
> There are many areas for improvement and extension of the code still
> (for example freeze and migration is not implemented. CAN controllers
> use proper QOM model but bus/interconnection emulation uses simple broadcast
> connection which is required for CAN, but it is not based on QEMU bus model).
> I have tried to look into QEMU VLANs implementation but it
> does not map straightforward to CAN and I would need some help/opinion
> from more advanced developers to decide what is their right
> mapping to CAN.

I think your series is quite ready to get accepted, although I'm not
sure through with tree it will goes.

Most of my review comments are not blocking issues and might get
addressed later (like having an abstract sja1000).

The principal issue I'd like to discuss with Paolo/Marc-André is the
chardev backend, they might say it's OK to accept it in this current
state and refactor the CANBus backend later.

I'd still avoid using directly the socket() syscall and use the QEMU
socket API instead (also suggested by Daniel).

> 
> CAN-FD support would be interesting requires other developers/
> companies contributions or setup of some project to allow invite
> some students and colleagues from my university into project.
> 
> But I believe that (even in its actual state) provided solution
> is great help for embedded systems developers when they can connect
> SocketCAN from one or more embedded systems running in virtual
> environment together or with Linux host SocketCAN virtual
> or real bus interfaces.
> 
> We have even tested our generic CANopen device configured
> for CANopen 401 profile for generic I/O running in the virtual
> system which can control GPIO inputs/outputs through virtual
> industrial I/O card.
> 
> Generally QEMU can be interesting setup which allows
> to test complete industrial and automotive applications
> in virtual environment even before real hardware is availabe.

I have been thinking a bit about how to test some frame operations
(rather than the PCI devices) and the Linux vcan driver might be a good
option (Virtual Local CAN Interface). This is also useful to test this
series without having CAN hardware. How to use vcan might be worth his
own paragraph in docs/can.txt.

Do you think some of your tests can be added in the QEMU test suite
(qtests)?

Regards,

Phil.

> 
> Deniz Eren (2):
>   CAN bus PCM-3680I PCI (dual SJA1000 channel) emulation added.
>   CAN bus MIOe-3680 PCI (dual SJA1000 channel) emulation added.
> 
> Pavel Pisa (5):
>   CAN bus simple messages transport implementation for QEMU
>   CAN bus support to connect bust to Linux host SocketCAN interface.
>   CAN bus SJA1000 chip register level emulation for QEMU
>   CAN bus Kvaser PCI CAN-S (single SJA1000 channel) emulation added.
>   QEMU CAN bus emulation documentation
> 
>  default-configs/pci.mak   |    3 +
>  docs/can.txt              |   78 ++++
>  hw/Makefile.objs          |    1 +
>  hw/can/Makefile.objs      |   14 +
>  hw/can/can_core.c         |  136 ++++++
>  hw/can/can_host_stub.c    |   36 ++
>  hw/can/can_kvaser_pci.c   |  375 +++++++++++++++++
>  hw/can/can_mioe3680_pci.c |  336 +++++++++++++++
>  hw/can/can_pcm3680_pci.c  |  336 +++++++++++++++
>  hw/can/can_sja1000.c      | 1013 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/can/can_sja1000.h      |  167 ++++++++
>  hw/can/can_socketcan.c    |  314 ++++++++++++++
>  include/can/can_emu.h     |  131 ++++++
>  13 files changed, 2940 insertions(+)
>  create mode 100644 docs/can.txt
>  create mode 100644 hw/can/Makefile.objs
>  create mode 100644 hw/can/can_core.c
>  create mode 100644 hw/can/can_host_stub.c
>  create mode 100644 hw/can/can_kvaser_pci.c
>  create mode 100644 hw/can/can_mioe3680_pci.c
>  create mode 100644 hw/can/can_pcm3680_pci.c
>  create mode 100644 hw/can/can_sja1000.c
>  create mode 100644 hw/can/can_sja1000.h
>  create mode 100644 hw/can/can_socketcan.c
>  create mode 100644 include/can/can_emu.h
> 

Hello Philippe,

On Monday 22 of January 2018 12:35:33 Philippe Mathieu-Daudé wrote:
> Hi Pavel,
>
> On 01/14/2018 05:14 PM, pisa@cmp.felk.cvut.cz wrote:
>
> I think your series is quite ready to get accepted, although I'm not
> sure through with tree it will goes.
>
> Most of my review comments are not blocking issues and might get
> addressed later (like having an abstract sja1000).

Great, I would be happy to reach acceptable state when development
can switch to incremental mode as time allows. I hope than even
actual state is usable from reports (at least for some usescases).

> The principal issue I'd like to discuss with Paolo/Marc-André is the
> chardev backend, they might say it's OK to accept it in this current
> state and refactor the CANBus backend later.

Do you think QOM based? I would like it to be implemented
that way but I need some assistance where to look how this
object kind should be implemented and from which base object
inherit from. But I prefer to left that for later work.

I would definitely like to use some mechanism which allows
to get rid of externally visible pointer and need to assign
it in the stub. It has been my initial idea and your review
sumbled across this hack as well. But I need suggestion what
is the preferred way for QEMU.

When Linux specific object file is linked in then some local
function needs to be called before QOM instances population.
I know how to do that GCC specific/non-portable way

static void __attribute__((constructor)) can_socketcan_setup_variant(void)
{

}

but I expect that something like

module_init()

in can_socketcan.c should be used.

Problem is that there is not module_init
type for plain function in include/qemu/module.h

    MODULE_INIT_BLOCK,
    MODULE_INIT_OPTS,
    MODULE_INIT_QOM,
    MODULE_INIT_TRACE,
    MODULE_INIT_MAX

I expect that QOM object would solve that in future
but I would be happy to left it simple for now.

What is preferred solution there?

> I'd still avoid using directly the socket() syscall and use the QEMU
> socket API instead (also suggested by Daniel).

I have already switched to qemu_socket(), implementation
looks fine and I have tested that it works.
I have tested functionality and updated can-pci branch.

> I have been thinking a bit about how to test some frame operations
> (rather than the PCI devices) and the Linux vcan driver might be a good
> option (Virtual Local CAN Interface). This is also useful to test this
> series without having CAN hardware. How to use vcan might be worth his
> own paragraph in docs/can.txt.
>
> Do you think some of your tests can be added in the QEMU test suite
> (qtests)?

I have added some more infomation into docs file

+ The CAN interface of the host system has to be configured for proper
+ bitrate and set up. Configuration is not propagated from emulated
+ devices through bus to the physical host device. Example configuration
+ for 1 Mbit/s
+
+   ip link set can0 type can bitrate 1000000
+   ip link set can0 up
+
+ Virtual (host local only) can interface can be used on the host
+ side instead of physical interface
+
+   ip link add dev can0 type vcan
+
+ The CAN interface on the host side can be used to analyze CAN
+ traffic with "candump" command which is included in "can-utils".
+
+   candump can0

As for the automatic testing, iproute2 tools are required
on host and guest side (considering use of Linux)
and kernel with CAN drivers support.
Root access is required on the host side to setup CAN
interface. Some simple tool is required. It can be based
on can-utils code or our older canping code for example.

Best wishes,

Pavel

Hello everybody,

On Tuesday 23 of January 2018 22:42:31 Pavel Pisa wrote:
> When Linux specific object file is linked in then some local
> function needs to be called before QOM instances population.
> I know how to do that GCC specific/non-portable way
>
> static void __attribute__((constructor)) can_socketcan_setup_variant(void)
> {
>
> }
>
> but I expect that something like
>
> module_init()
>
> in can_socketcan.c should be used.


I have experimented with code changes to get rid of stub for
non Linux systems. type_init() is used because it is more
portable than constructor attribute.

I have seen that a few other type_init-s do more
than simple sequence of type_register_static().
Is it acceptable to use type_init for registration
to CAN core by function call for now? Conversion simplifies
makefiles and unnecessary stub file is removed.

But I would use attribute if that solution is preferred because
it is allways present on Linux where SocketCAN is used anyway
and it is used in other Qemu subsystems as well.

----------------------------------------------------------------
Solution with attribute

#ifdef TOOLCHAIN_SUPPORTS_ATTRIBUTE_CONSTRUCTOR
static void __attribute__((constructor))
can_bus_socketcan_setup(void)
{
    can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
}
#endif

----------------------------------------------------------------
Solution with type_init
branch https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-socketcan-init

diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c
index 42099fb696..fb41853c2b 100644
--- a/hw/can/can_socketcan.c
+++ b/hw/can/can_socketcan.c
@@ -309,5 +309,14 @@ static int can_bus_connect_to_host_socketcan(CanBusState *bus,
     return 0;
 }
 
-int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
-        can_bus_connect_to_host_socketcan;
+static void can_bus_socketcan_type_init(void)
+{
+    /*
+     * There should be object registration when CanBusSocketcanConnectState
+     * is converted into QOM object. Use for registration of host
+     * can bus access for now.
+     */
+    can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
+}
+
+type_init(can_bus_socketcan_type_init);


diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs
index 1ce9950de0..14c2369718 100644
--- a/hw/can/Makefile.objs
+++ b/hw/can/Makefile.objs
@@ -2,11 +2,7 @@
 
 ifeq ($(CONFIG_CAN_BUS),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
+common-obj-$(CONFIG_LINUX) += can_socketcan.o
 common-obj-$(CONFIG_CAN_SJA1000) += can_sja1000.o
 common-obj-$(CONFIG_CAN_PCI) += can_kvaser_pci.o
 common-obj-$(CONFIG_CAN_PCI) += can_pcm3680_pci.o
diff --git a/hw/can/can_core.c b/hw/can/can_core.c
index 41c458c792..c14807b188 100644
--- a/hw/can/can_core.c
+++ b/hw/can/can_core.c
@@ -34,6 +34,8 @@
 static QTAILQ_HEAD(, CanBusState) can_buses =
     QTAILQ_HEAD_INITIALIZER(can_buses);
 
+static can_bus_connect_to_host_t can_bus_connect_to_host_fnc;
+
 CanBusState *can_bus_find_by_name(const char *name, bool create_missing)
 {
     CanBusState *bus;
@@ -127,10 +129,15 @@ int can_bus_client_set_filters(CanBusClientState *client,
 
 int can_bus_connect_to_host_device(CanBusState *bus, const char *name)
 {
-    if (can_bus_connect_to_host_variant == NULL) {
+    if (can_bus_connect_to_host_fnc == NULL) {
         error_report("CAN bus connect to host device is not "
                      "supported on this system");
         exit(1);
     }
-    return can_bus_connect_to_host_variant(bus, name);
+    return can_bus_connect_to_host_fnc(bus, name);
+}
+
+void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc)
+{
+    can_bus_connect_to_host_fnc = connect_fnc;
 }
diff --git a/hw/can/can_host_stub.c b/hw/can/can_host_stub.c
deleted file mode 100644
index 748d25f995..0000000000
--- a/hw/can/can_host_stub.c
+++ /dev/null
@@ -1,36 +0,0 @@
....
....
....
-
-
-int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
-        NULL;

diff --git a/include/can/can_emu.h b/include/can/can_emu.h
index 85237ee3c9..7f0705e49f 100644
--- a/include/can/can_emu.h
+++ b/include/can/can_emu.h
@@ -107,8 +107,9 @@ struct CanBusState {
     QTAILQ_ENTRY(CanBusState) next;
 };
 
-extern int (*can_bus_connect_to_host_variant)(CanBusState *bus,
-                                              const char *name);
+typedef int (*can_bus_connect_to_host_t)(CanBusState *bus, const char *name);
+
+void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc);
 
 int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t can_id);
----------------------------------------------------------------

Best wishes,

Pavel

Hi Pavel,

On 01/24/2018 05:22 PM, Pavel Pisa wrote:
> Hello everybody,
> 
> On Tuesday 23 of January 2018 22:42:31 Pavel Pisa wrote:
>> When Linux specific object file is linked in then some local
>> function needs to be called before QOM instances population.
>> I know how to do that GCC specific/non-portable way
>>
>> static void __attribute__((constructor)) can_socketcan_setup_variant(void)
>> {
>>
>> }
>>
>> but I expect that something like
>>
>> module_init()
>>
>> in can_socketcan.c should be used.
> 
> 
> I have experimented with code changes to get rid of stub for
> non Linux systems. type_init() is used because it is more
> portable than constructor attribute.
> 
> I have seen that a few other type_init-s do more
> than simple sequence of type_register_static().
> Is it acceptable to use type_init for registration
> to CAN core by function call for now? Conversion simplifies
> makefiles and unnecessary stub file is removed.
> 
> But I would use attribute if that solution is preferred because
> it is allways present on Linux where SocketCAN is used anyway
> and it is used in other Qemu subsystems as well.

using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't
work?

> 
> ----------------------------------------------------------------
> Solution with attribute
> 
> #ifdef TOOLCHAIN_SUPPORTS_ATTRIBUTE_CONSTRUCTOR
> static void __attribute__((constructor))
> can_bus_socketcan_setup(void)
> {
>     can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
> }
> #endif
> 
> ----------------------------------------------------------------
> Solution with type_init
> branch https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-socketcan-init
> 
> diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c
> index 42099fb696..fb41853c2b 100644
> --- a/hw/can/can_socketcan.c
> +++ b/hw/can/can_socketcan.c
> @@ -309,5 +309,14 @@ static int can_bus_connect_to_host_socketcan(CanBusState *bus,
>      return 0;
>  }
>  
> -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
> -        can_bus_connect_to_host_socketcan;
> +static void can_bus_socketcan_type_init(void)
> +{
> +    /*
> +     * There should be object registration when CanBusSocketcanConnectState
> +     * is converted into QOM object. Use for registration of host
> +     * can bus access for now.
> +     */
> +    can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
> +}
> +
> +type_init(can_bus_socketcan_type_init);
> 
> 
> diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs
> index 1ce9950de0..14c2369718 100644
> --- a/hw/can/Makefile.objs
> +++ b/hw/can/Makefile.objs
> @@ -2,11 +2,7 @@
>  
>  ifeq ($(CONFIG_CAN_BUS),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
> +common-obj-$(CONFIG_LINUX) += can_socketcan.o
>  common-obj-$(CONFIG_CAN_SJA1000) += can_sja1000.o
>  common-obj-$(CONFIG_CAN_PCI) += can_kvaser_pci.o
>  common-obj-$(CONFIG_CAN_PCI) += can_pcm3680_pci.o
> diff --git a/hw/can/can_core.c b/hw/can/can_core.c
> index 41c458c792..c14807b188 100644
> --- a/hw/can/can_core.c
> +++ b/hw/can/can_core.c
> @@ -34,6 +34,8 @@
>  static QTAILQ_HEAD(, CanBusState) can_buses =
>      QTAILQ_HEAD_INITIALIZER(can_buses);
>  
> +static can_bus_connect_to_host_t can_bus_connect_to_host_fnc;
> +
>  CanBusState *can_bus_find_by_name(const char *name, bool create_missing)
>  {
>      CanBusState *bus;
> @@ -127,10 +129,15 @@ int can_bus_client_set_filters(CanBusClientState *client,
>  
>  int can_bus_connect_to_host_device(CanBusState *bus, const char *name)
>  {
> -    if (can_bus_connect_to_host_variant == NULL) {
> +    if (can_bus_connect_to_host_fnc == NULL) {
>          error_report("CAN bus connect to host device is not "
>                       "supported on this system");
>          exit(1);
>      }
> -    return can_bus_connect_to_host_variant(bus, name);
> +    return can_bus_connect_to_host_fnc(bus, name);
> +}
> +
> +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc)
> +{
> +    can_bus_connect_to_host_fnc = connect_fnc;
>  }
> diff --git a/hw/can/can_host_stub.c b/hw/can/can_host_stub.c
> deleted file mode 100644
> index 748d25f995..0000000000
> --- a/hw/can/can_host_stub.c
> +++ /dev/null
> @@ -1,36 +0,0 @@
> ....
> ....
> ....
> -
> -
> -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
> -        NULL;
> 
> diff --git a/include/can/can_emu.h b/include/can/can_emu.h
> index 85237ee3c9..7f0705e49f 100644
> --- a/include/can/can_emu.h
> +++ b/include/can/can_emu.h
> @@ -107,8 +107,9 @@ struct CanBusState {
>      QTAILQ_ENTRY(CanBusState) next;
>  };
>  
> -extern int (*can_bus_connect_to_host_variant)(CanBusState *bus,
> -                                              const char *name);
> +typedef int (*can_bus_connect_to_host_t)(CanBusState *bus, const char *name);
> +
> +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc);
>  
>  int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t can_id);
> ----------------------------------------------------------------
> 
> Best wishes,
> 
> Pavel
> 

Hello Philippe,

On Wednesday 24 of January 2018 22:41:16 Philippe Mathieu-Daudé wrote:
> Hi Pavel,
> > I have seen that a few other type_init-s do more
> > than simple sequence of type_register_static().
> > Is it acceptable to use type_init for registration
> > to CAN core by function call for now? Conversion simplifies
> > makefiles and unnecessary stub file is removed.
> >
> > But I would use attribute if that solution is preferred because
> > it is allways present on Linux where SocketCAN is used anyway
> > and it is used in other Qemu subsystems as well.
>
> using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't
> work?

If that is preferred then I implement the stub this way.
As for the location, can I add

  stub-obj-y += can_host_stub.o

into hw/can/Makefile.objs same as it is in crypto/Makefile.objs
to keep CAN stuff together at least for now or it should go
to stubs directory?

  stub-obj-$(CONFIG_CAN_BUS) += can_host_variant.o

As for connection to host, again I have weak preference
to keep it in hw/can to keep CAN related code together
but if you think that it should go t chardev, I would prepare
patch that way

  chardev/can-socketcan.c

As for the rest of the remarks

You are right that there is some code duplication
in the SJA1000 CAN PCI cards support but problem
is that KVASER single uses S5920 PCI local bus bridge
which requires additional BARs and additional bridge
specific interrupt enable support. There are more KVASER
variants which combine multiple SJA1000 in a single BAR.
pcm3680 and mioe3680 have different BAR structure,
each SJA1000 uses one BAR. The first uses I/O mapped
SJA1000 and another memory mapped with stride 4.
Yes, it all can be combined into one C file.
But it would require to to add more more fields
into CardX_PCIState structure and some mechanisms
and code to select right combination of the BARs,
handlers etc for each card. It all can be done,
but I am not sure if I find time for such changes now.
I expect to have time again in summer when my teaching
semester ends.
Another disadvantage is that if somebody else wants
to implement other card emulation then actual simple
can_kvaser_pci.c is easily readable. Code gets much
more complex with all variants selection logic and
we have already abandoned that simple PCI example
(can_pci.c) with dummy PCI ID which as been included
in the past.

If the code duplication is a problem for now then
I vote to include only can_kvaser_pci.c for now.
But Deniz Eren would be sad because he uses the
cards (which he has contributed) in his test environment.

Anyway, I would follow what is proposed.

Thanks for your review and time,

Pavel

Hi Pavel, Philippe,

I’m happy with whatever way is best for the project.

However I would personally think merging the different drivers into one C file would not be a very modular way of handling the problem. As you can see from the Advantech drivers for example, the card supplier end can pose difficult Bar allocation logic, which I would think is best isolated within the particular driver model C file. Trying to come up with mechanisms to handle these could prove difficult.

Also keep in mind there should be more driver support added in future, which could pose other driver model specific difficulties that are best isolated to their own C files.

Having said all that, there might be a nice way of merging them all generically which I fail to see currently.



Best regards,
Deniz

Sent from my iPhone

Deniz Eren
+61 400 307 762

> On 25 Jan 2018, at 7:24 pm, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> 
> Hello Philippe,
> 
>> On Wednesday 24 of January 2018 22:41:16 Philippe Mathieu-Daudé wrote:
>> Hi Pavel,
>>> I have seen that a few other type_init-s do more
>>> than simple sequence of type_register_static().
>>> Is it acceptable to use type_init for registration
>>> to CAN core by function call for now? Conversion simplifies
>>> makefiles and unnecessary stub file is removed.
>>> 
>>> But I would use attribute if that solution is preferred because
>>> it is allways present on Linux where SocketCAN is used anyway
>>> and it is used in other Qemu subsystems as well.
>> 
>> using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't
>> work?
> 
> If that is preferred then I implement the stub this way.
> As for the location, can I add
> 
>  stub-obj-y += can_host_stub.o
> 
> into hw/can/Makefile.objs same as it is in crypto/Makefile.objs
> to keep CAN stuff together at least for now or it should go
> to stubs directory?
> 
>  stub-obj-$(CONFIG_CAN_BUS) += can_host_variant.o
> 
> As for connection to host, again I have weak preference
> to keep it in hw/can to keep CAN related code together
> but if you think that it should go t chardev, I would prepare
> patch that way
> 
>  chardev/can-socketcan.c
> 
> As for the rest of the remarks
> 
> You are right that there is some code duplication
> in the SJA1000 CAN PCI cards support but problem
> is that KVASER single uses S5920 PCI local bus bridge
> which requires additional BARs and additional bridge
> specific interrupt enable support. There are more KVASER
> variants which combine multiple SJA1000 in a single BAR.
> pcm3680 and mioe3680 have different BAR structure,
> each SJA1000 uses one BAR. The first uses I/O mapped
> SJA1000 and another memory mapped with stride 4.
> Yes, it all can be combined into one C file.
> But it would require to to add more more fields
> into CardX_PCIState structure and some mechanisms
> and code to select right combination of the BARs,
> handlers etc for each card. It all can be done,
> but I am not sure if I find time for such changes now.
> I expect to have time again in summer when my teaching
> semester ends.
> Another disadvantage is that if somebody else wants
> to implement other card emulation then actual simple
> can_kvaser_pci.c is easily readable. Code gets much
> more complex with all variants selection logic and
> we have already abandoned that simple PCI example
> (can_pci.c) with dummy PCI ID which as been included
> in the past.
> 
> If the code duplication is a problem for now then
> I vote to include only can_kvaser_pci.c for now.
> But Deniz Eren would be sad because he uses the
> cards (which he has contributed) in his test environment.
> 
> Anyway, I would follow what is proposed.
> 
> Thanks for your review and time,
> 
> Pavel

On 23/01/2018 22:42, Pavel Pisa wrote:
> Do you think QOM based? I would like it to be implemented
> that way but I need some assistance where to look how this
> object kind should be implemented and from which base object
> inherit from. But I prefer to left that for later work.
> 
> I would definitely like to use some mechanism which allows
> to get rid of externally visible pointer and need to assign
> it in the stub. It has been my initial idea and your review
> sumbled across this hack as well. But I need suggestion what
> is the preferred way for QEMU.

The best way would be a QOM object.  That is, you would do

   -object can-bus,id=canbus0
   -object can-host-socketcan,id=can0-host,canbus=canbus0,if=can0
   -device kvaser_pci,canbus=canbus0

In the current version, it's not clear to me:

* what it means if multiple controllers have the same canbus

* what it means if multiple controllers with the same canbus have
different host interfaces

Separating the QOM objects is a bit more work, but it makes the
semantics clearer.  The classes would be:

- can-bus and an abstract class can-host, which would inherit directly
from TYPE_OBJECT and implement TYPE_USER_CREATABLE

- can-host-socketcan, which would inherit from can-host (and take the
TYPE_USER_CREATABLE implementation from there)

while CanBusClientState and CanBusClientInfo need not be QOMified.

can-host's class structure would define a function pointer corresponding
to what you have now for the function pointer, more or less---except
that allocation is handled by QOM and the method only has to do the
connection.  You would have something like this:

static void can_host_disconnect(CANHost *ch, Error **errp)
{
    CANHostClass *chc = CAN_HOST_GET_CLASS(ch);

    chc->disconnect(ch);
}

static void can_host_connect(CANHost *ch, Error **errp)
{
    CANHostClass *chc = CAN_HOST_GET_CLASS(ch);
    Error *local_err = NULL;

    chc->connect(ch, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }

    can_bus_insert_client(ch->bus, &ch->bus_client, local_err);
    if (local_err) {
        can_host_disconnect(ch);
        error_propagate(errp, local_err);
        return;
    }
}

and TYPE_USER_CREATABLE's "complete" method would simply invoke
can_host_connect.  For can-host-socketcan, chc->connect would be
assigned something like this:

static void can_host_socketcan_connect(CANHost *ch, Error **errp)
{
    CANHostSocketCAN *chs = CAN_HOST_SOCKETCAN(ch);

    s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
    if (s < 0) {
        error_setg_errno(errp, errno "CAN_RAW socket create failed");
        return;
    }

    addr.can_family = AF_CAN;
    memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
    strcpy(ifr.ifr_name, chs->host_dev_name);
    if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
        error_setg_errno(errp, "host interface %s not available",
                         chs->host_dev_name);
        goto fail;
    }
    addr.can_ifindex = ifr.ifr_ifindex;
    ....
}

In particular, note the difference in error reporting with
error_report/exit vs. error_setg/error_propagate.  Any call to "exit" is
probably grounds for instant rejection of your patch. :)  This also
means that you have to check for leaks in the failure paths, such as
forgetting to close the PF_CAN socket.

Thanks,

Paolo

> When Linux specific object file is linked in then some local
> function needs to be called before QOM instances population.
> I know how to do that GCC specific/non-portable way
> 
> static void __attribute__((constructor)) can_socketcan_setup_variant(void)
> {
> 
> }
> 
> but I expect that something like
> 
> module_init()
> 
> in can_socketcan.c should be used.
> 
> Problem is that there is not module_init
> type for plain function in include/qemu/module.h
> 
>     MODULE_INIT_BLOCK,
>     MODULE_INIT_OPTS,
>     MODULE_INIT_QOM,
>     MODULE_INIT_TRACE,
>     MODULE_INIT_MAX
> 
> I expect that QOM object would solve that in future
> but I would be happy to left it simple for now.
> 
> What is preferred solution there?
> 
>> I'd still avoid using directly the socket() syscall and use the QEMU
>> socket API instead (also suggested by Daniel).
> 
> I have already switched to qemu_socket(), implementation
> looks fine and I have tested that it works.
> I have tested functionality and updated can-pci branch.
> 
>> I have been thinking a bit about how to test some frame operations
>> (rather than the PCI devices) and the Linux vcan driver might be a good
>> option (Virtual Local CAN Interface). This is also useful to test this
>> series without having CAN hardware. How to use vcan might be worth his
>> own paragraph in docs/can.txt.
>>
>> Do you think some of your tests can be added in the QEMU test suite
>> (qtests)?
> 
> I have added some more infomation into docs file
> 
> + The CAN interface of the host system has to be configured for proper
> + bitrate and set up. Configuration is not propagated from emulated
> + devices through bus to the physical host device. Example configuration
> + for 1 Mbit/s
> +
> +   ip link set can0 type can bitrate 1000000
> +   ip link set can0 up
> +
> + Virtual (host local only) can interface can be used on the host
> + side instead of physical interface
> +
> +   ip link add dev can0 type vcan
> +
> + The CAN interface on the host side can be used to analyze CAN
> + traffic with "candump" command which is included in "can-utils".
> +
> +   candump can0
> 
> As for the automatic testing, iproute2 tools are required
> on host and guest side (considering use of Linux)
> and kernel with CAN drivers support.
> Root access is required on the host side to setup CAN
> interface. Some simple tool is required. It can be based
> on can-utils code or our older canping code for example.
> 
> Best wishes,
> 
> Pavel
> 

Hello Paolo,

thanks for suggestions. I understand and fully agree with your
request to switch to QOM. I have succeed with that for CAN devices
some time ago. It worth to be done for the rest of the objects
but I fear that I do not find time to complete QOMification
in reasonable future. Contributions/suggestions from other
are welcomed. I can look for students for GSoC at our university
or under other funding.

On Thursday 25 of January 2018 14:58:41 Paolo Bonzini wrote:
> On 23/01/2018 22:42, Pavel Pisa wrote:
> > Do you think QOM based? I would like it to be implemented
> > that way but I need some assistance where to look how this
> > object kind should be implemented and from which base object
> > inherit from. But I prefer to left that for later work.
> >
> > I would definitely like to use some mechanism which allows
> > to get rid of externally visible pointer and need to assign
> > it in the stub. It has been my initial idea and your review
> > sumbled across this hack as well. But I need suggestion what
> > is the preferred way for QEMU.
>
> The best way would be a QOM object.  That is, you would do
>
>    -object can-bus,id=canbus0
>    -object can-host-socketcan,id=can0-host,canbus=canbus0,if=can0
>    -device kvaser_pci,canbus=canbus0

I would prefer to allow CAN emulation to be used without
explicit can-bus object creation specified on the command line.
The bus object could be created when requested by
can-host-socketcan or device (kvaser_pci) automatically.

When multiple QOM object are put on the list then the list content
should be preserved over save+restore (mostly hypothetical question
for now). There should be probably used some other construct instead
of

  QTAILQ_HEAD(, CanBusClientState) clients;
  QTAILQ_ENTRY(CanBusState) next;

> In the current version, it's not clear to me:
>
> * what it means if multiple controllers have the same canbus

That is fully supported and sane configuration.
CAN is publis/subscribe network in principle
so sending message from one controller to another
one on the host side is intended and can be used
to test drivers even if host connection is
not available/supported on given OS/setup.
Interconnection of multiple controllers with
host CAN bus is functional as well.

> * what it means if multiple controllers with the same canbus have
> different host interfaces

It is legitimate but probably not much usesfull/intended setup.
It would result is bridging two host CAN busses
together. It would work because SocketCAN does not
deliver message back to the socket which has been used to send
it by default. Connecting twice to the same SocketCAN bus
would lead to infinite message loop.

Connection of given can bus to the host twice can be prevented
if host connection flag is included in CanBusState and if it is
already set then next host connection attempt would be reported
as error.

> Separating the QOM objects is a bit more work, but it makes the
> semantics clearer.  The classes would be:
>
> - can-bus and an abstract class can-host, which would inherit directly
> from TYPE_OBJECT and implement TYPE_USER_CREATABLE
>
> - can-host-socketcan, which would inherit from can-host (and take the
> TYPE_USER_CREATABLE implementation from there)
>
> while CanBusClientState and CanBusClientInfo need not be QOMified.

May it be, can-host-socketcan can be based directly on TYPE_OBJECT,
because only QEMU CAN bus specific part is CanBusClientState which
allows it to connect to the CAN bus same way as other CAN controllers
connect to the bus.

> can-host's class structure would define a function pointer corresponding
> to what you have now for the function pointer, more or less---except
> that allocation is handled by QOM and the method only has to do the
> connection.  You would have something like this:
>
> static void can_host_disconnect(CANHost *ch, Error **errp)
> {
>     CANHostClass *chc = CAN_HOST_GET_CLASS(ch);
>
>     chc->disconnect(ch);
> }
>
> static void can_host_connect(CANHost *ch, Error **errp)
> {
>     CANHostClass *chc = CAN_HOST_GET_CLASS(ch);
>     Error *local_err = NULL;
>
>     chc->connect(ch, &local_err);
>     if (local_err) {
>         error_propagate(errp, local_err);
>         return;
>     }
>
>     can_bus_insert_client(ch->bus, &ch->bus_client, local_err);
>     if (local_err) {
>         can_host_disconnect(ch);
>         error_propagate(errp, local_err);
>         return;
>     }
> }
>
> and TYPE_USER_CREATABLE's "complete" method would simply invoke
> can_host_connect.  For can-host-socketcan, chc->connect would be
> assigned something like this:
>
> static void can_host_socketcan_connect(CANHost *ch, Error **errp)
> {
>     CANHostSocketCAN *chs = CAN_HOST_SOCKETCAN(ch);
>
>     s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
>     if (s < 0) {
>         error_setg_errno(errp, errno "CAN_RAW socket create failed");
>         return;
>     }
>
>     addr.can_family = AF_CAN;
>     memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
>     strcpy(ifr.ifr_name, chs->host_dev_name);
>     if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
>         error_setg_errno(errp, "host interface %s not available",
>                          chs->host_dev_name);
>         goto fail;
>     }
>     addr.can_ifindex = ifr.ifr_ifindex;
>     ....
> }

Thanks for providing ideas for future development directions.

> In particular, note the difference in error reporting with
> error_report/exit vs. error_setg/error_propagate.  Any call to "exit" is
> probably grounds for instant rejection of your patch. :)

It seems that it is necessary to switch to use realize()
method instead of init() to have initial **errp pointer
in the call chain.

> This also means that you have to check for leaks in the failure paths,
> such as  forgetting to close the PF_CAN socket.

The socket is closed in can_bus_socketcan_cleanup()
in the failure case. g_free(c->rfilter); is there
as well.

Thanks for ideas and review,

Pavel

On 25/01/2018 22:33, Pavel Pisa wrote:
> Hello Paolo,
> 
> thanks for suggestions. I understand and fully agree with your
> request to switch to QOM. I have succeed with that for CAN devices
> some time ago. It worth to be done for the rest of the objects
> but I fear that I do not find time to complete QOMification
> in reasonable future. Contributions/suggestions from other
> are welcomed. I can look for students for GSoC at our university
> or under other funding.

Coincidentially, I have some time on a flight next Monday. :)  I
obviously cannot really _test_ anything, but I can at least do the
changes below and set up all the QOM boilerplate.

> On Thursday 25 of January 2018 14:58:41 Paolo Bonzini wrote:
>> On 23/01/2018 22:42, Pavel Pisa wrote:
>>> Do you think QOM based? I would like it to be implemented
>>> that way but I need some assistance where to look how this
>>> object kind should be implemented and from which base object
>>> inherit from. But I prefer to left that for later work.
>>>
>>> I would definitely like to use some mechanism which allows
>>> to get rid of externally visible pointer and need to assign
>>> it in the stub. It has been my initial idea and your review
>>> sumbled across this hack as well. But I need suggestion what
>>> is the preferred way for QEMU.
>>
>> The best way would be a QOM object.  That is, you would do
>>
>>    -object can-bus,id=canbus0
>>    -object can-host-socketcan,id=can0-host,canbus=canbus0,if=can0
>>    -device kvaser_pci,canbus=canbus0
> 
> I would prefer to allow CAN emulation to be used without
> explicit can-bus object creation specified on the command line.
> The bus object could be created when requested by
> can-host-socketcan or device (kvaser_pci) automatically.

It could be fine to allow "-device kvaser_pci" to automatically create a
private bus.  However, IMO it's better to be explicit in the case where
multiple objects (e.g. can-host-socketcan and the kvaser_pci device, or
two controllers) want to connect to the same object.

>> Separating the QOM objects is a bit more work, but it makes the
>> semantics clearer.  The classes would be:
>>
>> - can-bus and an abstract class can-host, which would inherit directly
>> from TYPE_OBJECT and implement TYPE_USER_CREATABLE
>>
>> - can-host-socketcan, which would inherit from can-host (and take the
>> TYPE_USER_CREATABLE implementation from there)
>>
>> while CanBusClientState and CanBusClientInfo need not be QOMified.
> 
> May it be, can-host-socketcan can be based directly on TYPE_OBJECT,
> because only QEMU CAN bus specific part is CanBusClientState which
> allows it to connect to the CAN bus same way as other CAN controllers
> connect to the bus.

The abstract class "can-host" is useful to keep can-host-socketcan free
of things that are not specific to SocketCAN, but it's just a small detail.

Paolo

Hello Paolo,

On Friday 26 of January 2018 12:12:32 Paolo Bonzini wrote:
> Coincidentially, I have some time on a flight next Monday. :)  I
> obviously cannot really _test_ anything, but I can at least do the
> changes below and set up all the QOM boilerplate.

thanks much for offer to help.

Deniz Eren or Oleksij Rempel can test your changes as well.

I have prepared branch "can-pci-qom" in GitLab repository

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-qom

and I have updated all PCI devices to use realize() instead of init(),
eliminated all uses of exit() and changed error reporting to use
error_setg() and error_setg_errno(). So I think that there is
no fatal blocker in these files. Problematic is setup
of connect_to_host variant

 can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);

in "hw/can/can_socketcan.c" and location of this file
in HW. I keep it there to have all CAN support *.c
files in single place for now.

I have studied "tests/check-qom-proplist.c" for while
but I expect that you will be much more successfull
and efficient to define mainline acceptable object model.

Thanks again,

Pavel

> >> The best way would be a QOM object.  That is, you would do
> >>
> >>    -object can-bus,id=canbus0
> >>    -object can-host-socketcan,id=can0-host,canbus=canbus0,if=can0
> >>    -device kvaser_pci,canbus=canbus0
> >
> > I would prefer to allow CAN emulation to be used without
> > explicit can-bus object creation specified on the command line.
> > The bus object could be created when requested by
> > can-host-socketcan or device (kvaser_pci) automatically.
>
> It could be fine to allow "-device kvaser_pci" to automatically create a
> private bus.  However, IMO it's better to be explicit in the case where
> multiple objects (e.g. can-host-socketcan and the kvaser_pci device, or
> two controllers) want to connect to the same object.
>
> >> Separating the QOM objects is a bit more work, but it makes the
> >> semantics clearer.  The classes would be:
> >>
> >> - can-bus and an abstract class can-host, which would inherit directly
> >> from TYPE_OBJECT and implement TYPE_USER_CREATABLE
> >>
> >> - can-host-socketcan, which would inherit from can-host (and take the
> >> TYPE_USER_CREATABLE implementation from there)
> >>
> >> while CanBusClientState and CanBusClientInfo need not be QOMified.
> >
> > May it be, can-host-socketcan can be based directly on TYPE_OBJECT,
> > because only QEMU CAN bus specific part is CanBusClientState which
> > allows it to connect to the CAN bus same way as other CAN controllers
> > connect to the bus.
>
> The abstract class "can-host" is useful to keep can-host-socketcan free
> of things that are not specific to SocketCAN, but it's just a small detail.

Hi,

On 28.01.2018 10:02, Pavel Pisa wrote:
> Hello Paolo,
> 
> On Friday 26 of January 2018 12:12:32 Paolo Bonzini wrote:
>> Coincidentially, I have some time on a flight next Monday. :)  I
>> obviously cannot really _test_ anything, but I can at least do the
>> changes below and set up all the QOM boilerplate.
> 
> thanks much for offer to help.
> 
> Deniz Eren or Oleksij Rempel can test your changes as well.

just tell me wen and what should i test :)

> I have prepared branch "can-pci-qom" in GitLab repository
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-qom
> 
> and I have updated all PCI devices to use realize() instead of init(),
> eliminated all uses of exit() and changed error reporting to use
> error_setg() and error_setg_errno(). So I think that there is
> no fatal blocker in these files. Problematic is setup
> of connect_to_host variant
> 
>  can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
> 
> in "hw/can/can_socketcan.c" and location of this file
> in HW. I keep it there to have all CAN support *.c
> files in single place for now.
> 
> I have studied "tests/check-qom-proplist.c" for while
> but I expect that you will be much more successfull
> and efficient to define mainline acceptable object model.
> 
> Thanks again,
> 
> Pavel
> 
>>>> The best way would be a QOM object.  That is, you would do
>>>>
>>>>    -object can-bus,id=canbus0
>>>>    -object can-host-socketcan,id=can0-host,canbus=canbus0,if=can0
>>>>    -device kvaser_pci,canbus=canbus0
>>>
>>> I would prefer to allow CAN emulation to be used without
>>> explicit can-bus object creation specified on the command line.
>>> The bus object could be created when requested by
>>> can-host-socketcan or device (kvaser_pci) automatically.
>>
>> It could be fine to allow "-device kvaser_pci" to automatically create a
>> private bus.  However, IMO it's better to be explicit in the case where
>> multiple objects (e.g. can-host-socketcan and the kvaser_pci device, or
>> two controllers) want to connect to the same object.
>>
>>>> Separating the QOM objects is a bit more work, but it makes the
>>>> semantics clearer.  The classes would be:
>>>>
>>>> - can-bus and an abstract class can-host, which would inherit directly
>>>> from TYPE_OBJECT and implement TYPE_USER_CREATABLE
>>>>
>>>> - can-host-socketcan, which would inherit from can-host (and take the
>>>> TYPE_USER_CREATABLE implementation from there)
>>>>
>>>> while CanBusClientState and CanBusClientInfo need not be QOMified.
>>>
>>> May it be, can-host-socketcan can be based directly on TYPE_OBJECT,
>>> because only QEMU CAN bus specific part is CanBusClientState which
>>> allows it to connect to the CAN bus same way as other CAN controllers
>>> connect to the bus.
>>
>> The abstract class "can-host" is useful to keep can-host-socketcan free
>> of things that are not specific to SocketCAN, but it's just a small detail.
> 
> 

On 25/01/2018 22:33, Pavel Pisa wrote:
> Hello Paolo,
> 
> thanks for suggestions. I understand and fully agree with your
> request to switch to QOM. I have succeed with that for CAN devices
> some time ago. It worth to be done for the rest of the objects
> but I fear that I do not find time to complete QOMification
> in reasonable future. Contributions/suggestions from other
> are welcomed. I can look for students for GSoC at our university
> or under other funding.

Please take a look at branch can-pci-qom of github.com/bonzini/qemu.git.
 Apart from QOMification of the backend include, I simplified the IRQ
handling in can_kvaser_pci (fixing bugs too I think), and removed an
unnecessary mutex.  I also moved the files to net/can and hw/net/can so
that in the future Jason (networking maintainer) can take care of pull
requests.

I might have broken something, and the top commit in particular is
completely untested.

Paolo

Hello Paolo,

thanks much for conversion to acceptable QOM model.

On Tuesday 30 of January 2018 15:15:22 Paolo Bonzini wrote:
> On 25/01/2018 22:33, Pavel Pisa wrote:
> > Hello Paolo,
> >
> > thanks for suggestions. I understand and fully agree with your
> > request to switch to QOM. I have succeed with that for CAN devices
> > some time ago. It worth to be done for the rest of the objects
> > but I fear that I do not find time to complete QOMification
> > in reasonable future. Contributions/suggestions from other
> > are welcomed. I can look for students for GSoC at our university
> > or under other funding.
>
> Please take a look at branch can-pci-qom of github.com/bonzini/qemu.git.
>  Apart from QOMification of the backend include, I simplified the IRQ
> handling in can_kvaser_pci (fixing bugs too I think), and removed an
> unnecessary mutex.  I also moved the files to net/can and hw/net/can so
> that in the future Jason (networking maintainer) can take care of pull
> requests.
>
> I might have broken something, and the top commit in particular is
> completely untested.

I have run basic test with Linux kernel on both sides
for one kavser_pci card on guest side and vcan (no real interface)
on host side.

Mesages exchange tests passed and looks OK.

I have used next parameters

      -object can-bus,id=canbus0 \
      -device kvaser_pci,canbus=canbus0 \
      -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan

The id parameter is required for "can-host-socketcan" object.
Else next error is printed

  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0: Parameter 'id' is missing

If "-object can-bus,id=canbus0" is missing then next error is reported

  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan: Device 'canbus0' not found

I have inspected through monitor the state of objects

  (qemu) qom-list /objects
  canbus0-socketcan (child<can-host-socketcan>)
  type (string)
  canbus0 (child<can-bus>)

  (qemu) info qom-tree
  /machine (pc-i440fx-2.12-machine)
    ...
    /peripheral-anon (container)
      /device[1] (kvaser_pci)
        /bus master[0] (qemu:memory-region)
        /kvaser_pci-xilinx[0] (qemu:memory-region)
        /kvaser_pci-s5920[0] (qemu:memory-region)
        /kvaser_pci-sja[0] (qemu:memory-region)
        /bus master container[0] (qemu:memory-region)
    ...


  (qemu) qom-list /objects
  canbus0-socketcan (child<can-host-socketcan>)
  type (string)
  canbus0 (child<can-bus>)

  (qemu) qom-list /machine/peripheral-anon/device[1]
  bus master container[0] (child<qemu:memory-region>)
  canbus (link<can-bus>)
  rombar (uint32)
  hotpluggable (bool)
  x-pcie-lnksta-dllla (bool)
  kvaser_pci-sja[0] (child<qemu:memory-region>)
  multifunction (bool)
  hotplugged (bool)
  parent_bus (link<bus>)
  romfile (str)
  kvaser_pci-s5920[0] (child<qemu:memory-region>)
  x-pcie-extcap-init (bool)
  command_serr_enable (bool)
  addr (int32)
  type (string)
  legacy-addr (str)
  kvaser_pci-xilinx[0] (child<qemu:memory-region>)
  realized (bool)
  bus master[0] (child<qemu:memory-region>)

From the user point of view, it would be nice if "can-bus"
can be populated when required automatically.

I am not sure, but may it be that it would worth to
push can-bus objects under some category/specific
container. The path /objects is quite wide.
Into something like /object/can-bus or /net/can.

But generally thanks much, the progress you have made
in one day is really great. I hope that others check
your branch. I have pushed your unmodified version into
"can-pci-qom" branch of my repo

  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-qom

It would be great if others can check that everything
works in their setup. I think that then it can be pushed
into mainline and some usability improvements can be
done/experiment with later.

Thanks much,


                Pavel Pisa

Hi Pavel, Paolo,

I tried to rerun my environment to test however it seems the interface has changed a little and my standard program options cause complaints. Unfortunately I don’t have too much time to dig through at the moment.

My standard startup command is:

$ ./qemu-local/bin/qemu-system-i386 -hda sdd2gb-uno1483-16.04-2.0-dev.img -boot d -k en-us -device mioe3680_pci,canbus1=canbus0,host1=vcan0,canbus2=canbus1,host2=vcan1 -m size=2048 -netdev user,id=user.0 -device e1000,netdev=user.0 -redir tcp:5022::22 -enable-kvm &



Best regards,
Deniz

Sent from my iPhone

Deniz Eren
+61 400 307 762

> On 31 Jan 2018, at 9:12 am, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> 
> Hello Paolo,
> 
> thanks much for conversion to acceptable QOM model.
> 
>> On Tuesday 30 of January 2018 15:15:22 Paolo Bonzini wrote:
>>> On 25/01/2018 22:33, Pavel Pisa wrote:
>>> Hello Paolo,
>>> 
>>> thanks for suggestions. I understand and fully agree with your
>>> request to switch to QOM. I have succeed with that for CAN devices
>>> some time ago. It worth to be done for the rest of the objects
>>> but I fear that I do not find time to complete QOMification
>>> in reasonable future. Contributions/suggestions from other
>>> are welcomed. I can look for students for GSoC at our university
>>> or under other funding.
>> 
>> Please take a look at branch can-pci-qom of github.com/bonzini/qemu.git.
>> Apart from QOMification of the backend include, I simplified the IRQ
>> handling in can_kvaser_pci (fixing bugs too I think), and removed an
>> unnecessary mutex.  I also moved the files to net/can and hw/net/can so
>> that in the future Jason (networking maintainer) can take care of pull
>> requests.
>> 
>> I might have broken something, and the top commit in particular is
>> completely untested.
> 
> I have run basic test with Linux kernel on both sides
> for one kavser_pci card on guest side and vcan (no real interface)
> on host side.
> 
> Mesages exchange tests passed and looks OK.
> 
> I have used next parameters
> 
>      -object can-bus,id=canbus0 \
>      -device kvaser_pci,canbus=canbus0 \
>      -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan
> 
> The id parameter is required for "can-host-socketcan" object.
> Else next error is printed
> 
>  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0: Parameter 'id' is missing
> 
> If "-object can-bus,id=canbus0" is missing then next error is reported
> 
>  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan: Device 'canbus0' not found
> 
> I have inspected through monitor the state of objects
> 
>  (qemu) qom-list /objects
>  canbus0-socketcan (child<can-host-socketcan>)
>  type (string)
>  canbus0 (child<can-bus>)
> 
>  (qemu) info qom-tree
>  /machine (pc-i440fx-2.12-machine)
>    ...
>    /peripheral-anon (container)
>      /device[1] (kvaser_pci)
>        /bus master[0] (qemu:memory-region)
>        /kvaser_pci-xilinx[0] (qemu:memory-region)
>        /kvaser_pci-s5920[0] (qemu:memory-region)
>        /kvaser_pci-sja[0] (qemu:memory-region)
>        /bus master container[0] (qemu:memory-region)
>    ...
> 
> 
>  (qemu) qom-list /objects
>  canbus0-socketcan (child<can-host-socketcan>)
>  type (string)
>  canbus0 (child<can-bus>)
> 
>  (qemu) qom-list /machine/peripheral-anon/device[1]
>  bus master container[0] (child<qemu:memory-region>)
>  canbus (link<can-bus>)
>  rombar (uint32)
>  hotpluggable (bool)
>  x-pcie-lnksta-dllla (bool)
>  kvaser_pci-sja[0] (child<qemu:memory-region>)
>  multifunction (bool)
>  hotplugged (bool)
>  parent_bus (link<bus>)
>  romfile (str)
>  kvaser_pci-s5920[0] (child<qemu:memory-region>)
>  x-pcie-extcap-init (bool)
>  command_serr_enable (bool)
>  addr (int32)
>  type (string)
>  legacy-addr (str)
>  kvaser_pci-xilinx[0] (child<qemu:memory-region>)
>  realized (bool)
>  bus master[0] (child<qemu:memory-region>)
> 
> From the user point of view, it would be nice if "can-bus"
> can be populated when required automatically.
> 
> I am not sure, but may it be that it would worth to
> push can-bus objects under some category/specific
> container. The path /objects is quite wide.
> Into something like /object/can-bus or /net/can.
> 
> But generally thanks much, the progress you have made
> in one day is really great. I hope that others check
> your branch. I have pushed your unmodified version into
> "can-pci-qom" branch of my repo
> 
>  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-qom
> 
> It would be great if others can check that everything
> works in their setup. I think that then it can be pushed
> into mainline and some usability improvements can be
> done/experiment with later.
> 
> Thanks much,
> 
> 
>                Pavel Pisa

On 30/01/2018 19:13, Deniz Eren wrote:
> Hi Pavel, Paolo,
> 
> I tried to rerun my environment to test however it seems the interface has changed a little and my standard program options cause complaints. Unfortunately I don’t have too much time to dig through at the moment.
> 
> My standard startup command is:
> 
> $ ./qemu-local/bin/qemu-system-i386 -hda sdd2gb-uno1483-16.04-2.0-dev.img -boot d -k en-us -device mioe3680_pci,canbus1=canbus0,host1=vcan0,canbus2=canbus1,host2=vcan1 -m size=2048 -netdev user,id=user.0 -device e1000,netdev=user.0 -redir tcp:5022::22 -enable-kvm &

Yep, it's now like this:

./qemu-local/bin/qemu-system-i386 \
  -hda sdd2gb-uno1483-16.04-2.0-dev.img -boot d -k en-us \
  -object can-bus,id=canbus0 \
  -object can-bus,id=canbus1 \
  -object can-host-socketcan,id=canhost0,canbus=canbus0,ifname=vcan0 \
  -object can-host-socketcan,id=canhost1,canbus=canbus1,ifname=vcan1 \
  -device mioe3680_pci,canbus0=canbus0,canbus1=canbus1 \
  -m size=2048 -netdev user,id=user.0 -device e1000,netdev=user.0 \
  -redir tcp:5022::22 -enable-kvm

Thanks,

Paolo

> 
> 
> 
> Best regards,
> Deniz
> 
> Sent from my iPhone
> 
> Deniz Eren
> +61 400 307 762
> 
>> On 31 Jan 2018, at 9:12 am, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>>
>> Hello Paolo,
>>
>> thanks much for conversion to acceptable QOM model.
>>
>>> On Tuesday 30 of January 2018 15:15:22 Paolo Bonzini wrote:
>>>> On 25/01/2018 22:33, Pavel Pisa wrote:
>>>> Hello Paolo,
>>>>
>>>> thanks for suggestions. I understand and fully agree with your
>>>> request to switch to QOM. I have succeed with that for CAN devices
>>>> some time ago. It worth to be done for the rest of the objects
>>>> but I fear that I do not find time to complete QOMification
>>>> in reasonable future. Contributions/suggestions from other
>>>> are welcomed. I can look for students for GSoC at our university
>>>> or under other funding.
>>>
>>> Please take a look at branch can-pci-qom of github.com/bonzini/qemu.git.
>>> Apart from QOMification of the backend include, I simplified the IRQ
>>> handling in can_kvaser_pci (fixing bugs too I think), and removed an
>>> unnecessary mutex.  I also moved the files to net/can and hw/net/can so
>>> that in the future Jason (networking maintainer) can take care of pull
>>> requests.
>>>
>>> I might have broken something, and the top commit in particular is
>>> completely untested.
>>
>> I have run basic test with Linux kernel on both sides
>> for one kavser_pci card on guest side and vcan (no real interface)
>> on host side.
>>
>> Mesages exchange tests passed and looks OK.
>>
>> I have used next parameters
>>
>>      -object can-bus,id=canbus0 \
>>      -device kvaser_pci,canbus=canbus0 \
>>      -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan
>>
>> The id parameter is required for "can-host-socketcan" object.
>> Else next error is printed
>>
>>  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0: Parameter 'id' is missing
>>
>> If "-object can-bus,id=canbus0" is missing then next error is reported
>>
>>  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan: Device 'canbus0' not found
>>
>> I have inspected through monitor the state of objects
>>
>>  (qemu) qom-list /objects
>>  canbus0-socketcan (child<can-host-socketcan>)
>>  type (string)
>>  canbus0 (child<can-bus>)
>>
>>  (qemu) info qom-tree
>>  /machine (pc-i440fx-2.12-machine)
>>    ...
>>    /peripheral-anon (container)
>>      /device[1] (kvaser_pci)
>>        /bus master[0] (qemu:memory-region)
>>        /kvaser_pci-xilinx[0] (qemu:memory-region)
>>        /kvaser_pci-s5920[0] (qemu:memory-region)
>>        /kvaser_pci-sja[0] (qemu:memory-region)
>>        /bus master container[0] (qemu:memory-region)
>>    ...
>>
>>
>>  (qemu) qom-list /objects
>>  canbus0-socketcan (child<can-host-socketcan>)
>>  type (string)
>>  canbus0 (child<can-bus>)
>>
>>  (qemu) qom-list /machine/peripheral-anon/device[1]
>>  bus master container[0] (child<qemu:memory-region>)
>>  canbus (link<can-bus>)
>>  rombar (uint32)
>>  hotpluggable (bool)
>>  x-pcie-lnksta-dllla (bool)
>>  kvaser_pci-sja[0] (child<qemu:memory-region>)
>>  multifunction (bool)
>>  hotplugged (bool)
>>  parent_bus (link<bus>)
>>  romfile (str)
>>  kvaser_pci-s5920[0] (child<qemu:memory-region>)
>>  x-pcie-extcap-init (bool)
>>  command_serr_enable (bool)
>>  addr (int32)
>>  type (string)
>>  legacy-addr (str)
>>  kvaser_pci-xilinx[0] (child<qemu:memory-region>)
>>  realized (bool)
>>  bus master[0] (child<qemu:memory-region>)
>>
>> From the user point of view, it would be nice if "can-bus"
>> can be populated when required automatically.
>>
>> I am not sure, but may it be that it would worth to
>> push can-bus objects under some category/specific
>> container. The path /objects is quite wide.
>> Into something like /object/can-bus or /net/can.
>>
>> But generally thanks much, the progress you have made
>> in one day is really great. I hope that others check
>> your branch. I have pushed your unmodified version into
>> "can-pci-qom" branch of my repo
>>
>>  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-qom
>>
>> It would be great if others can check that everything
>> works in their setup. I think that then it can be pushed
>> into mainline and some usability improvements can be
>> done/experiment with later.
>>
>> Thanks much,
>>
>>
>>                Pavel Pisa

On 30/01/2018 20:08, Paolo Bonzini wrote:
> On 30/01/2018 19:13, Deniz Eren wrote:
>> Hi Pavel, Paolo,
>>
>> I tried to rerun my environment to test however it seems the interface has changed a little and my standard program options cause complaints. Unfortunately I don’t have too much time to dig through at the moment.
>>
>> My standard startup command is:
>>
>> $ ./qemu-local/bin/qemu-system-i386 -hda sdd2gb-uno1483-16.04-2.0-dev.img -boot d -k en-us -device mioe3680_pci,canbus1=canbus0,host1=vcan0,canbus2=canbus1,host2=vcan1 -m size=2048 -netdev user,id=user.0 -device e1000,netdev=user.0 -redir tcp:5022::22 -enable-kvm &
> 
> Yep, it's now like this:
> 
> ./qemu-local/bin/qemu-system-i386 \
>   -hda sdd2gb-uno1483-16.04-2.0-dev.img -boot d -k en-us \
>   -object can-bus,id=canbus0 \
>   -object can-bus,id=canbus1 \
>   -object can-host-socketcan,id=canhost0,canbus=canbus0,ifname=vcan0 \
>   -object can-host-socketcan,id=canhost1,canbus=canbus1,ifname=vcan1 \
>   -device mioe3680_pci,canbus0=canbus0,canbus1=canbus1 \
>   -m size=2048 -netdev user,id=user.0 -device e1000,netdev=user.0 \
>   -redir tcp:5022::22 -enable-kvm

Sorry, all "ifname" are "if".

Paolo

> Thanks,
> 
> Paolo
> 
>>
>>
>>
>> Best regards,
>> Deniz
>>
>> Sent from my iPhone
>>
>> Deniz Eren
>> +61 400 307 762
>>
>>> On 31 Jan 2018, at 9:12 am, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
>>>
>>> Hello Paolo,
>>>
>>> thanks much for conversion to acceptable QOM model.
>>>
>>>> On Tuesday 30 of January 2018 15:15:22 Paolo Bonzini wrote:
>>>>> On 25/01/2018 22:33, Pavel Pisa wrote:
>>>>> Hello Paolo,
>>>>>
>>>>> thanks for suggestions. I understand and fully agree with your
>>>>> request to switch to QOM. I have succeed with that for CAN devices
>>>>> some time ago. It worth to be done for the rest of the objects
>>>>> but I fear that I do not find time to complete QOMification
>>>>> in reasonable future. Contributions/suggestions from other
>>>>> are welcomed. I can look for students for GSoC at our university
>>>>> or under other funding.
>>>>
>>>> Please take a look at branch can-pci-qom of github.com/bonzini/qemu.git.
>>>> Apart from QOMification of the backend include, I simplified the IRQ
>>>> handling in can_kvaser_pci (fixing bugs too I think), and removed an
>>>> unnecessary mutex.  I also moved the files to net/can and hw/net/can so
>>>> that in the future Jason (networking maintainer) can take care of pull
>>>> requests.
>>>>
>>>> I might have broken something, and the top commit in particular is
>>>> completely untested.
>>>
>>> I have run basic test with Linux kernel on both sides
>>> for one kavser_pci card on guest side and vcan (no real interface)
>>> on host side.
>>>
>>> Mesages exchange tests passed and looks OK.
>>>
>>> I have used next parameters
>>>
>>>      -object can-bus,id=canbus0 \
>>>      -device kvaser_pci,canbus=canbus0 \
>>>      -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan
>>>
>>> The id parameter is required for "can-host-socketcan" object.
>>> Else next error is printed
>>>
>>>  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0: Parameter 'id' is missing
>>>
>>> If "-object can-bus,id=canbus0" is missing then next error is reported
>>>
>>>  qemu-system-x86_64: -object can-host-socketcan,if=can0,canbus=canbus0,id=canbus0-socketcan: Device 'canbus0' not found
>>>
>>> I have inspected through monitor the state of objects
>>>
>>>  (qemu) qom-list /objects
>>>  canbus0-socketcan (child<can-host-socketcan>)
>>>  type (string)
>>>  canbus0 (child<can-bus>)
>>>
>>>  (qemu) info qom-tree
>>>  /machine (pc-i440fx-2.12-machine)
>>>    ...
>>>    /peripheral-anon (container)
>>>      /device[1] (kvaser_pci)
>>>        /bus master[0] (qemu:memory-region)
>>>        /kvaser_pci-xilinx[0] (qemu:memory-region)
>>>        /kvaser_pci-s5920[0] (qemu:memory-region)
>>>        /kvaser_pci-sja[0] (qemu:memory-region)
>>>        /bus master container[0] (qemu:memory-region)
>>>    ...
>>>
>>>
>>>  (qemu) qom-list /objects
>>>  canbus0-socketcan (child<can-host-socketcan>)
>>>  type (string)
>>>  canbus0 (child<can-bus>)
>>>
>>>  (qemu) qom-list /machine/peripheral-anon/device[1]
>>>  bus master container[0] (child<qemu:memory-region>)
>>>  canbus (link<can-bus>)
>>>  rombar (uint32)
>>>  hotpluggable (bool)
>>>  x-pcie-lnksta-dllla (bool)
>>>  kvaser_pci-sja[0] (child<qemu:memory-region>)
>>>  multifunction (bool)
>>>  hotplugged (bool)
>>>  parent_bus (link<bus>)
>>>  romfile (str)
>>>  kvaser_pci-s5920[0] (child<qemu:memory-region>)
>>>  x-pcie-extcap-init (bool)
>>>  command_serr_enable (bool)
>>>  addr (int32)
>>>  type (string)
>>>  legacy-addr (str)
>>>  kvaser_pci-xilinx[0] (child<qemu:memory-region>)
>>>  realized (bool)
>>>  bus master[0] (child<qemu:memory-region>)
>>>
>>> From the user point of view, it would be nice if "can-bus"
>>> can be populated when required automatically.
>>>
>>> I am not sure, but may it be that it would worth to
>>> push can-bus objects under some category/specific
>>> container. The path /objects is quite wide.
>>> Into something like /object/can-bus or /net/can.
>>>
>>> But generally thanks much, the progress you have made
>>> in one day is really great. I hope that others check
>>> your branch. I have pushed your unmodified version into
>>> "can-pci-qom" branch of my repo
>>>
>>>  https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-qom
>>>
>>> It would be great if others can check that everything
>>> works in their setup. I think that then it can be pushed
>>> into mainline and some usability improvements can be
>>> done/experiment with later.
>>>
>>> Thanks much,
>>>
>>>
>>>                Pavel Pisa
>