Xen devel - Jan 2013 - [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.

I could use some feedback here, not sure if I covered everything here. 

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>

diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/hvm/irq.c
--- a/xen/arch/x86/hvm/irq.c	Fri Jan 11 16:34:17 2013 -0800
+++ b/xen/arch/x86/hvm/irq.c	Fri Jan 11 16:35:48 2013 -0800
@@ -405,6 +405,9 @@ struct hvm_intack hvm_vcpu_has_pending_i
          && vcpu_info(v, evtchn_upcall_pending) )
         return hvm_intack_vector(plat->irq.callback_via.vector);
 
+    if ( is_pvh_vcpu(v) )
+        return hvm_intack_none;
+
     if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
         return hvm_intack_pic(0);
 
diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/hvm/mtrr.c
--- a/xen/arch/x86/hvm/mtrr.c	Fri Jan 11 16:34:17 2013 -0800
+++ b/xen/arch/x86/hvm/mtrr.c	Fri Jan 11 16:35:48 2013 -0800
@@ -553,6 +553,9 @@ int32_t hvm_get_mem_pinned_cacheattr(
 
     *type = 0;
 
+    if ( is_pvh_domain(d) )
+        return 0;
+
     if ( !is_hvm_domain(d) )
         return 0;
 
@@ -578,6 +581,9 @@ int32_t hvm_set_mem_pinned_cacheattr(
 {
     struct hvm_mem_pinned_cacheattr_range *range;
 
+    /* PVH note: The guest writes to MSR_IA32_CR_PAT natively */
+    NO_PVH_ASSERT_DOMAIN(d);
+
     if ( !((type == PAT_TYPE_UNCACHABLE) ||
            (type == PAT_TYPE_WRCOMB) ||
            (type == PAT_TYPE_WRTHROUGH) ||
@@ -606,6 +612,7 @@ static int hvm_save_mtrr_msr(struct doma
     struct vcpu *v;
     struct hvm_hw_mtrr hw_mtrr;
     struct mtrr_state *mtrr_state;
+
     /* save mtrr&pat */
     for_each_vcpu(d, v)
     {
@@ -693,7 +700,15 @@ uint8_t epte_get_entry_emt(struct domain
          ((d->vcpu == NULL) || ((v = d->vcpu[0]) == NULL)) )
         return MTRR_TYPE_WRBACK;
 
-    if ( !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
+    /* PVH:fixme: this needs to be studied more */
+    if ( is_pvh_domain(d) ) {
+        if (direct_mmio)
+            return MTRR_TYPE_UNCACHABLE;
+        return MTRR_TYPE_WRBACK;
+    }
+
+    if ( !is_pvh_domain(d) &&
+         !v->domain->arch.hvm_domain.params[HVM_PARAM_IDENT_PT] )
         return MTRR_TYPE_WRBACK;
 
     if ( (v == current) &&
v->domain->arch.hvm_domain.is_in_uc_mode )
diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/hvm/vmx/intr.c
--- a/xen/arch/x86/hvm/vmx/intr.c	Fri Jan 11 16:34:17 2013 -0800
+++ b/xen/arch/x86/hvm/vmx/intr.c	Fri Jan 11 16:35:48 2013 -0800
@@ -216,15 +216,16 @@ void vmx_intr_assist(void)
         return;
     }
 
-    /* Crank the handle on interrupt state. */
-    pt_vector = pt_update_irq(v);
+    if ( !is_pvh_vcpu(v) )
+        /* Crank the handle on interrupt state. */
+        pt_vector = pt_update_irq(v);
 
     do {
         intack = hvm_vcpu_has_pending_irq(v);
         if ( likely(intack.source == hvm_intsrc_none) )
             goto out;
 
-        if ( unlikely(nvmx_intr_intercept(v, intack)) )
+        if ( !is_pvh_vcpu(v) && unlikely(nvmx_intr_intercept(v,
intack)) )
             goto out;
 
         intblk = hvm_interrupt_blocked(v, intack);
diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/msi.c
--- a/xen/arch/x86/msi.c	Fri Jan 11 16:34:17 2013 -0800
+++ b/xen/arch/x86/msi.c	Fri Jan 11 16:35:48 2013 -0800
@@ -766,10 +766,12 @@ static int msix_capability_init(struct p
         WARN_ON(rangeset_overlaps_range(mmio_ro_ranges, dev->msix_pba.first,
                                         dev->msix_pba.last));
 
-        if ( rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
+/* PVH: for now we don''t make the mmio range readonly. See xen-devel
for thread:
+ * "[PVH]: Help: msi.c". When linux msi.c is fixed, pvh check can be
removed */
+        if ( !is_pvh_domain(dev->domain) &&
rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
                                 dev->msix_table.last) )
             WARN();
-        if ( rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
+        if ( !is_pvh_domain(dev->domain) &&
rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
                                 dev->msix_pba.last) )
             WARN();
 
diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/time.c
--- a/xen/arch/x86/time.c	Fri Jan 11 16:34:17 2013 -0800
+++ b/xen/arch/x86/time.c	Fri Jan 11 16:35:48 2013 -0800
@@ -1923,7 +1923,7 @@ void tsc_set_info(struct domain *d,
         break;
     }
     d->arch.incarnation = incarnation + 1;
-    if ( is_hvm_domain(d) )
+    if ( is_hvm_or_pvh_domain(d) )
         hvm_set_rdtsc_exiting(d, d->arch.vtsc);
 }
 
diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/x86_emulate/x86_emulate.c
--- a/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Jan 11 16:34:17 2013 -0800
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Jan 11 16:35:48 2013 -0800
@@ -968,6 +968,10 @@ static int ioport_access_check(
     struct segment_register tr;
     int rc = X86EMUL_OKAY;
 
+    /* PVH should not really get here */
+    /* fixme: need bunch of headers for this assert. check why no headers. */
+    /* NO_PVH_ASSERT_VCPU(current); */
+
     if ( !(ctxt->regs->eflags & EFLG_VM) && mode_iopl() )
         return X86EMUL_OKAY;

>>> On 12.01.13 at 03:03, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/mtrr.c	Fri Jan 11 16:34:17 2013 -0800
> +++ b/xen/arch/x86/hvm/mtrr.c	Fri Jan 11 16:35:48 2013 -0800
> @@ -553,6 +553,9 @@ int32_t hvm_get_mem_pinned_cacheattr(
>  
>      *type = 0;
>  
> +    if ( is_pvh_domain(d) )
> +        return 0;
> +
>      if ( !is_hvm_domain(d) )
>          return 0;
Doesn''t the latter check by itself already do what you want?
> @@ -606,6 +612,7 @@ static int hvm_save_mtrr_msr(struct doma
>      struct vcpu *v;
>      struct hvm_hw_mtrr hw_mtrr;
>      struct mtrr_state *mtrr_state;
> +
>      /* save mtrr&pat */
>      for_each_vcpu(d, v)
>      {
Please drop benign changes like this from this already big patch
series.
> --- a/xen/arch/x86/msi.c	Fri Jan 11 16:34:17 2013 -0800
> +++ b/xen/arch/x86/msi.c	Fri Jan 11 16:35:48 2013 -0800
> @@ -766,10 +766,12 @@ static int msix_capability_init(struct p
>          WARN_ON(rangeset_overlaps_range(mmio_ro_ranges,
dev->msix_pba.first,
>                                          dev->msix_pba.last));
>  
> -        if ( rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
> +/* PVH: for now we don''t make the mmio range readonly. See
xen-devel for thread:
> + * "[PVH]: Help: msi.c". When linux msi.c is fixed, pvh check
can be removed */
> +        if ( !is_pvh_domain(dev->domain) &&
rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
>                                  dev->msix_table.last) )
>              WARN();
> -        if ( rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
> +        if ( !is_pvh_domain(dev->domain) &&
rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
>                                  dev->msix_pba.last) )
>              WARN();
I hope there is no plan for this to go in in this shape.
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Jan 11 16:34:17 2013 -0800
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Jan 11 16:35:48 2013 -0800
> @@ -968,6 +968,10 @@ static int ioport_access_check(
>      struct segment_register tr;
>      int rc = X86EMUL_OKAY;
>  
> +    /* PVH should not really get here */
> +    /* fixme: need bunch of headers for this assert. check why no headers.
*/
Because the emulator is intended to be (almost) standalone, so
building the emulator test (as user space app) is also possible.

Jan
> +    /* NO_PVH_ASSERT_VCPU(current); */
> +
>      if ( !(ctxt->regs->eflags & EFLG_VM) && mode_iopl()
)
>          return X86EMUL_OKAY;

On Mon, 14 Jan 2013 12:07:32 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 12.01.13 at 03:03, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> Doesn''t the latter check by itself already do what you want?
Yup. fixed.
> > @@ -606,6 +612,7 @@ static int hvm_save_mtrr_msr(struct doma
> > +
> >      /* save mtrr&pat */
> >      for_each_vcpu(d, v)
> >      {
> 
> Please drop benign changes like this from this already big patch
> series.
Ok. Undone.
> > --- a/xen/arch/x86/msi.c	Fri Jan 11 16:34:17 2013 -0800
> > +++ b/xen/arch/x86/msi.c	Fri Jan 11 16:35:48 2013 -0800
> > @@ -766,10 +766,12 @@ static int msix_capability_init(struct p
> >          WARN_ON(rangeset_overlaps_range(mmio_ro_ranges,
> > dev->msix_pba.first, dev->msix_pba.last));
> >  
> > -        if ( rangeset_add_range(mmio_ro_ranges,
> > dev->msix_table.first, +/* PVH: for now we don''t make the
mmio
> > range readonly. See xen-devel for thread:
> > + * "[PVH]: Help: msi.c". When linux msi.c is fixed, pvh
check can
> > be removed */
> > +        if ( !is_pvh_domain(dev->domain) &&
> > rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
> > dev->msix_table.last) ) WARN();
> > -        if ( rangeset_add_range(mmio_ro_ranges,
> > dev->msix_pba.first,
> > +        if ( !is_pvh_domain(dev->domain) &&
> > rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
> > dev->msix_pba.last) ) WARN();
> 
> I hope there is no plan for this to go in in this shape.

Can I ifdef it and make it go''able? Ifdef saying PVH is experimental?
Not sure who''s working on the issue on linux side.

> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Jan 11
> > 16:34:17 2013 -0800 +++
> > b/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Jan 11 16:35:48
> > 2013 -0800 @@ -968,6 +968,10 @@ static int
> > ioport_access_check( struct segment_register tr; int rc > >
X86EMUL_OKAY;
> > +    /* PVH should not really get here */
> > +    /* fixme: need bunch of headers for this assert. check why no
> > headers. */
> 
> Because the emulator is intended to be (almost) standalone, so
> building the emulator test (as user space app) is also possible.
Got it, thanks.

>>> On 16.01.13 at 02:02, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Mon, 14 Jan 2013 12:07:32 +0000 "Jan Beulich"
<JBeulich@suse.com> wrote:
>> >>> On 12.01.13 at 03:03, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> > --- a/xen/arch/x86/msi.c	Fri Jan 11 16:34:17 2013 -0800
>> > +++ b/xen/arch/x86/msi.c	Fri Jan 11 16:35:48 2013 -0800
>> > @@ -766,10 +766,12 @@ static int msix_capability_init(struct p
>> >          WARN_ON(rangeset_overlaps_range(mmio_ro_ranges,
>> > dev->msix_pba.first, dev->msix_pba.last));
>> >  
>> > -        if ( rangeset_add_range(mmio_ro_ranges,
>> > dev->msix_table.first, +/* PVH: for now we don''t make
the mmio
>> > range readonly. See xen-devel for thread:
>> > + * "[PVH]: Help: msi.c". When linux msi.c is fixed, pvh
check can
>> > be removed */
>> > +        if ( !is_pvh_domain(dev->domain) &&
>> > rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
>> > dev->msix_table.last) ) WARN();
>> > -        if ( rangeset_add_range(mmio_ro_ranges,
>> > dev->msix_pba.first,
>> > +        if ( !is_pvh_domain(dev->domain) &&
>> > rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
>> > dev->msix_pba.last) ) WARN();
>> 
>> I hope there is no plan for this to go in in this shape.
> 
> 
> Can I ifdef it and make it go''able? Ifdef saying PVH is
experimental?
> Not sure who''s working on the issue on linux side.
No, unless you intend the whole PVH code to become conditional,
default off. You''re widening a known security hole by suppressing
this.

Jan

At 18:03 -0800 on 11 Jan (1357927436), Mukesh Rathor
wrote:
> diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/time.c
> --- a/xen/arch/x86/time.c	Fri Jan 11 16:34:17 2013 -0800
> +++ b/xen/arch/x86/time.c	Fri Jan 11 16:35:48 2013 -0800
> @@ -1923,7 +1923,7 @@ void tsc_set_info(struct domain *d,
>          break;
>      }
>      d->arch.incarnation = incarnation + 1;
> -    if ( is_hvm_domain(d) )
> +    if ( is_hvm_or_pvh_domain(d) )
>          hvm_set_rdtsc_exiting(d, d->arch.vtsc);
>  }
The pvh vmexit handler in the last patch crashes the guest if it gets an
RDTSC interception vmexit, so presumably either something should be
added there or we need to make sure that d->arch.vtsc is never set on a
PVH domain. 

Tim.

On Thu, 24 Jan 2013 16:44:34 +0000
Tim Deegan <tim@xen.org> wrote:
> At 18:03 -0800 on 11 Jan (1357927436), Mukesh Rathor wrote:
> > diff -r 0a38c610f26b -r 33fc5356ad7c xen/arch/x86/time.c
> > --- a/xen/arch/x86/time.c	Fri Jan 11 16:34:17 2013 -0800
> > +++ b/xen/arch/x86/time.c	Fri Jan 11 16:35:48 2013 -0800
> > @@ -1923,7 +1923,7 @@ void tsc_set_info(struct domain *d,
> >          break;
> >      }
> >      d->arch.incarnation = incarnation + 1;
> > -    if ( is_hvm_domain(d) )
> > +    if ( is_hvm_or_pvh_domain(d) )
> >          hvm_set_rdtsc_exiting(d, d->arch.vtsc);
> >  }
> 
> The pvh vmexit handler in the last patch crashes the guest if it gets
> an RDTSC interception vmexit, so presumably either something should be
> added there or we need to make sure that d->arch.vtsc is never set on
> a PVH domain. 
Fixed. For now, making sure vtsc is not set for PVH. But, I added an
action item for Phase II to investigate and add support for it. At
first glance, staring at tsc code for couple hours, I''m still clueless.


Thanks,
Mukesh

On Wed, 16 Jan 2013 10:00:33 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 16.01.13 at 02:02, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > On Mon, 14 Jan 2013 12:07:32 +0000 "Jan Beulich"
> > <JBeulich@suse.com> wrote:
> >> >>> On 12.01.13 at 03:03, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >> > --- a/xen/arch/x86/msi.c	Fri Jan 11 16:34:17 2013 -0800
> >> > +++ b/xen/arch/x86/msi.c	Fri Jan 11 16:35:48 2013 -0800
> >> > @@ -766,10 +766,12 @@ static int msix_capability_init(struct
p
> >> >          WARN_ON(rangeset_overlaps_range(mmio_ro_ranges,
> >> > dev->msix_pba.first, dev->msix_pba.last));
> >> >  
> >> > -        if ( rangeset_add_range(mmio_ro_ranges,
> >> > dev->msix_table.first, +/* PVH: for now we don''t
make the mmio
> >> > range readonly. See xen-devel for thread:
> >> > + * "[PVH]: Help: msi.c". When linux msi.c is
fixed, pvh check
> >> > can be removed */
> >> > +        if ( !is_pvh_domain(dev->domain) &&
> >> > rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
> >> > dev->msix_table.last) ) WARN();
> >> > -        if ( rangeset_add_range(mmio_ro_ranges,
> >> > dev->msix_pba.first,
> >> > +        if ( !is_pvh_domain(dev->domain) &&
> >> > rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
> >> > dev->msix_pba.last) ) WARN();
> >> 
> >> I hope there is no plan for this to go in in this shape.
> > 
> > 
> > Can I ifdef it and make it go''able? Ifdef saying PVH is
> > experimental? Not sure who''s working on the issue on linux
side.
> 
> No, unless you intend the whole PVH code to become conditional,
> default off. You''re widening a known security hole by suppressing
> this.
No, it''s only for PVH that the rangesets are not added, and only
temporary so we''ve something working for PVH in xen, and others
can play with PVH, test, contribute fixes, etc... If it''s a problem, I
can look into disabling MSI for PVH too? If no one picks up the
issue on the linux side, I can start looking at it too after xen patches
for phase I are checked in.

thanks,
M-