Re: [PATCH V2 12/23] xen/ioreq: Remove "hvm" prefixes from involved function names

[Oleksandr <olekstysh@gmail.com>]

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()


>
>> 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?


> below here.
>
> Jan
>
>> hvm_update_ioreq_evtchn           -> ioreq_update_evtchn
>> hvm_ioreq_server_add_vcpu         -> ioreq_server_add_vcpu
>> hvm_ioreq_server_remove_vcpu      -> ioreq_server_remove_vcpu
>> hvm_ioreq_server_remove_all_vcpus -> ioreq_server_remove_all_vcpus
>> hvm_ioreq_server_alloc_pages      -> ioreq_server_alloc_pages
>> hvm_ioreq_server_free_pages       -> ioreq_server_free_pages
>> hvm_ioreq_server_free_rangesets   -> ioreq_server_free_rangesets
>> hvm_ioreq_server_alloc_rangesets  -> ioreq_server_alloc_rangesets
>> hvm_ioreq_server_enable           -> ioreq_server_enable
>> hvm_ioreq_server_disable          -> ioreq_server_disable
>> hvm_ioreq_server_init             -> ioreq_server_init
>> hvm_ioreq_server_deinit           -> ioreq_server_deinit
>> hvm_send_buffered_ioreq           -> ioreq_send_buffered
>>
>> hvm_wait_for_io                   -> wait_for_io
>>
>> 4. Local (existing) in x86 ioreq.c:
>> Everything related to legacy interface (hvm_alloc_legacy_ioreq_gfn, etc)
>> are going
>> to remain as is.
>>
>>
>>
-- 
Regards,

Oleksandr Tyshchenko