On Wed, Jul 24, 2024 at 11:58 AM Manos Pitsidianakis <manos.pitsidiana...@linaro.org> wrote: > > Hello Paolo, thank you for the thorough response, > > On Tue, 23 Jul 2024 18:07, Paolo Bonzini <pbonz...@redhat.com> wrote: > >On 7/22/24 13:43, Manos Pitsidianakis wrote: > >> Changes from v4->v5: > >> - Added CI patch from Alex Benee > >> - Removed all cargo use, use meson rust support > >> - Added Kconfig logic > > > >The following requests from the v4 review have also been evaluated (good!): > > > >✅ module structure should resemble the C part of the tree > > To expand on this, I tried really hard to make the rust code live along > the c files
Yes, I don't think it's a requirement that they live along the C files. Using rust/hw/... as you did is fine. > I agree. I personally prefer using meson wraps and fetch the > dependencies via network to be honest. While also providing Cargo.toml > and Cargo.lock manifests for developers. Ok, it's good that you agree because that's what I was worried about. :) > > > >In my opinion we should start with cargo workspaces as the > >known-imperfect (but good enough) solution, so that it could be evolved > >later. It is important that any change that deviates from common Rust > >conventions is documented, and v4 provided a nice basis to build upon, > >with documentation coming as things settle. This is why I explicitly > >didn't include Kconfig in the TODO list in my review of v4. > > After working with the latest meson releases, it seems we will soon have > a good enough way of handling all this with meson. It makes me sceptical > of adding cargo wrappers and using a build system out of meson when we > might be able to drop all that soonish. We might as well bite the bullet > now to avoid working on something we know we will remove. Ehh, as you say below it's complicated. Sometimes worse is better. Personally I wouldn't have minded keeping the v4 approach as a "known evil"; but if you can make the Cargo subprojects work, that would be fine for me. What I don't like is the vendoring and handwritten (I think?) meson.build, I think that's worse than the v4. > >Also, of the code changes (as opposed to the build system changes) that > >I had asked for in the review of v4, almost none of them have been > >applied. I'm copying them here for future reference: > > Thanks, this helps a lot. > > >❌ TODO comments when the code is doing potential undefined behavior > > Do you mean like Rust's safety comments? No I meant comments where we have known instances of undefined behavior. The two I had in my emails are (in pl011_init): // TODO: this assumes that "all zeroes" is a valid state for all fields of // PL011State. This is not necessarily true of any #[repr(Rust)] structs, // including bilge-generated types. It should instead use MaybeUninit. (before the call to qemu_chr_fe_accept_input): // TODO: this causes a callback that creates another "&mut self". // This is forbidden by Rust aliasing rules and has to be fixed // using interior mutability. > https://std-dev-guide.rust-lang.org/policy/safety-comments.html > > These can be required by lints which is really helpful. At this point > the UART library has safety comments (which needs to be reviewed for > validity). I plan on adding some at the macros in qemu-api as well. > > > > >❌ a trait to store the CStr corresponding to the structs > > I don't know yet if that is helpful in our usecase, because the strings > must be visible from C, thus be (rust, not c) statics, unmangled and > marked as #[used] for the linker. It makes sense from the Rust POV but > must also be FFI-accessible. Why do they have to be #[used]? You have + #[used] + static TYPE_NAME: &::core::ffi::CStr = $tname; + $tname.as_ptr() but since the cstr crate (and c"" literal) promise to return a &'static CStr, I thought it could be just $tname.as_ptr() About traits, I meant something like pub unsafe trait ObjectType: Sized { const TYPE_NAME: &'static CStr; } So that you can put the trait declaration in the pl011 crate and the type_info! macro can do <$t as ObjectType>::TYPE_NAME.as_ptr() (also for the parent). > >❌ a trait to generate all-zero structs without having to type "unsafe { > >MaybeUninit::zeroed().assume_init() }" > > Traits cannot have const fns at the moment (traits cannot have > type-level effects like const or async but it's WIP to get them into > rustc), so this cannot be used for statics and consts. Ah, I see. Anyhow, I've been looking at the Linux kernel's pinned-init crate (https://rust-for-linux.com/pinned-init) and it provides a Zeroable macro including #[derive] support. So that has gone lower in my priority. > >❌ I'd like to use ctor instead of non-portable linker magic, > > The linker sections are pretty much standard and in fact ctor uses the > same linker attributes. Would writing our own constructor macro be a > solution for you? My reasoning is that 1) we support our own specific > platforms and it's better for portability to reflect that in our source > tree and 2) it avoids the external dependency, linker sections do not > change so any ctor update would be in the API or adding more platform > support, not fixes in what we target. I'd still like to give someone else the burden. :) Writing our own constructor macro would be more work for little gain. > >and the cstr crate instead of CStr statics or c"" > > Oh yes, the c"" literals must be replaced. The cstr! macro is the same, > semantically, can you explain what you mean by "CStr statics"? Ah, I meant that it applies to both direct use: pub static CLK_NAME: &CStr = c"clk"; and arguments to macros (for example type_info). > >My suggestion is to do one of the following, or both: > > > >- start from this version; try using Cargo subproject support in 1.5.0 > >and see if it works, so that vendoring can be dropped. We can require > >Meson 1.5.0 to work on Rust support. In this case it's okay not to do > >any further code changes (the four that were marked ❌ above). > > This is my preference as stated above, if everyone also agrees. I think it's worth trying it anyway. > >- go back to the build system integration of v4, and do *only* the > >changes that were requested during review (in this case, all of them > >except link_whole, with you checked it does not work). > > > >If you try using Cargo subproject support, please provide the running > >time for configure and make, for both "v4" and "v5+subproject". When I > >tried it, the processing of the subprojects was very slow. > > Hmmm thanks for mentioning that, I did not notice any slow times > locally. Will check. Ok, thanks! Paolo
