[RFC PATCH v4 0/7] Add Rust support, implement ARM PL011

Manos Pitsidianakis posted 7 patches 4 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/rust-pl011-rfc-v4.git.manos.pitsidianakis@linaro.org
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Thomas Huth <thuth@redhat.com>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Beraldo Leal <bleal@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Manos Pitsidianakis <manos.pitsidianakis@linaro.org>, John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>
.gitattributes                    |   3 +
.gitlab-ci.d/buildtest.yml        |  64 +++-
MAINTAINERS                       |  20 ++
configure                         |  11 +
hw/arm/virt.c                     |   4 +
meson.build                       |  72 ++++
meson_options.txt                 |   5 +
rust/.cargo/config.toml           |   2 +
rust/.gitignore                   |   3 +
rust/meson.build                  | 114 ++++++
rust/pl011/.gitignore             |   2 +
rust/pl011/Cargo.lock             | 125 +++++++
rust/pl011/Cargo.toml             |  67 ++++
rust/pl011/README.md              |  31 ++
rust/pl011/deny.toml              |  57 +++
rust/pl011/meson.build            |   7 +
rust/pl011/rustfmt.toml           |   1 +
rust/pl011/src/definitions.rs     |  39 +++
rust/pl011/src/device.rs          | 509 +++++++++++++++++++++++++++
rust/pl011/src/device_class.rs    |  48 +++
rust/pl011/src/lib.rs             | 556 ++++++++++++++++++++++++++++++
rust/pl011/src/memory_ops.rs      |  45 +++
rust/qemu-api/.gitignore          |   2 +
rust/qemu-api/Cargo.lock          |   7 +
rust/qemu-api/Cargo.toml          |  59 ++++
rust/qemu-api/README.md           |  17 +
rust/qemu-api/build.rs            |  48 +++
rust/qemu-api/deny.toml           |  57 +++
rust/qemu-api/meson.build         |   0
rust/qemu-api/rustfmt.toml        |   1 +
rust/qemu-api/src/bindings.rs     |   8 +
rust/qemu-api/src/definitions.rs  | 112 ++++++
rust/qemu-api/src/device_class.rs | 131 +++++++
rust/qemu-api/src/lib.rs          |  29 ++
rust/qemu-api/src/tests.rs        |  48 +++
rust/rustfmt.toml                 |   7 +
rust/wrapper.h                    |  39 +++
scripts/cargo_wrapper.py          | 288 ++++++++++++++++
scripts/meson-buildoptions.sh     |   6 +
39 files changed, 2625 insertions(+), 19 deletions(-)
create mode 100644 rust/.cargo/config.toml
create mode 100644 rust/.gitignore
create mode 100644 rust/meson.build
create mode 100644 rust/pl011/.gitignore
create mode 100644 rust/pl011/Cargo.lock
create mode 100644 rust/pl011/Cargo.toml
create mode 100644 rust/pl011/README.md
create mode 100644 rust/pl011/deny.toml
create mode 100644 rust/pl011/meson.build
create mode 120000 rust/pl011/rustfmt.toml
create mode 100644 rust/pl011/src/definitions.rs
create mode 100644 rust/pl011/src/device.rs
create mode 100644 rust/pl011/src/device_class.rs
create mode 100644 rust/pl011/src/lib.rs
create mode 100644 rust/pl011/src/memory_ops.rs
create mode 100644 rust/qemu-api/.gitignore
create mode 100644 rust/qemu-api/Cargo.lock
create mode 100644 rust/qemu-api/Cargo.toml
create mode 100644 rust/qemu-api/README.md
create mode 100644 rust/qemu-api/build.rs
create mode 100644 rust/qemu-api/deny.toml
create mode 100644 rust/qemu-api/meson.build
create mode 120000 rust/qemu-api/rustfmt.toml
create mode 100644 rust/qemu-api/src/bindings.rs
create mode 100644 rust/qemu-api/src/definitions.rs
create mode 100644 rust/qemu-api/src/device_class.rs
create mode 100644 rust/qemu-api/src/lib.rs
create mode 100644 rust/qemu-api/src/tests.rs
create mode 100644 rust/rustfmt.toml
create mode 100644 rust/wrapper.h
create mode 100644 scripts/cargo_wrapper.py
[RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Manos Pitsidianakis 4 months, 3 weeks ago
Changes from v3->v4:
- Add rust-specific files to .gitattributes
- Added help text to scripts/cargo_wrapper.py arguments (thanks Stephan)
- Split bindings separate crate
- Add declarative macros for symbols exported to QEMU to said crate
- Lowered MSRV to 1.77.2
- Removed auto-download and install of bindgen-cli
- Fixed re-compilation of Rust objects in case they are missing from 
  filesystem
- Fixed optimized builds by adding #[used] (thanks Pierrick for the help 
  debugging this)

