On 07/03/2023 09:47, Cédric Le Goater wrote:
> On 3/7/23 10: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 think introducing a new trace event 'trace_vfio_listener_region_skip'
> to replace 'trace_vfio_listener_region_add_skip' above should be enough.
>
OK, I'll introduce a predecessor patch to change the name.
> Thanks,
>
> C.
>
>>
>> But other than that:
>>
>> Reviewed-by: Avihai Horon <avih...@nvidia.com>
>>
>> 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
>>>
>>
>
