| Message ID | 20201023091559.4858-1-lulu@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] virtio-net: Add check for mac address while peer is vdpa | expand |
On 2020/10/23 下午5:15, Cindy Lu wrote: > Sometime vdpa get an all 0 mac address from the hardware, this will cause the traffic down > So we add the check for this part. > if we get an 0 mac address we will use the default mac address instead > > Signed-off-by: Cindy Lu <lulu@redhat.com> > --- > hw/net/virtio-net.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 9179013ac4..f1648fc47d 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > VirtIONet *n = VIRTIO_NET(vdev); > struct virtio_net_config netcfg; > NetClientState *nc = qemu_get_queue(n->nic); > + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > int ret = 0; > memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, > n->config_size); > if (ret != -1) { > - memcpy(config, &netcfg, n->config_size); > + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) { > + memcpy(config, &netcfg, n->config_size); > + } else { > + error_report("Get an all zero mac address from hardware"); > + } > } > } > } Applied. Thanks
On 2020/10/26 上午10:43, Jason Wang wrote: > > On 2020/10/23 下午5:15, Cindy Lu wrote: >> Sometime vdpa get an all 0 mac address from the hardware, this will >> cause the traffic down >> So we add the check for this part. >> if we get an 0 mac address we will use the default mac address instead >> >> Signed-off-by: Cindy Lu <lulu@redhat.com> >> --- >> hw/net/virtio-net.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 9179013ac4..f1648fc47d 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice >> *vdev, uint8_t *config) >> VirtIONet *n = VIRTIO_NET(vdev); >> struct virtio_net_config netcfg; >> NetClientState *nc = qemu_get_queue(n->nic); >> + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; >> int ret = 0; >> memset(&netcfg, 0 , sizeof(struct virtio_net_config)); >> @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice >> *vdev, uint8_t *config) >> ret = vhost_net_get_config(get_vhost_net(nc->peer), >> (uint8_t *)&netcfg, >> n->config_size); >> if (ret != -1) { >> - memcpy(config, &netcfg, n->config_size); >> + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) { >> + memcpy(config, &netcfg, n->config_size); >> + } else { >> + error_report("Get an all zero mac address from >> hardware"); >> + } >> } >> } >> } > > > Applied. > > Thanks > > Speak too fast. Some questions: 1) Any reason that you do such check or get_config() instead of doing it once in device initalization 2) the indentation looks wrong 3) There's no need for an error here since we can workaround it Thanks
Hi jason On Mon, Oct 26, 2020 at 11:20 AM Jason Wang <jasowang@redhat.com> wrote: > > On 2020/10/26 上午10:43, Jason Wang wrote: > > > > On 2020/10/23 下午5:15, Cindy Lu wrote: > >> Sometime vdpa get an all 0 mac address from the hardware, this will > >> cause the traffic down > >> So we add the check for this part. > >> if we get an 0 mac address we will use the default mac address instead > >> > >> Signed-off-by: Cindy Lu <lulu@redhat.com> > >> --- > >> hw/net/virtio-net.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >> index 9179013ac4..f1648fc47d 100644 > >> --- a/hw/net/virtio-net.c > >> +++ b/hw/net/virtio-net.c > >> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice > >> *vdev, uint8_t *config) > >> VirtIONet *n = VIRTIO_NET(vdev); > >> struct virtio_net_config netcfg; > >> NetClientState *nc = qemu_get_queue(n->nic); > >> + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > >> int ret = 0; > >> memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > >> @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice > >> *vdev, uint8_t *config) > >> ret = vhost_net_get_config(get_vhost_net(nc->peer), > >> (uint8_t *)&netcfg, > >> n->config_size); > >> if (ret != -1) { > >> - memcpy(config, &netcfg, n->config_size); > >> + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) { > >> + memcpy(config, &netcfg, n->config_size); > >> + } else { > >> + error_report("Get an all zero mac address from > >> hardware"); > >> + } > >> } > >> } > >> } > > > > > > Applied. > > > > Thanks > > > > > > Speak too fast. Some questions: > > 1) Any reason that you do such check or get_config() instead of doing it > once in device initalization > get_config() was called before the device realized. If we check in the device initalization, the mac address is already overwritten to 0 and we lost the default mac address Also for my understanding. the mac address read from hardware should never been 0, so we need to check it every time we got it, Just in case it will change to 0 and overwrite the mac address while the qemu running or some other case > 2) the indentation looks wrong > sure will fix this > 3) There's no need for an error here since we can workaround it sure, I will change it to log > > > Thanks > > <div dir="ltr"><div dir="ltr">Hi jason <div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Oct 26, 2020 at 11:20 AM Jason Wang <<a href="mailto:jasowang@redhat.com">jasowang@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> On 2020/10/26 上午10:43, Jason Wang wrote:<br> ><br> > On 2020/10/23 下午5:15, Cindy Lu wrote:<br> >> Sometime vdpa get an all 0 mac address from the hardware, this will <br> >> cause the traffic down<br> >> So we add the check for this part.<br> >> if we get an 0 mac address we will use the default mac address instead<br> >><br> >> Signed-off-by: Cindy Lu <<a href="mailto:lulu@redhat.com" target="_blank">lulu@redhat.com</a>><br> >> ---<br> >> hw/net/virtio-net.c | 7 ++++++-<br> >> 1 file changed, 6 insertions(+), 1 deletion(-)<br> >><br> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c<br> >> index 9179013ac4..f1648fc47d 100644<br> >> --- a/hw/net/virtio-net.c<br> >> +++ b/hw/net/virtio-net.c<br> >> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice <br> >> *vdev, uint8_t *config)<br> >> VirtIONet *n = VIRTIO_NET(vdev);<br> >> struct virtio_net_config netcfg;<br> >> NetClientState *nc = qemu_get_queue(n->nic);<br> >> + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } };<br> >> int ret = 0;<br> >> memset(&netcfg, 0 , sizeof(struct virtio_net_config));<br> >> @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice <br> >> *vdev, uint8_t *config)<br> >> ret = vhost_net_get_config(get_vhost_net(nc->peer), <br> >> (uint8_t *)&netcfg,<br> >> n->config_size);<br> >> if (ret != -1) {<br> >> - memcpy(config, &netcfg, n->config_size);<br> >> + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) {<br> >> + memcpy(config, &netcfg, n->config_size);<br> >> + } else {<br> >> + error_report("Get an all zero mac address from <br> >> hardware");<br> >> + }<br> >> }<br> >> }<br> >> }<br> ><br> ><br> > Applied.<br> ><br> > Thanks<br> ><br> ><br> <br> Speak too fast. Some questions:<br> <br> 1) Any reason that you do such check or get_config() instead of doing it <br> once in device initalization<br></blockquote><div>get_config() was called before the device realized. If we check in the device initalization, the mac address is already overwritten to 0 and we lost the default mac address</div><div>Also for my understanding. the mac address read from hardware should never been 0, so we need to check it every time we got it, Just in case it will change to 0 and overwrite the mac address while the qemu running or some other case </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 2) the indentation looks wrong<br></blockquote><div>sure will fix this </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 3) There's no need for an error here since we can workaround it</blockquote><div>sure, I will change it to log</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Thanks<br> <br> </blockquote></div></div>
----- Original Message ----- > Hi jason > > > On Mon, Oct 26, 2020 at 11:20 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > On 2020/10/26 上午10:43, Jason Wang wrote: > > > > > > On 2020/10/23 下午5:15, Cindy Lu wrote: > > >> Sometime vdpa get an all 0 mac address from the hardware, this will > > >> cause the traffic down > > >> So we add the check for this part. > > >> if we get an 0 mac address we will use the default mac address instead > > >> > > >> Signed-off-by: Cindy Lu <lulu@redhat.com> > > >> --- > > >> hw/net/virtio-net.c | 7 ++++++- > > >> 1 file changed, 6 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > >> index 9179013ac4..f1648fc47d 100644 > > >> --- a/hw/net/virtio-net.c > > >> +++ b/hw/net/virtio-net.c > > >> @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice > > >> *vdev, uint8_t *config) > > >> VirtIONet *n = VIRTIO_NET(vdev); > > >> struct virtio_net_config netcfg; > > >> NetClientState *nc = qemu_get_queue(n->nic); > > >> + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; > > >> int ret = 0; > > >> memset(&netcfg, 0 , sizeof(struct virtio_net_config)); > > >> @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice > > >> *vdev, uint8_t *config) > > >> ret = vhost_net_get_config(get_vhost_net(nc->peer), > > >> (uint8_t *)&netcfg, > > >> n->config_size); > > >> if (ret != -1) { > > >> - memcpy(config, &netcfg, n->config_size); > > >> + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) { > > >> + memcpy(config, &netcfg, n->config_size); > > >> + } else { > > >> + error_report("Get an all zero mac address from > > >> hardware"); > > >> + } > > >> } > > >> } > > >> } > > > > > > > > > Applied. > > > > > > Thanks > > > > > > > > > > Speak too fast. Some questions: > > > > 1) Any reason that you do such check or get_config() instead of doing it > > once in device initalization > > > get_config() was called before the device realized. Intersting, any reason that we need call get_config() even before device is realized? > If we check in the > device initalization, the mac address is already overwritten to 0 and > we lost the default mac address > Also for my understanding. the mac address read from hardware should never > been 0, so we need to check it every time we got it, This sounds self-contradict. > Just in case it will > change to 0 and overwrite the mac address while the qemu running or some > other case This looks like a bug in the device or driver. Even if this is true, you can only detect it via get_config(). And I don't think we need to workaround a buggy device. Thanks > > > 2) the indentation looks wrong > > > sure will fix this > > > 3) There's no need for an error here since we can workaround it > > sure, I will change it to log > > > > > > > > > Thanks > > > > >
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 9179013ac4..f1648fc47d 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -126,6 +126,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) VirtIONet *n = VIRTIO_NET(vdev); struct virtio_net_config netcfg; NetClientState *nc = qemu_get_queue(n->nic); + static const MACAddr zero = { .a = { 0, 0, 0, 0, 0, 0 } }; int ret = 0; memset(&netcfg, 0 , sizeof(struct virtio_net_config)); @@ -151,7 +152,11 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config) ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg, n->config_size); if (ret != -1) { - memcpy(config, &netcfg, n->config_size); + if (memcmp(&netcfg.mac, &zero, sizeof(zero)) != 0) { + memcpy(config, &netcfg, n->config_size); + } else { + error_report("Get an all zero mac address from hardware"); + } } } }
Sometime vdpa get an all 0 mac address from the hardware, this will cause the traffic down So we add the check for this part. if we get an 0 mac address we will use the default mac address instead Signed-off-by: Cindy Lu <lulu@redhat.com> --- hw/net/virtio-net.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)