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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.09.2020 14:47, Oleksandr wrote:
> On 21.09.20 15:31, Jan Beulich wrote:
>> 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:
>>>>> @@ -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.
> Sounds reasonable. Please see the comment below 
> relocate_portio_handler() here:
> https://www.mail-archive.com/xen-devel@xxxxxxxxxxxxxxxxxxxx/msg78512.html
> 
> However, I might interpret the request incorrectly.

I'm afraid you do: The way you've coded it the function was a no-op.
But that's because you broke the caller by not bailing from
hvm_destroy_all_ioreq_servers() if relocate_portio_handler() returned
false. IOW you did assume that moving the "return" statement into an
inline function would have an effect on its caller(s). For questions
like this one it also often helps to look at the commit introducing
the construct in question (b3344bb1cae0 in this case): Chances are
that the description helps, albeit I agree there are many cases
(particularly the farther you get into distant past) where it isn't
of much help.

Jan