From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jaeyong Yoo Subject: Re: [PATCH v4 8/9] xen/arm: Implement hypercall for dirty page tracing Date: Tue, 08 Oct 2013 06:29:12 +0000 (GMT) Message-ID: <25269810.259001381213751804.JavaMail.weblogic@epml25> Reply-To: jaeyong.yoo@samsung.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: MIME-version: 1.0 List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org >------- Original Message ------- >Sender : Julien Grall >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