Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Tue, Jul 19, 2016 at 11:11 AM, George Dunlap
 wrote:
> On 18/07/16 18:06, Tamas K Lengyel wrote:
>>> Incremental improvements are welcome; but they must not cause
>>> regressions in existing functionality.
>>
>> Existing functionality does not get impaired by this as what happens
>> right now is a hypervisor crash. I don't see how things can get any
>> worst than that.
>
> Also from another thread:
>> If anyone else would have been interested in getting the two systems
>> working together othen then me they probably would have complained
>> already that hey this crashes the hypervisor. My point being that at
>> this point the impact of this patch is likely really low.
>
> From a user perspective, "failing intermittently in some strange and
> unpredictable way" is definitely worse than a hypervisor crash. :-)
>
> My concern is that someone will start using guests which use the altp2m
> interface internally, and that will all work; and then maybe separately
> they will start doing some sort of memory sharing between guests, and
> that will all work; and then at some point they'll do memory sharing on
> a guest using the altp2m functionality internally, and suddenly they'll
> get strange intermittent errors where things don't behave the way they
> expect and they don't know why.  A hypervisor crash that tells them
> exactly what code has the problem is definitely preferable.

Well, IMHO that's where documenting the expected use-case and the
known corner-cases comes into play.

>
>>> The code as it is in the tree right now was intended to allow both
>>> sharing and altp2m to be enabled on the same domain, just not over the
>>> same gfn range.  And it was intended to be robust -- that is, the
>>> sharing code and the altp2m code don't need to be aware of each other
>>> and try not to step on each other's toes; each can just do its own thing
>>> and Xen will make sure that nothing bad happens (by preventing pages
>>> with an altp2m entry from being shared, and unsharing pages for which an
>>> altp2m entry is created).
>>>
>>> It sounds like that's broken right now; it should therefore be fixed.
>>> When it is fixed, you'll be able to use both altp2m and sharing on the
>>> same domain; Xen will simply prevent sharing from happening on gfn
>>> ranges with altp2m entries.
>>
>> No, that's incorrect, it's the other way around. If you were to try to
>> share pages for which you have altp2m entries it will happily oblige.
>> It will just fail to do the altp2m actions for entries of shared
>> entries (copying the mapping to the altp2m view, mem_access, etc).
>
> It's quite possible I missed something, but that's not how I read the
> code.  Before sharing a page you have to have to call
> mem_sharing_nominate_page(), which calls page_make_sharable().
> page_make_sharable() will make sure that it has exactly the expected
> number of references; which for gfns is 2 and for grant references is 4.
>
> When you map an mfn into an altp2m of a different gfn, you'll increase
> the reference count.  So it appears to me that if you attempt to share a
> page which is mapped in an altp2m, then the nominate operation will fail
> (with -E2BIG, of all things).
>
> Am I mistaken about that?

Hm, no you may be right. I was thinking of the type checking only. If
the reference count prevents pages with alt2pm entries from being
shared  - going from p2m_ram_rw -> p2m_ram_shared - then from my
perspective that is fine and I'm not planning on changing that. What
I'm trying to get working is if the type is already p2m_ram_shared and
is going from p2m_ram_shared -> p2m_ram_rw. I would also like to be
able to do mem_access settings for the p2m_ram_shared type pte in an
altp2m view.

>
> (BTB this would probably still be the case even after your patch.)
>
> Also, as far as I can tell, "It will just fail to do the altp2m actions
> for entries of shared entries" is not true; instead, the page will be
> un-shared and the altp2m action will then take place.  Is this not the case?

So right now when the entry is p2m_ram_shared it will crash the
hypervisor because of the lock ordering issue during unsharing. If the
lock ordering issue is fixed, the unsharing event will result in the
altp2m propagate change taking the p2m setting from the hostp2m and
copying it to all affected altp2m views, overwriting any setting that
may have been stored there. This is the situation that can be
monitored with mem_access so that the user can perform the unsharing
and recreating the necessary altp2m settings manually. What I mean in
the quoted sentence is that the altp2m ops do a type-check right now,
so if you shared a page before, the type check will make all altp2m
ops fail on that entry. So for example if you have a shared pte, and
then try to do altp2m mem_access setting on it, it will fail.

>
>>> An even bigger improvement would be to allow the same gfns to be subject
>>> both to altp2m and sharing at the same time.  But this 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On 18/07/16 18:06, Tamas K Lengyel wrote:
>> Incremental improvements are welcome; but they must not cause
>> regressions in existing functionality.
> 
> Existing functionality does not get impaired by this as what happens
> right now is a hypervisor crash. I don't see how things can get any
> worst than that.

Also from another thread:
> If anyone else would have been interested in getting the two systems
> working together othen then me they probably would have complained
> already that hey this crashes the hypervisor. My point being that at
> this point the impact of this patch is likely really low.

From a user perspective, "failing intermittently in some strange and
unpredictable way" is definitely worse than a hypervisor crash. :-)

My concern is that someone will start using guests which use the altp2m
interface internally, and that will all work; and then maybe separately
they will start doing some sort of memory sharing between guests, and
that will all work; and then at some point they'll do memory sharing on
a guest using the altp2m functionality internally, and suddenly they'll
get strange intermittent errors where things don't behave the way they
expect and they don't know why.  A hypervisor crash that tells them
exactly what code has the problem is definitely preferable.

>> The code as it is in the tree right now was intended to allow both
>> sharing and altp2m to be enabled on the same domain, just not over the
>> same gfn range.  And it was intended to be robust -- that is, the
>> sharing code and the altp2m code don't need to be aware of each other
>> and try not to step on each other's toes; each can just do its own thing
>> and Xen will make sure that nothing bad happens (by preventing pages
>> with an altp2m entry from being shared, and unsharing pages for which an
>> altp2m entry is created).
>>
>> It sounds like that's broken right now; it should therefore be fixed.
>> When it is fixed, you'll be able to use both altp2m and sharing on the
>> same domain; Xen will simply prevent sharing from happening on gfn
>> ranges with altp2m entries.
> 
> No, that's incorrect, it's the other way around. If you were to try to
> share pages for which you have altp2m entries it will happily oblige.
> It will just fail to do the altp2m actions for entries of shared
> entries (copying the mapping to the altp2m view, mem_access, etc).

It's quite possible I missed something, but that's not how I read the
code.  Before sharing a page you have to have to call
mem_sharing_nominate_page(), which calls page_make_sharable().
page_make_sharable() will make sure that it has exactly the expected
number of references; which for gfns is 2 and for grant references is 4.

When you map an mfn into an altp2m of a different gfn, you'll increase
the reference count.  So it appears to me that if you attempt to share a
page which is mapped in an altp2m, then the nominate operation will fail
(with -E2BIG, of all things).

Am I mistaken about that?

(BTB this would probably still be the case even after your patch.)

Also, as far as I can tell, "It will just fail to do the altp2m actions
for entries of shared entries" is not true; instead, the page will be
un-shared and the altp2m action will then take place.  Is this not the case?

>> An even bigger improvement would be to allow the same gfns to be subject
>> both to altp2m and sharing at the same time.  But this requires thinking
>> carefully about all the corner cases and making sure that they all work
>> correctly.
> 
> And this is exactly what this patch allows you to do. An entry can now
> be both shared, get properly copied to altp2m views, allow setting
> mem_access in altp2m views, etc. The only situation you have to take
> core of is when the type of the entry changes from shared to unshared
> as that resets the altp2m views.

I described another situation you have to be careful of in an earlier
e-mail:
- host gfn A is marked "shared"
- altp2m gfn O is mapped to gfn A (thus also marked as 'shared')
- Guest writes to gfn O, Xen attempts to unshare the page.

In this circumstance, the fault will ends up in
__mem_sharing_unshare_page(), which will calls rmap_retrieve(d, O, mA).
This returns NULL because gfn O was never put in the reverse map, and
you BUG().

Again, am I misreading what would happen?

I'm pretty sure if I went looking I could find some more situations you
need to avoid to prevent problems.

So the next big missing piece of information in this discussion is
exactly what you do need from this system.  You're using altp2m and
mem_sharing (and mem_access) on the same domain, that's obvious.  Which
features of altp2m are you using -- are you mainly using the mem_access
changes, or are you also using the gfn mapping functionality?

Also, how important is it that pages using altp2m functionality not be
un-shared -- i.e., what proportion of a guest's pages do you expect to
be shared, and what proportion do you need to perform altp2m operations on?

So there 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Tue, Jul 19, 2016 at 7:36 AM, Ian Jackson  wrote:
> Tamas: George brought this thread to my attention.  I'm sorry that you
> feel blocked and/or overruled.  The hypervisor MM code is not my area
> of expertise, but I have a keen interest in seeing a good, productive
> and friendly Xen community.  I definitely don't want to see you pushed
> away, and driven to maintain an out-of-tree patchset.
>
> Reading through your mails there seem to still have unresolved
> detailed technical disagreements between you and George about the
> existing behaviours in Xen, and the effects of your proposed changes.
>
> Right now I would like to ask both you and George to sort out those
> factual disagreements.  I expect that you can do so.  I hope that then
> the way forward will be clear: ie that you and George willbe in
> agreement about the direction in which the code should be going.
>
> I think that would be better than getting into a more abstract
> conversation about which use cases exist or are important, or an
> argument about areas of responsibility or authority.
>
> If either of you feel that you aren't able to agree on the facts, or
> that that conversation is not proceeding constructively, I'm be happy
> to try to help.  You can contact me by email in public or private, or
> find me as Diziet on irc.
>
> (In a complex codebase like Xen there will always be overlap or
> interference between different maintainers' bailiwicks, so we
> definitely do need to be able to come to some kind of agreement,
> rather than everyone insisting on their own authority in what they
> regard as their own area.)
>
> Regards,
> Ian.

