Hello Zhao!
On Mon, 24 Jun 2024 13:12, Zhao Liu <zhao1....@intel.com> wrote:
Hi Manos,
Thanks for your patch. I have a few comments below:
diff --git a/meson.build b/meson.build
index 3533889852..2b305e745a 100644
--- a/meson.build
+++ b/meson.build
@@ -3876,6 +3876,62 @@ foreach target : target_dirs
lib_deps += dep.partial_dependency(compile_args: true, includes: true)
endforeach
+ if with_rust and target_type == 'system'
+ # FIXME:
Just nit, FIXME looks a bit ambiguous, is it intended to fix the
following warning? It could be a bit more specific.
Yes it could, that's true. Basically meson prints this warning when it
reconfigures the build targets. So to use these newer features in our
minimum supported meson version the code should be fixed to be backwards
compatible.
I will add a sentence explaining that this is meson's output.
+ # > WARNING: Project specifies a minimum meson_version '>=0.63.0' but
+ # > uses features which were added in newer versions:
+ # > * 0.64.0: {'fs.copyfile'}
+ # > * 1.0.0: {'dependencies arg in rust.bindgen', 'module rust as
stable module'}
+ rust_bindgen = import('rust')
+
+ # We need one generated_rs target per target, so give them
+ # target-specific names.
+ copy = fs.copyfile('rust/wrapper.h',
+ target + '_wrapper.h')
+ generated_rs = rust_bindgen.bindgen(
+ input: copy,
+ dependencies: arch_deps + lib_deps,
+ output: target + '-generated.rs',
+ include_directories: include_directories('.', 'include'),
+ args: [
+ '--ctypes-prefix', 'core::ffi',
+ '--formatter', 'rustfmt',
+ '--generate-block',
+ '--generate-cstr',
+ '--impl-debug',
+ '--merge-extern-blocks',
+ '--no-doc-comments',
+ '--no-include-path-detection',
+ '--use-core',
+ '--with-derive-default',
+ '--allowlist-file', meson.project_source_root() + '/include/.*',
+ '--allowlist-file', meson.project_source_root() + '/.*',
+ '--allowlist-file', meson.project_build_root() + '/.*'
+ ],
+ )
+
+ if target in rust_targets
+ rust_hw = ss.source_set()
+ foreach t: rust_targets[target]
+ rust_device_cargo = custom_target(t['name'],
+ output: t['output'],
+ depends: [generated_rs],
+ build_always_stale: true,
+ command: t['command'])
+ rust_dep = declare_dependency(link_args: [
+ '-Wl,--whole-archive',
+ t['output-path'],
+ '-Wl,--no-whole-archive'
+ ],
+ sources: [rust_device_cargo])
+ rust_hw.add(rust_dep)
+ endforeach
+ rust_hw_config = rust_hw.apply(config_target, strict: false)
+ arch_srcs += rust_hw_config.sources()
+ arch_deps += rust_hw_config.dependencies()
+ endif
+ endif
+
lib = static_library('qemu-' + target,
sources: arch_srcs + genh,
dependencies: lib_deps,
diff --git a/rust/.gitignore b/rust/.gitignore
new file mode 100644
index 0000000000..1bf71b1f68
--- /dev/null
+++ b/rust/.gitignore
@@ -0,0 +1,3 @@
+# Ignore any cargo development build artifacts; for qemu-wide builds, all build
+# artifacts will go to the meson build directory.
+target
diff --git a/rust/meson.build b/rust/meson.build
new file mode 100644
index 0000000000..435abd3e1c
--- /dev/null
+++ b/rust/meson.build
@@ -0,0 +1,129 @@
+# Supported hosts
+rust_supported_oses = {
+ 'linux': '-unknown-linux-gnu',
+ 'darwin': '-apple-darwin',
+ 'windows': '-pc-windows-gnu'
+}
Does the current test cover windows and apple? If not, I think we can
add these two platforms later on.
Do you mean the test for supported OSes?
This is for future-proofing the Rust integration in general. I haven't
been able to compile under macos yet because bindgen cannot find the
system clang header. I also don't have a windows pc to test it on. But
it should work theoretically under all three.
+rust_supported_cpus = ['x86_64', 'aarch64']
Similarly, here I have another question, x-pl011-rust in arm virt machine
I understand should only run on ARM platform, right? So compiling to
x86_64 doesn't seem necessary at the moment?
This refers to the host cpu not the virtualized ones, you can run
qemu-system-aarch64 under x86_64
+# Future-proof the above definitions against any change in the root
meson.build file:
+foreach rust_os: rust_supported_oses.keys()
+ if not supported_oses.contains(rust_os)
+ message()
+ warning('UNSUPPORTED OS VALUES IN ' + meson.current_source_dir() +
'/meson.build')
+ message()
+ message('This meson.build file claims OS `+' + rust_os + '` is supported
but')
+ message('it is not included in the global supported OSes list in')
+ message(meson.global_source_root() + '/meson.build.')
+ endif
+endforeach
+foreach rust_cpu: rust_supported_cpus
+ if not supported_cpus.contains(rust_cpu)
+ message()
+ warning('UNSUPPORTED CPU VALUES IN ' + meson.current_source_dir() +
'/meson.build')
+ message()
+ message('This meson.build file claims CPU `+' + rust_cpu + '` is supported
but')
+ message('it is not included in the global supported CPUs list in')
+ message(meson.global_source_root() + '/meson.build.')
+ endif
+endforeach
+
+# FIXME: retrieve target triple from meson and construct rust_target_triple
+if meson.is_cross_build()
+ message()
+ error('QEMU does not support cross compiling with Rust enabled.')
+endif
Is the check here to avoid inconsistencies between cross-build's target
and rust_target_triple?
If target consistency is not guaranteed, then it seems custom rust_target_triple
is useless, right? please educate me if I am wrong ;-)
It is, correct. I was hoping someone on the list would have advice on
how to best do this (see the FIXME comment right above)
+# FIXME: These are the latest stable versions, refine to actual minimum ones.
+msrv = {
+ 'rustc': '1.79.0',
+ 'cargo': '1.79.0',
+ 'bindgen': '0.69.4',
+}
+
+# rustup = find_program('rustup', required: false)
+foreach bin_dep: msrv.keys()
+ bin = find_program(bin_dep, required: true)
+ if bin.version() < msrv[bin_dep]
+ if bin_dep == 'bindgen' and get_option('wrap_mode') != 'nodownload'
What about adding a download info here?
It took a long time for my bindgen-cli to update quietly, and for a
moment I thought it was stuck here...
e.g., message('Installing bindgen-cli...')
Or can we just report the error and let users install bindgen-cli
themselves?
Now that I think about it again I agree it's better to not mess with
user/system installations without asking.
+ run_command(cargo, 'install', 'bindgen-cli', capture: true, check: true)
+ bin = find_program(bin_dep, required: true)
+ if bin.version() >= msrv[bin_dep]
+ continue
+ endif
+ endif
+ message()
+ error(bin_dep + ' version ' + bin.version() + ' is unsupported: Please
upgrade to at least ' + msrv[bin_dep])
+ endif
+endforeach
+
+rust_target_triple = get_option('with_rust_target_triple')
+
+if rust_target_triple == ''
+ if not supported_oses.contains(host_os)
+ message()
+ error('QEMU does not support `' + host_os +'` as a Rust platform.')
+ elif not supported_cpus.contains(host_arch)
+ message()
+ error('QEMU does not support `' + host_arch +'` as a Rust architecture.')
+ endif
+ rust_target_triple = host_arch + rust_supported_oses[host_os]
+ if host_os == 'windows' and host_arch == 'aarch64'
+ rust_target_triple += 'llvm'
+ endif
+else
+ # verify rust_target_triple if given as an option
+ rustc = find_program('rustc', required: true)
+ rustc_targets = run_command(rustc, '--print', 'target-list', capture: true,
check: true).stdout().strip().split()
+ if not rustc_targets.contains(rust_target_triple)
+ message()
+ error('Given rust_target_triple ' + rust_target_triple + ' is not listed
in rustc --print target-list output')
+ endif
It seems rust_target_triple missed support check, we should use
rust_supported_cpus
+ rust_supported_oses to check rust_target_triple, right? ;-)
+endif
+
+
An extra blank line :-)
+rust_targets = {}
+
+cargo_wrapper = [
+ find_program(meson.global_source_root() / 'scripts/cargo_wrapper.py'),
+ '--config-headers', meson.project_build_root() / 'config-host.h',
+ '--meson-build-root', meson.project_build_root(),
+ '--meson-build-dir', meson.current_build_dir(),
+ '--meson-source-dir', meson.current_source_dir(),
+]
+
+if get_option('b_colorout') != 'never'
+ cargo_wrapper += ['--color', 'always']
+endif
+
+if get_option('optimization') in ['0', '1', 'g']
+ rs_build_type = 'debug'
+else
+ rs_build_type = 'release'
+endif
Could we decouple 'optimization' with cargo's opt-level (maybe in the future)?
Or we could add new option to specify custom rust opt-level, which will
set the environment varialbe CARGO_PROFILE_<profile_name>_OPT_LEVEL and
override the default opt-level in Cargo.toml.
optimization here refers to LLVM's opt flags directly, so I think it
makes sense to follow the project-wide compiler settings?
+# Collect metadata for each (crate,qemu-target,compiler-target) combination.
+# Rust meson targets cannot be defined a priori because they depend on bindgen
+# generation that is created for each emulation target separately. Thus Rust
+# meson targets will be defined for each target after the target-specific
+# bindgen dependency is declared.
+rust_hw_target_list = {}
+
+foreach rust_hw_target, rust_hws: rust_hw_target_list
+ foreach rust_hw_dev: rust_hws
+ output = meson.current_build_dir() / rust_target_triple / rs_build_type /
rust_hw_dev['output']
+ crate_metadata = {
+ 'name': rust_hw_dev['name'],
+ 'output': [rust_hw_dev['output']],
+ 'output-path': output,
+ 'command': [cargo_wrapper,
+ '--crate-dir', meson.current_source_dir() / rust_hw_dev['dirname'],
+ '--profile', rs_build_type,
+ '--target-triple', rust_target_triple,
+ '--outdir', '@OUTDIR@',
+ 'build-lib'
+ ]
+ }
+ rust_targets += { rust_hw_target: [crate_metadata] }
+ endforeach
+endforeach
diff --git a/rust/wrapper.h b/rust/wrapper.h
new file mode 100644
index 0000000000..bcf808c8d7
--- /dev/null
+++ b/rust/wrapper.h
@@ -0,0 +1,39 @@
+/*
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2020 Fabrice Bellard
Here should the author be yourself?
Technically I copy pasted this file from the main.c executable so it's a
derived work :D
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/module.h"
+#include "qemu-io.h"
+#include "sysemu/sysemu.h"
+#include "hw/sysbus.h"
+#include "exec/memory.h"
+#include "chardev/char-fe.h"
+#include "hw/clock.h"
+#include "hw/qdev-clock.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
+#include "hw/irq.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+#include "chardev/char-serial.h"
Thanks,
Zhao
