mbox

[PULL,00/48] ivshmem series

Message ID 1444159184-18153-1-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Pull-request

https://github.com/elmarco/qemu tags/ivshmem-series

Message

Marc-André Lureau Oct. 6, 2015, 7:18 p.m. UTC
From: Marc-André Lureau <marcandre.lureau@redhat.com>

The following changes since commit 5fdb4671b08e0d1631447e81348b2b50a6b85bf7:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2015-10-06 13:42:33 +0100)

are available in the git repository at:

  https://github.com/elmarco/qemu tags/ivshmem-series

for you to fetch changes up to 097cadb155ef22be286af1403240b4fbf0f038ef:

  ivshmem: use little-endian int64_t for the protocol (2015-10-06 21:17:22 +0200)

----------------------------------------------------------------
Ivshmem series

----------------------------------------------------------------
David Marchand (3):
      contrib: add ivshmem client and server
      docs: update ivshmem device spec
      ivshmem: add check on protocol version in QEMU

Marc-André Lureau (45):
      char: add qemu_chr_free()
      msix: add VMSTATE_MSIX_TEST
      ivhsmem: read do not accept more than sizeof(long)
      ivshmem: fix number of bytes to push to fifo
      ivshmem: factor out the incoming fifo handling
      ivshmem: remove unnecessary dup()
      ivshmem: remove superflous ivshmem_attr field
      ivshmem: remove useless doorbell field
      ivshmem: more qdev conversion
      ivshmem: remove last exit(1)
      ivshmem: limit maximum number of peers to G_MAXUINT16
      ivshmem: simplify around increase_dynamic_storage()
      ivshmem: allocate eventfds in resize_peers()
      ivshmem: remove useless ivshmem_update_irq() val argument
      ivshmem: initialize max_peer to -1
      ivshmem: remove max_peer field
      ivshmem: improve debug messages
      ivshmem: improve error handling
      ivshmem: print error on invalid peer id
      ivshmem: simplify a bit the code
      ivshmem: use common return
      ivshmem: use common is_power_of_2()
      ivshmem: migrate with VMStateDescription
      ivshmem: shmfd can be 0
      ivshmem: check shm isn't already initialized
      ivshmem: add device description
      ivshmem: fix pci_ivshmem_exit()
      ivshmem: replace 'guest' for 'peer' appropriately
      ivshmem: error on too many eventfd received
      ivshmem: reset mask on device reset
      ivshmem-client: check the number of vectors
      ivshmem-server: use a uint16 for client ID
      ivshmem-server: fix hugetlbfs support
      contrib: remove unnecessary strdup()
      msix: implement pba write (but read-only)
      qtest: add qtest_add_abrt_handler()
      glib-compat: add 2.38/2.40/2.46 asserts
      tests: add ivshmem qtest
      ivshmem: do not keep shm_fd open
      ivshmem: use qemu_strtosz()
      ivshmem: add hostmem backend
      ivshmem: remove EventfdEntry.vector
      ivshmem: rename MSI eventfd_table
      ivshmem: use kvm irqfd for msi notifications
      ivshmem: use little-endian int64_t for the protocol

 Makefile                                |   8 +
 Makefile.objs                           |   5 +
 configure                               |   1 +
 contrib/ivshmem-client/Makefile.objs    |   1 +
 contrib/ivshmem-client/ivshmem-client.c | 446 ++++++++++++++++++++++++++++++++++
 contrib/ivshmem-client/ivshmem-client.h | 213 +++++++++++++++++
 contrib/ivshmem-client/main.c           | 239 +++++++++++++++++++
 contrib/ivshmem-server/Makefile.objs    |   1 +
 contrib/ivshmem-server/ivshmem-server.c | 480 +++++++++++++++++++++++++++++++++++++
 contrib/ivshmem-server/ivshmem-server.h | 166 +++++++++++++
 contrib/ivshmem-server/main.c           | 263 ++++++++++++++++++++
 docs/specs/ivshmem_device_spec.txt      | 127 +++++++---
 hw/misc/ivshmem.c                       | 833 ++++++++++++++++++++++++++++++++++++++++++++--------------------
 hw/pci/msix.c                           |   6 +
 include/glib-compat.h                   |  61 +++++
 include/hw/misc/ivshmem.h               |  25 ++
 include/hw/pci/msix.h                   |  16 +-
 include/sysemu/char.h                   |  10 +-
 qemu-char.c                             |   9 +-
 qemu-doc.texi                           |  10 +-
 tests/Makefile                          |   3 +
 tests/ivshmem-test.c                    | 496 ++++++++++++++++++++++++++++++++++++++
 tests/libqtest.c                        |  37 ++-
 tests/libqtest.h                        |   2 +
 24 files changed, 3142 insertions(+), 316 deletions(-)
 create mode 100644 contrib/ivshmem-client/Makefile.objs
 create mode 100644 contrib/ivshmem-client/ivshmem-client.c
 create mode 100644 contrib/ivshmem-client/ivshmem-client.h
 create mode 100644 contrib/ivshmem-client/main.c
 create mode 100644 contrib/ivshmem-server/Makefile.objs
 create mode 100644 contrib/ivshmem-server/ivshmem-server.c
 create mode 100644 contrib/ivshmem-server/ivshmem-server.h
 create mode 100644 contrib/ivshmem-server/main.c
 create mode 100644 include/hw/misc/ivshmem.h
 create mode 100644 tests/ivshmem-test.c