Thanks Ian, I agree and hope we can get back to technical issues as
well. I certainly didn't mean to escalate this. I do hope we can get
to the bottom of what concerns are applicable to this change and
discuss what we can do to address those in a reasonable fashion.

Best,
Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Tue, Jul 19, 2016 at 5:39 AM, George Dunlap  wrote:
> On 18/07/16 18:06, Tamas K Lengyel wrote:
 Anyhow, at this point I'm
 going to start carrying out-of-tree patches for Xen in my project and
 just resign from my mem_sharing maintainership as I feel like it's
 pretty pointless.
>>>
>>> I'm sorry that you're discouraged; all I can say is that I hope you
>>> reconsider.  I'm not trying to block you, and I'm not ignoring your use
>>> case; it's the job of a maintainer to look at *everyone's* use cases and
>>> try to make sure that they are all accommodated in so far as it is
>>> possible.
>>>
>>> I'm also trying to make sure that the code you end up using in your
>>> project is robust and reliable.  It seems to me like if the current
>>> implementation was fixed, your life would be a lot easier than if we
>>> accept your patch as it is -- your sharing code could just worry about
>>> sharing, your altp2m code could just worry about whatever it's trying to
>>> do, without having to carefully avoid corner cases or manually fix
>>> things up when corner cases happen.  A bit less sharing would happen,
>>> because fewer pages would be eligible to be shared, but overall you'd
>>> have a lot less bugs and headache.
>>>
>>> I invested a lot of my very limited time carefully going through both
>>> sets of code before I answered your e-mail, and I spent a lot of time
>>> trying to explain the kinds of interactions I think will be a problem.
>>> I could have just acked the patch without doing that; but I think that
>>> would have been both less good for you, and less good for the project as
>>> a whole.
>>
>> I certainly appreciate your time spent on this. However, I don't see
>> the point of being maintainer if my opinion on what constitutes an
>> improvement of the system just gets overruled.
>
> You're not being overruled; you're just being asked to make a case for a
> change you want to make to an area of code that I maintain (the p2m
> code).  And the discussion is by no means over; I started the most
> recent discussion by saying "Correct me if I'm wrong", and it looks like
> there are still a number of places where we have different views of the
> facts of the matter.  Once we've established those we may end up with
> closer opinions.
>
> Working together means that sometimes you have to spend the time and
> effort to understand where other people are coming from -- what they
> think is important, what they think is true; and then working with that
> -- correcting them on places where they have misconceptions (or
> double-checking your own beliefs to make sure that you're not mistaken);
> communicating what it is that you think is important, and then trying to
> come up with a way forward that takes everyone's values into account, or
> convincing someone that a particular way really is the best way forward
> (which may mean convincing them to change their priorities somewhat).
>
> I am sorry that the tone of this conversation has heated up.  But the
> reason I've been "raising my voice" as it were is because I've been
> trying to ask questions and raise potential issues, and I feel like
> you've been just hand-waving them away.  You may be 100% right, but it
> is my duty as the maintainer of the p2m code to not accept code until
> I'm reasonably convinced that it's a good way forward.

By no means I meant to heat-up the conversation or hand-wave your
concerns away. I do understand what it takes to work with the
community and that it takes cooperation for that to happen. I was not
hand-waving your concerns away but describing how the two systems
could interact safely together while agreeing with you that yes, there
are still scenarios where it would not be wise to turn two
experimental systems on together.

>
>> I would like to hear the
>> other maintainers opinion on this matter as well but I'm not
>> interested in arguing endlessly or initiating or vote, so if the patch
>> is not allowed in I will accept that decision but I will see no point
>> in continuing as maintainer of the system.
>
> At a basic level, the other maintainers will agree that I shouldn't
> accept code unless I am convinced it's for the good of the project.  But
> since this is a technical issue, before anyone would express an opinion
> to ask me to change my mind, they would want a more complete view of the
> facts of the matter -- facts which you and I are still in the process of
> sorting out.

Both of these systems are fairly complicated and not many people have
been looking at them in-depth, so I most certainly appreciate the time
you spent on reviewing thus far. But your conclusion that there is a
"long way to go here" tells me that an arbitrary criteria is getting
pushed on me that I don't even know how to address. The altp2m system
got merged while it was known that it crashes the hypervisor when
mem_sharing is used, but now when an incremental fix introduces at
least one setup where they 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Ian Jackson]

Tamas: George brought this thread to my attention.  I'm sorry that you
feel blocked and/or overruled.  The hypervisor MM code is not my area
of expertise, but I have a keen interest in seeing a good, productive
and friendly Xen community.  I definitely don't want to see you pushed
away, and driven to maintain an out-of-tree patchset.

Reading through your mails there seem to still have unresolved
detailed technical disagreements between you and George about the
existing behaviours in Xen, and the effects of your proposed changes.

Right now I would like to ask both you and George to sort out those
factual disagreements.  I expect that you can do so.  I hope that then
the way forward will be clear: ie that you and George willbe in
agreement about the direction in which the code should be going.

I think that would be better than getting into a more abstract
conversation about which use cases exist or are important, or an
argument about areas of responsibility or authority.

If either of you feel that you aren't able to agree on the facts, or
that that conversation is not proceeding constructively, I'm be happy
to try to help.  You can contact me by email in public or private, or
find me as Diziet on irc.

(In a complex codebase like Xen there will always be overlap or
interference between different maintainers' bailiwicks, so we
definitely do need to be able to come to some kind of agreement,
rather than everyone insisting on their own authority in what they
regard as their own area.)

Regards,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On 18/07/16 18:06, Tamas K Lengyel wrote:
>>> Anyhow, at this point I'm
>>> going to start carrying out-of-tree patches for Xen in my project and
>>> just resign from my mem_sharing maintainership as I feel like it's
>>> pretty pointless.
>>
>> I'm sorry that you're discouraged; all I can say is that I hope you
>> reconsider.  I'm not trying to block you, and I'm not ignoring your use
>> case; it's the job of a maintainer to look at *everyone's* use cases and
>> try to make sure that they are all accommodated in so far as it is
>> possible.
>>
>> I'm also trying to make sure that the code you end up using in your
>> project is robust and reliable.  It seems to me like if the current
>> implementation was fixed, your life would be a lot easier than if we
>> accept your patch as it is -- your sharing code could just worry about
>> sharing, your altp2m code could just worry about whatever it's trying to
>> do, without having to carefully avoid corner cases or manually fix
>> things up when corner cases happen.  A bit less sharing would happen,
>> because fewer pages would be eligible to be shared, but overall you'd
>> have a lot less bugs and headache.
>>
>> I invested a lot of my very limited time carefully going through both
>> sets of code before I answered your e-mail, and I spent a lot of time
>> trying to explain the kinds of interactions I think will be a problem.
>> I could have just acked the patch without doing that; but I think that
>> would have been both less good for you, and less good for the project as
>> a whole.
> 
> I certainly appreciate your time spent on this. However, I don't see
> the point of being maintainer if my opinion on what constitutes an
> improvement of the system just gets overruled.

You're not being overruled; you're just being asked to make a case for a
change you want to make to an area of code that I maintain (the p2m
code).  And the discussion is by no means over; I started the most
recent discussion by saying "Correct me if I'm wrong", and it looks like
there are still a number of places where we have different views of the
facts of the matter.  Once we've established those we may end up with
closer opinions.

Working together means that sometimes you have to spend the time and
effort to understand where other people are coming from -- what they
think is important, what they think is true; and then working with that
-- correcting them on places where they have misconceptions (or
double-checking your own beliefs to make sure that you're not mistaken);
communicating what it is that you think is important, and then trying to
come up with a way forward that takes everyone's values into account, or
convincing someone that a particular way really is the best way forward
(which may mean convincing them to change their priorities somewhat).

I am sorry that the tone of this conversation has heated up.  But the
reason I've been "raising my voice" as it were is because I've been
trying to ask questions and raise potential issues, and I feel like
you've been just hand-waving them away.  You may be 100% right, but it
is my duty as the maintainer of the p2m code to not accept code until
I'm reasonably convinced that it's a good way forward.

> I would like to hear the
> other maintainers opinion on this matter as well but I'm not
> interested in arguing endlessly or initiating or vote, so if the patch
> is not allowed in I will accept that decision but I will see no point
> in continuing as maintainer of the system.

At a basic level, the other maintainers will agree that I shouldn't
accept code unless I am convinced it's for the good of the project.  But
since this is a technical issue, before anyone would express an opinion
to ask me to change my mind, they would want a more complete view of the
facts of the matter -- facts which you and I are still in the process of
sorting out.

> As pretty much my
> project is the only use-case where these two systems would be used
> together at this point, and since I already require my users to
> compile Xen from source it is just easier to go this route then what
> you suggest and exploring and remedying all possible ways the two
> systems could be misused when setup in ways they were not intended. If
> these were considered stable features and not experimental I would
> agree, but that's just not the case. So I think both of our time is
> better spent doing other things then arguing. 

So a lot of points here.

You have no idea what other projects are doing.  Lots of people take the
Xen code, do something with it internally, and and we never hear from
them.  Or maybe they're in a start-up in stealth mode and will announce
themselves in due course.  If you step down from being a maintainer and
stop engaging with the community you'll be in the same position.

There's a very obvious other use case which I've been talking about from
the beginning: A host administrator / cloud provider / user wants to
both 1) use page sharing to improve 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Mon, Jul 18, 2016 at 10:10 AM, George Dunlap
 wrote:
