Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/29/2015 06:03 AM, Andrew Cooper wrote:
> On 26/06/15 17:30, Ed White wrote:
>> On 06/24/2015 11:19 AM, Andrew Cooper wrote:
>>> On 24/06/15 18:47, Ed White wrote:
>> This looks like some hoop jumping around the assertions in
>> domain_pause() and vcpu_pause().
>>
>> We should probably have some new helpers where the domain needs to be
>> paused, possibly while in context.  The current domain/vcpu_pause() are
>> almost always used where it is definitely not safe to pause in context,
>> hence the assertions.
>>
 It is. I'd be happy to use new helpers, I don't feel qualified to
 write them.

 Ed
>>> Something like this?  Only compile tested.  In the meantime, I have an
>>> optimisation in mind for domain_pause() on domains with large numbers of
>>> vcpus, but that will have to wait a while.
>>>
>>> From: Andrew Cooper 
>>> Date: Wed, 24 Jun 2015 19:06:14 +0100
>>> Subject: [PATCH] common/domain: Helpers to pause a domain while in context
>>>
>>> For use on codepaths which would need to use domain_pause() but might be in
>>> the target domain's context.  In the case that the target domain is in
>>> context,
>>> all other vcpus are paused.
>>>
>>> Signed-off-by: Andrew Cooper 
>>> ---
>>>  xen/common/domain.c |   28 
>>>  xen/include/xen/sched.h |5 +
>>>  2 files changed, 33 insertions(+)
>>>
>>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>>> index 3bc52e6..a1d27e3 100644
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1010,6 +1010,34 @@ int domain_unpause_by_systemcontroller(struct
>>> domain *d)
>>>  return 0;
>>>  }
>>>  
>>> +void domain_pause_except_self(struct domain *d)
>>> +{
>>> +struct vcpu *v, *curr = current;
>>> +
>>> +if ( curr->domain == d )
>>> +{
>>> +for_each_vcpu( d, v )
>>> +if ( likely(v != curr) )
>>> +vcpu_pause(v);
>>> +}
>>> +else
>>> +domain_pause(d);
>>> +}
>>> +
>>> +void domain_unpause_except_self(struct domain *d)
>>> +{
>>> +struct vcpu *v, *curr = current;
>>> +
>>> +if ( curr->domain == d )
>>> +{
>>> +for_each_vcpu( d, v )
>>> +if ( likely(v != curr) )
>>> +vcpu_unpause(v);
>>> +}
>>> +else
>>> +domain_unpause(d);
>>> +}
>>> +
>>>  int vcpu_reset(struct vcpu *v)
>>>  {
>>>  struct domain *d = v->domain;
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index b29d9e7..8e1345a 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -804,6 +804,11 @@ static inline int
>>> domain_pause_by_systemcontroller_nosync(struct domain *d)
>>>  {
>>>  return __domain_pause_by_systemcontroller(d, domain_pause_nosync);
>>>  }
>>> +
>>> +/* domain_pause() but safe against trying to pause current. */
>>> +void domain_pause_except_self(struct domain *d);
>>> +void domain_unpause_except_self(struct domain *d);
>>> +
>>>  void cpu_init(void);
>>>  
>>>  struct scheduler;
>>>
>>>
>> Did you commit this to staging?
> 
> I am not a committer, so couldn't even if I wished to.
> 
>> IOW, can I apply it to my branch
>> and assume it will already be in-tree when our patches are applied?
> 
> You will be the first user of the patch, and as noted, I have only
> compile tested.  Please take it and put it at the start of your series.
> 

Will do. I thought you were all-powerful.

Ed

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Andrew Cooper]

On 26/06/15 17:30, Ed White wrote:
> On 06/24/2015 11:19 AM, Andrew Cooper wrote:
>> On 24/06/15 18:47, Ed White wrote:
> This looks like some hoop jumping around the assertions in
> domain_pause() and vcpu_pause().
>
> We should probably have some new helpers where the domain needs to be
> paused, possibly while in context.  The current domain/vcpu_pause() are
> almost always used where it is definitely not safe to pause in context,
> hence the assertions.
>
>>> It is. I'd be happy to use new helpers, I don't feel qualified to
>>> write them.
>>>
>>> Ed
>> Something like this?  Only compile tested.  In the meantime, I have an
>> optimisation in mind for domain_pause() on domains with large numbers of
>> vcpus, but that will have to wait a while.
>>
>> From: Andrew Cooper 
>> Date: Wed, 24 Jun 2015 19:06:14 +0100
>> Subject: [PATCH] common/domain: Helpers to pause a domain while in context
>>
>> For use on codepaths which would need to use domain_pause() but might be in
>> the target domain's context.  In the case that the target domain is in
>> context,
>> all other vcpus are paused.
>>
>> Signed-off-by: Andrew Cooper 
>> ---
>>  xen/common/domain.c |   28 
>>  xen/include/xen/sched.h |5 +
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 3bc52e6..a1d27e3 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1010,6 +1010,34 @@ int domain_unpause_by_systemcontroller(struct
>> domain *d)
>>  return 0;
>>  }
>>  
>> +void domain_pause_except_self(struct domain *d)
>> +{
>> +struct vcpu *v, *curr = current;
>> +
>> +if ( curr->domain == d )
>> +{
>> +for_each_vcpu( d, v )
>> +if ( likely(v != curr) )
>> +vcpu_pause(v);
>> +}
>> +else
>> +domain_pause(d);
>> +}
>> +
>> +void domain_unpause_except_self(struct domain *d)
>> +{
>> +struct vcpu *v, *curr = current;
>> +
>> +if ( curr->domain == d )
>> +{
>> +for_each_vcpu( d, v )
>> +if ( likely(v != curr) )
>> +vcpu_unpause(v);
>> +}
>> +else
>> +domain_unpause(d);
>> +}
>> +
>>  int vcpu_reset(struct vcpu *v)
>>  {
>>  struct domain *d = v->domain;
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index b29d9e7..8e1345a 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -804,6 +804,11 @@ static inline int
>> domain_pause_by_systemcontroller_nosync(struct domain *d)
>>  {
>>  return __domain_pause_by_systemcontroller(d, domain_pause_nosync);
>>  }
>> +
>> +/* domain_pause() but safe against trying to pause current. */
>> +void domain_pause_except_self(struct domain *d);
>> +void domain_unpause_except_self(struct domain *d);
>> +
>>  void cpu_init(void);
>>  
>>  struct scheduler;
>>
>>
> Did you commit this to staging?

I am not a committer, so couldn't even if I wished to.

> IOW, can I apply it to my branch
> and assume it will already be in-tree when our patches are applied?

You will be the first user of the patch, and as noted, I have only
compile tested.  Please take it and put it at the start of your series.

~Andrew

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/24/2015 11:19 AM, Andrew Cooper wrote:
> On 24/06/15 18:47, Ed White wrote:
 This looks like some hoop jumping around the assertions in
 domain_pause() and vcpu_pause().

 We should probably have some new helpers where the domain needs to be
 paused, possibly while in context.  The current domain/vcpu_pause() are
 almost always used where it is definitely not safe to pause in context,
 hence the assertions.

>> It is. I'd be happy to use new helpers, I don't feel qualified to
>> write them.
>>
>> Ed
> 
> Something like this?  Only compile tested.  In the meantime, I have an
> optimisation in mind for domain_pause() on domains with large numbers of
> vcpus, but that will have to wait a while.
> 
> From: Andrew Cooper 
> Date: Wed, 24 Jun 2015 19:06:14 +0100
> Subject: [PATCH] common/domain: Helpers to pause a domain while in context
> 
> For use on codepaths which would need to use domain_pause() but might be in
> the target domain's context.  In the case that the target domain is in
> context,
> all other vcpus are paused.
> 
> Signed-off-by: Andrew Cooper 
> ---
>  xen/common/domain.c |   28 
>  xen/include/xen/sched.h |5 +
>  2 files changed, 33 insertions(+)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3bc52e6..a1d27e3 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1010,6 +1010,34 @@ int domain_unpause_by_systemcontroller(struct
> domain *d)
>  return 0;
>  }
>  
> +void domain_pause_except_self(struct domain *d)
> +{
> +struct vcpu *v, *curr = current;
> +
> +if ( curr->domain == d )
> +{
> +for_each_vcpu( d, v )
> +if ( likely(v != curr) )
> +vcpu_pause(v);
> +}
> +else
> +domain_pause(d);
> +}
> +
> +void domain_unpause_except_self(struct domain *d)
> +{
> +struct vcpu *v, *curr = current;
> +
> +if ( curr->domain == d )
> +{
> +for_each_vcpu( d, v )
> +if ( likely(v != curr) )
> +vcpu_unpause(v);
> +}
> +else
> +domain_unpause(d);
> +}
> +
>  int vcpu_reset(struct vcpu *v)
>  {
>  struct domain *d = v->domain;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index b29d9e7..8e1345a 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -804,6 +804,11 @@ static inline int
> domain_pause_by_systemcontroller_nosync(struct domain *d)
>  {
>  return __domain_pause_by_systemcontroller(d, domain_pause_nosync);
>  }
> +
> +/* domain_pause() but safe against trying to pause current. */
> +void domain_pause_except_self(struct domain *d);
> +void domain_unpause_except_self(struct domain *d);
> +
>  void cpu_init(void);
>  
>  struct scheduler;
> 
> 
Did you commit this to staging? IOW, can I apply it to my branch
and assume it will already be in-tree when our patches are applied?

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Sahita, Ravi]

