| Message ID | 20180830142708.14311-6-sameeh@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | Virtio-net: Support RSS | expand |
On 2018年08月30日 22:27, Sameeh Jubran wrote: > From: Sameeh Jubran <sjubran@redhat.com> > > Signed-off-by: Sameeh Jubran <sjubran@redhat.com> > --- > hw/net/virtio-net.c | 122 ++++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 105 insertions(+), 17 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index e7c4ce6f66..4a52a6a1d0 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -972,41 +972,129 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd, > return VIRTIO_NET_OK; > } > > -static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd, > + > +static int virtio_net_ctrl_sm_rss(VirtIONet *n, uint32_t cmd, > struct iovec *iov, unsigned int iov_cnt, > struct iovec *iov_in, unsigned int iov_cnt_in, > - size_t *size_in) > + size_t *size_in) > +{ > + size_t s; > + uint32_t supported_hash_function = 0; > + > + switch (cmd) { > + case VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS: > + supported_hash_function |= RSS_HASH_FUNCTION_TOEPLITZ; > + if (!size_in) { > + return VIRTIO_NET_ERR; > + } > + s = iov_from_buf(iov_in, iov_cnt_in, 0, > + &supported_hash_function, > + supported_hash_function); Indentation looks wrong. > + if (s != sizeof(n->supported_modes) || > + !size_in) { > + return VIRTIO_NET_ERR; > + } > + *size_in = s; > + break; > + case VIRTIO_NET_SM_CTRL_RSS_SET: > + if (!n->rss_conf) { > + n->rss_conf = g_malloc0( > + sizeof(struct virtio_net_rss_conf)); > + } else if (iov == NULL || iov_cnt == 0) { > + g_free(n->rss_conf->ptrs.hash_key); > + g_free(n->rss_conf->ptrs.indirection_table); > + g_free(n->rss_conf); > + return VIRTIO_NET_OK; > + } > + s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf, > + sizeof(struct virtio_net_rss_conf) - > + sizeof(struct virtio_net_rss_conf_ptrs)); > + > + if (s != sizeof(struct virtio_net_rss_conf) - > + sizeof(struct virtio_net_rss_conf_ptrs)) { > + return VIRTIO_NET_ERR; > + } > + n->rss_conf->ptrs.hash_key = g_malloc0(sizeof(uint8_t) * > + n->rss_conf->hash_key_length); What happens if n->rss_conf != 0 && iov != NULL? Looks like a guest trigger-able OOM? Btw e.g "conf_ptrs" sounds misleading, why not just embed hash key and indirection table pointers directly in rss_conf structure itself? > + s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf->ptrs.hash_key, > + sizeof(uint8_t) * n->rss_conf->hash_key_length); > + if (s != sizeof(uint8_t) * n->rss_conf->hash_key_length) { > + g_free(n->rss_conf->ptrs.hash_key); > + return VIRTIO_NET_ERR; > + } > + n->rss_conf->ptrs.indirection_table > + = g_malloc0(sizeof(uint32_t) * > + n->rss_conf->indirection_table_length); > + s = iov_to_buf(iov, iov_cnt, 0, > + n->rss_conf->ptrs.indirection_table, sizeof(uint32_t) * > + n->rss_conf->indirection_table_length); > + if (s != sizeof(uint32_t) * > + n->rss_conf->indirection_table_length) { > + g_free(n->rss_conf->ptrs.hash_key); > + g_free(n->rss_conf->ptrs.indirection_table); > + return VIRTIO_NET_ERR; > + } > + /* do bpf magic */ > + break; > + default: > + return VIRTIO_NET_ERR; > + } > + > + return VIRTIO_NET_OK; > +} > + > +static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd, > + struct iovec *iov, unsigned int iov_cnt, > + struct iovec *iov_in, unsigned int iov_in_cnt, > + size_t *size_in) > { > size_t s; > struct virtio_net_steering_mode sm; > + int status = 0; > + size_t size_in_cmd = 0; > > switch (cmd) { > case VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES: > if (!size_in) { > return VIRTIO_NET_ERR; > } > - s = iov_from_buf(iov_in, iov_cnt_in, 0, > - &n->supported_modes, sizeof(n->supported_modes)); > + n->supported_modes.steering_modes |= STEERING_MODE_RSS | > + STEERING_MODE_AUTO; We should have a property for RSS instead of hard coding it here. Thanks > + s = iov_from_buf(iov_in, iov_in_cnt, 0, > + &n->supported_modes, > + sizeof(n->supported_modes)); > if (s != sizeof(n->supported_modes) || > - !size_in) { > + !size_in) { > return VIRTIO_NET_ERR; > } > - *size_in = s; > - break; > + *size_in = s; > + break; > case VIRTIO_NET_CTRL_SM_CONTROL: > - s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm) - > - sizeof(union command_data)); > - if (s != sizeof(sm) - sizeof(union command_data)) { > + s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm)); > + if (s != sizeof(sm)) { > + return VIRTIO_NET_ERR; > + } > + iov_discard_front(&iov, &iov_cnt, sizeof(sm)); > + /* TODO handle the case where we change mode, call the old */ > + /* mode function with null ptrs should do the trick of */ > + /* freeing any resources */ > + switch (sm.steering_mode) { > + case STEERING_MODE_AUTO: > + break; > + case STEERING_MODE_RSS: > + status = virtio_net_ctrl_sm_rss(n, sm.command, > + iov, iov_cnt, iov_in, iov_in_cnt, > + &size_in_cmd); > + if (status == VIRTIO_NET_OK && size_in_cmd > 0) { > + *size_in += size_in_cmd; > + } > + break; > + default: > return VIRTIO_NET_ERR; > } > - /* switch (cmd) > - { > - dafault: > - return VIRTIO_NET_ERR; > - } */ > - break; > + break; > default: > - return VIRTIO_NET_ERR; > + return VIRTIO_NET_ERR; > } > > return VIRTIO_NET_OK;
On Mon, Sep 3, 2018 at 6:48 AM, Jason Wang <jasowang@redhat.com> wrote: > > > On 2018年08月30日 22:27, Sameeh Jubran wrote: >> >> From: Sameeh Jubran <sjubran@redhat.com> >> >> Signed-off-by: Sameeh Jubran <sjubran@redhat.com> >> --- >> hw/net/virtio-net.c | 122 >> ++++++++++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 105 insertions(+), 17 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index e7c4ce6f66..4a52a6a1d0 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -972,41 +972,129 @@ static int virtio_net_handle_mq(VirtIONet *n, >> uint8_t cmd, >> return VIRTIO_NET_OK; >> } >> -static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd, >> + >> +static int virtio_net_ctrl_sm_rss(VirtIONet *n, uint32_t cmd, >> struct iovec *iov, unsigned int iov_cnt, >> struct iovec *iov_in, unsigned int >> iov_cnt_in, >> - size_t *size_in) >> + size_t *size_in) >> +{ >> + size_t s; >> + uint32_t supported_hash_function = 0; >> + >> + switch (cmd) { >> + case VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS: >> + supported_hash_function |= RSS_HASH_FUNCTION_TOEPLITZ; >> + if (!size_in) { >> + return VIRTIO_NET_ERR; >> + } >> + s = iov_from_buf(iov_in, iov_cnt_in, 0, >> + &supported_hash_function, >> + supported_hash_function); > > > Indentation looks wrong. > > >> + if (s != sizeof(n->supported_modes) || >> + !size_in) { >> + return VIRTIO_NET_ERR; >> + } >> + *size_in = s; >> + break; >> + case VIRTIO_NET_SM_CTRL_RSS_SET: >> + if (!n->rss_conf) { >> + n->rss_conf = g_malloc0( >> + sizeof(struct virtio_net_rss_conf)); >> + } else if (iov == NULL || iov_cnt == 0) { >> + g_free(n->rss_conf->ptrs.hash_key); >> + g_free(n->rss_conf->ptrs.indirection_table); >> + g_free(n->rss_conf); >> + return VIRTIO_NET_OK; >> + } >> + s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf, >> + sizeof(struct virtio_net_rss_conf) - >> + sizeof(struct virtio_net_rss_conf_ptrs)); >> + >> + if (s != sizeof(struct virtio_net_rss_conf) - >> + sizeof(struct virtio_net_rss_conf_ptrs)) { >> + return VIRTIO_NET_ERR; >> + } >> + n->rss_conf->ptrs.hash_key = g_malloc0(sizeof(uint8_t) * >> + n->rss_conf->hash_key_length); > > > What happens if n->rss_conf != 0 && iov != NULL? Looks like a guest > trigger-able OOM? > > Btw e.g "conf_ptrs" sounds misleading, why not just embed hash key and > indirection table pointers directly in rss_conf structure itself? It was neater to do it like this so I can use: sizeof(struct virtio_net_rss_conf) - sizeof(struct virtio_net_rss_conf_ptrs) when reading from the iov, but yeah it not a big deal and I can put the pointers there as well > > >> + s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf->ptrs.hash_key, >> + sizeof(uint8_t) * n->rss_conf->hash_key_length); >> + if (s != sizeof(uint8_t) * n->rss_conf->hash_key_length) { >> + g_free(n->rss_conf->ptrs.hash_key); >> + return VIRTIO_NET_ERR; >> + } >> + n->rss_conf->ptrs.indirection_table >> + = g_malloc0(sizeof(uint32_t) * >> + n->rss_conf->indirection_table_length); >> + s = iov_to_buf(iov, iov_cnt, 0, >> + n->rss_conf->ptrs.indirection_table, sizeof(uint32_t) * >> + n->rss_conf->indirection_table_length); >> + if (s != sizeof(uint32_t) * >> + n->rss_conf->indirection_table_length) { >> + g_free(n->rss_conf->ptrs.hash_key); >> + g_free(n->rss_conf->ptrs.indirection_table); >> + return VIRTIO_NET_ERR; >> + } >> + /* do bpf magic */ >> + break; >> + default: >> + return VIRTIO_NET_ERR; >> + } >> + >> + return VIRTIO_NET_OK; >> +} >> + >> +static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd, >> + struct iovec *iov, unsigned int iov_cnt, >> + struct iovec *iov_in, unsigned int >> iov_in_cnt, >> + size_t *size_in) >> { >> size_t s; >> struct virtio_net_steering_mode sm; >> + int status = 0; >> + size_t size_in_cmd = 0; >> switch (cmd) { >> case VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES: >> if (!size_in) { >> return VIRTIO_NET_ERR; >> } >> - s = iov_from_buf(iov_in, iov_cnt_in, 0, >> - &n->supported_modes, sizeof(n->supported_modes)); >> + n->supported_modes.steering_modes |= STEERING_MODE_RSS | >> + STEERING_MODE_AUTO; > > > We should have a property for RSS instead of hard coding it here. Agree > > Thanks > > >> + s = iov_from_buf(iov_in, iov_in_cnt, 0, >> + &n->supported_modes, >> + sizeof(n->supported_modes)); >> if (s != sizeof(n->supported_modes) || >> - !size_in) { >> + !size_in) { >> return VIRTIO_NET_ERR; >> } >> - *size_in = s; >> - break; >> + *size_in = s; >> + break; >> case VIRTIO_NET_CTRL_SM_CONTROL: >> - s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm) - >> - sizeof(union command_data)); >> - if (s != sizeof(sm) - sizeof(union command_data)) { >> + s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm)); >> + if (s != sizeof(sm)) { >> + return VIRTIO_NET_ERR; >> + } >> + iov_discard_front(&iov, &iov_cnt, sizeof(sm)); >> + /* TODO handle the case where we change mode, call the old */ >> + /* mode function with null ptrs should do the trick of */ >> + /* freeing any resources */ >> + switch (sm.steering_mode) { >> + case STEERING_MODE_AUTO: >> + break; >> + case STEERING_MODE_RSS: >> + status = virtio_net_ctrl_sm_rss(n, sm.command, >> + iov, iov_cnt, iov_in, iov_in_cnt, >> + &size_in_cmd); >> + if (status == VIRTIO_NET_OK && size_in_cmd > 0) { >> + *size_in += size_in_cmd; >> + } >> + break; >> + default: >> return VIRTIO_NET_ERR; >> } >> - /* switch (cmd) >> - { >> - dafault: >> - return VIRTIO_NET_ERR; >> - } */ >> - break; >> + break; >> default: >> - return VIRTIO_NET_ERR; >> + return VIRTIO_NET_ERR; >> } >> return VIRTIO_NET_OK; > >
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index e7c4ce6f66..4a52a6a1d0 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -972,41 +972,129 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd, return VIRTIO_NET_OK; } -static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd, + +static int virtio_net_ctrl_sm_rss(VirtIONet *n, uint32_t cmd, struct iovec *iov, unsigned int iov_cnt, struct iovec *iov_in, unsigned int iov_cnt_in, - size_t *size_in) + size_t *size_in) +{ + size_t s; + uint32_t supported_hash_function = 0; + + switch (cmd) { + case VIRTIO_NET_SM_CTRL_RSS_GET_SUPPORTED_FUNCTIONS: + supported_hash_function |= RSS_HASH_FUNCTION_TOEPLITZ; + if (!size_in) { + return VIRTIO_NET_ERR; + } + s = iov_from_buf(iov_in, iov_cnt_in, 0, + &supported_hash_function, + supported_hash_function); + if (s != sizeof(n->supported_modes) || + !size_in) { + return VIRTIO_NET_ERR; + } + *size_in = s; + break; + case VIRTIO_NET_SM_CTRL_RSS_SET: + if (!n->rss_conf) { + n->rss_conf = g_malloc0( + sizeof(struct virtio_net_rss_conf)); + } else if (iov == NULL || iov_cnt == 0) { + g_free(n->rss_conf->ptrs.hash_key); + g_free(n->rss_conf->ptrs.indirection_table); + g_free(n->rss_conf); + return VIRTIO_NET_OK; + } + s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf, + sizeof(struct virtio_net_rss_conf) - + sizeof(struct virtio_net_rss_conf_ptrs)); + + if (s != sizeof(struct virtio_net_rss_conf) - + sizeof(struct virtio_net_rss_conf_ptrs)) { + return VIRTIO_NET_ERR; + } + n->rss_conf->ptrs.hash_key = g_malloc0(sizeof(uint8_t) * + n->rss_conf->hash_key_length); + s = iov_to_buf(iov, iov_cnt, 0, n->rss_conf->ptrs.hash_key, + sizeof(uint8_t) * n->rss_conf->hash_key_length); + if (s != sizeof(uint8_t) * n->rss_conf->hash_key_length) { + g_free(n->rss_conf->ptrs.hash_key); + return VIRTIO_NET_ERR; + } + n->rss_conf->ptrs.indirection_table + = g_malloc0(sizeof(uint32_t) * + n->rss_conf->indirection_table_length); + s = iov_to_buf(iov, iov_cnt, 0, + n->rss_conf->ptrs.indirection_table, sizeof(uint32_t) * + n->rss_conf->indirection_table_length); + if (s != sizeof(uint32_t) * + n->rss_conf->indirection_table_length) { + g_free(n->rss_conf->ptrs.hash_key); + g_free(n->rss_conf->ptrs.indirection_table); + return VIRTIO_NET_ERR; + } + /* do bpf magic */ + break; + default: + return VIRTIO_NET_ERR; + } + + return VIRTIO_NET_OK; +} + +static int virtio_net_ctrl_steering_mode(VirtIONet *n, uint8_t cmd, + struct iovec *iov, unsigned int iov_cnt, + struct iovec *iov_in, unsigned int iov_in_cnt, + size_t *size_in) { size_t s; struct virtio_net_steering_mode sm; + int status = 0; + size_t size_in_cmd = 0; switch (cmd) { case VIRTIO_NET_CTRL_SM_GET_SUPPORTED_MODES: if (!size_in) { return VIRTIO_NET_ERR; } - s = iov_from_buf(iov_in, iov_cnt_in, 0, - &n->supported_modes, sizeof(n->supported_modes)); + n->supported_modes.steering_modes |= STEERING_MODE_RSS | + STEERING_MODE_AUTO; + s = iov_from_buf(iov_in, iov_in_cnt, 0, + &n->supported_modes, + sizeof(n->supported_modes)); if (s != sizeof(n->supported_modes) || - !size_in) { + !size_in) { return VIRTIO_NET_ERR; } - *size_in = s; - break; + *size_in = s; + break; case VIRTIO_NET_CTRL_SM_CONTROL: - s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm) - - sizeof(union command_data)); - if (s != sizeof(sm) - sizeof(union command_data)) { + s = iov_to_buf(iov, iov_cnt, 0, &sm, sizeof(sm)); + if (s != sizeof(sm)) { + return VIRTIO_NET_ERR; + } + iov_discard_front(&iov, &iov_cnt, sizeof(sm)); + /* TODO handle the case where we change mode, call the old */ + /* mode function with null ptrs should do the trick of */ + /* freeing any resources */ + switch (sm.steering_mode) { + case STEERING_MODE_AUTO: + break; + case STEERING_MODE_RSS: + status = virtio_net_ctrl_sm_rss(n, sm.command, + iov, iov_cnt, iov_in, iov_in_cnt, + &size_in_cmd); + if (status == VIRTIO_NET_OK && size_in_cmd > 0) { + *size_in += size_in_cmd; + } + break; + default: return VIRTIO_NET_ERR; } - /* switch (cmd) - { - dafault: - return VIRTIO_NET_ERR; - } */ - break; + break; default: - return VIRTIO_NET_ERR; + return VIRTIO_NET_ERR; } return VIRTIO_NET_OK;