diff mbox series

[v3,13/14] dp8393x: Don't reset Silicon Revision register

Message ID 34caf9dea82cd30dc51b4f8768adb7a90b236bf2.1579474761.git.fthain@telegraphics.com.au
State New
Headers show
Series Fixes for DP8393X SONIC device emulation | expand

Commit Message

Finn Thain Jan. 19, 2020, 10:59 p.m. UTC 
The jazzsonic driver in Linux uses the Silicon Revision register value
to probe the chip. The driver fails unless the SR register contains 4.
Unfortunately, reading this register in QEMU usually returns 0 because
the s->regs[] array gets wiped after a software reset.

Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 hw/net/dp8393x.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Philippe Mathieu-Daudé Jan. 28, 2020, 11:08 a.m. UTC | #1 
On 1/19/20 11:59 PM, Finn Thain wrote:
> The jazzsonic driver in Linux uses the Silicon Revision register value
> to probe the chip. The driver fails unless the SR register contains 4.
> Unfortunately, reading this register in QEMU usually returns 0 because
> the s->regs[] array gets wiped after a software reset.
> 
> Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> ---
>   hw/net/dp8393x.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 1b73a8703b..71af0fad51 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
>                   val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg)];
>               }
>               break;
> +        /* Read-only */
> +        case SONIC_SR:
> +            val = 4; /* only revision recognized by Linux/mips */
> +            break;
>           /* All other registers have no special contrainst */
>           default:
>               val = s->regs[reg];
> @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
>       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>   
>       s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>   
>       memory_region_init_ram(&s->prom, OBJECT(dev),
>                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> 

Please fix in dp8393x_reset() instead:

-- >8 --
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cdc2631c0c..65eb9c23a7 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
      timer_del(s->watchdog);

      memset(s->regs, 0, sizeof(s->regs));
+    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
      s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
      s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
      s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | 
SONIC_RCR_BRD | SONIC_RCR_RNT);
@@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error 
**errp)
      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);

      s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
