[Qemu-devel] [PATCHv2] esp: Do not overwrite ESP_TCHI after reset

[Qemu-devel] [PATCHv2] esp: Do not overwrite ESP_TCHI after reset

[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.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/scsi/esp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5ab44d8..2caac1c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -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;
     }
-- 
1.7.12.4

Re: [Qemu-devel] [PATCHv2] esp: Do not overwrite ESP_TCHI after reset

[Paolo Bonzini]

On 10/11/2014 16:52, Hannes Reinecke wrote:
> 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?

Paolo

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  hw/scsi/esp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 5ab44d8..2caac1c 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -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;
>      }
> 

Re: [Qemu-devel] [PATCHv2] esp: Do not overwrite ESP_TCHI after reset

[Hannes Reinecke]

On 11/10/2014 07:32 PM, Paolo Bonzini wrote:
> On 10/11/2014 16:52, Hannes Reinecke wrote:
>> 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?
> 
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.
Plus we're reloading the ESP_TCHI register anyway whenever a transfer
is started.

> Can you explain exactly what sequence of register reads/writes leads to
> the bug?
> 
CMD RST
DMA CMD NOP
-> ESP_TCHI should contain chip_id, but doesn't.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [Qemu-devel] [PATCHv2] esp: Do not overwrite ESP_TCHI after reset

[Paolo Bonzini]

On 10/11/2014 21:11, Hannes Reinecke wrote:
> On 11/10/2014 07:32 PM, Paolo Bonzini wrote:
>> On 10/11/2014 16:52, Hannes Reinecke wrote:
>>> 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?
>>
> 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.

Paolo

Re: [Qemu-devel] [PATCHv2] esp: Do not overwrite ESP_TCHI after reset

[Hannes Reinecke]

On 11/11/2014 01:37 PM, Paolo Bonzini wrote:
> 
> 
> On 10/11/2014 21:11, Hannes Reinecke wrote:
>> On 11/10/2014 07:32 PM, Paolo Bonzini wrote:
>>> On 10/11/2014 16:52, Hannes Reinecke wrote:
>>>> 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?
>>>
>> 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.
> 
Right.

Will be sending an updated patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)