| Message ID | 1304454487-2539-2-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Sasha Levin <levinsasha928@gmail.com> wrote: > Increase idx only after updating the used element. > Not doing so may mark a buffer as used without having > it's head and length updated. > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > --- > tools/kvm/virtio.c | 19 ++++++++++++++++++- > 1 files changed, 18 insertions(+), 1 deletions(-) > > diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c > index 6249521..266a1b6 100644 > --- a/tools/kvm/virtio.c > +++ b/tools/kvm/virtio.c > @@ -1,15 +1,32 @@ > #include <linux/virtio_ring.h> > #include <stdint.h> > #include <sys/uio.h> > +#include <asm/system.h> If this system.h is included from the current kernel (and not from the system's) then: Acked-by: Ingo Molnar <mingo@elte.hu> Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-05-03 at 22:47 +0200, Ingo Molnar wrote: > * Sasha Levin <levinsasha928@gmail.com> wrote: > > > Increase idx only after updating the used element. > > Not doing so may mark a buffer as used without having > > it's head and length updated. > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > --- > > tools/kvm/virtio.c | 19 ++++++++++++++++++- > > 1 files changed, 18 insertions(+), 1 deletions(-) > > > > diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c > > index 6249521..266a1b6 100644 > > --- a/tools/kvm/virtio.c > > +++ b/tools/kvm/virtio.c > > @@ -1,15 +1,32 @@ > > #include <linux/virtio_ring.h> > > #include <stdint.h> > > #include <sys/uio.h> > > +#include <asm/system.h> > > If this system.h is included from the current kernel (and not from the > system's) then: > > Acked-by: Ingo Molnar <mingo@elte.hu> > The system.h that'll get picked up is '../../arch/$(ARCH)/include/asm/system.h' within the current kernel tree and not the system one. > Thanks, > > Ingo
* Sasha Levin <levinsasha928@gmail.com> wrote: > On Tue, 2011-05-03 at 22:47 +0200, Ingo Molnar wrote: > > * Sasha Levin <levinsasha928@gmail.com> wrote: > > > > > Increase idx only after updating the used element. > > > Not doing so may mark a buffer as used without having > > > it's head and length updated. > > > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > > --- > > > tools/kvm/virtio.c | 19 ++++++++++++++++++- > > > 1 files changed, 18 insertions(+), 1 deletions(-) > > > > > > diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c > > > index 6249521..266a1b6 100644 > > > --- a/tools/kvm/virtio.c > > > +++ b/tools/kvm/virtio.c > > > @@ -1,15 +1,32 @@ > > > #include <linux/virtio_ring.h> > > > #include <stdint.h> > > > #include <sys/uio.h> > > > +#include <asm/system.h> > > > > If this system.h is included from the current kernel (and not from the > > system's) then: > > > > Acked-by: Ingo Molnar <mingo@elte.hu> > > > > The system.h that'll get picked up is > '../../arch/$(ARCH)/include/asm/system.h' within the current kernel tree > and not the system one. ok, that's great - so my Ack stands. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/04/2011 12:41 PM, Sasha Levin wrote: > On Tue, 2011-05-03 at 22:47 +0200, Ingo Molnar wrote: >> * Sasha Levin <levinsasha928@gmail.com> wrote: >> >>> Increase idx only after updating the used element. >>> Not doing so may mark a buffer as used without having >>> it's head and length updated. >>> >>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>> --- >>> tools/kvm/virtio.c | 19 ++++++++++++++++++- >>> 1 files changed, 18 insertions(+), 1 deletions(-) >>> >>> diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c >>> index 6249521..266a1b6 100644 >>> --- a/tools/kvm/virtio.c >>> +++ b/tools/kvm/virtio.c >>> @@ -1,15 +1,32 @@ >>> #include <linux/virtio_ring.h> >>> #include <stdint.h> >>> #include <sys/uio.h> >>> +#include <asm/system.h> >> >> If this system.h is included from the current kernel (and not from the >> system's) then: >> >> Acked-by: Ingo Molnar <mingo@elte.hu> >> > > The system.h that'll get picked up is > '../../arch/$(ARCH)/include/asm/system.h' within the current kernel tree > and not the system one. >> Thanks, >> >> Ingo > Hi, With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master (kvm tools: Fix virt_queue__set_used_elem) I see this: asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make GEN include/common-cmds.h ../../include/asm-generic/bitops/fls64.h:33:2: error: #error BITS_PER_LONG not 32 or 64 ../../include/linux/bitops.h:133:2: error: #error BITS_PER_LONG not 32 or 64 ../../include/asm-generic/bitops/fls64.h:33:2: error: #error BITS_PER_LONG not 32 or 64 ../../include/linux/bitops.h:133:2: error: #error BITS_PER_LONG not 32 or 64 make: *** No rule to make target `virtio.d', needed by `kvm'. Stop. With 'make V=1', I found this one triggered the error: cc -M -MT virtio.o -DCONFIG_X86_32 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -Iinclude -I../../include -I../../arch/x86/include/ -Os -g -Werror -Wall -Wcast-align -Wformat=2 -Winit-self -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-system-headers -Wold-style-definition -Wredundant-decls -Wsign-compare -Wstrict-prototypes -Wundef -Wvolatile-register-var -Wwrite-strings virtio.c -o virtio.d
On Thu, 2011-05-05 at 07:47 +0800, Asias He wrote: > On 05/04/2011 12:41 PM, Sasha Levin wrote: > > On Tue, 2011-05-03 at 22:47 +0200, Ingo Molnar wrote: > >> * Sasha Levin <levinsasha928@gmail.com> wrote: > >> > >>> Increase idx only after updating the used element. > >>> Not doing so may mark a buffer as used without having > >>> it's head and length updated. > >>> > >>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >>> --- > >>> tools/kvm/virtio.c | 19 ++++++++++++++++++- > >>> 1 files changed, 18 insertions(+), 1 deletions(-) > >>> > >>> diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c > >>> index 6249521..266a1b6 100644 > >>> --- a/tools/kvm/virtio.c > >>> +++ b/tools/kvm/virtio.c > >>> @@ -1,15 +1,32 @@ > >>> #include <linux/virtio_ring.h> > >>> #include <stdint.h> > >>> #include <sys/uio.h> > >>> +#include <asm/system.h> > >> > >> If this system.h is included from the current kernel (and not from the > >> system's) then: > >> > >> Acked-by: Ingo Molnar <mingo@elte.hu> > >> > > > > The system.h that'll get picked up is > > '../../arch/$(ARCH)/include/asm/system.h' within the current kernel tree > > and not the system one. > >> Thanks, > >> > >> Ingo > > > > Hi, > > With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master > (kvm tools: Fix virt_queue__set_used_elem) > > I see this: > > asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make > GEN include/common-cmds.h > ../../include/asm-generic/bitops/fls64.h:33:2: error: #error > BITS_PER_LONG not 32 or 64 > ../../include/linux/bitops.h:133:2: error: #error BITS_PER_LONG not 32 or 64 > ../../include/asm-generic/bitops/fls64.h:33:2: error: #error > BITS_PER_LONG not 32 or 64 > ../../include/linux/bitops.h:133:2: error: #error BITS_PER_LONG not 32 or 64 > make: *** No rule to make target `virtio.d', needed by `kvm'. Stop. > > > With 'make V=1', I found this one triggered the error: > > cc -M -MT virtio.o -DCONFIG_X86_32 -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE > -Iinclude -I../../include -I../../arch/x86/include/ -Os -g -Werror -Wall > -Wcast-align -Wformat=2 -Winit-self -Wmissing-declarations > -Wmissing-prototypes -Wnested-externs -Wno-system-headers > -Wold-style-definition -Wredundant-decls -Wsign-compare > -Wstrict-prototypes -Wundef -Wvolatile-register-var -Wwrite-strings > virtio.c -o virtio.d > > It looks like gcc is trying to pull kernel headers instead the headers we have in include/linux. I'm not really sure why it happens though, I've tried building again with a clean kvm dir (in case I forgot to add a file to the commit) but it worked fine. Can you compare your include/linux dir to the one located on the master git tree?
* Asias He <asias.hejun@gmail.com> wrote: > With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master > (kvm tools: Fix virt_queue__set_used_elem) > > I see this: > > asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make > GEN include/common-cmds.h > ../../include/asm-generic/bitops/fls64.h:33:2: error: #error > BITS_PER_LONG not 32 or 64 The build fails here too, in a similar way, on a 32-bit Fedora 14 box. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 5, 2011 at 10:02 AM, Ingo Molnar <mingo@elte.hu> wrote: > > * Asias He <asias.hejun@gmail.com> wrote: > >> With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master >> (kvm tools: Fix virt_queue__set_used_elem) >> >> I see this: >> >> asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make >> GEN include/common-cmds.h >> ../../include/asm-generic/bitops/fls64.h:33:2: error: #error >> BITS_PER_LONG not 32 or 64 > > The build fails here too, in a similar way, on a 32-bit Fedora 14 box. Curious. It works fine on my box. I think it's missing #include <asm/bitsperlong.h>. I wonder why system.h isn't pulling that itself... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-05-05 at 10:13 +0300, Pekka Enberg wrote: > On Thu, May 5, 2011 at 10:02 AM, Ingo Molnar <mingo@elte.hu> wrote: > > > > * Asias He <asias.hejun@gmail.com> wrote: > > > >> With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master > >> (kvm tools: Fix virt_queue__set_used_elem) > >> > >> I see this: > >> > >> asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make > >> GEN include/common-cmds.h > >> ../../include/asm-generic/bitops/fls64.h:33:2: error: #error > >> BITS_PER_LONG not 32 or 64 > > > > The build fails here too, in a similar way, on a 32-bit Fedora 14 box. > > Curious. It works fine on my box. I think it's missing #include > <asm/bitsperlong.h>. I wonder why system.h isn't pulling that > itself... Pekka, Are you on a 64bit box? I suspect it's not working on 32bit boxes.
On Thu, May 5, 2011 at 10:18 AM, Sasha Levin <levinsasha928@gmail.com> wrote: > On Thu, 2011-05-05 at 10:13 +0300, Pekka Enberg wrote: >> On Thu, May 5, 2011 at 10:02 AM, Ingo Molnar <mingo@elte.hu> wrote: >> > >> > * Asias He <asias.hejun@gmail.com> wrote: >> > >> >> With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master >> >> (kvm tools: Fix virt_queue__set_used_elem) >> >> >> >> I see this: >> >> >> >> asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make >> >> GEN include/common-cmds.h >> >> ../../include/asm-generic/bitops/fls64.h:33:2: error: #error >> >> BITS_PER_LONG not 32 or 64 >> > >> > The build fails here too, in a similar way, on a 32-bit Fedora 14 box. >> >> Curious. It works fine on my box. I think it's missing #include >> <asm/bitsperlong.h>. I wonder why system.h isn't pulling that >> itself... > > Pekka, Are you on a 64bit box? > I suspect it's not working on 32bit boxes. Yes, I am. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pekka Enberg <penberg@kernel.org> wrote: > On Thu, May 5, 2011 at 10:02 AM, Ingo Molnar <mingo@elte.hu> wrote: > > > > * Asias He <asias.hejun@gmail.com> wrote: > > > >> With commit d7f0c07afeefa2d20739437306e4b8bb2853cf83 in master > >> (kvm tools: Fix virt_queue__set_used_elem) > >> > >> I see this: > >> > >> asias@hj:~/qemu-stuff/pekka.git/tools/kvm$ make > >> GEN include/common-cmds.h > >> ../../include/asm-generic/bitops/fls64.h:33:2: error: #error > >> BITS_PER_LONG not 32 or 64 > > > > The build fails here too, in a similar way, on a 32-bit Fedora 14 box. > > Curious. It works fine on my box. I think it's missing #include > <asm/bitsperlong.h>. I wonder why system.h isn't pulling that > itself... asm/system.h is one of the messier kernel headers, so i'm not surprised it has assymetric requirements on 64-bit and 32-bit systems. We could fix it (provide those dependencies), or we could pick tools/perf/perf.h's mb() definitions. That's much cruder than the nice kernel memory barriers though ... Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 5, 2011 at 10:47 AM, Ingo Molnar <mingo@elte.hu> wrote: >> > The build fails here too, in a similar way, on a 32-bit Fedora 14 box. >> >> Curious. It works fine on my box. I think it's missing #include >> <asm/bitsperlong.h>. I wonder why system.h isn't pulling that >> itself... > > asm/system.h is one of the messier kernel headers, so i'm not surprised it has > assymetric requirements on 64-bit and 32-bit systems. So does including <asm/bitsperlong.h> before <asm/system.h> fix the problem on 32-bit boxes? If so, I'd prefer we fix it like that for now. > We could fix it (provide those dependencies), or we could pick > tools/perf/perf.h's mb() definitions. That's much cruder than the nice kernel > memory barriers though ... That's the second best option, I think. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-05-05 at 10:52 +0300, Pekka Enberg wrote: > On Thu, May 5, 2011 at 10:47 AM, Ingo Molnar <mingo@elte.hu> wrote: > >> > The build fails here too, in a similar way, on a 32-bit Fedora 14 box. > >> > >> Curious. It works fine on my box. I think it's missing #include > >> <asm/bitsperlong.h>. I wonder why system.h isn't pulling that > >> itself... > > > > asm/system.h is one of the messier kernel headers, so i'm not surprised it has > > assymetric requirements on 64-bit and 32-bit systems. > > So does including <asm/bitsperlong.h> before <asm/system.h> fix the > problem on 32-bit boxes? If so, I'd prefer we fix it like that for > now. > > > We could fix it (provide those dependencies), or we could pick > > tools/perf/perf.h's mb() definitions. That's much cruder than the nice kernel > > memory barriers though ... > > That's the second best option, I think. I agree with Pekka, For the same reasons we provided the dependencies for including <linux/list.h> instead of just taking the source.
diff --git a/tools/kvm/virtio.c b/tools/kvm/virtio.c index 6249521..266a1b6 100644 --- a/tools/kvm/virtio.c +++ b/tools/kvm/virtio.c @@ -1,15 +1,32 @@ #include <linux/virtio_ring.h> #include <stdint.h> #include <sys/uio.h> +#include <asm/system.h> #include "kvm/kvm.h" #include "kvm/virtio.h" struct vring_used_elem *virt_queue__set_used_elem(struct virt_queue *queue, uint32_t head, uint32_t len) { struct vring_used_elem *used_elem; - used_elem = &queue->vring.used->ring[queue->vring.used->idx++ % queue->vring.num]; + used_elem = &queue->vring.used->ring[queue->vring.used->idx % queue->vring.num]; used_elem->id = head; used_elem->len = len; + + /* + * Use wmb to assure that used elem was updated with head and len. + * We need a wmb here since we can't advance idx unless we're ready + * to pass the used element to the guest. + */ + wmb(); + queue->vring.used->idx++; + + /* + * Use wmb to assure used idx has been increased before we signal the guest. + * Without a wmb here the guest may ignore the queue since it won't see + * an updated idx. + */ + wmb(); + return used_elem; }
Increase idx only after updating the used element. Not doing so may mark a buffer as used without having it's head and length updated. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- tools/kvm/virtio.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)