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 <pbonz...@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 <pbonz...@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....@intel.com>