> On 18/07/16 16:18, Tamas K Lengyel wrote:
>> On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap  
>> wrote:
>>> On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  
>>> wrote:
> I could go on in the analysis, but the point is that there's a morass
> of interactions here all of which need to be correct, which this patch
> does not address.  You have a long way to go before sharing and altp2m
> can be safely used on the same gfn ranges.
>

 Hi George,
 certainly there are cornercases where if the user does not setup things in
 the right order things can still go out of whack. My goal with this patch 
 is
 not to address all of them. The goal of this patch is to not crash the
 hypervisor when the user at least tries to experiment with it, which is the
 current state. So this patch improves the status quo. Also, both 
 mem_sharing
 and altp2m is considered experimental/tech_preview, so the fact that their
 combination is also experimental should be assumed as well. As I explained,
 with this patch in place there is at least one way they can be safely used
 together if the user tracks unsharing requirements through mem_access and
 does unsharing and fixup of the altp2m views manually. There are other ways
 where it would not be safe as after unsharing we don't know how the user
 would want things to look in altp2ms. I don't think we want to start
 guessing about that either so I will not be looking to implement that. So I
 don't agree with this reasoning being grounds for rejecting this patch that
 does incrementally improve the current state.
>>>
>>> So you keep saying "user"; I assume you mean whatever program is
>>> sitting in domain 0, which is going to be doing memsharing, altp2m,
>>> and memaccess stuff all at once?
>>>
>>> The altp2m code was not written for that purpose.  It was written for
>>> *guest* administrators to use within the guest.
>>
>> That's simply not true. It was written specifically to allow both
>> usecases - both internal *and* external. Mixing the use-cases was not
>> envisioned.
>
> Right, well in any case I certainly think that having external users of
> the feature is a good thing to pursue.
>
>>
>>   There's no problem
>>> with finding additional uses for it, as long as the *original* purpose
>>> doesn't get broken; and this patch most certainly does break things
>>> for that purpose.
>>
>> This patch most certainly *does not break* the in-guest usecase by
>> itself. If the in-guest usecase is used without any mem_sharing going
>> on, this patch does not have any effect on that usecase.
>>
>>   Guests using altp2m should not have to know that
>>> sharing is happening behind their backs; nor should they be required
>>> to use mem_access to manually fix up what the hypervisor has allowed
>>> to be broken.
>>
>> And they are not required. This requirement only applies if the user
>> mixes in in-guest and external usecases.
>
> I keep saying "dom0" and "guest administrator", and you keep saying
> "user", as though these are always going to be one and the same person.
> That is a valid use case, but it is not the normal use case for Xen.  We
> need to make it possible for host administrators to be able to decide to
> enable sharing without having to know whether the guest administrator is
> using altp2m; and the for the guest administrator to be able to decide
> to use altp2m without having to know whether the host administrator is
> going to enable sharing.
>
> And even if they are the same person, I think code that Just Works is
> better than code that has a lot of corner cases you have to avoid.
>
>>> If at the moment altp2m + memsharing just plain crashes, then that
>>> should be fixed; and if the lock-ordering parts of the patch fix that,
>>> then they should be applied.
>>
>> Yes, that would be a start at least.
>>
>>   But the code which make sure that the
>>> same gfn range cannot both be shared and subject to altp2m must remain
>>> until they interact properly, with all the corner cases carefully
>>> thought out.
>>
>> Well, I don't see that what you suggest is going to happen if
>> incremental improvements are not allowed in.
>
> Incremental improvements are welcome; but they must not cause
> regressions in existing functionality.

Existing functionality does not get impaired by this as what happens
right now is a hypervisor crash. I don't see how things can get any
worst than that.

>
> The code as it is in the tree right now was intended to allow both
> sharing and altp2m to be enabled on the same domain, just not over the
> same gfn range.  And it was intended to be robust -- that is, the
> sharing code and the altp2m code don't need to be aware of each other
> and try not to step on each other's toes; each can just do its own thing
> and Xen will make 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Mon, Jul 18, 2016 at 10:15 AM, George Dunlap
 wrote:
> On 18/07/16 16:27, Andrew Cooper wrote:
>> On 18/07/16 16:18, Tamas K Lengyel wrote:
>>> On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap  
>>> wrote:
 On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  
 wrote:
>> I could go on in the analysis, but the point is that there's a morass
>> of interactions here all of which need to be correct, which this patch
>> does not address.  You have a long way to go before sharing and altp2m
>> can be safely used on the same gfn ranges.
>>
> Hi George,
> certainly there are cornercases where if the user does not setup things in
> the right order things can still go out of whack. My goal with this patch 
> is
> not to address all of them. The goal of this patch is to not crash the
> hypervisor when the user at least tries to experiment with it, which is 
> the
> current state. So this patch improves the status quo. Also, both 
> mem_sharing
> and altp2m is considered experimental/tech_preview, so the fact that their
> combination is also experimental should be assumed as well. As I 
> explained,
> with this patch in place there is at least one way they can be safely used
> together if the user tracks unsharing requirements through mem_access and
> does unsharing and fixup of the altp2m views manually. There are other 
> ways
> where it would not be safe as after unsharing we don't know how the user
> would want things to look in altp2ms. I don't think we want to start
> guessing about that either so I will not be looking to implement that. So 
> I
> don't agree with this reasoning being grounds for rejecting this patch 
> that
> does incrementally improve the current state.
 So you keep saying "user"; I assume you mean whatever program is
 sitting in domain 0, which is going to be doing memsharing, altp2m,
 and memaccess stuff all at once?

 The altp2m code was not written for that purpose.  It was written for
 *guest* administrators to use within the guest.
>>> That's simply not true. It was written specifically to allow both
>>> usecases - both internal *and* external. Mixing the use-cases was not
>>> envisioned.
>>
>> Tamas is correct here.  altp2m was specifically designed and implemented
>> usable by both internal and external entities, irrespective of hardware
>> support.
>>
>> Any modifications to the subsystem should maintain this property.
>
> Indeed; it was also designed to be robust when used with sharing
> (although it was apparently never tested, because it's currently broken
> in that respect).  And it is this property that I'm trying to maintain,
> both for internal and external users.
>

It was tested and the hypervisor crash was pointed out during the
merging process of altp2m, but it was let in anyway as both
mem_sharing and altp2m was considered experimental and thus this crash
in a cornercase was deemed acceptable.

Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On 18/07/16 16:27, Andrew Cooper wrote:
> On 18/07/16 16:18, Tamas K Lengyel wrote:
>> On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap  
>> wrote:
>>> On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  
>>> wrote:
> I could go on in the analysis, but the point is that there's a morass
> of interactions here all of which need to be correct, which this patch
> does not address.  You have a long way to go before sharing and altp2m
> can be safely used on the same gfn ranges.
>
 Hi George,
 certainly there are cornercases where if the user does not setup things in
 the right order things can still go out of whack. My goal with this patch 
 is
 not to address all of them. The goal of this patch is to not crash the
 hypervisor when the user at least tries to experiment with it, which is the
 current state. So this patch improves the status quo. Also, both 
 mem_sharing
 and altp2m is considered experimental/tech_preview, so the fact that their
 combination is also experimental should be assumed as well. As I explained,
 with this patch in place there is at least one way they can be safely used
 together if the user tracks unsharing requirements through mem_access and
 does unsharing and fixup of the altp2m views manually. There are other ways
 where it would not be safe as after unsharing we don't know how the user
 would want things to look in altp2ms. I don't think we want to start
 guessing about that either so I will not be looking to implement that. So I
 don't agree with this reasoning being grounds for rejecting this patch that
 does incrementally improve the current state.
>>> So you keep saying "user"; I assume you mean whatever program is
>>> sitting in domain 0, which is going to be doing memsharing, altp2m,
>>> and memaccess stuff all at once?
>>>
>>> The altp2m code was not written for that purpose.  It was written for
>>> *guest* administrators to use within the guest.
>> That's simply not true. It was written specifically to allow both
>> usecases - both internal *and* external. Mixing the use-cases was not
>> envisioned.
> 
> Tamas is correct here.  altp2m was specifically designed and implemented
> usable by both internal and external entities, irrespective of hardware
> support.
> 
> Any modifications to the subsystem should maintain this property.