Also, Pierrick helped confirming it works on Windows with some 
windows-specific changes. I confirmed it works on macos by allowing 
bindgen to detect system paths for clang, which is a workaround and not 
a solution. However this series doesn't have the windows changes 
integrated.

Changes from v2->v3:
- Addressed minor mistakes (thanks Stefan)
- Setup supported version checks for cargo, rustc and bindgen (thanks 
  everyone who pointed it out / suggested it)
- Fixed problem with bindgen failing if certain system headers where 
  needed by defining an allowlist for headers instead of a blocklist for 
  what we don't want (thanks Alex Bennée for reporting it)
- Cleaned up bindgen target/dependendy definition in meson.build by 
  removing unnecessary bits

Changes from v1->v2:
- Create bindgen target first, then add commit for device (thanks 
  Pierrick)
- Create a special named generated.rs for each target as compilation 
  would fail if more than one targets were defined. The generated.rs 
  target names would clash.
- Add more descriptive commit messages
- Update MAINTAINERS
- Cleanup patch order for better review, hopefully


Manos Pitsidianakis (7):
  build-sys: Add rust feature option
  rust: add bindgen step as a meson dependency
  rust: add crate to expose bindings and interfaces
  rust: add PL011 device model
  .gitattributes: add Rust diff and merge attributes
  DO NOT MERGE: add rustdoc build for gitlab pages
  DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine

 .gitattributes                    |   3 +
 .gitlab-ci.d/buildtest.yml        |  64 +++-
 MAINTAINERS                       |  20 ++
 configure                         |  11 +
 hw/arm/virt.c                     |   4 +
 meson.build                       |  72 ++++
 meson_options.txt                 |   5 +
 rust/.cargo/config.toml           |   2 +
 rust/.gitignore                   |   3 +
 rust/meson.build                  | 114 ++++++
 rust/pl011/.gitignore             |   2 +
 rust/pl011/Cargo.lock             | 125 +++++++
 rust/pl011/Cargo.toml             |  67 ++++
 rust/pl011/README.md              |  31 ++
 rust/pl011/deny.toml              |  57 +++
 rust/pl011/meson.build            |   7 +
 rust/pl011/rustfmt.toml           |   1 +
 rust/pl011/src/definitions.rs     |  39 +++
 rust/pl011/src/device.rs          | 509 +++++++++++++++++++++++++++
 rust/pl011/src/device_class.rs    |  48 +++
 rust/pl011/src/lib.rs             | 556 ++++++++++++++++++++++++++++++
 rust/pl011/src/memory_ops.rs      |  45 +++
 rust/qemu-api/.gitignore          |   2 +
 rust/qemu-api/Cargo.lock          |   7 +
 rust/qemu-api/Cargo.toml          |  59 ++++
 rust/qemu-api/README.md           |  17 +
 rust/qemu-api/build.rs            |  48 +++
 rust/qemu-api/deny.toml           |  57 +++
 rust/qemu-api/meson.build         |   0
 rust/qemu-api/rustfmt.toml        |   1 +
 rust/qemu-api/src/bindings.rs     |   8 +
 rust/qemu-api/src/definitions.rs  | 112 ++++++
 rust/qemu-api/src/device_class.rs | 131 +++++++
 rust/qemu-api/src/lib.rs          |  29 ++
 rust/qemu-api/src/tests.rs        |  48 +++
 rust/rustfmt.toml                 |   7 +
 rust/wrapper.h                    |  39 +++
 scripts/cargo_wrapper.py          | 288 ++++++++++++++++
 scripts/meson-buildoptions.sh     |   6 +
 39 files changed, 2625 insertions(+), 19 deletions(-)
 create mode 100644 rust/.cargo/config.toml
 create mode 100644 rust/.gitignore
 create mode 100644 rust/meson.build
 create mode 100644 rust/pl011/.gitignore
 create mode 100644 rust/pl011/Cargo.lock
 create mode 100644 rust/pl011/Cargo.toml
 create mode 100644 rust/pl011/README.md
 create mode 100644 rust/pl011/deny.toml
 create mode 100644 rust/pl011/meson.build
 create mode 120000 rust/pl011/rustfmt.toml
 create mode 100644 rust/pl011/src/definitions.rs
 create mode 100644 rust/pl011/src/device.rs
 create mode 100644 rust/pl011/src/device_class.rs
 create mode 100644 rust/pl011/src/lib.rs
 create mode 100644 rust/pl011/src/memory_ops.rs
 create mode 100644 rust/qemu-api/.gitignore
 create mode 100644 rust/qemu-api/Cargo.lock
 create mode 100644 rust/qemu-api/Cargo.toml
 create mode 100644 rust/qemu-api/README.md
 create mode 100644 rust/qemu-api/build.rs
 create mode 100644 rust/qemu-api/deny.toml
 create mode 100644 rust/qemu-api/meson.build
 create mode 120000 rust/qemu-api/rustfmt.toml
 create mode 100644 rust/qemu-api/src/bindings.rs
 create mode 100644 rust/qemu-api/src/definitions.rs
 create mode 100644 rust/qemu-api/src/device_class.rs
 create mode 100644 rust/qemu-api/src/lib.rs
 create mode 100644 rust/qemu-api/src/tests.rs
 create mode 100644 rust/rustfmt.toml
 create mode 100644 rust/wrapper.h
 create mode 100644 scripts/cargo_wrapper.py


