[PATCH v2 0/2] vfio: Some fixes about vfio-pci MMIO RAM mapping

[PATCH v2 0/2] vfio: Some fixes about vfio-pci MMIO RAM mapping

[Kunkun Jiang]

This series include patches as below:

Patch 1:
- vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration

Patch 2:
- Added a trace point to informe users when a MMIO RAM ection less than PAGE_SIZE

History:

v1 -> v2:
- Add iterate sub-page BARs in vfio_pci_load_config and try to update them [Alex Williamson]

Kunkun Jiang (2):
  vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration
  vfio/common: Add trace point when a MMIO RAM section less than
    PAGE_SIZE

 hw/vfio/common.c |  7 +++++++
 hw/vfio/pci.c    | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.23.0

[PATCH v2 1/2] vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration

[Kunkun Jiang]

We expand MemoryRegions of vfio-pci sub-page MMIO BARs to
vfio_pci_write_config to improve IO performance.
The MemoryRegions of destination VM will not be expanded
successful in live migration, because their addresses have
been updated in vmstate_load_state (vfio_pci_load_config).

So iterate BARs in vfio_pci_write_config and try to update
sub-page BARs.

Fixes: c5e2fb3ce4d (vfio: Add save and load functions for VFIO PCI devices)
Reported-by: Nianyao Tang <tangnianyao@huawei.com>
Reported-by: Qixin Gan <ganqixin@huawei.com>
Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 hw/vfio/pci.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e1ea1d8a23..43c7e93153 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2453,7 +2453,12 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
 {
     VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
     PCIDevice *pdev = &vdev->pdev;
-    int ret;
+    pcibus_t old_addr[PCI_NUM_REGIONS - 1];
+    int bar, ret;
+
+    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
+        old_addr[bar] = pdev->io_regions[bar].addr;
+    }
 
     ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
     if (ret) {
@@ -2463,6 +2468,14 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
     vfio_pci_write_config(pdev, PCI_COMMAND,
                           pci_get_word(pdev->config + PCI_COMMAND), 2);
 
+    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
+        if (old_addr[bar] != pdev->io_regions[bar].addr &&
+            vdev->bars[bar].region.size > 0 &&
+            vdev->bars[bar].region.size < qemu_real_host_page_size) {
+            vfio_sub_page_bar_update_mapping(pdev, bar);
+        }
+    }
+
     if (msi_enabled(pdev)) {
         vfio_msi_enable(vdev);
     } else if (msix_enabled(pdev)) {
-- 
2.23.0

[PATCH v2 2/2] vfio/common: Add trace point when a MMIO RAM section less than PAGE_SIZE

[Kunkun Jiang]

The MSI-X structures of some devices and other non-MSI-X structures
are in the same BAR. They may share one host page, especially in the
case of large page granularity, such as 64K.

For example, MSIX-Table size of 82599 NIC is 0x30 and the offset in
Bar 3(size 64KB) is 0x0. If host page size is 64KB.
vfio_listener_region_add() will be called to map the remaining range
(0x30-0xffff). And it will return early at
'int128_ge((int128_make64(iova), llend))' and hasn't any message.
Let's add a trace point to informed users like commit 5c08600547c0
("vfio: Use a trace point when a RAM section cannot be DMA mapped")
did.

Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
---
 hw/vfio/common.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8728d4d5c2..2fc6213c0f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -892,6 +892,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
     llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask));
 
     if (int128_ge(int128_make64(iova), llend)) {
+        if (memory_region_is_ram_device(section->mr)) {
+            trace_vfio_listener_region_add_no_dma_map(
+                memory_region_name(section->mr),
+                section->offset_within_address_space,
+                int128_getlo(section->size),
+                qemu_real_host_page_size);
+        }
         return;
     }
     end = int128_get64(int128_sub(llend, int128_one()));
-- 
2.23.0

Re: [PATCH v2 0/2] vfio: Some fixes about vfio-pci MMIO RAM mapping

[Kunkun Jiang]

Kindly ping,

Hi all,

Will this patch be picked up soon, or is there any other advice?

Thanks,
Kunkun Jiang

