[SLOF] [PATCH v3 12/22] virtio-net: move setup-mac to the open routine

[Thomas Huth]

On 22.01.2016 11:54, Nikunj A Dadhania wrote:
> MAC reading should be done after the initialization of the device after
> the features negotiation.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj at linux.vnet.ibm.com>
> ---
>  board-qemu/slof/virtio-net.fs | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

Looking at this a little bit more closely, I think there are still some
issues left (sorry for not having this done during v2 already) ...

> diff --git a/board-qemu/slof/virtio-net.fs b/board-qemu/slof/virtio-net.fs
> index 412b34f..130ee15 100644
> --- a/board-qemu/slof/virtio-net.fs
> +++ b/board-qemu/slof/virtio-net.fs
> @@ -21,9 +21,21 @@ virtiodev virtio-setup-vd
>  0 VALUE virtio-net-priv
>  0 VALUE open-count
>  
> +\ Set up MAC address from config virtqueue
> +6 BUFFER: local-mac
> +: setup-mac  ( -- )
> +   s" local-mac-address" get-node get-property not IF 2drop EXIT THEN
> +   6 0 DO
> +      virtiodev i 1 virtio-get-config
> +      local-mac i + c!
> +   LOOP
> +   local-mac 6 encode-bytes  s" local-mac-address"  property
> +;
> +
>  : open  ( -- okay? )
>     open-count 0= IF
>        open IF
> +         setup-mac
>           \ my-unit 1 rtas-set-tce-bypass
>           s" local-mac-address" get-node get-property not IF
>              virtiodev virtio-net-open dup not IF ." virtio-net-open failed" EXIT THEN

So the open function now first calls setup-mac, then checks for the
"local-mac-address" property, and if it is available, it does the
initialization (during virtio-net-open), including feature negotiation.
So the mac address is still set before the feature negotiation, isn't it?

I think you likely have to rework the code a little bit:
- allow virtio-net-open to be also called if the local-mac-address
  property is not available yet, e.g. by calling it with with len = 0
- in virtionet_open() only memcpy the mac address if len > 0
  ... and if len = 0, set dev->mac_address from the config space after
  feature negotiation
- call the setup-mac function after virtio-net-open


Beside that, I think there is another pre-existing problem in the MAC
address handling. According to the virtio-1.0 spec:

"A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it. If
the driver negotiates the VIRTIO_NET_F_MAC feature, the driver MUST set
the physical address of the NIC to mac. Otherwise, it SHOULD use a
locally-administered MAC address"

However, the driver in SLOF does not use VIRTIO_NET_F_MAC as far as I
can see, neither does it set a locally-administered MAC in case the host
does not provide a MAC.
IIRC (but better check that again), QEMU can always provide a MAC, so it
might be enough to simply negotiate the VIRTIO_NET_F_MAC feature here.
But for full spec compliance, there likely should be some code fore
creating a locally-administered MAC, too.

 Thomas