Re: [Xen-devel] [PATCH RFC v2 6/9] xen/arm: Add handling write fault for dirty-page tracing

[Jaeyong Yoo <jaeyong.yoo@xxxxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> Sent: Wednesday, July 03, 2013 9:27 PM
> To: Jaeyong Yoo
> Cc: xen-devel@xxxxxxxxxxxxx; Elena Pyatunina
> Subject: Re: [Xen-devel] [PATCH RFC v2 6/9] xen/arm: Add handling write
> fault for dirty-page tracing
> 
> On Wed, 2013-07-03 at 18:15 +0900, Jaeyong Yoo wrote:
> > From: Elena Pyatunina <e.pyatunina@xxxxxxxxxxx>
> >
> > Add handling write fault in do_trap_data_abort_guest for dirty-page
> > tracing. Mark dirty function makes the bitmap of dirty pages.
> 
> You've gone with the model of allocating a bitmap when logdirty is enabled
> and updating it directly from the fault handler? The bit map is based
> around a three level structure mirroring the p2m tables?

Yes, you got it right.

> 
> I had been considering the option of reusing one of the bits available for
> software use in the p2m entries to track the dirty status and walking the
> p2m when the toolstack asked for the current bitmap instead.
> 
> I think this would have less memory overhead and also would allow us to be
> lock free on the fault handling path (by using suitable atomic exchanges
> on the p2m entries), as well as avoiding memory allocations on that path.
> 
> I think we'd want to establish a linear map of the current guest p2m for
> these purposes so we could also avoid all the map_domain_page stuff in the
> fault path. I think there is enough virtual address space left for that
> even on 32-bit, it's less critical on 64-bit anyway since we are about to
> have a direct map of RAM available to us.

I think it is a good idea to have an ever-mapping linear dirty-bit map for 
guest p2m. Maybe between fixmap and frametable? 


> 
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index
> > 398d209..9927712 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1011,6 +1011,13 @@ static void do_trap_data_abort_guest(struct
> cpu_user_regs *regs,
> >      if ( rc == -EFAULT )
> >          goto bad_data_abort;
> >
> > +    /* handle permission fault on write */
> > +    if ((dabt.dfsc & 0x3f) == (FSC_FLT_PERM + 3) && dabt.write)
> 
> Can we get some #defines for these magic constants please.

Got it.

> 
> > +    {
> > +        if (handle_page_fault(current->domain, info.gpa) == 0)
> > +            return;
> > +    }
> > +
> >      if (handle_mmio(&info))
> 
> Since you handle page fault first, won't this make MMIO trapping regions
> writable?

Oops. I forget to exchange the sequence of handle_page_fault and handle_mmio.
Then, it would fix the problem right?
Because handle_mmio is handling virtual emulated devices, 
I don't think writing to mmio is necessary to be dirty-page traced.


> 
> Ian.


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