On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
> On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
>> On Wed, Jun 24, 2015 at 2:06 PM, Ed White > > wrote:
>> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
>> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
>> >> + unsigned long pfn, xenmem_access_t
>> >> access)
>> >> +{
>> >>
>> >
>> > This function IMHO should be merged with p2m_set_mem_access and should 
>> be
>> > triggerable with the same memop (XENMEM_access_op) hypercall instead of
>> > introducing a new hvmop one.
>>
>> I think we should vote on this. My view is that it makes
>> XENMEM_access_op
>> too complicated to use.
>>
>> The two functions are not very long and share enough code that it 
>> would justify merging. The only big change added is the copy from 
>> host->alt when the entry doesn't exists in alt, and that itself is 
>> pretty self contained. Let's see if we can get a third opinion on it..
> 
> At first sight (I admit I'm rather late in the game and haven't had a 
> chance to follow the series closely from the beginning), the two 
> functions do seem to be mergeable (or at least the common code 
> factored out in static helper functions).
> 
> Also, if Ed's concern is that the libxc API would look unnatural if
> xc_set_mem_access() is used for both purposes, as far as I can tell 
> the only difference could be a non-zero last altp2m parameter, so I 
> agree with you that the less functions doing almost the same thing the 
> better (I have been guilty of this in the past too, for example with 
> my
> xc_enable_introspection() function ;) ).
> 
> So I'd say, yes, if possible merge them.

So here are my reasons why I don't think we should merge the hypercalls, in 
more detail:

Although the two hypercalls are similar, they are not identical. For one thing, 
the existing hypercall can only be used cross-domain whereas the altp2m one can 
be used cross-domain or intra-domain. Also, the existing hypercall can be used 
to modify a range of pages and the new one can only modify a single page, and 
that is intentional.

As I see it, the implementation in hvm.c would become a lot less clean, and 
every direct user of the existing hypercall would have to change for no good 
reason.

Razvan's suggestion to merge the functions that implement the p2m changes I'm 
more ambivalent about. Personally, I prefer not to have code that contains lots 
of conditional logic, which would be the result, but I don't feel that strongly 
about it.

Ed

Ravi> This also has implications for the XSM hooks used for these hypercalls - 
altp2m default policy is to allow for intra-domain , which is not the case for 
XENMEM_access_op - 
Any thoughts on how to manage this difference if we merge them?

Ravi


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/25/2015 03:45 PM, Lengyel, Tamas wrote:
> On Thu, Jun 25, 2015 at 4:46 PM, Ed White  wrote:
> 
>> On 06/25/2015 11:23 AM, Lengyel, Tamas wrote:
>>> On Thu, Jun 25, 2015 at 12:48 PM, Ed White 
>> wrote:
>>>
 On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
> On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
>> On Wed, Jun 24, 2015 at 2:06 PM, Ed White > > wrote:
>> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
>> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t
>> idx,
>> >> + unsigned long pfn,
 xenmem_access_t
>> >> access)
>> >> +{
>> >>
>> >
>> > This function IMHO should be merged with p2m_set_mem_access and
 should be
>> > triggerable with the same memop (XENMEM_access_op) hypercall
 instead of
>> > introducing a new hvmop one.
>>
>> I think we should vote on this. My view is that it makes
>> XENMEM_access_op
>> too complicated to use.
>>
>> The two functions are not very long and share enough code that it
>> would
>> justify merging. The only big change added is the copy from host->alt
>> when the entry doesn't exists in alt, and that itself is pretty self
>> contained. Let's see if we can get a third opinion on it..
>
> At first sight (I admit I'm rather late in the game and haven't had a
> chance to follow the series closely from the beginning), the two
> functions do seem to be mergeable (or at least the common code factored
> out in static helper functions).
>
> Also, if Ed's concern is that the libxc API would look unnatural if
> xc_set_mem_access() is used for both purposes, as far as I can tell the
> only difference could be a non-zero last altp2m parameter, so I agree
> with you that the less functions doing almost the same thing the better
> (I have been guilty of this in the past too, for example with my
> xc_enable_introspection() function ;) ).
>
> So I'd say, yes, if possible merge them.

 So here are my reasons why I don't think we should merge the hypercalls,
 in more detail:

 Although the two hypercalls are similar, they are not identical. For one
 thing, the existing hypercall can only be used cross-domain whereas the
 altp2m one can be used cross-domain or intra-domain.
>>>
>>>
>>> Fair point, the use of rcu_lock_live_remote_domain_by_id in the memaccess
>>> memop handler precludes it working for the intra-domain case. However,
>> now
>>> that we have a valid use-case for it working when a domain applies
>>> restrictions on itself, it would be fine to change that to
>>> rcu_lock_domain_by_any_id. It has been just used as a sanity check. The
>>> code you are using in hvm.c could be abstracted as
>> p2m_altp2m_sanity_check:
>>> "!is_hvm_domain(d) || !hvm_altp2m_supported() || !d->arch.altp2m_active"
>>> and ran when the altp2m field is non-zero to catch buggy tools.
>>
>> Whether or not it's possible to merge the two isn't in dispute. The
>> question is which path results in the easiest to understand and
>> maintain outcome for users of the hypercalls and maintainers of
>> the implementation.
> 
> 
> If it turns out to be that merging the two is too big of a hassle, I would
> agree with Razvan, some code-deduplication would be fine instead of a
> complete merger. I still think it would be cleaner.
> 
> 
>> Having said that, I don't think your check
>> catches an attempt to place an intra-domain restriction on the
>> host p2m with altp2m active.
>>
> 
> The check above I copied from the existing code you do your hvm op. Do you
> explicitly check for that conditions somewhere else? Why not append it you
> need to restrict that condition?
> 

The existing altp2m HVM op can't operate on the host p2m, so I don't
need a check, which I think reinforces the point I'm trying to make:
the code in hvm.c will get spaghetti-like if we go down this route.

>>
>>>
 Also, the existing
 hypercall can be used to modify a range of pages and the new one can
>> only
 modify a single page, and that is intentional.

>>>
>>> Please elaborate on this.
>>
>> In order to keep the p2m's coherent and respect the primacy of the host
>> p2m, changes that occur in the host p2m can cause changes in altp2m's to
>> be lost. At the moment there is not even any notification that has
>> occurred, although that's something I'm working on. The minimum
>> *guaranteed* granularity of that type of altp2m invalidation is
>> an entire altp2m. The more pages you change in an altp2m, the more
>> chance there is of a collision causing an invalidation, so for this
>> version of altp2m we encourage as few changes as possible by requiring
>> a separate hypercall for each page modification.
>>
> 
>> Ed
>>
> 
> OK, but we could check for the condition where npages>1 and an altp2m is
> specified to return -

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Thu, Jun 25, 2015 at 4:46 PM, Ed White  wrote:

> On 06/25/2015 11:23 AM, Lengyel, Tamas wrote:
> > On Thu, Jun 25, 2015 at 12:48 PM, Ed White 
> wrote:
> >
> >> On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
> >>> On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
>  On Wed, Jun 24, 2015 at 2:06 PM, Ed White   > wrote:
>  On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
>  >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t
> idx,
>  >> + unsigned long pfn,
> >> xenmem_access_t
>  >> access)
>  >> +{
>  >>
>  >
>  > This function IMHO should be merged with p2m_set_mem_access and
> >> should be
>  > triggerable with the same memop (XENMEM_access_op) hypercall
> >> instead of
>  > introducing a new hvmop one.
> 
>  I think we should vote on this. My view is that it makes
>  XENMEM_access_op
>  too complicated to use.
> 
>  The two functions are not very long and share enough code that it
> would
>  justify merging. The only big change added is the copy from host->alt
>  when the entry doesn't exists in alt, and that itself is pretty self
>  contained. Let's see if we can get a third opinion on it..
> >>>
> >>> At first sight (I admit I'm rather late in the game and haven't had a
> >>> chance to follow the series closely from the beginning), the two
> >>> functions do seem to be mergeable (or at least the common code factored
> >>> out in static helper functions).
> >>>
> >>> Also, if Ed's concern is that the libxc API would look unnatural if
> >>> xc_set_mem_access() is used for both purposes, as far as I can tell the
> >>> only difference could be a non-zero last altp2m parameter, so I agree
> >>> with you that the less functions doing almost the same thing the better
> >>> (I have been guilty of this in the past too, for example with my
> >>> xc_enable_introspection() function ;) ).
> >>>
> >>> So I'd say, yes, if possible merge them.
> >>
> >> So here are my reasons why I don't think we should merge the hypercalls,
> >> in more detail:
> >>
> >> Although the two hypercalls are similar, they are not identical. For one
> >> thing, the existing hypercall can only be used cross-domain whereas the
> >> altp2m one can be used cross-domain or intra-domain.
> >
> >
> > Fair point, the use of rcu_lock_live_remote_domain_by_id in the memaccess
> > memop handler precludes it working for the intra-domain case. However,
> now
> > that we have a valid use-case for it working when a domain applies
> > restrictions on itself, it would be fine to change that to
> > rcu_lock_domain_by_any_id. It has been just used as a sanity check. The
> > code you are using in hvm.c could be abstracted as
> p2m_altp2m_sanity_check:
> > "!is_hvm_domain(d) || !hvm_altp2m_supported() || !d->arch.altp2m_active"
> > and ran when the altp2m field is non-zero to catch buggy tools.
>
> Whether or not it's possible to merge the two isn't in dispute. The
> question is which path results in the easiest to understand and
> maintain outcome for users of the hypercalls and maintainers of
> the implementation.


