Re: [Qemu-devel] [PATCH v2 13/13] net: Introduce e1000e device emulation

[Dmitry Fleytman]

Hi Jason,

See below...

> On 7 Apr 2016, at 10:24 AM, Jason Wang  wrote:

[…]

> 
 +Device properties:
 +
 ++-++-+
 +| Propery name   | Description|  Type  | Default |
 ++|
 +|  subsys_ven| PCI device Subsystem Vendor ID | UINT16 | 0x8086  |
 +|||| |
 +|  subsys| PCI device Subsystem ID| UINT16 | 0   |
 +|||| |
 +|  vnet  | Use virtio headers or perform  | BOOL   | TRUE|
 +|| SW offloads emulation  || |
 +++++-+
>>> 
>>> You may using PropertyInfo which has a "description" field to do this
>>> better (e.g qdev_prop_vlan). Then there's even no need for this file.
>> 
>> We replaced this file with source code comments.
>> As for PropertyInfo description I can’t see any way to pass
>> description to DEFINE_PROP_* macros. Could you please elaborate on this?
> 
> You could use DEFINE_PROP() which can accept a PropertyInfo parameter.

I see now. Done.

> 
>> 
>>> 
 

[…]

 +if (!nc->peer || !qemu_has_vnet_hdr(nc->peer)) {
 +s->core.has_vnet = false;
>>> 
>>> So in fact we're probing the vnet support instead of doing vnet our own
>>> if backend does not support it. It seems to me that there's no need to
>>> having "vnet" property.
>> 
>> This property is intended for forcible disable of virtio headers
>> support. Possible use cases are verification of SW offloads logic and
>> work around for theoretical interoperability problems.
> 
> Ok, so maybe we'd better rename it to "disable_vnet”.

Ok. Renamed.

> 
>> 
>>> 
 +trace_e1000e_cfg_support_virtio(false);
 +return;
 +}
 +}
 +
> 
> 
> 
 +VMSTATE_UINT8(core.tx[1].sum_needed, E1000EState),
 +VMSTATE_UINT8(core.tx[1].ipcss, E1000EState),
 +VMSTATE_UINT8(core.tx[1].ipcso, E1000EState),
 +VMSTATE_UINT16(core.tx[1].ipcse, E1000EState),
 +VMSTATE_UINT8(core.tx[1].tucss, E1000EState),
 +VMSTATE_UINT8(core.tx[1].tucso, E1000EState),
 +VMSTATE_UINT16(core.tx[1].tucse, E1000EState),
 +VMSTATE_UINT8(core.tx[1].hdr_len, E1000EState),
 +VMSTATE_UINT16(core.tx[1].mss, E1000EState),
 +VMSTATE_UINT32(core.tx[1].paylen, E1000EState),
 +VMSTATE_INT8(core.tx[1].ip, E1000EState),
 +VMSTATE_INT8(core.tx[1].tcp, E1000EState),
 +VMSTATE_BOOL(core.tx[1].tse, E1000EState),
 +VMSTATE_BOOL(core.tx[1].cptse, E1000EState),
 +VMSTATE_BOOL(core.tx[1].skip_cp, E1000EState),
>>> 
>>> You can move the tx state into another structure and use VMSTATE_ARRAY()
>>> here.
>> 
>> Not sure I got you point. Could you please provide more details?
> 
> I mean e.g, move skip_cp into e1000e_txd_props and move tx_pkt out of
> e1000_tx. Then you can use VMSTATE_ARRAY to save and load
> e1000_txd_props array.

I got your point. Done via VMSTATE_STRUCT_ARRAY.


> 
>> 
>>> 
 +VMSTATE_END_OF_LIST()


[…]


>>> 
>>> Should we stop/start the above timers during vm stop/start through vm
>>> state change handler?
>> 
>> Not sure. Is there any reason for this?
> 
> Otherwise we may raise irq during vm stop?

Right. Done.

