Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Yongji Xie]

On 2016/10/1 0:05, Paolo Bonzini wrote:


On 14/09/2016 00:55, Alex Williamson wrote:

Paolo, as the reigning memory API expert, do you see any issues with
this?  Expanding the size of a container MemoryRegion and the contained
mmap'd subregion and manipulating priorities so that we don't interfere
with other MemoryRegions.

I guess it's fine if you are okay with maintaining it.
memory_region_set_size exists, might as well use it. :)

What I'm worried about, is what happens if two such regions end up in
the same guest page.  Then the two priorities conflict.



Hi Paolo,

I think I can answer this question. We would expand only one MemoryRegion
which is page aligned and set its priority to zero if we have two region 
in the same
guest page. Then the Memory Region with higher priority will overlap the 
expanded

part of page aligned one as if we didn't do the expanding.

Thanks,
Yongji

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Paolo Bonzini]

On 14/09/2016 00:55, Alex Williamson wrote:
> Paolo, as the reigning memory API expert, do you see any issues with
> this?  Expanding the size of a container MemoryRegion and the contained
> mmap'd subregion and manipulating priorities so that we don't interfere
> with other MemoryRegions.

I guess it's fine if you are okay with maintaining it.
memory_region_set_size exists, might as well use it. :)

What I'm worried about, is what happens if two such regions end up in
the same guest page.  Then the two priorities conflict.

Paolo

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Alex Williamson]

On Fri, 30 Sep 2016 06:00:42 +0200
Thorsten Kohfeldt  wrote:

> (Re-post, as my mail client somehow made my previous post attach to the wrong 
> thread.
> I do not mean to spam y'all, but maybe my previous mail got lost in your 
> filters ...
> ... as I have not yet seen any answer to my questions/remarks.
> )

Hi Thorsten,

I saw your message and I even spent part of a day looking again at the
rtl quirk (I think there's still a bug in it), but you still haven't
proven to me that there's an issue with quirks splitting pages.  On my
device BAR2 is 4k and the quirk covers 8 bytes, a dword at 0x70 and
another at 0x74.  I assign it to the guest and I use a test program to
mmap the pci-sysfs resource2 file to dump it from guest userspace.  It
works.  If I use gdb on QEMU, I see that 0x70 and 0x74 are handled via
the quirk functions and the remainder come from the mmap via the cpu
physical rw functions.

One interesting thing I found is that dumping the range from the
monitor doesn't work because the memcpy it uses does greater than dword
accesses and the device doesn't appear to support that, even on bare
metal.  So actually if we disable the mmap, a dump does work because
those accesses are broken into dword chunks.

However, I don't see what you're referring to with the quirk somehow
breaking access to the rest of the BAR.  Yes, it does prevent the
entire BAR from being mapped directly into the guest address space, it
will trap into QEMU, but that trapping works as intended AFAICT.  This
should behave the same way.  Please provide a test that explicitly
shows how accesses on the same page as a quirk are serviced
improperly.  Thanks,

Alex

>  > On 2016/9/14 6:55, Alex Williamson wrote:
>  >
>  > [cc +Paolo]
>  >  
>  >> On Thu, 11 Aug 2016 19:05:57 +0800
>  >> Yongji Xie  wrote:
>  >>
>  >> Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
>  >> sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
>  >> to mmap sub-page BARs. This is the corresponding QEMU patch.  
> 
> Immediate questions first:
> It seems that mentioned commit will be part of Kernel 4.8 ?
> But as far as I can judge this change should also cooperate with
> older/existing kernels (which then just have qemu behave as before) ?
> 
> (For my actual point of interrest related to this patch please see further 
> down.)
> 
>  >> With those patches applied, we could passthrough sub-page BARs
>  >> to guest, which can help to improve IO performance for some devices.
>  >>
>  >> In this patch, we expand MemoryRegions of these sub-page
>  >> MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
>  >> the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
>  >> with a valid size. The expanding size will be recovered when
>  >> the base address of sub-page BAR is changed and not page aligned
>  >> any more in guest. And we also set the priority of these BARs'
>  >> memory regions to zero in case of overlap with BARs which share
>  >> the same page with sub-page BARs in guest.
>  >>
>  >> Signed-off-by: Yongji Xie 
>  >> ---
>  >> hw/vfio/common.c |3 +--
>  >> hw/vfio/pci.c|   76 
> ++
>  >> 2 files changed, 77 insertions(+), 2 deletions(-)  
>  >
>  > Hi Yongji,  
> ...
>  >> +mr = region->mem;
>  >> +mmap_mr = >mmaps[0].mem;
>  >> +memory_region_transaction_begin();
>  >> +if (memory_region_size(mr) < qemu_real_host_page_size) {
>  >> +if (!(bar_addr & ~qemu_real_host_page_mask) &&
>  >> +memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
>  >> +/* Expand memory region to page size and set priority */
>  >> +memory_region_del_subregion(r->address_space, mr);
>  >> +memory_region_set_size(mr, qemu_real_host_page_size);
>  >> +memory_region_set_size(mmap_mr, qemu_real_host_page_size);
>  >> +memory_region_add_subregion_overlap(r->address_space,
>  >> +bar_addr, mr, 0);
>  >> +   }
>  >> +} else {
>  >> +/* This case would happen when guest rescan one PCI device */
>  >> +if (bar_addr & ~qemu_real_host_page_mask) {
>  >> +/* Recover the size of memory region */
>  >> +memory_region_set_size(mr, r->size);
>  >> +memory_region_set_size(mmap_mr, r->size);
>  >> +} else if (memory_region_is_mapped(mr)) {
>  >> +/* Set the priority of memory region to zero */
>  >> +memory_region_del_subregion(r->address_space, mr);
>  >> +memory_region_add_subregion_overlap(r->address_space,
>  >> +bar_addr, mr, 0);
>  >> +}
>  >> +}
>  >> +memory_region_transaction_commit();  
>  >
>  > Paolo, as the reigning memory API expert, do you see any issues with
>  > this?  Expanding the size of a container MemoryRegion 

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Thorsten Kohfeldt]

(Re-post, as my mail client somehow made my previous post attach to the wrong 
thread.
I do not mean to spam y'all, but maybe my previous mail got lost in your 
filters ...
... as I have not yet seen any answer to my questions/remarks.
)

> On 2016/9/14 6:55, Alex Williamson wrote:
>
> [cc +Paolo]
>
>> On Thu, 11 Aug 2016 19:05:57 +0800
>> Yongji Xie  wrote:
>>
>> Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
>> sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
>> to mmap sub-page BARs. This is the corresponding QEMU patch.

Immediate questions first:
It seems that mentioned commit will be part of Kernel 4.8 ?
But as far as I can judge this change should also cooperate with
older/existing kernels (which then just have qemu behave as before) ?

(For my actual point of interrest related to this patch please see further 
down.)

>> With those patches applied, we could passthrough sub-page BARs
>> to guest, which can help to improve IO performance for some devices.
>>
>> In this patch, we expand MemoryRegions of these sub-page
>> MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
>> the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
>> with a valid size. The expanding size will be recovered when
>> the base address of sub-page BAR is changed and not page aligned
>> any more in guest. And we also set the priority of these BARs'
>> memory regions to zero in case of overlap with BARs which share
>> the same page with sub-page BARs in guest.
>>
>> Signed-off-by: Yongji Xie 
>> ---
>> hw/vfio/common.c |3 +--
>> hw/vfio/pci.c|   76 
++
>> 2 files changed, 77 insertions(+), 2 deletions(-)
>
> Hi Yongji,
...
>> +mr = region->mem;
>> +mmap_mr = >mmaps[0].mem;
>> +memory_region_transaction_begin();
>> +if (memory_region_size(mr) < qemu_real_host_page_size) {
>> +if (!(bar_addr & ~qemu_real_host_page_mask) &&
>> +memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
>> +/* Expand memory region to page size and set priority */
>> +memory_region_del_subregion(r->address_space, mr);
>> +memory_region_set_size(mr, qemu_real_host_page_size);
>> +memory_region_set_size(mmap_mr, qemu_real_host_page_size);
>> +memory_region_add_subregion_overlap(r->address_space,
>> +bar_addr, mr, 0);
>> +   }
>> +} else {
>> +/* This case would happen when guest rescan one PCI device */
>> +if (bar_addr & ~qemu_real_host_page_mask) {
>> +/* Recover the size of memory region */
>> +memory_region_set_size(mr, r->size);
>> +memory_region_set_size(mmap_mr, r->size);
>> +} else if (memory_region_is_mapped(mr)) {
>> +/* Set the priority of memory region to zero */
>> +memory_region_del_subregion(r->address_space, mr);
>> +memory_region_add_subregion_overlap(r->address_space,
>> +bar_addr, mr, 0);
>> +}
>> +}
>> +memory_region_transaction_commit();
>
> Paolo, as the reigning memory API expert, do you see any issues with
> this?  Expanding the size of a container MemoryRegion and the contained
> mmap'd subregion and manipulating priorities so that we don't interfere
> with other MemoryRegions.