Comments

Andreas Färber Oct. 7, 2015, 12:11 p.m. UTC | #1
Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The following changes since commit 5fdb4671b08e0d1631447e81348b2b50a6b85bf7:
> 
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request' into staging (2015-10-06 13:42:33 +0100)
> 
> are available in the git repository at:
> 
>   https://github.com/elmarco/qemu tags/ivshmem-series
> 
> for you to fetch changes up to 097cadb155ef22be286af1403240b4fbf0f038ef:
> 
>   ivshmem: use little-endian int64_t for the protocol (2015-10-06 21:17:22 +0200)
> 
> ----------------------------------------------------------------
> Ivshmem series
> 
> ----------------------------------------------------------------
[...]
> Marc-André Lureau (45):
[...]
>       tests: add ivshmem qtest

I had NAK'ed this patch in v1 and it has not been fixed. If this pull
gets merged I will immediately revert it. Not funny.

Andreas
Marc-Andre Lureau Oct. 7, 2015, 12:16 p.m. UTC | #2
Hi Andreas

----- Original Message -----
> Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > The following changes since commit
> > 5fdb4671b08e0d1631447e81348b2b50a6b85bf7:
> > 
> >   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request'
> >   into staging (2015-10-06 13:42:33 +0100)
> > 
> > are available in the git repository at:
> > 
> >   https://github.com/elmarco/qemu tags/ivshmem-series
> > 
> > for you to fetch changes up to 097cadb155ef22be286af1403240b4fbf0f038ef:
> > 
> >   ivshmem: use little-endian int64_t for the protocol (2015-10-06 21:17:22
> >   +0200)
> > 
> > ----------------------------------------------------------------
> > Ivshmem series
> > 
> > ----------------------------------------------------------------
> [...]
> > Marc-André Lureau (45):
> [...]
> >       tests: add ivshmem qtest
> 
> I had NAK'ed this patch in v1 and it has not been fixed. If this pull
> gets merged I will immediately revert it. Not funny.
> 


Could stick to technical review, please. The test runs fine without kvm. Regarding your copyright claim, I already explain that your older version of boilerplate test is really nothing compare to this one. But if you feel so strongly about it, I don't care you add a copyright line.
Andreas Färber Oct. 7, 2015, 12:31 p.m. UTC | #3
Am 07.10.2015 um 14:16 schrieb Marc-André Lureau:
> ----- Original Message -----
>> Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
>>> Marc-André Lureau (45):
>> [...]
>>>       tests: add ivshmem qtest
>>
>> I had NAK'ed this patch in v1 and it has not been fixed. If this pull
>> gets merged I will immediately revert it. Not funny.
>>
> 
> 
> Could stick to technical review, please. The test runs fine without kvm. Regarding your copyright claim, I already explain that your older version of boilerplate test is really nothing compare to this one. But if you feel so strongly about it, I don't care you add a copyright line.

It is non-technical and called plagiarism.