On 2021/9/14 9:53, Kunkun Jiang wrote:
> This series include patches as below:
>
> Patch 1:
> - vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration
>
> Patch 2:
> - Added a trace point to informe users when a MMIO RAM ection less than PAGE_SIZE
>
> History:
>
> v1 -> v2:
> - Add iterate sub-page BARs in vfio_pci_load_config and try to update them [Alex Williamson]
>
> Kunkun Jiang (2):
>    vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration
>    vfio/common: Add trace point when a MMIO RAM section less than
>      PAGE_SIZE
>
>   hw/vfio/common.c |  7 +++++++
>   hw/vfio/pci.c    | 15 ++++++++++++++-
>   2 files changed, 21 insertions(+), 1 deletion(-)
>

Re: [PATCH v2 0/2] vfio: Some fixes about vfio-pci MMIO RAM mapping

[Alex Williamson]

On Fri, 8 Oct 2021 09:27:04 +0800
Kunkun Jiang <jiangkunkun@huawei.com> wrote:

> Kindly ping,
> 
> Hi all,
> 
> Will this patch be picked up soon, or is there any other advice?

More reviews are always appreciated.  Requesting reviews by your
colleagues and/or exchanging reviews with other contributors are good
ways to offload maintainers and provide more confidence in the patch
series.  Thanks,

Alex


> On 2021/9/14 9:53, Kunkun Jiang wrote:
> > This series include patches as below:
> >
> > Patch 1:
> > - vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration
> >
> > Patch 2:
> > - Added a trace point to informe users when a MMIO RAM ection less than PAGE_SIZE
> >
> > History:
> >
> > v1 -> v2:
> > - Add iterate sub-page BARs in vfio_pci_load_config and try to update them [Alex Williamson]
> >
> > Kunkun Jiang (2):
> >    vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration
> >    vfio/common: Add trace point when a MMIO RAM section less than
> >      PAGE_SIZE
> >
> >   hw/vfio/common.c |  7 +++++++
> >   hw/vfio/pci.c    | 15 ++++++++++++++-
> >   2 files changed, 21 insertions(+), 1 deletion(-)
> >  
> 

Re: [PATCH v2 1/2] vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration

[Eric Auger]

Hi Kunkun,

On 9/14/21 3:53 AM, Kunkun Jiang wrote:
> We expand MemoryRegions of vfio-pci sub-page MMIO BARs to
> vfio_pci_write_config to improve IO performance.
s/to vfio_pci_write_config/ in vfio_pci_write_config()
> The MemoryRegions of destination VM will not be expanded
> successful in live migration, because their addresses have
s/will not be expanded successful in live migration/are not expanded
anymore after live migration (?) Is that the correct symptom?
> been updated in vmstate_load_state (vfio_pci_load_config).
>
> So iterate BARs in vfio_pci_write_config and try to update
> sub-page BARs.
>
> Fixes: c5e2fb3ce4d (vfio: Add save and load functions for VFIO PCI devices)
is it an actual fix or an optimization?
> Reported-by: Nianyao Tang <tangnianyao@huawei.com>
> Reported-by: Qixin Gan <ganqixin@huawei.com>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  hw/vfio/pci.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index e1ea1d8a23..43c7e93153 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2453,7 +2453,12 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>  {
>      VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>      PCIDevice *pdev = &vdev->pdev;
> -    int ret;
> +    pcibus_t old_addr[PCI_NUM_REGIONS - 1];
> +    int bar, ret;
> +
> +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
> +        old_addr[bar] = pdev->io_regions[bar].addr;
> +    }
what are those values before the vmstate_load_state ie. can't you do the
vfio_sub_page_bar_update_mapping() unconditionnaly on old_addr[bar] !=
pdev->io_regions[bar].addr
>  
>      ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
>      if (ret) {
> @@ -2463,6 +2468,14 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>      vfio_pci_write_config(pdev, PCI_COMMAND,
>                            pci_get_word(pdev->config + PCI_COMMAND), 2);
>  
> +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
> +        if (old_addr[bar] != pdev->io_regions[bar].addr &&
> +            vdev->bars[bar].region.size > 0 &&
> +            vdev->bars[bar].region.size < qemu_real_host_page_size) {
> +            vfio_sub_page_bar_update_mapping(pdev, bar);
> +        }
> +    }
> +
>      if (msi_enabled(pdev)) {
>          vfio_msi_enable(vdev);
>      } else if (msix_enabled(pdev)) {
Thanks

Eric

Re: [PATCH v2 2/2] vfio/common: Add trace point when a MMIO RAM section less than PAGE_SIZE

[Eric Auger]

Hi Kunkun,

On 9/14/21 3:53 AM, Kunkun Jiang wrote:
> The MSI-X structures of some devices and other non-MSI-X structures
> are in the same BAR. They may share one host page, especially in the
may be in the same bar?
> case of large page granularity, such as 64K.
>
> For example, MSIX-Table size of 82599 NIC is 0x30 and the offset in
> Bar 3(size 64KB) is 0x0. If host page size is 64KB.
remove the '.'
In that case wouldn't the symptom be the same with a 4kB page?
> vfio_listener_region_add() will be called to map the remaining range
> (0x30-0xffff). And it will return early at
> 'int128_ge((int128_make64(iova), llend))' and hasn't any message.
s/and hasn't any message /without any message
> Let's add a trace point to informed users like commit 5c08600547c0
s/informed/inform
> ("vfio: Use a trace point when a RAM section cannot be DMA mapped")
> did.
>
> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  hw/vfio/common.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 8728d4d5c2..2fc6213c0f 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -892,6 +892,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
>      llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask));
>  
>      if (int128_ge(int128_make64(iova), llend)) {
> +        if (memory_region_is_ram_device(section->mr)) {
> +            trace_vfio_listener_region_add_no_dma_map(
> +                memory_region_name(section->mr),
> +                section->offset_within_address_space,
> +                int128_getlo(section->size),
> +                qemu_real_host_page_size);
> +        }
>          return;
>      }
>      end = int128_get64(int128_sub(llend, int128_one()));
Thanks

