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

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

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

Is this for say an unsigned long that lives across two pages? Off the top of my 
head, I think always allowing writes to the page in question and the next 
followed by reverting to default for both pages at the end of the write should 
take care of this. I would have to walk the page tables to figure out the next 
mfn. Or am I on the wrong track here?

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

Let me try to understand what happens in the non-mem_access case. Say the 
hypervisor is writing to three pages and all of them are not accessible in the 
guest. Which one of the following is true?
1. There is a pagefault for the first page which is resolved. The write is then 
retried which causes a fault for the second page which is resolved. Then the 
write is retried starting from the second page and so on for the third page too.
2. Or does the write get retried starting from the first page each time the 
page fault is resolved?

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

Will do.

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

Are you worried about performance or is there some other issue? 

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

OK, I will do:
+        cmpb  $0, VCPU_mfn_access_reset_req(%rbx)
+        jne    2f

Thanks,
Aravindh


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