Re: [PATCH v4 8/9] xen/arm: Implement hypercall for dirty page tracing

[Jaeyong Yoo <jaeyong.yoo@samsung.com>]

>------- Original Message -------
>Sender : Julien Grall<julien.grall@citrix.com>
>Date : 2013-10-07 22:02 (GMT+09:00)
>Title : Re: [Xen-devel] [PATCH v4 8/9] xen/arm: Implement hypercall for dirty page tracing
>
>On 10/04/2013 05:44 AM, Jaeyong Yoo wrote:
>> Add hypercall (shadow op: enable/disable and clean/peek dirted page bitmap).
>> It consists of two parts: dirty page detecting and saving.
>> For detecting, we setup the guest p2m's leaf PTE read-only and whenever
>> the guest tries to write something, permission fault happens and traps into xen.
>> The permission-faulted GPA should be saved for the toolstack (when it wants to see
>> which pages are dirted). For this purpose, we temporarily save the GPAs into linked
>> list by using 'add_mapped_vaddr' function and when toolstack wants
>> (log_dirty_op function) the GPAs are copied into bitmap and the linnked list is flushed.
>> 
>> Additionally, for supporting parallel migration of domUs, vlpt area should be context
>> switched.
>> 
>> Signed-off-by: Jaeyong Yoo 
>> ---
>
>[..]
>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 4c0fc32..3b78ed2 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1313,6 +1313,8 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>      const char *msg;
>>      int rc, level = -1;
>>      mmio_info_t info;
>> +    int page_fault = ((dabt.dfsc & FSC_MASK) ==
>> +                      (FSC_FLT_PERM + FSC_3D_LEVEL) && dabt.write);
>>  
>>      if ( !check_conditional_instr(regs, hsr) )
>>      {
>> @@ -1327,22 +1329,23 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>      info.gva = READ_SYSREG64(FAR_EL2);
>>  #endif
>>  
>> -    if (dabt.s1ptw)
>> +    if ( dabt.s1ptw && !page_fault )
>
>I think checking !page_fault is nearly everywhere is error-prone when
>this function will be modified.
>
>Can you do something like this?
>
>if ( page_fault )
>  // Your code to handle page fault
>else
>{
>  // handle_mmio
>}
>
>It will avoid && !page_fault.

That looks better.

>>          goto bad_data_abort;
>>  
>>      rc = gva_to_ipa(info.gva, &info.gpa);
>> -    if ( rc == -EFAULT )
>> +    if ( rc == -EFAULT && !page_fault )
>>          goto bad_data_abort;
>>  
>>      /* XXX: Decode the instruction if ISS is not valid */
>> -    if ( !dabt.valid )
>> +    if ( !dabt.valid && !page_fault )
>>          goto bad_data_abort;
>>  
>>      /*
>>       * Erratum 766422: Thumb store translation fault to Hypervisor may
>>       * not have correct HSR Rt value.
>>       */
>> -    if ( cpu_has_erratum_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
>> +    if ( cpu_has_erratum_766422() && (regs->cpsr & PSR_THUMB) && dabt.write
>> +            && !page_fault)
>>      {
>>          rc = decode_instruction(regs, &info.dabt);
>>          if ( rc )
>> @@ -1358,6 +1361,16 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>          return;
>>      }
>>  
>> +    /* handle permission fault on write */
>> +    if ( page_fault )
>> +    {
>> +        if ( current->domain->arch.dirty.mode == 0 )
>> +           goto bad_data_abort;
>> +
>> +        handle_page_fault(current->domain, info.gpa);
>
>You must call advance_pc(regs, hsr) here.

I got it.

>
>> +        return;
>> +    }
>> +
>>  bad_data_abort:
>
>-- 
>Julien Grall