Re: [Qemu-devel] eepro100c additions/changes required for VxWorks 5.4.2 to work

[Reimar Döffinger]

I am only looking at this because I hacked this code, not because
I have anything to say on whether it gets in...

On Sat, Aug 22, 2009 at 06:42:55PM +0200, ZAPPACOSTA, Rolando (Rolando) wrote:
> yes, you are correct, some of them could be considered cosmetic (I added them 
> initially for a better tracking of the emulation each time I launched it).    
>                                                                               
>                                                     
> 1)
> @@ -257,6 +266,8 @@
>      /* Temporary data. */
>      eepro100_tx_t tx;    
>      uint32_t cb_address; 
> +    /* used to store the partial values of the pointer before calling 
> eepro100_write_port */
> +    uint16_t port_lo_word;                                                   
>                
>                                                                               
>                
>      /* Statistical counters. */                                              
>                
>      eepro100_stats_t statistics;                                             
>                
> ===========> REASON: because VxWorks access it as two dwords and the 
> emulation fails if this is not implemented.

I don't really understand that, the values always already gets stored to and
restored from ->mem...

> @@ -782,27 +793,26 @@
>      TRACE(RXTX, logout
>          ("transmit, TBD array address 0x%08x, TCB byte count 0x%04x, TBD 
> count %u\n",
>           tbd_array, tcb_bytes, s->tx.tbd_count));                            
>         
> -    assert(!(s->tx.command & COMMAND_NC));                                   
>         
> -    assert(tcb_bytes <= sizeof(buf));                                        
>         
> +    if (s->tx.command & COMMAND_NC) {                                        
>         
> +        logout("support for NC=1 is not implemented\n");                     
>         
> +       assert (0);                                                           
>         
> +    }                                                                        
>         
> +    if (tcb_bytes > sizeof(buf)) {                                           
>         
> +        logout("illegal value of the TCB byte count! (cannot be greater than 
> 0x%04x)\n", sizeof(buf));
> +       assert (0);                                                           
>                          
> +    }                                                                        
>                          

That part I meant with cosmetics, I think it would simplify things to do it at 
best in
a different patch. Btw. there is a huge amount of trailing whitespace after 
that }

>      if (!((tcb_bytes > 0) || (tbd_array != 0xffffffff))) {                   
>                          
>          logout                                                               
>                          
>              ("illegal values of TBD array address and TCB byte count!\n");   
>                          
>      }                                                                        
>                          
> -    for (size = 0; size < tcb_bytes; ) {                                     
>                          
> -        uint32_t tx_buffer_address = ldl_le_phys(tbd_address);               
>                          
> -        uint16_t tx_buffer_size = lduw_le_phys(tbd_address + 4);             
>                          
> -        //~ uint16_t tx_buffer_el = lduw_le_phys(tbd_address + 6);           
>                          
> -        tbd_address += 8;                                                    
>                          
> -        TRACE(RXTX, logout                                                   
>                          
> -            ("TBD (simplified mode): buffer address 0x%08x, size 0x%04x\n",  
>                          
> -             tx_buffer_address, tx_buffer_size));                            
>                          
> -        assert(size + tx_buffer_size <= sizeof(buf));                        
>                          
> -        cpu_physical_memory_read(tx_buffer_address, &buf[size],              
>                          
> -                                 tx_buffer_size);                            
>                          
> -        size += tx_buffer_size;                                              
>                          
> +    if (tcb_bytes > 0) {                                                     
>                          
> +           TRACE(RXTX, logout("TCB byte count>0, adding the data after the 
> TCB to the buffer\n"));    
> +           cpu_physical_memory_read(s->cb_address + 0x10, &buf[0], 
> tcb_bytes);                        

Lots of trailing whitespace here too. And s->cb_address + 0x10 was already 
calulated
before and stored in tbd_address.
Also if extended TCB is enabled I think it must be + 0x20. I admit that the 
Intel
specification says differently but that does not make any sense (I suspect
they just did not consider that case)...
Apart from that I think this is correct, reading the specification.

> @@ -918,6 +931,9 @@
>              /* Starting with offset 8, the command contains
>               * 64 dwords microcode which we just ignore here. */
>              break;                                              
> +        case CmdDiagnose:                                       
> +            TRACE(OTHER, logout("   Rolando: diagnose\n"));     
> +            break;                                              
>          default:                                                
>              missing("undefined command");                       
>          }                                                       
> ===========> REASON: even though diagnose is called by VxWorks, it should 
> work without this (I realized that later).

No, it will crash with an assert due to the missing(...), as said I already
sent a patch myself (that sets status to 0).

> 4)
> @@ -1079,9 +1095,9 @@
>      return val;     
>  }                   
>                      
> -static void eepro100_write_eeprom(eeprom_t * eeprom, uint8_t val)
> +static void eepro100_write_eeprom(eeprom_t * eeprom, uint32_t val)
>  {                                                                 
> -    TRACE(OTHER, logout("val=0x%02x\n", val));                    
> +    TRACE(OTHER, logout("val=0x%08x\n", val));                    
>                                                                    
>      /* mask unwriteable bits */                                   
>      //~ val = SET_MASKED(val, 0x31, eeprom->value);               
> ===========> REASON: I don't remember exactly what part was requiring this 
> but my emulation failed without this change. If interested, I could change 
> this back and tell you what fails.

Except for a compiler bug I can't see how it could make a difference...

> 6)
> @@ -1484,6 +1504,23 @@
>      case SCBeeprom:
>          eepro100_write_eeprom(s->eeprom, val);
>          break;
> +    case SCBPointer:
> +       s->pointer = (s->pointer & 0xffff0000) + val;
> +       logout("   Rolando: wrote the General Pointer, it's now 0x%08x\n", 
> s->pointer);
> +       break;
> +    case SCBPointer+2:
> +       s->pointer = (s->pointer & 0xffff) + ( val * 0x10000 );
> +       logout("   Rolando: wrote the General Pointer, it's now 0x%08x\n", 
> s->pointer);
> +       break;

I guess reasonable in principle (though it might be simpler to get rid of the
pointer variable and just read it whenever necessary form s->mem),
except that you need to use le16_to_cpu on val I think.
(val << 16) seems better to me than (val * 0x10000).