Since the following qemu commit function memory_region_add_subregion_overlap()
actually has a misleading name:
http://git.qemu.org/?p=qemu.git;a=blobdiff;f=memory.c;h=ac5236b51587ee397edd177502fc20ce159f2235;hp=9daac5ea2d9a9c83533880a812760683f6e09765;hb=b61359781958759317ee6fd1a45b59be0b7dbbe1;hpb=ab0a99560857302b60053c245d1231acbd976cd4

The sole thing that memory_region_add_subregion_overlap() now actually does
differently from memory_region_add_subregion() is nothing else than setting
the region's priority to a value of callers choice.
The _default_ priority as chosen by memory_region_add_subregion() _is_ 0.

So, explicitly choosing memory_region_add_subregion_overlap(... , 0) does
nothing new.
Or does it:
Actually, _yes_, because I see Alex actually willing to discuss choice
of memory region priorities related to VFIO and mmap.
Why do I "invade" this thread ?
I would like you to consider thinking twice about selecting proper priorities
for _any_ mmap related region (i.e. also the aligned case), and here is why:
(I will also make a statement related to region expansion for alignment.)

First of all, I recently suggested a patch which can visualise what I
write about subsequently:
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01315.html
(I would appreciate if somebody would review and thus get it merged.)

As a general remark, the sub-page mmap case does not only occur when
a 'small' BAR is encountered, it also occurs when a fully mmap-ed
page is split by a 'small' VFIO quirk.
Hi Alex, here we go 

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Thorsten Kohfeldt]

> On 2016/9/14 6:55, Alex Williamson wrote:
>
> [cc +Paolo]
>
>> On Thu, 11 Aug 2016 19:05:57 +0800
>> Yongji Xie  wrote:
>>
>> Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
>> sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
>> to mmap sub-page BARs. This is the corresponding QEMU patch.

Immediate questions first:
It seems that mentioned commit will be part of Kernel 4.8 ?
But as far as I can judge this change should also cooperate with
older/existing kernels (which then just have qemu behave as before) ?

(For my actual point of interrest related to this patch please see further 
down.)

>> With those patches applied, we could passthrough sub-page BARs
>> to guest, which can help to improve IO performance for some devices.
>>
>> In this patch, we expand MemoryRegions of these sub-page
>> MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
>> the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
>> with a valid size. The expanding size will be recovered when
>> the base address of sub-page BAR is changed and not page aligned
>> any more in guest. And we also set the priority of these BARs'
>> memory regions to zero in case of overlap with BARs which share
>> the same page with sub-page BARs in guest.
>>
>> Signed-off-by: Yongji Xie 
>> ---
>> hw/vfio/common.c |3 +--
>> hw/vfio/pci.c|   76 
++
>> 2 files changed, 77 insertions(+), 2 deletions(-)
>
> Hi Yongji,
...
>> +mr = region->mem;
>> +mmap_mr = >mmaps[0].mem;
>> +memory_region_transaction_begin();
>> +if (memory_region_size(mr) < qemu_real_host_page_size) {
>> +if (!(bar_addr & ~qemu_real_host_page_mask) &&
>> +memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
>> +/* Expand memory region to page size and set priority */
>> +memory_region_del_subregion(r->address_space, mr);
>> +memory_region_set_size(mr, qemu_real_host_page_size);
>> +memory_region_set_size(mmap_mr, qemu_real_host_page_size);
>> +memory_region_add_subregion_overlap(r->address_space,
>> +bar_addr, mr, 0);
>> +   }
>> +} else {
>> +/* This case would happen when guest rescan one PCI device */
>> +if (bar_addr & ~qemu_real_host_page_mask) {
>> +/* Recover the size of memory region */
>> +memory_region_set_size(mr, r->size);
>> +memory_region_set_size(mmap_mr, r->size);
>> +} else if (memory_region_is_mapped(mr)) {
>> +/* Set the priority of memory region to zero */
>> +memory_region_del_subregion(r->address_space, mr);
>> +memory_region_add_subregion_overlap(r->address_space,
>> +bar_addr, mr, 0);
>> +}
>> +}
>> +memory_region_transaction_commit();
>
> Paolo, as the reigning memory API expert, do you see any issues with
> this?  Expanding the size of a container MemoryRegion and the contained
> mmap'd subregion and manipulating priorities so that we don't interfere
> with other MemoryRegions.

