Re: [RFC PATCH v2 3/5] rust: add PL011 device model

[Paolo Bonzini <pbonzini@redhat.com>]

On 6/19/24 07:34, Richard Henderson wrote:
> First silly question: how much of this is boiler plate that gets moved 
> the moment that the second rust subdirectory is added?

If my suggestion at 
https://lore.kernel.org/qemu-devel/CABgObfaP7DRD8dbSKNmUzhZNyxeHWO0MztaW3_EFYt=vf6SbzA@mail.gmail.com/ 
works, we'd have only two directories that have a Cargo.toml in it.  For 
example it could be rust/qemu/ (common code) and rust/hw/ (code that 
depends on Kconfig).

I think we can also have a rust/Cargo.toml file as in 
https://doc.rust-lang.org/cargo/reference/workspaces.html#virtual-workspace, 
and then the various configuration files under rust/ will be valid for 
all subpackages.

>> +[build]
>> +rustflags = ["-Crelocation-model=pic", "-Ctarget-feature=+crt-static"]
> 
> It seems certain that this is not specific to pl011, and will be commot to other rust 
> subdirectories.  Or, given the .cargo directory name, is this generated by cargo and 
> committed by mistake?

-Crelocation-mode should be pie.  But also, I am not sure it works 
because I think it's always going to be overridden by cargo_wrapper.py? 
See https://doc.rust-lang.org/cargo/reference/config.html#buildrustflags.

(I'm not sure what +crt-static is for).

I think the generate_cfg_flags() mechanism of cargo_wrapper.py has to be 
rewritten from Python to Rust and moved to build.rs (using 
cargo::rustc-cfg).  By doing this, the cfg flags are added to whatever 
is in .cargo/config.toml, rather than replaced.

>> diff --git a/rust/pl011/.gitignore b/rust/pl011/.gitignore
>> new file mode 100644
>> index 0000000000..28a02c847f
>> --- /dev/null
>> +++ b/rust/pl011/.gitignore
>> @@ -0,0 +1,2 @@
>> +target
>> +src/generated.rs.inc
> 
> Is this a symptom of generating files into the source directory and not 
> build directory?

If I understand correctly, Manos considered two possible ways to invoke 
cargo on the Rust code:

- directly, in which case you need to copy the generated source file to 
rust/pl011/src/generated.rs.inc, because cargo does not know where the 
build tree

- do everything through meson, which does the right thing because 
cargo_wrapper.py knows about the build tree and passes the information.

To avoid this, the first workflow could be adjusted so that cargo can 
still be invoked directly, _but from the build tree_, not the source 
tree.  For example configure could generate a 
.../build/.cargo/config.toml with

    [env]
    MESON_BUILD_ROOT = ".../build"

(extra advantage: -Crelocation-model=pie can be added based on 
--enable-pie/--disable-pie).  configure can also create symlinks in the 
build tree for the source tree's rust/, Cargo.toml and Cargo.lock.

This allows rust/pl011/src/generated.rs (which probably will become 
something like rust/common/src/generated.rs) to be:

    include!(concat!(env!("MESON_BUILD_ROOT"), "/generated.rs.inc"));

when cargo is invoked from the build tree.

Putting everything together you'd have

    build/
      rust/
        .cargo/
          config.toml   # generated by configure or meson.build
        Cargo.toml      # workspace generated by configure or meson.build
        Cargo.lock      # can be either linked to srctree or generated
        qemu/           # symlink to srctree/rust/qemu
        aarch64-softmmu-hw/
          Cargo.toml    # generated by meson.build (*)
          src/          # symlink to srctree/rust/hw/
        i386-softmmu-hw/
          Cargo.toml    # generated by meson.build
          src/          # symlink to srctree/rust/hw/
        generated/      # files generated by rust/generated/meson.build

(*) these have to be generated to change the package name, so 
configure_file() seems like a good match for it.

This is suspiciously similar to what tests/tcg/ looks like, except that 
tests/tcg/*/Makefile is just a symbolic link.  I tried creating a 
similar directory structure in a toy project, and it seemed to work...

> Second silly question: does this really need to be committed to the 
> repository? It *appears* to be specific to the host+os-version of the 
> build.  It is certainly very specific about versions and checksums...

Generally the idea is that libraries should not commit it, while 
applications should commit it.  The idea is that the Cargo.lock file 
reproduces a working configuration, and dependencies are updated to 
known-good releases when CI passes.  I don't think I like this, but it 
is what it is.  I ascribe it to me being from the Jurassic.

But for now I would consider not committing Cargo.lock, because we don't 
have the infrastructure to do that automatic dependency update (assuming 
we want it).  But we could generate it at release time so that it is 
included in tarballs, and create the symlink from 
srctree/rust/Cargo.lock into build/rust/ only if the file is present in 
the source tree.

>> diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
>> [...]
>> +# bilge deps included here to include them with docs
>> +[dependencies]
>> +arbitrary-int = { version = "1.2.7" }
>> +bilge = { version = "0.2.0" }
>> +bilge-impl = { version = "0.2.0" }

This one has to be included, but it is less restrictive than it seems.

It is equivalent to

arbitrary-int = { version = ">=1.2.7, <2.0.0" }
bilge = { version = ">=0.2.0, <0.3.0" }
bilge-impl = { version = ">=0.2.0, <0.3.0" }

That is, it assumes an API breakage when the first nonzero component 
changes in the version.  It is also possible to put a caret in front of 
the version, so that it's clearer that this is a range; but the behavior 
is the same.

Paolo