Re: [RFC PATCH v3 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine

[Peter Maydell]

On Tue, 25 Jun 2024 at 20:15, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jun 26, 2024 at 12:18:55AM +0800, Zhao Liu wrote:
> > Hi Manos,
> >
> > On Wed, Jun 19, 2024 at 11:14:02PM +0300, Manos Pitsidianakis wrote:
> > > Date: Wed, 19 Jun 2024 23:14:02 +0300
> > > From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > > Subject: [RFC PATCH v3 5/5] DO NOT MERGE: replace TYPE_PL011 with
> > >  x-pl011-rust in arm virt machine
> > > X-Mailer: git-send-email 2.44.0
> > >
> > > Convenience patch for testing the rust device.
> > >
> > > Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > > ---
> > >  hw/arm/virt.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 3c93c0c0a6..f33b58ae0d 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -912,7 +912,11 @@ static void create_uart(const VirtMachineState *vms, 
> > > int uart,
> > >      int irq = vms->irqmap[uart];
> > >      const char compat[] = "arm,pl011\0arm,primecell";
> > >      const char clocknames[] = "uartclk\0apb_pclk";
> > > +#ifdef CONFIG_WITH_RUST
> > > +    DeviceState *dev = qdev_new("x-pl011-rust");
> > > +#else
> > >      DeviceState *dev = qdev_new(TYPE_PL011);
> > > +#endif
> > >      SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > >      MachineState *ms = MACHINE(vms);
> > >
> >
> > I realized that if we want to merge the rust pl011 device, then this
> > patch or similar enablement support is necessary, otherwise, the rust
> > code is only used for compilation and cannot actually be run...
> >
> > This is also an open for the devices that are rewrite in Rust.
> >
> > I think there should be an option for the user to choose whether to
> > enable pl011 in C or pl011 in Rust. What do you think?
> >
> > Perhaps the easiest way to enable rust pl011 is to add an option for
> > virt machine... But that's certainly not a long-term approach. I think
> > the ideal way would be to allow rust pl011 to be specified in the
> > command line via -device, but this approach would mean allowing the
> > user to create pl011 and would require changes to the current buildin
> > pl011's creation logic.
>
> While having both impls present is reasonable for the RFC, IMHO, if
> we're to merge this, then the Rust impl of pl011 should fully replace
> the C impl. Maintaining 2 different impls of the same device in
> parallel is highly undesirable. This would of course impl that the
> Rust impls needs to be a feature match for the existing impl. If we
> do a full replacement, then the question of how the user chooses
> between the 2 impls is then entirely avoided.

Yes, I agree this is where we want to go for the long term. For
the short term of reaching consensus on all the earlier parts of
the patch series (build system integration, how to do a rust
device model, etc), I think this patch is fine as a "don't commit this"
way to test the series. In the medium term I guess "feature-parity
both in parallel for a (hopefully) short time and then drop the C
version" ? I definitely feel we don't want a user-facing runtime
option for selecting which one, at any rate.

thanks
-- PMM