Indeed; it was also designed to be robust when used with sharing
(although it was apparently never tested, because it's currently broken
in that respect).  And it is this property that I'm trying to maintain,
both for internal and external users.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On 18/07/16 16:18, Tamas K Lengyel wrote:
> On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap  
> wrote:
>> On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  wrote:
 I could go on in the analysis, but the point is that there's a morass
 of interactions here all of which need to be correct, which this patch
 does not address.  You have a long way to go before sharing and altp2m
 can be safely used on the same gfn ranges.

>>>
>>> Hi George,
>>> certainly there are cornercases where if the user does not setup things in
>>> the right order things can still go out of whack. My goal with this patch is
>>> not to address all of them. The goal of this patch is to not crash the
>>> hypervisor when the user at least tries to experiment with it, which is the
>>> current state. So this patch improves the status quo. Also, both mem_sharing
>>> and altp2m is considered experimental/tech_preview, so the fact that their
>>> combination is also experimental should be assumed as well. As I explained,
>>> with this patch in place there is at least one way they can be safely used
>>> together if the user tracks unsharing requirements through mem_access and
>>> does unsharing and fixup of the altp2m views manually. There are other ways
>>> where it would not be safe as after unsharing we don't know how the user
>>> would want things to look in altp2ms. I don't think we want to start
>>> guessing about that either so I will not be looking to implement that. So I
>>> don't agree with this reasoning being grounds for rejecting this patch that
>>> does incrementally improve the current state.
>>
>> So you keep saying "user"; I assume you mean whatever program is
>> sitting in domain 0, which is going to be doing memsharing, altp2m,
>> and memaccess stuff all at once?
>>
>> The altp2m code was not written for that purpose.  It was written for
>> *guest* administrators to use within the guest.
> 
> That's simply not true. It was written specifically to allow both
> usecases - both internal *and* external. Mixing the use-cases was not
> envisioned.

Right, well in any case I certainly think that having external users of
the feature is a good thing to pursue.

> 
>   There's no problem
>> with finding additional uses for it, as long as the *original* purpose
>> doesn't get broken; and this patch most certainly does break things
>> for that purpose.
> 
> This patch most certainly *does not break* the in-guest usecase by
> itself. If the in-guest usecase is used without any mem_sharing going
> on, this patch does not have any effect on that usecase.
> 
>   Guests using altp2m should not have to know that
>> sharing is happening behind their backs; nor should they be required
>> to use mem_access to manually fix up what the hypervisor has allowed
>> to be broken.
> 
> And they are not required. This requirement only applies if the user
> mixes in in-guest and external usecases.

I keep saying "dom0" and "guest administrator", and you keep saying
"user", as though these are always going to be one and the same person.
That is a valid use case, but it is not the normal use case for Xen.  We
need to make it possible for host administrators to be able to decide to
enable sharing without having to know whether the guest administrator is
using altp2m; and the for the guest administrator to be able to decide
to use altp2m without having to know whether the host administrator is
going to enable sharing.

And even if they are the same person, I think code that Just Works is
better than code that has a lot of corner cases you have to avoid.

>> If at the moment altp2m + memsharing just plain crashes, then that
>> should be fixed; and if the lock-ordering parts of the patch fix that,
>> then they should be applied.
> 
> Yes, that would be a start at least.
> 
>   But the code which make sure that the
>> same gfn range cannot both be shared and subject to altp2m must remain
>> until they interact properly, with all the corner cases carefully
>> thought out.
> 
> Well, I don't see that what you suggest is going to happen if
> incremental improvements are not allowed in. 

Incremental improvements are welcome; but they must not cause
regressions in existing functionality.

The code as it is in the tree right now was intended to allow both
sharing and altp2m to be enabled on the same domain, just not over the
same gfn range.  And it was intended to be robust -- that is, the
sharing code and the altp2m code don't need to be aware of each other
and try not to step on each other's toes; each can just do its own thing
and Xen will make sure that nothing bad happens (by preventing pages
with an altp2m entry from being shared, and unsharing pages for which an
altp2m entry is created).

It sounds like that's broken right now; it should therefore be fixed.
When it is fixed, you'll be able to use both altp2m and sharing on the
same domain; Xen will simply prevent sharing from happening on gfn

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Andrew Cooper]

On 18/07/16 16:18, Tamas K Lengyel wrote:
> On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap  
> wrote:
>> On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  wrote:
 I could go on in the analysis, but the point is that there's a morass
 of interactions here all of which need to be correct, which this patch
 does not address.  You have a long way to go before sharing and altp2m
 can be safely used on the same gfn ranges.

>>> Hi George,
>>> certainly there are cornercases where if the user does not setup things in
>>> the right order things can still go out of whack. My goal with this patch is
>>> not to address all of them. The goal of this patch is to not crash the
>>> hypervisor when the user at least tries to experiment with it, which is the
>>> current state. So this patch improves the status quo. Also, both mem_sharing
>>> and altp2m is considered experimental/tech_preview, so the fact that their
>>> combination is also experimental should be assumed as well. As I explained,
>>> with this patch in place there is at least one way they can be safely used
>>> together if the user tracks unsharing requirements through mem_access and
>>> does unsharing and fixup of the altp2m views manually. There are other ways
>>> where it would not be safe as after unsharing we don't know how the user
>>> would want things to look in altp2ms. I don't think we want to start
>>> guessing about that either so I will not be looking to implement that. So I
>>> don't agree with this reasoning being grounds for rejecting this patch that
>>> does incrementally improve the current state.
>> So you keep saying "user"; I assume you mean whatever program is
>> sitting in domain 0, which is going to be doing memsharing, altp2m,
>> and memaccess stuff all at once?
>>
>> The altp2m code was not written for that purpose.  It was written for
>> *guest* administrators to use within the guest.
> That's simply not true. It was written specifically to allow both
> usecases - both internal *and* external. Mixing the use-cases was not
> envisioned.

Tamas is correct here.  altp2m was specifically designed and implemented
usable by both internal and external entities, irrespective of hardware
support.

Any modifications to the subsystem should maintain this property.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Mon, Jul 18, 2016 at 5:04 AM, George Dunlap  wrote:
> On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  wrote:
>>> I could go on in the analysis, but the point is that there's a morass
>>> of interactions here all of which need to be correct, which this patch
>>> does not address.  You have a long way to go before sharing and altp2m
>>> can be safely used on the same gfn ranges.
>>>
>>
>> Hi George,
>> certainly there are cornercases where if the user does not setup things in
>> the right order things can still go out of whack. My goal with this patch is
>> not to address all of them. The goal of this patch is to not crash the
>> hypervisor when the user at least tries to experiment with it, which is the
>> current state. So this patch improves the status quo. Also, both mem_sharing
>> and altp2m is considered experimental/tech_preview, so the fact that their
>> combination is also experimental should be assumed as well. As I explained,
>> with this patch in place there is at least one way they can be safely used
>> together if the user tracks unsharing requirements through mem_access and
>> does unsharing and fixup of the altp2m views manually. There are other ways
>> where it would not be safe as after unsharing we don't know how the user
>> would want things to look in altp2ms. I don't think we want to start
>> guessing about that either so I will not be looking to implement that. So I
>> don't agree with this reasoning being grounds for rejecting this patch that
>> does incrementally improve the current state.
>
> So you keep saying "user"; I assume you mean whatever program is
> sitting in domain 0, which is going to be doing memsharing, altp2m,
> and memaccess stuff all at once?
>
> The altp2m code was not written for that purpose.  It was written for
> *guest* administrators to use within the guest.

That's simply not true. It was written specifically to allow both
usecases - both internal *and* external. Mixing the use-cases was not
envisioned.

  There's no problem
> with finding additional uses for it, as long as the *original* purpose
> doesn't get broken; and this patch most certainly does break things
> for that purpose.

This patch most certainly *does not break* the in-guest usecase by
itself. If the in-guest usecase is used without any mem_sharing going
on, this patch does not have any effect on that usecase.

  Guests using altp2m should not have to know that
> sharing is happening behind their backs; nor should they be required
> to use mem_access to manually fix up what the hypervisor has allowed
> to be broken.

And they are not required. This requirement only applies if the user
mixes in in-guest and external usecases.

>
> If at the moment altp2m + memsharing just plain crashes, then that
> should be fixed; and if the lock-ordering parts of the patch fix that,
> then they should be applied.

Yes, that would be a start at least.

  But the code which make sure that the
> same gfn range cannot both be shared and subject to altp2m must remain
> until they interact properly, with all the corner cases carefully
> thought out.

