Re: [Qemu-devel] vhost-user: broken mem regions

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 15:53, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 01:45:09PM +0000, Damjan Marion (damarion) wrote:
>> 
>> Michael,
>> 
>> there is another issue with commited vhost-user code.
> 
> I'm answering just this once, but I have a policy against
> answering off-list mail.
> Pls send follow-ups to the list.

ok, sorry, adding list...

> 
>> If there is bigger mem allocation (i.e. 4G or 6G of RAM) then
>> we have memory gap and then it happens that buffer pointer points to 
>> memory which is not mmaped. I already filled bug report:
>> 
>> https://bugs.launchpad.net/qemu/+bug/1333688
> 
> FYI I mostly ignore launchpad.
> Because of the unfortunate association with Ubuntu, most bugs
> there are distro-specific.
> 
>> Bellow is my proposed change to the code. Two things: 
>> - it will require changes on the user side also
> 
> why would it?
> format seems unchanged, right?

yes, but it will happen that multiple regions have same FD so call to mmap()
should look different, I’m still playing with this on user side...

> 
>> - not sure if qemu_get_ram_fd() is the best way to get FD
> 
> Paolo, what do you think?
> 
>> Any comments or better idea how to fix this?
>> 
>> Thanks,
>> 
>> Damjan
>> 
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 0df6a93..89fe5c7 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -14,6 +14,7 @@
>> #include "sysemu/kvm.h"
>> #include "qemu/error-report.h"
>> #include "qemu/sockets.h"
>> +#include "exec/ram_addr.h"
>> 
>> #include <fcntl.h>
>> #include <unistd.h>
>> @@ -183,10 +184,10 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>> {
>>     VhostUserMsg msg;
>>     VhostUserRequest msg_request;
>> -    RAMBlock *block = 0;
>>     struct vhost_vring_file *file = 0;
>>     int need_reply = 0;
>>     int fds[VHOST_MEMORY_MAX_NREGIONS];
>> +    int i, fd;
>>     size_t fd_num = 0;
>> 
>>     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>> @@ -212,14 +213,14 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>>         break;
>> 
>>     case VHOST_SET_MEM_TABLE:
>> -        QTAILQ_FOREACH(block, &ram_list.blocks, next)
>> -        {
>> -            if (block->fd > 0) {
>> -                msg.memory.regions[fd_num].userspace_addr =
>> -                    (uintptr_t) block->host;
>> -                msg.memory.regions[fd_num].memory_size = block->length;
>> -                msg.memory.regions[fd_num].guest_phys_addr = block->offset;
>> -                fds[fd_num++] = block->fd;
>> +        for (i = 0; i < dev->mem->nregions; ++i) {
>> +            struct vhost_memory_region *reg = dev->mem->regions + i;
>> +            fd = qemu_get_ram_fd(reg->guest_phys_addr);
>> +            if (fd > 0) {
>> +                msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
>> +                msg.memory.regions[fd_num].memory_size  = reg->memory_size;
>> +                msg.memory.regions[fd_num].guest_phys_addr = reg->memory_size;
>> +                fds[fd_num++] = fd;
>>             }
>>         }
>> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Nikolay Nikolaev]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

On Wed, Jun 25, 2014 at 5:06 PM, Damjan Marion (damarion) <
damarion@cisco.com> wrote:

>
> On 25 Jun 2014, at 15:53, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> > On Wed, Jun 25, 2014 at 01:45:09PM +0000, Damjan Marion (damarion) wrote:
> >>
> >> Michael,
> >>
> >> there is another issue with commited vhost-user code.
> >
> > I'm answering just this once, but I have a policy against
> > answering off-list mail.
> > Pls send follow-ups to the list.
>
> ok, sorry, adding list...
>
> >
> >> If there is bigger mem allocation (i.e. 4G or 6G of RAM) then
> >> we have memory gap and then it happens that buffer pointer points to
> >> memory which is not mmaped. I already filled bug report:
> >>
> >> https://bugs.launchpad.net/qemu/+bug/1333688
> >
> > FYI I mostly ignore launchpad.
> > Because of the unfortunate association with Ubuntu, most bugs
> > there are distro-specific.
> >
> >> Bellow is my proposed change to the code. Two things:
> >> - it will require changes on the user side also
> >
> > why would it?
> > format seems unchanged, right?
>
> yes, but it will happen that multiple regions have same FD so call to
> mmap()
> should look different, I’m still playing with this on user side...
>
but then you shoudl somehow accumulate the sizes and send just a single fd,
something along these lines.

>
> >
> >> - not sure if qemu_get_ram_fd() is the best way to get FD
> >
> > Paolo, what do you think?
> >
> >> Any comments or better idea how to fix this?
> >>
> >> Thanks,
> >>
> >> Damjan
> >>
> >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> >> index 0df6a93..89fe5c7 100644
> >> --- a/hw/virtio/vhost-user.c
> >> +++ b/hw/virtio/vhost-user.c
> >> @@ -14,6 +14,7 @@
> >> #include "sysemu/kvm.h"
> >> #include "qemu/error-report.h"
> >> #include "qemu/sockets.h"
> >> +#include "exec/ram_addr.h"
> >>
> >> #include <fcntl.h>
> >> #include <unistd.h>
> >> @@ -183,10 +184,10 @@ static int vhost_user_call(struct vhost_dev *dev,
> unsigned long int request,
> >> {
> >>     VhostUserMsg msg;
> >>     VhostUserRequest msg_request;
> >> -    RAMBlock *block = 0;
> >>     struct vhost_vring_file *file = 0;
> >>     int need_reply = 0;
> >>     int fds[VHOST_MEMORY_MAX_NREGIONS];
> >> +    int i, fd;
> >>     size_t fd_num = 0;
> >>
> >>     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> >> @@ -212,14 +213,14 @@ static int vhost_user_call(struct vhost_dev *dev,
> unsigned long int request,
> >>         break;
> >>
> >>     case VHOST_SET_MEM_TABLE:
> >> -        QTAILQ_FOREACH(block, &ram_list.blocks, next)
> >> -        {
> >> -            if (block->fd > 0) {
> >> -                msg.memory.regions[fd_num].userspace_addr =
> >> -                    (uintptr_t) block->host;
> >> -                msg.memory.regions[fd_num].memory_size = block->length;
> >> -                msg.memory.regions[fd_num].guest_phys_addr =
> block->offset;
> >> -                fds[fd_num++] = block->fd;
> >> +        for (i = 0; i < dev->mem->nregions; ++i) {
> >> +            struct vhost_memory_region *reg = dev->mem->regions + i;
> >> +            fd = qemu_get_ram_fd(reg->guest_phys_addr);
> >> +            if (fd > 0) {
> >> +                msg.memory.regions[fd_num].userspace_addr =
> reg->userspace_addr;
> >> +                msg.memory.regions[fd_num].memory_size  =
> reg->memory_size;
> >> +                msg.memory.regions[fd_num].guest_phys_addr =
> reg->memory_size;
> >> +                fds[fd_num++] = fd;
> >>             }
> >>         }
> >>
>
>

[-- Attachment #2: Type: text/html, Size: 5147 bytes --]

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 16:06, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:

> +                msg.memory.regions[fd_num].memory_size  = reg->memory_size; 
> so this is size 
> +                msg.memory.regions[fd_num].guest_phys_addr = reg->memory_size;
> and this is size again? 

mistake, i alredy noticed it. It should be guest_phys_addr

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 05:13:21PM +0300, Nikolay Nikolaev wrote:
> 
> 
> 
> On Wed, Jun 25, 2014 at 5:06 PM, Damjan Marion (damarion) <damarion@cisco.com>
> wrote:
> 
> 
>     On 25 Jun 2014, at 15:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     > On Wed, Jun 25, 2014 at 01:45:09PM +0000, Damjan Marion (damarion) wrote:
>     >>
>     >> Michael,
>     >>
>     >> there is another issue with commited vhost-user code.
>     >
>     > I'm answering just this once, but I have a policy against
>     > answering off-list mail.
>     > Pls send follow-ups to the list.
> 
>     ok, sorry, adding list...
> 
>     >
>     >> If there is bigger mem allocation (i.e. 4G or 6G of RAM) then
>     >> we have memory gap and then it happens that buffer pointer points to
>     >> memory which is not mmaped. I already filled bug report:
>     >>
>     >> https://bugs.launchpad.net/qemu/+bug/1333688
>     >
>     > FYI I mostly ignore launchpad.
>     > Because of the unfortunate association with Ubuntu, most bugs
>     > there are distro-specific.
>     >
>     >> Bellow is my proposed change to the code. Two things:
>     >> - it will require changes on the user side also
>     >
>     > why would it?
>     > format seems unchanged, right?
> 
>     yes, but it will happen that multiple regions have same FD so call to mmap
>     ()
>     should look different, I’m still playing with this on user side...
> 
> but then you shoudl somehow accumulate the sizes and send just a single fd,
> something along these lines.


Not going to work, these regions might not be contigious
(e.g. if a higher priority region hides RAM).
Just deal with the fact that same fd can appear
multiple times. I don't see why it's a big deal anyway.


> 
>     >
>     >> - not sure if qemu_get_ram_fd() is the best way to get FD
>     >
>     > Paolo, what do you think?
>     >
>     >> Any comments or better idea how to fix this?
>     >>
>     >> Thanks,
>     >>
>     >> Damjan
>     >>
>     >> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>     >> index 0df6a93..89fe5c7 100644
>     >> --- a/hw/virtio/vhost-user.c
>     >> +++ b/hw/virtio/vhost-user.c
>     >> @@ -14,6 +14,7 @@
>     >> #include "sysemu/kvm.h"
>     >> #include "qemu/error-report.h"
>     >> #include "qemu/sockets.h"
>     >> +#include "exec/ram_addr.h"
>     >>
>     >> #include <fcntl.h>
>     >> #include <unistd.h>
>     >> @@ -183,10 +184,10 @@ static int vhost_user_call(struct vhost_dev *dev,
>     unsigned long int request,
>     >> {
>     >>     VhostUserMsg msg;
>     >>     VhostUserRequest msg_request;
>     >> -    RAMBlock *block = 0;
>     >>     struct vhost_vring_file *file = 0;
>     >>     int need_reply = 0;
>     >>     int fds[VHOST_MEMORY_MAX_NREGIONS];
>     >> +    int i, fd;
>     >>     size_t fd_num = 0;
>     >>
>     >>     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
>     >> @@ -212,14 +213,14 @@ static int vhost_user_call(struct vhost_dev *dev,
>     unsigned long int request,
>     >>         break;
>     >>
>     >>     case VHOST_SET_MEM_TABLE:
>     >> -        QTAILQ_FOREACH(block, &ram_list.blocks, next)
>     >> -        {
>     >> -            if (block->fd > 0) {
>     >> -                msg.memory.regions[fd_num].userspace_addr =
>     >> -                    (uintptr_t) block->host;
>     >> -                msg.memory.regions[fd_num].memory_size = block->length;
>     >> -                msg.memory.regions[fd_num].guest_phys_addr = block->
>     offset;
>     >> -                fds[fd_num++] = block->fd;
>     >> +        for (i = 0; i < dev->mem->nregions; ++i) {
>     >> +            struct vhost_memory_region *reg = dev->mem->regions + i;
>     >> +            fd = qemu_get_ram_fd(reg->guest_phys_addr);
>     >> +            if (fd > 0) {
>     >> +                msg.memory.regions[fd_num].userspace_addr = reg->
>     userspace_addr;
>     >> +                msg.memory.regions[fd_num].memory_size  = reg->
>     memory_size;
>     >> +                msg.memory.regions[fd_num].guest_phys_addr = reg->
>     memory_size;
>     >> +                fds[fd_num++] = fd;
>     >>             }
>     >>         }
>     >>
> 
> 
> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:

> >> - it will require changes on the user side also
> >
> > why would it?
> > format seems unchanged, right?
> 
> yes, but it will happen that multiple regions have same FD so call to mmap()
> should look different, I’m still playing with this on user side...
> but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 

Yes, so I’m not very happy with that approach and looking if there is better proposal, 
or at least wider agreement how to address this issue.

Damjan

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 02:20:56PM +0000, Damjan Marion (damarion) wrote:
> 
> On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
> 
> > >> - it will require changes on the user side also
> > >
> > > why would it?
> > > format seems unchanged, right?
> > 
> > yes, but it will happen that multiple regions have same FD so call to mmap()
> > should look different, I’m still playing with this on user side...
> > but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 
> 
> Yes, so I’m not very happy with that approach and looking if there is better proposal, 
> or at least wider agreement how to address this issue.
> 
> Damjan

still not sure what the issue is ...

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 16:27, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 02:20:56PM +0000, Damjan Marion (damarion) wrote:
>> 
>> On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
>> 
>>>>> - it will require changes on the user side also
>>>> 
>>>> why would it?
>>>> format seems unchanged, right?
>>> 
>>> yes, but it will happen that multiple regions have same FD so call to mmap()
>>> should look different, I’m still playing with this on user side...
>>> but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 
>> 
>> Yes, so I’m not very happy with that approach and looking if there is better proposal, 
>> or at least wider agreement how to address this issue.
>> 
>> Damjan
> 
> still not sure what the issue is ...
> 

No issue, just additional logic is needed on user side to calculate total size of shared regions and call mmap() once per FD.

Agree?

If this is the way to go I will submit patch when I get it tested..

Damjan

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 02:57:52PM +0000, Damjan Marion (damarion) wrote:
> 
> On 25 Jun 2014, at 16:27, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Wed, Jun 25, 2014 at 02:20:56PM +0000, Damjan Marion (damarion) wrote:
> >> 
> >> On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
> >> 
> >>>>> - it will require changes on the user side also
> >>>> 
> >>>> why would it?
> >>>> format seems unchanged, right?
> >>> 
> >>> yes, but it will happen that multiple regions have same FD so call to mmap()
> >>> should look different, I’m still playing with this on user side...
> >>> but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 
> >> 
> >> Yes, so I’m not very happy with that approach and looking if there is better proposal, 
> >> or at least wider agreement how to address this issue.
> >> 
> >> Damjan
> > 
> > still not sure what the issue is ...
> > 
> 
> No issue, just additional logic is needed on user side to calculate total size of shared regions and call mmap() once per FD.
> 
> Agree?

why not just call it multiple times? AFAIK linux handles this just fine.

> If this is the way to go I will submit patch when I get it tested..
> 
> Damjan
> 
> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 17:29, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 02:57:52PM +0000, Damjan Marion (damarion) wrote:
>> 
>> On 25 Jun 2014, at 16:27, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>>> On Wed, Jun 25, 2014 at 02:20:56PM +0000, Damjan Marion (damarion) wrote:
>>>> 
>>>> On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
>>>> 
>>>>>>> - it will require changes on the user side also
>>>>>> 
>>>>>> why would it?
>>>>>> format seems unchanged, right?
>>>>> 
>>>>> yes, but it will happen that multiple regions have same FD so call to mmap()
>>>>> should look different, I’m still playing with this on user side...
>>>>> but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 
>>>> 
>>>> Yes, so I’m not very happy with that approach and looking if there is better proposal, 
>>>> or at least wider agreement how to address this issue.
>>>> 
>>>> Damjan
>>> 
>>> still not sure what the issue is ...
>>> 
>> 
>> No issue, just additional logic is needed on user side to calculate total size of shared regions and call mmap() once per FD.
>> 
>> Agree?
> 
> why not just call it multiple times? AFAIK linux handles this just fine.

I need to specify size when calling mmap(), so i need to run trough all regions and sum sizes before calling mmap().

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 03:46:04PM +0000, Damjan Marion (damarion) wrote:
> 
> On 25 Jun 2014, at 17:29, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Wed, Jun 25, 2014 at 02:57:52PM +0000, Damjan Marion (damarion) wrote:
> >> 
> >> On 25 Jun 2014, at 16:27, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> 
> >>> On Wed, Jun 25, 2014 at 02:20:56PM +0000, Damjan Marion (damarion) wrote:
> >>>> 
> >>>> On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
> >>>> 
> >>>>>>> - it will require changes on the user side also
> >>>>>> 
> >>>>>> why would it?
> >>>>>> format seems unchanged, right?
> >>>>> 
> >>>>> yes, but it will happen that multiple regions have same FD so call to mmap()
> >>>>> should look different, I’m still playing with this on user side...
> >>>>> but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 
> >>>> 
> >>>> Yes, so I’m not very happy with that approach and looking if there is better proposal, 
> >>>> or at least wider agreement how to address this issue.
> >>>> 
> >>>> Damjan
> >>> 
> >>> still not sure what the issue is ...
> >>> 
> >> 
> >> No issue, just additional logic is needed on user side to calculate total size of shared regions and call mmap() once per FD.
> >> 
> >> Agree?
> > 
> > why not just call it multiple times? AFAIK linux handles this just fine.
> 
> I need to specify size when calling mmap(), so i need to run trough all regions and sum sizes before calling mmap().

You can map same file in many places.
Just call mmap many times with offsets.

-- 
MST

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 17:50, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 03:46:04PM +0000, Damjan Marion (damarion) wrote:
>> 
>> On 25 Jun 2014, at 17:29, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>>> On Wed, Jun 25, 2014 at 02:57:52PM +0000, Damjan Marion (damarion) wrote:
>>>> 
>>>> On 25 Jun 2014, at 16:27, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> 
>>>>> On Wed, Jun 25, 2014 at 02:20:56PM +0000, Damjan Marion (damarion) wrote:
>>>>>> 
>>>>>> On 25 Jun 2014, at 16:13, Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
>>>>>> 
>>>>>>>>> - it will require changes on the user side also
>>>>>>>> 
>>>>>>>> why would it?
>>>>>>>> format seems unchanged, right?
>>>>>>> 
>>>>>>> yes, but it will happen that multiple regions have same FD so call to mmap()
>>>>>>> should look different, I’m still playing with this on user side...
>>>>>>> but then you shoudl somehow accumulate the sizes and send just a single fd, something along these lines. 
>>>>>> 
>>>>>> Yes, so I’m not very happy with that approach and looking if there is better proposal, 
>>>>>> or at least wider agreement how to address this issue.
>>>>>> 
>>>>>> Damjan
>>>>> 
>>>>> still not sure what the issue is ...
>>>>> 
>>>> 
>>>> No issue, just additional logic is needed on user side to calculate total size of shared regions and call mmap() once per FD.
>>>> 
>>>> Agree?
>>> 
>>> why not just call it multiple times? AFAIK linux handles this just fine.
>> 
>> I need to specify size when calling mmap(), so i need to run trough all regions and sum sizes before calling mmap().
> 
> You can map same file in many places.
> Just call mmap many times with offsets.

agree, but in that case I need to know offset, which i need to calculate by finding 1st region in the list.

I.e. My regions are:

nregions: 4
region:
	gpa = 0x100000000
	size = 3221225472
	ua = 0x2aab6ac00000
region:
	gpa = 0xFFFC0000
	size = 262144
	ua = 0x7fc13d200000
region:
	gpa = 0x0
	size = 655360
	ua = 0x2aaaaac00000
region:
	gpa = 0xC0000
	size = 3220439040
	ua = 0x2aaaaacc0000


So,  to calculate offset for 1st region I need to find region with GPA 0x0 which is region 3.
Offset for region 1 is 0x2aab6ac00000 - 0x2aaaaac00000 = 0xC0000000.

BTW Any idea what to do with region 2, it doesn’t look like the one belonging to the same place, but qemu_get_ram_fd() returns same FD for it.

Re: [Qemu-devel] vhost-user: broken mem regions

[Paolo Bonzini]

> nregions: 4
> region:
> 	gpa = 0x100000000
> 	size = 3221225472
> 	ua = 0x2aab6ac00000

High memory, above 3 gigabytes.

> region:
> 	gpa = 0xFFFC0000
> 	size = 262144
> 	ua = 0x7fc13d200000

This is the BIOS.  There shouldn't be any FD for this one, it
is not allocated in hugetlbfs.

> region:
> 	gpa = 0x0
> 	size = 655360
> 	ua = 0x2aaaaac00000
> region:
> 	gpa = 0xC0000
> 	size = 3220439040
> 	ua = 0x2aaaaacc0000

Together, it's the first 3 GB of memory.

I understand now what you mean.  Yeah, the format should be changed
to include the offset (why does vhost-user need the ua at all?
perhaps the offset can replace the ua).

> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
> to the same place, but qemu_get_ram_fd() returns same FD for it.

This must be a bug.  I would have expected qemu_get_ram_fd to return -1
here, so no descriptor should be passed to vhost-user.

Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Nikolay Nikolaev]

On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> nregions: 4
>> region:
>>       gpa = 0x100000000
>>       size = 3221225472
>>       ua = 0x2aab6ac00000
>
> High memory, above 3 gigabytes.
>
>> region:
>>       gpa = 0xFFFC0000
>>       size = 262144
>>       ua = 0x7fc13d200000
>
> This is the BIOS.  There shouldn't be any FD for this one, it
> is not allocated in hugetlbfs.
>
>> region:
>>       gpa = 0x0
>>       size = 655360
>>       ua = 0x2aaaaac00000
>> region:
>>       gpa = 0xC0000
>>       size = 3220439040
>>       ua = 0x2aaaaacc0000
>
> Together, it's the first 3 GB of memory.
>
> I understand now what you mean.  Yeah, the format should be changed
> to include the offset (why does vhost-user need the ua at all?
The vring addresses are QEMU UA addresses. Of course vhost-user can
translate them to guest physical before sending the message.
> perhaps the offset can replace the ua).
>
>> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
>> to the same place, but qemu_get_ram_fd() returns same FD for it.
>
> This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> here, so no descriptor should be passed to vhost-user.
>
> Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Paolo Bonzini]

Il 25/06/2014 18:56, Nikolay Nikolaev ha scritto:
>> > I understand now what you mean.  Yeah, the format should be changed
>> > to include the offset (why does vhost-user need the ua at all?
> The vring addresses are QEMU UA addresses. Of course vhost-user can
> translate them to guest physical before sending the message.

Yeah, that looks like a good idea.

Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Nikolay Nikolaev]

On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> nregions: 4
>> region:
>>       gpa = 0x100000000
>>       size = 3221225472
>>       ua = 0x2aab6ac00000
>
> High memory, above 3 gigabytes.
>
>> region:
>>       gpa = 0xFFFC0000
>>       size = 262144
>>       ua = 0x7fc13d200000
>
> This is the BIOS.  There shouldn't be any FD for this one, it
> is not allocated in hugetlbfs.
>
>> region:
>>       gpa = 0x0
>>       size = 655360
>>       ua = 0x2aaaaac00000
>> region:
>>       gpa = 0xC0000
>>       size = 3220439040
>>       ua = 0x2aaaaacc0000
>
> Together, it's the first 3 GB of memory.
>
> I understand now what you mean.  Yeah, the format should be changed
> to include the offset (why does vhost-user need the ua at all?
> perhaps the offset can replace the ua).

I am trying to do the math here. So if this file is mapped by QEMU
into a single file then probably
(region[4].ua - region[3].ua) should give you the offset. And also
(region[4].ua - region[1].ua) shoudl give the above 3G offset. Or I am
missing something?



>
>> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
>> to the same place, but qemu_get_ram_fd() returns same FD for it.
>
> This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> here, so no descriptor should be passed to vhost-user.
>
> Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Paolo Bonzini]

Il 25/06/2014 19:06, Nikolay Nikolaev ha scritto:
>> > I understand now what you mean.  Yeah, the format should be changed
>> > to include the offset (why does vhost-user need the ua at all?
>> > perhaps the offset can replace the ua).
> I am trying to do the math here. So if this file is mapped by QEMU
> into a single file then probably
> (region[4].ua - region[3].ua) should give you the offset. And also
> (region[4].ua - region[1].ua) shoudl give the above 3G offset. Or I am
> missing something?

This is correct, but in principle it's not required to map all the 
areas.  The lowest region could correspond to a offset=65536, or 
something like that.  It's better to change the vring values to use 
offsets instead of absolute addresses.

Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 07:56:46PM +0300, Nikolay Nikolaev wrote:
> On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> nregions: 4
> >> region:
> >>       gpa = 0x100000000
> >>       size = 3221225472
> >>       ua = 0x2aab6ac00000
> >
> > High memory, above 3 gigabytes.
> >
> >> region:
> >>       gpa = 0xFFFC0000
> >>       size = 262144
> >>       ua = 0x7fc13d200000
> >
> > This is the BIOS.  There shouldn't be any FD for this one, it
> > is not allocated in hugetlbfs.
> >
> >> region:
> >>       gpa = 0x0
> >>       size = 655360
> >>       ua = 0x2aaaaac00000
> >> region:
> >>       gpa = 0xC0000
> >>       size = 3220439040
> >>       ua = 0x2aaaaacc0000
> >
> > Together, it's the first 3 GB of memory.
> >
> > I understand now what you mean.  Yeah, the format should be changed
> > to include the offset (why does vhost-user need the ua at all?
> The vring addresses are QEMU UA addresses. Of course vhost-user can
> translate them to guest physical before sending the message.

It seems useful to have them as QEMU UA, this will allow
frontends other than virtio where QEMU operates the
ring.

> > perhaps the offset can replace the ua).
> >
> >> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
> >> to the same place, but qemu_get_ram_fd() returns same FD for it.
> >
> > This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> > here, so no descriptor should be passed to vhost-user.
> >
> > Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 07:05:08PM +0200, Paolo Bonzini wrote:
> Il 25/06/2014 18:56, Nikolay Nikolaev ha scritto:
> >>> I understand now what you mean.  Yeah, the format should be changed
> >>> to include the offset (why does vhost-user need the ua at all?
> >The vring addresses are QEMU UA addresses. Of course vhost-user can
> >translate them to guest physical before sending the message.
> 
> Yeah, that looks like a good idea.
> 
> Paolo

Is this is must?
Seems too close to hard freeze to change the protocol ...
Does vhost-user actually work if you use a UA outside
guest memory?
If not, ok, but maybe add some capability so this can
be discovered during init.

-- 
MST

Re: [Qemu-devel] vhost-user: broken mem regions

[Nikolay Nikolaev]

On Wed, Jun 25, 2014 at 9:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 25, 2014 at 07:56:46PM +0300, Nikolay Nikolaev wrote:
>> On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> >> nregions: 4
>> >> region:
>> >>       gpa = 0x100000000
>> >>       size = 3221225472
>> >>       ua = 0x2aab6ac00000
>> >
>> > High memory, above 3 gigabytes.
>> >
>> >> region:
>> >>       gpa = 0xFFFC0000
>> >>       size = 262144
>> >>       ua = 0x7fc13d200000
>> >
>> > This is the BIOS.  There shouldn't be any FD for this one, it
>> > is not allocated in hugetlbfs.
>> >
>> >> region:
>> >>       gpa = 0x0
>> >>       size = 655360
>> >>       ua = 0x2aaaaac00000
>> >> region:
>> >>       gpa = 0xC0000
>> >>       size = 3220439040
>> >>       ua = 0x2aaaaacc0000
>> >
>> > Together, it's the first 3 GB of memory.
>> >
>> > I understand now what you mean.  Yeah, the format should be changed
>> > to include the offset (why does vhost-user need the ua at all?
>> The vring addresses are QEMU UA addresses. Of course vhost-user can
>> translate them to guest physical before sending the message.
>
> It seems useful to have them as QEMU UA, this will allow
> frontends other than virtio where QEMU operates the
> ring.

OK - so there will be a new offset field. That's fine with me. What
would be the deadline for such change?
It's not exactly bugfix. On the other hand there's no wide adoption of
the protocol so it's still not critical to change it.

>
>> > perhaps the offset can replace the ua).
>> >
>> >> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
>> >> to the same place, but qemu_get_ram_fd() returns same FD for it.
>> >
>> > This must be a bug.  I would have expected qemu_get_ram_fd to return -1
>> > here, so no descriptor should be passed to vhost-user.
>> >
>> > Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 09:13:36PM +0300, Nikolay Nikolaev wrote:
> On Wed, Jun 25, 2014 at 9:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Jun 25, 2014 at 07:56:46PM +0300, Nikolay Nikolaev wrote:
> >> On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> >> nregions: 4
> >> >> region:
> >> >>       gpa = 0x100000000
> >> >>       size = 3221225472
> >> >>       ua = 0x2aab6ac00000
> >> >
> >> > High memory, above 3 gigabytes.
> >> >
> >> >> region:
> >> >>       gpa = 0xFFFC0000
> >> >>       size = 262144
> >> >>       ua = 0x7fc13d200000
> >> >
> >> > This is the BIOS.  There shouldn't be any FD for this one, it
> >> > is not allocated in hugetlbfs.
> >> >
> >> >> region:
> >> >>       gpa = 0x0
> >> >>       size = 655360
> >> >>       ua = 0x2aaaaac00000
> >> >> region:
> >> >>       gpa = 0xC0000
> >> >>       size = 3220439040
> >> >>       ua = 0x2aaaaacc0000
> >> >
> >> > Together, it's the first 3 GB of memory.
> >> >
> >> > I understand now what you mean.  Yeah, the format should be changed
> >> > to include the offset (why does vhost-user need the ua at all?
> >> The vring addresses are QEMU UA addresses. Of course vhost-user can
> >> translate them to guest physical before sending the message.
> >
> > It seems useful to have them as QEMU UA, this will allow
> > frontends other than virtio where QEMU operates the
> > ring.
> 
> OK - so there will be a new offset field. That's fine with me. What
> would be the deadline for such change?
> It's not exactly bugfix. On the other hand there's no wide adoption of
> the protocol so it's still not critical to change it.

Right. So you have to do this before the hard freeze.
And don't cut it too fine either :)

> >
> >> > perhaps the offset can replace the ua).
> >> >
> >> >> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
> >> >> to the same place, but qemu_get_ram_fd() returns same FD for it.
> >> >
> >> > This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> >> > here, so no descriptor should be passed to vhost-user.
> >> >
> >> > Paolo

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 20:18, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 09:13:36PM +0300, Nikolay Nikolaev wrote:
>> On Wed, Jun 25, 2014 at 9:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Wed, Jun 25, 2014 at 07:56:46PM +0300, Nikolay Nikolaev wrote:
>>>> On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>> nregions: 4
>>>>>> region:
>>>>>>      gpa = 0x100000000
>>>>>>      size = 3221225472
>>>>>>      ua = 0x2aab6ac00000
>>>>> 
>>>>> High memory, above 3 gigabytes.
>>>>> 
>>>>>> region:
>>>>>>      gpa = 0xFFFC0000
>>>>>>      size = 262144
>>>>>>      ua = 0x7fc13d200000
>>>>> 
>>>>> This is the BIOS.  There shouldn't be any FD for this one, it
>>>>> is not allocated in hugetlbfs.
>>>>> 
>>>>>> region:
>>>>>>      gpa = 0x0
>>>>>>      size = 655360
>>>>>>      ua = 0x2aaaaac00000
>>>>>> region:
>>>>>>      gpa = 0xC0000
>>>>>>      size = 3220439040
>>>>>>      ua = 0x2aaaaacc0000
>>>>> 
>>>>> Together, it's the first 3 GB of memory.
>>>>> 
>>>>> I understand now what you mean.  Yeah, the format should be changed
>>>>> to include the offset (why does vhost-user need the ua at all?
>>>> The vring addresses are QEMU UA addresses. Of course vhost-user can
>>>> translate them to guest physical before sending the message.
>>> 
>>> It seems useful to have them as QEMU UA, this will allow
>>> frontends other than virtio where QEMU operates the
>>> ring.
>> 
>> OK - so there will be a new offset field. That's fine with me. What
>> would be the deadline for such change?
>> It's not exactly bugfix. On the other hand there's no wide adoption of
>> the protocol so it's still not critical to change it.
> 
> Right. So you have to do this before the hard freeze.
> And don't cut it too fine either :)

OK, i did the change. Pasting code here for review, to keep the thread.

diff --git a/exec.c b/exec.c
index c849405..a94c583 100644
--- a/exec.c
+++ b/exec.c
@@ -1456,6 +1456,13 @@ int qemu_get_ram_fd(ram_addr_t addr)
     return block->fd;
 }

+void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
+{
+    RAMBlock *block = qemu_get_ram_block(addr);
+
+    return block->host;
+}
+
 /* Return a host pointer to ram allocated with qemu_ram_alloc.
    With the exception of the softmmu code in this file, this should
    only be used for local memory (e.g. video ram) that the device owns,
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 0df6a93..0cef2d3 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -14,6 +14,7 @@
 #include "sysemu/kvm.h"
 #include "qemu/error-report.h"
 #include "qemu/sockets.h"
+#include "exec/ram_addr.h"

 #include <fcntl.h>
 #include <unistd.h>
@@ -47,6 +48,7 @@ typedef struct VhostUserMemoryRegion {
     uint64_t guest_phys_addr;
     uint64_t memory_size;
     uint64_t userspace_addr;
+    uint64_t shm_offset;
 } VhostUserMemoryRegion;

 typedef struct VhostUserMemory {
@@ -183,10 +185,10 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
 {
     VhostUserMsg msg;
     VhostUserRequest msg_request;
-    RAMBlock *block = 0;
     struct vhost_vring_file *file = 0;
     int need_reply = 0;
     int fds[VHOST_MEMORY_MAX_NREGIONS];
+    int i, fd;
     size_t fd_num = 0;

     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
@@ -212,14 +214,16 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
         break;

     case VHOST_SET_MEM_TABLE:
-        QTAILQ_FOREACH(block, &ram_list.blocks, next)
-        {
-            if (block->fd > 0) {
-                msg.memory.regions[fd_num].userspace_addr =
-                    (uintptr_t) block->host;
-                msg.memory.regions[fd_num].memory_size = block->length;
-                msg.memory.regions[fd_num].guest_phys_addr = block->offset;
-                fds[fd_num++] = block->fd;
+        for (i = 0; i < dev->mem->nregions; ++i) {
+            struct vhost_memory_region *reg = dev->mem->regions + i;
+            fd = qemu_get_ram_fd(reg->guest_phys_addr);
+            if (fd > 0) {
+                msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
+                msg.memory.regions[fd_num].memory_size  = reg->memory_size;
+                msg.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
+                msg.memory.regions[fd_num].shm_offset = reg->userspace_addr -
+                    (ram_addr_t) qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
+                fds[fd_num++] = fd;
             }
         }

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 55ca676..e9eb831 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -29,6 +29,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
                                    MemoryRegion *mr);
 ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
 int qemu_get_ram_fd(ram_addr_t addr);
+void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
 void *qemu_get_ram_ptr(ram_addr_t addr);
 void qemu_ram_free(ram_addr_t addr);
 void qemu_ram_free_from_ptr(ram_addr_t addr);

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 25 Jun 2014, at 18:44, Paolo Bonzini <pbonzini@redhat.com> wrote:

>> nregions: 4
>> region:
>> 	gpa = 0x100000000
>> 	size = 3221225472
>> 	ua = 0x2aab6ac00000
> 
> High memory, above 3 gigabytes.
> 
>> region:
>> 	gpa = 0xFFFC0000
>> 	size = 262144
>> 	ua = 0x7fc13d200000
> 
> This is the BIOS.  There shouldn't be any FD for this one, it
> is not allocated in hugetlbfs.
> 
>> region:
>> 	gpa = 0x0
>> 	size = 655360
>> 	ua = 0x2aaaaac00000
>> region:
>> 	gpa = 0xC0000
>> 	size = 3220439040
>> 	ua = 0x2aaaaacc0000
> 
> Together, it's the first 3 GB of memory.
> 
> I understand now what you mean.  Yeah, the format should be changed
> to include the offset (why does vhost-user need the ua at all?
> perhaps the offset can replace the ua).
> 
>> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
>> to the same place, but qemu_get_ram_fd() returns same FD for it.
> 
> This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> here, so no descriptor should be passed to vhost-user.

Problem is inside qemu_get_ram_block():


    if (block && addr - block->offset < block->length) {
        goto found;
    }

this is true if we have > 4G of RAM allocated.

Any advice how to fix this?

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 09:52:09PM +0000, Damjan Marion (damarion) wrote:
> 
> On 25 Jun 2014, at 18:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> >> nregions: 4
> >> region:
> >> 	gpa = 0x100000000
> >> 	size = 3221225472
> >> 	ua = 0x2aab6ac00000
> > 
> > High memory, above 3 gigabytes.
> > 
> >> region:
> >> 	gpa = 0xFFFC0000
> >> 	size = 262144
> >> 	ua = 0x7fc13d200000
> > 
> > This is the BIOS.  There shouldn't be any FD for this one, it
> > is not allocated in hugetlbfs.
> > 
> >> region:
> >> 	gpa = 0x0
> >> 	size = 655360
> >> 	ua = 0x2aaaaac00000
> >> region:
> >> 	gpa = 0xC0000
> >> 	size = 3220439040
> >> 	ua = 0x2aaaaacc0000
> > 
> > Together, it's the first 3 GB of memory.
> > 
> > I understand now what you mean.  Yeah, the format should be changed
> > to include the offset (why does vhost-user need the ua at all?
> > perhaps the offset can replace the ua).
> > 
> >> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
> >> to the same place, but qemu_get_ram_fd() returns same FD for it.
> > 
> > This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> > here, so no descriptor should be passed to vhost-user.
> 
> Problem is inside qemu_get_ram_block():
> 
> 
>     if (block && addr - block->offset < block->length) {
>         goto found;
>     }
> 
> this is true if we have > 4G of RAM allocated.

Hmm I don't get it. Why is it always true for >4G RAM?

> 
> Any advice how to fix this?
> 
> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 26 Jun 2014, at 09:13, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Jun 25, 2014 at 09:52:09PM +0000, Damjan Marion (damarion) wrote:
>> 
>> On 25 Jun 2014, at 18:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 
>>>> nregions: 4
>>>> region:
>>>> 	gpa = 0x100000000
>>>> 	size = 3221225472
>>>> 	ua = 0x2aab6ac00000
>>> 
>>> High memory, above 3 gigabytes.
>>> 
>>>> region:
>>>> 	gpa = 0xFFFC0000
>>>> 	size = 262144
>>>> 	ua = 0x7fc13d200000
>>> 
>>> This is the BIOS.  There shouldn't be any FD for this one, it
>>> is not allocated in hugetlbfs.
>>> 
>>>> region:
>>>> 	gpa = 0x0
>>>> 	size = 655360
>>>> 	ua = 0x2aaaaac00000
>>>> region:
>>>> 	gpa = 0xC0000
>>>> 	size = 3220439040
>>>> 	ua = 0x2aaaaacc0000
>>> 
>>> Together, it's the first 3 GB of memory.
>>> 
>>> I understand now what you mean.  Yeah, the format should be changed
>>> to include the offset (why does vhost-user need the ua at all?
>>> perhaps the offset can replace the ua).
>>> 
>>>> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
>>>> to the same place, but qemu_get_ram_fd() returns same FD for it.
>>> 
>>> This must be a bug.  I would have expected qemu_get_ram_fd to return -1
>>> here, so no descriptor should be passed to vhost-user.
>> 
>> Problem is inside qemu_get_ram_block():
>> 
>> 
>>    if (block && addr - block->offset < block->length) {
>>        goto found;
>>    }
>> 
>> this is true if we have > 4G of RAM allocated.
> 
> Hmm I don't get it. Why is it always true for >4G RAM?

This check assumes that guest memory is mapped to RAM block 
without gaps. as BIOS is mapped to guest address space
near 4G this check will return true if block size is bigger than
BIOS address.

> 
>> 
>> Any advice how to fix this?
>> 
>> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Michael S. Tsirkin]

On Thu, Jun 26, 2014 at 07:44:24AM +0000, Damjan Marion (damarion) wrote:
> 
> On 26 Jun 2014, at 09:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > On Wed, Jun 25, 2014 at 09:52:09PM +0000, Damjan Marion (damarion) wrote:
> >> 
> >> On 25 Jun 2014, at 18:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> 
> >>>> nregions: 4
> >>>> region:
> >>>> 	gpa = 0x100000000
> >>>> 	size = 3221225472
> >>>> 	ua = 0x2aab6ac00000
> >>> 
> >>> High memory, above 3 gigabytes.
> >>> 
> >>>> region:
> >>>> 	gpa = 0xFFFC0000
> >>>> 	size = 262144
> >>>> 	ua = 0x7fc13d200000
> >>> 
> >>> This is the BIOS.  There shouldn't be any FD for this one, it
> >>> is not allocated in hugetlbfs.
> >>> 
> >>>> region:
> >>>> 	gpa = 0x0
> >>>> 	size = 655360
> >>>> 	ua = 0x2aaaaac00000
> >>>> region:
> >>>> 	gpa = 0xC0000
> >>>> 	size = 3220439040
> >>>> 	ua = 0x2aaaaacc0000
> >>> 
> >>> Together, it's the first 3 GB of memory.
> >>> 
> >>> I understand now what you mean.  Yeah, the format should be changed
> >>> to include the offset (why does vhost-user need the ua at all?
> >>> perhaps the offset can replace the ua).
> >>> 
> >>>> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
> >>>> to the same place, but qemu_get_ram_fd() returns same FD for it.
> >>> 
> >>> This must be a bug.  I would have expected qemu_get_ram_fd to return -1
> >>> here, so no descriptor should be passed to vhost-user.
> >> 
> >> Problem is inside qemu_get_ram_block():
> >> 
> >> 
> >>    if (block && addr - block->offset < block->length) {
> >>        goto found;
> >>    }
> >> 
> >> this is true if we have > 4G of RAM allocated.
> > 
> > Hmm I don't get it. Why is it always true for >4G RAM?
> 
> This check assumes that guest memory is mapped to RAM block 
> without gaps. as BIOS is mapped to guest address space
> near 4G this check will return true if block size is bigger than
> BIOS address.


But then offset is > addr so addr - offset is a huge value no?

> > 
> >> 
> >> Any advice how to fix this?
> >> 
> >> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Damjan Marion (damarion)]

On 26 Jun 2014, at 09:56, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Thu, Jun 26, 2014 at 07:44:24AM +0000, Damjan Marion (damarion) wrote:
>> 
>> On 26 Jun 2014, at 09:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>> 
>>> On Wed, Jun 25, 2014 at 09:52:09PM +0000, Damjan Marion (damarion) wrote:
>>>> 
>>>> On 25 Jun 2014, at 18:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> 
>>>>>> nregions: 4
>>>>>> region:
>>>>>> 	gpa = 0x100000000
>>>>>> 	size = 3221225472
>>>>>> 	ua = 0x2aab6ac00000
>>>>> 
>>>>> High memory, above 3 gigabytes.
>>>>> 
>>>>>> region:
>>>>>> 	gpa = 0xFFFC0000
>>>>>> 	size = 262144
>>>>>> 	ua = 0x7fc13d200000
>>>>> 
>>>>> This is the BIOS.  There shouldn't be any FD for this one, it
>>>>> is not allocated in hugetlbfs.
>>>>> 
>>>>>> region:
>>>>>> 	gpa = 0x0
>>>>>> 	size = 655360
>>>>>> 	ua = 0x2aaaaac00000
>>>>>> region:
>>>>>> 	gpa = 0xC0000
>>>>>> 	size = 3220439040
>>>>>> 	ua = 0x2aaaaacc0000
>>>>> 
>>>>> Together, it's the first 3 GB of memory.
>>>>> 
>>>>> I understand now what you mean.  Yeah, the format should be changed
>>>>> to include the offset (why does vhost-user need the ua at all?
>>>>> perhaps the offset can replace the ua).
>>>>> 
>>>>>> BTW Any idea what to do with region 2, it doesn’t look like the one belonging
>>>>>> to the same place, but qemu_get_ram_fd() returns same FD for it.
>>>>> 
>>>>> This must be a bug.  I would have expected qemu_get_ram_fd to return -1
>>>>> here, so no descriptor should be passed to vhost-user.
>>>> 
>>>> Problem is inside qemu_get_ram_block():
>>>> 
>>>> 
>>>>   if (block && addr - block->offset < block->length) {
>>>>       goto found;
>>>>   }
>>>> 
>>>> this is true if we have > 4G of RAM allocated.
>>> 
>>> Hmm I don't get it. Why is it always true for >4G RAM?
>> 
>> This check assumes that guest memory is mapped to RAM block 
>> without gaps. as BIOS is mapped to guest address space
>> near 4G this check will return true if block size is bigger than
>> BIOS address.
> 
> 
> But then offset is > addr so addr - offset is a huge value no?

With 6GB allocated to VM, I’see:

block->offset = 0
block->length = 0x180000000

So if addr = 0xFFFC0000 then it will be false positive.


> 
>>> 
>>>> 
>>>> Any advice how to fix this?
>>>> 
>>>> 

Re: [Qemu-devel] vhost-user: broken mem regions

[Nikolay Nikolaev]

On Thu, Jun 26, 2014 at 12:37 AM, Damjan Marion (damarion)
<damarion@cisco.com> wrote:
>
> On 25 Jun 2014, at 20:18, Michael S. Tsirkin <mst@redhat.com> wrote:
>
>> On Wed, Jun 25, 2014 at 09:13:36PM +0300, Nikolay Nikolaev wrote:
>>> On Wed, Jun 25, 2014 at 9:07 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Wed, Jun 25, 2014 at 07:56:46PM +0300, Nikolay Nikolaev wrote:
>>>>> On Wed, Jun 25, 2014 at 7:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>>> nregions: 4
>>>>>>> region:
>>>>>>>      gpa = 0x100000000
>>>>>>>      size = 3221225472
>>>>>>>      ua = 0x2aab6ac00000
>>>>>>
>>>>>> High memory, above 3 gigabytes.
>>>>>>
>>>>>>> region:
>>>>>>>      gpa = 0xFFFC0000
>>>>>>>      size = 262144
>>>>>>>      ua = 0x7fc13d200000
>>>>>>
>>>>>> This is the BIOS.  There shouldn't be any FD for this one, it
>>>>>> is not allocated in hugetlbfs.
>>>>>>
>>>>>>> region:
>>>>>>>      gpa = 0x0
>>>>>>>      size = 655360
>>>>>>>      ua = 0x2aaaaac00000
>>>>>>> region:
>>>>>>>      gpa = 0xC0000
>>>>>>>      size = 3220439040
>>>>>>>      ua = 0x2aaaaacc0000
>>>>>>
>>>>>> Together, it's the first 3 GB of memory.
>>>>>>
>>>>>> I understand now what you mean.  Yeah, the format should be changed
>>>>>> to include the offset (why does vhost-user need the ua at all?
>>>>> The vring addresses are QEMU UA addresses. Of course vhost-user can
>>>>> translate them to guest physical before sending the message.
>>>>
>>>> It seems useful to have them as QEMU UA, this will allow
>>>> frontends other than virtio where QEMU operates the
>>>> ring.
>>>
>>> OK - so there will be a new offset field. That's fine with me. What
>>> would be the deadline for such change?
>>> It's not exactly bugfix. On the other hand there's no wide adoption of
>>> the protocol so it's still not critical to change it.
>>
>> Right. So you have to do this before the hard freeze.
>> And don't cut it too fine either :)
>
> OK, i did the change. Pasting code here for review, to keep the thread.
>
> diff --git a/exec.c b/exec.c
> index c849405..a94c583 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1456,6 +1456,13 @@ int qemu_get_ram_fd(ram_addr_t addr)
>      return block->fd;
>  }
>
> +void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> +{
> +    RAMBlock *block = qemu_get_ram_block(addr);
> +
> +    return block->host;
> +}
> +
>  /* Return a host pointer to ram allocated with qemu_ram_alloc.
>     With the exception of the softmmu code in this file, this should
>     only be used for local memory (e.g. video ram) that the device owns,
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 0df6a93..0cef2d3 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -14,6 +14,7 @@
>  #include "sysemu/kvm.h"
>  #include "qemu/error-report.h"
>  #include "qemu/sockets.h"
> +#include "exec/ram_addr.h"
>
>  #include <fcntl.h>
>  #include <unistd.h>
> @@ -47,6 +48,7 @@ typedef struct VhostUserMemoryRegion {
>      uint64_t guest_phys_addr;
>      uint64_t memory_size;
>      uint64_t userspace_addr;
> +    uint64_t shm_offset;
It's not bound to shm. Can it be mmap_offset?
>  } VhostUserMemoryRegion;
>
>  typedef struct VhostUserMemory {
> @@ -183,10 +185,10 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>  {
>      VhostUserMsg msg;
>      VhostUserRequest msg_request;
> -    RAMBlock *block = 0;
>      struct vhost_vring_file *file = 0;
>      int need_reply = 0;
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
> +    int i, fd;
>      size_t fd_num = 0;
>
>      assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
> @@ -212,14 +214,16 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          break;
>
>      case VHOST_SET_MEM_TABLE:
> -        QTAILQ_FOREACH(block, &ram_list.blocks, next)
> -        {
> -            if (block->fd > 0) {
> -                msg.memory.regions[fd_num].userspace_addr =
> -                    (uintptr_t) block->host;
> -                msg.memory.regions[fd_num].memory_size = block->length;
> -                msg.memory.regions[fd_num].guest_phys_addr = block->offset;
> -                fds[fd_num++] = block->fd;
> +        for (i = 0; i < dev->mem->nregions; ++i) {
> +            struct vhost_memory_region *reg = dev->mem->regions + i;
> +            fd = qemu_get_ram_fd(reg->guest_phys_addr);
> +            if (fd > 0) {
> +                msg.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
> +                msg.memory.regions[fd_num].memory_size  = reg->memory_size;
> +                msg.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
> +                msg.memory.regions[fd_num].shm_offset = reg->userspace_addr -
> +                    (ram_addr_t) qemu_get_ram_block_host_ptr(reg->guest_phys_addr);
It may be a good idea to assert that fd_num is less than
VHOST_MEMORY_MAX_NREGIONS
> +                fds[fd_num++] = fd;
>              }
>          }
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 55ca676..e9eb831 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -29,6 +29,7 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>                                     MemoryRegion *mr);
>  ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
>  int qemu_get_ram_fd(ram_addr_t addr);
> +void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
>  void *qemu_get_ram_ptr(ram_addr_t addr);
>  void qemu_ram_free(ram_addr_t addr);
>  void qemu_ram_free_from_ptr(ram_addr_t addr);
>
>
>

In general proposed changes are OK with snabbswitch vhost-user
implementation. We'll adapt once this is upstream.

regards,
Nikolay Nikolaev