On Sat, Oct 26, 2024 at 12:06 PM Manos Pitsidianakis
<manos.pitsidiana...@linaro.org> wrote:
> Please reply with review comments underneath individual patches, this
> is hard to follow and I might miss some points.
Will do.
> > > Revert "rust: add PL011 device model"
> > > rust: add PL011 device model
> >
> > ... should definitely be moved on top to clean up the authorship in "git
> > blame" and other similar tools. Sorry about that.
>
> I will send them on a separate series and merge them from my tree when
> reviewed.
Those are trivial to review, just "git diff HEAD^^". No need to
separate them into a new submission.
> > > rust: add support for migration in device models
> > > rust/pl011: move CLK_NAME static to function scope
> > > rust/pl011: add TYPE_PL011_LUMINARY device
> > > rust/pl011: remove commented out C code
> > > rust/pl011: Use correct masks for IBRD and FBRD
> >
> > (minus the usage of #[derive()] should be included in that series, so
> > that qtests pass. It's not a huge amount of work and I can extract it,
> > of course with proper attribution/authorship.
>
> These are independent from CI; i.e. you can merge your CI patches after those.
That's what I did: I put them at the beginning of the series of
pending patches, so they _are_ indeed merged after the CI patches. The
only issue here is that patch 4 ("rust: add support for migration in
device models") was dependent on the Device proc macro, but that was
easy enough to extract.
> >
> > The rest are future work:
> >
> > > rust/qemu-api-macros: introduce Device proc macro
> >
> > As I said above, we first need to agree on the design.
>
> Post your review under the patches please,
Yep.
> > > rust/pl011: move pub callback decl to local scope
> >
> > This depends a lot on how we go implementing bindings to chardev. For
> > example if the callbacks turn out to be a trait, it would have to be
> > undone. Possibly the C callback wrappers would move to
> > rust/qemu-api/chardev. For now I'd leave it aside.
>
> This patch moves the callbacks scope from public to inside the
> function, no functional change related. It doesn't change or have
> anything to do with chardev interfaces
I understand. My point is that the callbacks you move
(pl011_can_receive, pl011_receive, pl011_event) in the end will belong
into the C<->Rust chardev bindings. A patch to remove "pub" would have
basically the same benefit without the churn in indentation.
> > > rust/qemu-api: add log module
> > > rust/pl011: log guest/unimp errors
> >
> > This also needs design discussion. Do we want the API to be the same as
> > C, i.e. keep the qemu_* prefix? Do we want wrapper macros that include
> > the format!() call?
>
> I'm guessing you did not see the patch messages, which cover these
> points. Post your review under the patches please,
I will but for now I'll say that the commit messages do not address
the questions I'm asking above.
More in general, we need to establish conventions on what the Rust
APIs look like (about qemu_* prefixes, for example). Personally I
prefer to have APIs that, while easy enough to connect to the C ones,
are idiomatic. But you can post these two patches separately and we
can discuss it there.
> > You also have "pub enum LogMask" to work around the fact that log masks
> > are preprocessor macros. Is that okay, or do we want to modify C code
> > to make the bindings nicer? I for example would prefer the latter and
> > then declaring LogMask as a bitfield in bindgen.
>
> A bindgen type is definitely preferred but for a Rust idiomatic
> interface a wrapper type is a UX improvement (`CPU_LOG_PCALL`?
> `LOG_GUEST_ERROR`? We can use friendlier symbols in the LogMask
> variants for that)
What I'm saying is that these are discussions where we need to reach
an agreement on, and document the choices.
Paolo
