Re: [PATCH] x86/hvm: Clean up track_dirty_vram() calltree

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.07.2020 17:15, Andrew Cooper wrote:
>  * Rename nr to nr_frames.  A plain 'nr' is confusing to follow in the the
>    lower levels.
>  * Use DIV_ROUND_UP() rather than opencoding it in several different ways
>  * The hypercall input is capped at uint32_t, so there is no need for
>    nr_frames to be unsigned long in the lower levels.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

I'd like to note though that ...

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -58,16 +58,16 @@
>  
>  int hap_track_dirty_vram(struct domain *d,
>                           unsigned long begin_pfn,
> -                         unsigned long nr,
> +                         unsigned int nr_frames,
>                           XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
>  {
>      long rc = 0;
>      struct sh_dirty_vram *dirty_vram;
>      uint8_t *dirty_bitmap = NULL;
>  
> -    if ( nr )
> +    if ( nr_frames )
>      {
> -        int size = (nr + BITS_PER_BYTE - 1) / BITS_PER_BYTE;
> +        unsigned int size = DIV_ROUND_UP(nr_frames, BITS_PER_BYTE);

... with the change from long to int this construct will now no
longer be correct for the (admittedly absurd) case of a hypercall
input in the range of [0xfffffff9,0xffffffff]. We now fully
depend on this getting properly rejected at the top level hypercall
handler (which limits to 1Gb worth of tracked space).

Jan