| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180104202250-mutt-send-email-mst@kernel.org>
Date: Thu, 4 Jan 2018 20:23:15 +0200
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Baron <jbaron@...mai.com>
Cc: davem@...emloft.net, jasowang@...hat.com, netdev@...r.kernel.org,
virtualization@...ts.linux-foundation.org, qemu-devel@...gnu.org,
virtio-dev@...ts.oasis-open.org
Subject: Re: [PATCH net-next v3 1/3] virtio_net: propagate linkspeed/duplex
settings from the hypervisor
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@...mai.com>
> > >> Cc: "Michael S. Tsirkin" <mst@...hat.com>
> > >> Cc: Jason Wang <jasowang@...hat.com>
> > >> Cc: virtio-dev@...ts.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
Powered by blists - more mailing lists