Re: [PATCH V1 01/16] x86/ioreq: Prepare IOREQ feature for making it common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.09.2020 14:22, Oleksandr wrote:
> On 14.09.20 16:52, Jan Beulich wrote:
>> On 10.09.2020 22:21, Oleksandr Tyshchenko wrote:
>>> --- a/xen/arch/x86/hvm/ioreq.c
>>> +++ b/xen/arch/x86/hvm/ioreq.c
>>> @@ -170,6 +170,29 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, 
>>> ioreq_t *p)
>>>       return true;
>>>   }
>>>   
>>> +bool arch_handle_hvm_io_completion(enum hvm_io_completion io_completion)
>> Do we need "handle" in here? Without it, I'd also not have to ask about
>> moving hvm further to the front of the name...
> For me without "handle" it will sound a bit confusing because of the 
> enum type which
> has a similar name but without "arch" prefix:
> bool arch_hvm_io_completion(enum hvm_io_completion io_completion)

Every function handles something; there's no point including
"handle" in every name. Or else we'd have handle_memset()
instead of just memset(), for example.

> Shall I then move "hvm" to the front of the name here?

As per comments on later patches, I think we want to consider dropping
hvm prefixes or infixes altogether from the functions involved here.

>>> @@ -1215,8 +1233,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>>>       struct hvm_ioreq_server *s;
>>>       unsigned int id;
>>>   
>>> -    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
>>> -        return;
>>> +    arch_hvm_ioreq_destroy(d);
>> So the call to relocate_portio_handler() simply goes away. No
>> replacement, no explanation?
> As I understand from the review comment on that for the RFC patch, there 
> is no
> a lot of point of keeping this. Indeed, looking at the code I got the 
> same opinion.
> I should have added an explanation in the commit description at least.
> Or shall I return the call back?

If there's a reason to drop it (which I can't see, but I also
don't recall seeing the discussion you're mentioning), then doing
so should be a separate patch with suitable reasoning. In the
patch here you definitely should only transform what's already
there.

Jan