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

["Aravindh Puthiyaparambil (aravindp)" <aravindp@xxxxxxxxx>]

>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.

OK, I will replace the check introduced with the !shadow_mode_refcounts() one.

>> >> +            /*
>> >> +             * 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.

OK, I will replace the %rip check with !guest_mode().

Thanks,
Aravindh


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