Re: [Qemu-devel] [PATCHv2] hw/net: implement MIB counters in mcf_fec driver

[Laurent Vivier]

Le 03/02/2017 à 09:40, Laurent Vivier a écrit :
> Add cc: to Network devices odd fixer.
> 
> Le 11/01/2017 à 12:30, Greg Ungerer a écrit :
>> The FEC ethernet hardware module used on ColdFire SoC parts contains a
>> block of RAM used to maintain hardware counters. This block is accessible
>> via the usual FEC register address space. There is currently no support
>> for this in the QEMU mcf_fec driver.
>>
>> Add support for storing a MIB RAM block, and provide register level
>> access to it. Also implement a basic set of stats collection functions
>> to populate MIB data fields.
>>
>> This support tested running a Linux target and using the net-tools
>> "ethtool -S" option. As of linux-4.9 the kernels FEC driver makes
>> accesses to the MIB counters during its initialization (which it never
>> did before), and so this version of Linux will now fail with the QEMU
>> error:
>>
>>     qemu: hardware error: mcf_fec_read: Bad address 0x200
>>
>> This MIB counter support fixes this problem.
>>
>> Signed-off-by: Greg Ungerer <address@hidden>
>> ---
>>
>> v2: fixed formatting problems picked up by checkpatch
>>
>>  hw/net/mcf_fec.c | 115 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 115 insertions(+)
>>
>> diff --git a/hw/net/mcf_fec.c b/hw/net/mcf_fec.c
>> index 4025eb3..c74804b 100644
>> --- a/hw/net/mcf_fec.c
>> +++ b/hw/net/mcf_fec.c
>> @@ -25,6 +25,7 @@ do { printf("mcf_fec: " fmt , ## __VA_ARGS__); } while (0)
>>  
>>  #define FEC_MAX_DESC 1024
>>  #define FEC_MAX_FRAME_SIZE 2032
>> +#define FEC_MIB_SIZE 64
>>  
>>  typedef struct {
>>      MemoryRegion *sysmem;
>> @@ -48,6 +49,7 @@ typedef struct {
>>      uint32_t erdsr;
>>      uint32_t etdsr;
>>      uint32_t emrbr;
>> +    uint32_t mib[FEC_MIB_SIZE];
>>  } mcf_fec_state;
>>  
>>  #define FEC_INT_HB   0x80000000
> ...
>>  static void mcf_fec_do_tx(mcf_fec_state *s)
>>  {
>>      uint32_t addr;
>> @@ -178,6 +262,7 @@ static void mcf_fec_do_tx(mcf_fec_state *s)
>>              /* Last buffer in frame.  */
>>              DPRINTF("Sending packet\n");
>>              qemu_send_packet(qemu_get_queue(s->nic), frame, frame_size);
>> +            mcf_fec_tx_stats(s, frame_size);
>>              ptr = frame;
>>              frame_size = 0;
>>              s->eir |= FEC_INT_TXF;
>> @@ -298,6 +383,7 @@ static uint64_t mcf_fec_read(void *opaque, hwaddr addr,
>>      case 0x180: return s->erdsr;
>>      case 0x184: return s->etdsr;
>>      case 0x188: return s->emrbr;
>> +    case 0x200 ... 0x2e0: return s->mib[(addr & 0x1ff) / 4];
> 
> As FEC_MIB_SIZE is 64 and 0x1ff / 4 is 127, you can have overflow here
> and below.

OK, this is limited by the "case 0x200 ... 0x2e0", so there is no problem.

Reviewed-by: Laurent Vivier <address@hidden>

Laurent