Eric

Re: [PATCH v2 1/2] vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration

[Kunkun Jiang]

Hi Eric,

On 2021/10/22 0:15, Eric Auger wrote:
> Hi Kunkun,
>
> On 9/14/21 3:53 AM, Kunkun Jiang wrote:
>> We expand MemoryRegions of vfio-pci sub-page MMIO BARs to
>> vfio_pci_write_config to improve IO performance.
> s/to vfio_pci_write_config/ in vfio_pci_write_config()
Thank you for your review. I will correct it in v3.
>> The MemoryRegions of destination VM will not be expanded
>> successful in live migration, because their addresses have
> s/will not be expanded successful in live migration/are not expanded
> anymore after live migration (?) Is that the correct symptom?
You are right. They are not expanded anymore after live migration,
not expanded unsuccessfully. I used the wrong words.
>> been updated in vmstate_load_state (vfio_pci_load_config).
>>
>> So iterate BARs in vfio_pci_write_config and try to update
>> sub-page BARs.
>>
>> Fixes: c5e2fb3ce4d (vfio: Add save and load functions for VFIO PCI devices)
> is it an actual fix or an optimization?
I recently realized that this is actually an optimization.

The VF driver in VM use the assembly language instructions,
which can operate two registers simultaneously, like stp, ldp.
These instructions would result in non-ISV data abort, which
cannot be handled well at the moment.

If the driver doesn't use such instructions,  not expanding
only affects performance.

I will add these to the commit message in the next version.
>> Reported-by: Nianyao Tang <tangnianyao@huawei.com>
>> Reported-by: Qixin Gan <ganqixin@huawei.com>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   hw/vfio/pci.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index e1ea1d8a23..43c7e93153 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2453,7 +2453,12 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>>   {
>>       VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>>       PCIDevice *pdev = &vdev->pdev;
>> -    int ret;
>> +    pcibus_t old_addr[PCI_NUM_REGIONS - 1];
>> +    int bar, ret;
>> +
>> +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
>> +        old_addr[bar] = pdev->io_regions[bar].addr;
>> +    }
> what are those values before the vmstate_load_state ie. can't you do the
Are you referring to pdev->io_regions[bar].addr ? All of the bars addr is
PCI_BAR_UNMAPPED (~(pcibus_t)0) before the vmstate_load_state.
> vfio_sub_page_bar_update_mapping() unconditionnaly on old_addr[bar] !=
> pdev->io_regions[bar].addr
The size of Bar is a power of 2. The Bar, whose size is greater than host
page size, doesn't need to be expanded.

Can you explain more? May be I misunderstood you.

