Re: [PATCH 2/2] almost fully ignore zero-size flush requests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2024 10:34, Julien Grall wrote:
> Hi,
> 
> On 20/02/2024 12:25, Jan Beulich wrote:
>> On 20.02.2024 12:52, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 20/02/2024 08:26, Jan Beulich wrote:
>>>> On 19.02.2024 23:22, Julien Grall wrote:
>>>>> Title: I would add 'gnttab:' to clarify which subsystem you are modifying.
>>>>
>>>> That's how I actually have it here; it's not clear to me why I lost the
>>>> prefix when sending.
>>>>
>>>>> On 05/02/2024 11:03, Jan Beulich wrote:
>>>>>> Along the line with observations in the context of XSA-448, besides
>>>>>> "op" no field is relevant when the range to be flushed is empty, much
>>>>>> like e.g. the pointers passed to memcpy() are irrelevant (and would
>>>>>> never be "validated") when the passed length is zero. Split the existing
>>>>>> condition validating "op", "offset", and "length", leaving only the "op"
>>>>>> part ahead of the check for length being zero (or no flushing to be
>>>>>> performed).
>>>>>
>>>>> I am probably missing something here. I understand the theory behind
>>>>> reducing the number of checks when len == 0. But an OS cannot rely on it:
>>>>>      1) older hypervisor would still return an error if the check doesn't
>>>>> pass)
>>>>
>>>> Right, but that's no reason to keep the bogus earlier behavior.
>>>
>>> Hmmm... I am not sure why you say the behavior is bogus. From the commit
>>> message, it seems this is just an optimization that have side effect
>>> (ignoring the other fields).
>>
>> I don't view this as primarily an optimization; I'm in particular after
>> not raising errors for cases where there is no error to be raised.
>> Hence the comparison to memcpy(), which you can pass "bogus" pointers
>> so long as you pass zero size.
> 
> The part I am missing is why this approach is better than what we have. 
> So far what you described is just a matter of taste.
> 
> To give a concrete example, if tomorrow a contributor decides to send a 
> patch undoing what you did (IOW enforcing the check for zero-length or 
> replace | with two branches), then on what grounds I will be able to 
> refuse their patch?

On the grounds of the argument I gave before: Consistency with other
more or less similar operations, where length 0 simply means "no-op",
up to and including "no errors from arguments specifying the address(es)
to operate on".

Jan