Re: [Xen-devel] [PATCH 2 of 6] REDO: mem_access & mem_access 2: mem event additions for access

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 14:47 +0000 on 05 Jan (1294238857), Joe Epstein wrote:
> True.  I had this as a separate function only in case others found it
> useful in the future, and as a bit of a layer separation, where
> knowledge of the _VPF_mem_event bit would happen only in mem_event.c.
> If you don't mind me breaking that, I can move it over.

If you'd like to keep that separation, you could make a function that
does just the mark-and-pause, but I'd be happy enough with it open-coded
at the caller. 

>> Moving the log-dirty check inside the test for access_w reintroduces a
>> race condition in multi-vcpu systems (where the same log-dirty fault
>> happens on two CPUs and the first CPU has changed the type back to
>> p2m_ram_rw by the time the second one looks up the type).  Please put
>> this case back unconditionally.
> 
> Question: the conditional I'm replacing just simply says if (
> p2m_is_ram(p2mt) ).  I have to better qualify that to know that this
> was an access to a page based on its type and not based on its access
> permissions.  Otherwise, we'll either take memory events on logdirty,
> etc., or we'll miss real ones that got sent to change the page type
> instead.  What is the right conditional then, besides access_w, that
> will direct the handling appropriately?

I think you should always send mem-events when the access bits don't
match the access type.  Then if you didn't send a mem_event, do the
log-dirty/spurious-fault branch with the same condition as before.
Will that do the right thing for the cases you care about?

>>> + * Copyright (c) 2009 Citrix Systems, Inc.
>> 
>> Eh, probably not. :)
> 
> Hmm.  It's a direct copy of mem_paging.c, which is copyright Citrix.
> Since I changed only two lines out of it, it seemed rather
> inappropriate to claim copyright on derivative work.  But, if you'd
> like, I can make that change to claim it for ourselves.

You could leave that copyright line and add your own as well?

>> Might be better to use p2m_change_type here; it's atomic so avoids
>> potential races.
> 
> Question: p2m_change_type doesn't have the access permissions: only
> set_entry does, so I can't use it.  Should I rather just move p2m_lock
> above the get_entry?

Yes, that would be fine. 

>> Why does this need a special case?  Could you just post the violation
>> to the ring and pause as normal, and then if a listener ever arrives it
>> can handle it?
>> 
>> In fact, I don't see why access_required needs to be a separate flag at
>> all - it's implied by setting the access permissions on a page or
>> setting the default to anything other than rwx.  That would address
>> Keir's concern about adding a separate domcrf flag.
> 
> The idea is to have two modes: fail-closed, and fail-open.  If
> access_required is true, then the VCPU must pause.  But if it is
> false, I want to revert the access flags and let the VCPU chug along.
> That's to make sure that life works if the memory event handler
> crashes, of course, and explains why the page type is set to rwx and
> not default_access in that case.

Oh I see.  

> So I'll add a DOMCTL.

Righto. 

>> this belongs in a separate patch; it's not part of the change described
>> at the top.
> 
> Are you sure?  I changed the flag type from u64 to u16, so that breaks this 
> DPRINTF.

Oh, right.  Sorry, I missed that.  Yes, leave this in this patch.

Tim.

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel