Re: [Xen-devel] [PATCH V9 6/7] p2m: Always use hostp2m when clipping rangesets

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.11.18 at 12:40, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> The logdirty rangesets of the altp2ms need to be kept in sync with the
> hostp2m.  This means when iterating through the altp2ms, we need to
> use the host p2m to clip the rangeset, not the indiviual altp2m's
> value.
> 
> This change also:
> 
> - Documents that the end is non-inclusive
> 
> - Calculates an "inclusive" value for the end once, rather than
>   open-coding the modification, and (worse) back-modifying updates so
>   that the calculation ends up correct
> 
> - Clarifies the logic deciding whether to call
>   change_entry_type_global() or change_entry_type_range()
> 
> - Handles the case where start >= hostp2m->max_mapped_pfn
> 
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> 
> ---
> CC: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: "Roger Pau Monné" <roger.pau@xxxxxxxxxx>
> 
> ---
> RFC: Wasn't sure what the best thing was to do if start >=
> host_max_pfn.  We silently clip the logdirty rangeset to
> max_mapped_pfn, and the chosen behavior seems consistent with that.
> But it seems like such a request would almost certainly be a bug
> somewhere that people might like to find out about.

Warning about this might be worthwhile as a first step, but I
don't think we should converted this to some form of error
right away, unless the "almost" could be dropped from your
explanation.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1002,32 +1002,44 @@ int p2m_change_type_one(struct domain *d, unsigned 
> long gfn_l,
>      return rc;
>  }
>  
> -/* Modify the p2m type of a range of gfns from ot to nt. */
> +/* Modify the p2m type of [start, end) from ot to nt. */
>  static void change_type_range(struct p2m_domain *p2m,
>                                unsigned long start, unsigned long end,
>                                p2m_type_t ot, p2m_type_t nt)
>  {
> -    unsigned long gfn = start;
> +    unsigned long rangeset_start, rangeset_end;
>      struct domain *d = p2m->domain;
> +    unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn;
>      int rc = 0;
>  
> +    rangeset_start = start;

You never change this value - do you really want to introduce
this redundant variable? I also don't see why you couldn't simply
re-purpose "end".

> +    rangeset_end   = end - 1;
> +
> +    /* Always clip the rangeset down to the host p2m */
> +    if ( unlikely(rangeset_end > host_max_pfn) )
> +        rangeset_end = host_max_pfn;
> +
> +    /* If the requested range is out of scope, return doing nothing */
> +    if ( rangeset_start > rangeset_end )
> +        return;

The original idea behind not using such an early return was to
make sure the subsequent rangeset manipulations would not
be skipped. If you're convinced this is unnecessary, could you
please reason about this in the description? After all the
logdirty rangeset also needs to cope with subsequently
increasing ->max_mapped_pfn.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel