| Message ID | ab861a8237c5e337bb0f969e1e8e761bc73901d5.1685359572.git.yin31149@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Vhost-vdpa Shadow Virtqueue Offloads support | expand |
On Mon, May 29, 2023 at 3:18 PM Hawkins Jiawei <yin31149@gmail.com> wrote: > > This patch introduces vhost_vdpa_net_load_offloads() to > restore offloads state at device's startup. > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > --- > net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 37cdc84562..682c749b19 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, > return *s->status != VIRTIO_NET_OK; > } > > +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, > + const VirtIONet *n) > +{ > + uint64_t features, offloads; > + ssize_t dev_written; > + > + features = n->parent_obj.guest_features; > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { > + return 0; > + } > + Maybe we can avoid sending this CVQ command if the guest already uses the default values? By default all features are enabled if I'm not wrong. I think the best way is to expose virtio_net_supported_guest_offloads or virtio_net_guest_offloads_by_features and then check if n->curr_guest_offloads is the same. We should do the same with vhost_vdpa_net_load_mq, but that is out of the scope of this series. Thanks! > + offloads = cpu_to_le64(n->curr_guest_offloads); > + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, > + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, > + &offloads, sizeof(offloads)); > + if (unlikely(dev_written < 0)) { > + return dev_written; > + } > + > + return *s->status != VIRTIO_NET_OK; > +} > + > static int vhost_vdpa_net_load(NetClientState *nc) > { > VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); > @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) > if (unlikely(r)) { > return r; > } > + r = vhost_vdpa_net_load_offloads(s, n); > + if (unlikely(r)) { > + return r; > + } > > return 0; > } > -- > 2.25.1 >
On Mon, May 29, 2023 at 9:18 PM Hawkins Jiawei <yin31149@gmail.com> wrote: > > This patch introduces vhost_vdpa_net_load_offloads() to > restore offloads state at device's startup. > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > --- > net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 37cdc84562..682c749b19 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, > return *s->status != VIRTIO_NET_OK; > } > > +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, > + const VirtIONet *n) > +{ > + uint64_t features, offloads; > + ssize_t dev_written; > + > + features = n->parent_obj.guest_features; Any reason you need to do tricks like this instead of using virtio_xxx_has_features()? > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { > + return 0; > + } > + > + offloads = cpu_to_le64(n->curr_guest_offloads); > + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, > + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, > + &offloads, sizeof(offloads)); > + if (unlikely(dev_written < 0)) { > + return dev_written; > + } > + > + return *s->status != VIRTIO_NET_OK; > +} > + > static int vhost_vdpa_net_load(NetClientState *nc) > { > VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); > @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) > if (unlikely(r)) { > return r; > } > + r = vhost_vdpa_net_load_offloads(s, n); > + if (unlikely(r)) { > + return r; > + } > > return 0; > } > -- > 2.25.1 >
On Wed, May 31, 2023 at 3:47 AM Jason Wang <jasowang@redhat.com> wrote: > > On Mon, May 29, 2023 at 9:18 PM Hawkins Jiawei <yin31149@gmail.com> wrote: > > > > This patch introduces vhost_vdpa_net_load_offloads() to > > restore offloads state at device's startup. > > > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > > --- > > net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > > index 37cdc84562..682c749b19 100644 > > --- a/net/vhost-vdpa.c > > +++ b/net/vhost-vdpa.c > > @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, > > return *s->status != VIRTIO_NET_OK; > > } > > > > +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, > > + const VirtIONet *n) > > +{ > > + uint64_t features, offloads; > > + ssize_t dev_written; > > + > > + features = n->parent_obj.guest_features; > > Any reason you need to do tricks like this instead of using > virtio_xxx_has_features()? > It can be replaced by virtio_vdev_has_feature, yes. Current code of vhost_vdpa_net_load_mac and vhost_vdpa_net_load_mq access to guest_features directly too, so I think we should change all of them at once. Thanks! > > + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { > > + return 0; > > + } > > + > > + offloads = cpu_to_le64(n->curr_guest_offloads); > > + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, > > + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, > > + &offloads, sizeof(offloads)); > > + if (unlikely(dev_written < 0)) { > > + return dev_written; > > + } > > + > > + return *s->status != VIRTIO_NET_OK; > > +} > > + > > static int vhost_vdpa_net_load(NetClientState *nc) > > { > > VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); > > @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) > > if (unlikely(r)) { > > return r; > > } > > + r = vhost_vdpa_net_load_offloads(s, n); > > + if (unlikely(r)) { > > + return r; > > + } > > > > return 0; > > } > > -- > > 2.25.1 > > >
On 2023/5/30 0:19, Eugenio Perez Martin wrote: > On Mon, May 29, 2023 at 3:18 PM Hawkins Jiawei <yin31149@gmail.com> wrote: >> >> This patch introduces vhost_vdpa_net_load_offloads() to >> restore offloads state at device's startup. >> >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> >> --- >> net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c >> index 37cdc84562..682c749b19 100644 >> --- a/net/vhost-vdpa.c >> +++ b/net/vhost-vdpa.c >> @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, >> return *s->status != VIRTIO_NET_OK; >> } >> >> +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, >> + const VirtIONet *n) >> +{ >> + uint64_t features, offloads; >> + ssize_t dev_written; >> + >> + features = n->parent_obj.guest_features; >> + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { >> + return 0; >> + } >> + > > Maybe we can avoid sending this CVQ command if the guest already uses > the default values? Hi Eugenio, Thanks for the review. However, I'm curious why we don't need to send this CVQ command if the guest is using the default values. Is it because the device automatically applies these default offloads, when the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated and QEMU doesn't send the CVQ command? Thanks! > > By default all features are enabled if I'm not wrong. I think the best > way is to expose virtio_net_supported_guest_offloads or > virtio_net_guest_offloads_by_features and then check if > n->curr_guest_offloads is the same. > > We should do the same with vhost_vdpa_net_load_mq, but that is out of > the scope of this series. > > Thanks! > >> + offloads = cpu_to_le64(n->curr_guest_offloads); >> + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, >> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, >> + &offloads, sizeof(offloads)); >> + if (unlikely(dev_written < 0)) { >> + return dev_written; >> + } >> + >> + return *s->status != VIRTIO_NET_OK; >> +} >> + >> static int vhost_vdpa_net_load(NetClientState *nc) >> { >> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); >> @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) >> if (unlikely(r)) { >> return r; >> } >> + r = vhost_vdpa_net_load_offloads(s, n); >> + if (unlikely(r)) { >> + return r; >> + } >> >> return 0; >> } >> -- >> 2.25.1 >> >
On 2023/5/31 14:37, Eugenio Perez Martin wrote: > On Wed, May 31, 2023 at 3:47 AM Jason Wang <jasowang@redhat.com> wrote: >> >> On Mon, May 29, 2023 at 9:18 PM Hawkins Jiawei <yin31149@gmail.com> wrote: >>> >>> This patch introduces vhost_vdpa_net_load_offloads() to >>> restore offloads state at device's startup. >>> >>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> >>> --- >>> net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c >>> index 37cdc84562..682c749b19 100644 >>> --- a/net/vhost-vdpa.c >>> +++ b/net/vhost-vdpa.c >>> @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, >>> return *s->status != VIRTIO_NET_OK; >>> } >>> >>> +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, >>> + const VirtIONet *n) >>> +{ >>> + uint64_t features, offloads; >>> + ssize_t dev_written; >>> + >>> + features = n->parent_obj.guest_features; >> >> Any reason you need to do tricks like this instead of using >> virtio_xxx_has_features()? >> > > It can be replaced by virtio_vdev_has_feature, yes. > > Current code of vhost_vdpa_net_load_mac and vhost_vdpa_net_load_mq > access to guest_features directly too, so I think we should change all > of them at once. Yes, I agree with you and Jason. I will refactor the patch as you and Jason have suggested. Thanks! > > Thanks! > >>> + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { >>> + return 0; >>> + } >>> + >>> + offloads = cpu_to_le64(n->curr_guest_offloads); >>> + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, >>> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, >>> + &offloads, sizeof(offloads)); >>> + if (unlikely(dev_written < 0)) { >>> + return dev_written; >>> + } >>> + >>> + return *s->status != VIRTIO_NET_OK; >>> +} >>> + >>> static int vhost_vdpa_net_load(NetClientState *nc) >>> { >>> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); >>> @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) >>> if (unlikely(r)) { >>> return r; >>> } >>> + r = vhost_vdpa_net_load_offloads(s, n); >>> + if (unlikely(r)) { >>> + return r; >>> + } >>> >>> return 0; >>> } >>> -- >>> 2.25.1 >>> >> >
On Wed, May 31, 2023 at 10:23 AM Hawkins Jiawei <yin31149@gmail.com> wrote: > > On 2023/5/30 0:19, Eugenio Perez Martin wrote: > > On Mon, May 29, 2023 at 3:18 PM Hawkins Jiawei <yin31149@gmail.com> wrote: > >> > >> This patch introduces vhost_vdpa_net_load_offloads() to > >> restore offloads state at device's startup. > >> > >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > >> --- > >> net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ > >> 1 file changed, 26 insertions(+) > >> > >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > >> index 37cdc84562..682c749b19 100644 > >> --- a/net/vhost-vdpa.c > >> +++ b/net/vhost-vdpa.c > >> @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, > >> return *s->status != VIRTIO_NET_OK; > >> } > >> > >> +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, > >> + const VirtIONet *n) > >> +{ > >> + uint64_t features, offloads; > >> + ssize_t dev_written; > >> + > >> + features = n->parent_obj.guest_features; > >> + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { > >> + return 0; > >> + } > >> + > > > > Maybe we can avoid sending this CVQ command if the guest already uses > > the default values? > > Hi Eugenio, > > Thanks for the review. However, I'm curious why we don't need to send > this CVQ command if the guest is using the default values. Is it because > the device automatically applies these default offloads, when the > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated and QEMU doesn't > send the CVQ command? > Exactly. You can check that either by the standard text or (sometimes easier) qemu virtio or virtio-net device code. The standard text says that: "Upon feature negotiation corresponding offload gets enabled to preserve backward compatibility." And you can check in the qemu code by hw/net/virtio-net:virtio_net_set_features(vdev, features), this chunk of code: n->curr_guest_offloads = virtio_net_guest_offloads_by_features(features); virtio_net_apply_guest_offloads(n); Thanks! > Thanks! > > > > > > By default all features are enabled if I'm not wrong. I think the best > > way is to expose virtio_net_supported_guest_offloads or > > virtio_net_guest_offloads_by_features and then check if > > n->curr_guest_offloads is the same. > > > > We should do the same with vhost_vdpa_net_load_mq, but that is out of > > the scope of this series. > > > > Thanks! > > > >> + offloads = cpu_to_le64(n->curr_guest_offloads); > >> + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, > >> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, > >> + &offloads, sizeof(offloads)); > >> + if (unlikely(dev_written < 0)) { > >> + return dev_written; > >> + } > >> + > >> + return *s->status != VIRTIO_NET_OK; > >> +} > >> + > >> static int vhost_vdpa_net_load(NetClientState *nc) > >> { > >> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); > >> @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) > >> if (unlikely(r)) { > >> return r; > >> } > >> + r = vhost_vdpa_net_load_offloads(s, n); > >> + if (unlikely(r)) { > >> + return r; > >> + } > >> > >> return 0; > >> } > >> -- > >> 2.25.1 > >> > > >
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 37cdc84562..682c749b19 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -680,6 +680,28 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s, return *s->status != VIRTIO_NET_OK; } +static int vhost_vdpa_net_load_offloads(VhostVDPAState *s, + const VirtIONet *n) +{ + uint64_t features, offloads; + ssize_t dev_written; + + features = n->parent_obj.guest_features; + if (!(features & BIT_ULL(VIRTIO_NET_F_CTRL_GUEST_OFFLOADS))) { + return 0; + } + + offloads = cpu_to_le64(n->curr_guest_offloads); + dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS, + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, + &offloads, sizeof(offloads)); + if (unlikely(dev_written < 0)) { + return dev_written; + } + + return *s->status != VIRTIO_NET_OK; +} + static int vhost_vdpa_net_load(NetClientState *nc) { VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc); @@ -702,6 +724,10 @@ static int vhost_vdpa_net_load(NetClientState *nc) if (unlikely(r)) { return r; } + r = vhost_vdpa_net_load_offloads(s, n); + if (unlikely(r)) { + return r; + } return 0; }
This patch introduces vhost_vdpa_net_load_offloads() to restore offloads state at device's startup. Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> --- net/vhost-vdpa.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)