* Re: [PATCH v3] virtio: update the comments for transport features
From: Stefan Hajnoczi @ 2018-06-07 13:04 UTC (permalink / raw)
To: Tiwei Bie; +Cc: virtio-dev, mst, cohuck, linux-kernel, virtualization
In-Reply-To: <20180601112632.12651-1-tiwei.bie@intel.com>
[-- Attachment #1.1: Type: text/plain, Size: 776 bytes --]
On Fri, Jun 01, 2018 at 07:26:32PM +0800, Tiwei Bie wrote:
> The existing comments for transport features are outdated.
> So update them to address the latest changes in the spec.
>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> ---
> This patch is generated on top of below patch:
> https://lists.oasis-open.org/archives/virtio-dev/201805/msg00212.html
>
> v3:
> - Fix a typo in the commit message (Cornelia);
>
> v2:
> - Improve the comments (Cornelia);
> - Improve the commit message;
>
> include/uapi/linux/virtio_config.h | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC v6 4/5] virtio_ring: add event idx support in packed ring
From: Jason Wang @ 2018-06-07 9:50 UTC (permalink / raw)
To: Tiwei Bie, mst, virtualization, linux-kernel, netdev; +Cc: wexu
In-Reply-To: <20180605074046.20709-5-tiwei.bie@intel.com>
On 2018年06月05日 15:40, Tiwei Bie wrote:
> static bool virtqueue_enable_cb_delayed_packed(struct virtqueue *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> + u16 bufs, used_idx, wrap_counter;
>
> START_USE(vq);
>
> /* We optimistically turn back on interrupts, then check if there was
> * more to do. */
> + /* Depending on the VIRTIO_RING_F_EVENT_IDX feature, we need to
> + * either clear the flags bit or point the event index at the next
> + * entry. Always update the event index to keep code simple. */
> +
Maybe for packed ring, it's time to treat event index separately to
avoid a virtio_wmb() for event idx is off.
> + /* TODO: tune this threshold */
> + if (vq->next_avail_idx < vq->last_used_idx)
> + bufs = (vq->vring_packed.num + vq->next_avail_idx -
> + vq->last_used_idx) * 3 / 4;
> + else
> + bufs = (vq->next_avail_idx - vq->last_used_idx) * 3 / 4;
vq->next_avail-idx could be equal to vq->last_usd_idx when the ring is
full. Though virito-net is the only user now and it can guarantee this
won't happen. But consider this is a core API, we should make sure it
can work for any cases.
It looks to me that bufs is just vq->vring_packed.num - vq->num_free?
> +
> + wrap_counter = vq->used_wrap_counter;
> +
> + used_idx = vq->last_used_idx + bufs;
> + if (used_idx >= vq->vring_packed.num) {
> + used_idx -= vq->vring_packed.num;
> + wrap_counter ^= 1;
> + }
> +
> + vq->vring_packed.driver->off_wrap = cpu_to_virtio16(_vq->vdev,
> + used_idx | (wrap_counter << 15));
>
> if (vq->event_flags_shadow == VRING_EVENT_F_DISABLE) {
> - vq->event_flags_shadow = VRING_EVENT_F_ENABLE;
> + /* We need to update event offset and event wrap
> + * counter first before updating event flags. */
> + virtio_wmb(vq->weak_barriers);
> + vq->event_flags_shadow = vq->event ? VRING_EVENT_F_DESC :
> + VRING_EVENT_F_ENABLE;
> vq->vring_packed.driver->flags = cpu_to_virtio16(_vq->vdev,
> vq->event_flags_shadow);
> - /* We need to enable interrupts first before re-checking
> - * for more used buffers. */
> - virtio_mb(vq->weak_barriers);
> }
>
> + /* We need to update event suppression structure first
> + * before re-checking for more used buffers. */
> + virtio_mb(vq->weak_barriers);
> +
> if (more_used_packed(vq)) {
> END_USE(vq);
> return false;
I think what we need to to make sure the descriptor used_idx is used?
Otherwise we may stop and restart qdisc too frequently?
Thanks
> --
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC V2] virtio: Add platform specific DMA API translation for virito devices
From: Christoph Hellwig @ 2018-06-07 5:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: robh, pawel.moll, Tom Lendacky, cohuck, Ram Pai, linux-kernel,
virtualization, hch, joe, Rustad, Mark D, david, linuxppc-dev,
elfring, Anshuman Khandual
In-Reply-To: <20180531204320-mutt-send-email-mst@kernel.org>
On Thu, May 31, 2018 at 08:43:58PM +0300, Michael S. Tsirkin wrote:
> Pls work on a long term solution. Short term needs can be served by
> enabling the iommu platform in qemu.
So, I spent some time looking at converting virtio to dma ops overrides,
and the current virtio spec, and the sad through I have to tell is that
both the spec and the Linux implementation are complete and utterly fucked
up.
Both in the flag naming and the implementation there is an implication
of DMA API == IOMMU, which is fundamentally wrong.
The DMA API does a few different things:
a) address translation
This does include IOMMUs. But it also includes random offsets
between PCI bars and system memory that we see on various
platforms. Worse so some of these offsets might be based on
banks, e.g. on the broadcom bmips platform. It also deals
with bitmask in physical addresses related to memory encryption
like AMD SEV. I'd be really curious how for example the
Intel virtio based NIC is going to work on any of those
plaforms.
b) coherency
On many architectures DMA is not cache coherent, and we need
to invalidate and/or write back cache lines before doing
DMA. Again, I wonder how this is every going to work with
hardware based virtio implementations. Even worse I think this
is actually broken at least for VIVT event for virtualized
implementations. E.g. a KVM guest is going to access memory
using different virtual addresses than qemu, vhost might throw
in another different address space.
c) bounce buffering
Many DMA implementations can not address all physical memory
due to addressing limitations. In such cases we copy the
DMA memory into a known addressable bounc buffer and DMA
from there.
d) flushing write combining buffers or similar
On some hardware platforms we need workarounds to e.g. read
from a certain mmio address to make sure DMA can actually
see memory written by the host.
All of this is bypassed by virtio by default despite generally being
platform issues, not particular to a given device.
^ permalink raw reply
* Re: [libvirt] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Samudrala, Sridhar @ 2018-06-06 19:39 UTC (permalink / raw)
To: Ján Tomko
Cc: alexander.h.duyck, virtio-dev, mst, qemu-devel, virtualization,
libvir-list, laine
In-Reply-To: <20180606185214.GH6767@dnr>
On 6/6/2018 11:52 AM, Ján Tomko wrote:
> On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote:
>> On 6/4/2018 7:06 PM, Jason Wang wrote:
>>>
>>>
>>> On 2018年06月05日 09:41, Samudrala, Sridhar wrote:
>>>> Ping on this patch now that the kernel patches are accepted into
>>>> davem's net-next tree.
>>>> https://patchwork.ozlabs.org/cover/920005/
>>>>
>>>>
>>>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>>>>> This feature bit can be used by hypervisor to indicate virtio_net
>>>>> device to
>>>>> act as a standby for another device with the same MAC address.
>>>>>
>>>>> I tested this with a small change to the patch to mark the STANDBY
>>>>> feature 'true'
>>>>> by default as i am using libvirt to start the VMs.
>>>>> Is there a way to pass the newly added feature bit 'standby' to qemu
>>>>> via libvirt
>>>>> XML file?
>>>>>
>>>
>>> Maybe you can try qemu command line passthrough:
>>>
>>> https://libvirt.org/drvqemu.html#qemucommand
>>
>> It looks like this can be used to pass command line arguments to qemu.
>> Is it possible to specify a virtio specific attribute via this method?
>>
>
> Yes, for testing purposes you should be able to do this via using QEMU's
> -set command line argument:
> http://blog.vmsplice.net/2011/04/how-to-pass-qemu-command-line-options.html
>
> i.e.:
>
> <domain type='kvm'
> xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
> ...
> <qemu:commandline>
> <qemu:arg value='-set'/>
> <qemu:arg value='device.net0.standby=on'/>
> </qemu:commandline>
> </domain>
Thanks. Will try this.
>
>> For ex: to say mrg_rxbuf is off we can add the following line to virtio
>> section of the domain xml file.
>> <host mrg_rxbuf='off'/>
>>
>> I think libvirt needs to be extended to to support the new 'standby'
>> attribute
>> via this mechanism.
>> Adding Liane Stump and libvirt to the CC list.
>
> *Laine
>
>>
>> Michael,
>> Can we start with getting this patch into Qemu and an update to
>> libvirt to
>> support the 'standby' feature so that this feature can be enabled via
>> some scripts/orchestration layer for now.
>>
>> We could improve this solution by enhancing Qemu to do automatic
>> management of the
>> addition/deletion of the primary device based on feature negotiation
>> as a later patch.
>>
>
> If that means the libvirt attribute would no longer be needed, I don't
> see the reason to add it to libvirt in the first place.
I think we still need this attribute supported via libvirt so that a user/admin
can enable this feature via domain XML specification.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-06 18:53 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, libvir-list, qemu-devel, laine,
virtualization
In-Reply-To: <8e411566-1146-03a2-5372-8f8134ee0ce6@intel.com>
On Wed, Jun 06, 2018 at 11:17:36AM -0700, Samudrala, Sridhar wrote:
> On 6/4/2018 7:06 PM, Jason Wang wrote:
> >
> >
> > On 2018年06月05日 09:41, Samudrala, Sridhar wrote:
> > > Ping on this patch now that the kernel patches are accepted into
> > > davem's net-next tree.
> > > https://patchwork.ozlabs.org/cover/920005/
> > >
> > >
> > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> > > > This feature bit can be used by hypervisor to indicate
> > > > virtio_net device to
> > > > act as a standby for another device with the same MAC address.
> > > >
> > > > I tested this with a small change to the patch to mark the
> > > > STANDBY feature 'true'
> > > > by default as i am using libvirt to start the VMs.
> > > > Is there a way to pass the newly added feature bit 'standby' to
> > > > qemu via libvirt
> > > > XML file?
> > > >
> >
> > Maybe you can try qemu command line passthrough:
> >
> > https://libvirt.org/drvqemu.html#qemucommand
>
> It looks like this can be used to pass command line arguments to qemu.
> Is it possible to specify a virtio specific attribute via this method?
>
> For ex: to say mrg_rxbuf is off we can add the following line to virtio
> section of the domain xml file.
> <host mrg_rxbuf='off'/>
>
> I think libvirt needs to be extended to to support the new 'standby' attribute
> via this mechanism.
> Adding Liane Stump and libvirt to the CC list.
>
> Michael,
> Can we start with getting this patch into Qemu and an update to libvirt to
> support the 'standby' feature so that this feature can be enabled via
> some scripts/orchestration layer for now.
Unfortunately we don't give the hypothetical orchestration layer
enough info about driver being bound, so it does not know
when is it safe to add a primary. And a similar issue around reset.
We could add events for that which would be a small patch,
but the issue then is that guest can trigger a storm of
these events.
> We could improve this solution by enhancing Qemu to do automatic management of the
> addition/deletion of the primary device based on feature negotiation as a later patch.
I'm not sure what the point of the back and forth would be though.
Typically after people invest in a specific config and get it working,
it's very hard to move them to another solution.
>
> >
> > The patch looks good to me. Let's see if Michael have any comment.
> >
> > Thanks
> >
> > > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > > > ---
> > > > hw/net/virtio-net.c | 2 ++
> > > > include/standard-headers/linux/virtio_net.h | 3 +++
> > > > 2 files changed, 5 insertions(+)
> > > >
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 90502fca7c..38b3140670 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
> > > > true),
> > > > DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
> > > > SPEED_UNKNOWN),
> > > > DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> > > > + DEFINE_PROP_BIT64("standby", VirtIONet, host_features,
> > > > VIRTIO_NET_F_STANDBY,
> > > > + false),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > > diff --git a/include/standard-headers/linux/virtio_net.h
> > > > b/include/standard-headers/linux/virtio_net.h
> > > > index e9f255ea3f..01ec09684c 100644
> > > > --- a/include/standard-headers/linux/virtio_net.h
> > > > +++ b/include/standard-headers/linux/virtio_net.h
> > > > @@ -57,6 +57,9 @@
> > > > * Steering */
> > > > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> > > > +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for
> > > > another device
> > > > + * with the same MAC.
> > > > + */
> > > > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set
> > > > linkspeed and duplex */
> > > > #ifndef VIRTIO_NET_NO_LEGACY
> > >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Samudrala, Sridhar @ 2018-06-06 18:17 UTC (permalink / raw)
To: Jason Wang, mst, virtualization, virtio-dev, jesse.brandeburg,
alexander.h.duyck, qemu-devel, laine, libvir-list
In-Reply-To: <d61c88c7-0803-ba1c-08ab-fc668cffa40b@redhat.com>
On 6/4/2018 7:06 PM, Jason Wang wrote:
>
>
> On 2018年06月05日 09:41, Samudrala, Sridhar wrote:
>> Ping on this patch now that the kernel patches are accepted into
>> davem's net-next tree.
>> https://patchwork.ozlabs.org/cover/920005/
>>
>>
>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>>> This feature bit can be used by hypervisor to indicate virtio_net
>>> device to
>>> act as a standby for another device with the same MAC address.
>>>
>>> I tested this with a small change to the patch to mark the STANDBY
>>> feature 'true'
>>> by default as i am using libvirt to start the VMs.
>>> Is there a way to pass the newly added feature bit 'standby' to qemu
>>> via libvirt
>>> XML file?
>>>
>
> Maybe you can try qemu command line passthrough:
>
> https://libvirt.org/drvqemu.html#qemucommand
It looks like this can be used to pass command line arguments to qemu.
Is it possible to specify a virtio specific attribute via this method?
For ex: to say mrg_rxbuf is off we can add the following line to virtio
section of the domain xml file.
<host mrg_rxbuf='off'/>
I think libvirt needs to be extended to to support the new 'standby' attribute
via this mechanism.
Adding Liane Stump and libvirt to the CC list.
Michael,
Can we start with getting this patch into Qemu and an update to libvirt to
support the 'standby' feature so that this feature can be enabled via
some scripts/orchestration layer for now.
We could improve this solution by enhancing Qemu to do automatic management of the
addition/deletion of the primary device based on feature negotiation as a later patch.
>
> The patch looks good to me. Let's see if Michael have any comment.
>
> Thanks
>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>> ---
>>> hw/net/virtio-net.c | 2 ++
>>> include/standard-headers/linux/virtio_net.h | 3 +++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 90502fca7c..38b3140670 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>>> true),
>>> DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
>>> SPEED_UNKNOWN),
>>> DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>> + DEFINE_PROP_BIT64("standby", VirtIONet, host_features,
>>> VIRTIO_NET_F_STANDBY,
>>> + false),
>>> DEFINE_PROP_END_OF_LIST(),
>>> };
>>> diff --git a/include/standard-headers/linux/virtio_net.h
>>> b/include/standard-headers/linux/virtio_net.h
>>> index e9f255ea3f..01ec09684c 100644
>>> --- a/include/standard-headers/linux/virtio_net.h
>>> +++ b/include/standard-headers/linux/virtio_net.h
>>> @@ -57,6 +57,9 @@
>>> * Steering */
>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>> +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for
>>> another device
>>> + * with the same MAC.
>>> + */
>>> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed
>>> and duplex */
>>> #ifndef VIRTIO_NET_NO_LEGACY
>>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
From: hpa @ 2018-06-06 16:39 UTC (permalink / raw)
To: sedat.dilek
Cc: linux-efi, brijesh.singh, boris.ostrovsky, jan.kiszka,
will.deacon, jarkko.sakkinen, virtualization, yamada.masahiro,
manojgupta, akataria, tweek, mawilcox, x86, ghackmann, mka, geert,
rientjes, aryabinin, thomas.lendacky, Arnd Bergmann, linux-kbuild,
keescook, rostedt, caoj.fnst, michal.lkml, jpoimboe, tglx, mingo,
jgross, astrachan, ard.biesheuvel, gregkh, tstellar,
Nick Desaulniers, linux-kernel, mjg59
In-Reply-To: <CA+icZUXEpe954z0TSbhSJPdFCfOqsBtpO9mPZBcJ2HWubSo_zQ@mail.gmail.com>
On June 6, 2018 1:05:45 AM PDT, Sedat Dilek <sedat.dilek@gmail.com> wrote:
>Hi,
>
>when discovering 'gnu_inline', I found ...
>
>$ git grep -w __FORTIFY_INLINE
>include/linux/string.h:#define __FORTIFY_INLINE extern __always_inline
>__attribute__((gnu_inline))
>include/linux/string.h:__FORTIFY_INLINE char *strncpy(char *p, const
>char *q, __kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE char *strcat(char *p, const
>char *q)
>include/linux/string.h:__FORTIFY_INLINE __kernel_size_t strlen(const
>char *p)
>include/linux/string.h:__FORTIFY_INLINE __kernel_size_t strnlen(const
>char *p, __kernel_size_t maxlen)
>include/linux/string.h:__FORTIFY_INLINE size_t strlcpy(char *p, const
>char *q, size_t size)
>include/linux/string.h:__FORTIFY_INLINE char *strncat(char *p, const
>char *q, __kernel_size_t count)
>include/linux/string.h:__FORTIFY_INLINE void *memset(void *p, int c,
>__kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE void *memcpy(void *p, const
>void *q, __kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE void *memmove(void *p, const
>void *q, __kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE void *memscan(void *p, int c,
>__kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE int memcmp(const void *p,
>const void *q, __kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE void *memchr(const void *p,
>int c, __kernel_size_t size)
>include/linux/string.h:__FORTIFY_INLINE void *memchr_inv(const void
>*p, int c, size_t size)
>include/linux/string.h:__FORTIFY_INLINE void *kmemdup(const void *p,
>size_t size, gfp_t gfp)
>include/linux/string.h:__FORTIFY_INLINE char *strcpy(char *p, const
>char *q)
>
>After the inline changes suggested by Joe this can be adapted?
>
>Beyond this, a general question: Can someone explain why all these
>inline defines are in compiler-gcc.h (as there exists compiler.h and
>compiler-clang.h)?
>
>Thanks.
>
>Regards,
>- Sedat -
Because gcc itself also supports both GNU89-style and C99-style inlines, but the kernel was built with the former, and it is not necessarily a trivial modification, except for "static inline" which is the same for both.
The other option is to pass -fgnu89-inline on the command line, which is supported by both gcc and clang. The two methods are fully equivalent.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs
From: Tiwei Bie @ 2018-06-06 15:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, stefanha, zhihong.wang, bhelgaas, mark.d.rustad
In-Reply-To: <20180606172139-mutt-send-email-mst@kernel.org>
On Wed, Jun 06, 2018 at 05:27:07PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2018 at 10:19:43PM +0800, Tiwei Bie wrote:
> > On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > > > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei Bie wrote:
> > > > > > > > There is a new feature bit allocated in virtio spec to
> > > > > > > > support SR-IOV (Single Root I/O Virtualization):
> > > > > > > >
> > > > > > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > > > > >
> > > > > > > > This patch enables the support for this feature bit in
> > > > > > > > virtio driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > > > > > ---
> > > > > > >
> > > > > > > OK but what about freeze/restore functions?
> > > > >
> > > > > So for restore, don't you need to restore the
> > > > > sriov capability?
> > > >
> > > > Currently I'm not familiar with the PM part.
> > > > But I still think the sriov capability should
> > > > be handled by PCI core.
> > >
> > > OK but the point is restore looks just like power up for device.
> > >
> > > > I'm trying to understand
> > > > all the relevant code..
> > > > For your question, based on what I found from
> > > > the code currently, I guess the sriov capability
> > > > will be restored by pci_restore_state() which
> > > > will be called by the ops in pci_dev_pm_ops.
> > > > The sriov_restore_state() will be called
> > > > eventually.
> > > >
> > > > Best regards,
> > > > Tiwei Bie
> > >
> > > Right but my point is during resume SRIOV gets enabled first before
> > > driver ok.
> > >
> > > Maybe we should relax the requirements in the spec:
> > > - only require FEATURES_OK from device, not DRIVER_OK from driver
> > > - explain that it only has to happen once, not on each reset,
> > > and driver can remember the result
> >
> > I got your point now! I'd like to relax the
> > requirements in the spec.
> >
> > Best regards,
> > Tiwei Bie
>
> Well the ballot approving your change closed. I think we should apply
> the first chunks reserving the feature bit then, and defer the rest, and
> you can work on new wording documenting the actual behaviour with a new
> github issue to track that - does this make sense?
> Let's do it quickly though - I don't want to bother the
> TC with re-voting the deferral, then the new patch.
Yeah. It makes sense! I think it's a good idea
to reserve the feature bit first and defer the
rest. I'll work on the new wording documenting
the actual behavior with a new github issue to
track it. Thanks a lot!
Best regards,
Tiwei Bie
>
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > > I also wonder about kexec - virtio.c currently does:
> > > > > > >
> > > > > > > /* We always start by resetting the device, in case a previous
> > > > > > > * driver messed it up. This also tests that code path a little. */
> > > > > > > dev->config->reset(dev);
> > > > > > >
> > > > > > > Do we need to do something like this for sriov?
> > > > > >
> > > > > > I think VFs are managed by PCI core. Once they are
> > > > > > allocated, virtio driver doesn't have to care too
> > > > > > much about how to manage them. The proposal for the
> > > > > > spec is just to provide a feature bit based virtio
> > > > > > way for virtio drivers to know whether a virtio
> > > > > > device is SR-IOV capable (and virtio drivers can
> > > > > > support configuring SR-IOV based on the feature
> > > > > > bit negotiation result).
> > > > > >
> > > > > > >
> > > > > > > I also wonder whether PCI core should disable sriov for us.
> > > > > > >
> > > > > > >
> > > > > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > > > > would make it easy to test your patches. Do you happen
> > > > > > > to have something like this?
> > > > > >
> > > > > > Sorry, currently I don't have anything like this..
> > > > > >
> > > > > > Best regards,
> > > > > > Tiwei Bie
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > >
> > > > > > >
> > > > > > > > v3:
> > > > > > > > - Drop the acks;
> > > > > > > >
> > > > > > > > v2:
> > > > > > > > - Disable VFs when unbinding the driver (Alex, MST);
> > > > > > > > - Don't use pci_sriov_configure_simple (Alex);
> > > > > > > >
> > > > > > > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++
> > > > > > > > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > > > > > > include/uapi/linux/virtio_config.h | 7 ++++++-
> > > > > > > > 3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > > > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > > > > struct device *dev = get_device(&vp_dev->vdev.dev);
> > > > > > > >
> > > > > > > > + pci_disable_sriov(pci_dev);
> > > > > > > > +
> > > > > > > > unregister_virtio_device(&vp_dev->vdev);
> > > > > > > >
> > > > > > > > if (vp_dev->ioaddr)
> > > > > > > > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > > > put_device(dev);
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs)
> > > > > > > > +{
> > > > > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > > > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > > > + return -EBUSY;
> > > > > > > > +
> > > > > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > > > > + return -EINVAL;
> > > > > > > > +
> > > > > > > > + if (pci_vfs_assigned(pci_dev))
> > > > > > > > + return -EPERM;
> > > > > > > > +
> > > > > > > > + if (num_vfs == 0) {
> > > > > > > > + pci_disable_sriov(pci_dev);
> > > > > > > > + return 0;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > > > > > + if (ret < 0)
> > > > > > > > + return ret;
> > > > > > > > +
> > > > > > > > + return num_vfs;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static struct pci_driver virtio_pci_driver = {
> > > > > > > > .name = "virtio-pci",
> > > > > > > > .id_table = virtio_pci_id_table,
> > > > > > > > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = {
> > > > > > > > #ifdef CONFIG_PM_SLEEP
> > > > > > > > .driver.pm = &virtio_pci_pm_ops,
> > > > > > > > #endif
> > > > > > > > + .sriov_configure = virtio_pci_sriov_configure,
> > > > > > > > };
> > > > > > > >
> > > > > > > > module_pci_driver(virtio_pci_driver);
> > > > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > > > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev)
> > > > > > > > return features;
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > > > > > +{
> > > > > > > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > > + struct pci_dev *pci_dev = vp_dev->pci_dev;
> > > > > > > > +
> > > > > > > > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > > > > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > > > > > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > /* virtio config->finalize_features() implementation */
> > > > > > > > static int vp_finalize_features(struct virtio_device *vdev)
> > > > > > > > {
> > > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > > + u64 features = vdev->features;
> > > > > > > >
> > > > > > > > /* Give virtio_ring a chance to accept features. */
> > > > > > > > vring_transport_features(vdev);
> > > > > > > >
> > > > > > > > + /* Give virtio_pci a chance to accept features. */
> > > > > > > > + vp_transport_features(vdev, features);
> > > > > > > > +
> > > > > > > > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > > > > > dev_err(&vdev->dev, "virtio: device uses modern interface "
> > > > > > > > "but does not have VIRTIO_F_VERSION_1\n");
> > > > > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > > @@ -49,7 +49,7 @@
> > > > > > > > * transport being used (eg. virtio_ring), the rest are per-device feature
> > > > > > > > * bits. */
> > > > > > > > #define VIRTIO_TRANSPORT_F_START 28
> > > > > > > > -#define VIRTIO_TRANSPORT_F_END 34
> > > > > > > > +#define VIRTIO_TRANSPORT_F_END 38
> > > > > > > >
> > > > > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > > > > @@ -71,4 +71,9 @@
> > > > > > > > * this is for compatibility with legacy systems.
> > > > > > > > */
> > > > > > > > #define VIRTIO_F_IOMMU_PLATFORM 33
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Does the device support Single Root I/O Virtualization?
> > > > > > > > + */
> > > > > > > > +#define VIRTIO_F_SR_IOV 37
> > > > > > > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > > > > > --
> > > > > > > > 2.17.0
> > > > > > >
> > > > > > > ---------------------------------------------------------------------
> > > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > > >
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs
From: Michael S. Tsirkin @ 2018-06-06 14:27 UTC (permalink / raw)
To: Tiwei Bie
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, stefanha, zhihong.wang, bhelgaas, mark.d.rustad
In-Reply-To: <20180606141943.GA13904@debian>
On Wed, Jun 06, 2018 at 10:19:43PM +0800, Tiwei Bie wrote:
> On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei Bie wrote:
> > > > > > > There is a new feature bit allocated in virtio spec to
> > > > > > > support SR-IOV (Single Root I/O Virtualization):
> > > > > > >
> > > > > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > > > >
> > > > > > > This patch enables the support for this feature bit in
> > > > > > > virtio driver.
> > > > > > >
> > > > > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > > > > ---
> > > > > >
> > > > > > OK but what about freeze/restore functions?
> > > >
> > > > So for restore, don't you need to restore the
> > > > sriov capability?
> > >
> > > Currently I'm not familiar with the PM part.
> > > But I still think the sriov capability should
> > > be handled by PCI core.
> >
> > OK but the point is restore looks just like power up for device.
> >
> > > I'm trying to understand
> > > all the relevant code..
> > > For your question, based on what I found from
> > > the code currently, I guess the sriov capability
> > > will be restored by pci_restore_state() which
> > > will be called by the ops in pci_dev_pm_ops.
> > > The sriov_restore_state() will be called
> > > eventually.
> > >
> > > Best regards,
> > > Tiwei Bie
> >
> > Right but my point is during resume SRIOV gets enabled first before
> > driver ok.
> >
> > Maybe we should relax the requirements in the spec:
> > - only require FEATURES_OK from device, not DRIVER_OK from driver
> > - explain that it only has to happen once, not on each reset,
> > and driver can remember the result
>
> I got your point now! I'd like to relax the
> requirements in the spec.
>
> Best regards,
> Tiwei Bie
Well the ballot approving your change closed. I think we should apply
the first chunks reserving the feature bit then, and defer the rest, and
you can work on new wording documenting the actual behaviour with a new
github issue to track that - does this make sense?
Let's do it quickly though - I don't want to bother the
TC with re-voting the deferral, then the new patch.
> >
> >
> > >
> > > >
> > > >
> > > > > > I also wonder about kexec - virtio.c currently does:
> > > > > >
> > > > > > /* We always start by resetting the device, in case a previous
> > > > > > * driver messed it up. This also tests that code path a little. */
> > > > > > dev->config->reset(dev);
> > > > > >
> > > > > > Do we need to do something like this for sriov?
> > > > >
> > > > > I think VFs are managed by PCI core. Once they are
> > > > > allocated, virtio driver doesn't have to care too
> > > > > much about how to manage them. The proposal for the
> > > > > spec is just to provide a feature bit based virtio
> > > > > way for virtio drivers to know whether a virtio
> > > > > device is SR-IOV capable (and virtio drivers can
> > > > > support configuring SR-IOV based on the feature
> > > > > bit negotiation result).
> > > > >
> > > > > >
> > > > > > I also wonder whether PCI core should disable sriov for us.
> > > > > >
> > > > > >
> > > > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > > > would make it easy to test your patches. Do you happen
> > > > > > to have something like this?
> > > > >
> > > > > Sorry, currently I don't have anything like this..
> > > > >
> > > > > Best regards,
> > > > > Tiwei Bie
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > >
> > > > > > > v3:
> > > > > > > - Drop the acks;
> > > > > > >
> > > > > > > v2:
> > > > > > > - Disable VFs when unbinding the driver (Alex, MST);
> > > > > > > - Don't use pci_sriov_configure_simple (Alex);
> > > > > > >
> > > > > > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++
> > > > > > > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > > > > > include/uapi/linux/virtio_config.h | 7 ++++++-
> > > > > > > 3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > > > struct device *dev = get_device(&vp_dev->vdev.dev);
> > > > > > >
> > > > > > > + pci_disable_sriov(pci_dev);
> > > > > > > +
> > > > > > > unregister_virtio_device(&vp_dev->vdev);
> > > > > > >
> > > > > > > if (vp_dev->ioaddr)
> > > > > > > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > > put_device(dev);
> > > > > > > }
> > > > > > >
> > > > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs)
> > > > > > > +{
> > > > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > > + return -EBUSY;
> > > > > > > +
> > > > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > > > + return -EINVAL;
> > > > > > > +
> > > > > > > + if (pci_vfs_assigned(pci_dev))
> > > > > > > + return -EPERM;
> > > > > > > +
> > > > > > > + if (num_vfs == 0) {
> > > > > > > + pci_disable_sriov(pci_dev);
> > > > > > > + return 0;
> > > > > > > + }
> > > > > > > +
> > > > > > > + ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > > > > + if (ret < 0)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + return num_vfs;
> > > > > > > +}
> > > > > > > +
> > > > > > > static struct pci_driver virtio_pci_driver = {
> > > > > > > .name = "virtio-pci",
> > > > > > > .id_table = virtio_pci_id_table,
> > > > > > > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = {
> > > > > > > #ifdef CONFIG_PM_SLEEP
> > > > > > > .driver.pm = &virtio_pci_pm_ops,
> > > > > > > #endif
> > > > > > > + .sriov_configure = virtio_pci_sriov_configure,
> > > > > > > };
> > > > > > >
> > > > > > > module_pci_driver(virtio_pci_driver);
> > > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev)
> > > > > > > return features;
> > > > > > > }
> > > > > > >
> > > > > > > +static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > > > > +{
> > > > > > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > + struct pci_dev *pci_dev = vp_dev->pci_dev;
> > > > > > > +
> > > > > > > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > > > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > > > > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > > > +}
> > > > > > > +
> > > > > > > /* virtio config->finalize_features() implementation */
> > > > > > > static int vp_finalize_features(struct virtio_device *vdev)
> > > > > > > {
> > > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > > + u64 features = vdev->features;
> > > > > > >
> > > > > > > /* Give virtio_ring a chance to accept features. */
> > > > > > > vring_transport_features(vdev);
> > > > > > >
> > > > > > > + /* Give virtio_pci a chance to accept features. */
> > > > > > > + vp_transport_features(vdev, features);
> > > > > > > +
> > > > > > > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > > > > dev_err(&vdev->dev, "virtio: device uses modern interface "
> > > > > > > "but does not have VIRTIO_F_VERSION_1\n");
> > > > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > @@ -49,7 +49,7 @@
> > > > > > > * transport being used (eg. virtio_ring), the rest are per-device feature
> > > > > > > * bits. */
> > > > > > > #define VIRTIO_TRANSPORT_F_START 28
> > > > > > > -#define VIRTIO_TRANSPORT_F_END 34
> > > > > > > +#define VIRTIO_TRANSPORT_F_END 38
> > > > > > >
> > > > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > > > @@ -71,4 +71,9 @@
> > > > > > > * this is for compatibility with legacy systems.
> > > > > > > */
> > > > > > > #define VIRTIO_F_IOMMU_PLATFORM 33
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Does the device support Single Root I/O Virtualization?
> > > > > > > + */
> > > > > > > +#define VIRTIO_F_SR_IOV 37
> > > > > > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > > > > --
> > > > > > > 2.17.0
> > > > > >
> > > > > > ---------------------------------------------------------------------
> > > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > > >
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs
From: Tiwei Bie @ 2018-06-06 14:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, stefanha, zhihong.wang, bhelgaas, mark.d.rustad
In-Reply-To: <20180606153236-mutt-send-email-mst@kernel.org>
On Wed, Jun 06, 2018 at 03:44:11PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> > On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei Bie wrote:
> > > > > > There is a new feature bit allocated in virtio spec to
> > > > > > support SR-IOV (Single Root I/O Virtualization):
> > > > > >
> > > > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > > >
> > > > > > This patch enables the support for this feature bit in
> > > > > > virtio driver.
> > > > > >
> > > > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > > > ---
> > > > >
> > > > > OK but what about freeze/restore functions?
> > >
> > > So for restore, don't you need to restore the
> > > sriov capability?
> >
> > Currently I'm not familiar with the PM part.
> > But I still think the sriov capability should
> > be handled by PCI core.
>
> OK but the point is restore looks just like power up for device.
>
> > I'm trying to understand
> > all the relevant code..
> > For your question, based on what I found from
> > the code currently, I guess the sriov capability
> > will be restored by pci_restore_state() which
> > will be called by the ops in pci_dev_pm_ops.
> > The sriov_restore_state() will be called
> > eventually.
> >
> > Best regards,
> > Tiwei Bie
>
> Right but my point is during resume SRIOV gets enabled first before
> driver ok.
>
> Maybe we should relax the requirements in the spec:
> - only require FEATURES_OK from device, not DRIVER_OK from driver
> - explain that it only has to happen once, not on each reset,
> and driver can remember the result
I got your point now! I'd like to relax the
requirements in the spec.
Best regards,
Tiwei Bie
>
>
> >
> > >
> > >
> > > > > I also wonder about kexec - virtio.c currently does:
> > > > >
> > > > > /* We always start by resetting the device, in case a previous
> > > > > * driver messed it up. This also tests that code path a little. */
> > > > > dev->config->reset(dev);
> > > > >
> > > > > Do we need to do something like this for sriov?
> > > >
> > > > I think VFs are managed by PCI core. Once they are
> > > > allocated, virtio driver doesn't have to care too
> > > > much about how to manage them. The proposal for the
> > > > spec is just to provide a feature bit based virtio
> > > > way for virtio drivers to know whether a virtio
> > > > device is SR-IOV capable (and virtio drivers can
> > > > support configuring SR-IOV based on the feature
> > > > bit negotiation result).
> > > >
> > > > >
> > > > > I also wonder whether PCI core should disable sriov for us.
> > > > >
> > > > >
> > > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > > would make it easy to test your patches. Do you happen
> > > > > to have something like this?
> > > >
> > > > Sorry, currently I don't have anything like this..
> > > >
> > > > Best regards,
> > > > Tiwei Bie
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > >
> > > > > > v3:
> > > > > > - Drop the acks;
> > > > > >
> > > > > > v2:
> > > > > > - Disable VFs when unbinding the driver (Alex, MST);
> > > > > > - Don't use pci_sriov_configure_simple (Alex);
> > > > > >
> > > > > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++
> > > > > > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > > > > include/uapi/linux/virtio_config.h | 7 ++++++-
> > > > > > 3 files changed, 50 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > > struct device *dev = get_device(&vp_dev->vdev.dev);
> > > > > >
> > > > > > + pci_disable_sriov(pci_dev);
> > > > > > +
> > > > > > unregister_virtio_device(&vp_dev->vdev);
> > > > > >
> > > > > > if (vp_dev->ioaddr)
> > > > > > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > > put_device(dev);
> > > > > > }
> > > > > >
> > > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs)
> > > > > > +{
> > > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > > > > + int ret;
> > > > > > +
> > > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > > + return -EBUSY;
> > > > > > +
> > > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + if (pci_vfs_assigned(pci_dev))
> > > > > > + return -EPERM;
> > > > > > +
> > > > > > + if (num_vfs == 0) {
> > > > > > + pci_disable_sriov(pci_dev);
> > > > > > + return 0;
> > > > > > + }
> > > > > > +
> > > > > > + ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + return num_vfs;
> > > > > > +}
> > > > > > +
> > > > > > static struct pci_driver virtio_pci_driver = {
> > > > > > .name = "virtio-pci",
> > > > > > .id_table = virtio_pci_id_table,
> > > > > > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = {
> > > > > > #ifdef CONFIG_PM_SLEEP
> > > > > > .driver.pm = &virtio_pci_pm_ops,
> > > > > > #endif
> > > > > > + .sriov_configure = virtio_pci_sriov_configure,
> > > > > > };
> > > > > >
> > > > > > module_pci_driver(virtio_pci_driver);
> > > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev)
> > > > > > return features;
> > > > > > }
> > > > > >
> > > > > > +static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > > > +{
> > > > > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > + struct pci_dev *pci_dev = vp_dev->pci_dev;
> > > > > > +
> > > > > > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > > > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > > +}
> > > > > > +
> > > > > > /* virtio config->finalize_features() implementation */
> > > > > > static int vp_finalize_features(struct virtio_device *vdev)
> > > > > > {
> > > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > > + u64 features = vdev->features;
> > > > > >
> > > > > > /* Give virtio_ring a chance to accept features. */
> > > > > > vring_transport_features(vdev);
> > > > > >
> > > > > > + /* Give virtio_pci a chance to accept features. */
> > > > > > + vp_transport_features(vdev, features);
> > > > > > +
> > > > > > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > > > dev_err(&vdev->dev, "virtio: device uses modern interface "
> > > > > > "but does not have VIRTIO_F_VERSION_1\n");
> > > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > @@ -49,7 +49,7 @@
> > > > > > * transport being used (eg. virtio_ring), the rest are per-device feature
> > > > > > * bits. */
> > > > > > #define VIRTIO_TRANSPORT_F_START 28
> > > > > > -#define VIRTIO_TRANSPORT_F_END 34
> > > > > > +#define VIRTIO_TRANSPORT_F_END 38
> > > > > >
> > > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > > @@ -71,4 +71,9 @@
> > > > > > * this is for compatibility with legacy systems.
> > > > > > */
> > > > > > #define VIRTIO_F_IOMMU_PLATFORM 33
> > > > > > +
> > > > > > +/*
> > > > > > + * Does the device support Single Root I/O Virtualization?
> > > > > > + */
> > > > > > +#define VIRTIO_F_SR_IOV 37
> > > > > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > > > --
> > > > > > 2.17.0
> > > > >
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > > >
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs
From: Michael S. Tsirkin @ 2018-06-06 12:44 UTC (permalink / raw)
To: Tiwei Bie
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, stefanha, zhihong.wang, bhelgaas, mark.d.rustad
In-Reply-To: <20180606121154.GA9599@debian>
On Wed, Jun 06, 2018 at 08:11:54PM +0800, Tiwei Bie wrote:
> On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei Bie wrote:
> > > > > There is a new feature bit allocated in virtio spec to
> > > > > support SR-IOV (Single Root I/O Virtualization):
> > > > >
> > > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > > >
> > > > > This patch enables the support for this feature bit in
> > > > > virtio driver.
> > > > >
> > > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > > ---
> > > >
> > > > OK but what about freeze/restore functions?
> >
> > So for restore, don't you need to restore the
> > sriov capability?
>
> Currently I'm not familiar with the PM part.
> But I still think the sriov capability should
> be handled by PCI core.
OK but the point is restore looks just like power up for device.
> I'm trying to understand
> all the relevant code..
> For your question, based on what I found from
> the code currently, I guess the sriov capability
> will be restored by pci_restore_state() which
> will be called by the ops in pci_dev_pm_ops.
> The sriov_restore_state() will be called
> eventually.
>
> Best regards,
> Tiwei Bie
Right but my point is during resume SRIOV gets enabled first before
driver ok.
Maybe we should relax the requirements in the spec:
- only require FEATURES_OK from device, not DRIVER_OK from driver
- explain that it only has to happen once, not on each reset,
and driver can remember the result
>
> >
> >
> > > > I also wonder about kexec - virtio.c currently does:
> > > >
> > > > /* We always start by resetting the device, in case a previous
> > > > * driver messed it up. This also tests that code path a little. */
> > > > dev->config->reset(dev);
> > > >
> > > > Do we need to do something like this for sriov?
> > >
> > > I think VFs are managed by PCI core. Once they are
> > > allocated, virtio driver doesn't have to care too
> > > much about how to manage them. The proposal for the
> > > spec is just to provide a feature bit based virtio
> > > way for virtio drivers to know whether a virtio
> > > device is SR-IOV capable (and virtio drivers can
> > > support configuring SR-IOV based on the feature
> > > bit negotiation result).
> > >
> > > >
> > > > I also wonder whether PCI core should disable sriov for us.
> > > >
> > > >
> > > > I wish there was a patch emulating this without vDPA for QEMU,
> > > > would make it easy to test your patches. Do you happen
> > > > to have something like this?
> > >
> > > Sorry, currently I don't have anything like this..
> > >
> > > Best regards,
> > > Tiwei Bie
> > >
> > > >
> > > > Thanks,
> > > >
> > > >
> > > > > v3:
> > > > > - Drop the acks;
> > > > >
> > > > > v2:
> > > > > - Disable VFs when unbinding the driver (Alex, MST);
> > > > > - Don't use pci_sriov_configure_simple (Alex);
> > > > >
> > > > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++
> > > > > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > > > include/uapi/linux/virtio_config.h | 7 ++++++-
> > > > > 3 files changed, 50 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > struct device *dev = get_device(&vp_dev->vdev.dev);
> > > > >
> > > > > + pci_disable_sriov(pci_dev);
> > > > > +
> > > > > unregister_virtio_device(&vp_dev->vdev);
> > > > >
> > > > > if (vp_dev->ioaddr)
> > > > > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > > put_device(dev);
> > > > > }
> > > > >
> > > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs)
> > > > > +{
> > > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > > > + int ret;
> > > > > +
> > > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > > + return -EBUSY;
> > > > > +
> > > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (pci_vfs_assigned(pci_dev))
> > > > > + return -EPERM;
> > > > > +
> > > > > + if (num_vfs == 0) {
> > > > > + pci_disable_sriov(pci_dev);
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + return num_vfs;
> > > > > +}
> > > > > +
> > > > > static struct pci_driver virtio_pci_driver = {
> > > > > .name = "virtio-pci",
> > > > > .id_table = virtio_pci_id_table,
> > > > > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = {
> > > > > #ifdef CONFIG_PM_SLEEP
> > > > > .driver.pm = &virtio_pci_pm_ops,
> > > > > #endif
> > > > > + .sriov_configure = virtio_pci_sriov_configure,
> > > > > };
> > > > >
> > > > > module_pci_driver(virtio_pci_driver);
> > > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > > index 2555d80f6eec..07571daccfec 100644
> > > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev)
> > > > > return features;
> > > > > }
> > > > >
> > > > > +static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > > +{
> > > > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > + struct pci_dev *pci_dev = vp_dev->pci_dev;
> > > > > +
> > > > > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > > +}
> > > > > +
> > > > > /* virtio config->finalize_features() implementation */
> > > > > static int vp_finalize_features(struct virtio_device *vdev)
> > > > > {
> > > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > > + u64 features = vdev->features;
> > > > >
> > > > > /* Give virtio_ring a chance to accept features. */
> > > > > vring_transport_features(vdev);
> > > > >
> > > > > + /* Give virtio_pci a chance to accept features. */
> > > > > + vp_transport_features(vdev, features);
> > > > > +
> > > > > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > > dev_err(&vdev->dev, "virtio: device uses modern interface "
> > > > > "but does not have VIRTIO_F_VERSION_1\n");
> > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > @@ -49,7 +49,7 @@
> > > > > * transport being used (eg. virtio_ring), the rest are per-device feature
> > > > > * bits. */
> > > > > #define VIRTIO_TRANSPORT_F_START 28
> > > > > -#define VIRTIO_TRANSPORT_F_END 34
> > > > > +#define VIRTIO_TRANSPORT_F_END 38
> > > > >
> > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > @@ -71,4 +71,9 @@
> > > > > * this is for compatibility with legacy systems.
> > > > > */
> > > > > #define VIRTIO_F_IOMMU_PLATFORM 33
> > > > > +
> > > > > +/*
> > > > > + * Does the device support Single Root I/O Virtualization?
> > > > > + */
> > > > > +#define VIRTIO_F_SR_IOV 37
> > > > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > > --
> > > > > 2.17.0
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > >
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v3] virtio_pci: support enabling VFs
From: Tiwei Bie @ 2018-06-06 12:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alexander.h.duyck, virtio-dev, linux-pci, linux-kernel,
virtualization, stefanha, zhihong.wang, bhelgaas, mark.d.rustad
In-Reply-To: <20180605152151-mutt-send-email-mst@kernel.org>
On Tue, Jun 05, 2018 at 03:23:11PM +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 05, 2018 at 09:36:53AM +0800, Tiwei Bie wrote:
> > On Mon, Jun 04, 2018 at 07:32:25PM +0300, Michael S. Tsirkin wrote:
> > > On Fri, Jun 01, 2018 at 12:02:39PM +0800, Tiwei Bie wrote:
> > > > There is a new feature bit allocated in virtio spec to
> > > > support SR-IOV (Single Root I/O Virtualization):
> > > >
> > > > https://github.com/oasis-tcs/virtio-spec/issues/11
> > > >
> > > > This patch enables the support for this feature bit in
> > > > virtio driver.
> > > >
> > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > ---
> > >
> > > OK but what about freeze/restore functions?
>
> So for restore, don't you need to restore the
> sriov capability?
Currently I'm not familiar with the PM part.
But I still think the sriov capability should
be handled by PCI core. I'm trying to understand
all the relevant code..
For your question, based on what I found from
the code currently, I guess the sriov capability
will be restored by pci_restore_state() which
will be called by the ops in pci_dev_pm_ops.
The sriov_restore_state() will be called
eventually.
Best regards,
Tiwei Bie
>
>
> > > I also wonder about kexec - virtio.c currently does:
> > >
> > > /* We always start by resetting the device, in case a previous
> > > * driver messed it up. This also tests that code path a little. */
> > > dev->config->reset(dev);
> > >
> > > Do we need to do something like this for sriov?
> >
> > I think VFs are managed by PCI core. Once they are
> > allocated, virtio driver doesn't have to care too
> > much about how to manage them. The proposal for the
> > spec is just to provide a feature bit based virtio
> > way for virtio drivers to know whether a virtio
> > device is SR-IOV capable (and virtio drivers can
> > support configuring SR-IOV based on the feature
> > bit negotiation result).
> >
> > >
> > > I also wonder whether PCI core should disable sriov for us.
> > >
> > >
> > > I wish there was a patch emulating this without vDPA for QEMU,
> > > would make it easy to test your patches. Do you happen
> > > to have something like this?
> >
> > Sorry, currently I don't have anything like this..
> >
> > Best regards,
> > Tiwei Bie
> >
> > >
> > > Thanks,
> > >
> > >
> > > > v3:
> > > > - Drop the acks;
> > > >
> > > > v2:
> > > > - Disable VFs when unbinding the driver (Alex, MST);
> > > > - Don't use pci_sriov_configure_simple (Alex);
> > > >
> > > > drivers/virtio/virtio_pci_common.c | 30 ++++++++++++++++++++++++++++++
> > > > drivers/virtio/virtio_pci_modern.c | 14 ++++++++++++++
> > > > include/uapi/linux/virtio_config.h | 7 ++++++-
> > > > 3 files changed, 50 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > > index 48d4d1cf1cb6..1d4467b2dc31 100644
> > > > --- a/drivers/virtio/virtio_pci_common.c
> > > > +++ b/drivers/virtio/virtio_pci_common.c
> > > > @@ -577,6 +577,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > struct device *dev = get_device(&vp_dev->vdev.dev);
> > > >
> > > > + pci_disable_sriov(pci_dev);
> > > > +
> > > > unregister_virtio_device(&vp_dev->vdev);
> > > >
> > > > if (vp_dev->ioaddr)
> > > > @@ -588,6 +590,33 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > > > put_device(dev);
> > > > }
> > > >
> > > > +static int virtio_pci_sriov_configure(struct pci_dev *pci_dev, int num_vfs)
> > > > +{
> > > > + struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> > > > + struct virtio_device *vdev = &vp_dev->vdev;
> > > > + int ret;
> > > > +
> > > > + if (!(vdev->config->get_status(vdev) & VIRTIO_CONFIG_S_DRIVER_OK))
> > > > + return -EBUSY;
> > > > +
> > > > + if (!__virtio_test_bit(vdev, VIRTIO_F_SR_IOV))
> > > > + return -EINVAL;
> > > > +
> > > > + if (pci_vfs_assigned(pci_dev))
> > > > + return -EPERM;
> > > > +
> > > > + if (num_vfs == 0) {
> > > > + pci_disable_sriov(pci_dev);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + ret = pci_enable_sriov(pci_dev, num_vfs);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + return num_vfs;
> > > > +}
> > > > +
> > > > static struct pci_driver virtio_pci_driver = {
> > > > .name = "virtio-pci",
> > > > .id_table = virtio_pci_id_table,
> > > > @@ -596,6 +625,7 @@ static struct pci_driver virtio_pci_driver = {
> > > > #ifdef CONFIG_PM_SLEEP
> > > > .driver.pm = &virtio_pci_pm_ops,
> > > > #endif
> > > > + .sriov_configure = virtio_pci_sriov_configure,
> > > > };
> > > >
> > > > module_pci_driver(virtio_pci_driver);
> > > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > > index 2555d80f6eec..07571daccfec 100644
> > > > --- a/drivers/virtio/virtio_pci_modern.c
> > > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > > @@ -153,14 +153,28 @@ static u64 vp_get_features(struct virtio_device *vdev)
> > > > return features;
> > > > }
> > > >
> > > > +static void vp_transport_features(struct virtio_device *vdev, u64 features)
> > > > +{
> > > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > + struct pci_dev *pci_dev = vp_dev->pci_dev;
> > > > +
> > > > + if ((features & BIT_ULL(VIRTIO_F_SR_IOV)) &&
> > > > + pci_find_ext_capability(pci_dev, PCI_EXT_CAP_ID_SRIOV))
> > > > + __virtio_set_bit(vdev, VIRTIO_F_SR_IOV);
> > > > +}
> > > > +
> > > > /* virtio config->finalize_features() implementation */
> > > > static int vp_finalize_features(struct virtio_device *vdev)
> > > > {
> > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > > > + u64 features = vdev->features;
> > > >
> > > > /* Give virtio_ring a chance to accept features. */
> > > > vring_transport_features(vdev);
> > > >
> > > > + /* Give virtio_pci a chance to accept features. */
> > > > + vp_transport_features(vdev, features);
> > > > +
> > > > if (!__virtio_test_bit(vdev, VIRTIO_F_VERSION_1)) {
> > > > dev_err(&vdev->dev, "virtio: device uses modern interface "
> > > > "but does not have VIRTIO_F_VERSION_1\n");
> > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > index 308e2096291f..b7c1f4e7d59e 100644
> > > > --- a/include/uapi/linux/virtio_config.h
> > > > +++ b/include/uapi/linux/virtio_config.h
> > > > @@ -49,7 +49,7 @@
> > > > * transport being used (eg. virtio_ring), the rest are per-device feature
> > > > * bits. */
> > > > #define VIRTIO_TRANSPORT_F_START 28
> > > > -#define VIRTIO_TRANSPORT_F_END 34
> > > > +#define VIRTIO_TRANSPORT_F_END 38
> > > >
> > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > @@ -71,4 +71,9 @@
> > > > * this is for compatibility with legacy systems.
> > > > */
> > > > #define VIRTIO_F_IOMMU_PLATFORM 33
> > > > +
> > > > +/*
> > > > + * Does the device support Single Root I/O Virtualization?
> > > > + */
> > > > +#define VIRTIO_F_SR_IOV 37
> > > > #endif /* _UAPI_LINUX_VIRTIO_CONFIG_H */
> > > > --
> > > > 2.17.0
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > >
^ permalink raw reply
* [PATCH v6] virtio_blk: add DISCARD and WRIET ZEROES commands support
From: Changpeng Liu @ 2018-06-06 4:19 UTC (permalink / raw)
To: virtualization, changpeng.liu; +Cc: pbonzini, cavery, stefanha
Existing virtio-blk protocol doesn't have DISCARD/WRITE ZEROES commands
support, this will impact the performance when using SSD backend over
file systems.
Commit 88c85538 "virtio-blk: add discard and write zeroes features to
specification"(see https://github.com/oasis-tcs/virtio-spec) extended
existing virtio-blk protocol, adding extra DISCARD and WRITE ZEROES
commands support.
While here, using 16 bytes descriptor to describe one segment of DISCARD
or WRITE ZEROES commands, each command may contain one or more decriptors.
The following data structure shows the definition of one descriptor:
struct virtio_blk_discard_write_zeroes {
le64 sector;
le32 num_sectors;
le32 unmap;
};
Field 'sector' means the start sector for DISCARD and WRITE ZEROES,
filed 'num_sectors' means the number of sectors for DISCARD and WRITE
ZEROES, if both DISCARD and WRITE ZEROES are supported, field 'unmap'
maybe used for WRITE ZEROES command with DISCARD enabled.
We also extended the virtio-blk configuration space to let backend
device put DISCARD and WRITE ZEROES configuration parameters.
struct virtio_blk_config {
[...]
le32 max_discard_sectors;
le32 max_discard_seg;
le32 discard_sector_alignment;
le32 max_write_zeroes_sectors;
le32 max_write_zeroes_seg;
u8 write_zeroes_may_unmap;
}
New feature bit [VIRTIO_BLK_F_DISCARD (13)]: Device can support discard
command, maximum discard sectors size in field 'max_discard_sectors' and
maximum discard segment number in field 'max_discard_seg'.
New feature [VIRTIO_BLK_F_WRITE_ZEROES (14)]: Device can support write
zeroes command, maximum write zeroes sectors size in field
'max_write_zeroes_sectors' and maximum write zeroes segment number in
field 'max_write_zeroes_seg'.
The parameters in the configuration space of the device field
'max_discard_sectors' and field 'discard_sector_alignment' are expressed in
512-byte units if the VIRTIO_BLK_F_DISCARD feature bit is negotiated. The
field 'max_write_zeroes_sectors' is expressed in 512-byte units if the
VIRTIO_BLK_F_WRITE_ZEROES feature bit is negotiated.
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
CHANGELOG:
v6: don't set T_OUT bit to discard and write zeroes commands.
v5: use new block layer API: blk_queue_flag_set.
v4: several optimizations based on MST's comments, remove bit field usage for
command descriptor.
v3: define the virtio-blk protocol to add discard and write zeroes support,
first version implementation based on proposed specification.
v2: add write zeroes command support.
v1: initial proposal implementation for discard command.
---
drivers/block/virtio_blk.c | 89 ++++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/virtio_blk.h | 43 ++++++++++++++++++++
2 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4a07593c..5aabc63 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -172,10 +172,45 @@ static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr,
return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
}
+static inline int virtblk_setup_discard_write_zeroes(struct request *req,
+ bool unmap)
+{
+ unsigned short segments = blk_rq_nr_discard_segments(req);
+ unsigned short n = 0;
+ struct virtio_blk_discard_write_zeroes *range;
+ struct bio *bio;
+
+ range = kmalloc_array(segments, sizeof(*range), GFP_KERNEL);
+ if (!range)
+ return -ENOMEM;
+
+ __rq_for_each_bio(bio, req) {
+ u64 sector = bio->bi_iter.bi_sector;
+ u32 num_sectors = bio->bi_iter.bi_size >> 9;
+
+ range[n].unmap = cpu_to_le32(unmap);
+ range[n].num_sectors = cpu_to_le32(num_sectors);
+ range[n].sector = cpu_to_le64(sector);
+ n++;
+ }
+
+ req->special_vec.bv_page = virt_to_page(range);
+ req->special_vec.bv_offset = offset_in_page(range);
+ req->special_vec.bv_len = sizeof(*range) * segments;
+ req->rq_flags |= RQF_SPECIAL_PAYLOAD;
+
+ return 0;
+}
+
static inline void virtblk_request_done(struct request *req)
{
struct virtblk_req *vbr = blk_mq_rq_to_pdu(req);
+ if (req->rq_flags & RQF_SPECIAL_PAYLOAD) {
+ kfree(page_address(req->special_vec.bv_page) +
+ req->special_vec.bv_offset);
+ }
+
switch (req_op(req)) {
case REQ_OP_SCSI_IN:
case REQ_OP_SCSI_OUT:
@@ -225,6 +260,7 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
int qid = hctx->queue_num;
int err;
bool notify = false;
+ bool unmap = false;
u32 type;
BUG_ON(req->nr_phys_segments + 2 > vblk->sg_elems);
@@ -237,6 +273,13 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
case REQ_OP_FLUSH:
type = VIRTIO_BLK_T_FLUSH;
break;
+ case REQ_OP_DISCARD:
+ type = VIRTIO_BLK_T_DISCARD;
+ break;
+ case REQ_OP_WRITE_ZEROES:
+ type = VIRTIO_BLK_T_WRITE_ZEROES;
+ unmap = !(req->cmd_flags & REQ_NOUNMAP);
+ break;
case REQ_OP_SCSI_IN:
case REQ_OP_SCSI_OUT:
type = VIRTIO_BLK_T_SCSI_CMD;
@@ -256,6 +299,12 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
blk_mq_start_request(req);
+ if (type == VIRTIO_BLK_T_DISCARD || type == VIRTIO_BLK_T_WRITE_ZEROES) {
+ err = virtblk_setup_discard_write_zeroes(req, unmap);
+ if (err)
+ return BLK_STS_RESOURCE;
+ }
+
num = blk_rq_map_sg(hctx->queue, req, vbr->sg);
if (num) {
if (rq_data_dir(req) == WRITE)
@@ -777,6 +826,42 @@ static int virtblk_probe(struct virtio_device *vdev)
if (!err && opt_io_size)
blk_queue_io_opt(q, blk_size * opt_io_size);
+ if (virtio_has_feature(vdev, VIRTIO_BLK_F_DISCARD)) {
+ q->limits.discard_granularity = blk_size;
+
+ virtio_cread(vdev, struct virtio_blk_config,
+ discard_sector_alignment, &v);
+ if (v)
+ q->limits.discard_alignment = v << 9;
+ else
+ q->limits.discard_alignment = 0;
+
+ virtio_cread(vdev, struct virtio_blk_config,
+ max_discard_sectors, &v);
+ if (v)
+ blk_queue_max_discard_sectors(q, v);
+ else
+ blk_queue_max_discard_sectors(q, UINT_MAX);
+
+ virtio_cread(vdev, struct virtio_blk_config, max_discard_seg,
+ &v);
+ if (v)
+ blk_queue_max_discard_segments(q, v);
+ else
+ blk_queue_max_discard_segments(q, USHRT_MAX);
+
+ blk_queue_flag_set(QUEUE_FLAG_DISCARD, q);
+ }
+
+ if (virtio_has_feature(vdev, VIRTIO_BLK_F_WRITE_ZEROES)) {
+ virtio_cread(vdev, struct virtio_blk_config,
+ max_write_zeroes_sectors, &v);
+ if (v)
+ blk_queue_max_write_zeroes_sectors(q, v);
+ else
+ blk_queue_max_write_zeroes_sectors(q, UINT_MAX);
+ }
+
virtblk_update_capacity(vblk, false);
virtio_device_ready(vdev);
@@ -885,14 +970,14 @@ static int virtblk_restore(struct virtio_device *vdev)
VIRTIO_BLK_F_SCSI,
#endif
VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
- VIRTIO_BLK_F_MQ,
+ VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
}
;
static unsigned int features[] = {
VIRTIO_BLK_F_SEG_MAX, VIRTIO_BLK_F_SIZE_MAX, VIRTIO_BLK_F_GEOMETRY,
VIRTIO_BLK_F_RO, VIRTIO_BLK_F_BLK_SIZE,
VIRTIO_BLK_F_FLUSH, VIRTIO_BLK_F_TOPOLOGY, VIRTIO_BLK_F_CONFIG_WCE,
- VIRTIO_BLK_F_MQ,
+ VIRTIO_BLK_F_MQ, VIRTIO_BLK_F_DISCARD, VIRTIO_BLK_F_WRITE_ZEROES,
};
static struct virtio_driver virtio_blk = {
diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
index 9ebe4d9..8e7a015 100644
--- a/include/uapi/linux/virtio_blk.h
+++ b/include/uapi/linux/virtio_blk.h
@@ -38,6 +38,8 @@
#define VIRTIO_BLK_F_BLK_SIZE 6 /* Block size of disk is available*/
#define VIRTIO_BLK_F_TOPOLOGY 10 /* Topology information is available */
#define VIRTIO_BLK_F_MQ 12 /* support more than one vq */
+#define VIRTIO_BLK_F_DISCARD 13 /* DISCARD is supported */
+#define VIRTIO_BLK_F_WRITE_ZEROES 14 /* WRITE ZEROES is supported */
/* Legacy feature bits */
#ifndef VIRTIO_BLK_NO_LEGACY
@@ -86,6 +88,29 @@ struct virtio_blk_config {
/* number of vqs, only available when VIRTIO_BLK_F_MQ is set */
__u16 num_queues;
+ /* The maximum discard sectors (in 512-byte sectors) for
+ * one segment (if VIRTIO_BLK_F_DISCARD)
+ */
+ __u32 max_discard_sectors;
+ /* The maximum number of discard segments in a
+ * discard command (if VIRTIO_BLK_F_DISCARD)
+ */
+ __u32 max_discard_seg;
+ /* The alignment sectors for discard (if VIRTIO_BLK_F_DISCARD) */
+ __u32 discard_sector_alignment;
+ /* The maximum number of write zeroes sectors (in 512-byte sectors) in
+ * one segment (if VIRTIO_BLK_F_WRITE_ZEROES)
+ */
+ __u32 max_write_zeroes_sectors;
+ /* The maximum number of segments in a write zeroes
+ * command (if VIRTIO_BLK_F_WRITE_ZEROES)
+ */
+ __u32 max_write_zeroes_seg;
+ /* Device clear this bit when write zeroes command can't result in
+ * unmapping sectors (if VIRITO_BLK_F_WRITE_ZEROES and with unmap)
+ */
+ __u8 write_zeroes_may_unmap;
+ __u8 unused1[3];
} __attribute__((packed));
/*
@@ -114,6 +139,12 @@ struct virtio_blk_config {
/* Get device ID command */
#define VIRTIO_BLK_T_GET_ID 8
+/* Discard command */
+#define VIRTIO_BLK_T_DISCARD 11
+
+/* Write zeroes command */
+#define VIRTIO_BLK_T_WRITE_ZEROES 13
+
#ifndef VIRTIO_BLK_NO_LEGACY
/* Barrier before this op. */
#define VIRTIO_BLK_T_BARRIER 0x80000000
@@ -133,6 +164,18 @@ struct virtio_blk_outhdr {
__virtio64 sector;
};
+/*
+ * discard/write zeroes range for each request.
+ */
+struct virtio_blk_discard_write_zeroes {
+ /* discard/write zeroes start sector */
+ __virtio64 sector;
+ /* number of discard/write zeroes sectors */
+ __virtio32 num_sectors;
+ /* valid for write zeroes command */
+ __virtio32 unmap;
+};
+
#ifndef VIRTIO_BLK_NO_LEGACY
struct virtio_scsi_inhdr {
__virtio32 errors;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Jason Wang @ 2018-06-06 2:29 UTC (permalink / raw)
To: Michael S. Tsirkin, Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, qemu-devel, virtualization
In-Reply-To: <20180605152344-mutt-send-email-mst@kernel.org>
On 2018年06月05日 20:33, Michael S. Tsirkin wrote:
> I don't think this is sufficient.
>
> If both primary and standby devices are present, a legacy guest without
> support for the feature might see two devices with same mac and get
> confused.
>
> I think that we should only make primary visible after guest acked the
> backup feature bit.
I think we want exactly the reverse? E.g fail the negotiation when guest
does not ack backup feature.
Otherwise legacy guest won't even have the chance to see primary device
in the guest.
>
> And on reset or when backup is cleared in some other way, unplug the
> primary.
What if guest does not support hotplug?
>
> Something like the below will do the job:
>
> Primary device is added with a special "primary-failover" flag.
> A virtual machine is then initialized with just a standby virtio
> device. Primary is not yet added.
A question is how to do the matching? Qemu knows nothing about e.g mac
address of a pass-through device I believe?
>
> Later QEMU detects that guest driver device set DRIVER_OK.
> It then exposes the primary device to the guest, and triggers
> a device addition event (hot-plug event) for it.
Do you mean we won't have primary device in the initial qemu cli?
>
> If QEMU detects guest driver removal, it initiates a hot-unplug sequence
> to remove the primary driver. In particular, if QEMU detects guest
> re-initialization (e.g. by detecting guest reset) it immediately removes
> the primary device.
I believe guest failover module should handle this gracefully?
Thanks
>
> We can move some of this code to management as well, architecturally it
> does not make too much sense but it might be easier implementation-wise.
>
> HTH
>
> On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
>> Ping on this patch now that the kernel patches are accepted into davem's net-next tree.
>> https://patchwork.ozlabs.org/cover/920005/
>>
>>
>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>>> This feature bit can be used by hypervisor to indicate virtio_net device to
>>> act as a standby for another device with the same MAC address.
>>>
>>> I tested this with a small change to the patch to mark the STANDBY feature 'true'
>>> by default as i am using libvirt to start the VMs.
>>> Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
>>> XML file?
>>>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>> ---
>>> hw/net/virtio-net.c | 2 ++
>>> include/standard-headers/linux/virtio_net.h | 3 +++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 90502fca7c..38b3140670 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>>> true),
>>> DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>>> DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>> + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY,
>>> + false),
>>> DEFINE_PROP_END_OF_LIST(),
>>> };
>>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>>> index e9f255ea3f..01ec09684c 100644
>>> --- a/include/standard-headers/linux/virtio_net.h
>>> +++ b/include/standard-headers/linux/virtio_net.h
>>> @@ -57,6 +57,9 @@
>>> * Steering */
>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>> +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
>>> + * with the same MAC.
>>> + */
>>> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>>> #ifndef VIRTIO_NET_NO_LEGACY
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-05 22:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, qemu-devel, Samudrala, Sridhar,
virtualization
In-Reply-To: <20180606002650-mutt-send-email-mst@kernel.org>
On Tue, Jun 5, 2018 at 2:32 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Jun 05, 2018 at 02:16:44PM -0700, Siwei Liu wrote:
>> Good to see this discussion going. I share the same feeling that the
>> decision of plugging the primary (passthrough) should only be made
>> until guest driver acknowledges DRIVER_OK and _F_STANDBY.
>> Architecturally this intelligence should be baken to QEMU itself
>> rather than moving up to management stack, such as libvirt.
>>
>> A few questions in line below.
>>
>> On Tue, Jun 5, 2018 at 5:33 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > I don't think this is sufficient.
>> >
>> > If both primary and standby devices are present, a legacy guest without
>> > support for the feature might see two devices with same mac and get
>> > confused.
>> >
>> > I think that we should only make primary visible after guest acked the
>> > backup feature bit.
>> >
>> > And on reset or when backup is cleared in some other way, unplug the
>> > primary.
>> >
>> > Something like the below will do the job:
>> >
>> > Primary device is added with a special "primary-failover" flag.
>>
>> I wonder if you envision this flag as a user interface or some
>> internal attribute/property to QEMU device?
>>
>> Since QEMU needs to associate this particular primary-failover device
>> with a virtio standby instance for checking DRIVER_OK as you indicate
>> below, I wonder if the group ID thing will be helpful to set
>> "primary-failover" flag internally without having to add another
>> option in CLI?
>
> I haven't thought about it but it's an option.
>
>> > A virtual machine is then initialized with just a standby virtio
>> > device. Primary is not yet added.
>> >
>> > Later QEMU detects that guest driver device set DRIVER_OK.
>> > It then exposes the primary device to the guest, and triggers
>> > a device addition event (hot-plug event) for it.
>>
>> Sounds OK. While there might be a small window the guest already
>> starts to use virtio interface before the passthrough gets plugged in,
>> I think that should be fine.
>>
>> Another option is to wait until the primary appears and gets
>> registered in the guest, so it can bring up the upper failover
>> interface.
>
> We can't be sure this will ever happen, can we? We might be asked to
> migrate at any time.
Right. For that to work, virtio driver needs a complicated state
tracking hand-in-hand with the host for VF's hot plugging activity. If
hot plugging is defered due to migration activity the failover
interface can be brought up immediately.
As said, this is just an option. I don't have strong preference here.
>
>> >
>> > If QEMU detects guest driver removal, it initiates a hot-unplug sequence
>> > to remove the primary driver. In particular, if QEMU detects guest
>> > re-initialization (e.g. by detecting guest reset) it immediately removes
>> > the primary device.
>>
>> Agreed.
>>
>> For legacy guest, user might prefer seeing a single passthrough device
>> rather than a virtio device. I would hope there's an option to allow
>> it to happen, instead of removing all passthrough devices.
>>
>> Regards,
>> -Siwei
>
> I don't see a way to make it work since then you can't migrate, can you?
The thing is cloud service provider might prefer sticking to the same
level of service agreement (SLA) of keeping VF over migration,
particularly when guest OS or kernel does not have the support.The
footprint in guest OS support might NOT be prevailing in the early
days. There's no point the cloud vendor can switch back and forth, as
there's no way for hypervisor to detect what kernel version even OS
the guest will be running ahead of time, until the guest gets fully
loaded and boots up, right?
-Siwei
>
> The way I see it, if you don't need migration, just use PT without
> failover. If you do, then you either use virtio or failover if guest
> supports it.
>
>> >
>> > We can move some of this code to management as well, architecturally it
>> > does not make too much sense but it might be easier implementation-wise.
>> >
>> > HTH
>> >
>> > On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
>> >> Ping on this patch now that the kernel patches are accepted into davem's net-next tree.
>> >> https://patchwork.ozlabs.org/cover/920005/
>> >>
>> >>
>> >> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>> >> > This feature bit can be used by hypervisor to indicate virtio_net device to
>> >> > act as a standby for another device with the same MAC address.
>> >> >
>> >> > I tested this with a small change to the patch to mark the STANDBY feature 'true'
>> >> > by default as i am using libvirt to start the VMs.
>> >> > Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
>> >> > XML file?
>> >> >
>> >> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> >> > ---
>> >> > hw/net/virtio-net.c | 2 ++
>> >> > include/standard-headers/linux/virtio_net.h | 3 +++
>> >> > 2 files changed, 5 insertions(+)
>> >> >
>> >> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> >> > index 90502fca7c..38b3140670 100644
>> >> > --- a/hw/net/virtio-net.c
>> >> > +++ b/hw/net/virtio-net.c
>> >> > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>> >> > true),
>> >> > DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>> >> > DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>> >> > + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY,
>> >> > + false),
>> >> > DEFINE_PROP_END_OF_LIST(),
>> >> > };
>> >> > diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>> >> > index e9f255ea3f..01ec09684c 100644
>> >> > --- a/include/standard-headers/linux/virtio_net.h
>> >> > +++ b/include/standard-headers/linux/virtio_net.h
>> >> > @@ -57,6 +57,9 @@
>> >> > * Steering */
>> >> > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>> >> > +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
>> >> > + * with the same MAC.
>> >> > + */
>> >> > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>> >> > #ifndef VIRTIO_NET_NO_LEGACY
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>> >
^ permalink raw reply
* Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
From: Joe Perches @ 2018-06-05 21:59 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-efi, brijesh.singh, boris.ostrovsky, J. Kiszka, Will Deacon,
jarkko.sakkinen, virtualization, Masahiro Yamada, Manoj Gupta,
hpa, akataria, Thiebaud Weksteen, mawilcox, x86, Greg Hackmann,
Matthias Kaehlcke, geert, David Rientjes, Andrey Ryabinin,
thomas.lendacky, Arnd Bergmann, Linux Kbuild mailing list,
Kees Cook, rostedt, Cao jin, Michal Marek, Josh Poimboeuf
In-Reply-To: <CAKwvOd=zuy0BJC3f3EKT_9Mg83qdZxAxw_RJ4okAMKrpKjJRHQ@mail.gmail.com>
On Tue, 2018-06-05 at 12:50 -0700, Nick Desaulniers wrote:
> On Tue, Jun 5, 2018 at 12:14 PM Joe Perches <joe@perches.com> wrote:
> >
> > On Tue, 2018-06-05 at 10:23 -0700, Joe Perches wrote:
> > > Perhaps these are simpler as
> > >
> > > #define __inline__ inline
> > > #define __inline inline
> >
> > Currently, there are these uses of inline variants in the kernel
> >
> > $ git grep -w inline | wc -l
> > 68410
> > $ git grep -w __inline__ | wc -l
> > 503
> > $ git grep -w __inline | wc -l
> > 57
> >
> > So it seems it's also reasonable to sed all uses of __inline to inline
> > and perhaps remove __inline eventually altogether.
> > (perhaps there are still too many __inline__ uses)
>
> Yeah, that sounds good. Should I split that into 3 patches:
>
> > Excluding scripts and a few other files,
> > here's a possible patch done with:
> >
> > $ git grep -w --name-only __inline | \
> > grep -vP '^(?:arch/alpha/|include/|scripts/)' | \
> > xargs sed -r -i -e 's/\b__inline\b/inline/g' \
> > -e 's/\binline\s+static\b/static inline/g'
> > ---
> > Documentation/trace/tracepoint-analysis.rst | 2 +-
> > drivers/staging/rtl8723bs/core/rtw_pwrctrl.c | 4 ++--
> > drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 2 +-
> > drivers/staging/rtl8723bs/include/drv_types.h | 6 +++---
> > drivers/staging/rtl8723bs/include/ieee80211.h | 6 +++---
> > drivers/staging/rtl8723bs/include/osdep_service.h | 10 +++++-----
> > drivers/staging/rtl8723bs/include/osdep_service_linux.h | 14 +++++++-------
> > drivers/staging/rtl8723bs/include/rtw_mlme.h | 14 +++++++-------
> > drivers/staging/rtl8723bs/include/rtw_recv.h | 16 ++++++++--------
> > drivers/staging/rtl8723bs/include/sta_info.h | 2 +-
> > drivers/staging/rtl8723bs/include/wifi.h | 14 +++++++-------
> > drivers/staging/rtl8723bs/include/wlan_bssdef.h | 2 +-
> > lib/zstd/mem.h | 2 +-
> > 13 files changed, 47 insertions(+), 47 deletions(-)
>
>
> 1 for documentation, 1 for rtl8723bs, 1 for zstd?
Seems sensible to me.
> Follow up set or include in v3?
Your choice. Probably a follow up would work best.
Also, the remaining __inline uses would be:
arch/alpha/include/asm/compiler.h:#undef __inline
include/linux/compiler-gcc.h:#define __inline __inline __attribute__((always_inline,unused)) notrace
include/linux/compiler-gcc.h:#define __inline __inline __attribute__((unused)) notrace
scripts/checkpatch.pl:our $Inline = qr{inline|__always_inline|noinline|__inline|__inline__};
scripts/checkpatch.pl:# Check for __inline__ and __inline, prefer inline
scripts/checkpatch.pl: $line =~ /\b(__inline__|__inline)\b/) {
scripts/checkpatch.pl: $fixed[$fixlinenr] =~ s/\b(__inline__|__inline)\b/inline/;
scripts/genksyms/keywords.c: { "__inline", INLINE_KEYW },
scripts/kernel-doc: $prototype =~ s/^__inline +//;
So all of these could be removed/updated appropriately too
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-05 21:32 UTC (permalink / raw)
To: Siwei Liu
Cc: Alexander Duyck, virtio-dev, qemu-devel, Samudrala, Sridhar,
virtualization
In-Reply-To: <CADGSJ21Qf6KpLzr+og5DOFjMPqu69n6dWRLvvj1FGAR5kVRNew@mail.gmail.com>
On Tue, Jun 05, 2018 at 02:16:44PM -0700, Siwei Liu wrote:
> Good to see this discussion going. I share the same feeling that the
> decision of plugging the primary (passthrough) should only be made
> until guest driver acknowledges DRIVER_OK and _F_STANDBY.
> Architecturally this intelligence should be baken to QEMU itself
> rather than moving up to management stack, such as libvirt.
>
> A few questions in line below.
>
> On Tue, Jun 5, 2018 at 5:33 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > I don't think this is sufficient.
> >
> > If both primary and standby devices are present, a legacy guest without
> > support for the feature might see two devices with same mac and get
> > confused.
> >
> > I think that we should only make primary visible after guest acked the
> > backup feature bit.
> >
> > And on reset or when backup is cleared in some other way, unplug the
> > primary.
> >
> > Something like the below will do the job:
> >
> > Primary device is added with a special "primary-failover" flag.
>
> I wonder if you envision this flag as a user interface or some
> internal attribute/property to QEMU device?
>
> Since QEMU needs to associate this particular primary-failover device
> with a virtio standby instance for checking DRIVER_OK as you indicate
> below, I wonder if the group ID thing will be helpful to set
> "primary-failover" flag internally without having to add another
> option in CLI?
I haven't thought about it but it's an option.
> > A virtual machine is then initialized with just a standby virtio
> > device. Primary is not yet added.
> >
> > Later QEMU detects that guest driver device set DRIVER_OK.
> > It then exposes the primary device to the guest, and triggers
> > a device addition event (hot-plug event) for it.
>
> Sounds OK. While there might be a small window the guest already
> starts to use virtio interface before the passthrough gets plugged in,
> I think that should be fine.
>
> Another option is to wait until the primary appears and gets
> registered in the guest, so it can bring up the upper failover
> interface.
We can't be sure this will ever happen, can we? We might be asked to
migrate at any time.
> >
> > If QEMU detects guest driver removal, it initiates a hot-unplug sequence
> > to remove the primary driver. In particular, if QEMU detects guest
> > re-initialization (e.g. by detecting guest reset) it immediately removes
> > the primary device.
>
> Agreed.
>
> For legacy guest, user might prefer seeing a single passthrough device
> rather than a virtio device. I would hope there's an option to allow
> it to happen, instead of removing all passthrough devices.
>
> Regards,
> -Siwei
I don't see a way to make it work since then you can't migrate, can you?
The way I see it, if you don't need migration, just use PT without
failover. If you do, then you either use virtio or failover if guest
supports it.
> >
> > We can move some of this code to management as well, architecturally it
> > does not make too much sense but it might be easier implementation-wise.
> >
> > HTH
> >
> > On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
> >> Ping on this patch now that the kernel patches are accepted into davem's net-next tree.
> >> https://patchwork.ozlabs.org/cover/920005/
> >>
> >>
> >> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> >> > This feature bit can be used by hypervisor to indicate virtio_net device to
> >> > act as a standby for another device with the same MAC address.
> >> >
> >> > I tested this with a small change to the patch to mark the STANDBY feature 'true'
> >> > by default as i am using libvirt to start the VMs.
> >> > Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
> >> > XML file?
> >> >
> >> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> >> > ---
> >> > hw/net/virtio-net.c | 2 ++
> >> > include/standard-headers/linux/virtio_net.h | 3 +++
> >> > 2 files changed, 5 insertions(+)
> >> >
> >> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> > index 90502fca7c..38b3140670 100644
> >> > --- a/hw/net/virtio-net.c
> >> > +++ b/hw/net/virtio-net.c
> >> > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
> >> > true),
> >> > DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
> >> > DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> >> > + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY,
> >> > + false),
> >> > DEFINE_PROP_END_OF_LIST(),
> >> > };
> >> > diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
> >> > index e9f255ea3f..01ec09684c 100644
> >> > --- a/include/standard-headers/linux/virtio_net.h
> >> > +++ b/include/standard-headers/linux/virtio_net.h
> >> > @@ -57,6 +57,9 @@
> >> > * Steering */
> >> > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> >> > +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
> >> > + * with the same MAC.
> >> > + */
> >> > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> >> > #ifndef VIRTIO_NET_NO_LEGACY
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> >
^ permalink raw reply
* Re: [PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
From: Arnd Bergmann @ 2018-06-05 21:31 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-efi, Brijesh Singh, Boris Ostrovsky, Jan Kiszka,
Will Deacon, Jarkko Sakkinen, virtualization, Masahiro Yamada,
manojgupta, H. Peter Anvin, akataria, tweek, mawilcox,
the arch/x86 maintainers, Greg Hackmann, Matthias Kaehlcke,
Geert Uytterhoeven, David Rientjes, Andrey Ryabinin,
Lendacky, Thomas, Linux Kbuild mailing list, Kees Cook,
Steven Rostedt, Cao jin <caoj.f>
In-Reply-To: <CAK8P3a0v6sr_oZmYTZV7E-uJ8JdG6D8JqUesHDPkPY2UrWKdvw@mail.gmail.com>
On Tue, Jun 5, 2018 at 11:28 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tue, Jun 5, 2018 at 7:05 PM, Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> The semantics of extern inline has changed since gnu89. This means that
>> folks using GCC versions >= 5.1 may see symbol redefinition errors at
>> link time for subdirs that override KBUILD_CFLAGS (making the C standard
>> used implicit) regardless of this patch. This has been cleaned up
>> earlier in the patch set, but is left as a note in the commit message
>> for future travelers.
>
> I think the keyword you are missing is
>
> __attribute__((gnu_inline))
>
> which forces the gnu89 behavior on all compiler versions. It's been supported
> since gcc-4.2, so it should not cause problems on any compiler that is able
> to build an x86 kernel.
Nevermind, I just saw you already posted that.
Arnd
^ permalink raw reply
* Re: [PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
From: Arnd Bergmann @ 2018-06-05 21:28 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-efi, Brijesh Singh, Boris Ostrovsky, Jan Kiszka,
Will Deacon, Jarkko Sakkinen, virtualization, Masahiro Yamada,
manojgupta, H. Peter Anvin, akataria, tweek, mawilcox,
the arch/x86 maintainers, Greg Hackmann, Matthias Kaehlcke,
Geert Uytterhoeven, David Rientjes, Andrey Ryabinin,
Lendacky, Thomas, Linux Kbuild mailing list, Kees Cook,
Steven Rostedt, Cao jin <caoj.f>
In-Reply-To: <20180605170532.170361-3-ndesaulniers@google.com>
On Tue, Jun 5, 2018 at 7:05 PM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> The semantics of extern inline has changed since gnu89. This means that
> folks using GCC versions >= 5.1 may see symbol redefinition errors at
> link time for subdirs that override KBUILD_CFLAGS (making the C standard
> used implicit) regardless of this patch. This has been cleaned up
> earlier in the patch set, but is left as a note in the commit message
> for future travelers.
I think the keyword you are missing is
__attribute__((gnu_inline))
which forces the gnu89 behavior on all compiler versions. It's been supported
since gcc-4.2, so it should not cause problems on any compiler that is able
to build an x86 kernel.
Arnd
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Siwei Liu @ 2018-06-05 21:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Alexander Duyck, virtio-dev, qemu-devel, Samudrala, Sridhar,
virtualization
In-Reply-To: <20180605152344-mutt-send-email-mst@kernel.org>
Good to see this discussion going. I share the same feeling that the
decision of plugging the primary (passthrough) should only be made
until guest driver acknowledges DRIVER_OK and _F_STANDBY.
Architecturally this intelligence should be baken to QEMU itself
rather than moving up to management stack, such as libvirt.
A few questions in line below.
On Tue, Jun 5, 2018 at 5:33 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> I don't think this is sufficient.
>
> If both primary and standby devices are present, a legacy guest without
> support for the feature might see two devices with same mac and get
> confused.
>
> I think that we should only make primary visible after guest acked the
> backup feature bit.
>
> And on reset or when backup is cleared in some other way, unplug the
> primary.
>
> Something like the below will do the job:
>
> Primary device is added with a special "primary-failover" flag.
I wonder if you envision this flag as a user interface or some
internal attribute/property to QEMU device?
Since QEMU needs to associate this particular primary-failover device
with a virtio standby instance for checking DRIVER_OK as you indicate
below, I wonder if the group ID thing will be helpful to set
"primary-failover" flag internally without having to add another
option in CLI?
> A virtual machine is then initialized with just a standby virtio
> device. Primary is not yet added.
>
> Later QEMU detects that guest driver device set DRIVER_OK.
> It then exposes the primary device to the guest, and triggers
> a device addition event (hot-plug event) for it.
Sounds OK. While there might be a small window the guest already
starts to use virtio interface before the passthrough gets plugged in,
I think that should be fine.
Another option is to wait until the primary appears and gets
registered in the guest, so it can bring up the upper failover
interface.
>
> If QEMU detects guest driver removal, it initiates a hot-unplug sequence
> to remove the primary driver. In particular, if QEMU detects guest
> re-initialization (e.g. by detecting guest reset) it immediately removes
> the primary device.
Agreed.
For legacy guest, user might prefer seeing a single passthrough device
rather than a virtio device. I would hope there's an option to allow
it to happen, instead of removing all passthrough devices.
Regards,
-Siwei
>
> We can move some of this code to management as well, architecturally it
> does not make too much sense but it might be easier implementation-wise.
>
> HTH
>
> On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
>> Ping on this patch now that the kernel patches are accepted into davem's net-next tree.
>> https://patchwork.ozlabs.org/cover/920005/
>>
>>
>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>> > This feature bit can be used by hypervisor to indicate virtio_net device to
>> > act as a standby for another device with the same MAC address.
>> >
>> > I tested this with a small change to the patch to mark the STANDBY feature 'true'
>> > by default as i am using libvirt to start the VMs.
>> > Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
>> > XML file?
>> >
>> > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>> > ---
>> > hw/net/virtio-net.c | 2 ++
>> > include/standard-headers/linux/virtio_net.h | 3 +++
>> > 2 files changed, 5 insertions(+)
>> >
>> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> > index 90502fca7c..38b3140670 100644
>> > --- a/hw/net/virtio-net.c
>> > +++ b/hw/net/virtio-net.c
>> > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>> > true),
>> > DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>> > DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>> > + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY,
>> > + false),
>> > DEFINE_PROP_END_OF_LIST(),
>> > };
>> > diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>> > index e9f255ea3f..01ec09684c 100644
>> > --- a/include/standard-headers/linux/virtio_net.h
>> > +++ b/include/standard-headers/linux/virtio_net.h
>> > @@ -57,6 +57,9 @@
>> > * Steering */
>> > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>> > +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
>> > + * with the same MAC.
>> > + */
>> > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>> > #ifndef VIRTIO_NET_NO_LEGACY
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
^ permalink raw reply
* Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Michael S. Tsirkin @ 2018-06-05 20:37 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: alexander.h.duyck, virtio-dev, qemu-devel, virtualization
In-Reply-To: <0a69d897-d084-8a7e-a681-86a49ab8f59a@intel.com>
On Tue, Jun 05, 2018 at 01:20:33PM -0700, Samudrala, Sridhar wrote:
>
> On 6/5/2018 5:33 AM, Michael S. Tsirkin wrote:
> > I don't think this is sufficient.
>
> Sure. This is not sufficient for a complete solution, but is Qemu the right place
> to manage primary/standby interfaces?
>
> I think the other steps including plugging/unplugging the primary interface needs
> to handled by some orchestration layer.
I don't see any real benefit to it but yes, you can push it up the stack.
Unfortunately the patch still won't be sufficient then, see below :)
>
> >
> > If both primary and standby devices are present, a legacy guest without
> > support for the feature might see two devices with same mac and get
> > confused.
>
> Yes. Isn't this possible today when a VM is started with a mis-configured domain
> XML file that passes a virtio-net interface and a VF with the same MAC?
Yes, so if you misconfigure the hypervisor then your guest won't run.
Not sure what you are trying to say by this though.
>
> >
> > I think that we should only make primary visible after guest acked the
> > backup feature bit.
>
> The primary can be plugged/unplugged at any time by the management layer.
> So i guess this needs to be handled in the libvirt layer.
management just wants to give the primary to guest and later take it back,
it really does not care about the details of the process,
so I don't see what does pushing it up the stack buy you.
So I don't think it *needs* to be done in libvirt. It probably can if you
add a bunch of hooks so it knows whenever vm reboots, driver binds and
unbinds from device, and can check that backup flag was set.
If you are pushing for a setup like that please get a buy-in
from libvirt maintainers or better write a patch.
> >
> > And on reset or when backup is cleared in some other way, unplug the
> > primary.
> >
> > Something like the below will do the job:
> >
> > Primary device is added with a special "primary-failover" flag.
>
> Are you suggesting an extension to Qemu to add another flag for
> primary device too?
Exactly.
> > A virtual machine is then initialized with just a standby virtio
> > device. Primary is not yet added.
> >
> > Later QEMU detects that guest driver device set DRIVER_OK.
> > It then exposes the primary device to the guest, and triggers
> > a device addition event (hot-plug event) for it.
> >
> > If QEMU detects guest driver removal, it initiates a hot-unplug sequence
> > to remove the primary driver. In particular, if QEMU detects guest
> > re-initialization (e.g. by detecting guest reset) it immediately removes
> > the primary device.
> >
> > We can move some of this code to management as well, architecturally it
> > does not make too much sense but it might be easier implementation-wise.
> >
> > HTH
> >
> > On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
> > > Ping on this patch now that the kernel patches are accepted into davem's net-next tree.
> > > https://patchwork.ozlabs.org/cover/920005/
> > >
> > >
> > > On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
> > > > This feature bit can be used by hypervisor to indicate virtio_net device to
> > > > act as a standby for another device with the same MAC address.
> > > >
> > > > I tested this with a small change to the patch to mark the STANDBY feature 'true'
> > > > by default as i am using libvirt to start the VMs.
> > > > Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
> > > > XML file?
> > > >
> > > > Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> > > > ---
> > > > hw/net/virtio-net.c | 2 ++
> > > > include/standard-headers/linux/virtio_net.h | 3 +++
> > > > 2 files changed, 5 insertions(+)
> > > >
> > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > > index 90502fca7c..38b3140670 100644
> > > > --- a/hw/net/virtio-net.c
> > > > +++ b/hw/net/virtio-net.c
> > > > @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
> > > > true),
> > > > DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
> > > > DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> > > > + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY,
> > > > + false),
> > > > DEFINE_PROP_END_OF_LIST(),
> > > > };
> > > > diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
> > > > index e9f255ea3f..01ec09684c 100644
> > > > --- a/include/standard-headers/linux/virtio_net.h
> > > > +++ b/include/standard-headers/linux/virtio_net.h
> > > > @@ -57,6 +57,9 @@
> > > > * Steering */
> > > > #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
> > > > +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
> > > > + * with the same MAC.
> > > > + */
> > > > #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
> > > > #ifndef VIRTIO_NET_NO_LEGACY
^ permalink raw reply
* Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net
From: Samudrala, Sridhar @ 2018-06-05 20:20 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: alexander.h.duyck, virtio-dev, qemu-devel, virtualization
In-Reply-To: <20180605152344-mutt-send-email-mst@kernel.org>
On 6/5/2018 5:33 AM, Michael S. Tsirkin wrote:
> I don't think this is sufficient.
Sure. This is not sufficient for a complete solution, but is Qemu the right place
to manage primary/standby interfaces?
I think the other steps including plugging/unplugging the primary interface needs
to handled by some orchestration layer.
>
> If both primary and standby devices are present, a legacy guest without
> support for the feature might see two devices with same mac and get
> confused.
Yes. Isn't this possible today when a VM is started with a mis-configured domain
XML file that passes a virtio-net interface and a VF with the same MAC?
>
> I think that we should only make primary visible after guest acked the
> backup feature bit.
The primary can be plugged/unplugged at any time by the management layer. So
i guess this needs to be handled in the libvirt layer.
>
> And on reset or when backup is cleared in some other way, unplug the
> primary.
>
> Something like the below will do the job:
>
> Primary device is added with a special "primary-failover" flag.
Are you suggesting an extension to Qemu to add another flag for
primary device too?
> A virtual machine is then initialized with just a standby virtio
> device. Primary is not yet added.
>
> Later QEMU detects that guest driver device set DRIVER_OK.
> It then exposes the primary device to the guest, and triggers
> a device addition event (hot-plug event) for it.
>
> If QEMU detects guest driver removal, it initiates a hot-unplug sequence
> to remove the primary driver. In particular, if QEMU detects guest
> re-initialization (e.g. by detecting guest reset) it immediately removes
> the primary device.
>
> We can move some of this code to management as well, architecturally it
> does not make too much sense but it might be easier implementation-wise.
>
> HTH
>
> On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote:
>> Ping on this patch now that the kernel patches are accepted into davem's net-next tree.
>> https://patchwork.ozlabs.org/cover/920005/
>>
>>
>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote:
>>> This feature bit can be used by hypervisor to indicate virtio_net device to
>>> act as a standby for another device with the same MAC address.
>>>
>>> I tested this with a small change to the patch to mark the STANDBY feature 'true'
>>> by default as i am using libvirt to start the VMs.
>>> Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt
>>> XML file?
>>>
>>> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
>>> ---
>>> hw/net/virtio-net.c | 2 ++
>>> include/standard-headers/linux/virtio_net.h | 3 +++
>>> 2 files changed, 5 insertions(+)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 90502fca7c..38b3140670 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = {
>>> true),
>>> DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>>> DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>> + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY,
>>> + false),
>>> DEFINE_PROP_END_OF_LIST(),
>>> };
>>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>>> index e9f255ea3f..01ec09684c 100644
>>> --- a/include/standard-headers/linux/virtio_net.h
>>> +++ b/include/standard-headers/linux/virtio_net.h
>>> @@ -57,6 +57,9 @@
>>> * Steering */
>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
>>> +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device
>>> + * with the same MAC.
>>> + */
>>> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
>>> #ifndef VIRTIO_NET_NO_LEGACY
^ permalink raw reply
* Re: [PATCH v2 1/2] compiler-gcc.h: add gnu_inline to all inline declarations
From: Joe Perches @ 2018-06-05 19:13 UTC (permalink / raw)
To: Nick Desaulniers, akpm, ard.biesheuvel, aryabinin, akataria,
boris.ostrovsky, brijesh.singh, caoj.fnst, gregkh, hpa,
jan.kiszka, jarkko.sakkinen, jgross, jpoimboe, kirill.shutemov,
mingo, mjg59, mka, pombredanne, rostedt, tglx, thomas.lendacky,
tweek
Cc: linux-efi, mawilcox, arnd, tstellar, x86, will.deacon, astrachan,
ghackmann, linux-kernel, yamada.masahiro, michal.lkml, geert,
manojgupta, keescook, sedat.dilek, rientjes, virtualization,
linux-kbuild
In-Reply-To: <202492204c2d5bd5ca27307cbca5e44673b739ed.camel@perches.com>
On Tue, 2018-06-05 at 10:23 -0700, Joe Perches wrote:
> Perhaps these are simpler as
>
> #define __inline__ inline
> #define __inline inline
Currently, there are these uses of inline variants in the kernel
$ git grep -w inline | wc -l
68410
$ git grep -w __inline__ | wc -l
503
$ git grep -w __inline | wc -l
57
So it seems it's also reasonable to sed all uses of __inline to inline
and perhaps remove __inline eventually altogether.
(perhaps there are still too many __inline__ uses)
Excluding scripts and a few other files,
here's a possible patch done with:
$ git grep -w --name-only __inline | \
grep -vP '^(?:arch/alpha/|include/|scripts/)' | \
xargs sed -r -i -e 's/\b__inline\b/inline/g' \
-e 's/\binline\s+static\b/static inline/g'
---
Documentation/trace/tracepoint-analysis.rst | 2 +-
drivers/staging/rtl8723bs/core/rtw_pwrctrl.c | 4 ++--
drivers/staging/rtl8723bs/core/rtw_wlan_util.c | 2 +-
drivers/staging/rtl8723bs/include/drv_types.h | 6 +++---
drivers/staging/rtl8723bs/include/ieee80211.h | 6 +++---
drivers/staging/rtl8723bs/include/osdep_service.h | 10 +++++-----
drivers/staging/rtl8723bs/include/osdep_service_linux.h | 14 +++++++-------
drivers/staging/rtl8723bs/include/rtw_mlme.h | 14 +++++++-------
drivers/staging/rtl8723bs/include/rtw_recv.h | 16 ++++++++--------
drivers/staging/rtl8723bs/include/sta_info.h | 2 +-
drivers/staging/rtl8723bs/include/wifi.h | 14 +++++++-------
drivers/staging/rtl8723bs/include/wlan_bssdef.h | 2 +-
lib/zstd/mem.h | 2 +-
13 files changed, 47 insertions(+), 47 deletions(-)
diff --git a/Documentation/trace/tracepoint-analysis.rst b/Documentation/trace/tracepoint-analysis.rst
index a4d3ff2e5efb..310f4c579cff 100644
--- a/Documentation/trace/tracepoint-analysis.rst
+++ b/Documentation/trace/tracepoint-analysis.rst
@@ -315,7 +315,7 @@ To see where within the function pixmanFillsse2 things are going wrong:
0.00 : 34eeb: 0f 18 08 prefetcht0 (%eax)
: }
:
- : extern __inline void __attribute__((__gnu_inline__, __always_inline__, _
+ : extern inline void __attribute__((__gnu_inline__, __always_inline__, _
: _mm_store_si128 (__m128i *__P, __m128i __B) : {
: *__P = __B;
12.40 : 34eee: 66 0f 7f 80 40 ff ff movdqa %xmm0,-0xc0(%eax)
diff --git a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
index 85f7769ecc2d..811492e64350 100644
--- a/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
+++ b/drivers/staging/rtl8723bs/core/rtw_pwrctrl.c
@@ -839,12 +839,12 @@ static void pwr_rpwm_timeout_handler(struct timer_list *t)
_set_workitem(&pwrpriv->rpwmtimeoutwi);
}
-static __inline void register_task_alive(struct pwrctrl_priv *pwrctrl, u32 tag)
+static inline void register_task_alive(struct pwrctrl_priv *pwrctrl, u32 tag)
{
pwrctrl->alives |= tag;
}
-static __inline void unregister_task_alive(struct pwrctrl_priv *pwrctrl, u32 tag)
+static inline void unregister_task_alive(struct pwrctrl_priv *pwrctrl, u32 tag)
{
pwrctrl->alives &= ~tag;
}
diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
index f6dc26c8bd3d..4e16087ef815 100644
--- a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
+++ b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
@@ -493,7 +493,7 @@ void set_channel_bwmode(struct adapter *padapter, unsigned char channel, unsigne
mutex_unlock(&(adapter_to_dvobj(padapter)->setch_mutex));
}
-__inline u8 *get_my_bssid(struct wlan_bssid_ex *pnetwork)
+inline u8 *get_my_bssid(struct wlan_bssid_ex *pnetwork)
{
return pnetwork->MacAddress;
}
diff --git a/drivers/staging/rtl8723bs/include/drv_types.h b/drivers/staging/rtl8723bs/include/drv_types.h
index 16b81b1a3f33..5d487243a822 100644
--- a/drivers/staging/rtl8723bs/include/drv_types.h
+++ b/drivers/staging/rtl8723bs/include/drv_types.h
@@ -493,7 +493,7 @@ struct dvobj_priv
#define dvobj_to_pwrctl(dvobj) (&(dvobj->pwrctl_priv))
#define pwrctl_to_dvobj(pwrctl) container_of(pwrctl, struct dvobj_priv, pwrctl_priv)
-__inline static struct device *dvobj_to_dev(struct dvobj_priv *dvobj)
+static inline struct device *dvobj_to_dev(struct dvobj_priv *dvobj)
{
/* todo: get interface type from dvobj and the return the dev accordingly */
#ifdef RTW_DVOBJ_CHIP_HW_TYPE
@@ -651,14 +651,14 @@ struct adapter {
/* define RTW_DISABLE_FUNC(padapter, func) (atomic_add(&adapter_to_dvobj(padapter)->disable_func, (func))) */
/* define RTW_ENABLE_FUNC(padapter, func) (atomic_sub(&adapter_to_dvobj(padapter)->disable_func, (func))) */
-__inline static void RTW_DISABLE_FUNC(struct adapter *padapter, int func_bit)
+static inline void RTW_DISABLE_FUNC(struct adapter *padapter, int func_bit)
{
int df = atomic_read(&adapter_to_dvobj(padapter)->disable_func);
df |= func_bit;
atomic_set(&adapter_to_dvobj(padapter)->disable_func, df);
}
-__inline static void RTW_ENABLE_FUNC(struct adapter *padapter, int func_bit)
+static inline void RTW_ENABLE_FUNC(struct adapter *padapter, int func_bit)
{
int df = atomic_read(&adapter_to_dvobj(padapter)->disable_func);
df &= ~(func_bit);
diff --git a/drivers/staging/rtl8723bs/include/ieee80211.h b/drivers/staging/rtl8723bs/include/ieee80211.h
index c8e5251c2760..78a4f2d836ce 100644
--- a/drivers/staging/rtl8723bs/include/ieee80211.h
+++ b/drivers/staging/rtl8723bs/include/ieee80211.h
@@ -858,18 +858,18 @@ enum ieee80211_state {
#define IP_FMT "%pI4"
#define IP_ARG(x) (x)
-extern __inline int is_multicast_mac_addr(const u8 *addr)
+extern inline int is_multicast_mac_addr(const u8 *addr)
{
return ((addr[0] != 0xff) && (0x01 & addr[0]));
}
-extern __inline int is_broadcast_mac_addr(const u8 *addr)
+extern inline int is_broadcast_mac_addr(const u8 *addr)
{
return ((addr[0] == 0xff) && (addr[1] == 0xff) && (addr[2] == 0xff) && \
(addr[3] == 0xff) && (addr[4] == 0xff) && (addr[5] == 0xff));
}
-extern __inline int is_zero_mac_addr(const u8 *addr)
+extern inline int is_zero_mac_addr(const u8 *addr)
{
return ((addr[0] == 0x00) && (addr[1] == 0x00) && (addr[2] == 0x00) && \
(addr[3] == 0x00) && (addr[4] == 0x00) && (addr[5] == 0x00));
diff --git a/drivers/staging/rtl8723bs/include/osdep_service.h b/drivers/staging/rtl8723bs/include/osdep_service.h
index e62ed71e1d80..ffaf9e366364 100644
--- a/drivers/staging/rtl8723bs/include/osdep_service.h
+++ b/drivers/staging/rtl8723bs/include/osdep_service.h
@@ -118,12 +118,12 @@ int _rtw_netif_rx(_nic_hdl ndev, struct sk_buff *skb);
extern void _rtw_init_queue(struct __queue *pqueue);
-static __inline void thread_enter(char *name)
+static inline void thread_enter(char *name)
{
allow_signal(SIGTERM);
}
-__inline static void flush_signals_thread(void)
+static inline void flush_signals_thread(void)
{
if (signal_pending (current))
{
@@ -133,7 +133,7 @@ __inline static void flush_signals_thread(void)
#define rtw_warn_on(condition) WARN_ON(condition)
-__inline static int rtw_bug_check(void *parg1, void *parg2, void *parg3, void *parg4)
+static inline int rtw_bug_check(void *parg1, void *parg2, void *parg3, void *parg4)
{
int ret = true;
@@ -144,7 +144,7 @@ __inline static int rtw_bug_check(void *parg1, void *parg2, void *parg3, void *p
#define _RND(sz, r) ((((sz)+((r)-1))/(r))*(r))
#define RND4(x) (((x >> 2) + (((x & 3) == 0) ? 0: 1)) << 2)
-__inline static u32 _RND4(u32 sz)
+static inline u32 _RND4(u32 sz)
{
u32 val;
@@ -155,7 +155,7 @@ __inline static u32 _RND4(u32 sz)
}
-__inline static u32 _RND8(u32 sz)
+static inline u32 _RND8(u32 sz)
{
u32 val;
diff --git a/drivers/staging/rtl8723bs/include/osdep_service_linux.h b/drivers/staging/rtl8723bs/include/osdep_service_linux.h
index 711863d74a01..a8d5456d7f85 100644
--- a/drivers/staging/rtl8723bs/include/osdep_service_linux.h
+++ b/drivers/staging/rtl8723bs/include/osdep_service_linux.h
@@ -74,12 +74,12 @@
typedef struct work_struct _workitem;
-__inline static struct list_head *get_next(struct list_head *list)
+static inline struct list_head *get_next(struct list_head *list)
{
return list->next;
}
-__inline static struct list_head *get_list_head(struct __queue *queue)
+static inline struct list_head *get_list_head(struct __queue *queue)
{
return (&(queue->queue));
}
@@ -88,28 +88,28 @@ __inline static struct list_head *get_list_head(struct __queue *queue)
#define LIST_CONTAINOR(ptr, type, member) \
container_of(ptr, type, member)
-__inline static void _set_timer(_timer *ptimer, u32 delay_time)
+static inline void _set_timer(_timer *ptimer, u32 delay_time)
{
mod_timer(ptimer , (jiffies+(delay_time*HZ/1000)));
}
-__inline static void _cancel_timer(_timer *ptimer, u8 *bcancelled)
+static inline void _cancel_timer(_timer *ptimer, u8 *bcancelled)
{
del_timer_sync(ptimer);
*bcancelled = true;/* true == 1; false == 0 */
}
-__inline static void _init_workitem(_workitem *pwork, void *pfunc, void *cntx)
+static inline void _init_workitem(_workitem *pwork, void *pfunc, void *cntx)
{
INIT_WORK(pwork, pfunc);
}
-__inline static void _set_workitem(_workitem *pwork)
+static inline void _set_workitem(_workitem *pwork)
{
schedule_work(pwork);
}
-__inline static void _cancel_workitem_sync(_workitem *pwork)
+static inline void _cancel_workitem_sync(_workitem *pwork)
{
cancel_work_sync(pwork);
}
diff --git a/drivers/staging/rtl8723bs/include/rtw_mlme.h b/drivers/staging/rtl8723bs/include/rtw_mlme.h
index 2e4f12b54929..40af3a34ed05 100644
--- a/drivers/staging/rtl8723bs/include/rtw_mlme.h
+++ b/drivers/staging/rtl8723bs/include/rtw_mlme.h
@@ -533,13 +533,13 @@ extern sint rtw_select_and_join_from_scanned_queue(struct mlme_priv *pmlmepriv);
extern sint rtw_set_key(struct adapter *adapter, struct security_priv *psecuritypriv, sint keyid, u8 set_tx, bool enqueue);
extern sint rtw_set_auth(struct adapter *adapter, struct security_priv *psecuritypriv);
-__inline static u8 *get_bssid(struct mlme_priv *pmlmepriv)
+static inline u8 *get_bssid(struct mlme_priv *pmlmepriv)
{ /* if sta_mode:pmlmepriv->cur_network.network.MacAddress => bssid */
/* if adhoc_mode:pmlmepriv->cur_network.network.MacAddress => ibss mac address */
return pmlmepriv->cur_network.network.MacAddress;
}
-__inline static sint check_fwstate(struct mlme_priv *pmlmepriv, sint state)
+static inline sint check_fwstate(struct mlme_priv *pmlmepriv, sint state)
{
if (pmlmepriv->fw_state & state)
return true;
@@ -547,7 +547,7 @@ __inline static sint check_fwstate(struct mlme_priv *pmlmepriv, sint state)
return false;
}
-__inline static sint get_fwstate(struct mlme_priv *pmlmepriv)
+static inline sint get_fwstate(struct mlme_priv *pmlmepriv)
{
return pmlmepriv->fw_state;
}
@@ -559,7 +559,7 @@ __inline static sint get_fwstate(struct mlme_priv *pmlmepriv)
* ### NOTE:#### (!!!!)
* MUST TAKE CARE THAT BEFORE CALLING THIS FUNC, YOU SHOULD HAVE LOCKED pmlmepriv->lock
*/
-__inline static void set_fwstate(struct mlme_priv *pmlmepriv, sint state)
+static inline void set_fwstate(struct mlme_priv *pmlmepriv, sint state)
{
pmlmepriv->fw_state |= state;
/* FOR HW integration */
@@ -568,7 +568,7 @@ __inline static void set_fwstate(struct mlme_priv *pmlmepriv, sint state)
}
}
-__inline static void _clr_fwstate_(struct mlme_priv *pmlmepriv, sint state)
+static inline void _clr_fwstate_(struct mlme_priv *pmlmepriv, sint state)
{
pmlmepriv->fw_state &= ~state;
/* FOR HW integration */
@@ -581,7 +581,7 @@ __inline static void _clr_fwstate_(struct mlme_priv *pmlmepriv, sint state)
* No Limit on the calling context,
* therefore set it to be the critical section...
*/
-__inline static void clr_fwstate(struct mlme_priv *pmlmepriv, sint state)
+static inline void clr_fwstate(struct mlme_priv *pmlmepriv, sint state)
{
spin_lock_bh(&pmlmepriv->lock);
if (check_fwstate(pmlmepriv, state) == true)
@@ -589,7 +589,7 @@ __inline static void clr_fwstate(struct mlme_priv *pmlmepriv, sint state)
spin_unlock_bh(&pmlmepriv->lock);
}
-__inline static void set_scanned_network_val(struct mlme_priv *pmlmepriv, sint val)
+static inline void set_scanned_network_val(struct mlme_priv *pmlmepriv, sint val)
{
spin_lock_bh(&pmlmepriv->lock);
pmlmepriv->num_of_scanned = val;
diff --git a/drivers/staging/rtl8723bs/include/rtw_recv.h b/drivers/staging/rtl8723bs/include/rtw_recv.h
index d4986f5685c5..eac0c4870dd4 100644
--- a/drivers/staging/rtl8723bs/include/rtw_recv.h
+++ b/drivers/staging/rtl8723bs/include/rtw_recv.h
@@ -413,7 +413,7 @@ struct recv_buf *rtw_dequeue_recvbuf (struct __queue *queue);
void rtw_reordering_ctrl_timeout_handler(struct timer_list *t);
-__inline static u8 *get_rxmem(union recv_frame *precvframe)
+static inline u8 *get_rxmem(union recv_frame *precvframe)
{
/* always return rx_head... */
if (precvframe == NULL)
@@ -422,7 +422,7 @@ __inline static u8 *get_rxmem(union recv_frame *precvframe)
return precvframe->u.hdr.rx_head;
}
-__inline static u8 *get_recvframe_data(union recv_frame *precvframe)
+static inline u8 *get_recvframe_data(union recv_frame *precvframe)
{
/* alwasy return rx_data */
@@ -433,7 +433,7 @@ __inline static u8 *get_recvframe_data(union recv_frame *precvframe)
}
-__inline static u8 *recvframe_pull(union recv_frame *precvframe, sint sz)
+static inline u8 *recvframe_pull(union recv_frame *precvframe, sint sz)
{
/* rx_data += sz; move rx_data sz bytes hereafter */
@@ -458,7 +458,7 @@ __inline static u8 *recvframe_pull(union recv_frame *precvframe, sint sz)
}
-__inline static u8 *recvframe_put(union recv_frame *precvframe, sint sz)
+static inline u8 *recvframe_put(union recv_frame *precvframe, sint sz)
{
/* rx_tai += sz; move rx_tail sz bytes hereafter */
@@ -487,7 +487,7 @@ __inline static u8 *recvframe_put(union recv_frame *precvframe, sint sz)
-__inline static u8 *recvframe_pull_tail(union recv_frame *precvframe, sint sz)
+static inline u8 *recvframe_pull_tail(union recv_frame *precvframe, sint sz)
{
/* rmv data from rx_tail (by yitsen) */
@@ -511,7 +511,7 @@ __inline static u8 *recvframe_pull_tail(union recv_frame *precvframe, sint sz)
}
-__inline static union recv_frame *rxmem_to_recvframe(u8 *rxmem)
+static inline union recv_frame *rxmem_to_recvframe(u8 *rxmem)
{
/* due to the design of 2048 bytes alignment of recv_frame, we can reference the union recv_frame */
/* from any given member of recv_frame. */
@@ -521,13 +521,13 @@ __inline static union recv_frame *rxmem_to_recvframe(u8 *rxmem)
}
-__inline static sint get_recvframe_len(union recv_frame *precvframe)
+static inline sint get_recvframe_len(union recv_frame *precvframe)
{
return precvframe->u.hdr.len;
}
-__inline static s32 translate_percentage_to_dbm(u32 SignalStrengthIndex)
+static inline s32 translate_percentage_to_dbm(u32 SignalStrengthIndex)
{
s32 SignalPower; /* in dBm. */
diff --git a/drivers/staging/rtl8723bs/include/sta_info.h b/drivers/staging/rtl8723bs/include/sta_info.h
index 84fa116fc5da..c1f7b9eed611 100644
--- a/drivers/staging/rtl8723bs/include/sta_info.h
+++ b/drivers/staging/rtl8723bs/include/sta_info.h
@@ -356,7 +356,7 @@ struct sta_priv {
};
-__inline static u32 wifi_mac_hash(u8 *mac)
+static inline u32 wifi_mac_hash(u8 *mac)
{
u32 x;
diff --git a/drivers/staging/rtl8723bs/include/wifi.h b/drivers/staging/rtl8723bs/include/wifi.h
index 530d698f50d9..475f2e07ad02 100644
--- a/drivers/staging/rtl8723bs/include/wifi.h
+++ b/drivers/staging/rtl8723bs/include/wifi.h
@@ -355,7 +355,7 @@ enum WIFI_REG_DOMAIN {
(addr[4] == 0xff) && (addr[5] == 0xff)) ? true : false \
)
-__inline static int IS_MCAST(unsigned char *da)
+static inline int IS_MCAST(unsigned char *da)
{
if ((*da) & 0x01)
return true;
@@ -363,20 +363,20 @@ __inline static int IS_MCAST(unsigned char *da)
return false;
}
-__inline static unsigned char * get_ra(unsigned char *pframe)
+static inline unsigned char * get_ra(unsigned char *pframe)
{
unsigned char *ra;
ra = GetAddr1Ptr(pframe);
return ra;
}
-__inline static unsigned char * get_ta(unsigned char *pframe)
+static inline unsigned char * get_ta(unsigned char *pframe)
{
unsigned char *ta;
ta = GetAddr2Ptr(pframe);
return ta;
}
-__inline static unsigned char * get_da(unsigned char *pframe)
+static inline unsigned char * get_da(unsigned char *pframe)
{
unsigned char *da;
unsigned int to_fr_ds = (GetToDs(pframe) << 1) | GetFrDs(pframe);
@@ -400,7 +400,7 @@ __inline static unsigned char * get_da(unsigned char *pframe)
}
-__inline static unsigned char * get_sa(unsigned char *pframe)
+static inline unsigned char * get_sa(unsigned char *pframe)
{
unsigned char *sa;
unsigned int to_fr_ds = (GetToDs(pframe) << 1) | GetFrDs(pframe);
@@ -423,7 +423,7 @@ __inline static unsigned char * get_sa(unsigned char *pframe)
return sa;
}
-__inline static unsigned char * get_hdr_bssid(unsigned char *pframe)
+static inline unsigned char * get_hdr_bssid(unsigned char *pframe)
{
unsigned char *sa = NULL;
unsigned int to_fr_ds = (GetToDs(pframe) << 1) | GetFrDs(pframe);
@@ -447,7 +447,7 @@ __inline static unsigned char * get_hdr_bssid(unsigned char *pframe)
}
-__inline static int IsFrameTypeCtrl(unsigned char *pframe)
+static inline int IsFrameTypeCtrl(unsigned char *pframe)
{
if (WIFI_CTRL_TYPE == GetFrameType(pframe))
return true;
diff --git a/drivers/staging/rtl8723bs/include/wlan_bssdef.h b/drivers/staging/rtl8723bs/include/wlan_bssdef.h
index af78d97980fa..767c444acc54 100644
--- a/drivers/staging/rtl8723bs/include/wlan_bssdef.h
+++ b/drivers/staging/rtl8723bs/include/wlan_bssdef.h
@@ -231,7 +231,7 @@ struct wlan_bssid_ex {
u8 IEs[MAX_IE_SZ]; /* timestamp, beacon interval, and capability information) */
} __packed;
-__inline static uint get_wlan_bssid_ex_sz(struct wlan_bssid_ex *bss)
+static inline uint get_wlan_bssid_ex_sz(struct wlan_bssid_ex *bss)
{
return (sizeof(struct wlan_bssid_ex) - MAX_IE_SZ + bss->IELength);
}
diff --git a/lib/zstd/mem.h b/lib/zstd/mem.h
index 3a0f34c8706c..739837a59ad6 100644
--- a/lib/zstd/mem.h
+++ b/lib/zstd/mem.h
@@ -27,7 +27,7 @@
/*-****************************************
* Compiler specifics
******************************************/
-#define ZSTD_STATIC static __inline __attribute__((unused))
+#define ZSTD_STATIC static inline __attribute__((unused))
/*-**************************************************************
* Basic Types
^ permalink raw reply related
* Re: [PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
From: H. Peter Anvin @ 2018-06-05 18:06 UTC (permalink / raw)
To: Nick Desaulniers
Cc: linux-efi, brijesh.singh, boris.ostrovsky, J. Kiszka, Will Deacon,
jarkko.sakkinen, virtualization, Masahiro Yamada, Manoj Gupta,
akataria, Thiebaud Weksteen, mawilcox, x86, Greg Hackmann,
Matthias Kaehlcke, geert, David Rientjes, Andrey Ryabinin,
thomas.lendacky, Arnd Bergmann, Linux Kbuild mailing list,
Kees Cook, rostedt, Cao jin, Michal Marek, Josh Poimboeuf,
sedat.di
In-Reply-To: <CAKwvOdk0v4gY04_pbMeUT7Ly5oRHetKorQXnjp_UVBabK74Ekw@mail.gmail.com>
On 06/05/18 10:52, Nick Desaulniers wrote:
>
> Does the kernel have a different calling convention for 32b x86? How
> does that work? regparm=3? Does that need to be added to the
> declaration?
>
Yes, -mregparm=3. No, doesn't need to be added to the declaration.
>> Something like this added to <asm/asm.h> might be useful; then you can
>> simply:
>>
>> push %_ASM_ARG1
>>
>> Version with fixed typo...
>
> Oh, nice, thanks! I'll pick this up and add it to my patch set for v3
> (or did you want me to review/sign-off now?) I can pick up Sedat's
> suggestion.
>
Add it to your patchset and add your own signoff underneath mine.
-hpa
^ permalink raw reply
* Re: [PATCH v2 2/2] x86: paravirt: make native_save_fl extern inline
From: H. Peter Anvin @ 2018-06-05 17:31 UTC (permalink / raw)
To: Nick Desaulniers, akpm, ard.biesheuvel, aryabinin, akataria,
boris.ostrovsky, brijesh.singh, caoj.fnst, gregkh, jan.kiszka,
jarkko.sakkinen, jgross, jpoimboe, kirill.shutemov, mingo, mjg59,
mka, pombredanne, rostedt, tglx, thomas.lendacky, tweek
Cc: linux-efi, mawilcox, arnd, tstellar, x86, will.deacon, astrachan,
ghackmann, linux-kernel, yamada.masahiro, michal.lkml, geert,
manojgupta, keescook, sedat.dilek, rientjes, virtualization,
linux-kbuild
In-Reply-To: <26c7fd1c-ab40-368f-c2e3-43a6b45157fd@zytor.com>
[-- Attachment #1: Type: text/plain, Size: 528 bytes --]
On 06/05/18 10:28, H. Peter Anvin wrote:
> On 06/05/18 10:05, Nick Desaulniers wrote:
>> +
>> +/*
>> + * void native_restore_fl(unsigned long flags)
>> + * %rdi: flags
>> + */
>> +ENTRY(native_restore_fl)
>> + push %_ASM_DI
>> + popf
>> + ret
>> +ENDPROC(native_restore_fl)
>> +EXPORT_SYMBOL(native_restore_fl)
>>
>
> To work on i386, this would have to be %_ASM_AX in that case.
>
> Something like this added to <asm/asm.h> might be useful; then you can
> simply:
>
> push %_ASM_ARG1
>
Version with fixed typo...
-hpa
[-- Attachment #2: 0001-x86-asm-add-_ASM_ARG-constants-for-argument-registes.patch --]
[-- Type: text/x-patch, Size: 2091 bytes --]
From 9946c03bc6648ea65e6f8e2576c390dca9555288 Mon Sep 17 00:00:00 2001
From: "H. Peter Anvin" <hpa@linux.intel.com>
Date: Tue, 5 Jun 2018 10:21:35 -0700
Subject: [PATCH] x86/asm: add _ASM_ARG* constants for argument registes to
<asm/asm.h>
i386 and x86-64 uses different registers for arguments; make them
available so we don't have to #ifdef in the actual code.
Native size and specified size (q, l, w, b) versions are provided.
Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>
---
arch/x86/include/asm/asm.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index 219faaec51df..990770f9e76b 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -46,6 +46,65 @@
#define _ASM_SI __ASM_REG(si)
#define _ASM_DI __ASM_REG(di)
+#ifndef __x86_64__
+/* 32 bit */
+
+#define _ASM_ARG1 _ASM_AX
+#define _ASM_ARG2 _ASM_DX
+#define _ASM_ARG3 _ASM_CX
+
+#define _ASM_ARG1L eax
+#define _ASM_ARG2L edx
+#define _ASM_ARG3L ecx
+
+#define _ASM_ARG1W ax
+#define _ASM_ARG2W dx
+#define _ASM_ARG3W cx
+
+#define _ASM_ARG1B al
+#define _ASM_ARG2B dl
+#define _ASM_ARG3B cl
+
+#else
+/* 64 bit */
+
+#define _ASM_ARG1 _ASM_DI
+#define _ASM_ARG2 _ASM_SI
+#define _ASM_ARG3 _ASM_DX
+#define _ASM_ARG4 _ASM_CX
+#define _ASM_ARG5 r8
+#define _ASM_ARG6 r9
+
+#define _ASM_ARG1Q rdi
+#define _ASM_ARG2Q rsi
+#define _ASM_ARG3Q rdx
+#define _ASM_ARG4Q rcx
+#define _ASM_ARG5Q r8
+#define _ASM_ARG6Q r9
+
+#define _ASM_ARG1L edi
+#define _ASM_ARG2L esi
+#define _ASM_ARG3L edx
+#define _ASM_ARG4L ecx
+#define _ASM_ARG5L r8d
+#define _ASM_ARG6L r9d
+
+#define _ASM_ARG1W di
+#define _ASM_ARG2W si
+#define _ASM_ARG3W dx
+#define _ASM_ARG4W cx
+#define _ASM_ARG5W r8w
+#define _ASM_ARG6W r9w
+
+#define _ASM_ARG1B dil
+#define _ASM_ARG2B sil
+#define _ASM_ARG3B dl
+#define _ASM_ARG4B cl
+#define _ASM_ARG5B r8b
+#define _ASM_ARG6B r9b
+
+#endif
+
/*
* Macros to generate condition code outputs from inline assembly,
* The output operand must be type "bool".
--
2.14.4
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
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