base-commit: 7914bda497f07965f15a91905cd7ed9eaf1c1092
-- 
γαῖα πυρί μιχθήτω


Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Paolo Bonzini 4 months, 2 weeks ago
On 7/4/24 14:15, Manos Pitsidianakis wrote:
> Changes from v3->v4:
> - Add rust-specific files to .gitattributes
> - Added help text to scripts/cargo_wrapper.py arguments (thanks Stephan)
> - Split bindings separate crate
> - Add declarative macros for symbols exported to QEMU to said crate
> - Lowered MSRV to 1.77.2
> - Removed auto-download and install of bindgen-cli
> - Fixed re-compilation of Rust objects in case they are missing from
>    filesystem
> - Fixed optimized builds by adding #[used] (thanks Pierrick for the help
>    debugging this)

I think the largest issue is that I'd rather have a single cargo build 
using a virtual manifest, because my hunch is that it'd be the easiest 
path towards Kconfig integration.  But it's better to do this after 
merge, as the changes are pretty large.  It's also independent from any 
other changes targeted at removing unsafe code, so no need to hold back 
on merging.

Other comments I made that should however be addressed before merging, 
from most to least important:

- TODO comments when the code is doing potential undefined behavior

- module structure should IMO resemble the C part of the tree

- only generate bindings.rs.inc once

- a couple abstractions that I'd like to have now: a trait to store the 
CStr corresponding to the structs, and one to generate all-zero structs 
without having to type "unsafe { MaybeUninit::zeroed().assume_init() }"

- I pointed out a couple lints that are too broad and should be enabled 
per-file, even if right now it's basically all files that include them.

- add support for --cargo and CARGO environment variables (if my patch 
works without too much hassle)

- I'd like to use ctor instead of non-portable linker magic, and the 
cstr crate instead of CStr statics or c""

- please check if -Wl,--whole-archive can be replaced with link_whole:

- probably, until Rust is enabled by default we should treat 
dependencies as a moving target and not commit Cargo.lock files.  In the 
meanwhile we can discuss how to handle them.

And a few aesthetic changes on top of this.

With respect to lints, marking entire groups as "deny" is problematic. 
Before merge, I'd rather have the groups as just "warn", and add a long 
list of denied lints[1].  After merge we can also add a non-fatal CI job 
that runs clippy with nightly rust and with groups marked as "deny". 
This matches the check-python-tox job in python/.