Thanks,
Kunkun Jiang
>>   
>>       ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
>>       if (ret) {
>> @@ -2463,6 +2468,14 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>>       vfio_pci_write_config(pdev, PCI_COMMAND,
>>                             pci_get_word(pdev->config + PCI_COMMAND), 2);
>>   
>> +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
>> +        if (old_addr[bar] != pdev->io_regions[bar].addr &&
>> +            vdev->bars[bar].region.size > 0 &&
>> +            vdev->bars[bar].region.size < qemu_real_host_page_size) {
>> +            vfio_sub_page_bar_update_mapping(pdev, bar);
>> +        }
>> +    }
>> +
>>       if (msi_enabled(pdev)) {
>>           vfio_msi_enable(vdev);
>>       } else if (msix_enabled(pdev)) {
> Thanks
>
> Eric
>
> .

Re: [PATCH v2 2/2] vfio/common: Add trace point when a MMIO RAM section less than PAGE_SIZE

[Kunkun Jiang]

Hi Eric,

On 2021/10/22 1:02, Eric Auger wrote:
> Hi Kunkun,
>
> On 9/14/21 3:53 AM, Kunkun Jiang wrote:
>> The MSI-X structures of some devices and other non-MSI-X structures
>> are in the same BAR. They may share one host page, especially in the
> may be in the same bar?
You are right. So embarrassing.😅
>> case of large page granularity, such as 64K.
>>
>> For example, MSIX-Table size of 82599 NIC is 0x30 and the offset in
>> Bar 3(size 64KB) is 0x0. If host page size is 64KB.
> remove the '.'
s/./, ?
> In that case wouldn't the symptom be the same with a 4kB page?
Host page size is different, iova is different.
> iova = REAL_HOST_PAGE_ALIGN(section->offset_within_address_space);

If host page size is 4KB, the iova will smaller than llend.
If host page size is 64KB, the iova will be equal to llend.

>> vfio_listener_region_add() will be called to map the remaining range
>> (0x30-0xffff). And it will return early at
>> 'int128_ge((int128_make64(iova), llend))' and hasn't any message.
> s/and hasn't any message /without any message

Ok, I will correct this in next version.

>> Let's add a trace point to informed users like commit 5c08600547c0
> s/informed/inform
Same for this.
>> ("vfio: Use a trace point when a RAM section cannot be DMA mapped")
>> did.
>>
>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>> ---
>>   hw/vfio/common.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 8728d4d5c2..2fc6213c0f 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -892,6 +892,13 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>       llend = int128_and(llend, int128_exts64(qemu_real_host_page_mask));
>>   
>>       if (int128_ge(int128_make64(iova), llend)) {
>> +        if (memory_region_is_ram_device(section->mr)) {
>> +            trace_vfio_listener_region_add_no_dma_map(
>> +                memory_region_name(section->mr),
>> +                section->offset_within_address_space,
>> +                int128_getlo(section->size),
>> +                qemu_real_host_page_size);
>> +        }
>>           return;
>>       }
>>       end = int128_get64(int128_sub(llend, int128_one()));
By the way, if this patch is accepted. The following code in the
vfio_listener_region_add may need to be deleted:
>     if (memory_region_is_ram_device(section->mr)) {
>         hwaddr pgmask = (1ULL << ctz64(hostwin->iova_pgsizes)) - 1;
>
>         if ((iova & pgmask) || (int128_get64(llsize) & pgmask)) {
>             trace_vfio_listener_region_add_no_dma_map(
>                 memory_region_name(section->mr),
>                 section->offset_within_address_space,
>                 int128_getlo(section->size),
>                 pgmask + 1);
>             return;
>         }
>     }
What do you think?

Thanks,
Kunkun Jiang
> Thanks
>
> Eric
>
> .

Re: [PATCH v2 1/2] vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration

[Eric Auger]

Hi Kunkun,

On 10/22/21 12:01 PM, Kunkun Jiang wrote:
> Hi Eric,
>
> On 2021/10/22 0:15, Eric Auger wrote:
>> Hi Kunkun,
>>
>> On 9/14/21 3:53 AM, Kunkun Jiang wrote:
>>> We expand MemoryRegions of vfio-pci sub-page MMIO BARs to
>>> vfio_pci_write_config to improve IO performance.
>> s/to vfio_pci_write_config/ in vfio_pci_write_config()
> Thank you for your review. I will correct it in v3.
>>> The MemoryRegions of destination VM will not be expanded
>>> successful in live migration, because their addresses have
>> s/will not be expanded successful in live migration/are not expanded
>> anymore after live migration (?) Is that the correct symptom?
> You are right. They are not expanded anymore after live migration,
> not expanded unsuccessfully. I used the wrong words.
>>> been updated in vmstate_load_state (vfio_pci_load_config).
>>>
>>> So iterate BARs in vfio_pci_write_config and try to update
>>> sub-page BARs.
>>>
>>> Fixes: c5e2fb3ce4d (vfio: Add save and load functions for VFIO PCI
>>> devices)
>> is it an actual fix or an optimization?
> I recently realized that this is actually an optimization.
>
> The VF driver in VM use the assembly language instructions,
> which can operate two registers simultaneously, like stp, ldp.
> These instructions would result in non-ISV data abort, which
> cannot be handled well at the moment.
>
> If the driver doesn't use such instructions,  not expanding
> only affects performance.
>
> I will add these to the commit message in the next version.
>>> Reported-by: Nianyao Tang <tangnianyao@huawei.com>
>>> Reported-by: Qixin Gan <ganqixin@huawei.com>
>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>> ---
>>>   hw/vfio/pci.c | 15 ++++++++++++++-
>>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index e1ea1d8a23..43c7e93153 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2453,7 +2453,12 @@ static int vfio_pci_load_config(VFIODevice
>>> *vbasedev, QEMUFile *f)
>>>   {
>>>       VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice,
>>> vbasedev);
>>>       PCIDevice *pdev = &vdev->pdev;
>>> -    int ret;
>>> +    pcibus_t old_addr[PCI_NUM_REGIONS - 1];
>>> +    int bar, ret;
>>> +
>>> +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
>>> +        old_addr[bar] = pdev->io_regions[bar].addr;
>>> +    }
>> what are those values before the vmstate_load_state ie. can't you do the
> Are you referring to pdev->io_regions[bar].addr ? All of the bars addr is
> PCI_BAR_UNMAPPED (~(pcibus_t)0) before the vmstate_load_state.
OK that was my assumption. What I meant is in that case you always have

old_addr[bar] != pdev->io_regions[bar].addr, right? In the positive this check is not needed and you don't need old_addr at all.
In the original code this was needed since one wanted to call 
vfio_sub_page_bar_update_mapping() only for the bar base address that were changed during the 
vfio_pci_write_config.

Thanks

Eric

>> vfio_sub_page_bar_update_mapping() unconditionnaly on old_addr[bar] !=
>> pdev->io_regions[bar].addr
> The size of Bar is a power of 2. The Bar, whose size is greater than host
> page size, doesn't need to be expanded.
>
> Can you explain more? May be I misunderstood you.
>
> Thanks,
> Kunkun Jiang
>>>         ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
>>>       if (ret) {
>>> @@ -2463,6 +2468,14 @@ static int vfio_pci_load_config(VFIODevice
>>> *vbasedev, QEMUFile *f)
>>>       vfio_pci_write_config(pdev, PCI_COMMAND,
>>>                             pci_get_word(pdev->config +
>>> PCI_COMMAND), 2);
>>>   +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
>>> +        if (old_addr[bar] != pdev->io_regions[bar].addr &&
>>> +            vdev->bars[bar].region.size > 0 &&
>>> +            vdev->bars[bar].region.size < qemu_real_host_page_size) {
>>> +            vfio_sub_page_bar_update_mapping(pdev, bar);
>>> +        }
>>> +    }
>>> +
>>>       if (msi_enabled(pdev)) {
>>>           vfio_msi_enable(vdev);
>>>       } else if (msix_enabled(pdev)) {
>> Thanks
>>
>> Eric
>>
>> .
>

Re: [PATCH v2 1/2] vfio/pci: Fix vfio-pci sub-page MMIO BAR mmaping in live migration

[Kunkun Jiang]

Hi Eric,

On 2021/10/23 22:26, Eric Auger wrote:
> Hi Kunkun,
>
> On 10/22/21 12:01 PM, Kunkun Jiang wrote:
>> Hi Eric,
>>
>> On 2021/10/22 0:15, Eric Auger wrote:
>>> Hi Kunkun,
>>>
>>> On 9/14/21 3:53 AM, Kunkun Jiang wrote:
>>>> We expand MemoryRegions of vfio-pci sub-page MMIO BARs to
>>>> vfio_pci_write_config to improve IO performance.
>>> s/to vfio_pci_write_config/ in vfio_pci_write_config()
>> Thank you for your review. I will correct it in v3.
>>>> The MemoryRegions of destination VM will not be expanded
>>>> successful in live migration, because their addresses have
>>> s/will not be expanded successful in live migration/are not expanded
>>> anymore after live migration (?) Is that the correct symptom?
>> You are right. They are not expanded anymore after live migration,
>> not expanded unsuccessfully. I used the wrong words.
>>>> been updated in vmstate_load_state (vfio_pci_load_config).
>>>>
>>>> So iterate BARs in vfio_pci_write_config and try to update
>>>> sub-page BARs.
>>>>
>>>> Fixes: c5e2fb3ce4d (vfio: Add save and load functions for VFIO PCI
>>>> devices)
>>> is it an actual fix or an optimization?
>> I recently realized that this is actually an optimization.
>>
>> The VF driver in VM use the assembly language instructions,
>> which can operate two registers simultaneously, like stp, ldp.
>> These instructions would result in non-ISV data abort, which
>> cannot be handled well at the moment.
>>
>> If the driver doesn't use such instructions,  not expanding
>> only affects performance.
>>
>> I will add these to the commit message in the next version.
>>>> Reported-by: Nianyao Tang <tangnianyao@huawei.com>
>>>> Reported-by: Qixin Gan <ganqixin@huawei.com>
>>>> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com>
>>>> ---
>>>>    hw/vfio/pci.c | 15 ++++++++++++++-
>>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index e1ea1d8a23..43c7e93153 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -2453,7 +2453,12 @@ static int vfio_pci_load_config(VFIODevice
>>>> *vbasedev, QEMUFile *f)
>>>>    {
>>>>        VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice,
>>>> vbasedev);
>>>>        PCIDevice *pdev = &vdev->pdev;
>>>> -    int ret;
>>>> +    pcibus_t old_addr[PCI_NUM_REGIONS - 1];
>>>> +    int bar, ret;
>>>> +
>>>> +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
>>>> +        old_addr[bar] = pdev->io_regions[bar].addr;
>>>> +    }
>>> what are those values before the vmstate_load_state ie. can't you do the
>> Are you referring to pdev->io_regions[bar].addr ? All of the bars addr is
>> PCI_BAR_UNMAPPED (~(pcibus_t)0) before the vmstate_load_state.
> OK that was my assumption. What I meant is in that case you always have
>
> old_addr[bar] != pdev->io_regions[bar].addr, right? In the positive this check is not needed and you don't need old_addr at all.
> In the original code this was needed since one wanted to call
> vfio_sub_page_bar_update_mapping() only for the bar base address that were changed during the
> vfio_pci_write_config.
As far as I know, there is at least one case. If the VF driver is not loaded
(insmod xxx.ko) in the VM, we will have old_addr[bar] == 
pdev->io_regions[bar].addr.
The vfio_sub_page_bar_update_mapping() will be called when 0 < bar size 
< host page size.
But vfio_sub_page_bar_update_mapping() will not change anything in this 
case.

