> -----Original Message----- > From: Oleksandr <olekst...@gmail.com> > Sent: 23 November 2020 15:48 > To: Jan Beulich <jbeul...@suse.com>; Paul Durrant <p...@xen.org> > Cc: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>; Andrew Cooper > <andrew.coop...@citrix.com>; > Roger Pau Monné <roger....@citrix.com>; Wei Liu <w...@xen.org>; George Dunlap > <george.dun...@citrix.com>; Ian Jackson <i...@xenproject.org>; Julien Grall > <jul...@xen.org>; Stefano > Stabellini <sstabell...@kernel.org>; Jun Nakajima <jun.nakaj...@intel.com>; > Kevin Tian > <kevin.t...@intel.com>; Julien Grall <julien.gr...@arm.com>; > xen-devel@lists.xenproject.org > Subject: Re: [PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved > function names > > > On 23.11.20 16:56, Jan Beulich wrote: > > Hi Jan, Paul > > > On 23.11.2020 15:39, Oleksandr wrote: > >> As it was agreed, below the list of proposed renaming (naming) within > >> current series. > > Thanks for compiling this. A couple of suggestions for consideration: > > > >> 1. Global (existing): > >> hvm_map_mem_type_to_ioreq_server -> ioreq_server_map_mem_type > >> hvm_select_ioreq_server -> ioreq_server_select > >> hvm_send_ioreq -> ioreq_send > >> hvm_ioreq_init -> ioreq_init > > ioreq_domain_init() (or, imo less desirable domain_ioreq_init())? > On Arm (for example) I see two variants are present: > 1. That starts with subsystem: > - tee_domain_init > - iommu_domain_init > > > 2. Where sybsystem in the middle: > - domain_io_init > - domain_vuart_init > - domain_vtimer_init > > If there is no rule, but a matter of taste then I would use > ioreq_domain_init(), > so arch_ioreq_init() wants to be arch_ioreq_domain_init(). > > > > >> hvm_destroy_all_ioreq_servers -> ioreq_server_destroy_all > >> hvm_all_ioreq_servers_add_vcpu -> ioreq_server_add_vcpu_all > >> hvm_all_ioreq_servers_remove_vcpu -> ioreq_server_remove_vcpu_all > >> hvm_broadcast_ioreq -> ioreq_broadcast > >> hvm_create_ioreq_server -> ioreq_server_create > >> hvm_get_ioreq_server_info -> ioreq_server_get_info > >> hvm_map_io_range_to_ioreq_server -> ioreq_server_map_io_range > >> hvm_unmap_io_range_from_ioreq_server -> ioreq_server_unmap_io_range > >> hvm_set_ioreq_server_state -> ioreq_server_set_state > >> hvm_destroy_ioreq_server -> ioreq_server_destroy > >> hvm_get_ioreq_server_frame -> ioreq_server_get_frame > >> hvm_ioreq_needs_completion -> ioreq_needs_completion > >> hvm_mmio_first_byte -> ioreq_mmio_first_byte > >> hvm_mmio_last_byte -> ioreq_mmio_last_byte > >> send_invalidate_req -> ioreq_signal_mapcache_invalidate > >> > >> handle_hvm_io_completion -> handle_io_completion > > For this one I'm not sure what to suggest, but I'm not overly happy > > with the name. > > I also failed to find a better name. Probably ioreq_ or vcpu_ioreq_ > prefix wants to be added here? > > > > > >> hvm_io_pending -> io_pending > > vcpu_ioreq_pending() or vcpu_any_ioreq_pending()? > > I am fine with vcpu_ioreq_pending() >
...in which case vcpu_ioreq_handle_completion() seems like a reasonable choice. > > > > >> 2. Global (new): > >> arch_io_completion > >> arch_ioreq_server_map_pages > >> arch_ioreq_server_unmap_pages > >> arch_ioreq_server_enable > >> arch_ioreq_server_disable > >> arch_ioreq_server_destroy > >> arch_ioreq_server_map_mem_type > >> arch_ioreq_server_destroy_all > >> arch_ioreq_server_get_type_addr > >> arch_ioreq_init > > Assuming this is the arch hook of the similarly named function > > further up, a similar adjustment may then be wanted here. > > Yes. > > > > > >> domain_has_ioreq_server > >> > >> > >> 3. Local (existing) in common ioreq.c: > >> hvm_alloc_ioreq_mfn -> ioreq_alloc_mfn > >> hvm_free_ioreq_mfn -> ioreq_free_mfn > > These two are server functions, so should imo be ioreq_server_...(). > > ok, but ... > > > > However, if they're static (as they're now), no distinguishing > > prefix is strictly necessary, i.e. alloc_mfn() and free_mfn() may > > be fine. The two names may be too short for Paul's taste, though. > > Some similar shortening may be possible for some or all of the ones > > > ... In general I would be fine with any option. However, using the > shortening rule for all > we are going to end up with single-word function names (enable, init, etc). > So I would prefer to leave locals as is (but dropping hvm prefixes of > course and > clarify ioreq_server_alloc_mfn/ioreq_server_free_mfn). > > Paul, Jan what do you think? I prefer ioreq_server_alloc_mfn/ioreq_server_free_mfn. The problem with shortening is that function names become ambiguous within the source base and hence harder to find. Paul