This is not about adding a copyright line to the file, it's about having
a Signed-off-by on your patch. I had the same discussion with Paolo
before, when he supposedly saw-but-not-read my patch. The common
denominator is that every time this happens to me it's *@redhat.com.

You were arguing that because your patch does more than mine you don't
need to carry my copyright and Sob - that's an invalid argument given
that even trivial refactoring changes by copyright holder IBM have been
blocking our relicensing efforts. We chose not to define a threshold.

Andreas
Andrew Jones Oct. 7, 2015, 12:42 p.m. UTC | #4
On Wed, Oct 07, 2015 at 08:16:40AM -0400, Marc-André Lureau wrote:
> 
> Hi Andreas
> 
> ----- Original Message -----
> > Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > The following changes since commit
> > > 5fdb4671b08e0d1631447e81348b2b50a6b85bf7:
> > > 
> > >   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-pull-request'
> > >   into staging (2015-10-06 13:42:33 +0100)
> > > 
> > > are available in the git repository at:
> > > 
> > >   https://github.com/elmarco/qemu tags/ivshmem-series
> > > 
> > > for you to fetch changes up to 097cadb155ef22be286af1403240b4fbf0f038ef:
> > > 
> > >   ivshmem: use little-endian int64_t for the protocol (2015-10-06 21:17:22
> > >   +0200)
> > > 
> > > ----------------------------------------------------------------
> > > Ivshmem series
> > > 
> > > ----------------------------------------------------------------
> > [...]
> > > Marc-André Lureau (45):
> > [...]
> > >       tests: add ivshmem qtest
> > 
> > I had NAK'ed this patch in v1 and it has not been fixed. If this pull
> > gets merged I will immediately revert it. Not funny.
> > 
> 
> 
> Could stick to technical review, please. The test runs fine without kvm. Regarding your copyright claim, I already explain that your older version of boilerplate test is really nothing compare to this one. But if you feel so strongly about it, I don't care you add a copyright line.
> 

I would care if we added it. If contributors are getting bullied into
outrageous demands, then there's something wrong. Something wrong is
something we should try to fix, not just shrug off. And, in this case,
Andreas' claim is quite outrageous. The patch[*] in question provided
absolutely nothing that couldn't have been copy+pasted from any other
qtest.

drew

[*] http://patchwork.ozlabs.org/patch/336367/
Marc-Andre Lureau Oct. 7, 2015, 12:44 p.m. UTC | #5
Hi

----- Original Message -----
> Am 07.10.2015 um 14:16 schrieb Marc-André Lureau:
> > ----- Original Message -----
> >> Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
> >>> Marc-André Lureau (45):
> >> [...]
> >>>       tests: add ivshmem qtest
> >>
> >> I had NAK'ed this patch in v1 and it has not been fixed. If this pull
> >> gets merged I will immediately revert it. Not funny.
> >>
> > 
> > 
> > Could stick to technical review, please. The test runs fine without kvm.
> > Regarding your copyright claim, I already explain that your older version
> > of boilerplate test is really nothing compare to this one. But if you feel
> > so strongly about it, I don't care you add a copyright line.
> 
> It is non-technical and called plagiarism.

All tests share common boilerplate. Your test is not even in my tests. You could add it back!

> This is not about adding a copyright line to the file, it's about having
> a Signed-off-by on your patch. I had the same discussion with Paolo
> before, when he supposedly saw-but-not-read my patch. The common
> denominator is that every time this happens to me it's *@redhat.com.

they are everywhere :)

> You were arguing that because your patch does more than mine you don't
> need to carry my copyright and Sob - that's an invalid argument given
> that even trivial refactoring changes by copyright holder IBM have been
> blocking our relicensing efforts. We chose not to define a threshold.
 
I propose to add your patch first, that way you get your test and your copyright. Would that work for you?
Andreas Färber Oct. 7, 2015, 1:05 p.m. UTC | #6
Drew,

Am 07.10.2015 um 14:42 schrieb Andrew Jones:
> On Wed, Oct 07, 2015 at 08:16:40AM -0400, Marc-André Lureau wrote:
>> ----- Original Message -----
>>> Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
>>>> Marc-André Lureau (45):
>>> [...]
>>>>       tests: add ivshmem qtest
>>>
>>> I had NAK'ed this patch in v1 and it has not been fixed. If this pull
>>> gets merged I will immediately revert it. Not funny.
>>>
>>
>>
>> Could stick to technical review, please. The test runs fine without kvm. Regarding your copyright claim, I already explain that your older version of boilerplate test is really nothing compare to this one. But if you feel so strongly about it, I don't care you add a copyright line.
>>
> 
> I would care if we added it. If contributors are getting bullied into
> outrageous demands, then there's something wrong. Something wrong is
> something we should try to fix, not just shrug off. And, in this case,
> Andreas' claim is quite outrageous. The patch[*] in question provided
> absolutely nothing that couldn't have been copy+pasted from any other
> qtest.

If something is outrageous, then the way Marc-André is bullying *me* by
1) spinning his own version of my tests/ivshmem-test.c (not just copying
boilerplate from somewhere else), 2) spinning his own version of my
"make test" patch (which Peter keeps refusing to apply for two releases
now) and 3) trying to sneak QOM changes in via trivial without CC'ing
me. Who knows what else I've missed. It's a recurring pattern.

I don't currently have as much time for upstream QEMU as I'd like, so
other people either ignoring the work that I did do or taking my work
and pretending that it is their own is truely offending to me. It was an
easy-to-address review comment that hardly qualifies as bullying - after
all he is also taking patches from 6wind.com properly.

I note that you are redhat.com, too.

Andreas
Marc-Andre Lureau Oct. 7, 2015, 1:26 p.m. UTC | #7
Hi

----- Original Message -----
> Drew,
> 
> Am 07.10.2015 um 14:42 schrieb Andrew Jones:
> > On Wed, Oct 07, 2015 at 08:16:40AM -0400, Marc-André Lureau wrote:
> >> ----- Original Message -----
> >>> Am 06.10.2015 um 21:18 schrieb marcandre.lureau@redhat.com:
> >>>> Marc-André Lureau (45):
> >>> [...]
> >>>>       tests: add ivshmem qtest
> >>>
> >>> I had NAK'ed this patch in v1 and it has not been fixed. If this pull
> >>> gets merged I will immediately revert it. Not funny.
> >>>
> >>
> >>
> >> Could stick to technical review, please. The test runs fine without kvm.
> >> Regarding your copyright claim, I already explain that your older version
> >> of boilerplate test is really nothing compare to this one. But if you
> >> feel so strongly about it, I don't care you add a copyright line.
> >>
> > 
> > I would care if we added it. If contributors are getting bullied into
> > outrageous demands, then there's something wrong. Something wrong is
> > something we should try to fix, not just shrug off. And, in this case,
> > Andreas' claim is quite outrageous. The patch[*] in question provided
> > absolutely nothing that couldn't have been copy+pasted from any other
> > qtest.
> 
> If something is outrageous, then the way Marc-André is bullying *me* by
> 1) spinning his own version of my tests/ivshmem-test.c (not just copying

It doesn't share anything but boilerplate qtest code. Btw, do you have a non-RFC version of this patch?

> boilerplate from somewhere else), 2) spinning his own version of my
> "make test" patch (which Peter keeps refusing to apply for two releases

which patch are you talking about precisely here?

> now) and 3) trying to sneak QOM changes in via trivial without CC'ing
> me. Who knows what else I've missed. It's a recurring pattern.

That's a mistake from me for removing a comment that I thought was trivial, and forgot to CC the maintainer of the file too.

Let's not mix unrelated things here.

> I don't currently have as much time for upstream QEMU as I'd like, so
> other people either ignoring the work that I did do or taking my work
> and pretending that it is their own is truely offending to me. It was an
> easy-to-address review comment that hardly qualifies as bullying - after
> all he is also taking patches from 6wind.com properly.

I don't think anyone is trying to offend you. However, since I am quite new to the qemu project, it's understandable I make mistakes. Please try to be a bit more friendly with newcomers.

> I note that you are redhat.com, too.

:)

cheers
Peter Maydell Oct. 7, 2015, 10 p.m. UTC | #8
On 7 October 2015 at 14:05, Andreas Färber <afaerber@suse.de> wrote:
> my "make test" patch (which Peter keeps refusing to apply for two
> releases now)

For what it's worth, I am not currently aware of a patch from
you that I am refusing to apply. There's a lot of traffic on
the list, and it's very easy for things to get lost in the flood,
or for me to forget about something we discussed in the past.
If there's something you'd like me to reconsider, please point me
at it. (I do sometimes make bad review decisions, too.)

(This next part is aimed at everybody in this discussion thread;
I'm just putting it here since I happened to be replying to
your email above.)

Due credit for work is an emotive issue, and it can be highly
frustrating when hard work you've put in fails to get through
our sometimes badly dysfunctional review process, or when
you did something you thought was reasonable and somebody
else objected. However could we please try to assume good
faith in this discussion? I don't believe anybody is setting
out to deliberately bully another contributor here, or to
deliberately plagiarise work, or anything like that.
People make mistakes, or forget, or don't know about our
(sometimes unwritten) conventions and process, because we're
all human.

thanks
-- PMM
Paolo Bonzini Oct. 7, 2015, 10:24 p.m. UTC | #9
On 07/10/2015 14:31, Andreas Färber wrote:
> It is non-technical and called plagiarism.

I don't think the text

   g_strdup_printf("-device ivshmem,shm=%s,size=1M",

(yes, even the final argument differs between your version and
Marc-André) counts as plagiarism.

> The common denominator is that
> every time this happens to me it's *@redhat.com.

Correlation does not imply causation.  There is obvious overlap between
what Red Hat and SuSE care about.  I have forgotten the specific episode
though.  I remember I was CCed on a patch but had not replied to it, or
something like that.

Paolo
Pavel Fedin Oct. 9, 2015, 11:55 a.m. UTC | #10
Hello!

 I have merged this PULL into our local repository and my colleague gave it a try.
 It seems to work, but looks like ivshmem + memdev + hugetlbfs doesn't do the right thing. Memdev does attach itself to a hugetlb, but we cannot actually share data with it.
 Our command line was:
--- cut ---
/usr/local/bin/qemu-system-aarch64 \
        -name ivshmem1_huge \
        -machine virt,accel=kvm,usb=off,gic-version=3 \
        -cpu host -m 2048 \
        -realtime mlock=off \
        -smp 8,sockets=8,cores=1,threads=1 \
        -nographic \
        -kernel /var/lib/libvirt/images/Image_uio \
        -append console='ttyAMA0,115200n8 root=/dev/sda4 rootwait hugepages=128 earlycon=pl011,0x9000000' \
        -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \
        -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \
        -device virtio-scsi-pci,id=scsi0,bus=pci.2,addr=0x1 \
        -drive file=/var/lib/libvirt/images/sda3_pool/ivshmem1_huge.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2 \
        -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1 \
        -object memory-backend-file,size=4G,mem-path=/dev/huge,id=mb1,share=on \
        -device ivshmem,memdev=mb1 \
        -msg timestamp=on
--- cut ---

 The problem is that memdev creates a file as temporary, and then immediately unlinks it. Therefore, we cannot feed the same file to another VM. These are open files, used by two running VMs, which are supposed to exchange data via ivshmem.
--- cut ---
[root@thunderx-2 igor]# ls -l /proc/34845/fd
total 0
lrwx------ 1 root root 64 Oct  9 06:59 0 -> /dev/pts/2
lrwx------ 1 root root 64 Oct  9 06:59 1 -> /dev/pts/2
...
lrwx------ 1 root root 64 Oct  9 06:59 7 -> anon_inode:[eventfd]
lrwx------ 1 root root 64 Oct  9 06:59 8 -> /dev/huge/qemu_back_mem._objects_mb1.lwEeDc (deleted)
lrwx------ 1 root root 64 Oct  9 06:59 9 -> /dev/kvm
[root@thunderx-2 igor]# ls -l /proc/34866/fd
total 0
lrwx------ 1 root root 64 Oct  9 06:59 0 -> /dev/pts/0
lrwx------ 1 root root 64 Oct  9 06:59 1 -> /dev/pts/0
...
lrwx------ 1 root root 64 Oct  9 06:59 7 -> anon_inode:[eventfd]
lrwx------ 1 root root 64 Oct  9 06:59 8 -> /dev/huge/qemu_back_mem._objects_mb1.0Myp8M (deleted)
lrwx------ 1 root root 64 Oct  9 06:59 9 -> /dev/kvm
--- cut ---
 Due to the same reason, we cannot also exchange data between host and VM.
 Is it a flaw or do we just do something wrong?

Tested-by: Igor Skalkin <i.skalkin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Marc-Andre Lureau Oct. 9, 2015, 12:12 p.m. UTC | #11
Hi

----- Original Message -----
> Hello!
> 
>  I have merged this PULL into our local repository and my colleague gave it a
>  try.
>  It seems to work, but looks like ivshmem + memdev + hugetlbfs doesn't do the
>  right thing. Memdev does attach itself to a hugetlb, but we cannot actually
>  share data with it.
>  Our command line was:
> --- cut ---
> /usr/local/bin/qemu-system-aarch64 \
>         -name ivshmem1_huge \
>         -machine virt,accel=kvm,usb=off,gic-version=3 \
>         -cpu host -m 2048 \
>         -realtime mlock=off \
>         -smp 8,sockets=8,cores=1,threads=1 \
>         -nographic \
>         -kernel /var/lib/libvirt/images/Image_uio \
>         -append console='ttyAMA0,115200n8 root=/dev/sda4 rootwait
>         hugepages=128 earlycon=pl011,0x9000000' \
>         -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1 \
>         -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \
>         -device virtio-scsi-pci,id=scsi0,bus=pci.2,addr=0x1 \
>         -drive
>         file=/var/lib/libvirt/images/sda3_pool/ivshmem1_huge.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2
>         \
>         -device
>         scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1
>         \
>         -object
>         memory-backend-file,size=4G,mem-path=/dev/huge,id=mb1,share=on \
>         -device ivshmem,memdev=mb1 \
>         -msg timestamp=on
> --- cut ---
> 
>  The problem is that memdev creates a file as temporary, and then immediately
>  unlinks it. Therefore, we cannot feed the same file to another VM. These
>  are open files, used by two running VMs, which are supposed to exchange
>  data via ivshmem.
> --- cut ---
> [root@thunderx-2 igor]# ls -l /proc/34845/fd
> total 0
> lrwx------ 1 root root 64 Oct  9 06:59 0 -> /dev/pts/2
> lrwx------ 1 root root 64 Oct  9 06:59 1 -> /dev/pts/2
> ...
> lrwx------ 1 root root 64 Oct  9 06:59 7 -> anon_inode:[eventfd]
> lrwx------ 1 root root 64 Oct  9 06:59 8 ->
> /dev/huge/qemu_back_mem._objects_mb1.lwEeDc (deleted)
> lrwx------ 1 root root 64 Oct  9 06:59 9 -> /dev/kvm
> [root@thunderx-2 igor]# ls -l /proc/34866/fd
> total 0
> lrwx------ 1 root root 64 Oct  9 06:59 0 -> /dev/pts/0
> lrwx------ 1 root root 64 Oct  9 06:59 1 -> /dev/pts/0
> ...
> lrwx------ 1 root root 64 Oct  9 06:59 7 -> anon_inode:[eventfd]
> lrwx------ 1 root root 64 Oct  9 06:59 8 ->
> /dev/huge/qemu_back_mem._objects_mb1.0Myp8M (deleted)
> lrwx------ 1 root root 64 Oct  9 06:59 9 -> /dev/kvm
> --- cut ---
>  Due to the same reason, we cannot also exchange data between host and VM.
>  Is it a flaw or do we just do something wrong?

file_ram_alloc() only allocates with temporary files, even when using share=true.

We could teach it to use an existing file instead. This is an additional feature that could be considered after this series, since it's not a regression. Also, the prefered way to share memory (including hugetlb) is to use the ivshmem-server (that way you have a clear owner, and you can signal vm2vm).

> Tested-by: Igor Skalkin <i.skalkin@samsung.com>

Thanks a lot for testing.

> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 
>
Pavel Fedin Oct. 9, 2015, 12:29 p.m. UTC | #12
Hello!

> file_ram_alloc() only allocates with temporary files, even when using share=true.

 Yes. I wonder, what's the purpose of share=true then.

> We could teach it to use an existing file instead. This is an additional feature that could be
> considered after this series, since it's not a regression.

 Yes, i agree, it's not a regression. We'll take a look at it, unfortunately i cannot propose the fix right now because i'm busy with vGICv3 live migration task.

 Can i help upstreaming with something else? I wish you all to quickly resolve your single-line-authorship conflict and get this awesome work in master. If i have the authority, then...

 Acked-by: Pavel Fedin <p.fedin@samsung.com>

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Marc-Andre Lureau Oct. 9, 2015, 12:45 p.m. UTC | #13
Hi

----- Original Message -----
> Hello!
> 
> > file_ram_alloc() only allocates with temporary files, even when using
> > share=true.
> 
>  Yes. I wonder, what's the purpose of share=true then.

Paolo added it, I am not sure either.
 
> > We could teach it to use an existing file instead. This is an additional
> > feature that could be
> > considered after this series, since it's not a regression.
> 
>  Yes, i agree, it's not a regression. We'll take a look at it, unfortunately
>  i cannot propose the fix right now because i'm busy with vGICv3 live
>  migration task.

Thanks

> 
>  Can i help upstreaming with something else? I wish you all to quickly
>  resolve your single-line-authorship conflict and get this awesome work in
>  master. If i have the authority, then...

Yeah, I wish to solve this quickly. Unfortunately, Andreas didn't reply to my proposal. Tbh, I am really tempted to just change the copyright lines, resend, and call it a day (I could even give him authorship after all ;)

>  Acked-by: Pavel Fedin <p.fedin@samsung.com>

Do you want me to ack every commit? I guess it's enought as a overall ack on the ML.

cheers
Paolo Bonzini Oct. 9, 2015, 12:47 p.m. UTC | #14
On 09/10/2015 14:45, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> Hello!
>>
>>> file_ram_alloc() only allocates with temporary files, even when using
>>> share=true.
>>
>>  Yes. I wonder, what's the purpose of share=true then.
> 
> Paolo added it, I am not sure either.

It is needed for use with vhost-user.  If the file descriptor you pass
to the vhost-user server is mapped with MAP_PRIVATE, the vhost-user
server will not get the guest's memory contents.  At least I think so. :)

Paolo
Pavel Fedin Oct. 9, 2015, 12:51 p.m. UTC | #15
Hello!

> Yeah, I wish to solve this quickly. Unfortunately, Andreas didn't reply to my proposal. Tbh, I
> am really tempted to just change the copyright lines, resend, and call it a day (I could even
> give him authorship after all ;)

 I didn't read the whole thing thoroughly, but IIRC he did the same change some time ago, which got drowned in reviewers' mailbox. Just pick it up with his authorship and make a peace finally :)

> >  Acked-by: Pavel Fedin <p.fedin@samsung.com>
> 
> Do you want me to ack every commit? I guess it's enought as a overall ack on the ML.

 No, not necessary, i hate paperwork and don't want to force a respin just for this purpose. You can do it if you respin for some more worthy reason.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Pavel Fedin Oct. 9, 2015, 1:02 p.m. UTC | #16
Hello!

> It is needed for use with vhost-user.  If the file descriptor you pass
> to the vhost-user server is mapped with MAP_PRIVATE, the vhost-user
> server will not get the guest's memory contents.  At least I think so. :)

 Aha, so it actually tested only when file descriptor is passed from outside, and not file name, isn't it?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Paolo Bonzini Oct. 9, 2015, 1:07 p.m. UTC | #17
On 09/10/2015 15:02, Pavel Fedin wrote:
>> It is needed for use with vhost-user.  If the file descriptor you
>> pass to the vhost-user server is mapped with MAP_PRIVATE, the
>> vhost-user server will not get the guest's memory contents.  At
>> least I think so. :)
>
> Aha, so it actually tested only when file descriptor is passed from
> outside, and not file name, isn't it?

When it's passed _to_ outside (via SCM_RIGHTS).  But yes, the file name
doesn't matter in this case.

Thanks,

Paolo