Since the following qemu commit function memory_region_add_subregion_overlap()
actually has a misleading name:
http://git.qemu.org/?p=qemu.git;a=blobdiff;f=memory.c;h=ac5236b51587ee397edd177502fc20ce159f2235;hp=9daac5ea2d9a9c83533880a812760683f6e09765;hb=b61359781958759317ee6fd1a45b59be0b7dbbe1;hpb=ab0a99560857302b60053c245d1231acbd976cd4

The sole thing that memory_region_add_subregion_overlap() now actually does
differently from memory_region_add_subregion() is nothing else than setting
the region's priority to a value of callers choice.
The _default_ priority as chosen by memory_region_add_subregion() _is_ 0.

So, explicitly choosing memory_region_add_subregion_overlap(... , 0) does
nothing new.
Or does it:
Actually, _yes_, because I see Alex actually willing to discuss choice
of memory region priorities related to VFIO and mmap.
Why do I "invade" this thread ?
I would like you to consider thinking twice about selecting proper priorities
for _any_ mmap related region (i.e. also the aligned case), and here is why:
(I will also make a statement related to region expansion for alignment.)

First of all, I recently suggested a patch which can visualise what I
write about subsequently:
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg01315.html
(I would appreciate if somebody would review and thus get it merged.)

As a general remark, the sub-page mmap case does not only occur when
a 'small' BAR is encountered, it also occurs when a fully mmap-ed
page is split by a 'small' VFIO quirk.
Hi Alex, here we go again about RTL8168 and its MSIX quirk.
(Subsequently I also relate to/conclude for Yongji's patch.)
Mentioned quirk cuts for certain RTL8168 models a full-page BAR
right into 3 pieces, 0..qirkaddr-1, quirk and quirk+qsize..pagesize-1.
What 

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Yongji Xie]

On 2016/9/14 6:55, Alex Williamson wrote:


[cc +Paolo]

On Thu, 11 Aug 2016 19:05:57 +0800
Yongji Xie  wrote:


Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
to mmap sub-page BARs. This is the corresponding QEMU patch.
With those patches applied, we could passthrough sub-page BARs
to guest, which can help to improve IO performance for some devices.

In this patch, we expand MemoryRegions of these sub-page
MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
with a valid size. The expanding size will be recovered when
the base address of sub-page BAR is changed and not page aligned
any more in guest. And we also set the priority of these BARs'
memory regions to zero in case of overlap with BARs which share
the same page with sub-page BARs in guest.

Signed-off-by: Yongji Xie 
---
  hw/vfio/common.c |3 +--
  hw/vfio/pci.c|   76 ++
  2 files changed, 77 insertions(+), 2 deletions(-)

Hi Yongji,

There was an automated patch checker reply to this patch already, see:

https://patchwork.kernel.org/patch/9275077/

Looks like there's a trivial whitespace issue with using a tab.  Also
another QEMU style issue noted below.


Yes,  I saw it. I'll fix it in next version. Thanks for your remind.


diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b313e7c..1a70307 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -661,8 +661,7 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, 
VFIORegion *region,
region, name, region->size);
  
  if (!vbasedev->no_mmap &&

-region->flags & VFIO_REGION_INFO_FLAG_MMAP &&
-!(region->size & ~qemu_real_host_page_mask)) {
+region->flags & VFIO_REGION_INFO_FLAG_MMAP) {
  
  vfio_setup_region_sparse_mmaps(region, info);
  
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c

index 7bfa17c..7035617 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1057,6 +1057,65 @@ static const MemoryRegionOps vfio_vga_ops = {
  };
  
  /*

+ * Expand memory region of sub-page(size < PAGE_SIZE) MMIO BAR to page
+ * size if the BAR is in an exclusive page in host so that we could map
+ * this BAR to guest. But this sub-page BAR may not occupy an exclusive
+ * page in guest. So we should set the priority of the expanded memory
+ * region to zero in case of overlap with BARs which share the same page
+ * with the sub-page BAR in guest. Besides, we should also recover the
+ * size of this sub-page BAR when its base address is changed in guest
+ * and not page aligned any more.
+ */
+static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
+{
+VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+VFIORegion *region = >bars[bar].region;
+MemoryRegion *mmap_mr, *mr;
+PCIIORegion *r;
+pcibus_t bar_addr;
+
+/* Make sure that the whole region is allowed to be mmapped */
+if (!(region->nr_mmaps == 1 &&
+region->mmaps[0].size == region->size)) {
+return;
+}
+
+r = >io_regions[bar];
+bar_addr = r->addr;
+if (bar_addr == PCI_BAR_UNMAPPED) {
+return;
+}
+
+mr = region->mem;
+mmap_mr = >mmaps[0].mem;
+memory_region_transaction_begin();
+if (memory_region_size(mr) < qemu_real_host_page_size) {
+if (!(bar_addr & ~qemu_real_host_page_mask) &&
+memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
+/* Expand memory region to page size and set priority */
+memory_region_del_subregion(r->address_space, mr);
+memory_region_set_size(mr, qemu_real_host_page_size);
+memory_region_set_size(mmap_mr, qemu_real_host_page_size);
+memory_region_add_subregion_overlap(r->address_space,
+bar_addr, mr, 0);
+   }
+} else {
+/* This case would happen when guest rescan one PCI device */
+if (bar_addr & ~qemu_real_host_page_mask) {
+/* Recover the size of memory region */
+memory_region_set_size(mr, r->size);
+memory_region_set_size(mmap_mr, r->size);
+} else if (memory_region_is_mapped(mr)) {
+/* Set the priority of memory region to zero */
+memory_region_del_subregion(r->address_space, mr);
+memory_region_add_subregion_overlap(r->address_space,
+bar_addr, mr, 0);
+}
+}
+memory_region_transaction_commit();

Paolo, as the reigning memory API expert, do you see any issues with
this?  Expanding the size of a container MemoryRegion and the contained
mmap'd subregion and manipulating priorities so that we don't interfere
with other MemoryRegions.


+}
+
+/*
   * PCI config space
   */
  uint32_t 

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Alex Williamson]

[cc +Paolo]

On Thu, 11 Aug 2016 19:05:57 +0800
Yongji Xie  wrote:

> Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
> sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
> to mmap sub-page BARs. This is the corresponding QEMU patch.
> With those patches applied, we could passthrough sub-page BARs
> to guest, which can help to improve IO performance for some devices.
> 
> In this patch, we expand MemoryRegions of these sub-page
> MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
> the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
> with a valid size. The expanding size will be recovered when
> the base address of sub-page BAR is changed and not page aligned
> any more in guest. And we also set the priority of these BARs'
> memory regions to zero in case of overlap with BARs which share
> the same page with sub-page BARs in guest.
> 
> Signed-off-by: Yongji Xie 
> ---
>  hw/vfio/common.c |3 +--
>  hw/vfio/pci.c|   76 
> ++
>  2 files changed, 77 insertions(+), 2 deletions(-)

Hi Yongji,

There was an automated patch checker reply to this patch already, see:

https://patchwork.kernel.org/patch/9275077/

Looks like there's a trivial whitespace issue with using a tab.  Also
another QEMU style issue noted below.

> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b313e7c..1a70307 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -661,8 +661,7 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, 
> VFIORegion *region,
>region, name, region->size);
>  
>  if (!vbasedev->no_mmap &&
> -region->flags & VFIO_REGION_INFO_FLAG_MMAP &&
> -!(region->size & ~qemu_real_host_page_mask)) {
> +region->flags & VFIO_REGION_INFO_FLAG_MMAP) {
>  
>  vfio_setup_region_sparse_mmaps(region, info);
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 7bfa17c..7035617 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1057,6 +1057,65 @@ static const MemoryRegionOps vfio_vga_ops = {
>  };
>  
>  /*
> + * Expand memory region of sub-page(size < PAGE_SIZE) MMIO BAR to page
> + * size if the BAR is in an exclusive page in host so that we could map
> + * this BAR to guest. But this sub-page BAR may not occupy an exclusive
> + * page in guest. So we should set the priority of the expanded memory
> + * region to zero in case of overlap with BARs which share the same page
> + * with the sub-page BAR in guest. Besides, we should also recover the
> + * size of this sub-page BAR when its base address is changed in guest
> + * and not page aligned any more.
> + */
> +static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
> +{
> +VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +VFIORegion *region = >bars[bar].region;
> +MemoryRegion *mmap_mr, *mr;
> +PCIIORegion *r;
> +pcibus_t bar_addr;
> +
> +/* Make sure that the whole region is allowed to be mmapped */
> +if (!(region->nr_mmaps == 1 &&
> +region->mmaps[0].size == region->size)) {
> +return;
> +}
> +
> +r = >io_regions[bar];
> +bar_addr = r->addr;
> +if (bar_addr == PCI_BAR_UNMAPPED) {
> +return;
> +}
> +
> +mr = region->mem;
> +mmap_mr = >mmaps[0].mem;
> +memory_region_transaction_begin();
> +if (memory_region_size(mr) < qemu_real_host_page_size) {
> +if (!(bar_addr & ~qemu_real_host_page_mask) &&
> +memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
> +/* Expand memory region to page size and set priority */
> +memory_region_del_subregion(r->address_space, mr);
> +memory_region_set_size(mr, qemu_real_host_page_size);
> +memory_region_set_size(mmap_mr, qemu_real_host_page_size);
> +memory_region_add_subregion_overlap(r->address_space,
> +bar_addr, mr, 0);
> + }
> +} else {
> +/* This case would happen when guest rescan one PCI device */
> +if (bar_addr & ~qemu_real_host_page_mask) {
> +/* Recover the size of memory region */
> +memory_region_set_size(mr, r->size);
> +memory_region_set_size(mmap_mr, r->size);
> +} else if (memory_region_is_mapped(mr)) {
> +/* Set the priority of memory region to zero */
> +memory_region_del_subregion(r->address_space, mr);
> +memory_region_add_subregion_overlap(r->address_space,
> +bar_addr, mr, 0);
> +}
> +}
> +memory_region_transaction_commit();

Paolo, as the reigning memory API expert, do you see any issues with
this?  Expanding the size of a container MemoryRegion and the contained
mmap'd subregion and manipulating priorities so that we don't interfere
with 

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[Yongji Xie]

Ping?

On 2016/8/11 19:05, Yongji Xie wrote:

Now the kernel commit 05f0c03fbac1 ("vfio-pci: Allow to mmap
sub-page MMIO BARs if the mmio page is exclusive") allows VFIO
to mmap sub-page BARs. This is the corresponding QEMU patch.
With those patches applied, we could passthrough sub-page BARs
to guest, which can help to improve IO performance for some devices.

In this patch, we expand MemoryRegions of these sub-page
MMIO BARs to PAGE_SIZE in vfio_pci_write_config(), so that
the BARs could be passed to KVM ioctl KVM_SET_USER_MEMORY_REGION
with a valid size. The expanding size will be recovered when
the base address of sub-page BAR is changed and not page aligned
any more in guest. And we also set the priority of these BARs'
memory regions to zero in case of overlap with BARs which share
the same page with sub-page BARs in guest.

Signed-off-by: Yongji Xie 
---
  hw/vfio/common.c |3 +--
  hw/vfio/pci.c|   76 ++
  2 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b313e7c..1a70307 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -661,8 +661,7 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev, 
VFIORegion *region,
region, name, region->size);

  if (!vbasedev->no_mmap &&
-region->flags & VFIO_REGION_INFO_FLAG_MMAP &&
-!(region->size & ~qemu_real_host_page_mask)) {
+region->flags & VFIO_REGION_INFO_FLAG_MMAP) {

  vfio_setup_region_sparse_mmaps(region, info);

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 7bfa17c..7035617 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1057,6 +1057,65 @@ static const MemoryRegionOps vfio_vga_ops = {
  };

  /*
+ * Expand memory region of sub-page(size < PAGE_SIZE) MMIO BAR to page
+ * size if the BAR is in an exclusive page in host so that we could map
+ * this BAR to guest. But this sub-page BAR may not occupy an exclusive
+ * page in guest. So we should set the priority of the expanded memory
+ * region to zero in case of overlap with BARs which share the same page
+ * with the sub-page BAR in guest. Besides, we should also recover the
+ * size of this sub-page BAR when its base address is changed in guest
+ * and not page aligned any more.
+ */
+static void vfio_sub_page_bar_update_mapping(PCIDevice *pdev, int bar)
+{
+VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+VFIORegion *region = >bars[bar].region;
+MemoryRegion *mmap_mr, *mr;
+PCIIORegion *r;
+pcibus_t bar_addr;
+
+/* Make sure that the whole region is allowed to be mmapped */
+if (!(region->nr_mmaps == 1 &&
+region->mmaps[0].size == region->size)) {
+return;
+}
+
+r = >io_regions[bar];
+bar_addr = r->addr;
+if (bar_addr == PCI_BAR_UNMAPPED) {
+return;
+}
+
+mr = region->mem;
+mmap_mr = >mmaps[0].mem;
+memory_region_transaction_begin();
+if (memory_region_size(mr) < qemu_real_host_page_size) {
+if (!(bar_addr & ~qemu_real_host_page_mask) &&
+memory_region_is_mapped(mr) && region->mmaps[0].mmap) {
+/* Expand memory region to page size and set priority */
+memory_region_del_subregion(r->address_space, mr);
+memory_region_set_size(mr, qemu_real_host_page_size);
+memory_region_set_size(mmap_mr, qemu_real_host_page_size);
+memory_region_add_subregion_overlap(r->address_space,
+bar_addr, mr, 0);
+   }
+} else {
+/* This case would happen when guest rescan one PCI device */
+if (bar_addr & ~qemu_real_host_page_mask) {
+/* Recover the size of memory region */
+memory_region_set_size(mr, r->size);
+memory_region_set_size(mmap_mr, r->size);
+} else if (memory_region_is_mapped(mr)) {
+/* Set the priority of memory region to zero */
+memory_region_del_subregion(r->address_space, mr);
+memory_region_add_subregion_overlap(r->address_space,
+bar_addr, mr, 0);
+}
+}
+memory_region_transaction_commit();
+}
+
+/*
   * PCI config space
   */
  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len)
@@ -1139,6 +1198,23 @@ void vfio_pci_write_config(PCIDevice *pdev,
  } else if (was_enabled && !is_enabled) {
  vfio_msix_disable(vdev);
  }
+} else if (ranges_overlap(addr, len, PCI_BASE_ADDRESS_0, 24) ||
+range_covers_byte(addr, len, PCI_COMMAND)) {
+pcibus_t old_addr[PCI_NUM_REGIONS - 1];
+int bar;
+
+for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
+old_addr[bar] = pdev->io_regions[bar].addr;
+}
+
+pci_default_write_config(pdev, addr, val, len);
+
+for (bar = 0; bar < PCI_ROM_SLOT; bar++) {

Re: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO BARs

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 1470913557-4355-1-git-send-email-xyj...@linux.vnet.ibm.com
Subject: [Qemu-devel] [PATCH v2] vfio: Add support for mmapping sub-page MMIO 
BARs
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/1470913557-4355-1-git-send-email-xyj...@linux.vnet.ibm.com -> 
patchew/1470913557-4355-1-git-send-email-xyj...@linux.vnet.ibm.com
Switched to a new branch 'test'
36f327d vfio: Add support for mmapping sub-page MMIO BARs

=== OUTPUT BEGIN ===
Checking PATCH 1/1: vfio: Add support for mmapping sub-page MMIO BARs...
ERROR: code indent should never use tabs
#87: FILE: hw/vfio/pci.c:1101:
+^I}$

total: 1 errors, 0 warnings, 97 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@freelists.org