[1] https://github.com/bonzini/rust-qemu/commit/95b25f7c5f4e

Thanks,

Paolo
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Daniel P. Berrangé 4 months, 2 weeks ago
On Mon, Jul 08, 2024 at 06:26:22PM +0200, Paolo Bonzini wrote:
> On 7/4/24 14:15, Manos Pitsidianakis wrote:
> > Changes from v3->v4:
> > - Add rust-specific files to .gitattributes
> > - Added help text to scripts/cargo_wrapper.py arguments (thanks Stephan)
> > - Split bindings separate crate
> > - Add declarative macros for symbols exported to QEMU to said crate
> > - Lowered MSRV to 1.77.2
> > - Removed auto-download and install of bindgen-cli
> > - Fixed re-compilation of Rust objects in case they are missing from
> >    filesystem
> > - Fixed optimized builds by adding #[used] (thanks Pierrick for the help
> >    debugging this)
> 
> I think the largest issue is that I'd rather have a single cargo build using
> a virtual manifest, because my hunch is that it'd be the easiest path
> towards Kconfig integration.  But it's better to do this after merge, as the
> changes are pretty large.  It's also independent from any other changes
> targeted at removing unsafe code, so no need to hold back on merging.
> 
> Other comments I made that should however be addressed before merging, from
> most to least important:
> 
> - TODO comments when the code is doing potential undefined behavior
> 
> - module structure should IMO resemble the C part of the tree
> 
> - only generate bindings.rs.inc once
> 
> - a couple abstractions that I'd like to have now: a trait to store the CStr
> corresponding to the structs, and one to generate all-zero structs without
> having to type "unsafe { MaybeUninit::zeroed().assume_init() }"
> 
> - I pointed out a couple lints that are too broad and should be enabled
> per-file, even if right now it's basically all files that include them.
> 
> - add support for --cargo and CARGO environment variables (if my patch works
> without too much hassle)
> 
> - I'd like to use ctor instead of non-portable linker magic, and the cstr
> crate instead of CStr statics or c""
> 
> - please check if -Wl,--whole-archive can be replaced with link_whole:
> 
> - probably, until Rust is enabled by default we should treat dependencies as
> a moving target and not commit Cargo.lock files.  In the meanwhile we can
> discuss how to handle them.
> 
> And a few aesthetic changes on top of this.

This series is still missing changes to enable build on all targets
during CI, including cross-compiles, to prove that we're doing the
correct thing on all our targetted platforms. That's a must have
before considering it suitable for merge.

I also believe we should default to enabling rust toolchain by
default in configure, and require and explicit --without-rust
to disable it, *despite* it not technically being a mandatory
feature....yet.

This is to give users a clear message that Rust is likely to
become a fundamental part of QEMU, so they need to give feedback
if they hit any problems / have use cases we've not anticipated
that are problematic wrt Rust.

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 :|
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Paolo Bonzini 4 months, 2 weeks ago
Il lun 8 lug 2024, 18:33 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

> This series is still missing changes to enable build on all targets
> during CI, including cross-compiles, to prove that we're doing the
> correct thing on all our targetted platforms. That's a must have
> before considering it suitable for merge.
>

But we're not—in particular it's still using several features not in all
supported distros.

I also believe we should default to enabling rust toolchain by
> default in configure, and require and explicit --without-rust
> to disable it, *despite* it not technically being a mandatory
> feature....yet.
>

I guess the detection could be done, but actually enabling the build part
needs to wait until the minimum supported version is low enough.

Paolo


> This is to give users a clear message that Rust is likely to
> become a fundamental part of QEMU, so they need to give feedback
> if they hit any problems / have use cases we've not anticipated
> that are problematic wrt Rust.
>
> 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 :|
>
>
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Daniel P. Berrangé 4 months, 2 weeks ago
On Mon, Jul 08, 2024 at 06:55:40PM +0200, Paolo Bonzini wrote:
> Il lun 8 lug 2024, 18:33 Daniel P. Berrangé <berrange@redhat.com> ha
> scritto:
> 
> > This series is still missing changes to enable build on all targets
> > during CI, including cross-compiles, to prove that we're doing the
> > correct thing on all our targetted platforms. That's a must have
> > before considering it suitable for merge.
> >
> 
> But we're not—in particular it's still using several features not in all
> supported distros.