> 
> [...]
> 
 +
 +static inline void
 +e1000e_tx_ring_init(E1000ECore *core, E1000E_TxRing *txr, int idx)
 +{
 +static const E1000E_RingInfo i[E1000E_NUM_QUEUES] = {
 +{ TDBAH,  TDBAL,  TDLEN,  TDH,  TDT, 0 },
 +{ TDBAH1, TDBAL1, TDLEN1, TDH1, TDT1, 1 }
 +};
>>> 
>>> Instead of using static inside a function, why not just use a global
>>> array and then there's no need for this function and caller can access
>>> it directly?
>> 
>> Anyway there should be a function to combine the two assignments below.
>> And since this array is used by this function only it should better be
>> hidden.
> 
> I mean you can avoid the assignment before each transmission by just
> introducing arrays like:
> 
> int tdt[] = {TDT, TDT1};
> 
> And then access them through qidx, e.g: core->mac[tdt[qidx]].

I’m not sure about this.

Current approach requires assignment of one pointer.
Alternative approach requires assignment of queue index in the same place or 
passing queue index as function parameter everywhere.
Also as for me the current approach make code look simpler.

What do you think?

> 
>> 
>>> 
 +

[…]

I’m sending v4,

Thanks,
Dmitry

Re: [Qemu-devel] [PATCH v2 13/13] net: Introduce e1000e device emulation

[Jason Wang]

On 04/06/2016 04:22 PM, Dmitry Fleytman wrote:
> Hi Jason,
>
> Please see my comments below.
>
>> On 8 Mar 2016, at 11:31 AM, Jason Wang > > wrote:
>>
>>
>>
>> On 02/23/2016 01:37 AM, Leonid Bloch wrote:
>>> From: Dmitry Fleytman >> >
>>>
>>> This patch introduces emulation for the Intel 82574 adapter, AKA e1000e.
>>>
>>> This implementation is derived from the e1000 emulation code, and
>>> utilizes the TX/RX packet abstractions that were initially developed for
>>> the vmxnet3 device. Although some parts of the introduced code may be
>>> shared with e1000, the differences are substantial enough so that the
>>> only shared resources for the two devices are the definitions in
>>> hw/net/e1000_regs.h.
>>>
>>> Similarly to vmxnet3, the new device uses virtio headers for task
>>> offloads (for backends that support virtio extensions). Usage of
>>> virtio headers may be forcibly disabled via a boolean device property
>>> "vnet" (which is enabled by default). In such case task offloads
>>> will be performed in software, in the same way it is done on
>>> backends that do not support virtio headers.
>>>
>>> The device code is split into two parts:
>>>
>>>  1. hw/net/e1000e.c: QEMU-specific code for a network device;
>>>  2. hw/net/e1000e_core.[hc]: Device emulation according to the spec.
>>>
>>> The new device name is e1000e.
>>>
>>> Intel specifications for the 82574 controller are available at:
>>> http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf

[...]

>>> +Device properties:
>>> +
>>> ++-++-+
>>> +| Propery name   | Description|  Type  | Default |
>>> ++|
>>> +|  subsys_ven| PCI device Subsystem Vendor ID | UINT16 | 0x8086  |
>>> +|||| |
>>> +|  subsys| PCI device Subsystem ID| UINT16 | 0   |
>>> +|||| |
>>> +|  vnet  | Use virtio headers or perform  | BOOL   | TRUE|
>>> +|| SW offloads emulation  || |
>>> +++++-+
>>
>> You may using PropertyInfo which has a "description" field to do this
>> better (e.g qdev_prop_vlan). Then there's even no need for this file.
>
> We replaced this file with source code comments.
> As for PropertyInfo description I can’t see any way to pass
> description to DEFINE_PROP_* macros. Could you please elaborate on this?

You could use DEFINE_PROP() which can accept a PropertyInfo parameter.

>
>>
>>> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
>>> index 527d264..4201115 100644
>>> --- a/hw/net/Makefile.objs
>>> +++ b/hw/net/Makefile.objs

[...]

>>
>>> +MemoryRegion io;
>>> +MemoryRegion msix;
>>> +
>>> +uint32_t ioaddr;
>>> +
>>> +uint16_t subsys_ven;
>>> +uint16_t subsys;
>>> +
>>> +uint16_t subsys_ven_used;
>>> +uint16_t subsys_used;
>>> +
>>> +uint32_t intr_state;
>>> +bool use_vnet;
>>> +
>>> +E1000ECore core;
>>
>> What's the advantages of introducing another indirection level here?
>> Looks like it causes lots of extra code overheads.
>
> This was done by intention.
> Unlike e1000 and e1000e devices which are really different, e1000e and
> newer Intel devices like ixgb* are rather similar. Introduction of
> e1000e core abstraction will simplify possible code reuse in the future.

Ok, I see.

>
>>
>>> +
>>> +} E1000EState;
>>> +

[...]

>>> +static NetClientInfo net_e1000e_info = {
>>> +.type = NET_CLIENT_OPTIONS_KIND_NIC,
>>> +.size = sizeof(NICState),
>>> +.can_receive = e1000e_nc_can_receive,
>>> +.receive = e1000e_nc_receive,
>>> +.receive_iov = e1000e_nc_receive_iov,
>>
>> All of the above three functions has a NetClientState parameter, so
>> probably no need to have "nc" in their name.
>
> The issue is there are other functions with similar names but without
> _nc_...
>

Right.

>>
>>> +.link_status_changed = e1000e_set_link_status,
>>> +};
>>> +

