On 12/5/18 9:18 AM, Razvan Cojocaru 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.dun...@citrix.com> > Signed-off-by: Razvan Cojocaru <rcojoc...@bitdefender.com> > Tested-by: Tamas K Lengyel <ta...@tklengyel.com> > > --- > CC: George Dunlap <george.dun...@eu.citrix.com> > CC: Jan Beulich <jbeul...@suse.com> > CC: Andrew Cooper <andrew.coop...@citrix.com> > CC: Wei Liu <wei.l...@citrix.com> > CC: "Roger Pau Monné" <roger....@citrix.com> > > --- > Changes since V10: > - Fixed a double-space in the patch description. > - Fixed a coding style issue for > "if ( !start && end >= p2m->max_mapped_pfn)" (no space before > closing ')'). > - Switched the early return comment back to "/* If the requested > range is out of scope, return doing nothing. */. > - Added Tamas' Tested-by. > --- > xen/arch/x86/mm/p2m.c | 47 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 17 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c > index d145850..539ea16 100644 > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -1002,30 +1002,43 @@ 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; > struct domain *d = p2m->domain; > + const unsigned long host_max_pfn = p2m_get_hostp2m(d)->max_mapped_pfn; > int rc = 0; > > - if ( unlikely(end > p2m->max_mapped_pfn) ) > - { > - if ( !gfn ) > - { > - p2m->change_entry_type_global(p2m, ot, nt); > - gfn = end; > - } > - end = p2m->max_mapped_pfn + 1; > - } > - if ( gfn < end ) > - rc = p2m->change_entry_type_range(p2m, ot, nt, gfn, end - 1); > + --end;
I don't like the idea of modifying arguments, which is why I duplicated them in the first place. What about calling the argument end_exclusive, and having a local variable, 'end', which we set to be end_exclusive-1? > + if ( start >= host_max_pfn ) > + printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to > max_mapped_pfn\n", > + d->domain_id); This doesn't seem to be the right condition for this warning.. Surely this warning would go better under the next conditional, where we actually do the clipping? > + /* Always clip the rangeset down to the host p2m. */ > + if ( unlikely(end > host_max_pfn) ) > + end = host_max_pfn; So what about modifying this comment thus: "Always clip the rangeset down to the host p2m. This is probably not the right behavior. This should be revisited later, but for now post a warning." Thanks, -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel