| Message ID | e185a03168d0ab68150d21770bf0d02c5ef537c2.1515041373.git.jbaron@akamai.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: > The ability to set speed and duplex for virtio_net is useful in various > scenarios as described here: > > 16032be virtio_net: add ethtool support for set and get of settings > > However, it would be nice to be able to set this from the hypervisor, > such that virtio_net doesn't require custom guest ethtool commands. > > Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows > the hypervisor to export a linkspeed and duplex setting. The user can > subsequently overwrite it later if desired via: 'ethtool -s'. > > Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention > is that device feature bits are to grow down from bit 63, since the > transports are starting from bit 24 and growing up. > > Signed-off-by: Jason Baron <jbaron@akamai.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: virtio-dev@lists.oasis-open.org > --- > drivers/net/virtio_net.c | 19 ++++++++++++++++++- > include/uapi/linux/virtio_net.h | 13 +++++++++++++ > 2 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 6fb7b65..0b2d314 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) > > vi->status = v; > > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { > + u32 speed; > + u8 duplex; > + > + speed = virtio_cread32(vi->vdev, > + offsetof(struct virtio_net_config, > + speed)); > + if (ethtool_validate_speed(speed)) > + vi->speed = speed; > + duplex = virtio_cread8(vi->vdev, > + offsetof(struct virtio_net_config, > + duplex)); > + if (ethtool_validate_duplex(duplex)) > + vi->duplex = duplex; > + } > + > if (vi->status & VIRTIO_NET_S_LINK_UP) { > netif_carrier_on(vi->dev); > netif_tx_wake_all_queues(vi->dev); > @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { > VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ > VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ > VIRTIO_NET_F_CTRL_MAC_ADDR, \ > - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ > + VIRTIO_NET_F_SPEED_DUPLEX > > static unsigned int features[] = { > VIRTNET_FEATURES, Still missing the update from virtnet_config_changed_work, and I think it's important to reflex host changes within guest when the feature bit has been acked. > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > index fc353b5..5de6ed3 100644 > --- a/include/uapi/linux/virtio_net.h > +++ b/include/uapi/linux/virtio_net.h > @@ -57,6 +57,8 @@ > * Steering */ > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ > > +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ > + > #ifndef VIRTIO_NET_NO_LEGACY > #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ > #endif /* VIRTIO_NET_NO_LEGACY */ > @@ -76,6 +78,17 @@ struct virtio_net_config { > __u16 max_virtqueue_pairs; > /* Default maximum transmit unit advice */ > __u16 mtu; > + /* > + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. > + * Any other value stands for unknown. > + */ > + __u32 speed; > + /* > + * 0x00 - half duplex > + * 0x01 - full duplex > + * Any other value stands for unknown. > + */ > + __u8 duplex; > } __attribute__((packed)); > > /* > -- > 2.6.1
On 01/04/2018 11:27 AM, Michael S. Tsirkin wrote: > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: >> The ability to set speed and duplex for virtio_net is useful in various >> scenarios as described here: >> >> 16032be virtio_net: add ethtool support for set and get of settings >> >> However, it would be nice to be able to set this from the hypervisor, >> such that virtio_net doesn't require custom guest ethtool commands. >> >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows >> the hypervisor to export a linkspeed and duplex setting. The user can >> subsequently overwrite it later if desired via: 'ethtool -s'. >> >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention >> is that device feature bits are to grow down from bit 63, since the >> transports are starting from bit 24 and growing up. >> >> Signed-off-by: Jason Baron <jbaron@akamai.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: virtio-dev@lists.oasis-open.org >> --- >> drivers/net/virtio_net.c | 19 ++++++++++++++++++- >> include/uapi/linux/virtio_net.h | 13 +++++++++++++ >> 2 files changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 6fb7b65..0b2d314 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) >> >> vi->status = v; >> >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { >> + u32 speed; >> + u8 duplex; >> + >> + speed = virtio_cread32(vi->vdev, >> + offsetof(struct virtio_net_config, >> + speed)); >> + if (ethtool_validate_speed(speed)) >> + vi->speed = speed; >> + duplex = virtio_cread8(vi->vdev, >> + offsetof(struct virtio_net_config, >> + duplex)); >> + if (ethtool_validate_duplex(duplex)) >> + vi->duplex = duplex; >> + } >> + >> if (vi->status & VIRTIO_NET_S_LINK_UP) { >> netif_carrier_on(vi->dev); >> netif_tx_wake_all_queues(vi->dev); >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ >> VIRTIO_NET_F_CTRL_MAC_ADDR, \ >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ >> + VIRTIO_NET_F_SPEED_DUPLEX >> >> static unsigned int features[] = { >> VIRTNET_FEATURES, > > Still missing the update from virtnet_config_changed_work, and I think > it's important to reflex host changes within guest when the > feature bit has been acked. > I update vi->speed and vi->duplex in virtnet_config_changed_work(). And I tested using the 'set_link' on/off from the qemu monitor. Specifically, an 'off' sets the speed and link to 'unknown', and an 'on' returns the speed and link to the configured speed and duplex. So they are being updated dynamically now. What host changes are you referring to that are not reflected? Thanks, -Jason >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h >> index fc353b5..5de6ed3 100644 >> --- a/include/uapi/linux/virtio_net.h >> +++ b/include/uapi/linux/virtio_net.h >> @@ -57,6 +57,8 @@ >> * Steering */ >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ >> >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ >> + >> #ifndef VIRTIO_NET_NO_LEGACY >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ >> #endif /* VIRTIO_NET_NO_LEGACY */ >> @@ -76,6 +78,17 @@ struct virtio_net_config { >> __u16 max_virtqueue_pairs; >> /* Default maximum transmit unit advice */ >> __u16 mtu; >> + /* >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. >> + * Any other value stands for unknown. >> + */ >> + __u32 speed; >> + /* >> + * 0x00 - half duplex >> + * 0x01 - full duplex >> + * Any other value stands for unknown. >> + */ >> + __u8 duplex; >> } __attribute__((packed)); >> >> /* >> -- >> 2.6.1
On Thu, Jan 04, 2018 at 11:57:44AM -0500, Jason Baron wrote: > > > On 01/04/2018 11:27 AM, Michael S. Tsirkin wrote: > > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: > >> The ability to set speed and duplex for virtio_net is useful in various > >> scenarios as described here: > >> > >> 16032be virtio_net: add ethtool support for set and get of settings > >> > >> However, it would be nice to be able to set this from the hypervisor, > >> such that virtio_net doesn't require custom guest ethtool commands. > >> > >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows > >> the hypervisor to export a linkspeed and duplex setting. The user can > >> subsequently overwrite it later if desired via: 'ethtool -s'. > >> > >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention > >> is that device feature bits are to grow down from bit 63, since the > >> transports are starting from bit 24 and growing up. > >> > >> Signed-off-by: Jason Baron <jbaron@akamai.com> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >> Cc: Jason Wang <jasowang@redhat.com> > >> Cc: virtio-dev@lists.oasis-open.org > >> --- > >> drivers/net/virtio_net.c | 19 ++++++++++++++++++- > >> include/uapi/linux/virtio_net.h | 13 +++++++++++++ > >> 2 files changed, 31 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > >> index 6fb7b65..0b2d314 100644 > >> --- a/drivers/net/virtio_net.c > >> +++ b/drivers/net/virtio_net.c > >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) > >> > >> vi->status = v; > >> > >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { > >> + u32 speed; > >> + u8 duplex; > >> + > >> + speed = virtio_cread32(vi->vdev, > >> + offsetof(struct virtio_net_config, > >> + speed)); > >> + if (ethtool_validate_speed(speed)) > >> + vi->speed = speed; > >> + duplex = virtio_cread8(vi->vdev, > >> + offsetof(struct virtio_net_config, > >> + duplex)); > >> + if (ethtool_validate_duplex(duplex)) > >> + vi->duplex = duplex; > >> + } > >> + > >> if (vi->status & VIRTIO_NET_S_LINK_UP) { > >> netif_carrier_on(vi->dev); > >> netif_tx_wake_all_queues(vi->dev); > >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { > >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ > >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ > >> VIRTIO_NET_F_CTRL_MAC_ADDR, \ > >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ > >> + VIRTIO_NET_F_SPEED_DUPLEX > >> > >> static unsigned int features[] = { > >> VIRTNET_FEATURES, > > > > Still missing the update from virtnet_config_changed_work, and I think > > it's important to reflex host changes within guest when the > > feature bit has been acked. > > > > I update vi->speed and vi->duplex in virtnet_config_changed_work(). And > I tested using the 'set_link' on/off from the qemu monitor. > Specifically, an 'off' sets the speed and link to 'unknown', and an 'on' > returns the speed and link to the configured speed and duplex. So they > are being updated dynamically now. What host changes are you referring > to that are not reflected? > > Thanks, > > -Jason Ouch, I was reviewing an old version and replied to this one. Sorry, will re-read now. > > >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > >> index fc353b5..5de6ed3 100644 > >> --- a/include/uapi/linux/virtio_net.h > >> +++ b/include/uapi/linux/virtio_net.h > >> @@ -57,6 +57,8 @@ > >> * Steering */ > >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ > >> > >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ > >> + > >> #ifndef VIRTIO_NET_NO_LEGACY > >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ > >> #endif /* VIRTIO_NET_NO_LEGACY */ > >> @@ -76,6 +78,17 @@ struct virtio_net_config { > >> __u16 max_virtqueue_pairs; > >> /* Default maximum transmit unit advice */ > >> __u16 mtu; > >> + /* > >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. > >> + * Any other value stands for unknown. > >> + */ > >> + __u32 speed; > >> + /* > >> + * 0x00 - half duplex > >> + * 0x01 - full duplex > >> + * Any other value stands for unknown. > >> + */ > >> + __u8 duplex; > >> } __attribute__((packed)); > >> > >> /* > >> -- > >> 2.6.1
On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: > The ability to set speed and duplex for virtio_net is useful in various > scenarios as described here: > > 16032be virtio_net: add ethtool support for set and get of settings > > However, it would be nice to be able to set this from the hypervisor, > such that virtio_net doesn't require custom guest ethtool commands. > > Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows > the hypervisor to export a linkspeed and duplex setting. The user can > subsequently overwrite it later if desired via: 'ethtool -s'. > > Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention > is that device feature bits are to grow down from bit 63, since the > transports are starting from bit 24 and growing up. > > Signed-off-by: Jason Baron <jbaron@akamai.com> > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Jason Wang <jasowang@redhat.com> > Cc: virtio-dev@lists.oasis-open.org > --- > drivers/net/virtio_net.c | 19 ++++++++++++++++++- > include/uapi/linux/virtio_net.h | 13 +++++++++++++ > 2 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 6fb7b65..0b2d314 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) > > vi->status = v; > > + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { BTW we can avoid this read for when link goes down. Not a big deal but still. > + u32 speed; > + u8 duplex; > + > + speed = virtio_cread32(vi->vdev, > + offsetof(struct virtio_net_config, > + speed)); > + if (ethtool_validate_speed(speed)) > + vi->speed = speed; > + duplex = virtio_cread8(vi->vdev, > + offsetof(struct virtio_net_config, > + duplex)); > + if (ethtool_validate_duplex(duplex)) > + vi->duplex = duplex; > + } > + > if (vi->status & VIRTIO_NET_S_LINK_UP) { > netif_carrier_on(vi->dev); > netif_tx_wake_all_queues(vi->dev); OK so this handles the case when VIRTIO_NET_F_STATUS is set, but when it's clear we need to call this from virtnet_probe. I propose moving this chunk to a function and calling from two places. > @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { > VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ > VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ > VIRTIO_NET_F_CTRL_MAC_ADDR, \ > - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ > + VIRTIO_NET_F_SPEED_DUPLEX > > static unsigned int features[] = { > VIRTNET_FEATURES, > diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > index fc353b5..5de6ed3 100644 > --- a/include/uapi/linux/virtio_net.h > +++ b/include/uapi/linux/virtio_net.h > @@ -57,6 +57,8 @@ > * Steering */ > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ > > +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ > + > #ifndef VIRTIO_NET_NO_LEGACY > #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ > #endif /* VIRTIO_NET_NO_LEGACY */ > @@ -76,6 +78,17 @@ struct virtio_net_config { > __u16 max_virtqueue_pairs; > /* Default maximum transmit unit advice */ > __u16 mtu; > + /* > + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. > + * Any other value stands for unknown. > + */ > + __u32 speed; > + /* > + * 0x00 - half duplex > + * 0x01 - full duplex > + * Any other value stands for unknown. > + */ > + __u8 duplex; > } __attribute__((packed)); > > /* > -- > 2.6.1
On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote: > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: >> The ability to set speed and duplex for virtio_net is useful in various >> scenarios as described here: >> >> 16032be virtio_net: add ethtool support for set and get of settings >> >> However, it would be nice to be able to set this from the hypervisor, >> such that virtio_net doesn't require custom guest ethtool commands. >> >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows >> the hypervisor to export a linkspeed and duplex setting. The user can >> subsequently overwrite it later if desired via: 'ethtool -s'. >> >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention >> is that device feature bits are to grow down from bit 63, since the >> transports are starting from bit 24 and growing up. >> >> Signed-off-by: Jason Baron <jbaron@akamai.com> >> Cc: "Michael S. Tsirkin" <mst@redhat.com> >> Cc: Jason Wang <jasowang@redhat.com> >> Cc: virtio-dev@lists.oasis-open.org >> --- >> drivers/net/virtio_net.c | 19 ++++++++++++++++++- >> include/uapi/linux/virtio_net.h | 13 +++++++++++++ >> 2 files changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >> index 6fb7b65..0b2d314 100644 >> --- a/drivers/net/virtio_net.c >> +++ b/drivers/net/virtio_net.c >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) >> >> vi->status = v; >> >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { > > BTW we can avoid this read for when link goes down. > Not a big deal but still. So you are saying that we can just set vi->speed and vi->duplex to 'unknown' when the link goes down and not check for the presence of VIRTIO_NET_F_SPEED_DUPLEX? If so, that could over-write what the user may have configured in the guest via 'ethtool -s' when the link goes down, so that would be a change in behavior, but perhaps that is ok? I think I would prefer to have the link down event still check for VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we still have 2 modes for updating the fields: 1) completely guest controlled. Same as we have now and host does not change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence don't remove above check). 2) if speed or duplex or speed is set in the qemu command line, then set the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of speed/duplex (with ability of guest to over-write if it wanted to). > >> + u32 speed; >> + u8 duplex; >> + >> + speed = virtio_cread32(vi->vdev, >> + offsetof(struct virtio_net_config, >> + speed)); >> + if (ethtool_validate_speed(speed)) >> + vi->speed = speed; >> + duplex = virtio_cread8(vi->vdev, >> + offsetof(struct virtio_net_config, >> + duplex)); >> + if (ethtool_validate_duplex(duplex)) >> + vi->duplex = duplex; >> + } >> + >> if (vi->status & VIRTIO_NET_S_LINK_UP) { >> netif_carrier_on(vi->dev); >> netif_tx_wake_all_queues(vi->dev); > > OK so this handles the case when VIRTIO_NET_F_STATUS is set, > but when it's clear we need to call this from virtnet_probe. > > I propose moving this chunk to a function and calling from two places. > good point. will update. Thanks, -Jason > >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ >> VIRTIO_NET_F_CTRL_MAC_ADDR, \ >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ >> + VIRTIO_NET_F_SPEED_DUPLEX >> >> static unsigned int features[] = { >> VIRTNET_FEATURES, >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h >> index fc353b5..5de6ed3 100644 >> --- a/include/uapi/linux/virtio_net.h >> +++ b/include/uapi/linux/virtio_net.h >> @@ -57,6 +57,8 @@ >> * Steering */ >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ >> >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ >> + >> #ifndef VIRTIO_NET_NO_LEGACY >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ >> #endif /* VIRTIO_NET_NO_LEGACY */ >> @@ -76,6 +78,17 @@ struct virtio_net_config { >> __u16 max_virtqueue_pairs; >> /* Default maximum transmit unit advice */ >> __u16 mtu; >> + /* >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. >> + * Any other value stands for unknown. >> + */ >> + __u32 speed; >> + /* >> + * 0x00 - half duplex >> + * 0x01 - full duplex >> + * Any other value stands for unknown. >> + */ >> + __u8 duplex; >> } __attribute__((packed)); >> >> /* >> -- >> 2.6.1
On Thu, Jan 04, 2018 at 01:12:30PM -0500, Jason Baron wrote: > > > On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote: > > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: > >> The ability to set speed and duplex for virtio_net is useful in various > >> scenarios as described here: > >> > >> 16032be virtio_net: add ethtool support for set and get of settings > >> > >> However, it would be nice to be able to set this from the hypervisor, > >> such that virtio_net doesn't require custom guest ethtool commands. > >> > >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows > >> the hypervisor to export a linkspeed and duplex setting. The user can > >> subsequently overwrite it later if desired via: 'ethtool -s'. > >> > >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention > >> is that device feature bits are to grow down from bit 63, since the > >> transports are starting from bit 24 and growing up. > >> > >> Signed-off-by: Jason Baron <jbaron@akamai.com> > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > >> Cc: Jason Wang <jasowang@redhat.com> > >> Cc: virtio-dev@lists.oasis-open.org > >> --- > >> drivers/net/virtio_net.c | 19 ++++++++++++++++++- > >> include/uapi/linux/virtio_net.h | 13 +++++++++++++ > >> 2 files changed, 31 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > >> index 6fb7b65..0b2d314 100644 > >> --- a/drivers/net/virtio_net.c > >> +++ b/drivers/net/virtio_net.c > >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) > >> > >> vi->status = v; > >> > >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { > > > > BTW we can avoid this read for when link goes down. > > Not a big deal but still. > > So you are saying that we can just set vi->speed and vi->duplex to > 'unknown' when the link goes down and not check for the presence of > VIRTIO_NET_F_SPEED_DUPLEX? > > If so, that could over-write what the user may have configured in the > guest via 'ethtool -s' when the link goes down, so that would be a > change in behavior, but perhaps that is ok? No - what I am saying is that your patch overwrites the values set by user when link goes down. I suggest limiting this call to when if (vi->status & VIRTIO_NET_S_LINK_UP) and then the values are overwritten when link goes up which seems closer to what a user might expect. > > I think I would prefer to have the link down event still check for > VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we > still have 2 modes for updating the fields: > > 1) completely guest controlled. Same as we have now and host does not > change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence > don't remove above check). > > 2) if speed or duplex or speed is set in the qemu command line, then set > the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of > speed/duplex (with ability of guest to over-write if it wanted to). > > > > > >> + u32 speed; > >> + u8 duplex; > >> + > >> + speed = virtio_cread32(vi->vdev, > >> + offsetof(struct virtio_net_config, > >> + speed)); > >> + if (ethtool_validate_speed(speed)) > >> + vi->speed = speed; > >> + duplex = virtio_cread8(vi->vdev, > >> + offsetof(struct virtio_net_config, > >> + duplex)); > >> + if (ethtool_validate_duplex(duplex)) > >> + vi->duplex = duplex; > >> + } > >> + > >> if (vi->status & VIRTIO_NET_S_LINK_UP) { > >> netif_carrier_on(vi->dev); > >> netif_tx_wake_all_queues(vi->dev); > > > > OK so this handles the case when VIRTIO_NET_F_STATUS is set, > > but when it's clear we need to call this from virtnet_probe. > > > > I propose moving this chunk to a function and calling from two places. > > > > good point. will update. > > Thanks, > > -Jason > > > > >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { > >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ > >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ > >> VIRTIO_NET_F_CTRL_MAC_ADDR, \ > >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ > >> + VIRTIO_NET_F_SPEED_DUPLEX > >> > >> static unsigned int features[] = { > >> VIRTNET_FEATURES, > >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > >> index fc353b5..5de6ed3 100644 > >> --- a/include/uapi/linux/virtio_net.h > >> +++ b/include/uapi/linux/virtio_net.h > >> @@ -57,6 +57,8 @@ > >> * Steering */ > >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ > >> > >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ > >> + > >> #ifndef VIRTIO_NET_NO_LEGACY > >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ > >> #endif /* VIRTIO_NET_NO_LEGACY */ > >> @@ -76,6 +78,17 @@ struct virtio_net_config { > >> __u16 max_virtqueue_pairs; > >> /* Default maximum transmit unit advice */ > >> __u16 mtu; > >> + /* > >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. > >> + * Any other value stands for unknown. > >> + */ > >> + __u32 speed; > >> + /* > >> + * 0x00 - half duplex > >> + * 0x01 - full duplex > >> + * Any other value stands for unknown. > >> + */ > >> + __u8 duplex; > >> } __attribute__((packed)); > >> > >> /* > >> -- > >> 2.6.1
On Thu, Jan 04, 2018 at 08:22:08PM +0200, Michael S. Tsirkin wrote: > On Thu, Jan 04, 2018 at 01:12:30PM -0500, Jason Baron wrote: > > > > > > On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote: > > > On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: > > >> The ability to set speed and duplex for virtio_net is useful in various > > >> scenarios as described here: > > >> > > >> 16032be virtio_net: add ethtool support for set and get of settings > > >> > > >> However, it would be nice to be able to set this from the hypervisor, > > >> such that virtio_net doesn't require custom guest ethtool commands. > > >> > > >> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows > > >> the hypervisor to export a linkspeed and duplex setting. The user can > > >> subsequently overwrite it later if desired via: 'ethtool -s'. > > >> > > >> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention > > >> is that device feature bits are to grow down from bit 63, since the > > >> transports are starting from bit 24 and growing up. > > >> > > >> Signed-off-by: Jason Baron <jbaron@akamai.com> > > >> Cc: "Michael S. Tsirkin" <mst@redhat.com> > > >> Cc: Jason Wang <jasowang@redhat.com> > > >> Cc: virtio-dev@lists.oasis-open.org > > >> --- > > >> drivers/net/virtio_net.c | 19 ++++++++++++++++++- > > >> include/uapi/linux/virtio_net.h | 13 +++++++++++++ > > >> 2 files changed, 31 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > >> index 6fb7b65..0b2d314 100644 > > >> --- a/drivers/net/virtio_net.c > > >> +++ b/drivers/net/virtio_net.c > > >> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) > > >> > > >> vi->status = v; > > >> > > >> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { > > > > > > BTW we can avoid this read for when link goes down. > > > Not a big deal but still. > > > > So you are saying that we can just set vi->speed and vi->duplex to > > 'unknown' when the link goes down and not check for the presence of > > VIRTIO_NET_F_SPEED_DUPLEX? > > > > If so, that could over-write what the user may have configured in the > > guest via 'ethtool -s' when the link goes down, so that would be a > > change in behavior, but perhaps that is ok? > > No - what I am saying is that your patch overwrites the values > set by user when link goes down. > > I suggest limiting this call to when > > if (vi->status & VIRTIO_NET_S_LINK_UP) > > and then the values are overwritten when link goes up > which seems closer to what a user might expect. > > > > > I think I would prefer to have the link down event still check for > > VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we > > still have 2 modes for updating the fields: > > > > 1) completely guest controlled. Same as we have now and host does not > > change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence > > don't remove above check). > > > > 2) if speed or duplex or speed is set in the qemu command line, then set > > the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of > > speed/duplex (with ability of guest to over-write if it wanted to). I agree - I don't see a reason to touch the speed/duplex values when VIRTIO_NET_F_SPEED_DUPLEX has not been negotiated. > > > > > > > > > >> + u32 speed; > > >> + u8 duplex; > > >> + > > >> + speed = virtio_cread32(vi->vdev, > > >> + offsetof(struct virtio_net_config, > > >> + speed)); > > >> + if (ethtool_validate_speed(speed)) > > >> + vi->speed = speed; > > >> + duplex = virtio_cread8(vi->vdev, > > >> + offsetof(struct virtio_net_config, > > >> + duplex)); > > >> + if (ethtool_validate_duplex(duplex)) > > >> + vi->duplex = duplex; > > >> + } > > >> + > > >> if (vi->status & VIRTIO_NET_S_LINK_UP) { > > >> netif_carrier_on(vi->dev); > > >> netif_tx_wake_all_queues(vi->dev); > > > > > > OK so this handles the case when VIRTIO_NET_F_STATUS is set, > > > but when it's clear we need to call this from virtnet_probe. > > > > > > I propose moving this chunk to a function and calling from two places. > > > > > > > good point. will update. > > > > Thanks, > > > > -Jason > > > > > > > >> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { > > >> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ > > >> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ > > >> VIRTIO_NET_F_CTRL_MAC_ADDR, \ > > >> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS > > >> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ > > >> + VIRTIO_NET_F_SPEED_DUPLEX > > >> > > >> static unsigned int features[] = { > > >> VIRTNET_FEATURES, > > >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h > > >> index fc353b5..5de6ed3 100644 > > >> --- a/include/uapi/linux/virtio_net.h > > >> +++ b/include/uapi/linux/virtio_net.h > > >> @@ -57,6 +57,8 @@ > > >> * Steering */ > > >> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ > > >> > > >> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ > > >> + > > >> #ifndef VIRTIO_NET_NO_LEGACY > > >> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ > > >> #endif /* VIRTIO_NET_NO_LEGACY */ > > >> @@ -76,6 +78,17 @@ struct virtio_net_config { > > >> __u16 max_virtqueue_pairs; > > >> /* Default maximum transmit unit advice */ > > >> __u16 mtu; > > >> + /* > > >> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. > > >> + * Any other value stands for unknown. > > >> + */ > > >> + __u32 speed; > > >> + /* > > >> + * 0x00 - half duplex > > >> + * 0x01 - full duplex > > >> + * Any other value stands for unknown. > > >> + */ > > >> + __u8 duplex; > > >> } __attribute__((packed)); > > >> > > >> /* > > >> -- > > >> 2.6.1
On 01/04/2018 01:22 PM, Michael S. Tsirkin wrote: > On Thu, Jan 04, 2018 at 01:12:30PM -0500, Jason Baron wrote: >> >> >> On 01/04/2018 12:05 PM, Michael S. Tsirkin wrote: >>> On Thu, Jan 04, 2018 at 12:16:44AM -0500, Jason Baron wrote: >>>> The ability to set speed and duplex for virtio_net is useful in various >>>> scenarios as described here: >>>> >>>> 16032be virtio_net: add ethtool support for set and get of settings >>>> >>>> However, it would be nice to be able to set this from the hypervisor, >>>> such that virtio_net doesn't require custom guest ethtool commands. >>>> >>>> Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows >>>> the hypervisor to export a linkspeed and duplex setting. The user can >>>> subsequently overwrite it later if desired via: 'ethtool -s'. >>>> >>>> Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention >>>> is that device feature bits are to grow down from bit 63, since the >>>> transports are starting from bit 24 and growing up. >>>> >>>> Signed-off-by: Jason Baron <jbaron@akamai.com> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>> Cc: Jason Wang <jasowang@redhat.com> >>>> Cc: virtio-dev@lists.oasis-open.org >>>> --- >>>> drivers/net/virtio_net.c | 19 ++++++++++++++++++- >>>> include/uapi/linux/virtio_net.h | 13 +++++++++++++ >>>> 2 files changed, 31 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>> index 6fb7b65..0b2d314 100644 >>>> --- a/drivers/net/virtio_net.c >>>> +++ b/drivers/net/virtio_net.c >>>> @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) >>>> >>>> vi->status = v; >>>> >>>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { >>> >>> BTW we can avoid this read for when link goes down. >>> Not a big deal but still. >> >> So you are saying that we can just set vi->speed and vi->duplex to >> 'unknown' when the link goes down and not check for the presence of >> VIRTIO_NET_F_SPEED_DUPLEX? >> >> If so, that could over-write what the user may have configured in the >> guest via 'ethtool -s' when the link goes down, so that would be a >> change in behavior, but perhaps that is ok? > > No - what I am saying is that your patch overwrites the values > set by user when link goes down. > > I suggest limiting this call to when > > if (vi->status & VIRTIO_NET_S_LINK_UP) > > and then the values are overwritten when link goes up > which seems closer to what a user might expect. ok, will update this for v4. Thanks, -Jason > >> >> I think I would prefer to have the link down event still check for >> VIRTIO_NET_F_SPEED_DUPLEX before changing speed/duplex. That way we >> still have 2 modes for updating the fields: >> >> 1) completely guest controlled. Same as we have now and host does not >> change any values and does not set VIRTIO_NET_F_SPEED_DUPLEX flag (hence >> don't remove above check). >> >> 2) if speed or duplex or speed is set in the qemu command line, then set >> the VIRTIO_NET_F_SPEED_DUPLEX and have host control the settings of >> speed/duplex (with ability of guest to over-write if it wanted to). >> >> >>> >>>> + u32 speed; >>>> + u8 duplex; >>>> + >>>> + speed = virtio_cread32(vi->vdev, >>>> + offsetof(struct virtio_net_config, >>>> + speed)); >>>> + if (ethtool_validate_speed(speed)) >>>> + vi->speed = speed; >>>> + duplex = virtio_cread8(vi->vdev, >>>> + offsetof(struct virtio_net_config, >>>> + duplex)); >>>> + if (ethtool_validate_duplex(duplex)) >>>> + vi->duplex = duplex; >>>> + } >>>> + >>>> if (vi->status & VIRTIO_NET_S_LINK_UP) { >>>> netif_carrier_on(vi->dev); >>>> netif_tx_wake_all_queues(vi->dev); >>> >>> OK so this handles the case when VIRTIO_NET_F_STATUS is set, >>> but when it's clear we need to call this from virtnet_probe. >>> >>> I propose moving this chunk to a function and calling from two places. >>> >> >> good point. will update. >> >> Thanks, >> >> -Jason >> >>> >>>> @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { >>>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ >>>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ >>>> VIRTIO_NET_F_CTRL_MAC_ADDR, \ >>>> - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS >>>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ >>>> + VIRTIO_NET_F_SPEED_DUPLEX >>>> >>>> static unsigned int features[] = { >>>> VIRTNET_FEATURES, >>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h >>>> index fc353b5..5de6ed3 100644 >>>> --- a/include/uapi/linux/virtio_net.h >>>> +++ b/include/uapi/linux/virtio_net.h >>>> @@ -57,6 +57,8 @@ >>>> * Steering */ >>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ >>>> >>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ >>>> + >>>> #ifndef VIRTIO_NET_NO_LEGACY >>>> #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ >>>> #endif /* VIRTIO_NET_NO_LEGACY */ >>>> @@ -76,6 +78,17 @@ struct virtio_net_config { >>>> __u16 max_virtqueue_pairs; >>>> /* Default maximum transmit unit advice */ >>>> __u16 mtu; >>>> + /* >>>> + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. >>>> + * Any other value stands for unknown. >>>> + */ >>>> + __u32 speed; >>>> + /* >>>> + * 0x00 - half duplex >>>> + * 0x01 - full duplex >>>> + * Any other value stands for unknown. >>>> + */ >>>> + __u8 duplex; >>>> } __attribute__((packed)); >>>> >>>> /* >>>> -- >>>> 2.6.1
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 6fb7b65..0b2d314 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -2146,6 +2146,22 @@ static void virtnet_config_changed_work(struct work_struct *work) vi->status = v; + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX)) { + u32 speed; + u8 duplex; + + speed = virtio_cread32(vi->vdev, + offsetof(struct virtio_net_config, + speed)); + if (ethtool_validate_speed(speed)) + vi->speed = speed; + duplex = virtio_cread8(vi->vdev, + offsetof(struct virtio_net_config, + duplex)); + if (ethtool_validate_duplex(duplex)) + vi->duplex = duplex; + } + if (vi->status & VIRTIO_NET_S_LINK_UP) { netif_carrier_on(vi->dev); netif_tx_wake_all_queues(vi->dev); @@ -2796,7 +2812,8 @@ static struct virtio_device_id id_table[] = { VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \ VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \ VIRTIO_NET_F_CTRL_MAC_ADDR, \ - VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \ + VIRTIO_NET_F_SPEED_DUPLEX static unsigned int features[] = { VIRTNET_FEATURES, diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h index fc353b5..5de6ed3 100644 --- a/include/uapi/linux/virtio_net.h +++ b/include/uapi/linux/virtio_net.h @@ -57,6 +57,8 @@ * Steering */ #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ +#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ + #ifndef VIRTIO_NET_NO_LEGACY #define VIRTIO_NET_F_GSO 6 /* Host handles pkts w/ any GSO type */ #endif /* VIRTIO_NET_NO_LEGACY */ @@ -76,6 +78,17 @@ struct virtio_net_config { __u16 max_virtqueue_pairs; /* Default maximum transmit unit advice */ __u16 mtu; + /* + * speed, in units of 1Mb. All values 0 to INT_MAX are legal. + * Any other value stands for unknown. + */ + __u32 speed; + /* + * 0x00 - half duplex + * 0x01 - full duplex + * Any other value stands for unknown. + */ + __u8 duplex; } __attribute__((packed)); /*
The ability to set speed and duplex for virtio_net is useful in various scenarios as described here: 16032be virtio_net: add ethtool support for set and get of settings However, it would be nice to be able to set this from the hypervisor, such that virtio_net doesn't require custom guest ethtool commands. Introduce a new feature flag, VIRTIO_NET_F_SPEED_DUPLEX, which allows the hypervisor to export a linkspeed and duplex setting. The user can subsequently overwrite it later if desired via: 'ethtool -s'. Note that VIRTIO_NET_F_SPEED_DUPLEX is defined as bit 63, the intention is that device feature bits are to grow down from bit 63, since the transports are starting from bit 24 and growing up. Signed-off-by: Jason Baron <jbaron@akamai.com> Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Jason Wang <jasowang@redhat.com> Cc: virtio-dev@lists.oasis-open.org --- drivers/net/virtio_net.c | 19 ++++++++++++++++++- include/uapi/linux/virtio_net.h | 13 +++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-)