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

[Nikunj A Dadhania]

Thomas Huth <thuth at redhat.com> writes:

> 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(-)

>>
>>> 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 ?

Sure, i can do that.

>> +         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?

Oops, it was introduced by me while optimizing ;-)

-            virtiodev virtio-net-open dup not IF ." virtio-net-open failed" EXIT THEN
-            drop TO virtio-net-priv

I removed "dup" and "drop" but missed to add a "false" in exit case.

>
>> +         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".

Sure

>
>>        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.

I will clean up this in 4/22

>
>>          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).

Right.

Regards
Nikunj