esp: Do not overwrite ESP_TCHI after reset

After a reset ESP_TCHI should contain the unique ID of the chip. This value will be overwritten with the current tranfer count if the transfer count has previously been set.
So we should always return the chip id if ESP_TCHI has never been written to.

Signed-off-by Hannes Reinecke <[email protected]>

---
 hw/scsi/esp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

hw/scsi/esp.c +5 -1

index 5ab44d8..2caac1c 100644
@@ -364,7 +364,6 @@ void esp_hard_reset(ESPState *s)
 {
     memset(s->rregs, 0, ESP_REGS);
     memset(s->wregs, 0, ESP_REGS);
-    s->rregs[ESP_TCHI] = s->chip_id;
     s->ti_size = 0;
     s->ti_rptr = 0;
     s->ti_wptr = 0;

@@ -422,6 +421,11 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
         esp_lower_irq(s);
 
         return old_val;
+    case ESP_TCHI:
+        /* Return the unique id if the value has never been written */
+        if (!s->wregs[ESP_TCHI]) {
+            return s->chip_id;
+        }
     default:
         break;
     }

Quoted 6 lines · Hannes Reinecke

After a reset ESP_TCHI should contain the unique ID of the chip. This value will be overwritten with the current tranfer count if the transfer count has previously been set.
So we should always return the chip id if ESP_TCHI has never been written to.

What if ESP_TCHI was written 0? Why should it return the chip id?

Can you explain exactly what sequence of register reads/writes leads to the bug?

Quoted 8 lines · Paolo Bonzini

Quoted 6 lines · Hannes Reinecke

After a reset ESP_TCHI should contain the unique ID of the chip. This value will be overwritten with the current tranfer count if the transfer count has previously been set.
So we should always return the chip id if ESP_TCHI has never been written to.

Quoted 2 lines

What if ESP_TCHI was written 0? Why should it return the chip id?

It's a complex thing. The documentation says 'ESP_TCHI returns the chip id until been written to'.
And ESP_TCHI is strictly speaking only valid if the 'Features enabled' bit is set in ESP_CFG2. (Not that the driver checks this). To handle it correctly we would need to add a flag whenever ESP_TCHI is written to, but I thought it'd be slightly too much.

Quoted 9 lines · Hannes Reinecke

Quoted 8 lines · Paolo Bonzini

Quoted 6 lines · Hannes Reinecke

After a reset ESP_TCHI should contain the unique ID of the chip. This value will be overwritten with the current tranfer count if the transfer count has previously been set.
So we should always return the chip id if ESP_TCHI has never been written to.

Quoted 8 lines

Quoted 2 lines

What if ESP_TCHI was written 0? Why should it return the chip id?

It's a complex thing. The documentation says 'ESP_TCHI returns the chip id until been written to'.
And ESP_TCHI is strictly speaking only valid if the 'Features enabled' bit is set in ESP_CFG2. (Not that the driver checks this). To handle it correctly we would need to add a flag whenever ESP_TCHI is written to, but I thought it'd be slightly too much.

I'm worried that the chip_id will reappear if you write a low (<65536) value to ESP_TCLOW/MED/HI.

Adding the flag is just a couple lines more than this patch, let's do it right.