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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>    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.

>> In the course of splitting also simplify the moved part of the condition
>> from 3 to 2 conditionals, potentially (depending on the architecture)
>> requiring one less (conditional) branch.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3528,15 +3528,16 @@ static int _cache_flush(const gnttab_cac
>>       void *v;
>>       int ret;
>>   
>> -    if ( (cflush->offset >= PAGE_SIZE) ||
>> -         (cflush->length > PAGE_SIZE) ||
>> -         (cflush->offset + cflush->length > PAGE_SIZE) ||
>> -         (cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN)) )
>> +    if ( cflush->op & ~(GNTTAB_CACHE_INVAL | GNTTAB_CACHE_CLEAN) )
>>           return -EINVAL;
>>   
>>       if ( cflush->length == 0 || cflush->op == 0 )
>>           return !*cur_ref ? 0 : -EILSEQ;
>>   
>> +    if ( (cflush->offset | cflush->length) > PAGE_SIZE ||
> 
> This is confusing. I understand you are trying to force the compiler to 
> optimize. But is it really worth it? After all, the rest of operation 
> will outweight this check (cache flush are quite expensive).

>From purely a performance point of view it may not be worth it. From
code size angle (taken globally) I already view this differently.
Plus I think that we ought to aim at avoiding undesirable patterns,
just because people tend to clone existing code when they can. Thing
is that (as per below) the two of us apparently disagree on what
"undesirable" is in cases like this one.

> We probably should take a more generic decision (and encode in our 
> policy) because you seem to like this pattern and I dislike it :). Not 
> sure what the others think.

Perhaps. If the folding alone was the problem, I'd accept to split (or
even undo) that part. But the earlier aspect you raised also needs
sorting before I can decide whether to adjust or whether to consider
the patch rejected.

Jan