If it turns out to be that merging the two is too big of a hassle, I would
agree with Razvan, some code-deduplication would be fine instead of a
complete merger. I still think it would be cleaner.


> Having said that, I don't think your check
> catches an attempt to place an intra-domain restriction on the
> host p2m with altp2m active.
>

The check above I copied from the existing code you do your hvm op. Do you
explicitly check for that conditions somewhere else? Why not append it you
need to restrict that condition?


>
> >
> >> Also, the existing
> >> hypercall can be used to modify a range of pages and the new one can
> only
> >> modify a single page, and that is intentional.
> >>
> >
> > Please elaborate on this.
>
> In order to keep the p2m's coherent and respect the primacy of the host
> p2m, changes that occur in the host p2m can cause changes in altp2m's to
> be lost. At the moment there is not even any notification that has
> occurred, although that's something I'm working on. The minimum
> *guaranteed* granularity of that type of altp2m invalidation is
> an entire altp2m. The more pages you change in an altp2m, the more
> chance there is of a collision causing an invalidation, so for this
> version of altp2m we encourage as few changes as possible by requiring
> a separate hypercall for each page modification.
>

> Ed
>

OK, but we could check for the condition where npages>1 and an altp2m is
specified to return -EOPNOTSUPP. It could be documented in the exposed part
of the API that with altp2m this is a restriction.

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Thu, Jun 25, 2015 at 4:27 PM, Ed White  wrote:

> On 06/25/2015 10:42 AM, Lengyel, Tamas wrote:
> > On Thu, Jun 25, 2015 at 12:31 PM, Ed White 
> wrote:
> >
> >> On 06/24/2015 07:44 PM, Lengyel, Tamas wrote:
>  +if ( altp2m_active )
>  +{
>  +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec,
> &p2m)
> >> ==
>  1 )
>  +{
>  +/* entry was lazily copied from host -- retry */
> 
> >>>
> >>> So I'm not fully following this logic here. I can see that the altp2m
> >> entry
> >>> got copied from the host. Why is there a need for the retry, why not
> just
> >>> continue?
> >>
> >> At this point the EPT's that the hardware is using have been made valid
> >> by software, but the hardware has already failed the access so you have
> >> to restart the operation. This isn't in any way specific to altp2m,
> >> it's how page fault logic works generally.
> >>
> >> Ed
> >>
> >
> > Oh I see, you are working with the assumption that the fault was
> triggered
> > by the entry not being present in the altp2m EPT, thus it's enough to
> copy
> > it to resolve the fault. However, if the hostp2m permissions are
> > restricted, there will be a follow-up fault again. Would it maybe make
> > sense to check for that condition and save having to hit two faults?
>
> It's not an assumption, it's a fact because the altp2m nested page fault
> handler returns 1 IFF it has copied from the host p2m.
>
> Once again this is standard page fault handling. Preemptively checking
> for a condition that would cause another fault shortens the path for
> cases that would re-fault, but lengthens it for all the cases that would
> not. In a typical scenario (which your current experiments are not) you
> expect most cases not to re-fault. The cases that do re-fault are much
> more expensive anyway.
>
> There are other reasons not to preemptively check, but that's the most
> straightforward one.
>
> Ed
>

OK, thanks, makes sense.

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/25/2015 11:23 AM, Lengyel, Tamas wrote:
> On Thu, Jun 25, 2015 at 12:48 PM, Ed White  wrote:
> 
>> On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
>>> On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
 On Wed, Jun 24, 2015 at 2:06 PM, Ed White >>> > wrote:
 On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
 >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
 >> + unsigned long pfn,
>> xenmem_access_t
 >> access)
 >> +{
 >>
 >
 > This function IMHO should be merged with p2m_set_mem_access and
>> should be
 > triggerable with the same memop (XENMEM_access_op) hypercall
>> instead of
 > introducing a new hvmop one.

 I think we should vote on this. My view is that it makes
 XENMEM_access_op
 too complicated to use.

 The two functions are not very long and share enough code that it would
 justify merging. The only big change added is the copy from host->alt
 when the entry doesn't exists in alt, and that itself is pretty self
 contained. Let's see if we can get a third opinion on it..
>>>
>>> At first sight (I admit I'm rather late in the game and haven't had a
>>> chance to follow the series closely from the beginning), the two
>>> functions do seem to be mergeable (or at least the common code factored
>>> out in static helper functions).
>>>
>>> Also, if Ed's concern is that the libxc API would look unnatural if
>>> xc_set_mem_access() is used for both purposes, as far as I can tell the
>>> only difference could be a non-zero last altp2m parameter, so I agree
>>> with you that the less functions doing almost the same thing the better
>>> (I have been guilty of this in the past too, for example with my
>>> xc_enable_introspection() function ;) ).
>>>
>>> So I'd say, yes, if possible merge them.
>>
>> So here are my reasons why I don't think we should merge the hypercalls,
>> in more detail:
>>
>> Although the two hypercalls are similar, they are not identical. For one
>> thing, the existing hypercall can only be used cross-domain whereas the
>> altp2m one can be used cross-domain or intra-domain.
> 
> 
> Fair point, the use of rcu_lock_live_remote_domain_by_id in the memaccess
> memop handler precludes it working for the intra-domain case. However, now
> that we have a valid use-case for it working when a domain applies
> restrictions on itself, it would be fine to change that to
> rcu_lock_domain_by_any_id. It has been just used as a sanity check. The
> code you are using in hvm.c could be abstracted as p2m_altp2m_sanity_check:
> "!is_hvm_domain(d) || !hvm_altp2m_supported() || !d->arch.altp2m_active"
> and ran when the altp2m field is non-zero to catch buggy tools.

Whether or not it's possible to merge the two isn't in dispute. The
question is which path results in the easiest to understand and
maintain outcome for users of the hypercalls and maintainers of
the implementation. Having said that, I don't think your check
catches an attempt to place an intra-domain restriction on the
host p2m with altp2m active.

> 
>> Also, the existing
>> hypercall can be used to modify a range of pages and the new one can only
>> modify a single page, and that is intentional.
>>
> 
> Please elaborate on this.

In order to keep the p2m's coherent and respect the primacy of the host
p2m, changes that occur in the host p2m can cause changes in altp2m's to
be lost. At the moment there is not even any notification that has
occurred, although that's something I'm working on. The minimum
*guaranteed* granularity of that type of altp2m invalidation is
an entire altp2m. The more pages you change in an altp2m, the more
chance there is of a collision causing an invalidation, so for this
version of altp2m we encourage as few changes as possible by requiring
a separate hypercall for each page modification.

Ed

>> As I see it, the implementation in hvm.c would become a lot less clean,
>> and every direct user of the existing hypercall would have to change for
>> no good reason.
>>
> 
> For 4.6 I reworked the entire vm_event/mem_access system, so that is
> already happening irrespective of altp2m. It's fine to add support for the
> altp2m field before 4.6 freezes.
> 
> 
>> Razvan's suggestion to merge the functions that implement the p2m changes
>> I'm more ambivalent about. Personally, I prefer not to have code that
>> contains lots of conditional logic, which would be the result, but I
>> don't feel that strongly about it.
>>
>> Ed
>>
> 
> Thanks,
> Tamas
> 

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/25/2015 10:42 AM, Lengyel, Tamas wrote:
> On Thu, Jun 25, 2015 at 12:31 PM, Ed White  wrote:
> 
>> On 06/24/2015 07:44 PM, Lengyel, Tamas wrote:
 +if ( altp2m_active )
 +{
 +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m)
