This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] [PATCH 2 of 6] REDO: mem_access & mem_access 2: mem even

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.


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

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


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

Xen-devel mailing list