Thanks,
Kunkun Jiang
>
> Thanks
>
> Eric
>
>>> vfio_sub_page_bar_update_mapping() unconditionnaly on old_addr[bar] !=
>>> pdev->io_regions[bar].addr
>> The size of Bar is a power of 2. The Bar, whose size is greater than host
>> page size, doesn't need to be expanded.
>>
>> Can you explain more? May be I misunderstood you.
>>
>> Thanks,
>> Kunkun Jiang
>>>>          ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
>>>>        if (ret) {
>>>> @@ -2463,6 +2468,14 @@ static int vfio_pci_load_config(VFIODevice
>>>> *vbasedev, QEMUFile *f)
>>>>        vfio_pci_write_config(pdev, PCI_COMMAND,
>>>>                              pci_get_word(pdev->config +
>>>> PCI_COMMAND), 2);
>>>>    +    for (bar = 0; bar < PCI_ROM_SLOT; bar++) {
>>>> +        if (old_addr[bar] != pdev->io_regions[bar].addr &&
>>>> +            vdev->bars[bar].region.size > 0 &&
>>>> +            vdev->bars[bar].region.size < qemu_real_host_page_size) {
>>>> +            vfio_sub_page_bar_update_mapping(pdev, bar);
>>>> +        }
>>>> +    }
>>>> +
>>>>        if (msi_enabled(pdev)) {
>>>>            vfio_msi_enable(vdev);
>>>>        } else if (msix_enabled(pdev)) {
>>> Thanks
>>>
>>> Eric
>>>
>>> .
> .