Well, I don't see that what you suggest is going to happen if
incremental improvements are not allowed in. Anyhow, at this point I'm
going to start carrying out-of-tree patches for Xen in my project and
just resign from my mem_sharing maintainership as I feel like it's
pretty pointless.

Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On Fri, Jul 15, 2016 at 3:59 PM, Tamas K Lengyel  wrote:
>> I could go on in the analysis, but the point is that there's a morass
>> of interactions here all of which need to be correct, which this patch
>> does not address.  You have a long way to go before sharing and altp2m
>> can be safely used on the same gfn ranges.
>>
>
> Hi George,
> certainly there are cornercases where if the user does not setup things in
> the right order things can still go out of whack. My goal with this patch is
> not to address all of them. The goal of this patch is to not crash the
> hypervisor when the user at least tries to experiment with it, which is the
> current state. So this patch improves the status quo. Also, both mem_sharing
> and altp2m is considered experimental/tech_preview, so the fact that their
> combination is also experimental should be assumed as well. As I explained,
> with this patch in place there is at least one way they can be safely used
> together if the user tracks unsharing requirements through mem_access and
> does unsharing and fixup of the altp2m views manually. There are other ways
> where it would not be safe as after unsharing we don't know how the user
> would want things to look in altp2ms. I don't think we want to start
> guessing about that either so I will not be looking to implement that. So I
> don't agree with this reasoning being grounds for rejecting this patch that
> does incrementally improve the current state.

So you keep saying "user"; I assume you mean whatever program is
sitting in domain 0, which is going to be doing memsharing, altp2m,
and memaccess stuff all at once?

The altp2m code was not written for that purpose.  It was written for
*guest* administrators to use within the guest.  There's no problem
with finding additional uses for it, as long as the *original* purpose
doesn't get broken; and this patch most certainly does break things
for that purpose.  Guests using altp2m should not have to know that
sharing is happening behind their backs; nor should they be required
to use mem_access to manually fix up what the hypervisor has allowed
to be broken.

If at the moment altp2m + memsharing just plain crashes, then that
should be fixed; and if the lock-ordering parts of the patch fix that,
then they should be applied.  But the code which make sure that the
same gfn range cannot both be shared and subject to altp2m must remain
until they interact properly, with all the corner cases carefully
thought out.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Jul 15, 2016 02:57, "George Dunlap"  wrote:
>
> On Thu, May 26, 2016 at 5:17 PM, Tamas K Lengyel 
wrote:
> >
> > On May 26, 2016 04:40, "George Dunlap"  wrote:
> >>
> >> On 26/05/16 04:55, Tamas K Lengyel wrote:
> >> > Move sharing locks above altp2m to avoid locking order violation.
Allow
> >> > applying altp2m mem_access settings when the hostp2m entries are
shared.
> >> > Also, do not trigger PoD for hostp2m when setting altp2m mem_access
to
> >> > be
> >> > in-line with non-altp2m mem_access path. Also allow gfn remapping
with
> >> > p2m_ram_shared type gfns in altp2m views.
> >> >
> >> > When using mem_sharing in combination with altp2m, unsharing events
> >> > overwrite
> >> > altp2m entries with that of the hostp2m (both remapped entries and
> >> > mem_access
> >> > settings). User should take precaution to not share pages where this
> >> > behavior
> >> > is undesired.
> >>
> >> I'm afraid this is not acceptable.  How could this ever be even
remotely
> >> usable?  If this is a necessary side effect of sharing, then I think
the
> >> original functionality, of un-sharing when setting an altp2m entry is
> >> the only option (and not allowing sharing to happen when an altp2m is
> >> present for a particular gfn).
> >>
> >> Hmm, but actually this also brings up another tricky point: In an
altp2m
> >> you can change the mfn which backs a gfn.  This would need to be
handled
> >> properly in the reverse map, which it doesn't look like it is at the
> >> moment.
> >>
> >> On the whole, I think if you're going to allow a single gfn to be
> >> simultaneously shared and allow an altp2m for it, you're going to need
> >> to do a lot more work.
> >>
> >> (Sorry for not catching a lot of this before...)
> >>
> >
> > Well this patch resolves the locking order violation and allows the
> > xen-access tool's altp2m tests to pass, so it does improve on the
current
> > situation which is a hypervisor crash. To help with the override issue
the
> > user can apply W mem_access permission on the shared hostp2m entries.
That
> > way they get notification through vm_event of the event that leads to
> > unsharing and can then reapply the altp2m changes. So IMHO this patch is
> > already quite workable and while it requires more setup from the
userside,
> > the VMM side is OK with this change.
>
> So correct me if I'm wrong here.
>
> The altp2m stuff was primarily designed for guest operating systems to
> be able to make alternate "views" of their own P2M.  When enabled,
> extra p2ms are allocated which allow a VM to remap a gfn to point to
> the gfn of an mfn in its own address space.
>
> For example, suppose domain 1 host p2m gfns A, B, and C point to mA,
> mB, and mC respectively.  The guest can create an altp2m a1 such that
> gfns O, P, and Q also point to mA, mB, and mC.  The guest can also
> register to receive #VE for violations of an altp2m which are not
> violations under the hostp2m.
>
> mem sharing allows a process in domain 0 (or some other privileged
> domain) to nominate two gfns in different domains to be shared; say,
> domain 1 gfn A and domain 2 gfn X (d1gA and d2gX, respectively).  This
> works by first "nominating" the respective gfns, then sharing them.
> "Nominating" a gfn tells the sharing infrastructure to start tracking
> mappings of that page in a reverse map; after nomination mA's page
> structure will point to d1gA, and mX's page structure will point to
> d2gX.  At that point they can be "shared", at which point (for
> example) both d1gA and d2gX will point to mA, and mA's reverse map
> will contain d1gA and d2gX.
>
> However, the mem sharing code has no visibility into altp2ms.  There
> are two cases we need to consider: the sharing of a gfn for which
> there is another gfn in an altp2m pointing to it (as in the case of
> domain 1 gfn A above -- both hostp2m gfn A and alt2m gfn O point to
> mA), and the sharing of a gfn for which there is an altp2m if that gfn
> pointing to a different gfn (as in the case of domain 1 gfn O above --
> hostp2m gfn O points to mO, altp2m gfn O points to mA).  Then we also
> have to consider the order in which things happen: altp2m then
> sharing, sharing then altp2m.
>
> Let's first consider the case of domain 1 gfn A being shared.  At the
> moment, if the situation described above happens in that order (altp2m
> set up first, then sharing) then the page nomination of d1gA will most
> likely fail because the refcount on mA will be one more than expected.
> If it happened in the reverse order (sharing set up first, then
> altp2m), then setting the altp2m would unshare d1gA.  Both should be
> safe.
>
> Now what happens if we accept your patch as-is?  It looks like altp2m
> first then sharing will behave the same way -- the refcount will be
> too high, so the nomination will fail.  But if you share first and
> then set the altp2m, then the altp2m gfn O will have a reference to mA
> that isn't in 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On Thu, May 26, 2016 at 5:17 PM, Tamas K Lengyel  wrote:
>
> On May 26, 2016 04:40, "George Dunlap"  wrote:
>>
>> On 26/05/16 04:55, Tamas K Lengyel wrote:
>> > Move sharing locks above altp2m to avoid locking order violation. Allow
>> > applying altp2m mem_access settings when the hostp2m entries are shared.
>> > Also, do not trigger PoD for hostp2m when setting altp2m mem_access to
>> > be
>> > in-line with non-altp2m mem_access path. Also allow gfn remapping with
>> > p2m_ram_shared type gfns in altp2m views.
>> >
>> > When using mem_sharing in combination with altp2m, unsharing events
>> > overwrite
>> > altp2m entries with that of the hostp2m (both remapped entries and
>> > mem_access
>> > settings). User should take precaution to not share pages where this
>> > behavior
>> > is undesired.
>>
>> I'm afraid this is not acceptable.  How could this ever be even remotely
>> usable?  If this is a necessary side effect of sharing, then I think the
>> original functionality, of un-sharing when setting an altp2m entry is
>> the only option (and not allowing sharing to happen when an altp2m is
>> present for a particular gfn).
>>
>> Hmm, but actually this also brings up another tricky point: In an altp2m
>> you can change the mfn which backs a gfn.  This would need to be handled
>> properly in the reverse map, which it doesn't look like it is at the
>> moment.
>>
>> On the whole, I think if you're going to allow a single gfn to be
>> simultaneously shared and allow an altp2m for it, you're going to need
>> to do a lot more work.
>>
>> (Sorry for not catching a lot of this before...)
>>
>
> Well this patch resolves the locking order violation and allows the
> xen-access tool's altp2m tests to pass, so it does improve on the current
> situation which is a hypervisor crash. To help with the override issue the
> user can apply W mem_access permission on the shared hostp2m entries. That
> way they get notification through vm_event of the event that leads to
> unsharing and can then reapply the altp2m changes. So IMHO this patch is
> already quite workable and while it requires more setup from the userside,
> the VMM side is OK with this change.

So correct me if I'm wrong here.

The altp2m stuff was primarily designed for guest operating systems to
be able to make alternate "views" of their own P2M.  When enabled,
extra p2ms are allocated which allow a VM to remap a gfn to point to
the gfn of an mfn in its own address space.

For example, suppose domain 1 host p2m gfns A, B, and C point to mA,
mB, and mC respectively.  The guest can create an altp2m a1 such that
gfns O, P, and Q also point to mA, mB, and mC.  The guest can also
register to receive #VE for violations of an altp2m which are not
violations under the hostp2m.

mem sharing allows a process in domain 0 (or some other privileged
domain) to nominate two gfns in different domains to be shared; say,
domain 1 gfn A and domain 2 gfn X (d1gA and d2gX, respectively).  This
works by first "nominating" the respective gfns, then sharing them.
"Nominating" a gfn tells the sharing infrastructure to start tracking
mappings of that page in a reverse map; after nomination mA's page
structure will point to d1gA, and mX's page structure will point to
d2gX.  At that point they can be "shared", at which point (for
example) both d1gA and d2gX will point to mA, and mA's reverse map
will contain d1gA and d2gX.

However, the mem sharing code has no visibility into altp2ms.  There
are two cases we need to consider: the sharing of a gfn for which
there is another gfn in an altp2m pointing to it (as in the case of
domain 1 gfn A above -- both hostp2m gfn A and alt2m gfn O point to
mA), and the sharing of a gfn for which there is an altp2m if that gfn
pointing to a different gfn (as in the case of domain 1 gfn O above --
hostp2m gfn O points to mO, altp2m gfn O points to mA).  Then we also
have to consider the order in which things happen: altp2m then
sharing, sharing then altp2m.

Let's first consider the case of domain 1 gfn A being shared.  At the
moment, if the situation described above happens in that order (altp2m
set up first, then sharing) then the page nomination of d1gA will most
likely fail because the refcount on mA will be one more than expected.
If it happened in the reverse order (sharing set up first, then
altp2m), then setting the altp2m would unshare d1gA.  Both should be
safe.

Now what happens if we accept your patch as-is?  It looks like altp2m
first then sharing will behave the same way -- the refcount will be
too high, so the nomination will fail.  But if you share first and
then set the altp2m, then the altp2m gfn O will have a reference to mA
that isn't in your reverse map.  If you get an unshare on domain 1
altp2m gfn O, it looks to me like you'll get an unshare on *hostp2m*
gfn O, which points to mO, which is *not* shared -- at which point it
looks like you'll BUG().  If you get an unshare on domain 1 gfn A, 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Mon, Jun 13, 2016 at 11:20 AM, Tamas K Lengyel  wrote:
> On Mon, Jun 13, 2016 at 3:28 AM, George Dunlap  
> wrote:
>> On 11/06/16 18:55, Tamas K Lengyel wrote:
>>> On Thu, May 26, 2016 at 10:17 AM, Tamas K Lengyel  
>>> wrote:

 On May 26, 2016 04:40, "George Dunlap"  wrote:
>
> On 26/05/16 04:55, Tamas K Lengyel wrote:
>> Move sharing locks above altp2m to avoid locking order violation. Allow
>> applying altp2m mem_access settings when the hostp2m entries are shared.
>> Also, do not trigger PoD for hostp2m when setting altp2m mem_access to
>> be
>> in-line with non-altp2m mem_access path. Also allow gfn remapping with
>> p2m_ram_shared type gfns in altp2m views.
>>
>> When using mem_sharing in combination with altp2m, unsharing events
>> overwrite
>> altp2m entries with that of the hostp2m (both remapped entries and
>> mem_access
>> settings). User should take precaution to not share pages where this
>> behavior
>> is undesired.
>
> I'm afraid this is not acceptable.  How could this ever be even remotely
> usable?  If this is a necessary side effect of sharing, then I think the
> original functionality, of un-sharing when setting an altp2m entry is
> the only option (and not allowing sharing to happen when an altp2m is
> present for a particular gfn).
>
> Hmm, but actually this also brings up another tricky point: In an altp2m
> you can change the mfn which backs a gfn.  This would need to be handled
> properly in the reverse map, which it doesn't look like it is at the
> moment.
>
> On the whole, I think if you're going to allow a single gfn to be
> simultaneously shared and allow an altp2m for it, you're going to need
> to do a lot more work.
>
> (Sorry for not catching a lot of this before...)
>

 Well this patch resolves the locking order violation and allows the
 xen-access tool's altp2m tests to pass, so it does improve on the current
 situation which is a hypervisor crash. To help with the override issue the
 user can apply W mem_access permission on the shared hostp2m entries. That
 way they get notification through vm_event of the event that leads to
 unsharing and can then reapply the altp2m changes. So IMHO this patch is
 already quite workable and while it requires more setup from the userside,
 the VMM side is OK with this change.
>>>
>>> Ping. Can I get an update on what the verdict is on this patch?
>>
>> To get a proper verdict requires actually taking a deeper look and
>> thinking carefully about things :-)  Sorry for the delay -- hopefully I
>> can get to this this week.
>
> Thanks, no rush ;) Just wanted to keep the discussion on the radar.
> The only caveat with what I proposed above with using mem_access to
> get notified of unsharing is that the mem_access notification is
> currently sent before the unsharing in hvm/hvm.c. This means the user
> getting the mem_access notification has to also do the unsharing
> before reapplying the altp2m changes as well. It can be done so it
> would still work just fine, just has to keep in mind. We could also
> swap around the order of things so unsharing happens before the
> mem_access notification happens but I don't think it's necessary.
>
> Tamas

PATCH ping.

Tamas

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Mon, Jun 13, 2016 at 3:28 AM, George Dunlap  wrote:
> On 11/06/16 18:55, Tamas K Lengyel wrote:
>> On Thu, May 26, 2016 at 10:17 AM, Tamas K Lengyel  
>> wrote:
>>>
>>> On May 26, 2016 04:40, "George Dunlap"  wrote:

 On 26/05/16 04:55, Tamas K Lengyel wrote:
> Move sharing locks above altp2m to avoid locking order violation. Allow
> applying altp2m mem_access settings when the hostp2m entries are shared.
> Also, do not trigger PoD for hostp2m when setting altp2m mem_access to
> be
> in-line with non-altp2m mem_access path. Also allow gfn remapping with
> p2m_ram_shared type gfns in altp2m views.
>
> When using mem_sharing in combination with altp2m, unsharing events
> overwrite
> altp2m entries with that of the hostp2m (both remapped entries and
> mem_access
> settings). User should take precaution to not share pages where this
> behavior
> is undesired.

 I'm afraid this is not acceptable.  How could this ever be even remotely
 usable?  If this is a necessary side effect of sharing, then I think the
 original functionality, of un-sharing when setting an altp2m entry is
 the only option (and not allowing sharing to happen when an altp2m is
 present for a particular gfn).

 Hmm, but actually this also brings up another tricky point: In an altp2m
 you can change the mfn which backs a gfn.  This would need to be handled
 properly in the reverse map, which it doesn't look like it is at the
 moment.

 On the whole, I think if you're going to allow a single gfn to be
 simultaneously shared and allow an altp2m for it, you're going to need
 to do a lot more work.

 (Sorry for not catching a lot of this before...)

>>>
>>> Well this patch resolves the locking order violation and allows the
>>> xen-access tool's altp2m tests to pass, so it does improve on the current
>>> situation which is a hypervisor crash. To help with the override issue the
>>> user can apply W mem_access permission on the shared hostp2m entries. That
>>> way they get notification through vm_event of the event that leads to
>>> unsharing and can then reapply the altp2m changes. So IMHO this patch is
>>> already quite workable and while it requires more setup from the userside,
>>> the VMM side is OK with this change.
>>
>> Ping. Can I get an update on what the verdict is on this patch?
>
> To get a proper verdict requires actually taking a deeper look and
> thinking carefully about things :-)  Sorry for the delay -- hopefully I
> can get to this this week.

Thanks, no rush ;) Just wanted to keep the discussion on the radar.
The only caveat with what I proposed above with using mem_access to
get notified of unsharing is that the mem_access notification is
currently sent before the unsharing in hvm/hvm.c. This means the user
getting the mem_access notification has to also do the unsharing
before reapplying the altp2m changes as well. It can be done so it
would still work just fine, just has to keep in mind. We could also
swap around the order of things so unsharing happens before the
mem_access notification happens but I don't think it's necessary.

Tamas

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

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On 11/06/16 18:55, Tamas K Lengyel wrote:
> On Thu, May 26, 2016 at 10:17 AM, Tamas K Lengyel  wrote:
>>
>> On May 26, 2016 04:40, "George Dunlap"  wrote:
>>>
>>> On 26/05/16 04:55, Tamas K Lengyel wrote:
 Move sharing locks above altp2m to avoid locking order violation. Allow
 applying altp2m mem_access settings when the hostp2m entries are shared.
 Also, do not trigger PoD for hostp2m when setting altp2m mem_access to
 be
 in-line with non-altp2m mem_access path. Also allow gfn remapping with
 p2m_ram_shared type gfns in altp2m views.

 When using mem_sharing in combination with altp2m, unsharing events
 overwrite
 altp2m entries with that of the hostp2m (both remapped entries and
 mem_access
 settings). User should take precaution to not share pages where this
 behavior
 is undesired.