That's exactly why I suggest its a pre-requisite for merging
this. Unless we're able to demonstrate that we can enable
Rust on all our CI platforms, the benefits of Rust will
not be realized in QEMU, and we'll have never ending debates
about whether each given feature needs to be in C or Rust.


> I also believe we should default to enabling rust toolchain by
> > default in configure, and require and explicit --without-rust
> > to disable it, *despite* it not technically being a mandatory
> > feature....yet.
> >
> 
> I guess the detection could be done, but actually enabling the build part
> needs to wait until the minimum supported version is low enough.

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 :|


Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Paolo Bonzini 4 months, 2 weeks ago
Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha
scritto:

> That's exactly why I suggest its a pre-requisite for merging
> this. Unless we're able to demonstrate that we can enable
> Rust on all our CI platforms, the benefits of Rust will
> not be realized in QEMU, and we'll have never ending debates
> about whether each given feature needs to be in C or Rust.
>

In that case we should develop it on a branch, so that more than one person
can contribute (unlike if we keep iterating on this RFC).

Paolo


>
> > I also believe we should default to enabling rust toolchain by
> > > default in configure, and require and explicit --without-rust
> > > to disable it, *despite* it not technically being a mandatory
> > > feature....yet.
> > >
> >
> > I guess the detection could be done, but actually enabling the build part
> > needs to wait until the minimum supported version is low enough.
>
> 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 :|
>
>
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Manos Pitsidianakis 4 months, 2 weeks ago
On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote:

>
>
> Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha
> scritto:
>
>> That's exactly why I suggest its a pre-requisite for merging
>> this. Unless we're able to demonstrate that we can enable
>> Rust on all our CI platforms, the benefits of Rust will
>> not be realized in QEMU, and we'll have never ending debates
>> about whether each given feature needs to be in C or Rust.
>>
>
> In that case we should develop it on a branch, so that more than one
> person can contribute (unlike if we keep iterating on this RFC).
>
> Paolo
>



If you do, I'd really appreciate it if you did not use any part of my
patches.

Thanks,
Manos
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Manos Pitsidianakis 4 months, 2 weeks ago
On Mon, 8 Jul 2024 at 21:39, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
>
>
> On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote:
>>
>>
>>
>> Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha scritto:
>>>
>>> That's exactly why I suggest its a pre-requisite for merging
>>> this. Unless we're able to demonstrate that we can enable
>>> Rust on all our CI platforms, the benefits of Rust will
>>> not be realized in QEMU, and we'll have never ending debates
>>> about whether each given feature needs to be in C or Rust.
>>
>>
>> In that case we should develop it on a branch, so that more than one person can contribute (unlike if we keep iterating on this RFC).
>>
>> Paolo
>
>
>
>
> If you do, I'd really appreciate it if you did not use any part of my patches.


Someone pointed out to me that my wording is really bad here and I
agree, I apologize. I did not express myself with the right words.
What I wanted to say is that hypothetically my work can be used as a
base just not the patches/sign offs. The changes in the patches are
all open source and part of the QEMU community now, of course.
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Paolo Bonzini 4 months, 2 weeks ago
Il lun 8 lug 2024, 20:39 Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
ha scritto:

>
>
> On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote:
>
>>
>>
>> Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha
>> scritto:
>>
>>> That's exactly why I suggest its a pre-requisite for merging
>>> this. Unless we're able to demonstrate that we can enable
>>> Rust on all our CI platforms, the benefits of Rust will
>>> not be realized in QEMU, and we'll have never ending debates
>>> about whether each given feature needs to be in C or Rust.
>>>
>>
>> In that case we should develop it on a branch, so that more than one
>> person can contribute (unlike if we keep iterating on this RFC).
>>
>
> If you do, I'd really appreciate it if you did not use any part of my
> patches.
>

"We" means that you would accept patches, review them and apply them until
any agreed-upon conditions for merging are satisfied, and then send either
a final series or a pull request for inclusion in QEMU.

