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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>>     2) it does feel odd to allow "invalid" offset when len == 0 (at least.
>>
>> I'm puzzled: You've given R-b for patch 1 (thanks), where exactly the
>> same reasoning is used, i.e. similarly referring to memcpy() to
>> justify the (new / supposed) behavior.
> 
> I realize it. But I viewed it slightly different as you are adding the 
> check. I think it is a good idea to add the check and ideally it should 
> be after.
> 
> Here you don't seem to add any check and only re-order it. Hence why I 
> treated it differently.

Right, there already was a zero-length check here. Just that zero
length requests still could have an error returned for no reason. So
the "optimization" part that you're talking about above was already
there, but as said, that's secondary to me.

Jan