Xen devel - Oct 2013 - Re: [PATCH v4 8/9] xen/arm: Implement hypercall for dirty page tracing

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

On Tue, 2013-10-08 at 06:29 +0000, Jaeyong Yoo wrote:
> >> +    /* 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.
Are you sure about this? Eugene''s reasoning that we need to replay the
faulting instruction seemed pretty sound.

Ian.

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Ian Campbell
> Sent: Tuesday, October 08, 2013 5:46 PM
> To: jaeyong.yoo@samsung.com
> Cc: Julien Grall; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v4 8/9] xen/arm: Implement hypercall for
> dirty page tracing
> 
> On Tue, 2013-10-08 at 06:29 +0000, Jaeyong Yoo wrote:
> 
> > >> +    /* 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.
> 
> Are you sure about this? Eugene''s reasoning that we need to replay
the
> faulting instruction seemed pretty sound.
Oh, I''ve got confused. I think Eugene''s reasoning is correct.
handle_mmio is for emulating and handle_page_fault is the generic fault,
which should be resolved and repeat the instruction. 
Sorry for the confusion.

Jaeyong
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

If we move necessary checking into handle_page_fault routine, so the 
resulting patch for traps.c will look more simple:

--- 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) )
      {
@@ -1334,6 +1336,13 @@ static void do_trap_data_abort_guest(struct 
cpu_user_regs *regs,
      if ( rc == -EFAULT )
          goto bad_data_abort;

+    /* domU page fault handling for guest live migration */
+    /* dabt.valid can be 0 here */
+    if ( page_fault && handle_page_fault(current->domain, info.gpa)
)
+    {
+        /* Do not modify pc after page fault to repeat memory operation */
+        return;
+    }
      /* XXX: Decode the instruction if ISS is not valid */
      if ( !dabt.valid )
          goto bad_data_abort;

Will it be acceptable, or you think "else" statement looks more
better?

Where handle_page_fault returns zero if dirty page tracing is not 
enabled for domain (dirty.mode==0) or address is not valid for domain 
(having memory map we check it), so MMIO and faults from dom0 are not 
handled by handle_page_fault.

The handle_page_fault routine (see  [PATCH v4 7/9] xen/arm: Implement 
virtual-linear page table for guest p2m mapping in live migration) will be:

  int handle_page_fault(struct domain *d, paddr_t addr)
{

     lpae_t *vlp2m_pte = 0;
     vaddr_t start, end;

     if (!d->arch.dirty.mode) return 0;
     /* Ensure that addr is inside guest''s RAM */
     get_gma_start_end(d, &start, &end);
     if ( addr < start || addr > end ) return 0;

     vlp2m_pte = get_vlpt_3lvl_pte(addr);
     if ( vlp2m_pte->p2m.valid && vlp2m_pte->p2m.write == 0 )
     {
         lpae_t pte = *vlp2m_pte;
         pte.p2m.write = 1;
         write_pte(vlp2m_pte, pte);
         flush_tlb_local();

         /* in order to remove mappings in cleanup stage */
         add_mapped_vaddr(d, addr);
     }

     return 1;
}

Best,
Evgeny.

On 10/08/2013 04:36 PM, Eugene Fedotov wrote:
> If we move necessary checking into handle_page_fault routine, so the
> resulting patch for traps.c will look more simple:
> 
> --- 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) )
>      {
> @@ -1334,6 +1336,13 @@ static void do_trap_data_abort_guest(struct
> cpu_user_regs *regs,
>      if ( rc == -EFAULT )
>          goto bad_data_abort;
> 
> +    /* domU page fault handling for guest live migration */
> +    /* dabt.valid can be 0 here */
> +    if ( page_fault && handle_page_fault(current->domain,
info.gpa) )
> +    {
> +        /* Do not modify pc after page fault to repeat memory operation */
> +        return;
> +    }
>      /* XXX: Decode the instruction if ISS is not valid */
>      if ( !dabt.valid )
>          goto bad_data_abort;
> 
> Will it be acceptable, or you think "else" statement looks more
better?
I''m fine with this solution.

Cheers,

-- 
Julien Grall

On Thu, 2013-10-10 at 15:13 +0100, Julien Grall wrote:
> On 10/08/2013 04:36 PM, Eugene Fedotov wrote:
> > If we move necessary checking into handle_page_fault routine, so the
> > resulting patch for traps.c will look more simple:
> > 
> > --- 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) )
> >      {
> > @@ -1334,6 +1336,13 @@ static void do_trap_data_abort_guest(struct
> > cpu_user_regs *regs,
> >      if ( rc == -EFAULT )
> >          goto bad_data_abort;
> > 
> > +    /* domU page fault handling for guest live migration */
> > +    /* dabt.valid can be 0 here */
> > +    if ( page_fault && handle_page_fault(current->domain,
info.gpa) )
> > +    {
> > +        /* Do not modify pc after page fault to repeat memory
operation */
> > +        return;
> > +    }
> >      /* XXX: Decode the instruction if ISS is not valid */
> >      if ( !dabt.valid )
> >          goto bad_data_abort;
> > 
> > Will it be acceptable, or you think "else" statement looks
more better?
> 
> I''m fine with this solution.
Looks good to me too, assuming you are happy with the ordering of MMIO
handling vs page faults.
> 
> Cheers,
>