* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-22 15:14 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
David Miller
In-Reply-To: <d9ef40a2-237b-0cce-4401-ecaeac4c602a@oracle.com>
On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
>
>
> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> >
> >
> > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > >
> > >
> > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > cleanly, see:
> > > > >
> > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > >
> > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > control (without getting netdev opened early by the other part of
> > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > which don't provides a solution if user care about consistent naming
> > > > > on the slave netdevs specifically.
> > > > >
> > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > Above says:
> > > >
> > > > there's no motivation in the systemd/udevd community at
> > > > this point to refactor the rename logic and make it work well with
> > > > 3-netdev.
> > > >
> > > > What would the fix be? Skip slave devices?
> > > >
> > > There's nothing user can get if just skipping slave devices - the
> > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > next reboot, while the rest may conform to the naming scheme (ens3
> > > and such). There's no way one can fix this in userspace alone - when
> > > the failover is created the enslaved netdev was opened by the kernel
> > > earlier than the userspace is made aware of, and there's no
> > > negotiation protocol for kernel to know when userspace has done
> > > initial renaming of the interface. I would expect netdev list should
> > > at least provide the direction in general for how this can be
> > > solved...
I was just wondering what did you mean when you said
"refactor the rename logic and make it work well with 3-netdev" -
was there a proposal udev rejected?
Anyway, can we write a time diagram for what happens in which order that
leads to failure? That would help look for triggers that we can tie
into, or add new ones.
> > >
> > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > to only work with the master failover device.
> Where does this expectation come from?
>
> Admin users may have ethtool or tc configurations that need to deal with
> predictable interface name. Third-party app which was built upon specifying
> certain interface name can't be modified to chase dynamic names.
>
> Specifically, we have pre-canned image that uses ethtool to fine tune VF
> offload settings post boot for specific workload. Those images won't work
> well if the name is constantly changing just after couple rounds of live
> migration.
It should be possible to specify the ethtool configuration on the
master and have it automatically propagated to the slave.
BTW this is something we should look at IMHO.
> > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> model as much transparent to a real NIC as possible, while a hidden netns is
> just the vehicle). However, I recall there was resistance around this
> discussion that even the concept of hiding itself is a taboo for Linux
> netdev. I would like to summon potential alternatives before concluding
> 1-netdev is the only solution too soon.
>
> Thanks,
> -Siwei
Your scripts would not work at all then, right?
> >
> > > -Siwei
> > >
> > >
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 15:23 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dgreid, virtualization, Jason Gunthorpe, linux-integrity,
Peter Huewe, apronin, David Tolnay
In-Reply-To: <20190222102610.GB5613@linux.intel.com>
On Fri, Feb 22, 2019 at 12:26:10PM +0200, Jarkko Sakkinen wrote:
> On Thu, Feb 21, 2019 at 06:14:02PM -0800, David Tolnay wrote:
> > Add a config TCG_VIRTIO_VTPM which enables a driver providing the guest
> > kernel side of TPM over virtio.
> >
> > Use case: TPM support is needed for performing trusted work from within
> > a virtual machine launched by Chrome OS.
> >
> > Tested inside crosvm, the Chrome OS virtual machine monitor. Crosvm's
> > implementation of the virtio TPM device can be found in these two source
> > files:
> >
> > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/devices/src/virtio/tpm.rs
> > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/tpm2/src/lib.rs
>
> These files/links do not make sense for kernel testing. Please remove
> them from the next version.
To clarify generally for a virtio device we want
- guest support
- device support
- spec
If the device is implemented in qemu and guest in linux kernel,
then there are lots of people familiar with these
programming environments, so sometimes we merge
guest and host code even if spec isn't written up at all.
If you don't want to do that there's a small number of people who can
properly review code, e.g. I don't think lots of people on this list are
familiar with crosvm. One way to address this would be to build a QEMU
implementation. Another would be to write up a spec. You can do both
too :)
> > and is currently backed by the libtpm2 TPM simulator:
> >
> > - https://chromium.googlesource.com/chromiumos/third_party/tpm2/
> >
> > Reviewed-on: https://chromium-review.googlesource.com/1387655
>
> A non-standard flag. Should be removed. Also
>
> > Reviewed-by: Andrey Pronin <apronin@chromium.org>
> > Tested-by: David Tolnay <dtolnay@gmail.com>
> > Signed-off-by: David Tolnay <dtolnay@gmail.com>
>
> Your SOB should first and you cannot peer test your own patches. Please
> remove tested-by.
>
> The whole thing looks like an early draft. Why the patch does not have
> an RFC tag? You should use it for early drafts. Now it is like saying
> "please merge this".
>
> I don't have much knowledge of virtio. The commit message should at
> least give rough overview what is meant by "kernel side" in this
> context.
>
> Since one cannot use standard Linux environment to test this I'm not too
> optimistic about this getting merged any time soon. And since even the
> commit message is broken I don't think it makes sense to review the code
> in detail at this point.
>
> /Jarkko
^ permalink raw reply
* Re: [PATCH] drm/bochs: Fix the ID mismatch error
From: Alistair Francis @ 2019-02-22 18:05 UTC (permalink / raw)
To: kraxel@redhat.com
Cc: david.abdurachmanov@gmail.com, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org,
virtualization@lists.linux-foundation.org, Alistair Francis,
linux-riscv@lists.infradead.org
In-Reply-To: <20190222053747.rhed6j6nyi6tmg4l@sirius.home.kraxel.org>
On Thu, Feb 21, 2019 at 9:37 PM kraxel@redhat.com <kraxel@redhat.com> wrote:
>
> On Thu, Feb 21, 2019 at 10:44:06AM -0800, Alistair Francis wrote:
> > On Thu, Feb 21, 2019 at 3:52 AM kraxel@redhat.com <kraxel@redhat.com> wrote:
> > >
> > > On Thu, Feb 21, 2019 at 12:33:03AM +0000, Alistair Francis wrote:
> > > > When running RISC-V QEMU with the Bochs device attached via PCIe the
> > > > probe of the Bochs device fails with:
> > > > [drm:bochs_hw_init] *ERROR* ID mismatch
> > > >
> > > > This was introduced by this commit:
> > > > 7780eb9ce8 bochs: convert to drm_dev_register
> > > >
> > > > To fix the error we ensure that pci_enable_device() is called before
> > > > bochs_load().
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > Reported-by: David Abdurachmanov <david.abdurachmanov@gmail.com>
> > >
> > > Pushed to drm-misc-fixes.
> >
> > Thanks. Any chance this will make it into 5.0?
>
> Hmm, we are damn close to the release, not sure there will be one more
> drm-fixes pull req. But I've added a proper Fixes: tag, so even if the
> patch misses the boat it should land in the stable branches shortly
> thereafter.
Landing in the stable branches is probably enough. If you do end up
sending another pull request it would be great if this gets in. It
would be nice to have this fixed in the official 5.0 tag.
Alistair
>
> cheers,
> Gerd
>
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 20:55 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dgreid, virtualization, Jason Gunthorpe, linux-integrity,
Peter Huewe, apronin, David Tolnay
In-Reply-To: <20190222193156.GA6475@linux.intel.com>
On Fri, Feb 22, 2019 at 09:31:56PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 22, 2019 at 10:23:02AM -0500, Michael S. Tsirkin wrote:
> > On Fri, Feb 22, 2019 at 12:26:10PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Feb 21, 2019 at 06:14:02PM -0800, David Tolnay wrote:
> > > > Add a config TCG_VIRTIO_VTPM which enables a driver providing the guest
> > > > kernel side of TPM over virtio.
> > > >
> > > > Use case: TPM support is needed for performing trusted work from within
> > > > a virtual machine launched by Chrome OS.
> > > >
> > > > Tested inside crosvm, the Chrome OS virtual machine monitor. Crosvm's
> > > > implementation of the virtio TPM device can be found in these two source
> > > > files:
> > > >
> > > > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/devices/src/virtio/tpm.rs
> > > > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/tpm2/src/lib.rs
> > >
> > > These files/links do not make sense for kernel testing. Please remove
> > > them from the next version.
> >
> > To clarify generally for a virtio device we want
> > - guest support
> > - device support
> > - spec
> >
> > If the device is implemented in qemu and guest in linux kernel,
> > then there are lots of people familiar with these
> > programming environments, so sometimes we merge
> > guest and host code even if spec isn't written up at all.
> >
> > If you don't want to do that there's a small number of people who can
> > properly review code, e.g. I don't think lots of people on this list are
> > familiar with crosvm. One way to address this would be to build a QEMU
> > implementation. Another would be to write up a spec. You can do both
> > too :)
>
> I don't really understand your arguments.
>
> /Jarkko
Jarkko, I am not making any argument at all.
I am trying to suggest ways for this driver to get upstream.
And I am saying that while I agree availability of an implementation in
crosvm isn't sufficient, availability of a software device
implementation in QEMU is not an absolute prerequisite for including a
virtio driver in Linux. If there's a virtio spec explaining how to
write one, that can be enough.
--
MST
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 21:16 UTC (permalink / raw)
To: James Bottomley
Cc: dgreid, Jarkko Sakkinen, virtualization, Jason Gunthorpe,
linux-integrity, Peter Huewe, apronin, David Tolnay
In-Reply-To: <1550849416.2787.5.camel@HansenPartnership.com>
On Fri, Feb 22, 2019 at 07:30:16AM -0800, James Bottomley wrote:
> On Thu, 2019-02-21 at 18:14 -0800, David Tolnay wrote:
> > Add a config TCG_VIRTIO_VTPM which enables a driver providing the
> > guest kernel side of TPM over virtio.
>
> What's the use case for using this over the current non-virtio vTPM?.
> I always thought virtio was about guest to host transport efficiency,
> but the phsical TPM, being connected over a very slow bus, is about as
> inefficient as you can get in that regard, so why do we need to use
> virtio to drive the virtual one?
I can't say for sure about TPM.
But generally there are many reasons to do virtio rather than emulating
a hardware device.
Ease of extending the device could be one. E.g. what if you want to make
an extension that hardware does not support? You are at cross-purposes
with a hardware vendor who can happen to be the driver maintainer as
well.
A decent specification and readiness to fix bugs in the right place
(e.g. driver violates spec? we'll fix driver not as you to work around
it in the device) is another.
You can also download the spec without clicking I agree once - and it
follows the Non-Assertion IPR Mode to help people not get sued.
Stuff like that is conductive to getting things done.
--
MST
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 21:25 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dgreid, virtualization, Jason Gunthorpe, linux-integrity,
Peter Huewe, apronin, David Tolnay
In-Reply-To: <20190222193305.GB6475@linux.intel.com>
On Fri, Feb 22, 2019 at 09:33:05PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 22, 2019 at 09:31:56PM +0200, Jarkko Sakkinen wrote:
> > On Fri, Feb 22, 2019 at 10:23:02AM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Feb 22, 2019 at 12:26:10PM +0200, Jarkko Sakkinen wrote:
> > > > On Thu, Feb 21, 2019 at 06:14:02PM -0800, David Tolnay wrote:
> > > > > Add a config TCG_VIRTIO_VTPM which enables a driver providing the guest
> > > > > kernel side of TPM over virtio.
> > > > >
> > > > > Use case: TPM support is needed for performing trusted work from within
> > > > > a virtual machine launched by Chrome OS.
> > > > >
> > > > > Tested inside crosvm, the Chrome OS virtual machine monitor. Crosvm's
> > > > > implementation of the virtio TPM device can be found in these two source
> > > > > files:
> > > > >
> > > > > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/devices/src/virtio/tpm.rs
> > > > > - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/tpm2/src/lib.rs
> > > >
> > > > These files/links do not make sense for kernel testing. Please remove
> > > > them from the next version.
> > >
> > > To clarify generally for a virtio device we want
> > > - guest support
> > > - device support
> > > - spec
> > >
> > > If the device is implemented in qemu and guest in linux kernel,
> > > then there are lots of people familiar with these
> > > programming environments, so sometimes we merge
> > > guest and host code even if spec isn't written up at all.
> > >
> > > If you don't want to do that there's a small number of people who can
> > > properly review code, e.g. I don't think lots of people on this list are
> > > familiar with crosvm. One way to address this would be to build a QEMU
> > > implementation. Another would be to write up a spec. You can do both
> > > too :)
> >
> > I don't really understand your arguments.
>
> ... and I did your response total three times and did not find any
> causality of any sort from anything.
>
> /Jarkko
Thanks for spending the time reading my response. What was included in
it was a general suggestion for a virtio based driver to be acceptable
in upstream Linux.
You pointed out that a pointer to a prototype implementation in Rust
isn't relevant. However, FYI just posting guest code and asking for it
to be merged alone won't work for a virtio driver either. I am merely
trying to speed things up instead of having the contributor repost with
a tweaked commit log just to immediately get another set of nacks.
--
MST
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 22:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dgreid, virtualization, James Bottomley, Jason Gunthorpe,
linux-integrity, Peter Huewe, apronin, David Tolnay
In-Reply-To: <20190222215923.GB21427@linux.intel.com>
On Fri, Feb 22, 2019 at 11:59:23PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 22, 2019 at 02:31:37PM -0700, Jason Gunthorpe wrote:
> > On Fri, Feb 22, 2019 at 04:16:01PM -0500, Michael S. Tsirkin wrote:
> > > On Fri, Feb 22, 2019 at 07:30:16AM -0800, James Bottomley wrote:
> > > > On Thu, 2019-02-21 at 18:14 -0800, David Tolnay wrote:
> > > > > Add a config TCG_VIRTIO_VTPM which enables a driver providing the
> > > > > guest kernel side of TPM over virtio.
> > > >
> > > > What's the use case for using this over the current non-virtio vTPM?.
> > > > I always thought virtio was about guest to host transport efficiency,
> > > > but the phsical TPM, being connected over a very slow bus, is about as
> > > > inefficient as you can get in that regard, so why do we need to use
> > > > virtio to drive the virtual one?
> > >
> > > I can't say for sure about TPM.
> > >
> > > But generally there are many reasons to do virtio rather than emulating
> > > a hardware device.
> >
> > We already have a xen 'virtioish' TPM driver, so I don't think there
> > is a good reason to block a virtio driver if someone cares about
> > it. There are enough good reasons to prefer virtio to other options,
> > IMHO.
> >
> > Provided it meets the general requirements for new virtio stuff you
> > outlined.
>
> Yeah, absolutely we can consider this.
>
> For me it boils down to testing and documentation part.
>
> No plans to merge code that I'm unable to run...
>
> /Jarkko
I do this sometimes. One can't require samples for all supported
hardware. If I can check that code matches spec, I might settle for
that.
--
MST
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 22:24 UTC (permalink / raw)
To: David Tolnay
Cc: dgreid, Jarkko Sakkinen, virtualization, Jason Gunthorpe,
Peter Huewe, apronin, linux-integrity
In-Reply-To: <461bd10a-0a30-81e3-63b4-0798eb75b9e7@gmail.com>
On Fri, Feb 22, 2019 at 01:40:25PM -0800, David Tolnay wrote:
> On 2/21/19 9:51 PM, Michael S. Tsirkin wrote:
> > On Thu, Feb 21, 2019 at 06:14:02PM -0800, David Tolnay wrote:
> >> Add a config TCG_VIRTIO_VTPM which enables a driver providing the guest
> >> kernel side of TPM over virtio.
> >>
> >> Use case: TPM support is needed for performing trusted work from within
> >> a virtual machine launched by Chrome OS.
> >>
> >> Tested inside crosvm, the Chrome OS virtual machine monitor. Crosvm's
> >> implementation of the virtio TPM device can be found in these two source
> >> files:
> >>
> >> - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/devices/src/virtio/tpm.rs
> >> - https://chromium.googlesource.com/chromiumos/platform/crosvm/+/18ce5713e6cb99c40aafec52b67c28ba12a44f31/tpm2/src/lib.rs
> >>
> >> and is currently backed by the libtpm2 TPM simulator:
> >>
> >> - https://chromium.googlesource.com/chromiumos/third_party/tpm2/
> >>
> >> Reviewed-on: https://chromium-review.googlesource.com/1387655
> >> Reviewed-by: Andrey Pronin <apronin@chromium.org>
> >> Tested-by: David Tolnay <dtolnay@gmail.com>
> >> Signed-off-by: David Tolnay <dtolnay@gmail.com>
> >> ---
> >> UNRESOLVED:
> >> The driver assumes virtio device number VIRTIO_ID_TPM=31. If there is
> >> interest in accepting a virtio TPM driver into the kernel, the Chrome OS
> >> team will coordinate with the OASIS virtio technical committee to secure
> >> an agreed upon device number and include it in a later patchset.
> >
> > I am not a tpm expert but I don't see why not.
> >
> >
> >> drivers/char/tpm/Kconfig | 9 +
> >> drivers/char/tpm/Makefile | 1 +
> >> drivers/char/tpm/tpm_virtio.c | 460 ++++++++++++++++++++++++++++++++++
> >
> > Maintainer entry?
>
> Good call, I will add one.
>
> Could you let me know what workflow would work best for you? I can direct
> patches toward Chrome OS's kernel tree and only a Chrome OS list, then send them
> your way only once they have been reviewed and accepted there. Or I can list one
> or both of the linux-integrity@v.k.o and virtualization@l.l-f.o lists along with
> a list for Chrome OS reviewers.
>
> Either way, we'll want eyes from people who know virtio and from people who know
> TPM on most changes.
Well if you are changing a host/guest interface then you also need to copy
virtio-dev. That one is subscriber only so that would
imply sending there after it's reviewed in chrome.
As an extra bonus reviewed code is hopefully higher quality
so less review work for me ;)
so the first option sounds like a better plan.
But I hope accepted above just means it goes on some branch,
such that we won't end up with production code that
is set in stone and needs to be maintained forever?
>
> >
> >> 3 files changed, 470 insertions(+)
> >> create mode 100644 drivers/char/tpm/tpm_virtio.c
> >>
> >> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> >> index 536e55d3919f..8997060e248e 100644
> >> --- a/drivers/char/tpm/Kconfig
> >> +++ b/drivers/char/tpm/Kconfig
> >> @@ -164,6 +164,15 @@ config TCG_VTPM_PROXY
> >> /dev/vtpmX and a server-side file descriptor on which the vTPM
> >> can receive commands.
> >>
> >> +config TCG_VIRTIO_VTPM
> >> + tristate "Virtio vTPM"
> >> + depends on TCG_TPM
> >> + help
> >> + This driver provides the guest kernel side of TPM over Virtio. If
> >> + you are building Linux to run inside of a hypervisor that supports
> >> + TPM over Virtio, say Yes and the virtualized TPM will be
> >> + accessible from the guest.
> >> +
> >>
> >> source "drivers/char/tpm/st33zp24/Kconfig"
> >> endif # TCG_TPM
> >> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> >> index a01c4cab902a..4f5d1071257a 100644
> >> --- a/drivers/char/tpm/Makefile
> >> +++ b/drivers/char/tpm/Makefile
> >> @@ -33,3 +33,4 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
> >> obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
> >> obj-$(CONFIG_TCG_CRB) += tpm_crb.o
> >> obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> >> +obj-$(CONFIG_TCG_VIRTIO_VTPM) += tpm_virtio.o
> >> diff --git a/drivers/char/tpm/tpm_virtio.c b/drivers/char/tpm/tpm_virtio.c
> >> new file mode 100644
> >> index 000000000000..f3239d983f18
> >> --- /dev/null
> >> +++ b/drivers/char/tpm/tpm_virtio.c
> >> @@ -0,0 +1,460 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright 2019 Google Inc.
> >> + *
> >> + * Author: David Tolnay <dtolnay@gmail.com>
> >> + *
> >> + * ---
> >> + *
> >> + * Device driver for TPM over virtio.
> >> + *
> >> + * This driver employs a single virtio queue to handle both send and recv. TPM
> >> + * commands are sent over virtio to the hypervisor during a TPM send operation
> >> + * and responses are received over the same queue during a recv operation.
> >> + *
> >> + * The driver contains a single buffer that is the only buffer we ever place on
> >> + * the virtio queue. Commands are copied from the caller's command buffer into
> >> + * the driver's buffer before handing off to virtio, and responses are received
> >> + * into the driver's buffer then copied into the caller's response buffer. This
> >> + * allows us to be resilient to timeouts. When a send or recv operation times
> >> + * out, the caller is free to destroy their buffer; we don't want the hypervisor
> >> + * continuing to perform reads or writes against that destroyed buffer.
> >> + *
> >> + * This driver does not support concurrent send and recv operations. Mutual
> >> + * exclusion is upheld by the tpm_mutex lock held in tpm-interface.c around the
> >> + * calls to chip->ops->send and chip->ops->recv.
> >> + *
> >> + * The intended hypervisor-side implementation is as follows.
> >> + *
> >> + * while true:
> >> + * await next virtio buffer.
> >> + * expect first descriptor in chain to be guest-to-host.
> >> + * read tpm command from that buffer.
> >> + * synchronously perform TPM work determined by the command.
> >> + * expect second descriptor in chain to be host-to-guest.
> >> + * write TPM response into that buffer.
> >> + * place buffer on virtio used queue indicating how many bytes written.
> >
> > That's fine I think except generally it should be legal for guest
> > to split each buffer to several segments.
>
> Acknowledged. I will adjust this comment.
>
> To clarify, this means the hypervisor will need to accept a single descriptor
> chain consisting of one or more guest-to-host descriptors which together form
> the command, followed by one or more host-to-guest descriptors into which the
> response may be written. Is that what you had in mind?
Exactly.
> TPM commands and responses are both capped at a moderate size (4096 bytes). With
> that in mind, is it still necessary to allow split buffers? We won't be dealing
> with multi-megabyte transfers.
This has been a general virtio rule, yes. See for example "2.6.4 Message
Framing" in v1.1 draft.
It's generally not hard to implement and generally easier than argue about
whether it's necessary. If you think it's a big problem, it's a good
idea to take it up with the virtio tc. Address this to virtio-comment
list.
>
> >> + */
> >> +
> >> +#include <linux/virtio_config.h>
> >> +
> >> +#include "tpm.h"
> >> +
> >> +/*
> >> + * Timeout duration when waiting on the hypervisor to complete its end of the
> >> + * TPM operation. This timeout is relatively high because certain TPM operations
> >> + * can take dozens of seconds.
> >> + */
> >> +#define TPM_VIRTIO_TIMEOUT (120 * HZ)
> >
> > Should we read this from device? E.g. a busy hypervisor might
> > take longer to respond ...
>
> That's true. Do you have an example of an existing virtio driver that reads
> timeout from the hypervisor?
>
> To understand your perspective, do you see one of the following as being the
> bigger advantage? -- either that we can use a timeout tighter than 120s for
> hypervisors that believe they can respond quickly to any command, or that we can
> relax the timeout beyond 120s for an especially overburdened hypervisor. The
> first is nice because the caller in the guest will receive their error code
> sooner in certain failure modes, but maybe that would be better addressed by
> some other way of poking the host to find out whether it is still alive and
> working. For the second, 120s is pretty generous and in our use case we would
> just want to give up and retry much later at that point; I don't know how much
> further it would be reasonable to grow a timeout.
I think the whole idea around timeout handling needs a bit more
thought. What kind of reasons for the timeout do you envision
that require the extra kicks?
>
> >> +
> >> +struct vtpm_device {
> >> + /*
> >> + * Data structure for integration with the common code of the TPM driver
> >> + * in tpm-chip.c.
> >> + */
> >> + struct tpm_chip *chip;
> >> +
> >> + /*
> >> + * Virtio queue for sending TPM commands out of the virtual machine and
> >> + * receiving TPM responses back from the hypervisor.
> >> + */
> >> + struct virtqueue *vq;
> >> +
> >> + /*
> >> + * Completion that is notified when a virtio operation has been
> >> + * fulfilled by the hypervisor.
> >> + */
> >> + struct completion complete;
> >> +
> >> + /*
> >> + * Whether driver currently holds ownership of the virtqueue buffer.
> >> + * When false, the hypervisor is in the process of reading or writing
> >> + * the buffer and the driver must not touch it.
> >> + */
> >> + bool driver_has_buffer;
> >> +
> >> + /*
> >> + * Whether during the most recent TPM operation, a virtqueue_kick failed
> >> + * or a wait timed out.
> >> + *
> >> + * The next send or recv operation will attempt a kick upon seeing this
> >> + * status. That should clear up the queue in the case that the
> >> + * hypervisor became temporarily nonresponsive, such as by resource
> >> + * exhaustion on the host. The extra kick enables recovery from kicks
> >> + * going unnoticed by the hypervisor as well as recovery from virtio
> >> + * callbacks going unnoticed by the guest kernel.
> >
> > Well not necessarily. E.g. virtqueue_kick does not
> > kick if hypervisor didn't enable notifications
> > (e.g. with event idx they get disabled automatically).
> > So it won't recover if hypervisor can discard
> > kicks.
> >
> > I think this comment needs clarification.
>
> Makes sense. I didn't think to consider "hypervisor discards kicks" as a failure
> mode. :) Would virtqueue_kick return false in that case? If so, needs_kick will
> stay true and the kick will keep being retried on subsequent send/recv
> operations until the hypervisor has enabled notifications again.
>
> I will clarify the comment to touch on that situation.
IIRC virtqueue_kick returns false when device is broken.
If you want to handle that you generally need to reset
the device. Kicking it won't fix it :)
>
> >> + */
> >> + bool needs_kick;
> >> +
> >> + /* Number of bytes available to read from the virtqueue buffer. */
> >> + unsigned int readable;
> >> +
> >> + /*
> >> + * Buffer in which all virtio transfers take place. Buffer size is the
> >> + * maximum legal TPM command or response message size.
> >> + */
> >> + u8 virtqueue_buffer[TPM_BUFSIZE];
> >> +};
> >> +
> >> +/*
> >> + * Wait for ownership of the virtqueue buffer.
> >> + *
> >> + * The why-string should begin with "waiting to..." or "waiting for..." with no
> >> + * trailing newline. It will appear in log output.
> >> + *
> >> + * Returns zero for success, otherwise negative error.
> >> + */
> >> +static int vtpm_wait_for_buffer(struct vtpm_device *dev, const char *why)
> >> +{
> >> + int ret;
> >> + bool did_kick;
> >> + struct tpm_chip *chip = dev->chip;
> >> + unsigned long deadline = jiffies + TPM_VIRTIO_TIMEOUT;
> >> +
> >> + /* Kick queue if needed. */
> >> + if (dev->needs_kick) {
> >> + did_kick = virtqueue_kick(dev->vq);
> >> + if (!did_kick) {
> >> + dev_notice(&chip->dev, "kick failed; will retry\n");
> >> + return -EBUSY;
> >> + }
> >> + dev->needs_kick = false;
> >> + }
> >> +
> >> + while (!dev->driver_has_buffer) {
> >> + unsigned long now = jiffies;
> >> +
> >> + /* Check timeout, otherwise `deadline - now` may underflow. */
> >> + if time_after_eq(now, deadline) {
> >> + dev_warn(&chip->dev, "timed out %s\n", why);
> >> + dev->needs_kick = true;
> >> + return -ETIMEDOUT;
> >> + }
> >> +
> >> + /*
> >> + * Wait to be signaled by virtio callback.
> >> + *
> >> + * Positive ret is jiffies remaining until timeout when the
> >> + * completion occurred, which means successful completion. Zero
> >> + * ret is timeout. Negative ret is error.
> >> + */
> >> + ret = wait_for_completion_killable_timeout(
> >> + &dev->complete, deadline - now);
> >> +
> >> + /* Log if completion did not occur. */
> >> + if (ret == -ERESTARTSYS) {
> >> + /* Not a warning if it was simply interrupted. */
> >> + dev_dbg(&chip->dev, "interrupted %s\n", why);
> >> + } else if (ret == 0) {
> >> + dev_warn(&chip->dev, "timed out %s\n", why);
> >> + ret = -ETIMEDOUT;
> >
> > Should we check NEEDS_RESET bit and try to reset the device?
> > Or is that too drastic?
>
> I'll let you make the call on whether we need a reset implemented. This driver
> was initially based on the simple virtio-rng driver which doesn't reset (but nor
> does it detect timeout). Let me know and I can give it a shot.
I think yes - generally NEEDS_RESET is the only mechanism we have for recovering from
device errors. And yes no one implemented it yet :)
>
> >
> >> + } else if (ret < 0) {
> >> + dev_warn(&chip->dev, "failed while %s: error %d\n",
> >> + why, -ret);
> >> + }
> >> +
> >> + /*
> >> + * Return error if completion did not occur. Schedule kick to be
> >> + * retried at the start of the next send/recv to help unblock
> >> + * the queue.
> >> + */
> >> + if (ret < 0) {
> >> + dev->needs_kick = true;
> >> + return ret;
> >> + }
> >> +
> >> + /* Completion occurred. Expect response buffer back. */
> >> + if (virtqueue_get_buf(dev->vq, &dev->readable)) {
> >> + dev->driver_has_buffer = true;
> >> +
> >> + if (dev->readable > TPM_BUFSIZE) {
> >> + dev_crit(&chip->dev,
> >> + "hypervisor bug: response exceeds max size, %u > %u\n",
> >> + dev->readable,
> >> + (unsigned int) TPM_BUFSIZE);
> >> + dev->readable = TPM_BUFSIZE;
> >> + return -EPROTO;
> >> + }
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int vtpm_op_send(struct tpm_chip *chip, u8 *caller_buf, size_t len)
> >> +{
> >> + int ret;
> >> + bool did_kick;
> >> + struct scatterlist sg_outbuf, sg_inbuf;
> >> + struct scatterlist *sgs[2] = { &sg_outbuf, &sg_inbuf };
> >> + struct vtpm_device *dev = dev_get_drvdata(&chip->dev);
> >> + u8 *virtqueue_buf = dev->virtqueue_buffer;
> >> +
> >> + dev_dbg(&chip->dev, __func__ " %zu bytes\n", len);
> >> +
> >> + if (len > TPM_BUFSIZE) {
> >> + dev_err(&chip->dev,
> >> + "command is too long, %zu > %zu\n",
> >> + len, (size_t) TPM_BUFSIZE);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /*
> >> + * Wait until hypervisor relinquishes ownership of the virtqueue buffer.
> >> + *
> >> + * This may block if the previous recv operation timed out in the guest
> >> + * kernel but is still being processed by the hypervisor. Also may block
> >> + * if send operations are performed back-to-back, such as if something
> >> + * in the caller failed in between a send and recv.
> >> + *
> >> + * During normal operation absent of any errors or timeouts, this does
> >> + * not block.
> >> + */
> >> + ret = vtpm_wait_for_buffer(dev, "waiting to begin send");
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Driver owns virtqueue buffer and may now write into it. */
> >> + memcpy(virtqueue_buf, caller_buf, len);
> >> +
> >> + /*
> >> + * Enqueue the virtqueue buffer once as outgoing virtio data (written by
> >> + * the virtual machine and read by the hypervisor) and again as incoming
> >> + * data (written by the hypervisor and read by the virtual machine).
> >> + * This step moves ownership of the virtqueue buffer from driver to
> >> + * hypervisor.
> >> + *
> >> + * Note that we don't know here how big of a buffer the caller will use
> >> + * with their later call to recv. We allow the hypervisor to write up to
> >> + * the TPM max message size. If the caller ends up using a smaller
> >> + * buffer with recv that is too small to hold the entire response, the
> >> + * recv will return an error. This conceivably breaks TPM
> >> + * implementations that want to produce a different verbosity of
> >> + * response depending on the receiver's buffer size.
> >> + */
> >> + sg_init_one(&sg_outbuf, virtqueue_buf, len);
> >> + sg_init_one(&sg_inbuf, virtqueue_buf, TPM_BUFSIZE);
> >> + ret = virtqueue_add_sgs(dev->vq, sgs, 1, 1, virtqueue_buf, GFP_KERNEL);
> >> + if (ret) {
> >> + dev_err(&chip->dev, "failed virtqueue_add_sgs\n");
> >> + return ret;
> >> + }
> >> +
> >> + /* Kick the other end of the virtqueue after having added a buffer. */
> >> + did_kick = virtqueue_kick(dev->vq);
> >> + if (!did_kick) {
> >> + dev->needs_kick = true;
> >> + dev_notice(&chip->dev, "kick failed; will retry\n");
> >> +
> >> + /*
> >> + * We return 0 anyway because what the caller doesn't know can't
> >> + * hurt them. They can call recv and it will retry the kick. If
> >> + * that works, everything is fine.
> >> + *
> >> + * If the retry in recv fails too, they will get -EBUSY from
> >> + * recv.
> >> + */
> >> + }
> >> +
> >> + /*
> >> + * Hypervisor is now processing the TPM command asynchronously. It will
> >> + * read the command from the output buffer and write the response into
> >> + * the input buffer (which are the same buffer). When done, it will send
> >> + * back the buffers over virtio and the driver's virtio callback will
> >> + * complete dev->complete so that we know the response is ready to be
> >> + * read.
> >> + *
> >> + * It is important to have copied data out of the caller's buffer into
> >> + * the driver's virtqueue buffer because the caller is free to destroy
> >> + * their buffer when this call returns. We can't avoid copying by
> >> + * waiting here for the hypervisor to finish reading, because if that
> >> + * wait times out, we return and the caller may destroy their buffer
> >> + * while the hypervisor is continuing to read from it.
> >> + */
> >> + dev->driver_has_buffer = false;
> >> + return 0;
> >> +}
> >> +
> >> +static int vtpm_op_recv(struct tpm_chip *chip, u8 *caller_buf, size_t len)
> >> +{
> >> + int ret;
> >> + struct vtpm_device *dev = dev_get_drvdata(&chip->dev);
> >> + u8 *virtqueue_buf = dev->virtqueue_buffer;
> >> +
> >> + dev_dbg(&chip->dev, __func__ "\n");
> >> +
> >> + /*
> >> + * Wait until the virtqueue buffer is owned by the driver.
> >> + *
> >> + * This will usually block while the hypervisor finishes processing the
> >> + * most recent TPM command.
> >> + */
> >> + ret = vtpm_wait_for_buffer(dev, "waiting for TPM response");
> >> + if (ret)
> >> + return ret;
> >> +
> >> + dev_dbg(&chip->dev, "received %u bytes\n", dev->readable);
> >> +
> >> + if (dev->readable > len) {
> >> + dev_notice(&chip->dev,
> >> + "TPM response is bigger than receiver's buffer: %u > %zu\n",
> >> + dev->readable, len);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Copy response over to the caller. */
> >> + memcpy(caller_buf, virtqueue_buf, dev->readable);
> >> +
> >> + return dev->readable;
> >> +}
> >> +
> >> +static void vtpm_op_cancel(struct tpm_chip *chip)
> >> +{
> >> + /*
> >> + * Cancel is not called in this driver's use of tpm-interface.c. It may
> >> + * be triggered through tpm-sysfs but that can be implemented as needed.
> >> + * Be aware that tpm-sysfs performs cancellation without holding the
> >> + * tpm_mutex that protects our send and recv operations, so a future
> >> + * implementation will need to consider thread safety of concurrent
> >> + * send/recv and cancel.
> >> + */
> >> + dev_notice(&chip->dev, "cancellation is not implemented\n");
> >> +}
> >> +
> >> +static u8 vtpm_op_status(struct tpm_chip *chip)
> >> +{
> >> + /*
> >> + * Status is for TPM drivers that want tpm-interface.c to poll for
> >> + * completion before calling recv. Usually this is when the hardware
> >> + * needs to be polled i.e. there is no other way for recv to block on
> >> + * the TPM command completion.
> >> + *
> >> + * Polling goes until `(status & complete_mask) == complete_val`. This
> >> + * driver defines both complete_mask and complete_val as 0 and blocks on
> >> + * our own completion object in recv instead.
> >> + */
> >> + return 0;
> >> +}
> >> +
> >> +static const struct tpm_class_ops vtpm_ops = {
> >> + .flags = TPM_OPS_AUTO_STARTUP,
> >> + .send = vtpm_op_send,
> >> + .recv = vtpm_op_recv,
> >> + .cancel = vtpm_op_cancel,
> >> + .status = vtpm_op_status,
> >> + .req_complete_mask = 0,
> >> + .req_complete_val = 0,
> >> +};
> >> +
> >> +static void vtpm_virtio_complete(struct virtqueue *vq)
> >> +{
> >> + struct virtio_device *vdev = vq->vdev;
> >> + struct vtpm_device *dev = vdev->priv;
> >> +
> >> + complete(&dev->complete);
> >> +}
> >> +
> >> +static int vtpm_probe(struct virtio_device *vdev)
> >> +{
> >> + int err;
> >> + struct vtpm_device *dev;
> >> + struct virtqueue *vq;
> >> + struct tpm_chip *chip;
> >> +
> >> + dev_dbg(&vdev->dev, __func__ "\n");
> >> +
> >> + dev = kzalloc(sizeof(struct vtpm_device), GFP_KERNEL);
> >> + if (!dev) {
> >> + err = -ENOMEM;
> >> + dev_err(&vdev->dev, "failed kzalloc\n");
> >> + goto err_dev_alloc;
> >> + }
> >> + vdev->priv = dev;
> >> +
> >> + vq = virtio_find_single_vq(vdev, vtpm_virtio_complete, "vtpm");
> >> + if (IS_ERR(vq)) {
> >> + err = PTR_ERR(vq);
> >> + dev_err(&vdev->dev, "failed virtio_find_single_vq\n");
> >> + goto err_virtio_find;
> >> + }
> >> + dev->vq = vq;
> >> +
> >> + chip = tpm_chip_alloc(&vdev->dev, &vtpm_ops);
> >> + if (IS_ERR(chip)) {
> >> + err = PTR_ERR(chip);
> >> + dev_err(&vdev->dev, "failed tpm_chip_alloc\n");
> >> + goto err_chip_alloc;
> >> + }
> >> + dev_set_drvdata(&chip->dev, dev);
> >> + chip->flags |= TPM_CHIP_FLAG_TPM2;
> >> + dev->chip = chip;
> >> +
> >> + init_completion(&dev->complete);
> >> + dev->driver_has_buffer = true;
> >> + dev->needs_kick = false;
> >> + dev->readable = 0;
> >> +
> >> + /*
> >> + * Required in order to enable vq use in probe function for auto
> >> + * startup.
> >> + */
> >> + virtio_device_ready(vdev);
> >> +
> >> + err = tpm_chip_register(dev->chip);
> >> + if (err) {
> >> + dev_err(&vdev->dev, "failed tpm_chip_register\n");
> >> + goto err_chip_register;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_chip_register:
> >> + put_device(&dev->chip->dev);
> >> +err_chip_alloc:
> >> + vdev->config->del_vqs(vdev);
> >> +err_virtio_find:
> >> + kfree(dev);
> >> +err_dev_alloc:
> >> + return err;
> >> +}
> >> +
> >> +static void vtpm_remove(struct virtio_device *vdev)
> >> +{
> >> + struct vtpm_device *dev = vdev->priv;
> >> +
> >> + /* Undo tpm_chip_register. */
> >> + tpm_chip_unregister(dev->chip);
> >> +
> >> + /* Undo tpm_chip_alloc. */
> >> + put_device(&dev->chip->dev);
> >> +
> >> + vdev->config->reset(vdev);
> >> + vdev->config->del_vqs(vdev);
> >> +
> >> + kfree(dev);
> >> +}
> >> +
> >> +#define VIRTIO_ID_TPM 31
> >> +
> >> +static struct virtio_device_id id_table[] = {
> >> + {
> >> + .device = VIRTIO_ID_TPM,
> >> + .vendor = VIRTIO_DEV_ANY_ID,
> >> + },
> >> + {},
> >> +};
> >
> > Let's write
> >
> > static struct virtio_device_id id_table[] = {
> > { VIRTIO_ID_TPM, VIRTIO_DEV_ANY_ID },
> > { 0 },
> > };
> >
> > for consistency with other virtio devices.
>
> Acknowledged, will do.
>
>
> >
> >> +
> >> +static struct virtio_driver vtpm_driver = {
> >> + .driver.name = KBUILD_MODNAME,
> >> + .driver.owner = THIS_MODULE,
> >> + .id_table = id_table,
> >> + .probe = vtpm_probe,
> >> + .remove = vtpm_remove,
> >
> > Freeze/restore?
>
> Good call. We don't need this for Chrome OS but I agree it should be in the
> driver. I will provide a basic freeze/restore in line with what I see in
> virtio-rng.
>
> Thanks!
>
>
> >
> >
> >> +};
> >> +
> >> +module_virtio_driver(vtpm_driver);
> >> +
> >> +MODULE_AUTHOR("David Tolnay (dtolnay@gmail.com)");
> >> +MODULE_DESCRIPTION("Virtio vTPM Driver");
> >> +MODULE_VERSION("1.0");
> >> +MODULE_LICENSE("GPL");
> >> --
> >> 2.20.1
> >
^ permalink raw reply
* Re: [PATCH] tpm: Add driver for TPM over virtio
From: Michael S. Tsirkin @ 2019-02-22 23:05 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: dgreid, virtualization, Jason Gunthorpe, linux-integrity,
Peter Huewe, apronin, David Tolnay
In-Reply-To: <20190222223634.GA27601@linux.intel.com>
On Sat, Feb 23, 2019 at 12:36:34AM +0200, Jarkko Sakkinen wrote:
> I do not require spec quality documentation. Just a few paragraphs of
> what is in crosvm, kernel etc. and something about inner workings to
> get a rough idea. No need for TCG level spec for this :-)
OTOH for virtio if there's no implementation in a popular
hypervisor/language, we do ask for a somewhat detailed
spec.
--
MST
^ permalink raw reply
* [PATCH v6 3/3] drm/qxl: remove conflicting framebuffers earlier
From: Gerd Hoffmann @ 2019-02-25 8:30 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, intel-gfx, open list,
open list:DRM DRIVER FOR QXL VIRTUAL GPU, daniel,
open list:DRM DRIVER FOR QXL VIRTUAL GPU, Dave Airlie
In-Reply-To: <20190225083009.16082-1-kraxel@redhat.com>
Add error checking while being at it.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/qxl/qxl_drv.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index bb81e310eb6d..578d867a81d5 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -79,6 +79,10 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (ret)
goto free_dev;
+ ret = drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
+ if (ret)
+ goto disable_pci;
+
ret = qxl_device_init(qdev, &qxl_driver, pdev);
if (ret)
goto disable_pci;
@@ -94,7 +98,6 @@ qxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
if (ret)
goto modeset_cleanup;
- drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, 0, "qxl");
drm_fbdev_generic_setup(&qdev->ddev, 32);
return 0;
--
2.9.3
^ permalink raw reply related
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Stephen Hemminger @ 2019-02-26 1:39 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
liran.alon, Netdev, David Miller
In-Reply-To: <e6a53bd1-83ab-f170-406a-03276e8c87e2@oracle.com>
On Mon, 25 Feb 2019 16:58:07 -0800
si-wei liu <si-wei.liu@oracle.com> wrote:
> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> >>
> >> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> >>>
> >>> On 2/21/2019 7:33 PM, si-wei liu wrote:
> >>>>
> >>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> >>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> >>>>>> Sorry for replying to this ancient thread. There was some remaining
> >>>>>> issue that I don't think the initial net_failover patch got addressed
> >>>>>> cleanly, see:
> >>>>>>
> >>>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> >>>>>>
> >>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> >>>>>> not specifically writtten for such kernel automatic enslavement.
> >>>>>> Specifically, if it is a bond or team, the slave would typically get
> >>>>>> renamed *before* virtual device gets created, that's what udev can
> >>>>>> control (without getting netdev opened early by the other part of
> >>>>>> kernel) and other userspace components for e.g. initramfs,
> >>>>>> init-scripts can coordinate well in between. The in-kernel
> >>>>>> auto-enslavement of net_failover breaks this userspace convention,
> >>>>>> which don't provides a solution if user care about consistent naming
> >>>>>> on the slave netdevs specifically.
> >>>>>>
> >>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
> >>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
> >>>>>> problem ever since. Please share your mind how to proceed and solve
> >>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
> >>>>> Above says:
> >>>>>
> >>>>> there's no motivation in the systemd/udevd community at
> >>>>> this point to refactor the rename logic and make it work well with
> >>>>> 3-netdev.
> >>>>>
> >>>>> What would the fix be? Skip slave devices?
> >>>>>
> >>>> There's nothing user can get if just skipping slave devices - the
> >>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
> >>>> next reboot, while the rest may conform to the naming scheme (ens3
> >>>> and such). There's no way one can fix this in userspace alone - when
> >>>> the failover is created the enslaved netdev was opened by the kernel
> >>>> earlier than the userspace is made aware of, and there's no
> >>>> negotiation protocol for kernel to know when userspace has done
> >>>> initial renaming of the interface. I would expect netdev list should
> >>>> at least provide the direction in general for how this can be
> >>>> solved...
> >
> > I was just wondering what did you mean when you said
> > "refactor the rename logic and make it work well with 3-netdev" -
> > was there a proposal udev rejected?
> No. I never believed this particular issue can be fixed in userspace
> alone. Previously someone had said it could be, but I never see any work
> or relevant discussion ever happened in various userspace communities
> (for e.g. dracut, initramfs-tools, systemd, udev, and NetworkManager).
> IMHO the root of the issue derives from the kernel, it makes more sense
> to start from netdev, work out and decide on a solution: see what can be
> done in the kernel in order to fix it, then after that engage userspace
> community for the feasibility...
>
> > Anyway, can we write a time diagram for what happens in which order that
> > leads to failure? That would help look for triggers that we can tie
> > into, or add new ones.
> >
>
> See attached diagram.
>
> >
> >
> >
> >
> >>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> >>> to only work with the master failover device.
> >> Where does this expectation come from?
> >>
> >> Admin users may have ethtool or tc configurations that need to deal with
> >> predictable interface name. Third-party app which was built upon specifying
> >> certain interface name can't be modified to chase dynamic names.
> >>
> >> Specifically, we have pre-canned image that uses ethtool to fine tune VF
> >> offload settings post boot for specific workload. Those images won't work
> >> well if the name is constantly changing just after couple rounds of live
> >> migration.
> > It should be possible to specify the ethtool configuration on the
> > master and have it automatically propagated to the slave.
> >
> > BTW this is something we should look at IMHO.
> I was elaborating a few examples that the expectation and assumption
> that user/admin scripts only deal with master failover device is
> incorrect. It had never been taken good care of, although I did try to
> emphasize it from the very beginning.
>
> Basically what you said about propagating the ethtool configuration down
> to the slave is the key pursuance of 1-netdev model. However, what I am
> seeking now is any alternative that can also fix the specific udev
> rename problem, before concluding that 1-netdev is the only solution.
> Generally a 1-netdev scheme would take time to implement, while I'm
> trying to find a way out to fix this particular naming problem under
> 3-netdev.
>
> >
> >>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> >>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
> >>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> >>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> >> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> >> model as much transparent to a real NIC as possible, while a hidden netns is
> >> just the vehicle). However, I recall there was resistance around this
> >> discussion that even the concept of hiding itself is a taboo for Linux
> >> netdev. I would like to summon potential alternatives before concluding
> >> 1-netdev is the only solution too soon.
> >>
> >> Thanks,
> >> -Siwei
> > Your scripts would not work at all then, right?
> At this point we don't claim images with such usage as SR-IOV live
> migrate-able. We would flag it as live migrate-able until this ethtool
> config issue is fully addressed and a transparent live migration
> solution emerges in upstream eventually.
The hyper-v netvsc with 1-dev model uses a timeout to allow udev to do its rename.
I proposed a patch to key state change off of the udev rename, but that patch was
rejected.
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-26 2:05 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
si-wei liu, David Miller
In-Reply-To: <20190225173912.26b93422@shemminger-XPS-13-9360>
On Mon, Feb 25, 2019 at 05:39:12PM -0800, Stephen Hemminger wrote:
> > >>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > >>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > >>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > >>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > >> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > >> model as much transparent to a real NIC as possible, while a hidden netns is
> > >> just the vehicle). However, I recall there was resistance around this
> > >> discussion that even the concept of hiding itself is a taboo for Linux
> > >> netdev. I would like to summon potential alternatives before concluding
> > >> 1-netdev is the only solution too soon.
> > >>
> > >> Thanks,
> > >> -Siwei
> > > Your scripts would not work at all then, right?
> > At this point we don't claim images with such usage as SR-IOV live
> > migrate-able. We would flag it as live migrate-able until this ethtool
> > config issue is fully addressed and a transparent live migration
> > solution emerges in upstream eventually.
>
> The hyper-v netvsc with 1-dev model uses a timeout to allow udev to do its rename.
> I proposed a patch to key state change off of the udev rename, but that patch was
> rejected.
Of course that would mean nothing works without udev - was
that the objection? Could you help me find that discussion pls?
--
MST
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-26 2:08 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
David Miller
In-Reply-To: <e6a53bd1-83ab-f170-406a-03276e8c87e2@oracle.com>
On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
>
>
> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > >
> > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > >
> > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > >
> > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > cleanly, see:
> > > > > > >
> > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > >
> > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > on the slave netdevs specifically.
> > > > > > >
> > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > Above says:
> > > > > >
> > > > > > there's no motivation in the systemd/udevd community at
> > > > > > this point to refactor the rename logic and make it work well with
> > > > > > 3-netdev.
> > > > > >
> > > > > > What would the fix be? Skip slave devices?
> > > > > >
> > > > > There's nothing user can get if just skipping slave devices - the
> > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > earlier than the userspace is made aware of, and there's no
> > > > > negotiation protocol for kernel to know when userspace has done
> > > > > initial renaming of the interface. I would expect netdev list should
> > > > > at least provide the direction in general for how this can be
> > > > > solved...
> >
> > I was just wondering what did you mean when you said
> > "refactor the rename logic and make it work well with 3-netdev" -
> > was there a proposal udev rejected?
> No. I never believed this particular issue can be fixed in userspace alone.
> Previously someone had said it could be, but I never see any work or
> relevant discussion ever happened in various userspace communities (for e.g.
> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> of the issue derives from the kernel, it makes more sense to start from
> netdev, work out and decide on a solution: see what can be done in the
> kernel in order to fix it, then after that engage userspace community for
> the feasibility...
>
> > Anyway, can we write a time diagram for what happens in which order that
> > leads to failure? That would help look for triggers that we can tie
> > into, or add new ones.
> >
>
> See attached diagram.
>
> >
> >
> >
> >
> > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > to only work with the master failover device.
> > > Where does this expectation come from?
> > >
> > > Admin users may have ethtool or tc configurations that need to deal with
> > > predictable interface name. Third-party app which was built upon specifying
> > > certain interface name can't be modified to chase dynamic names.
> > >
> > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > offload settings post boot for specific workload. Those images won't work
> > > well if the name is constantly changing just after couple rounds of live
> > > migration.
> > It should be possible to specify the ethtool configuration on the
> > master and have it automatically propagated to the slave.
> >
> > BTW this is something we should look at IMHO.
> I was elaborating a few examples that the expectation and assumption that
> user/admin scripts only deal with master failover device is incorrect. It
> had never been taken good care of, although I did try to emphasize it from
> the very beginning.
>
> Basically what you said about propagating the ethtool configuration down to
> the slave is the key pursuance of 1-netdev model. However, what I am seeking
> now is any alternative that can also fix the specific udev rename problem,
> before concluding that 1-netdev is the only solution. Generally a 1-netdev
> scheme would take time to implement, while I'm trying to find a way out to
> fix this particular naming problem under 3-netdev.
>
> >
> > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > just the vehicle). However, I recall there was resistance around this
> > > discussion that even the concept of hiding itself is a taboo for Linux
> > > netdev. I would like to summon potential alternatives before concluding
> > > 1-netdev is the only solution too soon.
> > >
> > > Thanks,
> > > -Siwei
> > Your scripts would not work at all then, right?
> At this point we don't claim images with such usage as SR-IOV live
> migrate-able. We would flag it as live migrate-able until this ethtool
> config issue is fully addressed and a transparent live migration solution
> emerges in upstream eventually.
>
>
> Thanks,
> -Siwei
> >
> >
> > > > > -Siwei
> > > > >
> > > > >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
>
>
> net_failover(kernel) | network.service (user) | systemd-udevd (user)
> --------------------------------------------------+------------------------------+--------------------------------------------
> (standby virtio-net and net_failover | |
> devices created and initialized, | |
> i.e. virtnet_probe()-> | |
> net_failover_create() | |
> was done.) | |
> | |
> | runs `ifup ens3' -> |
> | ip link set dev ens3 up |
> net_failover_open() | |
> dev_open(virtnet_dev) | |
> virtnet_open(virtnet_dev) | |
> netif_carrier_on(failover_dev) | |
> ... | |
> | |
> (VF hot plugged in) | |
> ixgbevf_probe() | |
> register_netdev(ixgbevf_netdev) | |
> netdev_register_kobject(ixgbevf_netdev) | |
> kobject_add(ixgbevf_dev) | |
> device_add(ixgbevf_dev) | |
> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> netlink_broadcast() | |
> ... | |
> call_netdevice_notifiers(NETDEV_REGISTER) | |
> failover_event(..., NETDEV_REGISTER, ...) | |
> failover_slave_register(ixgbevf_netdev) | |
> net_failover_slave_register(ixgbevf_netdev) | |
> dev_open(ixgbevf_netdev) | |
> | |
> | |
> | | received ADD uevent from netlink fd
> | | ...
> | | udev-builtin-net_id.c:dev_pci_slot()
> | | (decided to renamed 'eth0' )
> | | ip link set dev eth0 name ens4
> (dev_change_name() returns -EBUSY as | |
> ixgbevf_netdev->flags has IFF_UP) | |
> | |
>
Given renaming slaves does not work anyway: would it work if we just
hard-coded slave names instead?
E.g.
1. fail slave renames
2. rename of failover to XX automatically renames standby to XXnsby
and primary to XXnpry
--
MST
^ permalink raw reply
* Re: [PATCH v2 char-misc-next 4/7] mic: vop: Add loopback driver
From: Greg KH @ 2019-02-26 11:56 UTC (permalink / raw)
To: Vincent Whitchurch
Cc: Vincent Whitchurch, arnd, linux-kernel, sudeep.dutt,
virtualization, ashutosh.dixit
In-Reply-To: <20190222153056.18878-7-vincent.whitchurch@axis.com>
On Fri, Feb 22, 2019 at 04:30:52PM +0100, Vincent Whitchurch wrote:
> Add a loopback driver to allow testing and evaluation of the VOP
> framework without special hardware. The host and the guest will run
> under the same kernel.
>
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> drivers/misc/mic/Kconfig | 10 +
> drivers/misc/mic/vop/Makefile | 2 +
> drivers/misc/mic/vop/vop_loopback.c | 382 ++++++++++++++++++++++++++++
> 3 files changed, 394 insertions(+)
> create mode 100644 drivers/misc/mic/vop/vop_loopback.c
You sent 2 4/7 patches in this series, is that intentional? Are they
different?
I've applied the first 3 here, can you rebase and resend the rest of
these, without any duplicated patches?
thanks,
greg k-h
^ permalink raw reply
* [PATCH 1/3] drm/virtio: implement prime mmap
From: Gerd Hoffmann @ 2019-02-27 14:44 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:VIRTIO GPU DRIVER
In-Reply-To: <20190227144441.6755-1-kraxel@redhat.com>
Sync gem vm_node.start with ttm vm_node.start,
then we can just call drm_gem_prime_mmap().
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_prime.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
index c59ec34c80a5..b4c9199349e7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -56,7 +56,10 @@ void virtgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
}
int virtgpu_gem_prime_mmap(struct drm_gem_object *obj,
- struct vm_area_struct *area)
+ struct vm_area_struct *vma)
{
- return -ENODEV;
+ struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+
+ bo->gem_base.vma_node.vm_node.start = bo->tbo.vma_node.vm_node.start;
+ return drm_gem_prime_mmap(obj, vma);
}
--
2.9.3
^ permalink raw reply related
* [PATCH 2/3] drm/virtio: implement prime pin/unpin
From: Gerd Hoffmann @ 2019-02-27 14:44 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:VIRTIO GPU DRIVER
In-Reply-To: <20190227144441.6755-1-kraxel@redhat.com>
virtio-gpu objects never move around, so effectively they are pinned
all the time. This makes the the implementation pretty easy ;)
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_prime.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
index b4c9199349e7..0fcae0e46abd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -30,13 +30,13 @@
int virtgpu_gem_prime_pin(struct drm_gem_object *obj)
{
- WARN_ONCE(1, "not implemented");
- return -ENODEV;
+ /* nothing: all virtio-gpu objects are pinned all the time */
+ return 0;
}
void virtgpu_gem_prime_unpin(struct drm_gem_object *obj)
{
- WARN_ONCE(1, "not implemented");
+ /* nothing */
}
void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj)
--
2.9.3
^ permalink raw reply related
* [PATCH 3/3] drm/virtio: implement prime export
From: Gerd Hoffmann @ 2019-02-27 14:44 UTC (permalink / raw)
To: dri-devel
Cc: David Airlie, open list, Daniel Vetter,
open list:VIRTIO GPU DRIVER
In-Reply-To: <20190227144441.6755-1-kraxel@redhat.com>
Just run drm_prime_pages_to_sg() on the ttm pages list to get an
sg_table for export. The pages list is created at object initialization
time, so there should be no need to handle an unpopulated page list.
Add a sanity check nevertheless.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
drivers/gpu/drm/virtio/virtgpu_drv.c | 2 ++
drivers/gpu/drm/virtio/virtgpu_prime.c | 12 ++++++++++++
3 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 3238fdf58eb4..fb1095fa873f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -354,6 +354,7 @@ int virtio_gpu_object_wait(struct virtio_gpu_object *bo, bool no_wait);
/* virtgpu_prime.c */
int virtgpu_gem_prime_pin(struct drm_gem_object *obj);
void virtgpu_gem_prime_unpin(struct drm_gem_object *obj);
+struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj);
void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj);
void virtgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
int virtgpu_gem_prime_mmap(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index b996ac1d4fcc..4f4f578752fa 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -205,10 +205,12 @@ static struct drm_driver driver = {
#if defined(CONFIG_DEBUG_FS)
.debugfs_init = virtio_gpu_debugfs_init,
#endif
+ .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.gem_prime_export = drm_gem_prime_export,
.gem_prime_import = drm_gem_prime_import,
.gem_prime_pin = virtgpu_gem_prime_pin,
.gem_prime_unpin = virtgpu_gem_prime_unpin,
+ .gem_prime_get_sg_table = virtgpu_gem_prime_get_sg_table,
.gem_prime_vmap = virtgpu_gem_prime_vmap,
.gem_prime_vunmap = virtgpu_gem_prime_vunmap,
.gem_prime_mmap = virtgpu_gem_prime_mmap,
diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
index 0fcae0e46abd..1fe593e391b5 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -39,6 +39,18 @@ void virtgpu_gem_prime_unpin(struct drm_gem_object *obj)
/* nothing */
}
+struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj)
+{
+ struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+
+ if (!bo->tbo.ttm->pages || !bo->tbo.ttm->num_pages)
+ /* should not happen */
+ return ERR_PTR(-EINVAL);
+
+ return drm_prime_pages_to_sg(bo->tbo.ttm->pages,
+ bo->tbo.ttm->num_pages);
+}
+
void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj)
{
struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
--
2.9.3
^ permalink raw reply related
* Re: [PATCH 2/3] drm/virtio: implement prime pin/unpin
From: Noralf Trønnes @ 2019-02-27 17:07 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER
In-Reply-To: <20190227144441.6755-3-kraxel@redhat.com>
Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> virtio-gpu objects never move around, so effectively they are pinned
> all the time. This makes the the implementation pretty easy ;)
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> drivers/gpu/drm/virtio/virtgpu_prime.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index b4c9199349e7..0fcae0e46abd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -30,13 +30,13 @@
>
> int virtgpu_gem_prime_pin(struct drm_gem_object *obj)
> {
> - WARN_ONCE(1, "not implemented");
> - return -ENODEV;
> + /* nothing: all virtio-gpu objects are pinned all the time */
> + return 0;
> }
>
> void virtgpu_gem_prime_unpin(struct drm_gem_object *obj)
> {
> - WARN_ONCE(1, "not implemented");
> + /* nothing */
> }
You can just remove these dummies the callbacks are optional. See
drm_gem_pin().
With that:
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
>
> void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj)
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 1/3] drm/virtio: implement prime mmap
From: Noralf Trønnes @ 2019-02-27 17:11 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER
In-Reply-To: <20190227144441.6755-2-kraxel@redhat.com>
Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> Sync gem vm_node.start with ttm vm_node.start,
> then we can just call drm_gem_prime_mmap().
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
Acked-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH 3/3] drm/virtio: implement prime export
From: Noralf Trønnes @ 2019-02-27 17:11 UTC (permalink / raw)
To: Gerd Hoffmann, dri-devel
Cc: David Airlie, open list, open list:VIRTIO GPU DRIVER
In-Reply-To: <20190227144441.6755-4-kraxel@redhat.com>
Den 27.02.2019 15.44, skrev Gerd Hoffmann:
> Just run drm_prime_pages_to_sg() on the ttm pages list to get an
> sg_table for export. The pages list is created at object initialization
> time, so there should be no need to handle an unpopulated page list.
> Add a sanity check nevertheless.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Acked-by: Noralf Trønnes <noralf@tronnes.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Stephen Hemminger @ 2019-02-27 21:57 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Michael S. Tsirkin,
Jakub Kicinski, Samudrala, Sridhar, virtualization, Siwei Liu,
liran.alon, Netdev, David Miller
In-Reply-To: <d1060c75-eaba-ab6f-ff31-38cb3a47c711@oracle.com>
On Tue, 26 Feb 2019 16:17:21 -0800
si-wei liu <si-wei.liu@oracle.com> wrote:
> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> >>
> >> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> >>>> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> >>>>> On 2/21/2019 7:33 PM, si-wei liu wrote:
> >>>>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> >>>>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> >>>>>>>> Sorry for replying to this ancient thread. There was some remaining
> >>>>>>>> issue that I don't think the initial net_failover patch got addressed
> >>>>>>>> cleanly, see:
> >>>>>>>>
> >>>>>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> >>>>>>>>
> >>>>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> >>>>>>>> not specifically writtten for such kernel automatic enslavement.
> >>>>>>>> Specifically, if it is a bond or team, the slave would typically get
> >>>>>>>> renamed *before* virtual device gets created, that's what udev can
> >>>>>>>> control (without getting netdev opened early by the other part of
> >>>>>>>> kernel) and other userspace components for e.g. initramfs,
> >>>>>>>> init-scripts can coordinate well in between. The in-kernel
> >>>>>>>> auto-enslavement of net_failover breaks this userspace convention,
> >>>>>>>> which don't provides a solution if user care about consistent naming
> >>>>>>>> on the slave netdevs specifically.
> >>>>>>>>
> >>>>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
> >>>>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
> >>>>>>>> problem ever since. Please share your mind how to proceed and solve
> >>>>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
> >>>>>>> Above says:
> >>>>>>>
> >>>>>>> there's no motivation in the systemd/udevd community at
> >>>>>>> this point to refactor the rename logic and make it work well with
> >>>>>>> 3-netdev.
> >>>>>>>
> >>>>>>> What would the fix be? Skip slave devices?
> >>>>>>>
> >>>>>> There's nothing user can get if just skipping slave devices - the
> >>>>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
> >>>>>> next reboot, while the rest may conform to the naming scheme (ens3
> >>>>>> and such). There's no way one can fix this in userspace alone - when
> >>>>>> the failover is created the enslaved netdev was opened by the kernel
> >>>>>> earlier than the userspace is made aware of, and there's no
> >>>>>> negotiation protocol for kernel to know when userspace has done
> >>>>>> initial renaming of the interface. I would expect netdev list should
> >>>>>> at least provide the direction in general for how this can be
> >>>>>> solved...
> >>> I was just wondering what did you mean when you said
> >>> "refactor the rename logic and make it work well with 3-netdev" -
> >>> was there a proposal udev rejected?
> >> No. I never believed this particular issue can be fixed in userspace alone.
> >> Previously someone had said it could be, but I never see any work or
> >> relevant discussion ever happened in various userspace communities (for e.g.
> >> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> >> of the issue derives from the kernel, it makes more sense to start from
> >> netdev, work out and decide on a solution: see what can be done in the
> >> kernel in order to fix it, then after that engage userspace community for
> >> the feasibility...
> >>
> >>> Anyway, can we write a time diagram for what happens in which order that
> >>> leads to failure? That would help look for triggers that we can tie
> >>> into, or add new ones.
> >>>
> >> See attached diagram.
> >>
> >>>
> >>>
> >>>
> >>>>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> >>>>> to only work with the master failover device.
> >>>> Where does this expectation come from?
> >>>>
> >>>> Admin users may have ethtool or tc configurations that need to deal with
> >>>> predictable interface name. Third-party app which was built upon specifying
> >>>> certain interface name can't be modified to chase dynamic names.
> >>>>
> >>>> Specifically, we have pre-canned image that uses ethtool to fine tune VF
> >>>> offload settings post boot for specific workload. Those images won't work
> >>>> well if the name is constantly changing just after couple rounds of live
> >>>> migration.
> >>> It should be possible to specify the ethtool configuration on the
> >>> master and have it automatically propagated to the slave.
> >>>
> >>> BTW this is something we should look at IMHO.
> >> I was elaborating a few examples that the expectation and assumption that
> >> user/admin scripts only deal with master failover device is incorrect. It
> >> had never been taken good care of, although I did try to emphasize it from
> >> the very beginning.
> >>
> >> Basically what you said about propagating the ethtool configuration down to
> >> the slave is the key pursuance of 1-netdev model. However, what I am seeking
> >> now is any alternative that can also fix the specific udev rename problem,
> >> before concluding that 1-netdev is the only solution. Generally a 1-netdev
> >> scheme would take time to implement, while I'm trying to find a way out to
> >> fix this particular naming problem under 3-netdev.
> >>
> >>>>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> >>>>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
> >>>>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> >>>>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> >>>> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> >>>> model as much transparent to a real NIC as possible, while a hidden netns is
> >>>> just the vehicle). However, I recall there was resistance around this
> >>>> discussion that even the concept of hiding itself is a taboo for Linux
> >>>> netdev. I would like to summon potential alternatives before concluding
> >>>> 1-netdev is the only solution too soon.
> >>>>
> >>>> Thanks,
> >>>> -Siwei
> >>> Your scripts would not work at all then, right?
> >> At this point we don't claim images with such usage as SR-IOV live
> >> migrate-able. We would flag it as live migrate-able until this ethtool
> >> config issue is fully addressed and a transparent live migration solution
> >> emerges in upstream eventually.
> >>
> >>
> >> Thanks,
> >> -Siwei
> >>>
> >>>>>> -Siwei
> >>>>>>
> >>>>>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> >>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >>>
> >> net_failover(kernel) | network.service (user) | systemd-udevd (user)
> >> --------------------------------------------------+------------------------------+--------------------------------------------
> >> (standby virtio-net and net_failover | |
> >> devices created and initialized, | |
> >> i.e. virtnet_probe()-> | |
> >> net_failover_create() | |
> >> was done.) | |
> >> | |
> >> | runs `ifup ens3' -> |
> >> | ip link set dev ens3 up |
> >> net_failover_open() | |
> >> dev_open(virtnet_dev) | |
> >> virtnet_open(virtnet_dev) | |
> >> netif_carrier_on(failover_dev) | |
> >> ... | |
> >> | |
> >> (VF hot plugged in) | |
> >> ixgbevf_probe() | |
> >> register_netdev(ixgbevf_netdev) | |
> >> netdev_register_kobject(ixgbevf_netdev) | |
> >> kobject_add(ixgbevf_dev) | |
> >> device_add(ixgbevf_dev) | |
> >> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> >> netlink_broadcast() | |
> >> ... | |
> >> call_netdevice_notifiers(NETDEV_REGISTER) | |
> >> failover_event(..., NETDEV_REGISTER, ...) | |
> >> failover_slave_register(ixgbevf_netdev) | |
> >> net_failover_slave_register(ixgbevf_netdev) | |
> >> dev_open(ixgbevf_netdev) | |
> >> | |
> >> | |
> >> | | received ADD uevent from netlink fd
> >> | | ...
> >> | | udev-builtin-net_id.c:dev_pci_slot()
> >> | | (decided to renamed 'eth0' )
> >> | | ip link set dev eth0 name ens4
> >> (dev_change_name() returns -EBUSY as | |
> >> ixgbevf_netdev->flags has IFF_UP) | |
> >> | |
> >>
> > Given renaming slaves does not work anyway:
> I was actually thinking what if we relieve the rename restriction just
> for the failover slave? What the impact would be? I think users don't
> care about slave being renamed when it's in use, especially the initial
> rename. Thoughts?
>
> > would it work if we just
> > hard-coded slave names instead?
> >
> > E.g.
> > 1. fail slave renames
> > 2. rename of failover to XX automatically renames standby to XXnsby
> > and primary to XXnpry
> That wouldn't help. The time when the failover master gets renamed, the
> VF may not be present. I don't like the idea to delay exposing failover
> master until VF is hot plugged in (probably subject to various failures)
> later.
What netvsc does now is wait 2 seconds (to allow udev to do rename)
before bringing the VF link up. This works, has had no problems even
with slow distributions and is widely used.
A patch to allow ending the timeout after rename was proposed but
rejected.
https://lore.kernel.org/netdev/20171220223323.21125-1-sthemmin@microsoft.com/
Allow network devices to change name when up is too risky. There are things
like netfilter rules and other state in and out of the kernel that may break.
Userspace does not like it when the rules change.
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-27 22:38 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
David Miller
In-Reply-To: <d1060c75-eaba-ab6f-ff31-38cb3a47c711@oracle.com>
On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
>
>
> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> > >
> > > On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > > > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > > > cleanly, see:
> > > > > > > > >
> > > > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > > > >
> > > > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > > > on the slave netdevs specifically.
> > > > > > > > >
> > > > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > > > Above says:
> > > > > > > >
> > > > > > > > there's no motivation in the systemd/udevd community at
> > > > > > > > this point to refactor the rename logic and make it work well with
> > > > > > > > 3-netdev.
> > > > > > > >
> > > > > > > > What would the fix be? Skip slave devices?
> > > > > > > >
> > > > > > > There's nothing user can get if just skipping slave devices - the
> > > > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > > > earlier than the userspace is made aware of, and there's no
> > > > > > > negotiation protocol for kernel to know when userspace has done
> > > > > > > initial renaming of the interface. I would expect netdev list should
> > > > > > > at least provide the direction in general for how this can be
> > > > > > > solved...
> > > > I was just wondering what did you mean when you said
> > > > "refactor the rename logic and make it work well with 3-netdev" -
> > > > was there a proposal udev rejected?
> > > No. I never believed this particular issue can be fixed in userspace alone.
> > > Previously someone had said it could be, but I never see any work or
> > > relevant discussion ever happened in various userspace communities (for e.g.
> > > dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> > > of the issue derives from the kernel, it makes more sense to start from
> > > netdev, work out and decide on a solution: see what can be done in the
> > > kernel in order to fix it, then after that engage userspace community for
> > > the feasibility...
> > >
> > > > Anyway, can we write a time diagram for what happens in which order that
> > > > leads to failure? That would help look for triggers that we can tie
> > > > into, or add new ones.
> > > >
> > > See attached diagram.
> > >
> > > >
> > > >
> > > >
> > > > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > > > to only work with the master failover device.
> > > > > Where does this expectation come from?
> > > > >
> > > > > Admin users may have ethtool or tc configurations that need to deal with
> > > > > predictable interface name. Third-party app which was built upon specifying
> > > > > certain interface name can't be modified to chase dynamic names.
> > > > >
> > > > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > > > offload settings post boot for specific workload. Those images won't work
> > > > > well if the name is constantly changing just after couple rounds of live
> > > > > migration.
> > > > It should be possible to specify the ethtool configuration on the
> > > > master and have it automatically propagated to the slave.
> > > >
> > > > BTW this is something we should look at IMHO.
> > > I was elaborating a few examples that the expectation and assumption that
> > > user/admin scripts only deal with master failover device is incorrect. It
> > > had never been taken good care of, although I did try to emphasize it from
> > > the very beginning.
> > >
> > > Basically what you said about propagating the ethtool configuration down to
> > > the slave is the key pursuance of 1-netdev model. However, what I am seeking
> > > now is any alternative that can also fix the specific udev rename problem,
> > > before concluding that 1-netdev is the only solution. Generally a 1-netdev
> > > scheme would take time to implement, while I'm trying to find a way out to
> > > fix this particular naming problem under 3-netdev.
> > >
> > > > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > > > just the vehicle). However, I recall there was resistance around this
> > > > > discussion that even the concept of hiding itself is a taboo for Linux
> > > > > netdev. I would like to summon potential alternatives before concluding
> > > > > 1-netdev is the only solution too soon.
> > > > >
> > > > > Thanks,
> > > > > -Siwei
> > > > Your scripts would not work at all then, right?
> > > At this point we don't claim images with such usage as SR-IOV live
> > > migrate-able. We would flag it as live migrate-able until this ethtool
> > > config issue is fully addressed and a transparent live migration solution
> > > emerges in upstream eventually.
> > >
> > >
> > > Thanks,
> > > -Siwei
> > > >
> > > > > > > -Siwei
> > > > > > >
> > > > > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > >
> > > net_failover(kernel) | network.service (user) | systemd-udevd (user)
> > > --------------------------------------------------+------------------------------+--------------------------------------------
> > > (standby virtio-net and net_failover | |
> > > devices created and initialized, | |
> > > i.e. virtnet_probe()-> | |
> > > net_failover_create() | |
> > > was done.) | |
> > > | |
> > > | runs `ifup ens3' -> |
> > > | ip link set dev ens3 up |
> > > net_failover_open() | |
> > > dev_open(virtnet_dev) | |
> > > virtnet_open(virtnet_dev) | |
> > > netif_carrier_on(failover_dev) | |
> > > ... | |
> > > | |
> > > (VF hot plugged in) | |
> > > ixgbevf_probe() | |
> > > register_netdev(ixgbevf_netdev) | |
> > > netdev_register_kobject(ixgbevf_netdev) | |
> > > kobject_add(ixgbevf_dev) | |
> > > device_add(ixgbevf_dev) | |
> > > kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> > > netlink_broadcast() | |
> > > ... | |
> > > call_netdevice_notifiers(NETDEV_REGISTER) | |
> > > failover_event(..., NETDEV_REGISTER, ...) | |
> > > failover_slave_register(ixgbevf_netdev) | |
> > > net_failover_slave_register(ixgbevf_netdev) | |
> > > dev_open(ixgbevf_netdev) | |
> > > | |
> > > | |
> > > | | received ADD uevent from netlink fd
> > > | | ...
> > > | | udev-builtin-net_id.c:dev_pci_slot()
> > > | | (decided to renamed 'eth0' )
> > > | | ip link set dev eth0 name ens4
> > > (dev_change_name() returns -EBUSY as | |
> > > ixgbevf_netdev->flags has IFF_UP) | |
> > > | |
> > >
> > Given renaming slaves does not work anyway:
> I was actually thinking what if we relieve the rename restriction just for
> the failover slave? What the impact would be? I think users don't care about
> slave being renamed when it's in use, especially the initial rename.
> Thoughts?
>
> > would it work if we just
> > hard-coded slave names instead?
> >
> > E.g.
> > 1. fail slave renames
> > 2. rename of failover to XX automatically renames standby to XXnsby
> > and primary to XXnpry
> That wouldn't help. The time when the failover master gets renamed, the VF
> may not be present.
In this scheme if VF is not there it will be renamed immediately after registration.
> I don't like the idea to delay exposing failover master
> until VF is hot plugged in (probably subject to various failures) later.
>
> Thanks,
> -Siwei
I agree, this was not what I meant.
> >
> >
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Michael S. Tsirkin @ 2019-02-27 23:50 UTC (permalink / raw)
To: si-wei liu
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
David Miller
In-Reply-To: <c72ce9eb-254c-cc3e-1969-f7f108506d5e@oracle.com>
On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
>
>
> On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> > On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
> > >
> > > On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > > > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> > > > > On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > > > > > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > > > > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > > > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > > > > > cleanly, see:
> > > > > > > > > > >
> > > > > > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > > > > > >
> > > > > > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > > > > > on the slave netdevs specifically.
> > > > > > > > > > >
> > > > > > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > > > > > Above says:
> > > > > > > > > >
> > > > > > > > > > there's no motivation in the systemd/udevd community at
> > > > > > > > > > this point to refactor the rename logic and make it work well with
> > > > > > > > > > 3-netdev.
> > > > > > > > > >
> > > > > > > > > > What would the fix be? Skip slave devices?
> > > > > > > > > >
> > > > > > > > > There's nothing user can get if just skipping slave devices - the
> > > > > > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > > > > > earlier than the userspace is made aware of, and there's no
> > > > > > > > > negotiation protocol for kernel to know when userspace has done
> > > > > > > > > initial renaming of the interface. I would expect netdev list should
> > > > > > > > > at least provide the direction in general for how this can be
> > > > > > > > > solved...
> > > > > > I was just wondering what did you mean when you said
> > > > > > "refactor the rename logic and make it work well with 3-netdev" -
> > > > > > was there a proposal udev rejected?
> > > > > No. I never believed this particular issue can be fixed in userspace alone.
> > > > > Previously someone had said it could be, but I never see any work or
> > > > > relevant discussion ever happened in various userspace communities (for e.g.
> > > > > dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> > > > > of the issue derives from the kernel, it makes more sense to start from
> > > > > netdev, work out and decide on a solution: see what can be done in the
> > > > > kernel in order to fix it, then after that engage userspace community for
> > > > > the feasibility...
> > > > >
> > > > > > Anyway, can we write a time diagram for what happens in which order that
> > > > > > leads to failure? That would help look for triggers that we can tie
> > > > > > into, or add new ones.
> > > > > >
> > > > > See attached diagram.
> > > > >
> > > > > >
> > > > > >
> > > > > > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > > > > > to only work with the master failover device.
> > > > > > > Where does this expectation come from?
> > > > > > >
> > > > > > > Admin users may have ethtool or tc configurations that need to deal with
> > > > > > > predictable interface name. Third-party app which was built upon specifying
> > > > > > > certain interface name can't be modified to chase dynamic names.
> > > > > > >
> > > > > > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > > > > > offload settings post boot for specific workload. Those images won't work
> > > > > > > well if the name is constantly changing just after couple rounds of live
> > > > > > > migration.
> > > > > > It should be possible to specify the ethtool configuration on the
> > > > > > master and have it automatically propagated to the slave.
> > > > > >
> > > > > > BTW this is something we should look at IMHO.
> > > > > I was elaborating a few examples that the expectation and assumption that
> > > > > user/admin scripts only deal with master failover device is incorrect. It
> > > > > had never been taken good care of, although I did try to emphasize it from
> > > > > the very beginning.
> > > > >
> > > > > Basically what you said about propagating the ethtool configuration down to
> > > > > the slave is the key pursuance of 1-netdev model. However, what I am seeking
> > > > > now is any alternative that can also fix the specific udev rename problem,
> > > > > before concluding that 1-netdev is the only solution. Generally a 1-netdev
> > > > > scheme would take time to implement, while I'm trying to find a way out to
> > > > > fix this particular naming problem under 3-netdev.
> > > > >
> > > > > > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > > > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > > > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > > > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > > > > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > > > > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > > > > > just the vehicle). However, I recall there was resistance around this
> > > > > > > discussion that even the concept of hiding itself is a taboo for Linux
> > > > > > > netdev. I would like to summon potential alternatives before concluding
> > > > > > > 1-netdev is the only solution too soon.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > -Siwei
> > > > > > Your scripts would not work at all then, right?
> > > > > At this point we don't claim images with such usage as SR-IOV live
> > > > > migrate-able. We would flag it as live migrate-able until this ethtool
> > > > > config issue is fully addressed and a transparent live migration solution
> > > > > emerges in upstream eventually.
> > > > >
> > > > >
> > > > > Thanks,
> > > > > -Siwei
> > > > > > > > > -Siwei
> > > > > > > > >
> > > > > > > > >
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > >
> > > > > net_failover(kernel) | network.service (user) | systemd-udevd (user)
> > > > > --------------------------------------------------+------------------------------+--------------------------------------------
> > > > > (standby virtio-net and net_failover | |
> > > > > devices created and initialized, | |
> > > > > i.e. virtnet_probe()-> | |
> > > > > net_failover_create() | |
> > > > > was done.) | |
> > > > > | |
> > > > > | runs `ifup ens3' -> |
> > > > > | ip link set dev ens3 up |
> > > > > net_failover_open() | |
> > > > > dev_open(virtnet_dev) | |
> > > > > virtnet_open(virtnet_dev) | |
> > > > > netif_carrier_on(failover_dev) | |
> > > > > ... | |
> > > > > | |
> > > > > (VF hot plugged in) | |
> > > > > ixgbevf_probe() | |
> > > > > register_netdev(ixgbevf_netdev) | |
> > > > > netdev_register_kobject(ixgbevf_netdev) | |
> > > > > kobject_add(ixgbevf_dev) | |
> > > > > device_add(ixgbevf_dev) | |
> > > > > kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> > > > > netlink_broadcast() | |
> > > > > ... | |
> > > > > call_netdevice_notifiers(NETDEV_REGISTER) | |
> > > > > failover_event(..., NETDEV_REGISTER, ...) | |
> > > > > failover_slave_register(ixgbevf_netdev) | |
> > > > > net_failover_slave_register(ixgbevf_netdev) | |
> > > > > dev_open(ixgbevf_netdev) | |
> > > > > | |
> > > > > | |
> > > > > | | received ADD uevent from netlink fd
> > > > > | | ...
> > > > > | | udev-builtin-net_id.c:dev_pci_slot()
> > > > > | | (decided to renamed 'eth0' )
> > > > > | | ip link set dev eth0 name ens4
> > > > > (dev_change_name() returns -EBUSY as | |
> > > > > ixgbevf_netdev->flags has IFF_UP) | |
> > > > > | |
> > > > >
> > > > Given renaming slaves does not work anyway:
> > > I was actually thinking what if we relieve the rename restriction just for
> > > the failover slave? What the impact would be? I think users don't care about
> > > slave being renamed when it's in use, especially the initial rename.
> > > Thoughts?
> > >
> > > > would it work if we just
> > > > hard-coded slave names instead?
> > > >
> > > > E.g.
> > > > 1. fail slave renames
> > > > 2. rename of failover to XX automatically renames standby to XXnsby
> > > > and primary to XXnpry
> > > That wouldn't help. The time when the failover master gets renamed, the VF
> > > may not be present.
> > In this scheme if VF is not there it will be renamed immediately after registration.
> Who will be responsible to rename the slave, the kernel?
That's the idea.
> Note the master's
> name may or may not come from the userspace. If it comes from the userspace,
> should the userspace daemon change their expectation not to name/rename
> _any_ slaves (today there's no distinction)?
Yes the idea would be to fail renaming slaves.
> How do users know which name to
> trust, depending on which wins the race more often? Say if kernel wants a
> ens3npry name while userspace wants it named as ens4.
>
> -Siwei
With this approach kernel will deny attempts by userspace to rename
slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
will rename both slaves.
It seems pretty solid to me, the only issue is that in theory userspace
can use a name like XXXnsby for something else. But this seems unlikely.
> >
> > > I don't like the idea to delay exposing failover master
> > > until VF is hot plugged in (probably subject to various failures) later.
> > >
> > > Thanks,
> > > -Siwei
> >
> > I agree, this was not what I meant.
> >
> > > >
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Liran Alon @ 2019-02-28 0:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, Netdev, si-wei liu,
David Miller
In-Reply-To: <20190227184601-mutt-send-email-mst@kernel.org>
> On 28 Feb 2019, at 1:50, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
>>
>>
>> On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
>>> On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
>>>>
>>>> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
>>>>> On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
>>>>>> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
>>>>>>>> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
>>>>>>>>> On 2/21/2019 7:33 PM, si-wei liu wrote:
>>>>>>>>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
>>>>>>>>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
>>>>>>>>>>>> Sorry for replying to this ancient thread. There was some remaining
>>>>>>>>>>>> issue that I don't think the initial net_failover patch got addressed
>>>>>>>>>>>> cleanly, see:
>>>>>>>>>>>>
>>>>>>>>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.launchpad.net_ubuntu_-2Bsource_linux_-2Bbug_1815268&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=aL-QfUoSYx8r0XCOBkcDtF8f-cYxrJI3skYLFTb8XJE&s=yk6Nqv3a6_JMzyrXKY67h00FyNrDJyQ-PYMFffDSTXM&e=
>>>>>>>>>>>>
>>>>>>>>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
>>>>>>>>>>>> not specifically writtten for such kernel automatic enslavement.
>>>>>>>>>>>> Specifically, if it is a bond or team, the slave would typically get
>>>>>>>>>>>> renamed *before* virtual device gets created, that's what udev can
>>>>>>>>>>>> control (without getting netdev opened early by the other part of
>>>>>>>>>>>> kernel) and other userspace components for e.g. initramfs,
>>>>>>>>>>>> init-scripts can coordinate well in between. The in-kernel
>>>>>>>>>>>> auto-enslavement of net_failover breaks this userspace convention,
>>>>>>>>>>>> which don't provides a solution if user care about consistent naming
>>>>>>>>>>>> on the slave netdevs specifically.
>>>>>>>>>>>>
>>>>>>>>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
>>>>>>>>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
>>>>>>>>>>>> problem ever since. Please share your mind how to proceed and solve
>>>>>>>>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
>>>>>>>>>>> Above says:
>>>>>>>>>>>
>>>>>>>>>>> there's no motivation in the systemd/udevd community at
>>>>>>>>>>> this point to refactor the rename logic and make it work well with
>>>>>>>>>>> 3-netdev.
>>>>>>>>>>>
>>>>>>>>>>> What would the fix be? Skip slave devices?
>>>>>>>>>>>
>>>>>>>>>> There's nothing user can get if just skipping slave devices - the
>>>>>>>>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
>>>>>>>>>> next reboot, while the rest may conform to the naming scheme (ens3
>>>>>>>>>> and such). There's no way one can fix this in userspace alone - when
>>>>>>>>>> the failover is created the enslaved netdev was opened by the kernel
>>>>>>>>>> earlier than the userspace is made aware of, and there's no
>>>>>>>>>> negotiation protocol for kernel to know when userspace has done
>>>>>>>>>> initial renaming of the interface. I would expect netdev list should
>>>>>>>>>> at least provide the direction in general for how this can be
>>>>>>>>>> solved...
>>>>>>> I was just wondering what did you mean when you said
>>>>>>> "refactor the rename logic and make it work well with 3-netdev" -
>>>>>>> was there a proposal udev rejected?
>>>>>> No. I never believed this particular issue can be fixed in userspace alone.
>>>>>> Previously someone had said it could be, but I never see any work or
>>>>>> relevant discussion ever happened in various userspace communities (for e.g.
>>>>>> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
>>>>>> of the issue derives from the kernel, it makes more sense to start from
>>>>>> netdev, work out and decide on a solution: see what can be done in the
>>>>>> kernel in order to fix it, then after that engage userspace community for
>>>>>> the feasibility...
>>>>>>
>>>>>>> Anyway, can we write a time diagram for what happens in which order that
>>>>>>> leads to failure? That would help look for triggers that we can tie
>>>>>>> into, or add new ones.
>>>>>>>
>>>>>> See attached diagram.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
>>>>>>>>> to only work with the master failover device.
>>>>>>>> Where does this expectation come from?
>>>>>>>>
>>>>>>>> Admin users may have ethtool or tc configurations that need to deal with
>>>>>>>> predictable interface name. Third-party app which was built upon specifying
>>>>>>>> certain interface name can't be modified to chase dynamic names.
>>>>>>>>
>>>>>>>> Specifically, we have pre-canned image that uses ethtool to fine tune VF
>>>>>>>> offload settings post boot for specific workload. Those images won't work
>>>>>>>> well if the name is constantly changing just after couple rounds of live
>>>>>>>> migration.
>>>>>>> It should be possible to specify the ethtool configuration on the
>>>>>>> master and have it automatically propagated to the slave.
>>>>>>>
>>>>>>> BTW this is something we should look at IMHO.
>>>>>> I was elaborating a few examples that the expectation and assumption that
>>>>>> user/admin scripts only deal with master failover device is incorrect. It
>>>>>> had never been taken good care of, although I did try to emphasize it from
>>>>>> the very beginning.
>>>>>>
>>>>>> Basically what you said about propagating the ethtool configuration down to
>>>>>> the slave is the key pursuance of 1-netdev model. However, what I am seeking
>>>>>> now is any alternative that can also fix the specific udev rename problem,
>>>>>> before concluding that 1-netdev is the only solution. Generally a 1-netdev
>>>>>> scheme would take time to implement, while I'm trying to find a way out to
>>>>>> fix this particular naming problem under 3-netdev.
>>>>>>
>>>>>>>>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
>>>>>>>>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
>>>>>>>>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
>>>>>>>>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
>>>>>>>> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
>>>>>>>> model as much transparent to a real NIC as possible, while a hidden netns is
>>>>>>>> just the vehicle). However, I recall there was resistance around this
>>>>>>>> discussion that even the concept of hiding itself is a taboo for Linux
>>>>>>>> netdev. I would like to summon potential alternatives before concluding
>>>>>>>> 1-netdev is the only solution too soon.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> -Siwei
>>>>>>> Your scripts would not work at all then, right?
>>>>>> At this point we don't claim images with such usage as SR-IOV live
>>>>>> migrate-able. We would flag it as live migrate-able until this ethtool
>>>>>> config issue is fully addressed and a transparent live migration solution
>>>>>> emerges in upstream eventually.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> -Siwei
>>>>>>>>>> -Siwei
>>>>>>>>>>
>>>>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>>>>>
>>>>>> net_failover(kernel) | network.service (user) | systemd-udevd (user)
>>>>>> --------------------------------------------------+------------------------------+--------------------------------------------
>>>>>> (standby virtio-net and net_failover | |
>>>>>> devices created and initialized, | |
>>>>>> i.e. virtnet_probe()-> | |
>>>>>> net_failover_create() | |
>>>>>> was done.) | |
>>>>>> | |
>>>>>> | runs `ifup ens3' -> |
>>>>>> | ip link set dev ens3 up |
>>>>>> net_failover_open() | |
>>>>>> dev_open(virtnet_dev) | |
>>>>>> virtnet_open(virtnet_dev) | |
>>>>>> netif_carrier_on(failover_dev) | |
>>>>>> ... | |
>>>>>> | |
>>>>>> (VF hot plugged in) | |
>>>>>> ixgbevf_probe() | |
>>>>>> register_netdev(ixgbevf_netdev) | |
>>>>>> netdev_register_kobject(ixgbevf_netdev) | |
>>>>>> kobject_add(ixgbevf_dev) | |
>>>>>> device_add(ixgbevf_dev) | |
>>>>>> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
>>>>>> netlink_broadcast() | |
>>>>>> ... | |
>>>>>> call_netdevice_notifiers(NETDEV_REGISTER) | |
>>>>>> failover_event(..., NETDEV_REGISTER, ...) | |
>>>>>> failover_slave_register(ixgbevf_netdev) | |
>>>>>> net_failover_slave_register(ixgbevf_netdev) | |
>>>>>> dev_open(ixgbevf_netdev) | |
>>>>>> | |
>>>>>> | |
>>>>>> | | received ADD uevent from netlink fd
>>>>>> | | ...
>>>>>> | | udev-builtin-net_id.c:dev_pci_slot()
>>>>>> | | (decided to renamed 'eth0' )
>>>>>> | | ip link set dev eth0 name ens4
>>>>>> (dev_change_name() returns -EBUSY as | |
>>>>>> ixgbevf_netdev->flags has IFF_UP) | |
>>>>>> | |
>>>>>>
>>>>> Given renaming slaves does not work anyway:
>>>> I was actually thinking what if we relieve the rename restriction just for
>>>> the failover slave? What the impact would be? I think users don't care about
>>>> slave being renamed when it's in use, especially the initial rename.
>>>> Thoughts?
>>>>
>>>>> would it work if we just
>>>>> hard-coded slave names instead?
>>>>>
>>>>> E.g.
>>>>> 1. fail slave renames
>>>>> 2. rename of failover to XX automatically renames standby to XXnsby
>>>>> and primary to XXnpry
>>>> That wouldn't help. The time when the failover master gets renamed, the VF
>>>> may not be present.
>>> In this scheme if VF is not there it will be renamed immediately after registration.
>> Who will be responsible to rename the slave, the kernel?
>
> That's the idea.
>
>> Note the master's
>> name may or may not come from the userspace. If it comes from the userspace,
>> should the userspace daemon change their expectation not to name/rename
>> _any_ slaves (today there's no distinction)?
>
> Yes the idea would be to fail renaming slaves.
>
>> How do users know which name to
>> trust, depending on which wins the race more often? Say if kernel wants a
>> ens3npry name while userspace wants it named as ens4.
>>
>> -Siwei
>
> With this approach kernel will deny attempts by userspace to rename
> slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> will rename both slaves.
>
> It seems pretty solid to me, the only issue is that in theory userspace
> can use a name like XXXnsby for something else. But this seems unlikely.
I’m fond of this idea and I have similar opinion.
I think it simplifies the issue here.
I don’t see a real reason for customer to define udev rule to rename a net-failover slave to have different postfix.
-Liran
>
>
>>>
>>>> I don't like the idea to delay exposing failover master
>>>> until VF is hot plugged in (probably subject to various failures) later.
>>>>
>>>> Thanks,
>>>> -Siwei
>>>
>>> I agree, this was not what I meant.
>>>
>>>>>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: Stephen Hemminger @ 2019-02-28 0:03 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, Jiri Pirko, Jakub Kicinski,
Samudrala, Sridhar, virtualization, Siwei Liu, liran.alon, Netdev,
si-wei liu, David Miller
In-Reply-To: <20190227184601-mutt-send-email-mst@kernel.org>
On Wed, 27 Feb 2019 18:50:44 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
> >
> >
> > On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> > > On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
> > > >
> > > > On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
> > > > > On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
> > > > > > On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
> > > > > > > On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
> > > > > > > > On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
> > > > > > > > > On 2/21/2019 7:33 PM, si-wei liu wrote:
> > > > > > > > > > On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
> > > > > > > > > > > On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
> > > > > > > > > > > > Sorry for replying to this ancient thread. There was some remaining
> > > > > > > > > > > > issue that I don't think the initial net_failover patch got addressed
> > > > > > > > > > > > cleanly, see:
> > > > > > > > > > > >
> > > > > > > > > > > > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
> > > > > > > > > > > >
> > > > > > > > > > > > The renaming of 'eth0' to 'ens4' fails because the udev userspace was
> > > > > > > > > > > > not specifically writtten for such kernel automatic enslavement.
> > > > > > > > > > > > Specifically, if it is a bond or team, the slave would typically get
> > > > > > > > > > > > renamed *before* virtual device gets created, that's what udev can
> > > > > > > > > > > > control (without getting netdev opened early by the other part of
> > > > > > > > > > > > kernel) and other userspace components for e.g. initramfs,
> > > > > > > > > > > > init-scripts can coordinate well in between. The in-kernel
> > > > > > > > > > > > auto-enslavement of net_failover breaks this userspace convention,
> > > > > > > > > > > > which don't provides a solution if user care about consistent naming
> > > > > > > > > > > > on the slave netdevs specifically.
> > > > > > > > > > > >
> > > > > > > > > > > > Previously this issue had been specifically called out when IFF_HIDDEN
> > > > > > > > > > > > and the 1-netdev was proposed, but no one gives out a solution to this
> > > > > > > > > > > > problem ever since. Please share your mind how to proceed and solve
> > > > > > > > > > > > this userspace issue if netdev does not welcome a 1-netdev model.
> > > > > > > > > > > Above says:
> > > > > > > > > > >
> > > > > > > > > > > there's no motivation in the systemd/udevd community at
> > > > > > > > > > > this point to refactor the rename logic and make it work well with
> > > > > > > > > > > 3-netdev.
> > > > > > > > > > >
> > > > > > > > > > > What would the fix be? Skip slave devices?
> > > > > > > > > > >
> > > > > > > > > > There's nothing user can get if just skipping slave devices - the
> > > > > > > > > > name is still unchanged and unpredictable e.g. eth0, or eth1 the
> > > > > > > > > > next reboot, while the rest may conform to the naming scheme (ens3
> > > > > > > > > > and such). There's no way one can fix this in userspace alone - when
> > > > > > > > > > the failover is created the enslaved netdev was opened by the kernel
> > > > > > > > > > earlier than the userspace is made aware of, and there's no
> > > > > > > > > > negotiation protocol for kernel to know when userspace has done
> > > > > > > > > > initial renaming of the interface. I would expect netdev list should
> > > > > > > > > > at least provide the direction in general for how this can be
> > > > > > > > > > solved...
> > > > > > > I was just wondering what did you mean when you said
> > > > > > > "refactor the rename logic and make it work well with 3-netdev" -
> > > > > > > was there a proposal udev rejected?
> > > > > > No. I never believed this particular issue can be fixed in userspace alone.
> > > > > > Previously someone had said it could be, but I never see any work or
> > > > > > relevant discussion ever happened in various userspace communities (for e.g.
> > > > > > dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
> > > > > > of the issue derives from the kernel, it makes more sense to start from
> > > > > > netdev, work out and decide on a solution: see what can be done in the
> > > > > > kernel in order to fix it, then after that engage userspace community for
> > > > > > the feasibility...
> > > > > >
> > > > > > > Anyway, can we write a time diagram for what happens in which order that
> > > > > > > leads to failure? That would help look for triggers that we can tie
> > > > > > > into, or add new ones.
> > > > > > >
> > > > > > See attached diagram.
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > Is there an issue if slave device names are not predictable? The user/admin scripts are expected
> > > > > > > > > to only work with the master failover device.
> > > > > > > > Where does this expectation come from?
> > > > > > > >
> > > > > > > > Admin users may have ethtool or tc configurations that need to deal with
> > > > > > > > predictable interface name. Third-party app which was built upon specifying
> > > > > > > > certain interface name can't be modified to chase dynamic names.
> > > > > > > >
> > > > > > > > Specifically, we have pre-canned image that uses ethtool to fine tune VF
> > > > > > > > offload settings post boot for specific workload. Those images won't work
> > > > > > > > well if the name is constantly changing just after couple rounds of live
> > > > > > > > migration.
> > > > > > > It should be possible to specify the ethtool configuration on the
> > > > > > > master and have it automatically propagated to the slave.
> > > > > > >
> > > > > > > BTW this is something we should look at IMHO.
> > > > > > I was elaborating a few examples that the expectation and assumption that
> > > > > > user/admin scripts only deal with master failover device is incorrect. It
> > > > > > had never been taken good care of, although I did try to emphasize it from
> > > > > > the very beginning.
> > > > > >
> > > > > > Basically what you said about propagating the ethtool configuration down to
> > > > > > the slave is the key pursuance of 1-netdev model. However, what I am seeking
> > > > > > now is any alternative that can also fix the specific udev rename problem,
> > > > > > before concluding that 1-netdev is the only solution. Generally a 1-netdev
> > > > > > scheme would take time to implement, while I'm trying to find a way out to
> > > > > > fix this particular naming problem under 3-netdev.
> > > > > >
> > > > > > > > > Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
> > > > > > > > > about moving them to a hidden network namespace so that they are not visible from the default namespace.
> > > > > > > > > I looked into this sometime back, but did not find the right kernel api to create a network namespace within
> > > > > > > > > kernel. If so, we could use this mechanism to simulate a 1-netdev model.
> > > > > > > > Yes, that's one possible implementation (IMHO the key is to make 1-netdev
> > > > > > > > model as much transparent to a real NIC as possible, while a hidden netns is
> > > > > > > > just the vehicle). However, I recall there was resistance around this
> > > > > > > > discussion that even the concept of hiding itself is a taboo for Linux
> > > > > > > > netdev. I would like to summon potential alternatives before concluding
> > > > > > > > 1-netdev is the only solution too soon.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > -Siwei
> > > > > > > Your scripts would not work at all then, right?
> > > > > > At this point we don't claim images with such usage as SR-IOV live
> > > > > > migrate-able. We would flag it as live migrate-able until this ethtool
> > > > > > config issue is fully addressed and a transparent live migration solution
> > > > > > emerges in upstream eventually.
> > > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > -Siwei
> > > > > > > > > > -Siwei
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > > >
> > > > > > net_failover(kernel) | network.service (user) | systemd-udevd (user)
> > > > > > --------------------------------------------------+------------------------------+--------------------------------------------
> > > > > > (standby virtio-net and net_failover | |
> > > > > > devices created and initialized, | |
> > > > > > i.e. virtnet_probe()-> | |
> > > > > > net_failover_create() | |
> > > > > > was done.) | |
> > > > > > | |
> > > > > > | runs `ifup ens3' -> |
> > > > > > | ip link set dev ens3 up |
> > > > > > net_failover_open() | |
> > > > > > dev_open(virtnet_dev) | |
> > > > > > virtnet_open(virtnet_dev) | |
> > > > > > netif_carrier_on(failover_dev) | |
> > > > > > ... | |
> > > > > > | |
> > > > > > (VF hot plugged in) | |
> > > > > > ixgbevf_probe() | |
> > > > > > register_netdev(ixgbevf_netdev) | |
> > > > > > netdev_register_kobject(ixgbevf_netdev) | |
> > > > > > kobject_add(ixgbevf_dev) | |
> > > > > > device_add(ixgbevf_dev) | |
> > > > > > kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
> > > > > > netlink_broadcast() | |
> > > > > > ... | |
> > > > > > call_netdevice_notifiers(NETDEV_REGISTER) | |
> > > > > > failover_event(..., NETDEV_REGISTER, ...) | |
> > > > > > failover_slave_register(ixgbevf_netdev) | |
> > > > > > net_failover_slave_register(ixgbevf_netdev) | |
> > > > > > dev_open(ixgbevf_netdev) | |
> > > > > > | |
> > > > > > | |
> > > > > > | | received ADD uevent from netlink fd
> > > > > > | | ...
> > > > > > | | udev-builtin-net_id.c:dev_pci_slot()
> > > > > > | | (decided to renamed 'eth0' )
> > > > > > | | ip link set dev eth0 name ens4
> > > > > > (dev_change_name() returns -EBUSY as | |
> > > > > > ixgbevf_netdev->flags has IFF_UP) | |
> > > > > > | |
> > > > > >
> > > > > Given renaming slaves does not work anyway:
> > > > I was actually thinking what if we relieve the rename restriction just for
> > > > the failover slave? What the impact would be? I think users don't care about
> > > > slave being renamed when it's in use, especially the initial rename.
> > > > Thoughts?
> > > >
> > > > > would it work if we just
> > > > > hard-coded slave names instead?
> > > > >
> > > > > E.g.
> > > > > 1. fail slave renames
> > > > > 2. rename of failover to XX automatically renames standby to XXnsby
> > > > > and primary to XXnpry
> > > > That wouldn't help. The time when the failover master gets renamed, the VF
> > > > may not be present.
> > > In this scheme if VF is not there it will be renamed immediately after registration.
> > Who will be responsible to rename the slave, the kernel?
>
> That's the idea.
>
> > Note the master's
> > name may or may not come from the userspace. If it comes from the userspace,
> > should the userspace daemon change their expectation not to name/rename
> > _any_ slaves (today there's no distinction)?
>
> Yes the idea would be to fail renaming slaves.
>
> > How do users know which name to
> > trust, depending on which wins the race more often? Say if kernel wants a
> > ens3npry name while userspace wants it named as ens4.
> >
> > -Siwei
>
> With this approach kernel will deny attempts by userspace to rename
> slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> will rename both slaves.
>
> It seems pretty solid to me, the only issue is that in theory userspace
> can use a name like XXXnsby for something else. But this seems unlikely.
Similar schemes (with kernel providing naming) were also previously rejected
upstream. It has been a consistent theme that the kernel should not be in
the renaming business. It will certainly break userspace.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox