| Message ID | 229703c7f4394691f254b02c012ee0d7dcf57afb.1718827153.git.manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement ARM PL011 in Rust | expand |
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
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.
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
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
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);
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(+)