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