From: Stefan Stanacar <stefanst@google.com>
From: Stefan Stanacar <stefanst@google.com>
The CMSIS System View Description format(CMSIS-SVD) is an XML based
description of Arm Cortex-M microcontrollers provided and maintained
by sillicon vendors. It includes details such as peripherals registers
(down to bitfields), peripheral register block addresses, reset
values, etc.
This script uses this information to create header files that makes it
easier to emulate peripherals.
The script can be used to create either peripheral specific headers or
board / system specific information. The script generated headers are
similar to the SVDConv utility.
Peripheral specific headers contains information such as register
layout, register names and reset values for registers:
typedef struct {
...
union {
uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
* Flexcomm module ID */
struct {
uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
uint32_t : 4;
uint32_t ID : 20; /* [31..12] Flexcomm ID */
} PSELID_b;
};
...
} FLEXCOMM_Type; /* Size = 4096 (0x1000) */
#define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
#define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
#define FLEXCOMM_PSELID_LOCK_Pos (3UL)
#define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
...
typedef enum { /* FLEXCOMM_PSELID_LOCK */
/* Peripheral select can be changed by software. */
FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
/* Peripheral select is locked and cannot be changed until this
* Flexcomm module or the entire device is reset. */
FLEXCOMM_PSELID_LOCK_LOCKED = 1,
} FLEXCOMM_PSELID_LOCK_Enum;
...
#define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
const char *_name[sizeof(FLEXCOMM_Type)] = { \
[4088 ... 4091] = "PSELID", \
[4092 ... 4095] = "PID", \
}
Board specific headers contains information about peripheral base
register addresses.
Signed-off-by: Stefan Stanacar <stefanst@google.com>
Signed-off-by: Octavian Purdila <tavip@google.com>
---
configure | 2 +-
meson.build | 4 +
python/setup.cfg | 1 +
python/tests/minreqs.txt | 3 +
pythondeps.toml | 3 +
scripts/svd-gen-header.py | 342 ++++++++++++++++++++++++++++++++++++++
6 files changed, 354 insertions(+), 1 deletion(-)
create mode 100755 scripts/svd-gen-header.py
diff --git a/configure b/configure
index 5ad1674ca5..811bfa5d54 100755
--- a/configure
+++ b/configure
@@ -956,7 +956,7 @@ mkvenv="$python ${source_path}/python/scripts/mkvenv.py"
# Finish preparing the virtual environment using vendored .whl files
$mkvenv ensuregroup --dir "${source_path}/python/wheels" \
- ${source_path}/pythondeps.toml meson || exit 1
+ ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
# At this point, we expect Meson to be installed and available.
# We expect mkvenv or pip to have created pyvenv/bin/meson for us.
diff --git a/meson.build b/meson.build
index ec59effca2..dee587483b 100644
--- a/meson.build
+++ b/meson.build
@@ -3235,6 +3235,10 @@ tracetool_depends = files(
'scripts/tracetool/vcpu.py'
)
+svd_gen_header = [
+ python, files('scripts/svd-gen-header.py')
+]
+
qemu_version_cmd = [find_program('scripts/qemu-version.sh'),
meson.current_source_dir(),
get_option('pkgversion'), meson.project_version()]
diff --git a/python/setup.cfg b/python/setup.cfg
index 48668609d3..bc830c541a 100644
--- a/python/setup.cfg
+++ b/python/setup.cfg
@@ -45,6 +45,7 @@ devel =
urwid >= 2.1.2
urwid-readline >= 0.13
Pygments >= 2.9.0
+ pysvd >= 0.2.3
# Provides qom-fuse functionality
fuse =
diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
index a3f423efd8..7993fcd23c 100644
--- a/python/tests/minreqs.txt
+++ b/python/tests/minreqs.txt
@@ -22,6 +22,9 @@ distlib==0.3.6
# Dependencies for FUSE support for qom-fuse
fusepy==2.0.4
+# Dependencies for svd-gen-regs
+pysvd==0.2.3
+
# Test-runners, utilities, etc.
avocado-framework==90.0
diff --git a/pythondeps.toml b/pythondeps.toml
index 9c16602d30..8416b17650 100644
--- a/pythondeps.toml
+++ b/pythondeps.toml
@@ -32,3 +32,6 @@ sphinx_rtd_theme = { accepted = ">=0.5", installed = "1.1.1" }
# avocado-framework, for example right now the limit is 92.x.
avocado-framework = { accepted = "(>=88.1, <93.0)", installed = "88.1", canary = "avocado" }
pycdlib = { accepted = ">=1.11.0" }
+
+[svd-gen-header]
+pysvd = { accepted = ">=0.2.3.", installed = "0.2.3" }
diff --git a/scripts/svd-gen-header.py b/scripts/svd-gen-header.py
new file mode 100755
index 0000000000..ab8cb4b665
--- /dev/null
+++ b/scripts/svd-gen-header.py
@@ -0,0 +1,342 @@
+#!/usr/bin/env python3
+
+# Copyright 2024 Google LLC
+#
+# This work is licensed under the terms of the GNU GPL, version 2 or later.
+# See the COPYING file in the top-level directory.
+#
+# Use this script to generate a C header file from an SVD xml
+#
+# Two mode of operations are supported: peripheral and system.
+#
+# When running in peripheral mode a header for a specific peripheral
+# is going to be generated. It will define a type and structure with
+# all of the available registers at the bitfield level. An array that
+# contains the reigster names indexed by address is also going to be
+# generated as well as a function to initialize registers to their
+# reset values.
+#
+# Invocation example:
+#
+# svd_gen_header -i MIMXRT595S_cm33.xml -o flexcomm.h -p FLEXCOMM0 -t FLEXCOMM
+#
+# When running in system mode a header for a specific system /
+# platform will be generated. It will define register base addresses
+# and interrupt numbers for selected peripherals.
+#
+# Invocation example:
+#
+# svd_gen_header -i MIMXRT595S_cm33.xml -o rt500.h -s RT500 -p FLEXCOMM0 \
+# -p CLKCTL0 -p CLKCTL1
+#
+
+import argparse
+import datetime
+import re
+import os
+import sys
+import xml.etree.ElementTree
+import pysvd
+
+data_type_by_bits = {
+ 8: "uint8_t",
+ 16: "uint16_t",
+ 32: "uint32_t",
+}
+
+
+def get_register_array_name_and_size(register):
+ """Return register name and register array size.
+
+ The SVD can define register arrays and pysvd encodes the whole set
+ as as regular register with their name prepended by [<array size>].
+
+ Returns a tuple with the register name and the size of the array or
+ zero if this is not a register set.
+
+ """
+
+ split = re.split(r"[\[\]]", register.name)
+ return (split[0], int(split[1]) if len(split) > 1 else 0)
+
+
+def generate_register(register):
+ """Generate register data.
+
+ This include a field for accessing the full 32bits as we as
+ bitfield based register fields.
+
+ """
+
+ data_type = data_type_by_bits[register.size]
+
+ out = f" /* 0x{register.addressOffset:08X} {register.description} */\n"
+ out += " union {\n"
+ out += f" {data_type} {register.name};\n"
+ out += " struct {\n"
+
+ fields = sorted(register.fields, key=lambda field: field.bitOffset)
+ last_msb = -1
+ for field in fields:
+ reserve_bits = 0
+ lsb = field.bitOffset
+ msb = field.bitWidth + lsb - 1
+
+ if last_msb == -1 and lsb > 0:
+ reserve_bits = lsb
+
+ if last_msb != -1 and (lsb - last_msb) > 1:
+ reserve_bits = lsb - last_msb - 1
+
+ if reserve_bits > 0:
+ out += f" {data_type}:{reserve_bits};\n"
+
+ out += f" /* [{msb}..{lsb}] {field.description} */\n"
+ out += f" {data_type} {field.name} : {field.bitWidth};\n"
+
+ last_msb = msb
+
+ if register.size - last_msb > 1:
+ out += f" {data_type}:{register.size - last_msb - 1};\n"
+
+ reg_name, reg_array_size = get_register_array_name_and_size(register)
+ if reg_array_size > 0:
+ out += f" }} {reg_name}_b[{reg_array_size}];\n"
+ else:
+ out += f" }} {reg_name}_b;\n"
+ out += " };\n\n"
+
+ return out
+
+
+def generate_pos_and_msk_defines(name, periph):
+ """Generate Pos and Msk defines"""
+
+ out = "\n"
+ for reg in periph.registers:
+ for field in reg.fields:
+ reg_name, _ = get_register_array_name_and_size(reg)
+ field_name = f"{name}_{reg_name}_{field.name}"
+ out += f"#define {field_name}_Pos ({field.bitOffset}UL)\n"
+ mask = ((1 << field.bitWidth) - 1) << field.bitOffset
+ out += f"#define {field_name}_Msk (0x{mask:x}UL)\n"
+
+ return out
+
+
+def generate_enum_values(name, periph):
+ """Generate enum values"""
+
+ out = "\n"
+ for reg in periph.registers:
+ reg_name, _ = get_register_array_name_and_size(reg)
+ for field in reg.fields:
+ if hasattr(field, "enumeratedValues"):
+ out += "\n"
+ out += "typedef enum {\n"
+ for enum in field.enumeratedValues.enumeratedValues:
+ enum_name = f"{name}_{reg_name}_{field.name}_{enum.name}"
+ out += f" /* {enum.description} */\n"
+ out += f" {enum_name} = {enum.value},\n"
+ out += f"}} {name}_{reg_name}_{field.name}_Enum;\n"
+
+ return out
+
+
+def generate_register_names_array_macro(name, periph):
+ """Generate register names array macro"""
+
+ out = f"#define {name}_REGISTER_NAMES_ARRAY(_name) \\\n"
+ out += f" const char *_name[sizeof({name}_Type)] = {{ \\\n"
+ for reg in periph.registers:
+ reg_name, reg_array_size = get_register_array_name_and_size(reg)
+ if reg_array_size > 0:
+ for i in range(0, reg_array_size):
+ start = reg.addressOffset + i * reg.size // 8
+ stop = reg.addressOffset + (i + 1) * reg.size // 8 - 1
+ out += f' [{start} ... {stop}] = "{reg_name}{i}", \\\n'
+ else:
+ start = reg.addressOffset
+ stop = reg.addressOffset + reg.size // 8 - 1
+ out += f' [{start} ... {stop}] = "{reg.name}", \\\n'
+ out += " }\n"
+
+ return out
+
+
+def generate_reset_registers_function(name, periph):
+ """Generate reset registers function"""
+
+ out = "\n"
+ fname = f"{name.lower()}_reset_registers"
+ out += f"static inline void {fname}({name}_Type *regs)\n"
+ out += "{\n"
+ for reg in periph.registers:
+ reg_name, reg_array_size = get_register_array_name_and_size(reg)
+ if reg_array_size > 0:
+ for i in range(0, int(reg_array_size)):
+ out += f" regs->{reg_name}[{i}] = {hex(reg.resetValue)};\n"
+ else:
+ out += f" regs->{reg_name} = {hex(reg.resetValue)};\n"
+ out += "}\n"
+
+ return out
+
+
+def generate_peripheral_header(periph, name):
+ """Generate peripheral header
+
+ The following information is generated:
+
+ * typedef with all of the available registers and register fields,
+ position and mask defines for register fields.
+
+ * enum values that encode register fields options.
+
+ * a macro that defines the register names indexed by the relative
+ address of the register.
+
+ * a function that sets the registers to their reset values
+
+ """
+
+ out = f"/* {name} - {periph.description} */\n\n"
+ out += "typedef struct {\n"
+
+ last_reg_offset = 0
+ cnt = 0
+ for reg in periph.registers:
+ reserved_words = 0
+ if (reg.addressOffset - last_reg_offset) > 0:
+ reserved_words = int((reg.addressOffset - last_reg_offset) // 4)
+ cnt += 1
+
+ if cnt:
+ show_count = cnt
+ else:
+ show_count = ""
+
+ if reserved_words == 1:
+ out += f" uint32_t RESERVED{show_count};\n\n"
+ elif reserved_words > 1:
+ out += f" uint32_t RESERVED{show_count}[{reserved_words}];\n\n"
+
+ out += generate_register(reg)
+ last_reg_offset = reg.addressOffset + reg.size // 8
+
+ size = periph.addressBlocks[0].size
+ out += f"}} {name}_Type; /* Size = {size} (0x{size:X}) */\n"
+
+ out += "\n\n"
+
+ out += generate_pos_and_msk_defines(name, periph)
+
+ out += generate_enum_values(name, periph)
+
+ out += generate_register_names_array_macro(name, periph)
+
+ out += generate_reset_registers_function(name, periph)
+
+ return out
+
+
+def get_same_class_peripherals(svd, periph):
+ """Get a list of peripherals that are instances of the same class."""
+
+ return [periph] + [
+ p
+ for p in svd.peripherals
+ if p.derivedFrom and p.derivedFrom.name == periph.name
+ ]
+
+
+def generate_system_header(system, svd, periph):
+ """Generate base and irq defines for given list of peripherals"""
+
+ out = ""
+
+ for p in get_same_class_peripherals(svd, periph):
+ out += f"#define {system}_{p.name}_BASE 0x{p.baseAddress:X}UL\n"
+ out += "\n"
+
+ for p in get_same_class_peripherals(svd, periph):
+ for irq in p.interrupts:
+ out += f"#define {system}_{irq.name}_IRQn 0x{irq.value}UL\n"
+ out += "\n"
+
+ return out
+
+
+def main():
+ """Script to generate C header file from an SVD file"""
+
+ parser = argparse.ArgumentParser()
+ parser.add_argument(
+ "-i", "--input", type=str, help="Input SVD file", required=True
+ )
+ parser.add_argument(
+ "-o", "--output", type=str, help="Output .h file", required=True
+ )
+ parser.add_argument(
+ "-p",
+ "--peripheral",
+ action="append",
+ help="peripheral name from the SVD file",
+ required=True,
+ )
+ parser.add_argument(
+ "-t",
+ "--type-name",
+ type=str,
+ help="name to be used for peripheral definitions",
+ required=False,
+ )
+ parser.add_argument(
+ "-s",
+ "--system",
+ type=str,
+ help="name to be used for the system definitions",
+ required=False,
+ )
+
+ args = parser.parse_args()
+
+ node = xml.etree.ElementTree.parse(args.input).getroot()
+ svd = pysvd.element.Device(node)
+
+ # Write license header
+ out = "/*\n"
+ license_text = svd.licenseText.split("\\n")
+ for line in license_text:
+ sline = f" * {line.strip()}"
+ out += f"{sline.rstrip()}\n"
+
+ now = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S")
+ out += f" * @note Automatically generated by {os.path.basename(__file__)}"
+ out += f" on {now} UTC from {os.path.basename(args.input)}.\n"
+ out += " *\n */\n\n"
+
+ # Write some generic defines
+ out += "#pragma once\n\n"
+
+ for name in args.peripheral:
+ periph = svd.find(name)
+ if periph:
+ if args.system:
+ out += generate_system_header(args.system, svd, periph)
+ else:
+ out += generate_peripheral_header(
+ periph, args.type_name if args.type_name else periph.name
+ )
+ else:
+ print(f"No such peripheral: {name}")
+ return 1
+
+ with open(args.output, "w", encoding="ascii") as output:
+ output.write(out)
+
+ return 0
+
+
+if __name__ == "__main__":
+ sys.exit(main())
--
2.46.0.rc2.264.g509ed76dc8-googOn Mon, 5 Aug 2024 at 21:17, Octavian Purdila <tavip@google.com> wrote:
>
> From: Stefan Stanacar <stefanst@google.com>
>
> From: Stefan Stanacar <stefanst@google.com>
>
> The CMSIS System View Description format(CMSIS-SVD) is an XML based
> description of Arm Cortex-M microcontrollers provided and maintained
> by sillicon vendors. It includes details such as peripherals registers
> (down to bitfields), peripheral register block addresses, reset
> values, etc.
>
> This script uses this information to create header files that makes it
> easier to emulate peripherals.
>
> The script can be used to create either peripheral specific headers or
> board / system specific information. The script generated headers are
> similar to the SVDConv utility.
>
> Peripheral specific headers contains information such as register
> layout, register names and reset values for registers:
>
> typedef struct {
> ...
> union {
> uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
> * Flexcomm module ID */
> struct {
> uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
> uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
> uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
> uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
> uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
> uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
> uint32_t : 4;
> uint32_t ID : 20; /* [31..12] Flexcomm ID */
> } PSELID_b;
> };
Bitfield layout in C isn't portable, so don't generate this kind
of union-of-a-integer-and-some-bitfields, please. You can
generate FIELD() macro invocations (see include/hw/registerfields.h)
which define shift/mask/length macros that can be used with
FIELD_EX*/FIELD_DP* to do extract/deposit operations.
> ...
> } FLEXCOMM_Type; /* Size = 4096 (0x1000) */
>
> #define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
> #define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
> #define FLEXCOMM_PSELID_LOCK_Pos (3UL)
> #define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
> ...
>
> typedef enum { /* FLEXCOMM_PSELID_LOCK */
> /* Peripheral select can be changed by software. */
> FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
> /* Peripheral select is locked and cannot be changed until this
> * Flexcomm module or the entire device is reset. */
> FLEXCOMM_PSELID_LOCK_LOCKED = 1,
> } FLEXCOMM_PSELID_LOCK_Enum;
> ...
>
> #define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
> const char *_name[sizeof(FLEXCOMM_Type)] = { \
> [4088 ... 4091] = "PSELID", \
> [4092 ... 4095] = "PID", \
> }
>
> Board specific headers contains information about peripheral base
> register addresses.
thanks
-- PMM
On Mon, Aug 12, 2024 at 8:27 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
Hi Peter,
Thanks for the review!
> On Mon, 5 Aug 2024 at 21:17, Octavian Purdila <tavip@google.com> wrote:
> >
> > From: Stefan Stanacar <stefanst@google.com>
> >
> > From: Stefan Stanacar <stefanst@google.com>
> >
> > The CMSIS System View Description format(CMSIS-SVD) is an XML based
> > description of Arm Cortex-M microcontrollers provided and maintained
> > by sillicon vendors. It includes details such as peripherals registers
> > (down to bitfields), peripheral register block addresses, reset
> > values, etc.
> >
> > This script uses this information to create header files that makes it
> > easier to emulate peripherals.
> >
> > The script can be used to create either peripheral specific headers or
> > board / system specific information. The script generated headers are
> > similar to the SVDConv utility.
> >
> > Peripheral specific headers contains information such as register
> > layout, register names and reset values for registers:
> >
> > typedef struct {
> > ...
> > union {
> > uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
> > * Flexcomm module ID */
> > struct {
> > uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
> > uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
> > uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
> > uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
> > uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
> > uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
> > uint32_t : 4;
> > uint32_t ID : 20; /* [31..12] Flexcomm ID */
> > } PSELID_b;
> > };
>
> Bitfield layout in C isn't portable, so don't generate this kind
> of union-of-a-integer-and-some-bitfields, please. You can
> generate FIELD() macro invocations (see include/hw/registerfields.h)
> which define shift/mask/length macros that can be used with
> FIELD_EX*/FIELD_DP* to do extract/deposit operations.
>
I see that C bitfields are already used in a few places in qemu. Could
you please elaborate on the portability issue?
> > ...
> > } FLEXCOMM_Type; /* Size = 4096 (0x1000) */
> >
> > #define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
> > #define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
> > #define FLEXCOMM_PSELID_LOCK_Pos (3UL)
> > #define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
> > ...
> >
> > typedef enum { /* FLEXCOMM_PSELID_LOCK */
> > /* Peripheral select can be changed by software. */
> > FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
> > /* Peripheral select is locked and cannot be changed until this
> > * Flexcomm module or the entire device is reset. */
> > FLEXCOMM_PSELID_LOCK_LOCKED = 1,
> > } FLEXCOMM_PSELID_LOCK_Enum;
> > ...
> >
> > #define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
> > const char *_name[sizeof(FLEXCOMM_Type)] = { \
> > [4088 ... 4091] = "PSELID", \
> > [4092 ... 4095] = "PID", \
> > }
> >
> > Board specific headers contains information about peripheral base
> > register addresses.
>
> thanks
> -- PMM
On 8/13/24 03:56, Octavian Purdila wrote:
>>> typedef struct {
>>> ...
>>> union {
>>> uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
>>> * Flexcomm module ID */
>>> struct {
>>> uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
>>> uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
>>> uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
>>> uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
>>> uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
>>> uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
>>> uint32_t : 4;
>>> uint32_t ID : 20; /* [31..12] Flexcomm ID */
>>> } PSELID_b;
>>> };
>>
>> Bitfield layout in C isn't portable, so don't generate this kind
>> of union-of-a-integer-and-some-bitfields, please. You can
>> generate FIELD() macro invocations (see include/hw/registerfields.h)
>> which define shift/mask/length macros that can be used with
>> FIELD_EX*/FIELD_DP* to do extract/deposit operations.
>>
>
> I see that C bitfields are already used in a few places in qemu. Could
> you please elaborate on the portability issue?
Bitfields are fine, so long as you're only using them for storage compression and do not
care about the underlying layout.
The moment you put them into a union with an integer, clearly you are expecting the
bitfields to be in some particular order with respect to the integer, and that is the
portability issue.
In particular, big-endian hosts will generally flip the order, layout starting at the most
signifiacnt bit and work down. Other compilers will pad bits for alignment in ways that
you do not expect.
Just Don't Do It.
r~
On Mon, Aug 12, 2024 at 3:43 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/13/24 03:56, Octavian Purdila wrote:
> >>> typedef struct {
> >>> ...
> >>> union {
> >>> uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
> >>> * Flexcomm module ID */
> >>> struct {
> >>> uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
> >>> uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
> >>> uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
> >>> uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
> >>> uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
> >>> uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
> >>> uint32_t : 4;
> >>> uint32_t ID : 20; /* [31..12] Flexcomm ID */
> >>> } PSELID_b;
> >>> };
> >>
> >> Bitfield layout in C isn't portable, so don't generate this kind
> >> of union-of-a-integer-and-some-bitfields, please. You can
> >> generate FIELD() macro invocations (see include/hw/registerfields.h)
> >> which define shift/mask/length macros that can be used with
> >> FIELD_EX*/FIELD_DP* to do extract/deposit operations.
> >>
> >
> > I see that C bitfields are already used in a few places in qemu. Could
> > you please elaborate on the portability issue?
>
> Bitfields are fine, so long as you're only using them for storage compression and do not
> care about the underlying layout.
>
> The moment you put them into a union with an integer, clearly you are expecting the
> bitfields to be in some particular order with respect to the integer, and that is the
> portability issue.
>
> In particular, big-endian hosts will generally flip the order, layout starting at the most
> signifiacnt bit and work down. Other compilers will pad bits for alignment in ways that
> you do not expect.
>
> Just Don't Do It.
>
Thanks Richard, I appreciate the detailed explanation! I will look
into the approach Peter suggested.
>
> r~
On Mon, Aug 5, 2024 at 4:17 PM Octavian Purdila <tavip@google.com> wrote:
> From: Stefan Stanacar <stefanst@google.com>
>
> From: Stefan Stanacar <stefanst@google.com>
>
> The CMSIS System View Description format(CMSIS-SVD) is an XML based
> description of Arm Cortex-M microcontrollers provided and maintained
> by sillicon vendors. It includes details such as peripherals registers
> (down to bitfields), peripheral register block addresses, reset
> values, etc.
>
> This script uses this information to create header files that makes it
> easier to emulate peripherals.
>
> The script can be used to create either peripheral specific headers or
> board / system specific information. The script generated headers are
> similar to the SVDConv utility.
>
> Peripheral specific headers contains information such as register
> layout, register names and reset values for registers:
>
> typedef struct {
> ...
> union {
> uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
> * Flexcomm module ID */
> struct {
> uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
> uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
> uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
> uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
> uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
> uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
> uint32_t : 4;
> uint32_t ID : 20; /* [31..12] Flexcomm ID */
> } PSELID_b;
> };
> ...
> } FLEXCOMM_Type; /* Size = 4096 (0x1000) */
>
> #define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
> #define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
> #define FLEXCOMM_PSELID_LOCK_Pos (3UL)
> #define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
> ...
>
> typedef enum { /* FLEXCOMM_PSELID_LOCK */
> /* Peripheral select can be changed by software. */
> FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
> /* Peripheral select is locked and cannot be changed until this
> * Flexcomm module or the entire device is reset. */
> FLEXCOMM_PSELID_LOCK_LOCKED = 1,
> } FLEXCOMM_PSELID_LOCK_Enum;
> ...
>
> #define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
> const char *_name[sizeof(FLEXCOMM_Type)] = { \
> [4088 ... 4091] = "PSELID", \
> [4092 ... 4095] = "PID", \
> }
>
> Board specific headers contains information about peripheral base
> register addresses.
>
> Signed-off-by: Stefan Stanacar <stefanst@google.com>
> Signed-off-by: Octavian Purdila <tavip@google.com>
> ---
> configure | 2 +-
> meson.build | 4 +
> python/setup.cfg | 1 +
> python/tests/minreqs.txt | 3 +
> pythondeps.toml | 3 +
> scripts/svd-gen-header.py | 342 ++++++++++++++++++++++++++++++++++++++
> 6 files changed, 354 insertions(+), 1 deletion(-)
> create mode 100755 scripts/svd-gen-header.py
>
> diff --git a/configure b/configure
> index 5ad1674ca5..811bfa5d54 100755
> --- a/configure
> +++ b/configure
> @@ -956,7 +956,7 @@ mkvenv="$python
> ${source_path}/python/scripts/mkvenv.py"
> # Finish preparing the virtual environment using vendored .whl files
>
> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
> - ${source_path}/pythondeps.toml meson || exit 1
> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
>
I haven't read the rest of this series; I'm chiming in solely from the
build/python maintainer angle. Do we *always* need pysvd, no matter how
QEMU was configured? Adding it to the meson line here is a very big hammer.
If not, consider looking at how sphinx (the "docs" group) is only
conditionally installed into the configure venv and mimic that using the
appropriate configure flags that necessitate the availability of pyvsd for
the QEMU build.
We also need to provide a way for pysvd to be available offline; some
packages are available via distro libs and if this package is available for
every distro we officially support, that's sufficient (but requires updates
to our various docker and VM test configuration files to add the new
dependency). Otherwise, like we do for meson, we need to vendor the wheel
in the tree so offline tarball builds will continue to work.
It looks like pysvd is a pure python package with no dependencies, so it
should be OK to vendor it in qemu.git/python/wheels/ - look at
qemu.git/python/scripts/vendor.py and consider updating and running this
script.
(The real blocker here is that RPM builds are performed offline and
dependencies that cannot be satisfied via rpm can't be added through pip.
We need any one of these to be true: (A) pyvsd is available (of a
sufficient version) in all distro repositories we target; (B) This build
feature is conditional and nobody minds if it never gets enabled for RPM
builds; (C) The package can be vendored.)
~~js
That said, you might be the first person I've seen outside of Paolo and I
to brave mucking around with the python build venv. You deserve a bravery
sticker :)
> # At this point, we expect Meson to be installed and available.
> # We expect mkvenv or pip to have created pyvenv/bin/meson for us.
> diff --git a/meson.build b/meson.build
> index ec59effca2..dee587483b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3235,6 +3235,10 @@ tracetool_depends = files(
> 'scripts/tracetool/vcpu.py'
> )
>
> +svd_gen_header = [
> + python, files('scripts/svd-gen-header.py')
> +]
> +
> qemu_version_cmd = [find_program('scripts/qemu-version.sh'),
> meson.current_source_dir(),
> get_option('pkgversion'), meson.project_version()]
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 48668609d3..bc830c541a 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -45,6 +45,7 @@ devel =
> urwid >= 2.1.2
> urwid-readline >= 0.13
> Pygments >= 2.9.0
> + pysvd >= 0.2.3
>
> # Provides qom-fuse functionality
> fuse =
> diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
> index a3f423efd8..7993fcd23c 100644
> --- a/python/tests/minreqs.txt
> +++ b/python/tests/minreqs.txt
> @@ -22,6 +22,9 @@ distlib==0.3.6
> # Dependencies for FUSE support for qom-fuse
> fusepy==2.0.4
>
> +# Dependencies for svd-gen-regs
> +pysvd==0.2.3
> +
> # Test-runners, utilities, etc.
> avocado-framework==90.0
>
> diff --git a/pythondeps.toml b/pythondeps.toml
> index 9c16602d30..8416b17650 100644
> --- a/pythondeps.toml
> +++ b/pythondeps.toml
> @@ -32,3 +32,6 @@ sphinx_rtd_theme = { accepted = ">=0.5", installed =
> "1.1.1" }
> # avocado-framework, for example right now the limit is 92.x.
> avocado-framework = { accepted = "(>=88.1, <93.0)", installed = "88.1",
> canary = "avocado" }
> pycdlib = { accepted = ">=1.11.0" }
> +
> +[svd-gen-header]
> +pysvd = { accepted = ">=0.2.3.", installed = "0.2.3" }
> diff --git a/scripts/svd-gen-header.py b/scripts/svd-gen-header.py
> new file mode 100755
> index 0000000000..ab8cb4b665
> --- /dev/null
> +++ b/scripts/svd-gen-header.py
> @@ -0,0 +1,342 @@
> +#!/usr/bin/env python3
> +
> +# Copyright 2024 Google LLC
> +#
> +# This work is licensed under the terms of the GNU GPL, version 2 or
> later.
> +# See the COPYING file in the top-level directory.
> +#
> +# Use this script to generate a C header file from an SVD xml
> +#
> +# Two mode of operations are supported: peripheral and system.
> +#
> +# When running in peripheral mode a header for a specific peripheral
> +# is going to be generated. It will define a type and structure with
> +# all of the available registers at the bitfield level. An array that
> +# contains the reigster names indexed by address is also going to be
> +# generated as well as a function to initialize registers to their
> +# reset values.
> +#
> +# Invocation example:
> +#
> +# svd_gen_header -i MIMXRT595S_cm33.xml -o flexcomm.h -p FLEXCOMM0 -t
> FLEXCOMM
> +#
> +# When running in system mode a header for a specific system /
> +# platform will be generated. It will define register base addresses
> +# and interrupt numbers for selected peripherals.
> +#
> +# Invocation example:
> +#
> +# svd_gen_header -i MIMXRT595S_cm33.xml -o rt500.h -s RT500 -p FLEXCOMM0 \
> +# -p CLKCTL0 -p CLKCTL1
> +#
> +
> +import argparse
> +import datetime
> +import re
> +import os
> +import sys
> +import xml.etree.ElementTree
> +import pysvd
> +
> +data_type_by_bits = {
> + 8: "uint8_t",
> + 16: "uint16_t",
> + 32: "uint32_t",
> +}
> +
> +
> +def get_register_array_name_and_size(register):
> + """Return register name and register array size.
> +
> + The SVD can define register arrays and pysvd encodes the whole set
> + as as regular register with their name prepended by [<array size>].
> +
> + Returns a tuple with the register name and the size of the array or
> + zero if this is not a register set.
> +
> + """
> +
> + split = re.split(r"[\[\]]", register.name)
> + return (split[0], int(split[1]) if len(split) > 1 else 0)
> +
> +
> +def generate_register(register):
> + """Generate register data.
> +
> + This include a field for accessing the full 32bits as we as
> + bitfield based register fields.
> +
> + """
> +
> + data_type = data_type_by_bits[register.size]
> +
> + out = f" /* 0x{register.addressOffset:08X} {register.description}
> */\n"
> + out += " union {\n"
> + out += f" {data_type} {register.name};\n"
> + out += " struct {\n"
> +
> + fields = sorted(register.fields, key=lambda field: field.bitOffset)
> + last_msb = -1
> + for field in fields:
> + reserve_bits = 0
> + lsb = field.bitOffset
> + msb = field.bitWidth + lsb - 1
> +
> + if last_msb == -1 and lsb > 0:
> + reserve_bits = lsb
> +
> + if last_msb != -1 and (lsb - last_msb) > 1:
> + reserve_bits = lsb - last_msb - 1
> +
> + if reserve_bits > 0:
> + out += f" {data_type}:{reserve_bits};\n"
> +
> + out += f" /* [{msb}..{lsb}] {field.description} */\n"
> + out += f" {data_type} {field.name} :
> {field.bitWidth};\n"
> +
> + last_msb = msb
> +
> + if register.size - last_msb > 1:
> + out += f" {data_type}:{register.size - last_msb -
> 1};\n"
> +
> + reg_name, reg_array_size = get_register_array_name_and_size(register)
> + if reg_array_size > 0:
> + out += f" }} {reg_name}_b[{reg_array_size}];\n"
> + else:
> + out += f" }} {reg_name}_b;\n"
> + out += " };\n\n"
> +
> + return out
> +
> +
> +def generate_pos_and_msk_defines(name, periph):
> + """Generate Pos and Msk defines"""
> +
> + out = "\n"
> + for reg in periph.registers:
> + for field in reg.fields:
> + reg_name, _ = get_register_array_name_and_size(reg)
> + field_name = f"{name}_{reg_name}_{field.name}"
> + out += f"#define {field_name}_Pos ({field.bitOffset}UL)\n"
> + mask = ((1 << field.bitWidth) - 1) << field.bitOffset
> + out += f"#define {field_name}_Msk (0x{mask:x}UL)\n"
> +
> + return out
> +
> +
> +def generate_enum_values(name, periph):
> + """Generate enum values"""
> +
> + out = "\n"
> + for reg in periph.registers:
> + reg_name, _ = get_register_array_name_and_size(reg)
> + for field in reg.fields:
> + if hasattr(field, "enumeratedValues"):
> + out += "\n"
> + out += "typedef enum {\n"
> + for enum in field.enumeratedValues.enumeratedValues:
> + enum_name = f"{name}_{reg_name}_{field.name}_{
> enum.name}"
> + out += f" /* {enum.description} */\n"
> + out += f" {enum_name} = {enum.value},\n"
> + out += f"}} {name}_{reg_name}_{field.name}_Enum;\n"
> +
> + return out
> +
> +
> +def generate_register_names_array_macro(name, periph):
> + """Generate register names array macro"""
> +
> + out = f"#define {name}_REGISTER_NAMES_ARRAY(_name) \\\n"
> + out += f" const char *_name[sizeof({name}_Type)] = {{ \\\n"
> + for reg in periph.registers:
> + reg_name, reg_array_size = get_register_array_name_and_size(reg)
> + if reg_array_size > 0:
> + for i in range(0, reg_array_size):
> + start = reg.addressOffset + i * reg.size // 8
> + stop = reg.addressOffset + (i + 1) * reg.size // 8 - 1
> + out += f' [{start} ... {stop}] = "{reg_name}{i}",
> \\\n'
> + else:
> + start = reg.addressOffset
> + stop = reg.addressOffset + reg.size // 8 - 1
> + out += f' [{start} ... {stop}] = "{reg.name}", \\\n'
> + out += " }\n"
> +
> + return out
> +
> +
> +def generate_reset_registers_function(name, periph):
> + """Generate reset registers function"""
> +
> + out = "\n"
> + fname = f"{name.lower()}_reset_registers"
> + out += f"static inline void {fname}({name}_Type *regs)\n"
> + out += "{\n"
> + for reg in periph.registers:
> + reg_name, reg_array_size = get_register_array_name_and_size(reg)
> + if reg_array_size > 0:
> + for i in range(0, int(reg_array_size)):
> + out += f" regs->{reg_name}[{i}] =
> {hex(reg.resetValue)};\n"
> + else:
> + out += f" regs->{reg_name} = {hex(reg.resetValue)};\n"
> + out += "}\n"
> +
> + return out
> +
> +
> +def generate_peripheral_header(periph, name):
> + """Generate peripheral header
> +
> + The following information is generated:
> +
> + * typedef with all of the available registers and register fields,
> + position and mask defines for register fields.
> +
> + * enum values that encode register fields options.
> +
> + * a macro that defines the register names indexed by the relative
> + address of the register.
> +
> + * a function that sets the registers to their reset values
> +
> + """
> +
> + out = f"/* {name} - {periph.description} */\n\n"
> + out += "typedef struct {\n"
> +
> + last_reg_offset = 0
> + cnt = 0
> + for reg in periph.registers:
> + reserved_words = 0
> + if (reg.addressOffset - last_reg_offset) > 0:
> + reserved_words = int((reg.addressOffset - last_reg_offset) //
> 4)
> + cnt += 1
> +
> + if cnt:
> + show_count = cnt
> + else:
> + show_count = ""
> +
> + if reserved_words == 1:
> + out += f" uint32_t RESERVED{show_count};\n\n"
> + elif reserved_words > 1:
> + out += f" uint32_t
> RESERVED{show_count}[{reserved_words}];\n\n"
> +
> + out += generate_register(reg)
> + last_reg_offset = reg.addressOffset + reg.size // 8
> +
> + size = periph.addressBlocks[0].size
> + out += f"}} {name}_Type; /* Size = {size} (0x{size:X}) */\n"
> +
> + out += "\n\n"
> +
> + out += generate_pos_and_msk_defines(name, periph)
> +
> + out += generate_enum_values(name, periph)
> +
> + out += generate_register_names_array_macro(name, periph)
> +
> + out += generate_reset_registers_function(name, periph)
> +
> + return out
> +
> +
> +def get_same_class_peripherals(svd, periph):
> + """Get a list of peripherals that are instances of the same class."""
> +
> + return [periph] + [
> + p
> + for p in svd.peripherals
> + if p.derivedFrom and p.derivedFrom.name == periph.name
> + ]
> +
> +
> +def generate_system_header(system, svd, periph):
> + """Generate base and irq defines for given list of peripherals"""
> +
> + out = ""
> +
> + for p in get_same_class_peripherals(svd, periph):
> + out += f"#define {system}_{p.name}_BASE 0x{p.baseAddress:X}UL\n"
> + out += "\n"
> +
> + for p in get_same_class_peripherals(svd, periph):
> + for irq in p.interrupts:
> + out += f"#define {system}_{irq.name}_IRQn 0x{irq.value}UL\n"
> + out += "\n"
> +
> + return out
> +
> +
> +def main():
> + """Script to generate C header file from an SVD file"""
> +
> + parser = argparse.ArgumentParser()
> + parser.add_argument(
> + "-i", "--input", type=str, help="Input SVD file", required=True
> + )
> + parser.add_argument(
> + "-o", "--output", type=str, help="Output .h file", required=True
> + )
> + parser.add_argument(
> + "-p",
> + "--peripheral",
> + action="append",
> + help="peripheral name from the SVD file",
> + required=True,
> + )
> + parser.add_argument(
> + "-t",
> + "--type-name",
> + type=str,
> + help="name to be used for peripheral definitions",
> + required=False,
> + )
> + parser.add_argument(
> + "-s",
> + "--system",
> + type=str,
> + help="name to be used for the system definitions",
> + required=False,
> + )
> +
> + args = parser.parse_args()
> +
> + node = xml.etree.ElementTree.parse(args.input).getroot()
> + svd = pysvd.element.Device(node)
> +
> + # Write license header
> + out = "/*\n"
> + license_text = svd.licenseText.split("\\n")
> + for line in license_text:
> + sline = f" * {line.strip()}"
> + out += f"{sline.rstrip()}\n"
> +
> + now = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S")
> + out += f" * @note Automatically generated by
> {os.path.basename(__file__)}"
> + out += f" on {now} UTC from {os.path.basename(args.input)}.\n"
> + out += " *\n */\n\n"
> +
> + # Write some generic defines
> + out += "#pragma once\n\n"
> +
> + for name in args.peripheral:
> + periph = svd.find(name)
> + if periph:
> + if args.system:
> + out += generate_system_header(args.system, svd, periph)
> + else:
> + out += generate_peripheral_header(
> + periph, args.type_name if args.type_name else
> periph.name
> + )
> + else:
> + print(f"No such peripheral: {name}")
> + return 1
> +
> + with open(args.output, "w", encoding="ascii") as output:
> + output.write(out)
> +
> + return 0
> +
> +
> +if __name__ == "__main__":
> + sys.exit(main())
> --
> 2.46.0.rc2.264.g509ed76dc8-goog
>
>
On 8/8/24 23:56, John Snow wrote: > I haven't read the rest of this series; I'm chiming in solely from the > build/python maintainer angle. Do we *always* need pysvd, no matter how > QEMU was configured? Adding it to the meson line here is a very big hammer. In general I'd agree, though for a 7.5 kB package with no other dependencies I'm willing to make an exception. The alternative would be pretty complex: - check if pysvd is installed in the host - check if a machine type that needs pysvd is enabled, defaulting to y or n depending on the previous step and --enable-download - use that to decide between doing nothing, installing pysvd or erroring out - pass the availability of pysvd to Kconfig - use that to make the final determination on whether to enable those machine types that use pysvd This is quite obviously overengineered compared to the alternative. Another possibility is to ship the generated file, skip regeneration if pysvd is not installed (on the host), and not touch pythondeps.toml at all. Whether shipping a generated file is acceptable should be decided by Peter as ARM maintainer, personally I would go the way that Octavian is going already and I'm mentioning the rest only for completeness and education. However... > We also need to provide a way for pysvd to be available offline; some > packages are available via distro libs and if this package is available > for every distro we officially support, that's sufficient (but requires > updates to our various docker and VM test configuration files to add the > new dependency). Otherwise, like we do for meson, we need to vendor the > wheel in the tree so offline tarball builds will continue to work. > > It looks like pysvd is a pure python package with no dependencies, so it > should be OK to vendor it in qemu.git/python/wheels/ - look at > qemu.git/python/scripts/vendor.py and consider updating and running this > script. ... this is indeed correct. It's not hard and it helps building on older distros. Future versions of Debian or Fedora might package pysvd, but right now it's not included anywhere (https://repology.org/project/python:pysvd/history). > That said, you might be the first person I've seen outside of Paolo and > I to brave mucking around with the python build venv. You deserve a > bravery sticker :) It's not that bad, come on. :) But yeah, I'm positively surprised by the effort to include pysvd in the virtual environment, and any suggestions to improve the documentation and discoverability of the venv setup are welcome. I'm curious whether you figured it out yourself or found https://www.qemu.org/docs/master/devel/build-system.html. One thing I heard from others is that pythondeps.toml looks "too much like" a fringe standard Python feature that no one has heard about. In some sense that's a great compliment, but I can see how it can be a bit disconcerting. Paolo
On Thu, Aug 8, 2024 at 11:34 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 8/8/24 23:56, John Snow wrote: > > I haven't read the rest of this series; I'm chiming in solely from the > > build/python maintainer angle. Do we *always* need pysvd, no matter how > > QEMU was configured? Adding it to the meson line here is a very big hammer. > > In general I'd agree, though for a 7.5 kB package with no other > dependencies I'm willing to make an exception. > > The alternative would be pretty complex: > > - check if pysvd is installed in the host > > - check if a machine type that needs pysvd is enabled, defaulting to y > or n depending on the previous step and --enable-download > > - use that to decide between doing nothing, installing pysvd or erroring out > > - pass the availability of pysvd to Kconfig > > - use that to make the final determination on whether to enable those > machine types that use pysvd > > This is quite obviously overengineered compared to the alternative. > > Another possibility is to ship the generated file, skip regeneration if > pysvd is not installed (on the host), and not touch pythondeps.toml at > all. Whether shipping a generated file is acceptable should be decided > by Peter as ARM maintainer, personally I would go the way that Octavian > is going already and I'm mentioning the rest only for completeness and > education. > > However... > > > We also need to provide a way for pysvd to be available offline; some > > packages are available via distro libs and if this package is available > > for every distro we officially support, that's sufficient (but requires > > updates to our various docker and VM test configuration files to add the > > new dependency). Otherwise, like we do for meson, we need to vendor the > > wheel in the tree so offline tarball builds will continue to work. > > > > It looks like pysvd is a pure python package with no dependencies, so it > > should be OK to vendor it in qemu.git/python/wheels/ - look at > > qemu.git/python/scripts/vendor.py and consider updating and running this > > script. > > ... this is indeed correct. It's not hard and it helps building on > older distros. Future versions of Debian or Fedora might package pysvd, > but right now it's not included anywhere > (https://repology.org/project/python:pysvd/history). > > > That said, you might be the first person I've seen outside of Paolo and > > I to brave mucking around with the python build venv. You deserve a > > bravery sticker :) > > It's not that bad, come on. :) But yeah, I'm positively surprised by > the effort to include pysvd in the virtual environment, and any > suggestions to improve the documentation and discoverability of the venv > setup are welcome. I'm curious whether you figured it out yourself or > found https://www.qemu.org/docs/master/devel/build-system.html. > A combination of reading the docs above and looking at how things are done for other packages (meson, sphinx). It really was straightforward. > One thing I heard from others is that pythondeps.toml looks "too much > like" a fringe standard Python feature that no one has heard about. In > some sense that's a great compliment, but I can see how it can be a bit > disconcerting. > Now that you mentioned it I can see how people might think that :) But the build system docs are pretty clear IMO.
On Thu, Aug 8, 2024 at 2:56 PM John Snow <jsnow@redhat.com> wrote:
>
>
>
> On Mon, Aug 5, 2024 at 4:17 PM Octavian Purdila <tavip@google.com> wrote:
>>
>> From: Stefan Stanacar <stefanst@google.com>
>>
>> From: Stefan Stanacar <stefanst@google.com>
>>
>> The CMSIS System View Description format(CMSIS-SVD) is an XML based
>> description of Arm Cortex-M microcontrollers provided and maintained
>> by sillicon vendors. It includes details such as peripherals registers
>> (down to bitfields), peripheral register block addresses, reset
>> values, etc.
>>
>> This script uses this information to create header files that makes it
>> easier to emulate peripherals.
>>
>> The script can be used to create either peripheral specific headers or
>> board / system specific information. The script generated headers are
>> similar to the SVDConv utility.
>>
>> Peripheral specific headers contains information such as register
>> layout, register names and reset values for registers:
>>
>> typedef struct {
>> ...
>> union {
>> uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
>> * Flexcomm module ID */
>> struct {
>> uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
>> uint32_t LOCK : 1; /* [3..3] Lock the peripheral select */
>> uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
>> uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
>> uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
>> uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
>> uint32_t : 4;
>> uint32_t ID : 20; /* [31..12] Flexcomm ID */
>> } PSELID_b;
>> };
>> ...
>> } FLEXCOMM_Type; /* Size = 4096 (0x1000) */
>>
>> #define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
>> #define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
>> #define FLEXCOMM_PSELID_LOCK_Pos (3UL)
>> #define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
>> ...
>>
>> typedef enum { /* FLEXCOMM_PSELID_LOCK */
>> /* Peripheral select can be changed by software. */
>> FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
>> /* Peripheral select is locked and cannot be changed until this
>> * Flexcomm module or the entire device is reset. */
>> FLEXCOMM_PSELID_LOCK_LOCKED = 1,
>> } FLEXCOMM_PSELID_LOCK_Enum;
>> ...
>>
>> #define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
>> const char *_name[sizeof(FLEXCOMM_Type)] = { \
>> [4088 ... 4091] = "PSELID", \
>> [4092 ... 4095] = "PID", \
>> }
>>
>> Board specific headers contains information about peripheral base
>> register addresses.
>>
>> Signed-off-by: Stefan Stanacar <stefanst@google.com>
>> Signed-off-by: Octavian Purdila <tavip@google.com>
>> ---
>> configure | 2 +-
>> meson.build | 4 +
>> python/setup.cfg | 1 +
>> python/tests/minreqs.txt | 3 +
>> pythondeps.toml | 3 +
>> scripts/svd-gen-header.py | 342 ++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 354 insertions(+), 1 deletion(-)
>> create mode 100755 scripts/svd-gen-header.py
>>
>> diff --git a/configure b/configure
>> index 5ad1674ca5..811bfa5d54 100755
>> --- a/configure
>> +++ b/configure
>> @@ -956,7 +956,7 @@ mkvenv="$python ${source_path}/python/scripts/mkvenv.py"
>> # Finish preparing the virtual environment using vendored .whl files
>>
>> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
>> - ${source_path}/pythondeps.toml meson || exit 1
>> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
>
Hi John,
Thanks for reviewing!
>
> I haven't read the rest of this series; I'm chiming in solely from the build/python maintainer angle. Do we *always* need pysvd, no matter how QEMU was configured? Adding it to the meson line here is a very big hammer.
>
I think the minimum we can do is to install it only if CONFIG_ARM is
enabled. That might change in the future if the models we create with
pysvd are enabled for other architectures.
> If not, consider looking at how sphinx (the "docs" group) is only conditionally installed into the configure venv and mimic that using the appropriate configure flags that necessitate the availability of pyvsd for the QEMU build.
Thanks for the pointer, I'll take a look.
>
> We also need to provide a way for pysvd to be available offline; some packages are available via distro libs and if this package is available for every distro we officially support, that's sufficient (but requires updates to our various docker and VM test configuration files to add the new dependency). Otherwise, like we do for meson, we need to vendor the wheel in the tree so offline tarball builds will continue to work.
>
> It looks like pysvd is a pure python package with no dependencies, so it should be OK to vendor it in qemu.git/python/wheels/ - look at qemu.git/python/scripts/vendor.py and consider updating and running this script.
Thanks, I'll look at it and add it in v2.
>
> (The real blocker here is that RPM builds are performed offline and dependencies that cannot be satisfied via rpm can't be added through pip. We need any one of these to be true: (A) pyvsd is available (of a sufficient version) in all distro repositories we target; (B) This build feature is conditional and nobody minds if it never gets enabled for RPM builds; (C) The package can be vendored.)
>
> ~~js
>
> That said, you might be the first person I've seen outside of Paolo and I to brave mucking around with the python build venv. You deserve a bravery sticker :)
>
>>
>> # At this point, we expect Meson to be installed and available.
>> # We expect mkvenv or pip to have created pyvenv/bin/meson for us.
>> diff --git a/meson.build b/meson.build
>> index ec59effca2..dee587483b 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -3235,6 +3235,10 @@ tracetool_depends = files(
>> 'scripts/tracetool/vcpu.py'
>> )
>>
>> +svd_gen_header = [
>> + python, files('scripts/svd-gen-header.py')
>> +]
>> +
>> qemu_version_cmd = [find_program('scripts/qemu-version.sh'),
>> meson.current_source_dir(),
>> get_option('pkgversion'), meson.project_version()]
>> diff --git a/python/setup.cfg b/python/setup.cfg
>> index 48668609d3..bc830c541a 100644
>> --- a/python/setup.cfg
>> +++ b/python/setup.cfg
>> @@ -45,6 +45,7 @@ devel =
>> urwid >= 2.1.2
>> urwid-readline >= 0.13
>> Pygments >= 2.9.0
>> + pysvd >= 0.2.3
>>
>> # Provides qom-fuse functionality
>> fuse =
>> diff --git a/python/tests/minreqs.txt b/python/tests/minreqs.txt
>> index a3f423efd8..7993fcd23c 100644
>> --- a/python/tests/minreqs.txt
>> +++ b/python/tests/minreqs.txt
>> @@ -22,6 +22,9 @@ distlib==0.3.6
>> # Dependencies for FUSE support for qom-fuse
>> fusepy==2.0.4
>>
>> +# Dependencies for svd-gen-regs
>> +pysvd==0.2.3
>> +
>> # Test-runners, utilities, etc.
>> avocado-framework==90.0
>>
>> diff --git a/pythondeps.toml b/pythondeps.toml
>> index 9c16602d30..8416b17650 100644
>> --- a/pythondeps.toml
>> +++ b/pythondeps.toml
>> @@ -32,3 +32,6 @@ sphinx_rtd_theme = { accepted = ">=0.5", installed = "1.1.1" }
>> # avocado-framework, for example right now the limit is 92.x.
>> avocado-framework = { accepted = "(>=88.1, <93.0)", installed = "88.1", canary = "avocado" }
>> pycdlib = { accepted = ">=1.11.0" }
>> +
>> +[svd-gen-header]
>> +pysvd = { accepted = ">=0.2.3.", installed = "0.2.3" }
>> diff --git a/scripts/svd-gen-header.py b/scripts/svd-gen-header.py
>> new file mode 100755
>> index 0000000000..ab8cb4b665
>> --- /dev/null
>> +++ b/scripts/svd-gen-header.py
>> @@ -0,0 +1,342 @@
>> +#!/usr/bin/env python3
>> +
>> +# Copyright 2024 Google LLC
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or later.
>> +# See the COPYING file in the top-level directory.
>> +#
>> +# Use this script to generate a C header file from an SVD xml
>> +#
>> +# Two mode of operations are supported: peripheral and system.
>> +#
>> +# When running in peripheral mode a header for a specific peripheral
>> +# is going to be generated. It will define a type and structure with
>> +# all of the available registers at the bitfield level. An array that
>> +# contains the reigster names indexed by address is also going to be
>> +# generated as well as a function to initialize registers to their
>> +# reset values.
>> +#
>> +# Invocation example:
>> +#
>> +# svd_gen_header -i MIMXRT595S_cm33.xml -o flexcomm.h -p FLEXCOMM0 -t FLEXCOMM
>> +#
>> +# When running in system mode a header for a specific system /
>> +# platform will be generated. It will define register base addresses
>> +# and interrupt numbers for selected peripherals.
>> +#
>> +# Invocation example:
>> +#
>> +# svd_gen_header -i MIMXRT595S_cm33.xml -o rt500.h -s RT500 -p FLEXCOMM0 \
>> +# -p CLKCTL0 -p CLKCTL1
>> +#
>> +
>> +import argparse
>> +import datetime
>> +import re
>> +import os
>> +import sys
>> +import xml.etree.ElementTree
>> +import pysvd
>> +
>> +data_type_by_bits = {
>> + 8: "uint8_t",
>> + 16: "uint16_t",
>> + 32: "uint32_t",
>> +}
>> +
>> +
>> +def get_register_array_name_and_size(register):
>> + """Return register name and register array size.
>> +
>> + The SVD can define register arrays and pysvd encodes the whole set
>> + as as regular register with their name prepended by [<array size>].
>> +
>> + Returns a tuple with the register name and the size of the array or
>> + zero if this is not a register set.
>> +
>> + """
>> +
>> + split = re.split(r"[\[\]]", register.name)
>> + return (split[0], int(split[1]) if len(split) > 1 else 0)
>> +
>> +
>> +def generate_register(register):
>> + """Generate register data.
>> +
>> + This include a field for accessing the full 32bits as we as
>> + bitfield based register fields.
>> +
>> + """
>> +
>> + data_type = data_type_by_bits[register.size]
>> +
>> + out = f" /* 0x{register.addressOffset:08X} {register.description} */\n"
>> + out += " union {\n"
>> + out += f" {data_type} {register.name};\n"
>> + out += " struct {\n"
>> +
>> + fields = sorted(register.fields, key=lambda field: field.bitOffset)
>> + last_msb = -1
>> + for field in fields:
>> + reserve_bits = 0
>> + lsb = field.bitOffset
>> + msb = field.bitWidth + lsb - 1
>> +
>> + if last_msb == -1 and lsb > 0:
>> + reserve_bits = lsb
>> +
>> + if last_msb != -1 and (lsb - last_msb) > 1:
>> + reserve_bits = lsb - last_msb - 1
>> +
>> + if reserve_bits > 0:
>> + out += f" {data_type}:{reserve_bits};\n"
>> +
>> + out += f" /* [{msb}..{lsb}] {field.description} */\n"
>> + out += f" {data_type} {field.name} : {field.bitWidth};\n"
>> +
>> + last_msb = msb
>> +
>> + if register.size - last_msb > 1:
>> + out += f" {data_type}:{register.size - last_msb - 1};\n"
>> +
>> + reg_name, reg_array_size = get_register_array_name_and_size(register)
>> + if reg_array_size > 0:
>> + out += f" }} {reg_name}_b[{reg_array_size}];\n"
>> + else:
>> + out += f" }} {reg_name}_b;\n"
>> + out += " };\n\n"
>> +
>> + return out
>> +
>> +
>> +def generate_pos_and_msk_defines(name, periph):
>> + """Generate Pos and Msk defines"""
>> +
>> + out = "\n"
>> + for reg in periph.registers:
>> + for field in reg.fields:
>> + reg_name, _ = get_register_array_name_and_size(reg)
>> + field_name = f"{name}_{reg_name}_{field.name}"
>> + out += f"#define {field_name}_Pos ({field.bitOffset}UL)\n"
>> + mask = ((1 << field.bitWidth) - 1) << field.bitOffset
>> + out += f"#define {field_name}_Msk (0x{mask:x}UL)\n"
>> +
>> + return out
>> +
>> +
>> +def generate_enum_values(name, periph):
>> + """Generate enum values"""
>> +
>> + out = "\n"
>> + for reg in periph.registers:
>> + reg_name, _ = get_register_array_name_and_size(reg)
>> + for field in reg.fields:
>> + if hasattr(field, "enumeratedValues"):
>> + out += "\n"
>> + out += "typedef enum {\n"
>> + for enum in field.enumeratedValues.enumeratedValues:
>> + enum_name = f"{name}_{reg_name}_{field.name}_{enum.name}"
>> + out += f" /* {enum.description} */\n"
>> + out += f" {enum_name} = {enum.value},\n"
>> + out += f"}} {name}_{reg_name}_{field.name}_Enum;\n"
>> +
>> + return out
>> +
>> +
>> +def generate_register_names_array_macro(name, periph):
>> + """Generate register names array macro"""
>> +
>> + out = f"#define {name}_REGISTER_NAMES_ARRAY(_name) \\\n"
>> + out += f" const char *_name[sizeof({name}_Type)] = {{ \\\n"
>> + for reg in periph.registers:
>> + reg_name, reg_array_size = get_register_array_name_and_size(reg)
>> + if reg_array_size > 0:
>> + for i in range(0, reg_array_size):
>> + start = reg.addressOffset + i * reg.size // 8
>> + stop = reg.addressOffset + (i + 1) * reg.size // 8 - 1
>> + out += f' [{start} ... {stop}] = "{reg_name}{i}", \\\n'
>> + else:
>> + start = reg.addressOffset
>> + stop = reg.addressOffset + reg.size // 8 - 1
>> + out += f' [{start} ... {stop}] = "{reg.name}", \\\n'
>> + out += " }\n"
>> +
>> + return out
>> +
>> +
>> +def generate_reset_registers_function(name, periph):
>> + """Generate reset registers function"""
>> +
>> + out = "\n"
>> + fname = f"{name.lower()}_reset_registers"
>> + out += f"static inline void {fname}({name}_Type *regs)\n"
>> + out += "{\n"
>> + for reg in periph.registers:
>> + reg_name, reg_array_size = get_register_array_name_and_size(reg)
>> + if reg_array_size > 0:
>> + for i in range(0, int(reg_array_size)):
>> + out += f" regs->{reg_name}[{i}] = {hex(reg.resetValue)};\n"
>> + else:
>> + out += f" regs->{reg_name} = {hex(reg.resetValue)};\n"
>> + out += "}\n"
>> +
>> + return out
>> +
>> +
>> +def generate_peripheral_header(periph, name):
>> + """Generate peripheral header
>> +
>> + The following information is generated:
>> +
>> + * typedef with all of the available registers and register fields,
>> + position and mask defines for register fields.
>> +
>> + * enum values that encode register fields options.
>> +
>> + * a macro that defines the register names indexed by the relative
>> + address of the register.
>> +
>> + * a function that sets the registers to their reset values
>> +
>> + """
>> +
>> + out = f"/* {name} - {periph.description} */\n\n"
>> + out += "typedef struct {\n"
>> +
>> + last_reg_offset = 0
>> + cnt = 0
>> + for reg in periph.registers:
>> + reserved_words = 0
>> + if (reg.addressOffset - last_reg_offset) > 0:
>> + reserved_words = int((reg.addressOffset - last_reg_offset) // 4)
>> + cnt += 1
>> +
>> + if cnt:
>> + show_count = cnt
>> + else:
>> + show_count = ""
>> +
>> + if reserved_words == 1:
>> + out += f" uint32_t RESERVED{show_count};\n\n"
>> + elif reserved_words > 1:
>> + out += f" uint32_t RESERVED{show_count}[{reserved_words}];\n\n"
>> +
>> + out += generate_register(reg)
>> + last_reg_offset = reg.addressOffset + reg.size // 8
>> +
>> + size = periph.addressBlocks[0].size
>> + out += f"}} {name}_Type; /* Size = {size} (0x{size:X}) */\n"
>> +
>> + out += "\n\n"
>> +
>> + out += generate_pos_and_msk_defines(name, periph)
>> +
>> + out += generate_enum_values(name, periph)
>> +
>> + out += generate_register_names_array_macro(name, periph)
>> +
>> + out += generate_reset_registers_function(name, periph)
>> +
>> + return out
>> +
>> +
>> +def get_same_class_peripherals(svd, periph):
>> + """Get a list of peripherals that are instances of the same class."""
>> +
>> + return [periph] + [
>> + p
>> + for p in svd.peripherals
>> + if p.derivedFrom and p.derivedFrom.name == periph.name
>> + ]
>> +
>> +
>> +def generate_system_header(system, svd, periph):
>> + """Generate base and irq defines for given list of peripherals"""
>> +
>> + out = ""
>> +
>> + for p in get_same_class_peripherals(svd, periph):
>> + out += f"#define {system}_{p.name}_BASE 0x{p.baseAddress:X}UL\n"
>> + out += "\n"
>> +
>> + for p in get_same_class_peripherals(svd, periph):
>> + for irq in p.interrupts:
>> + out += f"#define {system}_{irq.name}_IRQn 0x{irq.value}UL\n"
>> + out += "\n"
>> +
>> + return out
>> +
>> +
>> +def main():
>> + """Script to generate C header file from an SVD file"""
>> +
>> + parser = argparse.ArgumentParser()
>> + parser.add_argument(
>> + "-i", "--input", type=str, help="Input SVD file", required=True
>> + )
>> + parser.add_argument(
>> + "-o", "--output", type=str, help="Output .h file", required=True
>> + )
>> + parser.add_argument(
>> + "-p",
>> + "--peripheral",
>> + action="append",
>> + help="peripheral name from the SVD file",
>> + required=True,
>> + )
>> + parser.add_argument(
>> + "-t",
>> + "--type-name",
>> + type=str,
>> + help="name to be used for peripheral definitions",
>> + required=False,
>> + )
>> + parser.add_argument(
>> + "-s",
>> + "--system",
>> + type=str,
>> + help="name to be used for the system definitions",
>> + required=False,
>> + )
>> +
>> + args = parser.parse_args()
>> +
>> + node = xml.etree.ElementTree.parse(args.input).getroot()
>> + svd = pysvd.element.Device(node)
>> +
>> + # Write license header
>> + out = "/*\n"
>> + license_text = svd.licenseText.split("\\n")
>> + for line in license_text:
>> + sline = f" * {line.strip()}"
>> + out += f"{sline.rstrip()}\n"
>> +
>> + now = datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S")
>> + out += f" * @note Automatically generated by {os.path.basename(__file__)}"
>> + out += f" on {now} UTC from {os.path.basename(args.input)}.\n"
>> + out += " *\n */\n\n"
>> +
>> + # Write some generic defines
>> + out += "#pragma once\n\n"
>> +
>> + for name in args.peripheral:
>> + periph = svd.find(name)
>> + if periph:
>> + if args.system:
>> + out += generate_system_header(args.system, svd, periph)
>> + else:
>> + out += generate_peripheral_header(
>> + periph, args.type_name if args.type_name else periph.name
>> + )
>> + else:
>> + print(f"No such peripheral: {name}")
>> + return 1
>> +
>> + with open(args.output, "w", encoding="ascii") as output:
>> + output.write(out)
>> +
>> + return 0
>> +
>> +
>> +if __name__ == "__main__":
>> + sys.exit(main())
>> --
>> 2.46.0.rc2.264.g509ed76dc8-goog
>>
On 9/8/24 00:30, Octavian Purdila wrote:
> On Thu, Aug 8, 2024 at 2:56 PM John Snow <jsnow@redhat.com> wrote:
>>> diff --git a/configure b/configure
>>> index 5ad1674ca5..811bfa5d54 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -956,7 +956,7 @@ mkvenv="$python ${source_path}/python/scripts/mkvenv.py"
>>> # Finish preparing the virtual environment using vendored .whl files
>>>
>>> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
>>> - ${source_path}/pythondeps.toml meson || exit 1
>>> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
>>
>
> Hi John,
>
> Thanks for reviewing!
>
>>
>> I haven't read the rest of this series; I'm chiming in solely from the build/python maintainer angle. Do we *always* need pysvd, no matter how QEMU was configured? Adding it to the meson line here is a very big hammer.
>>
>
> I think the minimum we can do is to install it only if CONFIG_ARM is
> enabled. That might change in the future if the models we create with
> pysvd are enabled for other architectures.
Similarly on how we manage libfdt, you can have meson defines
SVDGEN as:
config_host_data.set('CONFIG_SVDGEN', svd_gen_header.found())
Then declare SVDGEN in Kconfig.host, and finally use in the Kconfigs:
config FLEXCOMM_UART
bool
depends on SVDGEN
config RT500
bool
depends on ARM
select FLEXCOMM_UART
See for FDT examples:
$ git grep 'depends on.*FDT'
hw/core/Kconfig:10: depends on FDT
hw/i386/Kconfig:118: depends on I386 && FDT
hw/loongarch/Kconfig:4: depends on LOONGARCH64 && FDT
hw/mips/Kconfig:84: depends on MIPS64 && !TARGET_BIG_ENDIAN && FDT
hw/ppc/Kconfig:4: depends on PPC64 && FDT
hw/ppc/Kconfig:29: depends on PPC64 && FDT
hw/ppc/Kconfig:58: depends on PPC && FDT
hw/ppc/Kconfig:77: depends on PPC && FDT
hw/ppc/Kconfig:174: depends on PPC && FDT
hw/ppc/Kconfig:180: depends on PPC && FDT
hw/ppc/Kconfig:186: depends on PPC && FDT
hw/rx/Kconfig:11: depends on RX && FDT
hw/xtensa/Kconfig:17: depends on XTENSA && FDT
>> If not, consider looking at how sphinx (the "docs" group) is only conditionally installed into the configure venv and mimic that using the appropriate configure flags that necessitate the availability of pyvsd for the QEMU build.
>
> Thanks for the pointer, I'll take a look.
>
>>
>> We also need to provide a way for pysvd to be available offline; some packages are available via distro libs and if this package is available for every distro we officially support, that's sufficient (but requires updates to our various docker and VM test configuration files to add the new dependency). Otherwise, like we do for meson, we need to vendor the wheel in the tree so offline tarball builds will continue to work.
>>
>> It looks like pysvd is a pure python package with no dependencies, so it should be OK to vendor it in qemu.git/python/wheels/ - look at qemu.git/python/scripts/vendor.py and consider updating and running this script.
>
> Thanks, I'll look at it and add it in v2.
>
>>
>> (The real blocker here is that RPM builds are performed offline and dependencies that cannot be satisfied via rpm can't be added through pip. We need any one of these to be true: (A) pyvsd is available (of a sufficient version) in all distro repositories we target; (B) This build feature is conditional and nobody minds if it never gets enabled for RPM builds; (C) The package can be vendored.)
>>
>> ~~js
>>
>> That said, you might be the first person I've seen outside of Paolo and I to brave mucking around with the python build venv. You deserve a bravery sticker :)
>>
>>>
>>> # At this point, we expect Meson to be installed and available.
>>> # We expect mkvenv or pip to have created pyvenv/bin/meson for us.
>>> diff --git a/meson.build b/meson.build
>>> index ec59effca2..dee587483b 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -3235,6 +3235,10 @@ tracetool_depends = files(
>>> 'scripts/tracetool/vcpu.py'
>>> )
>>>
>>> +svd_gen_header = [
>>> + python, files('scripts/svd-gen-header.py')
>>> +]
>>> +
>>> qemu_version_cmd = [find_program('scripts/qemu-version.sh'),
>>> meson.current_source_dir(),
>>> get_option('pkgversion'), meson.project_version()]
>>> diff --git a/python/setup.cfg b/python/setup.cfg
>>> index 48668609d3..bc830c541a 100644
>>> --- a/python/setup.cfg
>>> +++ b/python/setup.cfg
>>> @@ -45,6 +45,7 @@ devel =
>>> urwid >= 2.1.2
>>> urwid-readline >= 0.13
>>> Pygments >= 2.9.0
>>> + pysvd >= 0.2.3
>>>
On Fri, Aug 9, 2024 at 11:30 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 9/8/24 00:30, Octavian Purdila wrote:
> > On Thu, Aug 8, 2024 at 2:56 PM John Snow <jsnow@redhat.com> wrote:
>
>
> >>> diff --git a/configure b/configure
> >>> index 5ad1674ca5..811bfa5d54 100755
> >>> --- a/configure
> >>> +++ b/configure
> >>> @@ -956,7 +956,7 @@ mkvenv="$python ${source_path}/python/scripts/mkvenv.py"
> >>> # Finish preparing the virtual environment using vendored .whl files
> >>>
> >>> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
> >>> - ${source_path}/pythondeps.toml meson || exit 1
> >>> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
> >>
> >
> > Hi John,
> >
> > Thanks for reviewing!
> >
> >>
> >> I haven't read the rest of this series; I'm chiming in solely from the build/python maintainer angle. Do we *always* need pysvd, no matter how QEMU was configured? Adding it to the meson line here is a very big hammer.
> >>
> >
> > I think the minimum we can do is to install it only if CONFIG_ARM is
> > enabled. That might change in the future if the models we create with
> > pysvd are enabled for other architectures.
>
> Similarly on how we manage libfdt, you can have meson defines
> SVDGEN as:
>
> config_host_data.set('CONFIG_SVDGEN', svd_gen_header.found())
>
> Then declare SVDGEN in Kconfig.host, and finally use in the Kconfigs:
That would force people to install pysvd on the host, which is a pity
for such a small and little-known library. We have used submodules
in the past for much larger and common dependencies, for example
capstone.
Paolo
On Fri, Aug 09, 2024 at 11:42:38AM +0200, Paolo Bonzini wrote:
> On Fri, Aug 9, 2024 at 11:30 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > On 9/8/24 00:30, Octavian Purdila wrote:
> > > On Thu, Aug 8, 2024 at 2:56 PM John Snow <jsnow@redhat.com> wrote:
> >
> >
> > >>> diff --git a/configure b/configure
> > >>> index 5ad1674ca5..811bfa5d54 100755
> > >>> --- a/configure
> > >>> +++ b/configure
> > >>> @@ -956,7 +956,7 @@ mkvenv="$python ${source_path}/python/scripts/mkvenv.py"
> > >>> # Finish preparing the virtual environment using vendored .whl files
> > >>>
> > >>> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
> > >>> - ${source_path}/pythondeps.toml meson || exit 1
> > >>> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
> > >>
> > >
> > > Hi John,
> > >
> > > Thanks for reviewing!
> > >
> > >>
> > >> I haven't read the rest of this series; I'm chiming in solely from the build/python maintainer angle. Do we *always* need pysvd, no matter how QEMU was configured? Adding it to the meson line here is a very big hammer.
> > >>
> > >
> > > I think the minimum we can do is to install it only if CONFIG_ARM is
> > > enabled. That might change in the future if the models we create with
> > > pysvd are enabled for other architectures.
> >
> > Similarly on how we manage libfdt, you can have meson defines
> > SVDGEN as:
> >
> > config_host_data.set('CONFIG_SVDGEN', svd_gen_header.found())
> >
> > Then declare SVDGEN in Kconfig.host, and finally use in the Kconfigs:
>
> That would force people to install pysvd on the host, which is a pity
> for such a small and little-known library. We have used submodules
> in the past for much larger and common dependencies, for example
> capstone.
As mentioned elsewhere in thsi threads, IMHO we shoud not be running
this generator during the build at all. It should be run once to
create the headers needed for a particular device & the output committed
to QEMU. There after any changes to the header (if any) need reviewing
to ensure they don't imply an impact on guest ABI. This avoids having
the 8.6 MB XML file in QEMU git too, as well as the pysvd dep on the
host. Only the very rare times when we create a new device would need
to have the pysvd code & XML.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 9/8/24 11:59, Daniel P. Berrangé wrote:
> On Fri, Aug 09, 2024 at 11:42:38AM +0200, Paolo Bonzini wrote:
>> On Fri, Aug 9, 2024 at 11:30 AM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>
>>> On 9/8/24 00:30, Octavian Purdila wrote:
>>>> On Thu, Aug 8, 2024 at 2:56 PM John Snow <jsnow@redhat.com> wrote:
>>>
>>>
>>>>>> diff --git a/configure b/configure
>>>>>> index 5ad1674ca5..811bfa5d54 100755
>>>>>> --- a/configure
>>>>>> +++ b/configure
>>>>>> @@ -956,7 +956,7 @@ mkvenv="$python ${source_path}/python/scripts/mkvenv.py"
>>>>>> # Finish preparing the virtual environment using vendored .whl files
>>>>>>
>>>>>> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
>>>>>> - ${source_path}/pythondeps.toml meson || exit 1
>>>>>> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
>>>>>
>>>>
>>>> Hi John,
>>>>
>>>> Thanks for reviewing!
>>>>
>>>>>
>>>>> I haven't read the rest of this series; I'm chiming in solely from the build/python maintainer angle. Do we *always* need pysvd, no matter how QEMU was configured? Adding it to the meson line here is a very big hammer.
>>>>>
>>>>
>>>> I think the minimum we can do is to install it only if CONFIG_ARM is
>>>> enabled. That might change in the future if the models we create with
>>>> pysvd are enabled for other architectures.
>>>
>>> Similarly on how we manage libfdt, you can have meson defines
>>> SVDGEN as:
>>>
>>> config_host_data.set('CONFIG_SVDGEN', svd_gen_header.found())
>>>
>>> Then declare SVDGEN in Kconfig.host, and finally use in the Kconfigs:
>>
>> That would force people to install pysvd on the host, which is a pity
>> for such a small and little-known library. We have used submodules
>> in the past for much larger and common dependencies, for example
>> capstone.
>
> As mentioned elsewhere in thsi threads, IMHO we shoud not be running
> this generator during the build at all. It should be run once to
> create the headers needed for a particular device & the output committed
> to QEMU. There after any changes to the header (if any) need reviewing
> to ensure they don't imply an impact on guest ABI. This avoids having
> the 8.6 MB XML file in QEMU git too, as well as the pysvd dep on the
> host. Only the very rare times when we create a new device would need
> to have the pysvd code & XML.
OK, fine by me, as long as we don't end in a similar use of dtc where
we use distrib dtc to rebuild outdated dts and emit a bunch of pointless
warnings cluttering the build output and that nobody agrees how to fix.
On Thu, Aug 8, 2024 at 6:30 PM Octavian Purdila <tavip@google.com> wrote:
> On Thu, Aug 8, 2024 at 2:56 PM John Snow <jsnow@redhat.com> wrote:
> >
> >
> >
> > On Mon, Aug 5, 2024 at 4:17 PM Octavian Purdila <tavip@google.com>
> wrote:
> >>
> >> From: Stefan Stanacar <stefanst@google.com>
> >>
> >> From: Stefan Stanacar <stefanst@google.com>
> >>
> >> The CMSIS System View Description format(CMSIS-SVD) is an XML based
> >> description of Arm Cortex-M microcontrollers provided and maintained
> >> by sillicon vendors. It includes details such as peripherals registers
> >> (down to bitfields), peripheral register block addresses, reset
> >> values, etc.
> >>
> >> This script uses this information to create header files that makes it
> >> easier to emulate peripherals.
> >>
> >> The script can be used to create either peripheral specific headers or
> >> board / system specific information. The script generated headers are
> >> similar to the SVDConv utility.
> >>
> >> Peripheral specific headers contains information such as register
> >> layout, register names and reset values for registers:
> >>
> >> typedef struct {
> >> ...
> >> union {
> >> uint32_t PSELID; /* 0x00000FF8 Peripheral Select and
> >> * Flexcomm module ID */
> >> struct {
> >> uint32_t PERSEL : 3; /* [2..0] Peripheral Select */
> >> uint32_t LOCK : 1; /* [3..3] Lock the peripheral
> select */
> >> uint32_t USARTPRESENT : 1; /* [4..4] USART present indicator */
> >> uint32_t SPIPRESENT : 1; /* [5..5] SPI present indicator */
> >> uint32_t I2CPRESENT : 1; /* [6..6] I2C present indicator */
> >> uint32_t I2SPRESENT : 1; /* [7..7] I2S Present */
> >> uint32_t : 4;
> >> uint32_t ID : 20; /* [31..12] Flexcomm ID */
> >> } PSELID_b;
> >> };
> >> ...
> >> } FLEXCOMM_Type; /* Size = 4096 (0x1000) */
> >>
> >> #define FLEXCOMM_PSELID_PERSEL_Pos (0UL)
> >> #define FLEXCOMM_PSELID_PERSEL_Msk (0x7UL)
> >> #define FLEXCOMM_PSELID_LOCK_Pos (3UL)
> >> #define FLEXCOMM_PSELID_LOCK_Msk (0x8UL)
> >> ...
> >>
> >> typedef enum { /* FLEXCOMM_PSELID_LOCK */
> >> /* Peripheral select can be changed by software. */
> >> FLEXCOMM_PSELID_LOCK_UNLOCKED = 0,
> >> /* Peripheral select is locked and cannot be changed until this
> >> * Flexcomm module or the entire device is reset. */
> >> FLEXCOMM_PSELID_LOCK_LOCKED = 1,
> >> } FLEXCOMM_PSELID_LOCK_Enum;
> >> ...
> >>
> >> #define FLEXCOMM_REGISTER_NAMES_ARRAY(_name) \
> >> const char *_name[sizeof(FLEXCOMM_Type)] = { \
> >> [4088 ... 4091] = "PSELID", \
> >> [4092 ... 4095] = "PID", \
> >> }
> >>
> >> Board specific headers contains information about peripheral base
> >> register addresses.
> >>
> >> Signed-off-by: Stefan Stanacar <stefanst@google.com>
> >> Signed-off-by: Octavian Purdila <tavip@google.com>
> >> ---
> >> configure | 2 +-
> >> meson.build | 4 +
> >> python/setup.cfg | 1 +
> >> python/tests/minreqs.txt | 3 +
> >> pythondeps.toml | 3 +
> >> scripts/svd-gen-header.py | 342 ++++++++++++++++++++++++++++++++++++++
> >> 6 files changed, 354 insertions(+), 1 deletion(-)
> >> create mode 100755 scripts/svd-gen-header.py
> >>
> >> diff --git a/configure b/configure
> >> index 5ad1674ca5..811bfa5d54 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -956,7 +956,7 @@ mkvenv="$python
> ${source_path}/python/scripts/mkvenv.py"
> >> # Finish preparing the virtual environment using vendored .whl files
> >>
> >> $mkvenv ensuregroup --dir "${source_path}/python/wheels" \
> >> - ${source_path}/pythondeps.toml meson || exit 1
> >> + ${source_path}/pythondeps.toml meson svd-gen-header || exit 1
> >
>
> Hi John,
>
> Thanks for reviewing!
>
:)
>
> >
> > I haven't read the rest of this series; I'm chiming in solely from the
> build/python maintainer angle. Do we *always* need pysvd, no matter how
> QEMU was configured? Adding it to the meson line here is a very big hammer.
> >
>
> I think the minimum we can do is to install it only if CONFIG_ARM is
> enabled. That might change in the future if the models we create with
> pysvd are enabled for other architectures.
>
OK, Understood. I will see if Paolo has any thoughts on this; I am
admittedly a little hesitant to add only our 2nd unconditional python
dependency. For context, meson is our first and only such dependency and
Paolo has been very active in development of that project as well; this
would be the first that none of us have any connection with. If the project
were to ever update in such a way that it became not pure-python or
required additional dependencies, the challenges in vendoring it might
cause unscheduled future headaches.
We can always pin to/vendor versions that suit our needs, but it may be
worth sending the developer an email to ask what his policies on
dependencies are, if only to let him know he might have a user out there
that's hedging on it remaining pure python and dep-free, as a courtesy.
(For my money, personally: it's probably fine; but very possibly any
boards/devices/models you add that require it *might* wind up needing a
configure flag that add this dependency, like --enable-svd-devices or
something of the sort. I'll let Paolo comment on that, don't take my word
as gospel - I handle the Python more than I do the build system; you
managed to find the precise intersection of the two.)
>
> > If not, consider looking at how sphinx (the "docs" group) is only
> conditionally installed into the configure venv and mimic that using the
> appropriate configure flags that necessitate the availability of pyvsd for
> the QEMU build.
>
> Thanks for the pointer, I'll take a look.
>
> >
> > We also need to provide a way for pysvd to be available offline; some
> packages are available via distro libs and if this package is available for
> every distro we officially support, that's sufficient (but requires updates
> to our various docker and VM test configuration files to add the new
> dependency). Otherwise, like we do for meson, we need to vendor the wheel
> in the tree so offline tarball builds will continue to work.
> >
> > It looks like pysvd is a pure python package with no dependencies, so it
> should be OK to vendor it in qemu.git/python/wheels/ - look at
> qemu.git/python/scripts/vendor.py and consider updating and running this
> script.
>
> Thanks, I'll look at it and add it in v2.
>
Thanks! Sorry this area of the build system is so persnickety. If you run
into troubles please reach out. I've also pinged Paolo to take a look as
the build system "SME" in case there's something I overlooked, too.
Good luck! :)
~js
>
> >
> > (The real blocker here is that RPM builds are performed offline and
> dependencies that cannot be satisfied via rpm can't be added through pip.
> We need any one of these to be true: (A) pyvsd is available (of a
> sufficient version) in all distro repositories we target; (B) This build
> feature is conditional and nobody minds if it never gets enabled for RPM
> builds; (C) The package can be vendored.)
> >
> > ~~js
> >
> > That said, you might be the first person I've seen outside of Paolo and
> I to brave mucking around with the python build venv. You deserve a bravery
> sticker :)
> >
> >>
>
[...]
© 2016 - 2024 Red Hat, Inc.