[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