-    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */

      memory_region_init_ram(&s->prom, OBJECT(dev),
                             "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
---
Finn Thain Jan. 28, 2020, 10:28 p.m. UTC | #2 
On Tue, 28 Jan 2020, Philippe Mathieu-Daud? wrote:

> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The jazzsonic driver in Linux uses the Silicon Revision register value
> > to probe the chip. The driver fails unless the SR register contains 4.
> > Unfortunately, reading this register in QEMU usually returns 0 because
> > the s->regs[] array gets wiped after a software reset.
> > 
> > Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
> > Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> > ---
> >   hw/net/dp8393x.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 1b73a8703b..71af0fad51 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> > unsigned int size)
> >                   val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 -
> > reg)];
> >               }
> >               break;
> > +        /* Read-only */
> > +        case SONIC_SR:
> > +            val = 4; /* only revision recognized by Linux/mips */
> > +            break;
> >           /* All other registers have no special contrainst */
> >           default:
> >               val = s->regs[reg];
> > @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> > **errp)
> >       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> >         s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> > -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> >         memory_region_init_ram(&s->prom, OBJECT(dev),
> >                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> > 
> 
> Please fix in dp8393x_reset() instead:
> 
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cdc2631c0c..65eb9c23a7 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
>      timer_del(s->watchdog);
> 
>      memset(s->regs, 0, sizeof(s->regs));
> +    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>      s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
>      s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
>      s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
> SONIC_RCR_RNT);
> @@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> **errp)
>      qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> 
>      s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> 
>      memory_region_init_ram(&s->prom, OBJECT(dev),
>                             "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> ---
> 

This would allow the host to change the value of the Silicon Revision 
register. However, the datasheet says,

    4.3.13 Silicon Revision Register
    This is a 16-bit read only register. It contains information on the 
    current revision of the SONIC. The value of the DP83932CVF revision 
    register is 6h.

I haven't actually tried storing a different value in this register on 
National Semiconductor hardware, but I'm willing to do that test if you 
wish.
Philippe Mathieu-Daudé Jan. 29, 2020, 6:53 a.m. UTC | #3 
Hi Finn,

On 1/28/20 11:28 PM, Finn Thain wrote:
> On Tue, 28 Jan 2020, Philippe Mathieu-Daud? wrote:
>> On 1/19/20 11:59 PM, Finn Thain wrote:
>>> The jazzsonic driver in Linux uses the Silicon Revision register value
>>> to probe the chip. The driver fails unless the SR register contains 4.
>>> Unfortunately, reading this register in QEMU usually returns 0 because
>>> the s->regs[] array gets wiped after a software reset.
>>>
>>> Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
>>> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
>>> ---
>>>    hw/net/dp8393x.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 1b73a8703b..71af0fad51 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
>>> unsigned int size)
>>>                    val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 -
>>> reg)];
>>>                }
>>>                break;
>>> +        /* Read-only */
>>> +        case SONIC_SR:
>>> +            val = 4; /* only revision recognized by Linux/mips */
>>> +            break;
>>>            /* All other registers have no special contrainst */
>>>            default:
>>>                val = s->regs[reg];
>>> @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error
>>> **errp)
>>>        qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>>>          s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
>>> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>>          memory_region_init_ram(&s->prom, OBJECT(dev),
>>>                               "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
>>>
>>
>> Please fix in dp8393x_reset() instead:
>>
>> -- >8 --
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index cdc2631c0c..65eb9c23a7 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
>>       timer_del(s->watchdog);
>>
>>       memset(s->regs, 0, sizeof(s->regs));
>> +    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>       s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
>>       s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
>>       s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
>> SONIC_RCR_RNT);
>> @@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error
>> **errp)
>>       qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>>
>>       s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
>> -    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>>
>>       memory_region_init_ram(&s->prom, OBJECT(dev),
>>                              "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
>> ---
>>
> 
> This would allow the host to change the value of the Silicon Revision
> register.
How the guest can modify it? We have:

589 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
590                           unsigned int size)
591 {
592     dp8393xState *s = opaque;
593     int reg = addr >> s->it_shift;
594
...
597     switch (reg) {
...
602         /* Prevent write to read-only registers */
...
606         case SONIC_SR:
...
608             DPRINTF("writing to reg %d invalid\n", reg);
609             break;

> However, the datasheet says,
> 
>      4.3.13 Silicon Revision Register
>      This is a 16-bit read only register. It contains information on the
>      current revision of the SONIC. The value of the DP83932CVF revision
>      register is 6h.
> 
> I haven't actually tried storing a different value in this register on
> National Semiconductor hardware, but I'm willing to do that test if you
> wish.
>
Finn Thain Jan. 29, 2020, 7:52 a.m. UTC | #4 
On Wed, 29 Jan 2020, Philippe Mathieu-Daudé wrote:

> > 
> > This would allow the host to change the value of the Silicon Revision 
> > register.
> How the guest can modify it? We have:
> 
> 589 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> 590                           unsigned int size)
> 591 {
> 592     dp8393xState *s = opaque;
> 593     int reg = addr >> s->it_shift;
> 594
> ...
> 597     switch (reg) {
> ...
> 602         /* Prevent write to read-only registers */
> ...
> 606         case SONIC_SR:
> ...
> 608             DPRINTF("writing to reg %d invalid\n", reg);
> 609             break;
> 

My mistake. I had completely overlooked that logic.

I'll revise this patch in accordance with your suggestion.
diff mbox series

Patch

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 1b73a8703b..71af0fad51 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -591,6 +591,10 @@  static uint64_t dp8393x_read(void *opaque, hwaddr addr, unsigned int size)
                 val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg)];
             }
             break;
+        /* Read-only */
+        case SONIC_SR:
+            val = 4; /* only revision recognized by Linux/mips */
+            break;
         /* All other registers have no special contrainst */
         default:
             val = s->regs[reg];
@@ -971,7 +975,6 @@  static void dp8393x_realize(DeviceState *dev, Error **errp)
     qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
 
     s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
-    s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
 
     memory_region_init_ram(&s->prom, OBJECT(dev),
                            "dp8393x-prom", SONIC_PROM_SIZE, &local_err);