| Message ID | 1415634775-4288-1-git-send-email-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
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; > } >
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
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
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
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; }
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(-)