[...]

>>> +for (i = 0; i < s->conf.peers.queues; i++) {
>>> +nc = qemu_get_subqueue(s->nic, i);
>>> +if (!nc->peer || !qemu_has_vnet_hdr(nc->peer)) {
>>> +s->core.has_vnet = false;
>>
>> So in fact we're probing the vnet support instead of doing vnet our own
>> if backend does not support it. It seems to me that there's no need to
>> having "vnet" property.
>
> This property is intended for forcible disable of virtio headers
> support. Possible use cases are verification of SW offloads logic and
> work around for theoretical interoperability problems.

Ok, so maybe we'd better rename it to "disable_vnet".

>
>>
>>> +

Re: [Qemu-devel] [PATCH v2 13/13] net: Introduce e1000e device emulation

[Dmitry Fleytman]

> On 6 Apr 2016, at 16:44 PM, Michael S. Tsirkin  wrote:
> 
> On Wed, Apr 06, 2016 at 04:42:57PM +0300, Dmitry Fleytman wrote:
>> 
>>> On 6 Apr 2016, at 16:23 PM, Michael S. Tsirkin  wrote:
>>> 
>>> On Wed, Apr 06, 2016 at 11:22:24AM +0300, Dmitry Fleytman wrote:
   +MemoryRegion flash;
 
 
   Looks there's no real implementation for flash. So is this really needed
   (e.g did WHQL or other test check for this?).
 
 
 From our experience some drivers may verify that device exposes all memory
 regions as required by HW specification, so we added this dummy memory 
 region
 to be on the safe side.
>>> 
>>> But then, why use an io region?
>> 
>> Do you mean we should use memory_region_init() instead of 
>> memory_region_init_io() for registration?
> 
> Exactly.

Good idea, thanks!

> 
>>> Also, pls add some comments that explains this.
>> 
>> I’ll add. Thanks.
>> 
>>> 
>>> -- 
>>> MSt

Re: [Qemu-devel] [PATCH v2 13/13] net: Introduce e1000e device emulation

[Michael S. Tsirkin]

On Wed, Apr 06, 2016 at 04:42:57PM +0300, Dmitry Fleytman wrote:
> 
> > On 6 Apr 2016, at 16:23 PM, Michael S. Tsirkin  wrote:
> > 
> > On Wed, Apr 06, 2016 at 11:22:24AM +0300, Dmitry Fleytman wrote:
> >>+MemoryRegion flash;
> >> 
> >> 
> >>Looks there's no real implementation for flash. So is this really needed
> >>(e.g did WHQL or other test check for this?).
> >> 
> >> 
> >> From our experience some drivers may verify that device exposes all memory
> >> regions as required by HW specification, so we added this dummy memory 
> >> region
> >> to be on the safe side.
> > 
> > But then, why use an io region?
> 
> Do you mean we should use memory_region_init() instead of 
> memory_region_init_io() for registration?

Exactly.

> > Also, pls add some comments that explains this.
> 
> I’ll add. Thanks.
> 
> > 
> > -- 
> > MSt

Re: [Qemu-devel] [PATCH v2 13/13] net: Introduce e1000e device emulation

[Dmitry Fleytman]

> On 6 Apr 2016, at 16:23 PM, Michael S. Tsirkin  wrote:
> 
> On Wed, Apr 06, 2016 at 11:22:24AM +0300, Dmitry Fleytman wrote:
>>+MemoryRegion flash;
>> 
>> 
>>Looks there's no real implementation for flash. So is this really needed
>>(e.g did WHQL or other test check for this?).
>> 
>> 
>> From our experience some drivers may verify that device exposes all memory
>> regions as required by HW specification, so we added this dummy memory region
>> to be on the safe side.
> 
> But then, why use an io region?

Do you mean we should use memory_region_init() instead of 
memory_region_init_io() for registration?

> Also, pls add some comments that explains this.

I’ll add. Thanks.

> 
> -- 
> MSt

Re: [Qemu-devel] [PATCH v2 13/13] net: Introduce e1000e device emulation

[Michael S. Tsirkin]

On Wed, Apr 06, 2016 at 11:22:24AM +0300, Dmitry Fleytman wrote:
> +MemoryRegion flash;
> 
> 
> Looks there's no real implementation for flash. So is this really needed
> (e.g did WHQL or other test check for this?).
> 
> 
> From our experience some drivers may verify that device exposes all memory
> regions as required by HW specification, so we added this dummy memory region
> to be on the safe side.

But then, why use an io region?
Also, pls add some comments that explains this.

-- 
MSt