On Fri, 15 Aug 2025 at 10:01, Corvin Köhne <corvin.koe...@gmail.com> wrote:
>
> From: YannickV <y.vos...@beckhoff.com>
>
> This adds the Beckhoff Communication Controller (CCAT). The information
> block, EEPROM interface and DMA controller are currently implemented.
>
> The EEPROM provides production information for Beckhoff Devices.
> An EEPORM binary must therefor be handed over. It should be aligned to
> a power of two. If no EEPROM binary is handed over an empty EEPROM of
> size 4096 is initialized.
>
> This device is needed for the Beckhoff CX7200 board emulation.
>
> Signed-off-by: Yannick Voßen <y.vos...@beckhoff.com>
> ---
> hw/misc/Kconfig | 3 +
> hw/misc/beckhoff_ccat.c | 365 ++++++++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 1 +
> 3 files changed, 369 insertions(+)
> create mode 100644 hw/misc/beckhoff_ccat.c
>
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 99548e146f..f3a2efa350 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -223,4 +223,7 @@ config XLNX_VERSAL_TRNG
> config XLNX_ZYNQ_DDRC
> bool
>
> +config BECKHOFF_CCAT
> + bool
> +
> source macio/Kconfig
> diff --git a/hw/misc/beckhoff_ccat.c b/hw/misc/beckhoff_ccat.c
> new file mode 100644
> index 0000000000..0e21962a98
> --- /dev/null
> +++ b/hw/misc/beckhoff_ccat.c
> @@ -0,0 +1,365 @@
> +/*
> + * Beckhoff Communication Controller Emulation
> + *
> + * Copyright (c) Beckhoff Automation GmbH. & Co. KG
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
This needs an SPDX license identifier line. If you have that,
then you don't need the human-readable license blurb.
Is there a datasheet/manual for this device? The comment
at the top of the file is a good place to put the URL to it.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "hw/register.h"
> +#include "qemu/bitops.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "system/block-backend.h"
> +#include "exec/address-spaces.h"
> +#include "exec/memory.h"
> +#include "system/dma.h"
> +#include "qemu/error-report.h"
> +#include "block/block.h"
> +#include "block/block_int.h"
> +#include "block/qdict.h"
> +#include "hw/block/block.h"
> +
> +#ifndef CCAT_ERR_DEBUG
> +#define CCAT_ERR_DEBUG 0
> +#endif
> +
> +#define DB_PRINT_L(level, ...) do { \
> + if (CCAT_ERR_DEBUG > (level)) { \
> + fprintf(stderr, ": %s: ", __func__); \
> + fprintf(stderr, ## __VA_ARGS__); \
> + } \
> +} while (0)
No debug printf macros in new code, please. Use either:
* tracepoints, for "this is information that might be
interesting for somebody debugging the device or code
that runs on the device"
* qemu_log_mask(LOG_UNIMP, ...), for "the guest tried
to use a feature that we don't implement"
* qemu_log_mask(LOG_GUEST_ERROR, ...), for "the guest
tried to do something that the datasheet says is
undefined behaviour/reserved/unpredictable"
> +
> +#define DB_PRINT(...) DB_PRINT_L(0, ## __VA_ARGS__)
> +
> +#define TYPE_BECKHOFF_CCAT "beckhoff-ccat"
> +#define BECKHOFF_CCAT(obj) \
> + OBJECT_CHECK(BeckhoffCcat, (obj), TYPE_BECKHOFF_CCAT)
Prefer the new-style OBJECT_DECLARE_SIMPLE_TYPE etc
macros, rather than defining the cast macro yourself.
> +
> +#define MAX_NUM_SLOTS 32
> +
> +#define CCAT_EEPROM_OFFSET 0x100
> +#define CCAT_DMA_OFFSET 0x8000
> +
> +#define CCAT_MEM_SIZE 0xFFFF
> +#define CCAT_DMA_SIZE 0x800
> +#define CCAT_EEPROM_SIZE 0x20
> +
> +#define EEPROM_MEMORY_SIZE 0x1000
> +
> +#define EEPROM_CMD_OFFSET (CCAT_EEPROM_OFFSET + 0x00)
> + #define EEPROM_CMD_WRITE_MASK 0x2
> + #define EEPROM_CMD_READ_MASK 0x1
> +#define EEPROM_ADR_OFFSET (CCAT_EEPROM_OFFSET + 0x04)
> +#define EEPROM_DATA_OFFSET (CCAT_EEPROM_OFFSET + 0x08)
> +
> +#define DMA_BUFFER_OFFSET (CCAT_DMA_OFFSET + 0x00)
> +#define DMA_DIRECTION_OFFSET (CCAT_DMA_OFFSET + 0x7c0)
> + #define DMA_DIRECTION_MASK 1
> +#define DMA_TRANSFER_OFFSET (CCAT_DMA_OFFSET + 0x7c4)
> +#define DMA_HOST_ADR_OFFSET (CCAT_DMA_OFFSET + 0x7c8)
> +#define DMA_TRANSFER_LENGTH_OFFSET (CCAT_DMA_OFFSET + 0x7cc)
> +
> +/*
> + * The informationblock is always located at address 0x0.
> + * Address and size are therefor replaced by two identifiers.
> + * The Parameter give information about the maximal number of
> + * function slots and the creation date (in this case 01.01.2001)
> + */
> +#define CCAT_ID_1 0x88a4
> +#define CCAT_ID_2 0x54414343
> +#define CCAT_INFO_BLOCK_PARAMS (MAX_NUM_SLOTS << 0) | (0x1 << 8) | \
> + (0x1 << 16) | (0x1 << 24)
> +
> +#define CCAT_FUN_TYPE_ENTRY 0x0001
> +#define CCAT_FUN_TYPE_EEPROM 0x0012
> +#define CCAT_FUN_TYPE_DMA 0x0013
> +
> +typedef struct BeckhoffCcat {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion iomem;
> +
> + uint8_t mem[CCAT_MEM_SIZE];
> +
> + BlockBackend *eeprom_blk;
> + uint8_t *eeprom_storage;
> + int64_t eeprom_size;
> +} BeckhoffCcat;
> +
> +typedef struct __attribute__((packed)) CcatFunctionBlock {
> + uint16_t type;
> + uint16_t revision;
> + uint32_t parameter;
> + uint32_t address_offset;
> + uint32_t size;
> +} CcatFunctionBlock;
"attribute((packed))" is a bit of a red flag of doing something
wrong. In this case, you use this in beckhoff_ccat_reset() to
set up a host C structure that you memcpy() into s->mem[].
That is not portable to big-endian systems.
> +
> +static void sync_eeprom(BeckhoffCcat *s)
> +{
> + if (!s->eeprom_blk) {
> + return;
> + }
> + blk_pwrite(s->eeprom_blk, 0, s->eeprom_size, s->eeprom_storage, 0);
> +}
> +
> +static uint64_t beckhoff_ccat_eeprom_read(void *opaque, hwaddr addr,
> + unsigned size)
> +{
> + BeckhoffCcat *s = opaque;
> + uint64_t val = 0;
> + memcpy(&val, &s->mem[addr], size);
What is this trying to do ? If you want to read
N bytes of data in little endian order from a host
address, use ldn_le_p(). (Also available in "big endian"
and "host endianness" flavours.)
> + return val;
> +}
> +
> +static void beckhoff_ccat_eeprom_write(void *opaque, hwaddr addr, uint64_t
> val,
> + unsigned size)
> +{
> + BeckhoffCcat *s = opaque;
> + uint64_t eeprom_adr;
> + switch (addr) {
> + case EEPROM_CMD_OFFSET:
> + eeprom_adr = *(uint32_t *)&s->mem[EEPROM_ADR_OFFSET];
> + eeprom_adr = (eeprom_adr * 2) % s->eeprom_size;
> + if (val & EEPROM_CMD_READ_MASK) {
> + uint64_t buf = 0;
> + uint32_t bytes_to_read = 8;
> + if (eeprom_adr > s->eeprom_size - 8) {
> + bytes_to_read = s->eeprom_size - eeprom_adr;
> + }
> + memcpy(&buf, s->eeprom_storage + eeprom_adr, bytes_to_read);
> + *(uint64_t *)&s->mem[EEPROM_DATA_OFFSET] = buf;
All this code with casting to different pointer types and
using memcpy() looks very suspicious. Use the relevant
ld*_*_p() functions to load the right sized values at the
right endianness. (Casting from one pointer type to another
is usually an indication that you're going down a wrong path.)
> +
> + } else if (val & EEPROM_CMD_WRITE_MASK) {
> + uint32_t buf = *(uint32_t *)&s->mem[EEPROM_DATA_OFFSET];
> + memcpy(s->eeprom_storage + eeprom_adr, &buf, 2);
> + sync_eeprom(s);
> + }
> + break;
> + default:
> + memcpy(&s->mem[addr], &val, size);
> + }
> +}
> +
> +static uint64_t beckhoff_ccat_dma_read(void *opaque, hwaddr addr, unsigned
> size)
> +{
> + BeckhoffCcat *s = opaque;
> + uint64_t val = 0;
> +
> + switch (addr) {
> + case DMA_TRANSFER_OFFSET:
> + if (s->mem[DMA_TRANSFER_OFFSET] & 0x1) {
> + DB_PRINT("DMA transfer finished\n");
> + s->mem[DMA_TRANSFER_OFFSET] = 0;
> + }
> + break;
> + }
> + memcpy(&val, &s->mem[addr], size);
> + return val;
> +}
> +
> +static void beckhoff_ccat_dma_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> + BeckhoffCcat *s = opaque;
> + switch (addr) {
> + case DMA_TRANSFER_OFFSET:
> + uint8_t len = s->mem[DMA_TRANSFER_LENGTH_OFFSET];
> + uint8_t *mem_buf = &s->mem[DMA_BUFFER_OFFSET];
> +
> + if (s->mem[DMA_DIRECTION_OFFSET] & DMA_DIRECTION_MASK) {
> + dma_addr_t dmaAddr = *(uint32_t *)&s->mem[DMA_HOST_ADR_OFFSET];
Should probably be ldl_le_p().
> + dma_memory_read(&address_space_memory, dmaAddr,
> + mem_buf, len * 8, MEMTXATTRS_UNSPECIFIED);
> + } else {
> + dma_addr_t dmaAddr = *(uint32_t *)&s->mem[DMA_HOST_ADR_OFFSET];
> + dma_memory_write(&address_space_memory, dmaAddr + 8,
> + mem_buf, len * 8, MEMTXATTRS_UNSPECIFIED);
Does this really use addr for reads and addr + 8 for writes ?
> + }
> + break;
> + }
> + memcpy(&s->mem[addr], &val, size);
> +}
> +static uint64_t beckhoff_ccat_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + DB_PRINT("CCAT_READ addr=0x%lx size=%u\n", addr, size);
> +
> + BeckhoffCcat *s = opaque;
> + uint64_t val = 0;
> +
> + if (addr > CCAT_MEM_SIZE - size) {
> + error_report("Overflow. Address or size is too large.\n");
> + exit(1);
Don't error_report() and exit for things like this.
Either:
(a) this is not possible unless there's a bug in QEMU:
in that case, use assert()
(b) this is possible if the guest does something silly:
in that case use qemu_log_mask(LOG_GUEST_ERROR, ...)
In this case it looks like an assert() is appropriate, because
the memory_region_init_io() size argument should ensure that
you can't get here with an addr + size that is beyond the
end of the block.
> + }
> +
> + if (addr >= CCAT_EEPROM_OFFSET &&
> + addr <= CCAT_EEPROM_OFFSET + s->eeprom_size) {
> + return beckhoff_ccat_eeprom_read(opaque, addr, size);
> + } else if (addr >= CCAT_DMA_OFFSET &&
> + addr <= CCAT_DMA_OFFSET + CCAT_DMA_SIZE) {
> + return beckhoff_ccat_dma_read(opaque, addr, size);
> + } else {
> + memcpy(&val, &s->mem[addr], size);
> + }
> +
> + return val;
> +}
> +
> +static void beckhoff_ccat_write(void *opaque, hwaddr addr, uint64_t val,
> + unsigned size)
> +{
> + DB_PRINT("CCAT_WRITE addr=0x%lx size=%u val=0x%lx\n", addr, size, val);
> +
> + BeckhoffCcat *s = opaque;
> +
> + if (addr > CCAT_MEM_SIZE - size) {
> + error_report("Overflow. Address or size is too large.\n");
> + exit(1);
> + }
> +
> + if (addr >= CCAT_EEPROM_OFFSET &&
> + addr <= CCAT_EEPROM_OFFSET + s->eeprom_size) {
> + beckhoff_ccat_eeprom_write(opaque, addr, val, size);
> + } else if (addr >= CCAT_DMA_OFFSET &&
> + addr <= CCAT_DMA_OFFSET + CCAT_DMA_SIZE) {
> + beckhoff_ccat_dma_write(opaque, addr, val, size);
> + } else {
> + memcpy(&s->mem[addr], &val, size);
> + }
> +}
> +
> +static const MemoryRegionOps beckhoff_ccat_ops = {
> + .read = beckhoff_ccat_read,
> + .write = beckhoff_ccat_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .valid = {
> + .min_access_size = 1,
> + .max_access_size = 8,
> + },
> +};
> +
> +
> +static void beckhoff_ccat_reset(DeviceState *dev)
> +{
> + BeckhoffCcat *s = BECKHOFF_CCAT(dev);
> +
> + CcatFunctionBlock function_blocks[MAX_NUM_SLOTS] = {0};
> +
> + CcatFunctionBlock info_block = {
> + .type = CCAT_FUN_TYPE_ENTRY,
> + .revision = 0x0001,
> + .parameter = CCAT_INFO_BLOCK_PARAMS,
> + .address_offset = CCAT_ID_1,
> + .size = CCAT_ID_2
> + };
> + CcatFunctionBlock eeprom_block = {
> + .type = CCAT_FUN_TYPE_EEPROM,
> + .revision = 0x0001,
> + .parameter = 0,
> + .address_offset = CCAT_EEPROM_OFFSET,
> + .size = CCAT_EEPROM_SIZE
> + };
> + CcatFunctionBlock dma_block = {
> + .type = CCAT_FUN_TYPE_DMA,
> + .revision = 0x0000,
> + .parameter = 0,
> + .address_offset = CCAT_DMA_OFFSET,
> + .size = CCAT_DMA_SIZE
> + };
> +
> + /*
> + * The EEPROM interface is usually at function slot 11.
> + * The DMA controller is usually at function slot 15.
> + */
> + function_blocks[0] = info_block;
> + function_blocks[11] = eeprom_block;
> + function_blocks[15] = dma_block;
> +
> + memcpy(&s->mem[0], function_blocks, sizeof(function_blocks));
This isn't going to work on big-endian hosts, as noted above.
> +}
> +
> +static void beckhoff_ccat_realize(DeviceState *dev, Error **errp)
> +{
> + BeckhoffCcat *s = BECKHOFF_CCAT(dev);
> + BlockBackend *blk;
> +
> + blk = blk_by_name("ccat-eeprom");
No other device calls blk_by_name() to get its block backend:
you should not do so either. Use a qdev property and have
the SoC/board model set it.
> +
> + if (blk) {
> + uint64_t blk_size = blk_getlength(blk);
> + if (!is_power_of_2(blk_size)) {
> + error_report("Blockend size is not a power of two.");
In a realize function you have an Error** argument, so you
should report errors via error_setg() + return.
> + }
> +
> + if (blk_size < 512) {
> + error_report("Blockend size is too small. Using backup.");
> + s->eeprom_size = EEPROM_MEMORY_SIZE;
> + s->eeprom_storage = blk_blockalign(NULL, s->eeprom_size);
> + memset(s->eeprom_storage, 0x00, s->eeprom_size);
We don't try to fix things up like this in other block-backed
devices, I think, so we shouldn't do it here either.
> + } else {
> + DB_PRINT("EEPROM block backend found\n");
> + blk_set_perm(blk, BLK_PERM_WRITE, BLK_PERM_ALL, errp);
> +
> + s->eeprom_size = blk_size;
> + s->eeprom_blk = blk;
> + s->eeprom_storage = blk_blockalign(s->eeprom_blk,
> s->eeprom_size);
> +
> + if (!blk_check_size_and_read_all(s->eeprom_blk, DEVICE(s),
> + s->eeprom_storage,
> s->eeprom_size,
> + errp)) {
> + exit(1);
Don't silently exit(): use 'return', to report the error
to your caller.
> + }
> + }
> + } else {
> + s->eeprom_size = EEPROM_MEMORY_SIZE;
> + s->eeprom_storage = blk_blockalign(NULL, s->eeprom_size);
> + memset(s->eeprom_storage, 0x00, s->eeprom_size);
> + }
> +
> + beckhoff_ccat_reset(dev);
> +}
> +
> +static void beckhoff_ccat_init(Object *obj)
> +{
> + BeckhoffCcat *s = BECKHOFF_CCAT(obj);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->iomem, obj, &beckhoff_ccat_ops, s,
> + TYPE_BECKHOFF_CCAT, CCAT_MEM_SIZE);
> + sysbus_init_mmio(sbd, &s->iomem);
> +}
> +
> +static void beckhoff_ccat_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + dc->realize = beckhoff_ccat_realize;
The guest can read and write mem[], so I think you need
a reset handler, and also a VMState for migration.
> +}
thanks
-- PMM
