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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 22.08.14 at 04:29, <aravindp@cisco.com> wrote:
> I have a solution for the create_bounc_frame() issue I described above. 
> Please find below a POC patch that includes pausing and unpausing the domain 
> during the Xen writes to guest memory. I have it on top of the patch that was 
> using CR0.WP to highlight the difference. Please take a look and let me know 
> if this solution is acceptable. 

As Andrew already pointed out, you absolutely need to deal with
page crossing accesses, and I think you also need to deal with
hypervisor accesses extending beyond a page worth of memory
(I'm not sure we have a firmly determined upper bound of how
much memory we may copy in one go).

> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -1168,9 +1168,13 @@ int __init construct_dom0(
>                 COMPAT_L2_PAGETABLE_XEN_SLOTS(d) * sizeof(*l2tab));
>      }
>  
> -    /* Pages that are part of page tables must be read only. */
>      if  ( is_pv_domain(d) )
> +    {
> +        v->arch.pv_vcpu.mfn_access_reset_req = 0;
> +        v->arch.pv_vcpu.mfn_access_reset = INVALID_MFN;
> +        /* Pages that are part of page tables must be read only. */
>          mark_pv_pt_pages_rdonly(d, l4start, vpt_start, nr_pt_pages);

The order of these should be reversed, with a blank line in between,
to have the important thing first.

>              if ( violation && access_w &&
>                   regs->eip >= XEN_VIRT_START && regs->eip <= XEN_VIRT_END )
>              {
> -                unsigned long cr0 = read_cr0();
> -
>                  violation = 0;
> -                if ( cr0 & X86_CR0_WP &&
> -                     guest_l1e_get_flags(gw.l1e) & _PAGE_RW )
> +                if ( guest_l1e_get_flags(gw.l1e) & _PAGE_RW )
>                  {
> -                    cr0 &= ~X86_CR0_WP;
> -                    write_cr0(cr0);
> -                    v->arch.pv_vcpu.need_cr0_wp_set = 1;
> +                    domain_pause_nosync(d);

I don't think a "nosync" pause is enough here, as that leaves a
window for the guest to write to the page. Since the sync version
may take some time to complete it may become difficult for you to
actually handle this in an acceptable way.

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -441,6 +441,12 @@ UNLIKELY_START(z, create_bounce_frame_bad_bounce_ip)
>          jmp   asm_domain_crash_synchronous  /* Does not return */
>  __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
>          movq  %rax,UREGS_rip+8(%rsp)
> +        cmpb  $1, VCPU_mfn_access_reset_req(%rbx)
> +        je    2f

Please avoid comparing boolean values against other than zero.