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

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(+)

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

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
>
> .