>>>
>>> I'm afraid this is not acceptable.  How could this ever be even remotely
>>> usable?  If this is a necessary side effect of sharing, then I think the
>>> original functionality, of un-sharing when setting an altp2m entry is
>>> the only option (and not allowing sharing to happen when an altp2m is
>>> present for a particular gfn).
>>>
>>> Hmm, but actually this also brings up another tricky point: In an altp2m
>>> you can change the mfn which backs a gfn.  This would need to be handled
>>> properly in the reverse map, which it doesn't look like it is at the
>>> moment.
>>>
>>> On the whole, I think if you're going to allow a single gfn to be
>>> simultaneously shared and allow an altp2m for it, you're going to need
>>> to do a lot more work.
>>>
>>> (Sorry for not catching a lot of this before...)
>>>
>>
>> Well this patch resolves the locking order violation and allows the
>> xen-access tool's altp2m tests to pass, so it does improve on the current
>> situation which is a hypervisor crash. To help with the override issue the
>> user can apply W mem_access permission on the shared hostp2m entries. That
>> way they get notification through vm_event of the event that leads to
>> unsharing and can then reapply the altp2m changes. So IMHO this patch is
>> already quite workable and while it requires more setup from the userside,
>> the VMM side is OK with this change.
> 
> Ping. Can I get an update on what the verdict is on this patch?

To get a proper verdict requires actually taking a deeper look and
thinking carefully about things :-)  Sorry for the delay -- hopefully I
can get to this this week.

Peace,
 -George



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

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On Thu, May 26, 2016 at 10:17 AM, Tamas K Lengyel  wrote:
>
> On May 26, 2016 04:40, "George Dunlap"  wrote:
>>
>> On 26/05/16 04:55, Tamas K Lengyel wrote:
>> > Move sharing locks above altp2m to avoid locking order violation. Allow
>> > applying altp2m mem_access settings when the hostp2m entries are shared.
>> > Also, do not trigger PoD for hostp2m when setting altp2m mem_access to
>> > be
>> > in-line with non-altp2m mem_access path. Also allow gfn remapping with
>> > p2m_ram_shared type gfns in altp2m views.
>> >
>> > When using mem_sharing in combination with altp2m, unsharing events
>> > overwrite
>> > altp2m entries with that of the hostp2m (both remapped entries and
>> > mem_access
>> > settings). User should take precaution to not share pages where this
>> > behavior
>> > is undesired.
>>
>> I'm afraid this is not acceptable.  How could this ever be even remotely
>> usable?  If this is a necessary side effect of sharing, then I think the
>> original functionality, of un-sharing when setting an altp2m entry is
>> the only option (and not allowing sharing to happen when an altp2m is
>> present for a particular gfn).
>>
>> Hmm, but actually this also brings up another tricky point: In an altp2m
>> you can change the mfn which backs a gfn.  This would need to be handled
>> properly in the reverse map, which it doesn't look like it is at the
>> moment.
>>
>> On the whole, I think if you're going to allow a single gfn to be
>> simultaneously shared and allow an altp2m for it, you're going to need
>> to do a lot more work.
>>
>> (Sorry for not catching a lot of this before...)
>>
>
> Well this patch resolves the locking order violation and allows the
> xen-access tool's altp2m tests to pass, so it does improve on the current
> situation which is a hypervisor crash. To help with the override issue the
> user can apply W mem_access permission on the shared hostp2m entries. That
> way they get notification through vm_event of the event that leads to
> unsharing and can then reapply the altp2m changes. So IMHO this patch is
> already quite workable and while it requires more setup from the userside,
> the VMM side is OK with this change.

Ping. Can I get an update on what the verdict is on this patch?

Thanks,
Tamas

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

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

On May 26, 2016 04:40, "George Dunlap"  wrote:
>
> On 26/05/16 04:55, Tamas K Lengyel wrote:
> > Move sharing locks above altp2m to avoid locking order violation. Allow
> > applying altp2m mem_access settings when the hostp2m entries are shared.
> > Also, do not trigger PoD for hostp2m when setting altp2m mem_access to
be
> > in-line with non-altp2m mem_access path. Also allow gfn remapping with
> > p2m_ram_shared type gfns in altp2m views.
> >
> > When using mem_sharing in combination with altp2m, unsharing events
overwrite
> > altp2m entries with that of the hostp2m (both remapped entries and
mem_access
> > settings). User should take precaution to not share pages where this
behavior
> > is undesired.
>
> I'm afraid this is not acceptable.  How could this ever be even remotely
> usable?  If this is a necessary side effect of sharing, then I think the
> original functionality, of un-sharing when setting an altp2m entry is
> the only option (and not allowing sharing to happen when an altp2m is
> present for a particular gfn).
>
> Hmm, but actually this also brings up another tricky point: In an altp2m
> you can change the mfn which backs a gfn.  This would need to be handled
> properly in the reverse map, which it doesn't look like it is at the
moment.
>
> On the whole, I think if you're going to allow a single gfn to be
> simultaneously shared and allow an altp2m for it, you're going to need
> to do a lot more work.
>
> (Sorry for not catching a lot of this before...)
>

Well this patch resolves the locking order violation and allows the
xen-access tool's altp2m tests to pass, so it does improve on the current
situation which is a hypervisor crash. To help with the override issue the
user can apply W mem_access permission on the shared hostp2m entries. That
way they get notification through vm_event of the event that leads to
unsharing and can then reapply the altp2m changes. So IMHO this patch is
already quite workable and while it requires more setup from the userside,
the VMM side is OK with this change.

Tamas

>
> >
> > Signed-off-by: Tamas K Lengyel 
> > ---
> > Cc: George Dunlap 
> > Cc: Jan Beulich 
> > Cc: Andrew Cooper 
> >
> > v4: Resolve locking problem by moving sharing locks above altp2m locks
> > ---
> >  xen/arch/x86/mm/mm-locks.h | 30 +++---
> >  xen/arch/x86/mm/p2m.c  | 10 +-
> >  2 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> > index 086c8bb..74fdfc1 100644
> > --- a/xen/arch/x86/mm/mm-locks.h
> > +++ b/xen/arch/x86/mm/mm-locks.h
> > @@ -242,6 +242,21 @@ declare_mm_lock(nestedp2m)
> >
> >  declare_mm_rwlock(p2m);
> >
> > +/* Sharing per page lock
> > + *
> > + * This is an external lock, not represented by an mm_lock_t. The
memory
> > + * sharing lock uses it to protect addition and removal of (gfn,domain)
> > + * tuples to a shared page. We enforce order here against the p2m lock,
> > + * which is taken after the page_lock to change the gfn's p2m entry.
> > + *
> > + * The lock is recursive because during share we lock two pages. */
> > +
> > +declare_mm_order_constraint(per_page_sharing)
> > +#define page_sharing_mm_pre_lock()
 mm_enforce_order_lock_pre_per_page_sharing()
> > +#define page_sharing_mm_post_lock(l, r) \
> > +mm_enforce_order_lock_post_per_page_sharing((l), (r))
> > +#define page_sharing_mm_unlock(l, r) mm_enforce_order_unlock((l), (r))
> > +
> >  /* Alternate P2M list lock (per-domain)
> >   *
> >   * A per-domain lock that protects the list of alternate p2m's.
> > @@ -287,21 +302,6 @@ declare_mm_rwlock(altp2m);
> >  #define p2m_locked_by_me(p)   mm_write_locked_by_me(&(p)->lock)
> >  #define gfn_locked_by_me(p,g) p2m_locked_by_me(p)
> >
> > -/* Sharing per page lock
> > - *
> > - * This is an external lock, not represented by an mm_lock_t. The
memory
> > - * sharing lock uses it to protect addition and removal of (gfn,domain)
> > - * tuples to a shared page. We enforce order here against the p2m lock,
> > - * which is taken after the page_lock to change the gfn's p2m entry.
> > - *
> > - * The lock is recursive because during share we lock two pages. */
> > -
> > -declare_mm_order_constraint(per_page_sharing)
> > -#define page_sharing_mm_pre_lock()
 mm_enforce_order_lock_pre_per_page_sharing()
