* [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
@ 2013-01-12 2:03 Mukesh Rathor
2013-01-14 12:07 ` Jan Beulich
2013-01-24 16:44 ` Tim Deegan
0 siblings, 2 replies; 7+ messages in thread
From: Mukesh Rathor @ 2013-01-12 2:03 UTC (permalink / raw)
To: Xen-devel@lists.xensource.com
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
2013-01-12 2:03 [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi Mukesh Rathor
@ 2013-01-14 12:07 ` Jan Beulich
2013-01-16 1:02 ` Mukesh Rathor
2013-01-24 16:44 ` Tim Deegan
1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2013-01-14 12:07 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: xen-devel
>>> 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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
2013-01-14 12:07 ` Jan Beulich
@ 2013-01-16 1:02 ` Mukesh Rathor
2013-01-16 10:00 ` Jan Beulich
0 siblings, 1 reply; 7+ messages in thread
From: Mukesh Rathor @ 2013-01-16 1:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
2013-01-16 1:02 ` Mukesh Rathor
@ 2013-01-16 10:00 ` Jan Beulich
2013-02-06 0:31 ` Mukesh Rathor
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2013-01-16 10:00 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: xen-devel
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
2013-01-16 10:00 ` Jan Beulich
@ 2013-02-06 0:31 ` Mukesh Rathor
0 siblings, 0 replies; 7+ messages in thread
From: Mukesh Rathor @ 2013-02-06 0:31 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
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-
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
2013-01-12 2:03 [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi Mukesh Rathor
2013-01-14 12:07 ` Jan Beulich
@ 2013-01-24 16:44 ` Tim Deegan
2013-02-01 0:05 ` Mukesh Rathor
1 sibling, 1 reply; 7+ messages in thread
From: Tim Deegan @ 2013-01-24 16:44 UTC (permalink / raw)
To: Mukesh Rathor; +Cc: Xen-devel@lists.xensource.com
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi.
2013-01-24 16:44 ` Tim Deegan
@ 2013-02-01 0:05 ` Mukesh Rathor
0 siblings, 0 replies; 7+ messages in thread
From: Mukesh Rathor @ 2013-02-01 0:05 UTC (permalink / raw)
To: Tim Deegan; +Cc: Xen-devel@lists.xensource.com
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-02-06 0:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-12 2:03 [RFC PATCH 11/16]: PVH xen: some misc changes like mtrr, intr, msi Mukesh Rathor
2013-01-14 12:07 ` Jan Beulich
2013-01-16 1:02 ` Mukesh Rathor
2013-01-16 10:00 ` Jan Beulich
2013-02-06 0:31 ` Mukesh Rathor
2013-01-24 16:44 ` Tim Deegan
2013-02-01 0:05 ` Mukesh Rathor
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).