Re: [PATCH 1/2] rust: add BQL-enforcing Cell variant

[Paolo Bonzini <pbonzini@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]

Il mer 27 nov 2024, 07:17 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> > and a single "regs: BqlRefCell<PL011Registers>" in PL011State.
>
> Here I have a possibly common question about the choice of BqlCell and
> future BqlRefCell. Pls refer my comment below...
>
> I understand we need BqlRefCell instead of BqlCell for registers since
> there may be more than one reference, e.g., callbacks from CharBackend
> and MemoryRegion may hold multiple references at the same time, right?
>
> If right, like HPET, with only MemoryRegion callbacks, reads and writes
> I guess which should not be able to happen at the same time, so BqlCell
> is also feasible, as is Irq?
>

Choose between BqlCell/BqlRefCell like you would choose between
Cell/RefCell. They result in different code so pick what looks better.

That said for HPET you have the array of timers structs, and these structs
are not Copy, and therefore BqlRefCell is almost the only choice for that
array. With BqlRefCell used for the timers you might as well put the other
registers in the same BqlRefCell.

(This piece of the thread model is a bit more complex, and I'm fully
> aware that the right TYPE relies a lot on understanding the underlying
> mechanism, which I'm not yet familiar enough with :-) ).
>

It's not too complex—the point is to make it as similar as possible to
normal single threaded Rust.

However, all in all, using BqlRefCell for register fields looks to be
> always better than BqlCell.
>

Yep :)

Thank you! I'll change the current code to support this. Instead of
> implementing a register space like PL011, I continue to handle registers
> with u64 variables and bit macro definitions like the C version


That's fine. Experimenting in different directions makes it easier for
future developers to evaluate the tradeoffs.

Also related to the above question, I'm a bit hesitant to use BqlCell
> directly
> or RefCell for such u64 fields...
>

Do whatever looks best to you!

> Plus, speaking in general, "it does something in a different way than the
> > pl011 device model" is a good reason to merge the HPET model earlier
> too. :)
>
> There must be a lot more opens :-(, such as the memattrs/timer binding,
> which I hope to discuss with you at the later RFC!
>

Yes, but stuff like "correctly uses interior mutability" is a very good
reason to include the HPET early.

I will send the next version and include BqlRefCell once I incorporate
Junjie's feedback, in the meanwhile I will send you the BqlRefCell patch
off list.

Paolo

[-- Attachment #2: Type: text/html, Size: 4422 bytes --]