> > -#define page_sharing_mm_post_lock(l, r) \
> > -mm_enforce_order_lock_post_per_page_sharing((l), (r))
> > -#define page_sharing_mm_unlock(l, r) mm_enforce_order_unlock((l), (r))
> > -
> >  /* PoD lock (per-p2m-table)
> >   *
> >   * Protects private PoD data structs: entry and cache
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index 9b19769..dc97082 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -1768,10 +1768,10 @@ int p2m_set_altp2m_mem_access(struct domain *d,
struct 

Re: [Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[George Dunlap]

On 26/05/16 04:55, Tamas K Lengyel wrote:
> Move sharing locks above altp2m to avoid locking order violation. Allow
> applying altp2m mem_access settings when the hostp2m entries are shared.
> Also, do not trigger PoD for hostp2m when setting altp2m mem_access to be
> in-line with non-altp2m mem_access path. Also allow gfn remapping with
> p2m_ram_shared type gfns in altp2m views.
> 
> When using mem_sharing in combination with altp2m, unsharing events overwrite
> altp2m entries with that of the hostp2m (both remapped entries and mem_access
> settings). User should take precaution to not share pages where this behavior
> is undesired.

I'm afraid this is not acceptable.  How could this ever be even remotely
usable?  If this is a necessary side effect of sharing, then I think the
original functionality, of un-sharing when setting an altp2m entry is
the only option (and not allowing sharing to happen when an altp2m is
present for a particular gfn).

Hmm, but actually this also brings up another tricky point: In an altp2m
you can change the mfn which backs a gfn.  This would need to be handled
properly in the reverse map, which it doesn't look like it is at the moment.

On the whole, I think if you're going to allow a single gfn to be
simultaneously shared and allow an altp2m for it, you're going to need
to do a lot more work.

(Sorry for not catching a lot of this before...)

 -George

> 
> Signed-off-by: Tamas K Lengyel 
> ---
> Cc: George Dunlap 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> 
> v4: Resolve locking problem by moving sharing locks above altp2m locks
> ---
>  xen/arch/x86/mm/mm-locks.h | 30 +++---
>  xen/arch/x86/mm/p2m.c  | 10 +-
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> index 086c8bb..74fdfc1 100644
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -242,6 +242,21 @@ declare_mm_lock(nestedp2m)
>  
>  declare_mm_rwlock(p2m);
>  
> +/* Sharing per page lock
> + *
> + * This is an external lock, not represented by an mm_lock_t. The memory
> + * sharing lock uses it to protect addition and removal of (gfn,domain)
> + * tuples to a shared page. We enforce order here against the p2m lock,
> + * which is taken after the page_lock to change the gfn's p2m entry.
> + *
> + * The lock is recursive because during share we lock two pages. */
> +
> +declare_mm_order_constraint(per_page_sharing)
> +#define page_sharing_mm_pre_lock()   
> mm_enforce_order_lock_pre_per_page_sharing()
> +#define page_sharing_mm_post_lock(l, r) \
> +mm_enforce_order_lock_post_per_page_sharing((l), (r))
> +#define page_sharing_mm_unlock(l, r) mm_enforce_order_unlock((l), (r))
> +
>  /* Alternate P2M list lock (per-domain)
>   *
>   * A per-domain lock that protects the list of alternate p2m's.
> @@ -287,21 +302,6 @@ declare_mm_rwlock(altp2m);
>  #define p2m_locked_by_me(p)   mm_write_locked_by_me(&(p)->lock)
>  #define gfn_locked_by_me(p,g) p2m_locked_by_me(p)
>  
> -/* Sharing per page lock
> - *
> - * This is an external lock, not represented by an mm_lock_t. The memory
> - * sharing lock uses it to protect addition and removal of (gfn,domain)
> - * tuples to a shared page. We enforce order here against the p2m lock,
> - * which is taken after the page_lock to change the gfn's p2m entry.
> - *
> - * The lock is recursive because during share we lock two pages. */
> -
> -declare_mm_order_constraint(per_page_sharing)
> -#define page_sharing_mm_pre_lock()   
> mm_enforce_order_lock_pre_per_page_sharing()
> -#define page_sharing_mm_post_lock(l, r) \
> -mm_enforce_order_lock_post_per_page_sharing((l), (r))
> -#define page_sharing_mm_unlock(l, r) mm_enforce_order_unlock((l), (r))
> -
>  /* PoD lock (per-p2m-table)
>   * 
>   * Protects private PoD data structs: entry and cache
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 9b19769..dc97082 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1768,10 +1768,10 @@ int p2m_set_altp2m_mem_access(struct domain *d, 
> struct p2m_domain *hp2m,
>  if ( !mfn_valid(mfn) )
>  {
>  mfn = hp2m->get_entry(hp2m, gfn_l, , _a,
> -  P2M_ALLOC | P2M_UNSHARE, _order, NULL);
> +  0, _order, NULL);
>  
>  rc = -ESRCH;
> -if ( !mfn_valid(mfn) || t != p2m_ram_rw )
> +if ( !mfn_valid(mfn) || (t != p2m_ram_rw && t != p2m_ram_shared) )
>  return rc;
>  
>  /* If this is a superpage, copy that first */
> @@ -2542,9 +2542,9 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned 
> int idx,
>  if ( !mfn_valid(mfn) )
>  {
>  mfn = hp2m->get_entry(hp2m, gfn_x(old_gfn), , ,
> -  P2M_ALLOC | P2M_UNSHARE, _order, NULL);
> +  P2M_ALLOC, 

[Xen-devel] [PATCH v4] altp2m: Allow the hostp2m entries to be of type p2m_ram_shared

[Tamas K Lengyel]

Move sharing locks above altp2m to avoid locking order violation. Allow
applying altp2m mem_access settings when the hostp2m entries are shared.
Also, do not trigger PoD for hostp2m when setting altp2m mem_access to be
in-line with non-altp2m mem_access path. Also allow gfn remapping with
p2m_ram_shared type gfns in altp2m views.

When using mem_sharing in combination with altp2m, unsharing events overwrite
altp2m entries with that of the hostp2m (both remapped entries and mem_access
settings). User should take precaution to not share pages where this behavior
is undesired.

Signed-off-by: Tamas K Lengyel 
---
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v4: Resolve locking problem by moving sharing locks above altp2m locks
---
 xen/arch/x86/mm/mm-locks.h | 30 +++---
 xen/arch/x86/mm/p2m.c  | 10 +-
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
index 086c8bb..74fdfc1 100644
--- a/xen/arch/x86/mm/mm-locks.h
+++ b/xen/arch/x86/mm/mm-locks.h
@@ -242,6 +242,21 @@ declare_mm_lock(nestedp2m)
 
 declare_mm_rwlock(p2m);
 
+/* Sharing per page lock
+ *
+ * This is an external lock, not represented by an mm_lock_t. The memory
+ * sharing lock uses it to protect addition and removal of (gfn,domain)
+ * tuples to a shared page. We enforce order here against the p2m lock,
+ * which is taken after the page_lock to change the gfn's p2m entry.
+ *
+ * The lock is recursive because during share we lock two pages. */
+
+declare_mm_order_constraint(per_page_sharing)
+#define page_sharing_mm_pre_lock()   
mm_enforce_order_lock_pre_per_page_sharing()
+#define page_sharing_mm_post_lock(l, r) \
+mm_enforce_order_lock_post_per_page_sharing((l), (r))
+#define page_sharing_mm_unlock(l, r) mm_enforce_order_unlock((l), (r))
+
 /* Alternate P2M list lock (per-domain)
  *
  * A per-domain lock that protects the list of alternate p2m's.
@@ -287,21 +302,6 @@ declare_mm_rwlock(altp2m);
 #define p2m_locked_by_me(p)   mm_write_locked_by_me(&(p)->lock)
 #define gfn_locked_by_me(p,g) p2m_locked_by_me(p)
 
-/* Sharing per page lock
- *
- * This is an external lock, not represented by an mm_lock_t. The memory
- * sharing lock uses it to protect addition and removal of (gfn,domain)
- * tuples to a shared page. We enforce order here against the p2m lock,
- * which is taken after the page_lock to change the gfn's p2m entry.
- *
- * The lock is recursive because during share we lock two pages. */
-
-declare_mm_order_constraint(per_page_sharing)
-#define page_sharing_mm_pre_lock()   
mm_enforce_order_lock_pre_per_page_sharing()
-#define page_sharing_mm_post_lock(l, r) \
-mm_enforce_order_lock_post_per_page_sharing((l), (r))
-#define page_sharing_mm_unlock(l, r) mm_enforce_order_unlock((l), (r))
-
 /* PoD lock (per-p2m-table)
  * 
  * Protects private PoD data structs: entry and cache
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 9b19769..dc97082 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1768,10 +1768,10 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct 
p2m_domain *hp2m,
 if ( !mfn_valid(mfn) )
 {
 mfn = hp2m->get_entry(hp2m, gfn_l, , _a,
-  P2M_ALLOC | P2M_UNSHARE, _order, NULL);
+  0, _order, NULL);
 
 rc = -ESRCH;
-if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+if ( !mfn_valid(mfn) || (t != p2m_ram_rw && t != p2m_ram_shared) )
 return rc;
 
 /* If this is a superpage, copy that first */
@@ -2542,9 +2542,9 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int 
idx,
 if ( !mfn_valid(mfn) )
 {
 mfn = hp2m->get_entry(hp2m, gfn_x(old_gfn), , ,
-  P2M_ALLOC | P2M_UNSHARE, _order, NULL);
+  P2M_ALLOC, _order, NULL);
 
-if ( !mfn_valid(mfn) || t != p2m_ram_rw )
+if ( !mfn_valid(mfn) || (t != p2m_ram_rw && t != p2m_ram_shared) )
 goto out;
 
 /* If this is a superpage, copy that first */
@@ -2567,7 +2567,7 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int 
idx,
 if ( !mfn_valid(mfn) )
 mfn = hp2m->get_entry(hp2m, gfn_x(new_gfn), , , 0, NULL, NULL);
 
-if ( !mfn_valid(mfn) || (t != p2m_ram_rw) )
+if ( !mfn_valid(mfn) || (t != p2m_ram_rw && t != p2m_ram_shared) )
 goto out;
 
 if ( !ap2m->set_entry(ap2m, gfn_x(old_gfn), mfn, PAGE_ORDER_4K, t, a,
-- 
2.8.1


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