[v3] Implement ARM PL011 in Rust

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

Posted by Manos Pitsidianakis 4 months, 1 week ago

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);
 
-- 
γαῖα πυρί μιχθήτω

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

Posted by Zhao Liu 4 months ago

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.

-Zhao

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

Posted by Manos Pitsidianakis 4 months ago

On Tue, 25 Jun 2024 19:18, Zhao Liu <zhao1.liu@intel.com> 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.
>
>-Zhao
>

We should definitely refer to ARM maintainers. The peculiarity of it 
being a chardev seems to make it not straightforward... I think it might 
have to become a machine property like discussed here:

https://lore.kernel.org/qemu-devel/CAFEAcA94twaBSx--NVXQcRBQ7v9TuK9iTq9kTWP4FYpRzgPbBA@mail.gmail.com/

And that'd be a bigger change not related just to Rust.

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

Posted by Daniel P. Berrangé 4 months ago

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.

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 v3 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine

Posted by Peter Maydell 4 months ago

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