Re: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset

[Zhao Liu]

On Fri, Jan 17, 2025 at 10:26:50AM +0100, Paolo Bonzini wrote:
> Date: Fri, 17 Jan 2025 10:26:50 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 03/10] rust: pl011: extract conversion to RegisterOffset
> X-Mailer: git-send-email 2.47.1
> 
> As an added bonus, this also makes the new function return u32 instead
> of u64, thus factoring some casts into a single place.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 114 +++++++++++++++++--------------
>  1 file changed, 63 insertions(+), 51 deletions(-)

[snip]

> -    pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> 
> std::ops::ControlFlow<u64, u64> {
> +    fn regs_read(&mut self, offset: RegisterOffset) -> ControlFlow<u32, u32> 
> {
>          use RegisterOffset::*;

Can we move this "use" to the start of the file?

IMO, placing it in the local scope appears unnecessary and somewhat
fragmented.

> -        let value = match RegisterOffset::try_from(offset) {
> -            Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
> -                let device_id = self.get_class().device_id;
> -                u32::from(device_id[(offset - 0xfe0) >> 2])
> -            }
> -            Err(_) => {
> -                // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 
> 0x%x\n", (int)offset);
> -                0
> -            }
> -            Ok(DR) => {
> +        std::ops::ControlFlow::Break(match offset {

std::ops can be omitted now.

> +            DR => {
>                  self.flags.set_receive_fifo_full(false);
>                  let c = self.read_fifo[self.read_pos];
>                  if self.read_count > 0 {

[snip]

> -    pub fn write(&mut self, offset: hwaddr, value: u64) {
> +    fn regs_write(&mut self, offset: RegisterOffset, value: u32) {
>          // eprintln!("write offset {offset} value {value}");
>          use RegisterOffset::*;
> -        let value: u32 = value as u32;
> -        match RegisterOffset::try_from(offset) {
> -            Err(_bad_offset) => {
> -                eprintln!("write bad offset {offset} value {value}");
> -            }
> -            Ok(DR) => {
> +        match offset {
> +            DR => {
>                  // ??? Check if transmitter is enabled.
>                  let ch: u8 = value as u8;
>                  // XXX this blocks entire thread. Rewrite to use
> @@ -290,22 +277,22 @@ pub fn write(&mut self, offset: hwaddr, value: u64) {
>                  self.int_level |= registers::INT_TX;
>                  self.update();
>              }
> -            Ok(RSR) => {
> -                self.receive_status_error_clear.reset();
> +            RSR => {
> +                self.receive_status_error_clear = 0.into();

Emm, why do we use 0.into() instead of reset() here? It looks they're
same.

[snip]

> @@ -562,6 +549,31 @@ pub fn post_load(&mut self, _version_id: u32) -> 
> Result<(), ()> {
>  
>          Ok(())
>      }
> +
> +    pub fn read(&mut self, offset: hwaddr, _size: u32) -> ControlFlow<u64, 
> u64> {

Maybe pub(crate)? But both are fine for me :-)

> +        match RegisterOffset::try_from(offset) {
> +            Err(v) if (0x3f8..0x400).contains(&(v >> 2)) => {
> +                let device_id = self.get_class().device_id;
> +                ControlFlow::Break(u64::from(device_id[(offset - 0xfe0) >> 
> 2]))
> +            }
> +            Err(_) => {
> +                // qemu_log_mask(LOG_GUEST_ERROR, "pl011_read: Bad offset 
> 0x%x\n", (int)offset);
> +                ControlFlow::Break(0)
> +            }
> +            Ok(field) => match self.regs_read(field) {
> +                ControlFlow::Break(value) => 
> ControlFlow::Break(value.into()),
> +                ControlFlow::Continue(value) => 
> ControlFlow::Continue(value.into()),
> +            }
> +        }
> +    }
> +

Look good to me,

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>