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

[Vikram Garhwal]

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);
> > +}