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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

Title: I would add 'gnttab:' to clarify which subsystem you are modifying.

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)
  2) it does feel odd to allow "invalid" offset when len == 0 (at least.

So to me, it is better to keep those checks early. That said, I agree 
this is a matter of opinion, so I will not Nack it but also I will not 
Ack it.
> 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).
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.
Cheers,

--
Julien Grall