Check loopback_enabled(), not fifo_enabled(), like the C code.

Also, set_break_error() must not happen until the break is read from
the FIFO.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 rust/hw/char/pl011/src/device.rs | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
index 41220c99a83..c6a8dbe1af4 100644
--- a/rust/hw/char/pl011/src/device.rs
+++ b/rust/hw/char/pl011/src/device.rs
@@ -465,9 +465,8 @@ pub fn can_receive(&self) -> bool {
     }
 
     pub fn event(&mut self, event: QEMUChrEvent) {
-        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.fifo_enabled() {
+        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
             self.put_fifo(DATA_BREAK);
-            self.receive_status_error_clear.set_break_error(true);
         }
     }
 
-- 
2.47.1

On Thu, Dec 12, 2024 at 06:21:59PM +0100, Paolo Bonzini wrote:
> Date: Thu, 12 Dec 2024 18:21:59 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 2/7] rust: pl011: match break logic of C version
> X-Mailer: git-send-email 2.47.1
> 
> Check loopback_enabled(), not fifo_enabled(), like the C code.
> 
> Also, set_break_error() must not happen until the break is read from
> the FIFO.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  rust/hw/char/pl011/src/device.rs | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Now the logic here matches C version, so

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

But...

> diff --git a/rust/hw/char/pl011/src/device.rs b/rust/hw/char/pl011/src/device.rs
> index 41220c99a83..c6a8dbe1af4 100644
> --- a/rust/hw/char/pl011/src/device.rs
> +++ b/rust/hw/char/pl011/src/device.rs
> @@ -465,9 +465,8 @@ pub fn can_receive(&self) -> bool {
>      }
>  
>      pub fn event(&mut self, event: QEMUChrEvent) {
> -        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.fifo_enabled() {
> +        if event == bindings::QEMUChrEvent::CHR_EVENT_BREAK && !self.loopback_enabled() {
>              self.put_fifo(DATA_BREAK);
> -            self.receive_status_error_clear.set_break_error(true);
>          }
>      }
>   

...but when I double-check where to set up the rsr, I realized that the
rust version and the C version seem to be inconsistent?

C:

static uint32_t pl011_read_rxdata(PL011State *s)
{
    ...
    c = s->read_fifo[s->read_pos];
    ...
    s->rsr = c >> 8;
    ...
}

Rust:

    pub fn read(&mut self, offset: hwaddr, _size: c_uint) -> std::ops::ControlFlow<u64, u64> {
        ...
	std::ops::ControlFlow::Break(match RegisterOffset::try_from(offset) {
            ...
	    Ok(DR) => {
                ...
		let c = self.read_fifo[self.read_pos];
                ...
		// Update error bits.
                self.receive_status_error_clear = c.to_be_bytes()[3].into();
                ...
	    }
        ...
    }

`to_be_bytes()` retures a byte array in big-endian byte order [*], so rust
accesses the first 8 bits and C accesses the last 24 bits, am I understanding
this correctly and which one is correct?

IIUC, I guess Rust should uses to_be_bytes()[2].

[*]: https://doc.rust-lang.org/std/primitive.u32.html#method.to_be_bytes

Regards,
Zhao

Il gio 19 dic 2024, 05:20 Zhao Liu <zhao1.liu@intel.com> ha scritto:

> But when I double-check where to set up the rsr, I realized that the
> rust version and the C version seem to be inconsistent?
>
> IIUC, I guess Rust should uses to_be_bytes()[2].


Yes, that's patch 4 in this series. :)

Paolo

[*]: https://doc.rust-lang.org/std/primitive.u32.html#method.to_be_bytes
>
> Regards,
> Zhao
>
>
>

On 12/12/24 18:21, Paolo Bonzini wrote:
> Check loopback_enabled(), not fifo_enabled(), like the C code.
> 
> Also, set_break_error() must not happen until the break is read from
> the FIFO.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   rust/hw/char/pl011/src/device.rs | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>