[V1 PATCH 0/3] pvh: misc bug fixes

[V1 PATCH 0/3] pvh: misc bug fixes

[Mukesh Rathor]

Resend of previous patch, but broken into multiple patches as
suggested. Please note, this for 4.5, as they seem to affect pvh dom0 
paths only.

Based on c/s: 5224dcca4ac222477d751120fe63c1ed251ad4b1

thanks
mukesh

[V1 PATCH 1/3] pvh: early return from hvm_hap_nested_page_fault

[Mukesh Rathor]

pvh does not support nested hvm at present. As such, return if pvh.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/hvm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 69f7e74..a4a3dcf 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1416,6 +1416,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
     int sharing_enomem = 0;
     mem_event_request_t *req_ptr = NULL;
 
+    if ( is_pvh_vcpu(v) )
+        return 0;
+
     /* On Nested Virtualization, walk the guest page table.
      * If this succeeds, all is fine.
      * If this fails, inject a nested page fault into the guest.
-- 
1.8.3.1

[V1 PATCH 2/3] pvh: fix pirq path for pvh

[Mukesh Rathor]

Just like hvm, pirq eoi shared page is not there for pvh. pvh should
not touch any pv_domain fields.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/irq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index db70077..88444be 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1068,13 +1068,13 @@ bool_t cpu_has_pending_apic_eoi(void)
 
 static inline void set_pirq_eoi(struct domain *d, unsigned int irq)
 {
-    if ( !is_hvm_domain(d) && d->arch.pv_domain.pirq_eoi_map )
+    if ( is_pv_domain(d) && d->arch.pv_domain.pirq_eoi_map )
         set_bit(irq, d->arch.pv_domain.pirq_eoi_map);
 }
 
 static inline void clear_pirq_eoi(struct domain *d, unsigned int irq)
 {
-    if ( !is_hvm_domain(d) && d->arch.pv_domain.pirq_eoi_map )
+    if ( is_pv_domain(d) && d->arch.pv_domain.pirq_eoi_map )
         clear_bit(irq, d->arch.pv_domain.pirq_eoi_map);
 }
 
-- 
1.8.3.1

[V1 PATCH 3/3] pvh: disallow PHYSDEVOP_pirq_eoi_gmfn_v2/v1

[Mukesh Rathor]

A call to do_physdev_op with PHYSDEVOP_pirq_eoi_gmfn_v2/v1 will crash
xen later. Disallow that. Currently, such a path exists for linux
dom0 pvh.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/physdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index bc0634c..9f85857 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -339,6 +339,10 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         unsigned long mfn;
         struct page_info *page;
 
+        ret = -ENOSYS;
+        if ( is_pvh_vcpu(current) )
+            break;
+
         ret = -EFAULT;
         if ( copy_from_guest(&info, arg, 1) != 0 )
             break;
-- 
1.8.3.1

Re: [V1 PATCH 1/3] pvh: early return from hvm_hap_nested_page_fault

[Jan Beulich]

>>> On 25.02.14 at 02:03, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> pvh does not support nested hvm at present. As such, return if pvh.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 69f7e74..a4a3dcf 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1416,6 +1416,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>      int sharing_enomem = 0;
>      mem_event_request_t *req_ptr = NULL;
>  
> +    if ( is_pvh_vcpu(v) )
> +        return 0;
> +

Afaict the "nested" in the function name means "nested paging",
not "nested virtualization", i.e. the function here handles more
than just nested HVM cases. With that, the change appears to be
wrong. What's the motivation for putting such a check here
anyway?

Jan

Re: [V1 PATCH 2/3] pvh: fix pirq path for pvh

[Jan Beulich]

>>> On 25.02.14 at 02:03, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> Just like hvm, pirq eoi shared page is not there for pvh. pvh should
> not touch any pv_domain fields.

While the latter is true, wasn't it that IRQ handling wise PVH is using
PV mechanisms? In which case the EOI map page would be of
interest, and rather than guarding the accesses you ought to move
the field out of pv_domain.

Jan

> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/irq.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index db70077..88444be 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1068,13 +1068,13 @@ bool_t cpu_has_pending_apic_eoi(void)
>  
>  static inline void set_pirq_eoi(struct domain *d, unsigned int irq)
>  {
> -    if ( !is_hvm_domain(d) && d->arch.pv_domain.pirq_eoi_map )
> +    if ( is_pv_domain(d) && d->arch.pv_domain.pirq_eoi_map )
>          set_bit(irq, d->arch.pv_domain.pirq_eoi_map);
>  }
>  
>  static inline void clear_pirq_eoi(struct domain *d, unsigned int irq)
>  {
> -    if ( !is_hvm_domain(d) && d->arch.pv_domain.pirq_eoi_map )
> +    if ( is_pv_domain(d) && d->arch.pv_domain.pirq_eoi_map )
>          clear_bit(irq, d->arch.pv_domain.pirq_eoi_map);
>  }
>  
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [V1 PATCH 2/3] pvh: fix pirq path for pvh

[Mukesh Rathor]

On Tue, 25 Feb 2014 09:28:24 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 25.02.14 at 02:03, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> wrote:
> > Just like hvm, pirq eoi shared page is not there for pvh. pvh should
> > not touch any pv_domain fields.
> 
> While the latter is true, wasn't it that IRQ handling wise PVH is
> using PV mechanisms? In which case the EOI map page would be of
> interest, and rather than guarding the accesses you ought to move
> the field out of pv_domain.

It would be, but my proposals to move the fields out in earlier patches
had been turned down by you. So, while I work on another solution, this
would help. It's on my list to work on. As soon as dom0 pvh patches
are in, I intend to work on fixmes. I don't understand very well whats
going on with pirq eoi, so need to study it both in xen and linux, 
and focussing on one thing at a time really helps mortals like me :) ... 

thanks,
Mukesh

Re: [V1 PATCH 2/3] pvh: fix pirq path for pvh

[Jan Beulich]

>>> On 25.02.14 at 21:44, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> On Tue, 25 Feb 2014 09:28:24 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 25.02.14 at 02:03, Mukesh Rathor <mukesh.rathor@oracle.com>
>> >>> wrote:
>> > Just like hvm, pirq eoi shared page is not there for pvh. pvh should
>> > not touch any pv_domain fields.
>> 
>> While the latter is true, wasn't it that IRQ handling wise PVH is
>> using PV mechanisms? In which case the EOI map page would be of
>> interest, and rather than guarding the accesses you ought to move
>> the field out of pv_domain.
> 
> It would be, but my proposals to move the fields out in earlier patches
> had been turned down by you

I don't think I intentionally turned down any movement of fields
provably useful for PVH. I'm not immune to making mistakes
during patch review, so I'm sorry if I indeed prevented this to be
done right away.

Jan

Re: [V1 PATCH 1/3] pvh: early return from hvm_hap_nested_page_fault

[Tim Deegan]

At 17:03 -0800 on 24 Feb (1393257835), Mukesh Rathor wrote:
> pvh does not support nested hvm at present. As such, return if pvh.

Nack, sorry.  

1: this is the nested pagefault (i.e. EPT/NPT) handler, not part of
the nested HVM code.

2: If there _is_ a problem with the interaction between nested HVM and
PVH, the right way to fix it is to enforce that they can't both be
enabled at the same time, and then make sure all the nested-HVM code
properly checks for being enabled.  It's not a good idea to scatter
PVH checks all over code for unrelated features.

Cheers,

Tim.

> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 69f7e74..a4a3dcf 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1416,6 +1416,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
>      int sharing_enomem = 0;
>      mem_event_request_t *req_ptr = NULL;
>  
> +    if ( is_pvh_vcpu(v) )
> +        return 0;
> +
>      /* On Nested Virtualization, walk the guest page table.
>       * If this succeeds, all is fine.
>       * If this fails, inject a nested page fault into the guest.
> -- 
> 1.8.3.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [V1 PATCH 1/3] pvh: early return from hvm_hap_nested_page_fault

[Mukesh Rathor]

On Thu, 27 Feb 2014 11:56:05 +0100
Tim Deegan <tim@xen.org> wrote:

> At 17:03 -0800 on 24 Feb (1393257835), Mukesh Rathor wrote:
> > pvh does not support nested hvm at present. As such, return if pvh.
> 
> Nack, sorry.  
> 
> 1: this is the nested pagefault (i.e. EPT/NPT) handler, not part of
> the nested HVM code.

yeah, jan already pointed that out. i just quickly whipped it out last
minute after seeing a crash in hvm_hap_nested_page_fault from running
xentrace on pvh. anyways, i'll take a look at it.

> 2: If there _is_ a problem with the interaction between nested HVM and
> PVH, the right way to fix it is to enforce that they can't both be
> enabled at the same time, and then make sure all the nested-HVM code
> properly checks for being enabled.  It's not a good idea to scatter
> PVH checks all over code for unrelated features.

Yup. thanks.

Mueksh

> 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/hvm/hvm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 69f7e74..a4a3dcf 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1416,6 +1416,9 @@ int hvm_hap_nested_page_fault(paddr_t gpa,
> >      int sharing_enomem = 0;
> >      mem_event_request_t *req_ptr = NULL;
> >  
> > +    if ( is_pvh_vcpu(v) )
> > +        return 0;
> > +
> >      /* On Nested Virtualization, walk the guest page table.
> >       * If this succeeds, all is fine.
> >       * If this fails, inject a nested page fault into the guest.
> > -- 
> > 1.8.3.1
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: [V1 PATCH 3/3] pvh: disallow PHYSDEVOP_pirq_eoi_gmfn_v2/v1

[George Dunlap]

On 02/28/2014 08:09 AM, Jan Beulich wrote:
>>>> On 28.02.14 at 04:06, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>> So, sigh, in conclusion, what would you prefer:
>>
>>    a. add this new check to hvm_physdev_op.
>>    b. leave the new check here.
>>    c. add this check to hvm_physdev_op, and move the existing pvh checks
>>       from do_physdev_op to hvm_physdev_op also.
>

>   From a formal pov, all PVH special casing
> should go away from hvm_physdev_op _and_ do_physdev_op,
> with the possibly exception of things that are really unsuitable
> for PVH (in which case it would be more clean to have a distinct
> pvh_physdev_op). Temporary workarounds like the one here
> should obviously(!?) go where other temporary workarounds
> are - with what we have, that's hvm_physdev_op(). As I think
> Tim said elsewhere - sprinkling around arbitrary PVH checks is
> just not acceptable. I begin to regret that I gave my okay for
> all these fixme-s and stuff to go in (with it yet to be seen when
> their elimination would start), because I'm getting the feeling
> that you take this as an excuse to add further such stuff.
>
> So to answer your question above: The way to go depends on
> the long term intended behavior: If there is anything that will
> need special casing no matter what, option d) is likely going to
> be the cleanest one - introduce pvh_physdev_op(). But each
> exception there either needs a proper rationale, or a proper
> fixme annotation. And such special casing should be done with
> future code changes in mind, i.e. considering the cost of
> maintaining all of {do,hvm,pvh}_physdev_op() when new
> (sub-)ops get added.

[Moving back to the main list]

So is the main reason that the checks in do_physdev_op() exist just that 
some of the commands have not yet been implemented for PVH?

If so, I'd much rather have the checks in do_physdev_op().  I think 
long-term, I'd prefer to have them in do_physdev_op() (as Mukesh has 
done here) even if they're going to stay, rather than having an extra 
function in another file with a whitelist / blacklist.  Yes, having the 
checks in do_physdev_op() is a bit ugly, but it should make it clear in 
one place which paths are valid for which kinds of guests.

  -George

Re: [V1 PATCH 3/3] pvh: disallow PHYSDEVOP_pirq_eoi_gmfn_v2/v1

[Jan Beulich]

>>> On 03.03.14 at 13:30, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 02/28/2014 08:09 AM, Jan Beulich wrote:
>>>>> On 28.02.14 at 04:06, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
>>> So, sigh, in conclusion, what would you prefer:
>>>
>>>    a. add this new check to hvm_physdev_op.
>>>    b. leave the new check here.
>>>    c. add this check to hvm_physdev_op, and move the existing pvh checks
>>>       from do_physdev_op to hvm_physdev_op also.
>>
> 
>>   From a formal pov, all PVH special casing
>> should go away from hvm_physdev_op _and_ do_physdev_op,
>> with the possibly exception of things that are really unsuitable
>> for PVH (in which case it would be more clean to have a distinct
>> pvh_physdev_op). Temporary workarounds like the one here
>> should obviously(!?) go where other temporary workarounds
>> are - with what we have, that's hvm_physdev_op(). As I think
>> Tim said elsewhere - sprinkling around arbitrary PVH checks is
>> just not acceptable. I begin to regret that I gave my okay for
>> all these fixme-s and stuff to go in (with it yet to be seen when
>> their elimination would start), because I'm getting the feeling
>> that you take this as an excuse to add further such stuff.
>>
>> So to answer your question above: The way to go depends on
>> the long term intended behavior: If there is anything that will
>> need special casing no matter what, option d) is likely going to
>> be the cleanest one - introduce pvh_physdev_op(). But each
>> exception there either needs a proper rationale, or a proper
>> fixme annotation. And such special casing should be done with
>> future code changes in mind, i.e. considering the cost of
>> maintaining all of {do,hvm,pvh}_physdev_op() when new
>> (sub-)ops get added.
> 
> [Moving back to the main list]
> 
> So is the main reason that the checks in do_physdev_op() exist just that 
> some of the commands have not yet been implemented for PVH?
> 
> If so, I'd much rather have the checks in do_physdev_op().  I think 
> long-term, I'd prefer to have them in do_physdev_op() (as Mukesh has 
> done here) even if they're going to stay, rather than having an extra 
> function in another file with a whitelist / blacklist.  Yes, having the 
> checks in do_physdev_op() is a bit ugly, but it should make it clear in 
> one place which paths are valid for which kinds of guests.

I perhaps wouldn't mind that approach, if there wasn't already a
hvm_physdev_op() wrapper. What I first of all want is that things
get done consistently, so that one doesn't have to go guess why
similar things where done one way once, and then another way the
second time.

But there's restriction to this: Long term special cases I think would
be fine to get placed in the generic routine. Temporary workarounds
otoh seem better placed in a (temporary) wrapper, centralizing the
resulting ugliness.

Jan