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

[Thomas Huth]

On 27.01.2016 10:02, Nikunj A Dadhania wrote:
> Thomas Huth <thuth at redhat.com> writes:
> 
>> 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
> 
> 
> How about something like this:
> * changed the signature of virtio-net-open
> * get the mac address during the virtionet_init routine
> * setup-mac after the virtionet_open

Sounds like this should work, too, and is even easier!
(I initially thought we might need support the possibility to let the
user override the local-mac-address property, but it seems this is not
supported by the other drivers, too, so we likely also don't need this
for virtio-net)

> * BTW, .code file has some indentation issues.

Maybe clean it up in your clean-up patch 04/22 ?

> diff --git a/board-qemu/slof/virtio-net.fs b/board-qemu/slof/virtio-net.fs
> index 412b34f..2a5a728 100644
> --- a/board-qemu/slof/virtio-net.fs
> +++ b/board-qemu/slof/virtio-net.fs
> @@ -21,15 +21,25 @@ 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
>           \ 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
> -            drop TO virtio-net-priv
> -         THEN
> -         true
> +         virtiodev virtio-net-open not IF ." virtio-net-open failed" EXIT THEN

The bug was there in the old code already ... but anyway, I think there
should be a FALSE right before the EXIT here?

> +         TO virtio-net-priv
> +         setup-mac
> +         s" local-mac-address" get-node get-property not IF 2drop true ELSE FALSE THEN

After "setup-mac", I think there's always a "local-mac-address"
property, so you can simply replace this last line with "true".

>        ELSE
>           false
>        THEN
> @@ -77,17 +87,6 @@ virtiodev virtio-setup-vd
>     s" ping" obp-tftp-package @ $call-method
>  ;
>  
> -\ Set up MAC address from config virtqueue
> -6 BUFFER: local-mac
> -: setup-mac  ( -- )
> -   6 0 DO
> -      virtiodev i 1 virtio-get-config
> -      local-mac i + c!
> -   LOOP
> -   local-mac 6 encode-bytes  s" local-mac-address"  property
> -;
> -setup-mac
> -
>  : setup-alias  ( -- )
>     " net" get-next-alias ?dup IF
>        get-node node>path set-alias
...
> diff --git a/lib/libvirtio/virtio.code b/lib/libvirtio/virtio.code
> index 258b9bb..217c0a1 100644
> --- a/lib/libvirtio/virtio.code
> +++ b/lib/libvirtio/virtio.code
> @@ -122,14 +122,12 @@ MIRP
>  
>  /******** virtio-net ********/
>  
> -// : virtio-net-open ( mac-addr-str len dev -- false | [ driver true ] )
> +// : virtio-net-open ( dev -- false | [ driver true ] )
>  PRIM(virtio_X2d_net_X2d_open)
>  {
> -	void *dev = TOS.a; POP;
> -	int len = TOS.u; POP;
> -        char *mac_addr = TOS.a;
> +	void *dev = TOS.a;
>  
> -        net_driver_t *net_driver = virtionet_open(mac_addr, len, dev);
> +        net_driver_t *net_driver = virtionet_open(dev);

Indent the new line with a tab? (no need to keep the bad coding style
here) ... or, as mentioned above, clean it up in 04/22 already.

>          if (net_driver) {
>                  TOS.u = (unsigned long)net_driver; PUSH;
>>
>>
>> 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.
> 
> Not in this patch, but while enabling virtio 1.0, i have enabled it. Its
> a magic number(BIT(5)), forgot to convert to a define.
> 
> 		if (virtio_negotiate_guest_features(&virtiodev, VIRTIO_F_VERSION_1 | (BIT(5))))

Ok, so that should be fine (but a define would be nice, of course).

 Thomas