On 07/03/2023 09:13, Avihai Horon wrote:
> On 07/03/2023 4:02, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> The checks are replicated against region_add and region_del
>> and will be soon added in another memory listener dedicated
>> for dirty tracking.
>>
>> Move these into a new helper for avoid duplication.
>>
>> Signed-off-by: Joao Martins <joao.m.mart...@oracle.com>
>> Reviewed-by: Cédric Le Goater <c...@redhat.com>
>> ---
>> hw/vfio/common.c | 52 +++++++++++++++++++-----------------------------
>> 1 file changed, 21 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 99acb998eb14..54b4a4fc7daf 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -933,23 +933,14 @@ static bool
>> vfio_known_safe_misalignment(MemoryRegionSection *section)
>> return true;
>> }
>>
>> -static void vfio_listener_region_add(MemoryListener *listener,
>> - MemoryRegionSection *section)
>> +static bool vfio_listener_valid_section(MemoryRegionSection *section)
>> {
>> - VFIOContainer *container = container_of(listener, VFIOContainer,
>> listener);
>> - hwaddr iova, end;
>> - Int128 llend, llsize;
>> - void *vaddr;
>> - int ret;
>> - VFIOHostDMAWindow *hostwin;
>> - Error *err = NULL;
>> -
>> if (vfio_listener_skipped_section(section)) {
>> trace_vfio_listener_region_add_skip(
>> section->offset_within_address_space,
>> section->offset_within_address_space +
>> int128_get64(int128_sub(section->size, int128_one())));
>
> The original code uses two different traces depending on add or del --
> trace_vfio_listener_region_{add,del}_skip.
> Should we combine the two traces into a single trace? If the distinction is
> important then maybe pass a flag or the caller name to indicate whether it's
> add, del or dirty tracking update?
>
I should say that the way I distinct both of them is because there's a
dma_tracking_update new tracepoint where you can tell it's from. And there's a
region_add/del tracepoints. So despite the name we won't get confused IMHO
> But other than that:
>
> Reviewed-by: Avihai Horon <avih...@nvidia.com>
> Thanks!
> Thanks.
>
>> - return;
>> + return false;
>> }
>>
>> if (unlikely((section->offset_within_address_space &
>> @@ -964,6 +955,24 @@ static void vfio_listener_region_add(MemoryListener
>> *listener,
>> section->offset_within_region,
>> qemu_real_host_page_size());
>> }
>> + return false;
>> + }
>> +
>> + return true;
>> +}
>> +
>> +static void vfio_listener_region_add(MemoryListener *listener,
>> + MemoryRegionSection *section)
>> +{
>> + VFIOContainer *container = container_of(listener, VFIOContainer,
>> listener);
>> + hwaddr iova, end;
>> + Int128 llend, llsize;
>> + void *vaddr;
>> + int ret;
>> + VFIOHostDMAWindow *hostwin;
>> + Error *err = NULL;
>> +
>> + if (!vfio_listener_valid_section(section)) {
>> return;
>> }
>>
>> @@ -1182,26 +1191,7 @@ static void vfio_listener_region_del(MemoryListener
>> *listener,
>> int ret;
>> bool try_unmap = true;
>>
>> - if (vfio_listener_skipped_section(section)) {
>> - trace_vfio_listener_region_del_skip(
>> - section->offset_within_address_space,
>> - section->offset_within_address_space +
>> - int128_get64(int128_sub(section->size, int128_one())));
>> - return;
>> - }
>> -
>> - if (unlikely((section->offset_within_address_space &
>> - ~qemu_real_host_page_mask()) !=
>> - (section->offset_within_region &
>> ~qemu_real_host_page_mask()))) {
>> - if (!vfio_known_safe_misalignment(section)) {
>> - error_report("%s received unaligned region %s iova=0x%"PRIx64
>> - " offset_within_region=0x%"PRIx64
>> - " qemu_real_host_page_size=0x%"PRIxPTR,
>> - __func__, memory_region_name(section->mr),
>> - section->offset_within_address_space,
>> - section->offset_within_region,
>> - qemu_real_host_page_size());
>> - }
>> + if (!vfio_listener_valid_section(section)) {
>> return;
>> }
>>
>> --
>> 2.17.2
>>