Paolo
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Manos Pitsidianakis 4 months, 2 weeks ago
On Mon, 8 Jul 2024 at 21:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> Il lun 8 lug 2024, 20:39 Manos Pitsidianakis <manos.pitsidianakis@linaro.org> ha scritto:
>>
>>
>>
>> On Mon, 8 Jul 2024, 21:34 Paolo Bonzini, <pbonzini@redhat.com> wrote:
>>>
>>>
>>>
>>> Il lun 8 lug 2024, 19:12 Daniel P. Berrangé <berrange@redhat.com> ha scritto:
>>>>
>>>> That's exactly why I suggest its a pre-requisite for merging
>>>> this. Unless we're able to demonstrate that we can enable
>>>> Rust on all our CI platforms, the benefits of Rust will
>>>> not be realized in QEMU, and we'll have never ending debates
>>>> about whether each given feature needs to be in C or Rust.
>>>
>>>
>>> In that case we should develop it on a branch, so that more than one person can contribute (unlike if we keep iterating on this RFC).
>>
>>
>> If you do, I'd really appreciate it if you did not use any part of my patches.
>
>
> "We" means that you would accept patches, review them and apply them until any agreed-upon conditions for merging are satisfied, and then send either a final series or a pull request for inclusion in QEMU.

Ah, alright. That wasn't obvious because that e-mail was not directed
to me nor did it mention my name :)

I do not want to do that, in any case. I do not think it's the right approach.


-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Paolo Bonzini 4 months, 2 weeks ago
On Tue, Jul 9, 2024 at 9:38 AM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
> Ah, alright. That wasn't obvious because that e-mail was not directed
> to me nor did it mention my name :)

Oh, ok. Sorry about that. Generally when I say "we" I include as large
a part of the community as applicable.

> I do not want to do that, in any case. I do not think it's the right approach.

No problem with that (and in fact I agree, as I'd prefer a speedy
merge and doing the work on the QEMU master branch); however, we need
to reach an agreement on that and everybody (including Daniel) needs
to explain the reason for their position.

Daniel's proposed criteria for merging include:
- CI integration
- CI passing for all supported targets (thus lowering the MSRV to 1.63.0)
- plus any the code changes that were or will be requested during review

That seems to be a pretty high amount of work, and until it's done
everyone else is unable to contribute, not even in directions
orthogonal to the above (cross compilation support, less unsafe code,
porting more devices). So something has to give: either we decide for
an early merge, where the code is marked as experimental and disabled
by default. Personally I think it's fine, the contingency plan is
simply to "git rm -rf rust/". Or we can keep the above stringent
requirements for merging, but then I don't see it as a one-person job.

If I can say so, developing on a branch would also be a useful warm-up
for you in the maintainer role, if we expect that there will be
significant community contributions to Rust.

Paolo
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Daniel P. Berrangé 4 months, 2 weeks ago
On Tue, Jul 09, 2024 at 09:54:43AM +0200, Paolo Bonzini wrote:
> On Tue, Jul 9, 2024 at 9:38 AM Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
> > Ah, alright. That wasn't obvious because that e-mail was not directed
> > to me nor did it mention my name :)
> 
> Oh, ok. Sorry about that. Generally when I say "we" I include as large
> a part of the community as applicable.
> 
> > I do not want to do that, in any case. I do not think it's the right approach.
> 
> No problem with that (and in fact I agree, as I'd prefer a speedy
> merge and doing the work on the QEMU master branch); however, we need
> to reach an agreement on that and everybody (including Daniel) needs
> to explain the reason for their position.
> 
> Daniel's proposed criteria for merging include:
> - CI integration
> - CI passing for all supported targets (thus lowering the MSRV to 1.63.0)
> - plus any the code changes that were or will be requested during review
> 
> That seems to be a pretty high amount of work, and until it's done
> everyone else is unable to contribute, not even in directions
> orthogonal to the above (cross compilation support, less unsafe code,
> porting more devices).

My thought is that the initial merge focuses only on the build system
integration. So that's basically patches 1 + 2 in this series.

IMHO that is small enough that we should be able to demonstrate that
we detect Rust, run bindgen & compile its result, on all our supported
platforms without an unreasonable amount of effort.

>                         So something has to give: either we decide for
> an early merge, where the code is marked as experimental and disabled
> by default. Personally I think it's fine, the contingency plan is
> simply to "git rm -rf rust/". Or we can keep the above stringent
> requirements for merging, but then I don't see it as a one-person job.

