Re: [QEMU][PATCH v4 04/10] xen-hvm: reorganize xen-hvm and move common function to xen-hvm-common

[Vikram Garhwal <vikram.garhwal@amd.com>]

Hi Stefano,

On 1/25/23 1:55 PM, Stefano Stabellini wrote:
> On Wed, 25 Jan 2023, Vikram Garhwal wrote:
>> From: Stefano Stabellini <stefano.stabellini@amd.com>
>>
>> This patch does following:
>> 1. creates arch_handle_ioreq() and arch_xen_set_memory(). This is done in
>>      preparation for moving most of xen-hvm code to an arch-neutral location,
>>      move the x86-specific portion of xen_set_memory to arch_xen_set_memory.
>>      Also, move handle_vmport_ioreq to arch_handle_ioreq.
>>
>> 2. Pure code movement: move common functions to hw/xen/xen-hvm-common.c
>>      Extract common functionalities from hw/i386/xen/xen-hvm.c and move them to
>>      hw/xen/xen-hvm-common.c. These common functions are useful for creating
>>      an IOREQ server.
>>
>>      xen_hvm_init_pc() contains the architecture independent code for creating
>>      and mapping a IOREQ server, connecting memory and IO listeners, initializing
>>      a xen bus and registering backends. Moved this common xen code to a new
>>      function xen_register_ioreq() which can be used by both x86 and ARM machines.
>>
>>      Following functions are moved to hw/xen/xen-hvm-common.c:
>>          xen_vcpu_eport(), xen_vcpu_ioreq(), xen_ram_alloc(), xen_set_memory(),
>>          xen_region_add(), xen_region_del(), xen_io_add(), xen_io_del(),
>>          xen_device_realize(), xen_device_unrealize(),
>>          cpu_get_ioreq_from_shared_memory(), cpu_get_ioreq(), do_inp(),
>>          do_outp(), rw_phys_req_item(), read_phys_req_item(),
>>          write_phys_req_item(), cpu_ioreq_pio(), cpu_ioreq_move(),
>>          cpu_ioreq_config(), handle_ioreq(), handle_buffered_iopage(),
>>          handle_buffered_io(), cpu_handle_ioreq(), xen_main_loop_prepare(),
>>          xen_hvm_change_state_handler(), xen_exit_notifier(),
>>          xen_map_ioreq_server(), destroy_hvm_domain() and
>>          xen_shutdown_fatal_error()
>>
>> 3. Removed static type from below functions:
>>      1. xen_region_add()
>>      2. xen_region_del()
>>      3. xen_io_add()
>>      4. xen_io_del()
>>      5. xen_device_realize()
>>      6. xen_device_unrealize()
>>      7. xen_hvm_change_state_handler()
>>      8. cpu_ioreq_pio()
>>      9. xen_exit_notifier()
>>
>> 4. Replace TARGET_PAGE_SIZE with XC_PAGE_SIZE to match the page side with Xen.
>>
>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> One comment below
>
> [...]
>
>> +void xen_exit_notifier(Notifier *n, void *data)
>> +{
>> +    XenIOState *state = container_of(n, XenIOState, exit);
>> +
>> +    xen_destroy_ioreq_server(xen_domid, state->ioservid);
> In the original code we had:
>
> -    if (state->fres != NULL) {
> -        xenforeignmemory_unmap_resource(xen_fmem, state->fres);
> -    }
>
> Should we add it here?
>
>
> I went through the manual process of comparing all the code additions
> and deletions (not fun!) and everything checks out except for this.
thanks for catching this. There were two recent commits in upstream and 
i missed those. I rechecked and there are actually three other lines 
which needs update. I will address it in v5.
>
>> +    xenevtchn_close(state->xce_handle);
>> +    xs_daemon_close(state->xenstore);
>> +}