[SLOF] [PATCH v3 12/22] virtio-net: move setup-mac to the open routine
thuth at redhat.com
Sat Jan 23 19:30:18 AEDT 2016
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
- 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.
More information about the SLOF