Patch 3, the high level APIs is where I see most of the work and
collaboration being needed, but that doesn't need to be rushed into
the first merge. We would have a "rust" subsystem + maintainer who
would presumably have a staging tree, etc in the normal way we work
and collaborate

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 :|


Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Paolo Bonzini 4 months, 2 weeks ago
On Tue, Jul 9, 2024 at 2:18 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> My thought is that the initial merge focuses only on the build system
> integration. So that's basically patches 1 + 2 in this series.
>
> Patch 3, the high level APIs is where I see most of the work and
> collaboration being needed, but that doesn't need to be rushed into
> the first merge. We would have a "rust" subsystem + maintainer who
> would presumably have a staging tree, etc in the normal way we work
> and collaborate

It's complicated. A "perfect" build system integration would include
integration with Kconfig, but it's not simple work and it may not be
Manos's preference for what to work on (or maybe it is), and it's also
not a blocker for further work on patches 3-4.

On the other hand, patches 3 and 4 are _almost_ ready except for
requiring a very new Rust - we know how to tackle that, but again it
may take some time and it's completely separate work from better build
system integration.

In other words, improving build system integration is harder until
merge, but merge is blocked by independent work on lowering the
minimum supported Rust version. This is why I liked the idea of having
either a development tree to allow a merge into early 9.2.

On the other hand, given the exceptional scope (completely new code
that can be disabled at will) and circumstances, even a very early
merge into 9.1 (disabled by default) might be better to provide
developers with the easiest base for experimenting. The requirements
for merging, here, would basically amount to a good roadmap and some
established good habits.

An merge into early 9.2 would be a bit harder for experimenting, while
merging it now would sacrifice CI integration in the initial stages of
the work but make cooperation easier.

However, it's difficult to say what's best without knowing Manos's
rationale for preferring not to have a development tree yet.

Paolo
Re: [RFC PATCH v4 0/7] Add Rust support, implement ARM PL011
Posted by Richard Henderson 4 months, 2 weeks ago
On 7/9/24 09:51, Paolo Bonzini wrote:
> On Tue, Jul 9, 2024 at 2:18 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> My thought is that the initial merge focuses only on the build system
>> integration. So that's basically patches 1 + 2 in this series.
>>
>> Patch 3, the high level APIs is where I see most of the work and
>> collaboration being needed, but that doesn't need to be rushed into
>> the first merge. We would have a "rust" subsystem + maintainer who
>> would presumably have a staging tree, etc in the normal way we work
>> and collaborate
> 
> It's complicated. A "perfect" build system integration would include
> integration with Kconfig, but it's not simple work and it may not be
> Manos's preference for what to work on (or maybe it is), and it's also
> not a blocker for further work on patches 3-4.
> 
> On the other hand, patches 3 and 4 are _almost_ ready except for
> requiring a very new Rust - we know how to tackle that, but again it
> may take some time and it's completely separate work from better build
> system integration.
> 
> In other words, improving build system integration is harder until
> merge, but merge is blocked by independent work on lowering the
> minimum supported Rust version. This is why I liked the idea of having
> either a development tree to allow a merge into early 9.2.
> 
> On the other hand, given the exceptional scope (completely new code
> that can be disabled at will) and circumstances, even a very early
> merge into 9.1 (disabled by default) might be better to provide
> developers with the easiest base for experimenting. The requirements
> for merging, here, would basically amount to a good roadmap and some
> established good habits.
> 
> An merge into early 9.2 would be a bit harder for experimenting, while
> merging it now would sacrifice CI integration in the initial stages of
> the work but make cooperation easier.

I would be in favor of a 9.1 merge (disabled, not auto).
Noting that soft-freeze is two weeks from today, so no dilly dallying.  :-)

The only reasonable alternative that I see is the kind of development branch that qemu is 
not used to having: a long-term shared branch on qemu-projects.  With which I would be ok, 
up to and including a branch merge at the end, as I'm used to working that way with other 
projects.

I think the only reason to delay the current development work until 9.2 is if we were 
still questioning whether using Rust *at all* is a good idea.  I think we're beyond that 
point.


r~