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

[Laurent Vivier]

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.

>      default:
>          hw_error("mcf_fec_read: Bad address 0x%x\n", (int)addr);
>          return 0;
> @@ -395,12 +481,40 @@ static void mcf_fec_write(void *opaque, hwaddr addr,
>      case 0x188:
>          s->emrbr = value > 0 ? value & 0x7F0 : 0x7F0;
>          break;
> +    case 0x200 ... 0x2e0:
> +        s->mib[(addr & 0x1ff) / 4] = value;

overflow here again.

Thanks,
Laurent