>> ==
 1 )
 +{
 +/* entry was lazily copied from host -- retry */

>>>
>>> So I'm not fully following this logic here. I can see that the altp2m
>> entry
>>> got copied from the host. Why is there a need for the retry, why not just
>>> continue?
>>
>> At this point the EPT's that the hardware is using have been made valid
>> by software, but the hardware has already failed the access so you have
>> to restart the operation. This isn't in any way specific to altp2m,
>> it's how page fault logic works generally.
>>
>> Ed
>>
> 
> Oh I see, you are working with the assumption that the fault was triggered
> by the entry not being present in the altp2m EPT, thus it's enough to copy
> it to resolve the fault. However, if the hostp2m permissions are
> restricted, there will be a follow-up fault again. Would it maybe make
> sense to check for that condition and save having to hit two faults?

It's not an assumption, it's a fact because the altp2m nested page fault
handler returns 1 IFF it has copied from the host p2m.

Once again this is standard page fault handling. Preemptively checking
for a condition that would cause another fault shortens the path for
cases that would re-fault, but lengthens it for all the cases that would
not. In a typical scenario (which your current experiments are not) you
expect most cases not to re-fault. The cases that do re-fault are much
more expensive anyway.

There are other reasons not to preemptively check, but that's the most
straightforward one.

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Thu, Jun 25, 2015 at 12:48 PM, Ed White  wrote:

> On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
> > On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
> >> On Wed, Jun 24, 2015 at 2:06 PM, Ed White  >> > wrote:
> >> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
> >> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
> >> >> + unsigned long pfn,
> xenmem_access_t
> >> >> access)
> >> >> +{
> >> >>
> >> >
> >> > This function IMHO should be merged with p2m_set_mem_access and
> should be
> >> > triggerable with the same memop (XENMEM_access_op) hypercall
> instead of
> >> > introducing a new hvmop one.
> >>
> >> I think we should vote on this. My view is that it makes
> >> XENMEM_access_op
> >> too complicated to use.
> >>
> >> The two functions are not very long and share enough code that it would
> >> justify merging. The only big change added is the copy from host->alt
> >> when the entry doesn't exists in alt, and that itself is pretty self
> >> contained. Let's see if we can get a third opinion on it..
> >
> > At first sight (I admit I'm rather late in the game and haven't had a
> > chance to follow the series closely from the beginning), the two
> > functions do seem to be mergeable (or at least the common code factored
> > out in static helper functions).
> >
> > Also, if Ed's concern is that the libxc API would look unnatural if
> > xc_set_mem_access() is used for both purposes, as far as I can tell the
> > only difference could be a non-zero last altp2m parameter, so I agree
> > with you that the less functions doing almost the same thing the better
> > (I have been guilty of this in the past too, for example with my
> > xc_enable_introspection() function ;) ).
> >
> > So I'd say, yes, if possible merge them.
>
> So here are my reasons why I don't think we should merge the hypercalls,
> in more detail:
>
> Although the two hypercalls are similar, they are not identical. For one
> thing, the existing hypercall can only be used cross-domain whereas the
> altp2m one can be used cross-domain or intra-domain.


Fair point, the use of rcu_lock_live_remote_domain_by_id in the memaccess
memop handler precludes it working for the intra-domain case. However, now
that we have a valid use-case for it working when a domain applies
restrictions on itself, it would be fine to change that to
rcu_lock_domain_by_any_id. It has been just used as a sanity check. The
code you are using in hvm.c could be abstracted as p2m_altp2m_sanity_check:
"!is_hvm_domain(d) || !hvm_altp2m_supported() || !d->arch.altp2m_active"
and ran when the altp2m field is non-zero to catch buggy tools.


> Also, the existing
> hypercall can be used to modify a range of pages and the new one can only
> modify a single page, and that is intentional.
>

Please elaborate on this.


>
> As I see it, the implementation in hvm.c would become a lot less clean,
> and every direct user of the existing hypercall would have to change for
> no good reason.
>

For 4.6 I reworked the entire vm_event/mem_access system, so that is
already happening irrespective of altp2m. It's fine to add support for the
altp2m field before 4.6 freezes.


> Razvan's suggestion to merge the functions that implement the p2m changes
> I'm more ambivalent about. Personally, I prefer not to have code that
> contains lots of conditional logic, which would be the result, but I
> don't feel that strongly about it.
>
> Ed
>

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Razvan Cojocaru]

On 06/25/2015 07:48 PM, Ed White wrote:
> On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
>> On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
>>> On Wed, Jun 24, 2015 at 2:06 PM, Ed White >> > wrote:
>>> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
>>> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
>>> >> + unsigned long pfn, xenmem_access_t
>>> >> access)
>>> >> +{
>>> >>
>>> >
>>> > This function IMHO should be merged with p2m_set_mem_access and 
>>> should be
>>> > triggerable with the same memop (XENMEM_access_op) hypercall instead 
>>> of
>>> > introducing a new hvmop one.
>>>
>>> I think we should vote on this. My view is that it makes
>>> XENMEM_access_op
>>> too complicated to use.
>>>
>>> The two functions are not very long and share enough code that it would
>>> justify merging. The only big change added is the copy from host->alt
>>> when the entry doesn't exists in alt, and that itself is pretty self
>>> contained. Let's see if we can get a third opinion on it..
>>
>> At first sight (I admit I'm rather late in the game and haven't had a
>> chance to follow the series closely from the beginning), the two
>> functions do seem to be mergeable (or at least the common code factored
>> out in static helper functions).
>>
>> Also, if Ed's concern is that the libxc API would look unnatural if
>> xc_set_mem_access() is used for both purposes, as far as I can tell the
>> only difference could be a non-zero last altp2m parameter, so I agree
>> with you that the less functions doing almost the same thing the better
>> (I have been guilty of this in the past too, for example with my
>> xc_enable_introspection() function ;) ).
>>
>> So I'd say, yes, if possible merge them.
> 
> So here are my reasons why I don't think we should merge the hypercalls,
> in more detail:
> 
> Although the two hypercalls are similar, they are not identical. For one
> thing, the existing hypercall can only be used cross-domain whereas the
> altp2m one can be used cross-domain or intra-domain. Also, the existing
> hypercall can be used to modify a range of pages and the new one can only
> modify a single page, and that is intentional.
> 
> As I see it, the implementation in hvm.c would become a lot less clean,
> and every direct user of the existing hypercall would have to change for
> no good reason.

Thank you for the explanation. While it could be argued that a non-zero
altp2m parameter passed to a merged xc_set_mem_access() could be the
xc_set_altp2m_mem_access() selector, and that the function can then
return EINVAL for parameters that don't fit the semantics of the
selected behaviour, I also don't have a strong aversion to those
functions not being merged. So I'll defer this to Tamas.

> Razvan's suggestion to merge the functions that implement the p2m changes
> I'm more ambivalent about. Personally, I prefer not to have code that
> contains lots of conditional logic, which would be the result, but I
> don't feel that strongly about it.

Well, not necessarily merge the functions, but at least have as much
common code as possible factored out in helper static functions that
both of them call.


Thanks,
Razvan

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Thu, Jun 25, 2015 at 12:31 PM, Ed White  wrote:

> On 06/24/2015 07:44 PM, Lengyel, Tamas wrote:
> >> +if ( altp2m_active )
> >> +{
> >> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m)
> ==
> >> 1 )
> >> +{
> >> +/* entry was lazily copied from host -- retry */
> >>
> >
> > So I'm not fully following this logic here. I can see that the altp2m
> entry
> > got copied from the host. Why is there a need for the retry, why not just
> > continue?
>
> At this point the EPT's that the hardware is using have been made valid
> by software, but the hardware has already failed the access so you have
> to restart the operation. This isn't in any way specific to altp2m,
> it's how page fault logic works generally.
>
> Ed
>

Oh I see, you are working with the assumption that the fault was triggered
by the entry not being present in the altp2m EPT, thus it's enough to copy
it to resolve the fault. However, if the hostp2m permissions are
restricted, there will be a follow-up fault again. Would it maybe make
sense to check for that condition and save having to hit two faults?

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/25/2015 06:40 AM, Razvan Cojocaru wrote:
> On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
>> On Wed, Jun 24, 2015 at 2:06 PM, Ed White > > wrote:
>> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
>> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
>> >> + unsigned long pfn, xenmem_access_t
>> >> access)
>> >> +{
>> >>
>> >
>> > This function IMHO should be merged with p2m_set_mem_access and should 
>> be
>> > triggerable with the same memop (XENMEM_access_op) hypercall instead of
>> > introducing a new hvmop one.
>>
>> I think we should vote on this. My view is that it makes
>> XENMEM_access_op
>> too complicated to use.
>>
>> The two functions are not very long and share enough code that it would
>> justify merging. The only big change added is the copy from host->alt
>> when the entry doesn't exists in alt, and that itself is pretty self
>> contained. Let's see if we can get a third opinion on it..
> 
> At first sight (I admit I'm rather late in the game and haven't had a
> chance to follow the series closely from the beginning), the two
> functions do seem to be mergeable (or at least the common code factored
> out in static helper functions).
> 
> Also, if Ed's concern is that the libxc API would look unnatural if
> xc_set_mem_access() is used for both purposes, as far as I can tell the
> only difference could be a non-zero last altp2m parameter, so I agree
> with you that the less functions doing almost the same thing the better
> (I have been guilty of this in the past too, for example with my
> xc_enable_introspection() function ;) ).
> 
> So I'd say, yes, if possible merge them.

So here are my reasons why I don't think we should merge the hypercalls,
in more detail:

Although the two hypercalls are similar, they are not identical. For one
thing, the existing hypercall can only be used cross-domain whereas the
altp2m one can be used cross-domain or intra-domain. Also, the existing
hypercall can be used to modify a range of pages and the new one can only
modify a single page, and that is intentional.

As I see it, the implementation in hvm.c would become a lot less clean,
and every direct user of the existing hypercall would have to change for
no good reason.

