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

[Tim Deegan <tim@xxxxxxx>]

Hi,

Apologies for the very late review - just catching up with this thread.

At 23:34 +0000 on 24 Jul (1406241272), Aravindh Puthiyaparambil (aravindp) 
wrote:
> >> -         * share_xen_page_with_guest(). */
> >> +         * share_xen_page_with_guest().
> >> +         * PV domains that have a mem_access listener, runs in shadow mode
> >> +         * without refcounts.
> >> +         */
> >>          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))) &&
> >> +             !mem_event_check_ring(&v->domain->mem_event->access) )
> >
> >To me this doesn't look to be in sync with the comment, as the new
> >check is being carried out regardless of domain type. Furthermore
> >this continues to have the problem of also hiding issues unrelated
> >to mem-access handling.
> 
> I will add a check for PV domain. This check is wrt to
> refcouting. From what Tim told me PV domains cannot run in that
> mode, so I don't think any issues will be hidden if I add the check
> for PV domain.

I think the right check here is for !shadow_mode_refcounts() -- after
all, this check makes no sense if the refcounts aren't
affected by changes to shadow pagetables. 

> >> +            /*
> >> +             * Do not police writes to guest memory from the Xen 
> >> hypervisor.
> >> +             * This keeps PV mem_access on par with HVM. Turn off CR0.WP
> >here to
> >> +             * allow the write to go through if the guest has marked the 
> >> page as
> >> +             * writable. Turn it back on in the guest access functions
> >> +             * __copy_to_user / __put_user_size() after the write is 
> >> completed.
> >> +             */
> >> +            if ( violation && access_w &&
> >> +                 regs->eip >= XEN_VIRT_START && regs->eip <= XEN_VIRT_END 
> >> )
> >
> >Definitely < instead of <= on the right side. But - is this safe, the more
> >that this again doesn't appear to be sitting in a guest kind specific block?
> >I'd at least expect this to be qualified by a regs->cs and/or
> >guest_mode() check.
> 
> I will add the guest kind check. Should I do it for the whole block i.e add 
> is_pv_domain() in addition to mem_event_check_ring()? That will also address 
> next comment below. I will add the guest_mode() in addition to the above 
> check for policing Xen writes to guest memory.
> 

Sounds good -- I would be inclined to replace the %rip test entirely
with a !guest_mode one, which should be a more reliable check for
whether the access is made by Xen.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel