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

Nikunj A Dadhania nikunj at linux.vnet.ibm.com
Wed Jan 27 20:02:15 AEDT 2016


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
* BTW, .code file has some indentation issues.

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
+         TO virtio-net-priv
+         setup-mac
+         s" local-mac-address" get-node get-property not IF 2drop true ELSE FALSE THEN
       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-net.c b/lib/libvirtio/virtio-net.c
index 8ae3ae1..b92382c 100644
--- a/lib/libvirtio/virtio-net.c
+++ b/lib/libvirtio/virtio-net.c
@@ -157,7 +157,9 @@ static int virtionet_init(net_driver_t *driver)
 	virtio_queue_notify(&virtiodev, VQ_RX);
 
 	driver->running = 1;
-
+	for(i = 0; i < (int)sizeof(driver->mac_addr); i++) {
+		driver->mac_addr[i] = virtio_get_config(&virtiodev, i, 1);
+	}
 	return 0;
 
 dev_error:
@@ -284,7 +286,7 @@ static int virtionet_receive(char *buf, int maxlen)
 	return len;
 }
 
-net_driver_t *virtionet_open(char *mac_addr, int len, struct virtio_device *dev)
+net_driver_t *virtionet_open(struct virtio_device *dev)
 {
 	net_driver_t *driver;
 
@@ -294,7 +296,6 @@ net_driver_t *virtionet_open(char *mac_addr, int len, struct virtio_device *dev)
 		return NULL;
 	}
 
-	memcpy(driver->mac_addr, mac_addr, 6);
 	driver->running = 0;
 
 	if (virtionet_init_pci(dev))
diff --git a/lib/libvirtio/virtio-net.h b/lib/libvirtio/virtio-net.h
index 2196f87..195afd4 100644
--- a/lib/libvirtio/virtio-net.h
+++ b/lib/libvirtio/virtio-net.h
@@ -23,7 +23,7 @@ enum {
 	VQ_TX = 1,	/* Transmit Queue */
 };
 
-extern net_driver_t *virtionet_open(char *mac_addr, int len, struct virtio_device *dev);
+extern net_driver_t *virtionet_open(struct virtio_device *dev);
 extern void virtionet_close(net_driver_t *driver);
 extern int virtionet_read(char *buf, int len);
 extern int virtionet_write(char *buf, int len);
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);
 
         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))))

Regards,
Nikunj




More information about the SLOF mailing list