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

[Julien Grall <julien.grall@citrix.com>]

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 <jaeyong.yoo@samsung.com>
> ---

[..]

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

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

> +        return;
> +    }
> +
>  bad_data_abort:

-- 
Julien Grall