Razvan's suggestion to merge the functions that implement the p2m changes
I'm more ambivalent about. Personally, I prefer not to have code that
contains lots of conditional logic, which would be the result, but I
don't feel that strongly about it.

Ed

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/24/2015 07:44 PM, Lengyel, Tamas wrote:
>> +if ( altp2m_active )
>> +{
>> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) ==
>> 1 )
>> +{
>> +/* entry was lazily copied from host -- retry */
>>
> 
> So I'm not fully following this logic here. I can see that the altp2m entry
> got copied from the host. Why is there a need for the retry, why not just
> continue?

At this point the EPT's that the hardware is using have been made valid
by software, but the hardware has already failed the access so you have
to restart the operation. This isn't in any way specific to altp2m,
it's how page fault logic works generally.

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/25/2015 01:52 AM, Ian Campbell wrote:
> On Wed, 2015-06-24 at 11:06 -0700, Ed White wrote:
>> I think we should vote on this.
> 
> In general we vote on things only when there has been a failure to reach
> consensus. Unless there has been some prior discussion around this issue
> which isn't referenced from the bits of the thread I've looked at then I
> don't think we are at that point.

I didn't mean vote quite as literally as you seem to have read it. I
just meant that IMHO it needs more discussion. I didn't even realize
there was a formal procedure for voting -- if I had I would have been
more careful with my choice of words.

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Razvan Cojocaru]

On 06/25/2015 03:44 PM, Lengyel, Tamas wrote:
> On Wed, Jun 24, 2015 at 2:06 PM, Ed White  > wrote:
> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
> >> + unsigned long pfn, xenmem_access_t
> >> access)
> >> +{
> >>
> >
> > This function IMHO should be merged with p2m_set_mem_access and should 
> be
> > triggerable with the same memop (XENMEM_access_op) hypercall instead of
> > introducing a new hvmop one.
> 
> I think we should vote on this. My view is that it makes
> XENMEM_access_op
> too complicated to use.
> 
> The two functions are not very long and share enough code that it would
> justify merging. The only big change added is the copy from host->alt
> when the entry doesn't exists in alt, and that itself is pretty self
> contained. Let's see if we can get a third opinion on it..

At first sight (I admit I'm rather late in the game and haven't had a
chance to follow the series closely from the beginning), the two
functions do seem to be mergeable (or at least the common code factored
out in static helper functions).

Also, if Ed's concern is that the libxc API would look unnatural if
xc_set_mem_access() is used for both purposes, as far as I can tell the
only difference could be a non-zero last altp2m parameter, so I agree
with you that the less functions doing almost the same thing the better
(I have been guilty of this in the past too, for example with my
xc_enable_introspection() function ;) ).

So I'd say, yes, if possible merge them.


Regards,
Razvan

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Wed, Jun 24, 2015 at 2:06 PM, Ed White  wrote:

> On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
> >> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
> >> + unsigned long pfn, xenmem_access_t
> >> access)
> >> +{
> >>
> >
> > This function IMHO should be merged with p2m_set_mem_access and should be
> > triggerable with the same memop (XENMEM_access_op) hypercall instead of
> > introducing a new hvmop one.
>
> I think we should vote on this. My view is that it makes XENMEM_access_op
> too complicated to use.


The two functions are not very long and share enough code that it would
justify merging. The only big change added is the copy from host->alt when
the entry doesn't exists in alt, and that itself is pretty self contained.
Let's see if we can get a third opinion on it..


> It also makes using this one specific altp2m
> capability different to using any of the others
>

That argument goes both ways - a new mem_access function being introduced
that is different from the others.

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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ian Campbell]

On Wed, 2015-06-24 at 11:06 -0700, Ed White wrote:
> I think we should vote on this.

In general we vote on things only when there has been a failure to reach
consensus. Unless there has been some prior discussion around this issue
which isn't referenced from the bits of the thread I've looked at then I
don't think we are at that point.

Ian.


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Mon, Jun 22, 2015 at 2:56 PM, Ed White  wrote:

> Add the remaining routines required to support enabling the alternate
> p2m functionality.
>
> Signed-off-by: Ed White 
> ---
>  xen/arch/x86/hvm/hvm.c  |  60 +-
>  xen/arch/x86/mm/hap/Makefile|   1 +
>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
>  xen/arch/x86/mm/p2m-ept.c   |   3 +
>  xen/arch/x86/mm/p2m.c   | 405
> 
>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
>  xen/include/asm-x86/p2m.h   |  33 +++
>  7 files changed, 601 insertions(+), 8 deletions(-)
>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d75c12d..b758ee1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>  p2m_access_t p2ma;
>  mfn_t mfn;
>  struct vcpu *v = current;
> -struct p2m_domain *p2m;
> +struct p2m_domain *p2m, *hostp2m;
>  int rc, fall_through = 0, paged = 0;
>  int sharing_enomem = 0;
>  vm_event_request_t *req_ptr = NULL;
> +int altp2m_active = 0;
>
>  /* On Nested Virtualization, walk the guest page table.
>   * If this succeeds, all is fine.
> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>  {
>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec)
> )
>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -rc = 1;
> -goto out;
> +return 1;
>  }
>
> -p2m = p2m_get_hostp2m(v->domain);
> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
> +altp2m_active = altp2mhvm_active(v->domain);
> +
> +/* Take a lock on the host p2m speculatively, to avoid potential
> + * locking order problems later and to handle unshare etc.
> + */
> +hostp2m = p2m_get_hostp2m(v->domain);
> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>P2M_ALLOC | (npfec.write_access ?
> P2M_UNSHARE : 0),
>NULL);
>
> +if ( altp2m_active )
> +{
> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) ==
> 1 )
> +{
> +/* entry was lazily copied from host -- retry */
>

So I'm not fully following this logic here. I can see that the altp2m entry
got copied from the host. Why is there a need for the retry, why not just
continue?


> +__put_gfn(hostp2m, gfn);
> +return 1;
> +}
> +
> +mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> +}
> +else
> +p2m = hostp2m;
> +
>  /* Check access permissions first, then handle faults */
>  if ( mfn_x(mfn) != INVALID_MFN )
>  {
>


-- 

[image: www.novetta.com]

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email  tleng...@novetta.com
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Andrew Cooper]

On 24/06/15 18:47, Ed White wrote:
>> > This looks like some hoop jumping around the assertions in
>> > domain_pause() and vcpu_pause().
>> > 
>> > We should probably have some new helpers where the domain needs to be
>> > paused, possibly while in context.  The current domain/vcpu_pause() are
>> > almost always used where it is definitely not safe to pause in context,
>> > hence the assertions.
>> > 
> It is. I'd be happy to use new helpers, I don't feel qualified to
> write them.
>
> Ed

Something like this?  Only compile tested.  In the meantime, I have an
optimisation in mind for domain_pause() on domains with large numbers of
vcpus, but that will have to wait a while.

From: Andrew Cooper 
Date: Wed, 24 Jun 2015 19:06:14 +0100
Subject: [PATCH] common/domain: Helpers to pause a domain while in context

For use on codepaths which would need to use domain_pause() but might be in
the target domain's context.  In the case that the target domain is in
context,
all other vcpus are paused.

Signed-off-by: Andrew Cooper 
---
 xen/common/domain.c |   28 
 xen/include/xen/sched.h |5 +
 2 files changed, 33 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3bc52e6..a1d27e3 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1010,6 +1010,34 @@ int domain_unpause_by_systemcontroller(struct
domain *d)
 return 0;
 }
 
