Re: [Qemu-devel] [PATCH 1/2 V4] e1000: add the ability to select among several specific types of e1000[e]; 82566DM emulation ; some pointers to documentations and details.

[Romain Dolbeau]

2014-03-10 13:56 GMT+01:00 Stefan Hajnoczi <address@hidden>:
>> @@ -409,6 +468,9 @@ set_ctrl(E1000State *s, int index, uint32_t val)
>>  {
>>      /* RST is self clearing */
>>      s->mac_reg[CTRL] = val & ~E1000_CTRL_RST;
>> +    if (val & E1000_CTRL_RST) {
>> +        e1000_reset(s);
>> +    }
>
> Maybe this should be a separate commit.  Is this a bugfix?

Honoring resets is required for the semaphores used in recent cards, IIRC.

> Where does this template come from and what is the relationship between
> the EEPROM and the flash?  Does the device have both EEPROM and flash?

As far as I can tell, same kind of data, different storage medium. And it comes
from the documentation :-)

> How are the BARs numbered on the real device?  Do all models have flash?

The driver always use 1 for flash, even though not all devices have
flash. Didn't
quite understand how the driver tells the difference.

> We cannot change the BARs, it would break migration between old and new
> QEMU.

1 is required for flash in the driver, it's hardcoded.

>> @@ -542,9 +587,9 @@
>>  #define E1000_EEPROM_SWDPIN0   0x0001   /* SWDPIN 0 EEPROM Value */
>>  #define E1000_EEPROM_LED_LOGIC 0x0020   /* Led Logic Word */
>>  #define E1000_EEPROM_RW_REG_DATA   16   /* Offset to data in EEPROM 
>> read/write registers */
>> -#define E1000_EEPROM_RW_REG_DONE   0x10 /* Offset to READ/WRITE done bit */
>> +#define E1000_EEPROM_RW_REG_DONE   0x2  /* Offset to READ/WRITE done bit */
>
> Please put a change like this in a separate commit and explain the
> reasoning.

Bugfix. The old data are wrong (according to the doc and the driver and the
fact that they don't work with the device that need them :-)

>>  #define E1000_EEPROM_RW_REG_START  1    /* First bit for telling part to 
>> start operation */
>> -#define E1000_EEPROM_RW_ADDR_SHIFT 8    /* Shift to the address bits */
>> +#define E1000_EEPROM_RW_ADDR_SHIFT 2    /* Shift to the address bits */
>
> This can go together with the E1000_EEPROM_RW_REG_DONE change.

Same answer.

> I don't think you can use C bitfields since the bit-ordering is up to
> the compiler.  It may or may not match the hardware register layout!
> Instead, use uint16_t and define bitmask constants so you can use
> bitwise-AND/OR.

Copy/paste from the e1000e driver... works for it :-)

Anyway, I probably won't be able to update the patch, I've already spent
a lot more time on it than I should have :-/

Cordially,

-- 
Romain Dolbeau