On 23.11.20 17:54, Paul Durrant wrote:
Hi Paul
-----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.
ok, will rename here ...
2. Global (new):
arch_io_completion
and here arch_vcpu_ioreq_completion() (without handle in the middle).
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.
Got it.
Thank you
--
Regards,
Oleksandr Tyshchenko