+void domain_pause_except_self(struct domain *d)
+{
+struct vcpu *v, *curr = current;
+
+if ( curr->domain == d )
+{
+for_each_vcpu( d, v )
+if ( likely(v != curr) )
+vcpu_pause(v);
+}
+else
+domain_pause(d);
+}
+
+void domain_unpause_except_self(struct domain *d)
+{
+struct vcpu *v, *curr = current;
+
+if ( curr->domain == d )
+{
+for_each_vcpu( d, v )
+if ( likely(v != curr) )
+vcpu_unpause(v);
+}
+else
+domain_unpause(d);
+}
+
 int vcpu_reset(struct vcpu *v)
 {
 struct domain *d = v->domain;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b29d9e7..8e1345a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -804,6 +804,11 @@ static inline int
domain_pause_by_systemcontroller_nosync(struct domain *d)
 {
 return __domain_pause_by_systemcontroller(d, domain_pause_nosync);
 }
+
+/* domain_pause() but safe against trying to pause current. */
+void domain_pause_except_self(struct domain *d);
+void domain_unpause_except_self(struct domain *d);
+
 void cpu_init(void);
 
 struct scheduler;



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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/24/2015 09:15 AM, Lengyel, Tamas wrote:
>> +bool_t p2m_set_altp2m_mem_access(struct domain *d, uint16_t idx,
>> + unsigned long pfn, xenmem_access_t
>> access)
>> +{
>>
> 
> This function IMHO should be merged with p2m_set_mem_access and should be
> triggerable with the same memop (XENMEM_access_op) hypercall instead of
> introducing a new hvmop one.

I think we should vote on this. My view is that it makes XENMEM_access_op
too complicated to use. It also makes using this one specific altp2m
capability different to using any of the others -- especially if we adopt
Andrew's suggestion and make all the altp2m ops subops.

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/24/2015 06:46 AM, Andrew Cooper wrote:
> On 22/06/15 19:56, Ed White wrote:
>> Add the remaining routines required to support enabling the alternate
>> p2m functionality.
>>
>> Signed-off-by: Ed White 
>> ---
>>  xen/arch/x86/hvm/hvm.c  |  60 +-
>>  xen/arch/x86/mm/hap/Makefile|   1 +
>>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
>>  xen/arch/x86/mm/p2m-ept.c   |   3 +
>>  xen/arch/x86/mm/p2m.c   | 405 
>> 
>>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
>>  xen/include/asm-x86/p2m.h   |  33 +++
>>  7 files changed, 601 insertions(+), 8 deletions(-)
>>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index d75c12d..b758ee1 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
>> long gla,
>>  p2m_access_t p2ma;
>>  mfn_t mfn;
>>  struct vcpu *v = current;
>> -struct p2m_domain *p2m;
>> +struct p2m_domain *p2m, *hostp2m;
>>  int rc, fall_through = 0, paged = 0;
>>  int sharing_enomem = 0;
>>  vm_event_request_t *req_ptr = NULL;
>> +int altp2m_active = 0;
> 
> bool_t
> 
>>  
>>  /* On Nested Virtualization, walk the guest page table.
>>   * If this succeeds, all is fine.
>> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
>> long gla,
>>  {
>>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
>>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> -rc = 1;
>> -goto out;
>> +return 1;
> 
> What is the justification for skipping the normal out: processing?
> 

At one point in the development of this patch, I had some code after
the out label that assumed at least 1 p2m lock was held. I observed
that at the point above, none of the conditions that require extra
processing after out could be true, so to avoid even more complication
I made the above change. Since the change after out: was later factored
out, the above change is no longer relevant, but it remains true that
none of the conditions requiring extra out: processing can be true here.

>>  }
>>  
>> -p2m = p2m_get_hostp2m(v->domain);
>> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 
>> +altp2m_active = altp2mhvm_active(v->domain);
>> +
>> +/* Take a lock on the host p2m speculatively, to avoid potential
>> + * locking order problems later and to handle unshare etc.
>> + */
>> +hostp2m = p2m_get_hostp2m(v->domain);
>> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>>P2M_ALLOC | (npfec.write_access ? P2M_UNSHARE 
>> : 0),
>>NULL);
>>  
>> +if ( altp2m_active )
>> +{
>> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) == 1 
>> )
>> +{
>> +/* entry was lazily copied from host -- retry */
>> +__put_gfn(hostp2m, gfn);
>> +return 1;
> 
> Again, please don't skip the out: processing.

Same thing. There is no possibility of extra out processing being
required. There is precedent for this: the MMIO bypass skips out
processing.

>> +if ( rv ) {
> 
> Style (brace on new line)
> 
>> +gdprintk(XENLOG_ERR,
>> +"failed to set entry for %#"PRIx64" -> %#"PRIx64"\n",
> 
> It would be useful to know more information, (which altp2m), and to
> prefer gfn over gpa to avoid mixing unqualified linear and frame numbers.

Ack on both counts. I copied this from somewhere else, and in my
private branch I carry a patch which logs much more info.

>> +bool_t p2m_destroy_altp2m_by_id(struct domain *d, uint16_t idx)
>> +{
>> +struct p2m_domain *p2m;
>> +struct vcpu *curr = current;
>> +struct vcpu *v;
>> +bool_t rc = 0;
>> +
>> +if ( !idx || idx > MAX_ALTP2M )
>> +return rc;
>> +
>> +if ( curr->domain != d )
>> +domain_pause(d);
>> +else
>> +for_each_vcpu( d, v )
>> +if ( curr != v )
>> +vcpu_pause(v);
> 
> This looks like some hoop jumping around the assertions in
> domain_pause() and vcpu_pause().
> 
> We should probably have some new helpers where the domain needs to be
> paused, possibly while in context.  The current domain/vcpu_pause() are
> almost always used where it is definitely not safe to pause in context,
> hence the assertions.
> 

It is. I'd be happy to use new helpers, I don't feel qualified to
write them.

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Mon, Jun 22, 2015 at 2:56 PM, Ed White  wrote:

> Add the remaining routines required to support enabling the alternate
> p2m functionality.
>
> Signed-off-by: Ed White 
> ---
>  xen/arch/x86/hvm/hvm.c  |  60 +-
>  xen/arch/x86/mm/hap/Makefile|   1 +
>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
>  xen/arch/x86/mm/p2m-ept.c   |   3 +
>  xen/arch/x86/mm/p2m.c   | 405
> 
>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
>  xen/include/asm-x86/p2m.h   |  33 +++
>  7 files changed, 601 insertions(+), 8 deletions(-)
>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d75c12d..b758ee1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>  p2m_access_t p2ma;
>  mfn_t mfn;
>  struct vcpu *v = current;
> -struct p2m_domain *p2m;
> +struct p2m_domain *p2m, *hostp2m;
>  int rc, fall_through = 0, paged = 0;
>  int sharing_enomem = 0;
>  vm_event_request_t *req_ptr = NULL;
> +int altp2m_active = 0;
>
>  /* On Nested Virtualization, walk the guest page table.
>   * If this succeeds, all is fine.
> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>  {
>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec)
> )
>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -rc = 1;
> -goto out;
> +return 1;
>  }
>
> -p2m = p2m_get_hostp2m(v->domain);
> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
> +altp2m_active = altp2mhvm_active(v->domain);
> +
> +/* Take a lock on the host p2m speculatively, to avoid potential
> + * locking order problems later and to handle unshare etc.
> + */
> +hostp2m = p2m_get_hostp2m(v->domain);
> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>P2M_ALLOC | (npfec.write_access ?
> P2M_UNSHARE : 0),
>NULL);
>
> +if ( altp2m_active )
> +{
> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) ==
> 1 )
> +{
> +/* entry was lazily copied from host -- retry */
> +__put_gfn(hostp2m, gfn);
> +return 1;
> +}
> +
> +mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> +}
> +else
> +p2m = hostp2m;
> +
>  /* Check access permissions first, then handle faults */
>  if ( mfn_x(mfn) != INVALID_MFN )
>  {
> @@ -2893,6 +2912,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
>
>  if ( violation )
>  {
> +/* Should #VE be emulated for this fault? */
> +if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
> +{
> +unsigned int sve;
> +
> +p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL,
> &sve);
> +
> +if ( !sve && ahvm_vcpu_emulate_ve(v) )
> +{
> +rc = 1;
> +goto out_put_gfn;
> +}
> +}
> +
>  if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
>  {
>  fall_through = 1;
> @@ -2912,7 +2945,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
>   (npfec.write_access &&
>(p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
>  {
> -put_gfn(p2m->domain, gfn);
> +__put_gfn(p2m, gfn);
> +if ( altp2m_active )
> +__put_gfn(hostp2m, gfn);
>
>  rc = 0;
>  if ( unlikely(is_pvh_vcpu(v)) )
> @@ -2941,6 +2976,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
>  /* Spurious fault? PoD and log-dirty also take this path. */
>  if ( p2m_is_ram(p2mt) )
>  {
> +rc = 1;
>  /*
>   * Page log dirty is always done with order 0. If this mfn
> resides in
>   * a large page, we do not change other pages type within that
> large
> @@ -2949,9 +2985,15 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
>  if ( npfec.write_access )
>  {
>  paging_mark_dirty(v->domain, mfn_x(mfn));
> +/* If p2m is really an altp2m, unlock here to avoid lock
> ordering
> + * violation when the change below is propagated from host
> p2m */
> +if ( altp2m_active )
> +__put_gfn(p2m, gfn);
>  p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> p2m_ram_rw);
> +__put_gfn(altp2m_active ? hostp2m : p2m, gfn);
> +
> +goto out;
>  }
> -rc = 1;
>  goto out_put_gfn;
>  }
>
> @@ -2961,7 +3003,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
>  rc 

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Andrew Cooper]

On 22/06/15 19:56, Ed White wrote:
> Add the remaining routines required to support enabling the alternate
> p2m functionality.
>
> Signed-off-by: Ed White 
> ---
>  xen/arch/x86/hvm/hvm.c  |  60 +-
>  xen/arch/x86/mm/hap/Makefile|   1 +
>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
>  xen/arch/x86/mm/p2m-ept.c   |   3 +
>  xen/arch/x86/mm/p2m.c   | 405 
> 
>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
>  xen/include/asm-x86/p2m.h   |  33 +++
>  7 files changed, 601 insertions(+), 8 deletions(-)
>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d75c12d..b758ee1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>  p2m_access_t p2ma;
>  mfn_t mfn;
>  struct vcpu *v = current;
> -struct p2m_domain *p2m;
> +struct p2m_domain *p2m, *hostp2m;
>  int rc, fall_through = 0, paged = 0;
>  int sharing_enomem = 0;
>  vm_event_request_t *req_ptr = NULL;
> +int altp2m_active = 0;

bool_t

>  
>  /* On Nested Virtualization, walk the guest page table.
>   * If this succeeds, all is fine.
> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>  {
>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -rc = 1;
> -goto out;
> +return 1;

What is the justification for skipping the normal out: processing?

>  }
>  
> -p2m = p2m_get_hostp2m(v->domain);
> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 
> +altp2m_active = altp2mhvm_active(v->domain);
> +
> +/* Take a lock on the host p2m speculatively, to avoid potential
> + * locking order problems later and to handle unshare etc.
> + */
> +hostp2m = p2m_get_hostp2m(v->domain);
> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>P2M_ALLOC | (npfec.write_access ? P2M_UNSHARE 
> : 0),
>NULL);
>  
> +if ( altp2m_active )
> +{
> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) == 1 )
> +{
> +/* entry was lazily copied from host -- retry */
> +__put_gfn(hostp2m, gfn);
> +return 1;

Again, please don't skip the out: processing.

> +}
> +
> +mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> +}
> +else
> +p2m = hostp2m;
> +
>  /* Check access permissions first, then handle faults */
>  if ( mfn_x(mfn) != INVALID_MFN )
>  {
> @@ -2893,6 +2912,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>  
>  if ( violation )
>  {
> +/* Should #VE be emulated for this fault? */
> +if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
> +{
> +unsigned int sve;
> +
> +p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL, &sve);
> +
> +if ( !sve && ahvm_vcpu_emulate_ve(v) )
> +{
> +rc = 1;
> +goto out_put_gfn;
> +}
> +}
> +
>  if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
>  {
>  fall_through = 1;
> @@ -2912,7 +2945,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>   (npfec.write_access &&
>(p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
>  {
> -put_gfn(p2m->domain, gfn);
> +__put_gfn(p2m, gfn);
> +if ( altp2m_active )
> +__put_gfn(hostp2m, gfn);
>  
>  rc = 0;
>  if ( unlikely(is_pvh_vcpu(v)) )
> @@ -2941,6 +2976,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>  /* Spurious fault? PoD and log-dirty also take this path. */
>  if ( p2m_is_ram(p2mt) )
>  {
> +rc = 1;
>  /*
>   * Page log dirty is always done with order 0. If this mfn resides in
>   * a large page, we do not change other pages type within that large
> @@ -2949,9 +2985,15 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
> long gla,
>  if ( npfec.write_access )
>  {
>  paging_mark_dirty(v->domain, mfn_x(mfn));
> +/* If p2m is really an altp2m, unlock here to avoid lock ordering
> + * violation when the change below is propagated from host p2m */
> +if ( altp2m_active )
> +__put_gfn(p2m, gfn);
>  p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty, 
> p2m_ram_rw);
> +__put_gfn(altp2m_active ? hostp2m : p2m, gfn);
> +
> +goto out;
>  }
> -rc = 1;
>  goto out_pu

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Tue, Jun 23, 2015 at 2:52 PM, Ed White  wrote:

> On 06/23/2015 11:15 AM, Lengyel, Tamas wrote:
> > On Mon, Jun 22, 2015 at 2:56 PM, Ed White 
> wrote:
> >
> >> Add the remaining routines required to support enabling the alternate
> >> p2m functionality.
> >>
> >> Signed-off-by: Ed White 
> >> ---
> >>  xen/arch/x86/hvm/hvm.c  |  60 +-
> >>  xen/arch/x86/mm/hap/Makefile|   1 +
> >>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
> >>  xen/arch/x86/mm/p2m-ept.c   |   3 +
> >>  xen/arch/x86/mm/p2m.c   | 405
> >> 
> >>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
> >>  xen/include/asm-x86/p2m.h   |  33 +++
> >>  7 files changed, 601 insertions(+), 8 deletions(-)
> >>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
> >>
> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >> index d75c12d..b758ee1 100644
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> >> unsigned long gla,
> >>  p2m_access_t p2ma;
> >>  mfn_t mfn;
> >>  struct vcpu *v = current;
> >> -struct p2m_domain *p2m;
> >> +struct p2m_domain *p2m, *hostp2m;
> >>  int rc, fall_through = 0, paged = 0;
> >>  int sharing_enomem = 0;
> >>  vm_event_request_t *req_ptr = NULL;
> >> +int altp2m_active = 0;
> >>
> >>  /* On Nested Virtualization, walk the guest page table.
> >>   * If this succeeds, all is fine.
> >> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> >> unsigned long gla,
> >>  {
> >>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT,
> npfec)
> >> )
> >>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> >> -rc = 1;
> >> -goto out;
> >> +return 1;
> >>  }
> >>
> >> -p2m = p2m_get_hostp2m(v->domain);
> >> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
> >> +altp2m_active = altp2mhvm_active(v->domain);
> >> +
> >> +/* Take a lock on the host p2m speculatively, to avoid potential
> >> + * locking order problems later and to handle unshare etc.
> >> + */
> >> +hostp2m = p2m_get_hostp2m(v->domain);
> >> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
> >>P2M_ALLOC | (npfec.write_access ?
> >> P2M_UNSHARE : 0),
> >>NULL);
> >>
> >> +if ( altp2m_active )
> >> +{
> >> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m)
> ==
> >> 1 )
> >> +{
> >> +/* entry was lazily copied from host -- retry */
> >> +__put_gfn(hostp2m, gfn);
> >> +return 1;
> >> +}
> >> +
> >> +mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> >> +}
> >> +else
> >> +p2m = hostp2m;
> >> +
> >>  /* Check access permissions first, then handle faults */
> >>  if ( mfn_x(mfn) != INVALID_MFN )
> >>  {
> >> @@ -2893,6 +2912,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned
> >> long gla,
> >>
> >>  if ( violation )
> >>  {
> >> +/* Should #VE be emulated for this fault? */
> >> +if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
> >> +{
> >> +unsigned int sve;
> >> +
> >> +p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL,
> >> &sve);
> >> +
> >> +if ( !sve && ahvm_vcpu_emulate_ve(v) )
> >>
> >
> > This line generates the following compile-time error: "hvm.c:2923:51:
> > error: 'v' undeclared (first use in this function)". Did you mean to pass
> > curr to ahvm_vcpu_emulate_ve instead of v?
> >
> > I would recommend doing a compile-test on each patch of the series to
> catch
> > small things like this. Travis-ci has been working really great for me to
> > automate that process (https://github.com/tklengyel/xen/compare/travis)
> ;)
> >
>
> I don't know why you are seeing that error, you can clearly see that v is
> defined and initialised at the start of the containing function, and has
> been used earlier.
>
> I always compile test every patch individually.
>
> Ed
>

Was applying the series to the latest master branch. Things have apparently
changed enough in that frame to cause breakage =) Sorry for the noise.



-- 

[image: www.novetta.com]

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email  tleng...@novetta.com
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

On 06/23/2015 11:15 AM, Lengyel, Tamas wrote:
> On Mon, Jun 22, 2015 at 2:56 PM, Ed White  wrote:
> 
>> Add the remaining routines required to support enabling the alternate
>> p2m functionality.
>>
>> Signed-off-by: Ed White 
>> ---
>>  xen/arch/x86/hvm/hvm.c  |  60 +-
>>  xen/arch/x86/mm/hap/Makefile|   1 +
>>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
>>  xen/arch/x86/mm/p2m-ept.c   |   3 +
>>  xen/arch/x86/mm/p2m.c   | 405
>> 
>>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
>>  xen/include/asm-x86/p2m.h   |  33 +++
>>  7 files changed, 601 insertions(+), 8 deletions(-)
>>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index d75c12d..b758ee1 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> unsigned long gla,
>>  p2m_access_t p2ma;
>>  mfn_t mfn;
>>  struct vcpu *v = current;
>> -struct p2m_domain *p2m;
>> +struct p2m_domain *p2m, *hostp2m;
>>  int rc, fall_through = 0, paged = 0;
>>  int sharing_enomem = 0;
>>  vm_event_request_t *req_ptr = NULL;
>> +int altp2m_active = 0;
>>
>>  /* On Nested Virtualization, walk the guest page table.
>>   * If this succeeds, all is fine.
>> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>> unsigned long gla,
>>  {
>>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec)
>> )
>>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
>> -rc = 1;
>> -goto out;
>> +return 1;
>>  }
>>
>> -p2m = p2m_get_hostp2m(v->domain);
>> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
>> +altp2m_active = altp2mhvm_active(v->domain);
>> +
>> +/* Take a lock on the host p2m speculatively, to avoid potential
>> + * locking order problems later and to handle unshare etc.
>> + */
>> +hostp2m = p2m_get_hostp2m(v->domain);
>> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>>P2M_ALLOC | (npfec.write_access ?
>> P2M_UNSHARE : 0),
>>NULL);
>>
>> +if ( altp2m_active )
>> +{
>> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) ==
>> 1 )
>> +{
>> +/* entry was lazily copied from host -- retry */
>> +__put_gfn(hostp2m, gfn);
>> +return 1;
>> +}
>> +
>> +mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
>> +}
>> +else
>> +p2m = hostp2m;
>> +
>>  /* Check access permissions first, then handle faults */
>>  if ( mfn_x(mfn) != INVALID_MFN )
>>  {
>> @@ -2893,6 +2912,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
>> long gla,
>>
>>  if ( violation )
>>  {
>> +/* Should #VE be emulated for this fault? */
>> +if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
>> +{
>> +unsigned int sve;
>> +
>> +p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL,
>> &sve);
>> +
>> +if ( !sve && ahvm_vcpu_emulate_ve(v) )
>>
> 
> This line generates the following compile-time error: "hvm.c:2923:51:
> error: ‘v’ undeclared (first use in this function)". Did you mean to pass
> curr to ahvm_vcpu_emulate_ve instead of v?
> 
> I would recommend doing a compile-test on each patch of the series to catch
> small things like this. Travis-ci has been working really great for me to
> automate that process (https://github.com/tklengyel/xen/compare/travis) ;)
> 

I don't know why you are seeing that error, you can clearly see that v is
defined and initialised at the start of the containing function, and has
been used earlier.

I always compile test every patch individually.

Ed


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

Re: [Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Lengyel, Tamas]

On Mon, Jun 22, 2015 at 2:56 PM, Ed White  wrote:

> Add the remaining routines required to support enabling the alternate
> p2m functionality.
>
> Signed-off-by: Ed White 
> ---
>  xen/arch/x86/hvm/hvm.c  |  60 +-
>  xen/arch/x86/mm/hap/Makefile|   1 +
>  xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
>  xen/arch/x86/mm/p2m-ept.c   |   3 +
>  xen/arch/x86/mm/p2m.c   | 405
> 
>  xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
>  xen/include/asm-x86/p2m.h   |  33 +++
>  7 files changed, 601 insertions(+), 8 deletions(-)
>  create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d75c12d..b758ee1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>  p2m_access_t p2ma;
>  mfn_t mfn;
>  struct vcpu *v = current;
> -struct p2m_domain *p2m;
> +struct p2m_domain *p2m, *hostp2m;
>  int rc, fall_through = 0, paged = 0;
>  int sharing_enomem = 0;
>  vm_event_request_t *req_ptr = NULL;
> +int altp2m_active = 0;
>
>  /* On Nested Virtualization, walk the guest page table.
>   * If this succeeds, all is fine.
> @@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> unsigned long gla,
>  {
>  if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec)
> )
>  hvm_inject_hw_exception(TRAP_gp_fault, 0);
> -rc = 1;
> -goto out;
> +return 1;
>  }
>
> -p2m = p2m_get_hostp2m(v->domain);
> -mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma,
> +altp2m_active = altp2mhvm_active(v->domain);
> +
> +/* Take a lock on the host p2m speculatively, to avoid potential
> + * locking order problems later and to handle unshare etc.
> + */
> +hostp2m = p2m_get_hostp2m(v->domain);
> +mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
>P2M_ALLOC | (npfec.write_access ?
> P2M_UNSHARE : 0),
>NULL);
>
> +if ( altp2m_active )
> +{
> +if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) ==
> 1 )
> +{
> +/* entry was lazily copied from host -- retry */
> +__put_gfn(hostp2m, gfn);
> +return 1;
> +}
> +
> +mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
> +}
> +else
> +p2m = hostp2m;
> +
>  /* Check access permissions first, then handle faults */
>  if ( mfn_x(mfn) != INVALID_MFN )
>  {
> @@ -2893,6 +2912,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned
> long gla,
>
>  if ( violation )
>  {
> +/* Should #VE be emulated for this fault? */
> +if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
> +{
> +unsigned int sve;
> +
> +p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL,
> &sve);
> +
> +if ( !sve && ahvm_vcpu_emulate_ve(v) )
>

This line generates the following compile-time error: "hvm.c:2923:51:
error: ‘v’ undeclared (first use in this function)". Did you mean to pass
curr to ahvm_vcpu_emulate_ve instead of v?

I would recommend doing a compile-test on each patch of the series to catch
small things like this. Travis-ci has been working really great for me to
automate that process (https://github.com/tklengyel/xen/compare/travis) ;)

-- 

[image: www.novetta.com]

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email  tleng...@novetta.com
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2 09/12] x86/altp2m: add remaining support routines.

[Ed White]

Add the remaining routines required to support enabling the alternate
p2m functionality.

Signed-off-by: Ed White 
---
 xen/arch/x86/hvm/hvm.c  |  60 +-
 xen/arch/x86/mm/hap/Makefile|   1 +
 xen/arch/x86/mm/hap/altp2m_hap.c| 103 +
 xen/arch/x86/mm/p2m-ept.c   |   3 +
 xen/arch/x86/mm/p2m.c   | 405 
 xen/include/asm-x86/hvm/altp2mhvm.h |   4 +
 xen/include/asm-x86/p2m.h   |  33 +++
 7 files changed, 601 insertions(+), 8 deletions(-)
 create mode 100644 xen/arch/x86/mm/hap/altp2m_hap.c

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d75c12d..b758ee1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2786,10 +2786,11 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
long gla,
 p2m_access_t p2ma;
 mfn_t mfn;
 struct vcpu *v = current;
-struct p2m_domain *p2m;
+struct p2m_domain *p2m, *hostp2m;
 int rc, fall_through = 0, paged = 0;
 int sharing_enomem = 0;
 vm_event_request_t *req_ptr = NULL;
+int altp2m_active = 0;
 
 /* On Nested Virtualization, walk the guest page table.
  * If this succeeds, all is fine.
@@ -2845,15 +2846,33 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned 
long gla,
 {
 if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
 hvm_inject_hw_exception(TRAP_gp_fault, 0);
-rc = 1;
-goto out;
+return 1;
 }
 
-p2m = p2m_get_hostp2m(v->domain);
-mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 
+altp2m_active = altp2mhvm_active(v->domain);
+
+/* Take a lock on the host p2m speculatively, to avoid potential
+ * locking order problems later and to handle unshare etc.
+ */
+hostp2m = p2m_get_hostp2m(v->domain);
+mfn = get_gfn_type_access(hostp2m, gfn, &p2mt, &p2ma,
   P2M_ALLOC | (npfec.write_access ? P2M_UNSHARE : 
0),
   NULL);
 
+if ( altp2m_active )
+{
+if ( altp2mhvm_hap_nested_page_fault(v, gpa, gla, npfec, &p2m) == 1 )
+{
+/* entry was lazily copied from host -- retry */
+__put_gfn(hostp2m, gfn);
+return 1;
+}
+
+mfn = get_gfn_type_access(p2m, gfn, &p2mt, &p2ma, 0, NULL);
+}
+else
+p2m = hostp2m;
+
 /* Check access permissions first, then handle faults */
 if ( mfn_x(mfn) != INVALID_MFN )
 {
@@ -2893,6 +2912,20 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
gla,
 
 if ( violation )
 {
+/* Should #VE be emulated for this fault? */
+if ( p2m_is_altp2m(p2m) && !cpu_has_vmx_virt_exceptions )
+{
+unsigned int sve;
+
+p2m->get_entry_full(p2m, gfn, &p2mt, &p2ma, 0, NULL, &sve);
+
+if ( !sve && ahvm_vcpu_emulate_ve(v) )
+{
+rc = 1;
+goto out_put_gfn;
+}
+}
+
 if ( p2m_mem_access_check(gpa, gla, npfec, &req_ptr) )
 {
 fall_through = 1;
@@ -2912,7 +2945,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
gla,
  (npfec.write_access &&
   (p2m_is_discard_write(p2mt) || (p2mt == p2m_mmio_write_dm))) )
 {
-put_gfn(p2m->domain, gfn);
+__put_gfn(p2m, gfn);
+if ( altp2m_active )
+__put_gfn(hostp2m, gfn);
 
 rc = 0;
 if ( unlikely(is_pvh_vcpu(v)) )
@@ -2941,6 +2976,7 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
gla,
 /* Spurious fault? PoD and log-dirty also take this path. */
 if ( p2m_is_ram(p2mt) )
 {
+rc = 1;
 /*
  * Page log dirty is always done with order 0. If this mfn resides in
  * a large page, we do not change other pages type within that large
@@ -2949,9 +2985,15 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
gla,
 if ( npfec.write_access )
 {
 paging_mark_dirty(v->domain, mfn_x(mfn));
+/* If p2m is really an altp2m, unlock here to avoid lock ordering
+ * violation when the change below is propagated from host p2m */
+if ( altp2m_active )
+__put_gfn(p2m, gfn);
 p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty, p2m_ram_rw);
+__put_gfn(altp2m_active ? hostp2m : p2m, gfn);
+
+goto out;
 }
-rc = 1;
 goto out_put_gfn;
 }
 
@@ -2961,7 +3003,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
gla,
 rc = fall_through;
 
 out_put_gfn:
-put_gfn(p2m->domain, gfn);
+__put_gfn(p2m, gfn);
+if ( altp2m_active )
+__put_gfn(hostp2m, gfn);
 out:
 /* All of these are delayed until we exit, since we might 
  * sleep on event ring wait queues, and we must not hold
diff --git a/xen/arch/x86/mm/hap/Makef