Re: [PATCH RFC 1/4] x86/mm: Shadow and p2m changes for PV mem_access

[Tim Deegan <tim@xen.org>]

At 22:20 +0000 on 30 Apr (1398892838), Aravindh Puthiyaparambil (aravindp) wrot> >>>> @@ -2443,7 +2443,8 @@ int sh_remove_all_mappings(struct vcpu *v,
> >>>mfn_t gmfn)
> >>>>          if ( !(shadow_mode_external(v->domain)
> >>>>                 && (page->count_info & PGC_count_mask) <= 3
> >>>>                 && ((page->u.inuse.type_info & PGT_count_mask)
> >>>> -                   == !!is_xen_heap_page(page))) )
> >>>> +                       == !!is_xen_heap_page(page)))
> >>>> +                    && !(shadow_mode_mem_access(v->domain)) )
> >>>
> >>>You're breaking indentation, there are pointless parentheses again,
> >>>but most importantly - why?
> >>
> >> Sorry, I meant to ask a question about this in my patch message and
> >> mark this as a workaround. I am seeing the message on all
> >> sh_remove_all_mappings() and I was not able to figure out why this was
> >> happening. I just added this as a work around. I was hoping you or Tim
> >would shed more light on this.
> >
> >I'm afraid that without detail on which pages the triggers on, and you at least
> >having spent some time finding out where the stray/extra references may be
> >coming from it's going to be hard to help.
> 
> Here are some of the prints I saw. I typically saw it for every set_entry() call.
> 
> (XEN) sh error: sh_remove_all_mappings(): can't find all mappings of mfn 33960: c=8000000000000002 t=7400000000000000 <most common>
>  (XEN) sh error: sh_remove_all_mappings(): can't find all mappings of mfn 134b8: c=8000000000000038 t=7400000000000001

This is because you are not in shadow_mode_refcounts() (i.e. the
page's refcount and typecount are based on the _guest_ pagetables
rather than the _shadow_ ones).  That means that sh_remove_all_mappings()
can't use the refcounts to figure out when it's removed the last
shadow mapping.

That also means that sh_remove_all_mappings() will have had to search
every sl1e in the system, which is expensive.  If there will be a lot
of these updates, you might consider batching them and using
shadow_blow_tables() after each batch instead.

Tim.