Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
On Mon, Aug 27, 2018 at 01:46:10AM -0600, Jan Beulich wrote: > >>> On 26.08.18 at 13:04, wrote: > > On Tue, Aug 21, 2018 at 02:27:40AM -0600, Jan Beulich wrote: > >> > >> > --- a/xen/arch/x86/mm/shadow/multi.c > >> > +++ b/xen/arch/x86/mm/shadow/multi.c > >> > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > >> > } > >> > else > >> > { > >> > +#if CONFIG_HVM > >> > /* Magic MMIO marker: extract gfn for MMIO address */ > >> > ASSERT(sh_l1e_is_mmio(sl1e)); > >> > +ASSERT(is_hvm_vcpu(v)); > >> > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > >> > << PAGE_SHIFT) > >> > | (va & ~PAGE_MASK); > >> > +perfc_incr(shadow_fault_fast_mmio); > >> > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > >> > +sh_reset_early_unshadow(v); > >> > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > >> > +return handle_mmio_with_translation(va, gpa >> > >> > PAGE_SHIFT, > >> > +access) > >> > +? EXCRET_fault_fixed : 0; > >> > +#else > >> > +/* When HVM is not enabled, there shouldn't be MMIO > >> > marker */ > >> > +BUG(); > >> > >> At the example of this, while I agree we shouldn't reach here for PV, > >> can this nevertheless be the less impactful domain_crash() please? > >> > > > > Do you only want this BUG() to be replaced? > > > > I think the two in shadonw_*_emulation should stay because you will only > > ever get NULL pointer deref if you allow the code to continue. > > Did you perhaps remove too much context? From what's left I can't > judge which others you refer to, or what NULL deref you talk about. The BUG()s in shadow_*_emulation, like I mentioned in my reply. What I meant was if I make shadown_init_emulation like: domain_crash(d); return NULL; Nothing good is going to happen. > Looking back at the full patch - I think I had already suggested that > the two shadow_*_emulation() should altogether go inside #ifdef > CONFIG_HVM, not just their bodies. I will see about doing that later this week. (Today is public holiday in UK) Wei. > > Jan > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
>>> On 26.08.18 at 13:04, wrote: > On Tue, Aug 21, 2018 at 02:27:40AM -0600, Jan Beulich wrote: >> >> > --- a/xen/arch/x86/mm/shadow/multi.c >> > +++ b/xen/arch/x86/mm/shadow/multi.c >> > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, >> > } >> > else >> > { >> > +#if CONFIG_HVM >> > /* Magic MMIO marker: extract gfn for MMIO address */ >> > ASSERT(sh_l1e_is_mmio(sl1e)); >> > +ASSERT(is_hvm_vcpu(v)); >> > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e >> > << PAGE_SHIFT) >> > | (va & ~PAGE_MASK); >> > +perfc_incr(shadow_fault_fast_mmio); >> > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); >> > +sh_reset_early_unshadow(v); >> > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); >> > +return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, >> > +access) >> > +? EXCRET_fault_fixed : 0; >> > +#else >> > +/* When HVM is not enabled, there shouldn't be MMIO >> > marker */ >> > +BUG(); >> >> At the example of this, while I agree we shouldn't reach here for PV, >> can this nevertheless be the less impactful domain_crash() please? >> > > Do you only want this BUG() to be replaced? > > I think the two in shadonw_*_emulation should stay because you will only > ever get NULL pointer deref if you allow the code to continue. Did you perhaps remove too much context? From what's left I can't judge which others you refer to, or what NULL deref you talk about. Looking back at the full patch - I think I had already suggested that the two shadow_*_emulation() should altogether go inside #ifdef CONFIG_HVM, not just their bodies. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
On Tue, Aug 21, 2018 at 02:27:40AM -0600, Jan Beulich wrote: > > > --- a/xen/arch/x86/mm/shadow/multi.c > > +++ b/xen/arch/x86/mm/shadow/multi.c > > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > > } > > else > > { > > +#if CONFIG_HVM > > /* Magic MMIO marker: extract gfn for MMIO address */ > > ASSERT(sh_l1e_is_mmio(sl1e)); > > +ASSERT(is_hvm_vcpu(v)); > > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > > << PAGE_SHIFT) > > | (va & ~PAGE_MASK); > > +perfc_incr(shadow_fault_fast_mmio); > > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > > +sh_reset_early_unshadow(v); > > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > > +return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > > +access) > > +? EXCRET_fault_fixed : 0; > > +#else > > +/* When HVM is not enabled, there shouldn't be MMIO marker > > */ > > +BUG(); > > At the example of this, while I agree we shouldn't reach here for PV, > can this nevertheless be the less impactful domain_crash() please? > Do you only want this BUG() to be replaced? I think the two in shadonw_*_emulation should stay because you will only ever get NULL pointer deref if you allow the code to continue. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
On Tue, Aug 21, 2018 at 02:27:40AM -0600, Jan Beulich wrote: > >>> On 17.08.18 at 17:12, wrote: > > @@ -423,6 +426,10 @@ const struct x86_emulate_ops *shadow_init_emulation( > > ? sizeof(sh_ctxt->insn_buf) : 0; > > > > return _shadow_emulator_ops; > > +#else > > +BUG(); > > +return NULL; > > +#endif > > } > > The sole invocation of the function sits right _after_ an is_hvm_...() > conditional (which is odd enough, but presumably a result of the > history of the code). Leaving aside a couple of labels (the goto-s to > which are, I think, provable to be HVM-only as well), that is > preceded by a shadow_mode_refcounts() check (right at the > "emulate" label). I think shadow_mode_refcounts() wants to > become constant false for !HVM, at which point the is_hvm_...() > could even go away (but there's no strict need for this to happen > right away). > > Bottom line - once again the entire function (not just its body) should > be put inside the #ifdef, and then of course the same for > shadow_continue_emulation(). > > > --- a/xen/arch/x86/mm/shadow/multi.c > > +++ b/xen/arch/x86/mm/shadow/multi.c > > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > > } > > else > > { > > +#if CONFIG_HVM > > /* Magic MMIO marker: extract gfn for MMIO address */ > > ASSERT(sh_l1e_is_mmio(sl1e)); > > +ASSERT(is_hvm_vcpu(v)); > > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > > << PAGE_SHIFT) > > | (va & ~PAGE_MASK); > > +perfc_incr(shadow_fault_fast_mmio); > > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > > +sh_reset_early_unshadow(v); > > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > > +return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > > +access) > > +? EXCRET_fault_fixed : 0; > > +#else > > +/* When HVM is not enabled, there shouldn't be MMIO marker > > */ > > +BUG(); > > At the example of this, while I agree we shouldn't reach here for PV, > can this nevertheless be the less impactful domain_crash() please? Since Tim has reviewed this patch, I will submit a follow-up patch for switching to domain_crash. Wei. > > Jan > > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
On Tue, Aug 21, 2018 at 10:29:21AM +0200, Roger Pau Monné wrote: > On Fri, Aug 17, 2018 at 04:12:43PM +0100, Wei Liu wrote: > > Enclose HVM only emulation code under CONFIG_HVM. Add some BUG()s to > > to catch any issue. > > > > Note that although some code checks is_hvm_*, which hints it can be > > called for PV as well, I can't find such paths. > > > > Signed-off-by: Wei Liu > > --- > > xen/arch/x86/mm/shadow/common.c | 18 -- > > xen/arch/x86/mm/shadow/multi.c | 27 +-- > > 2 files changed, 37 insertions(+), 8 deletions(-) > > > > diff --git a/xen/arch/x86/mm/shadow/common.c > > b/xen/arch/x86/mm/shadow/common.c > > index 0856650..4381538 100644 > > --- a/xen/arch/x86/mm/shadow/common.c > > +++ b/xen/arch/x86/mm/shadow/common.c > > @@ -113,6 +113,7 @@ __initcall(shadow_audit_key_init); > > #endif /* SHADOW_AUDIT */ > > > > > > +#if CONFIG_HVM > > > > /**/ > > /* x86 emulator support for the shadow code > > */ > > @@ -380,11 +381,13 @@ static const struct x86_emulate_ops > > hvm_shadow_emulator_ops = { > > .cmpxchg= hvm_emulate_cmpxchg, > > .cpuid = hvmemul_cpuid, > > }; > > +#endif > > > > const struct x86_emulate_ops *shadow_init_emulation( > > struct sh_emulate_ctxt *sh_ctxt, struct cpu_user_regs *regs, > > unsigned int pte_size) > > { > > +#if CONFIG_HVM > > struct segment_register *creg, *sreg; > > struct vcpu *v = current; > > unsigned long addr; > > @@ -423,6 +426,10 @@ const struct x86_emulate_ops *shadow_init_emulation( > > ? sizeof(sh_ctxt->insn_buf) : 0; > > > > return _shadow_emulator_ops; > > +#else > > +BUG(); > > I would usually use ASSERT_UNREACHABLE in such situations. > > And here I wonder whether this cannot be called from a PV path, > AFAICT: > > do_page_fault -> fixup_page_fault -> paging_fault -> page_fault > (sh_page_fault) -> shadow_init_emulation > > But maybe there are other conditions that make this path actually > unreachable (or maybe something in your series changed this path). It can't / shouldn't be reached from PV. See Jan's replies. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
On Tue, Aug 21, 2018 at 02:41:06AM -0600, Jan Beulich wrote: > >>> On 17.08.18 at 17:12, wrote: > > --- a/xen/arch/x86/mm/shadow/multi.c > > +++ b/xen/arch/x86/mm/shadow/multi.c > > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > > } > > else > > { > > +#if CONFIG_HVM > > /* Magic MMIO marker: extract gfn for MMIO address */ > > ASSERT(sh_l1e_is_mmio(sl1e)); > > +ASSERT(is_hvm_vcpu(v)); > > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > > << PAGE_SHIFT) > > | (va & ~PAGE_MASK); > > +perfc_incr(shadow_fault_fast_mmio); > > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > > +sh_reset_early_unshadow(v); > > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > > +return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > > +access) > > +? EXCRET_fault_fixed : 0; > > +#else > > +/* When HVM is not enabled, there shouldn't be MMIO marker > > */ > > +BUG(); > > +#endif > > } > > -perfc_incr(shadow_fault_fast_mmio); > > -SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > > -sh_reset_early_unshadow(v); > > -trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > > -return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > > access) > > -? EXCRET_fault_fixed : 0); > > } > > Actually, while I'm not the maintainer of this code, instead of moving > the code up and increasing indentation, would you mind dropping the > pointless "else" (and decrease indentation of the code in its body)? Done. Wei. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
At 16:12 +0100 on 17 Aug (1534522363), Wei Liu wrote: > Enclose HVM only emulation code under CONFIG_HVM. Add some BUG()s to > to catch any issue. > > Note that although some code checks is_hvm_*, which hints it can be > called for PV as well, I can't find such paths. > > Signed-off-by: Wei Liu Reviewed-by: Tim Deegan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
>>> On 17.08.18 at 17:12, wrote: > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > } > else > { > +#if CONFIG_HVM > /* Magic MMIO marker: extract gfn for MMIO address */ > ASSERT(sh_l1e_is_mmio(sl1e)); > +ASSERT(is_hvm_vcpu(v)); > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > << PAGE_SHIFT) > | (va & ~PAGE_MASK); > +perfc_incr(shadow_fault_fast_mmio); > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > +sh_reset_early_unshadow(v); > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > +return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > +access) > +? EXCRET_fault_fixed : 0; > +#else > +/* When HVM is not enabled, there shouldn't be MMIO marker */ > +BUG(); > +#endif > } > -perfc_incr(shadow_fault_fast_mmio); > -SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > -sh_reset_early_unshadow(v); > -trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > -return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > access) > -? EXCRET_fault_fixed : 0); > } Actually, while I'm not the maintainer of this code, instead of moving the code up and increasing indentation, would you mind dropping the pointless "else" (and decrease indentation of the code in its body)? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
On Fri, Aug 17, 2018 at 04:12:43PM +0100, Wei Liu wrote: > Enclose HVM only emulation code under CONFIG_HVM. Add some BUG()s to > to catch any issue. > > Note that although some code checks is_hvm_*, which hints it can be > called for PV as well, I can't find such paths. > > Signed-off-by: Wei Liu > --- > xen/arch/x86/mm/shadow/common.c | 18 -- > xen/arch/x86/mm/shadow/multi.c | 27 +-- > 2 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c > index 0856650..4381538 100644 > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -113,6 +113,7 @@ __initcall(shadow_audit_key_init); > #endif /* SHADOW_AUDIT */ > > > +#if CONFIG_HVM > /**/ > /* x86 emulator support for the shadow code > */ > @@ -380,11 +381,13 @@ static const struct x86_emulate_ops > hvm_shadow_emulator_ops = { > .cmpxchg= hvm_emulate_cmpxchg, > .cpuid = hvmemul_cpuid, > }; > +#endif > > const struct x86_emulate_ops *shadow_init_emulation( > struct sh_emulate_ctxt *sh_ctxt, struct cpu_user_regs *regs, > unsigned int pte_size) > { > +#if CONFIG_HVM > struct segment_register *creg, *sreg; > struct vcpu *v = current; > unsigned long addr; > @@ -423,6 +426,10 @@ const struct x86_emulate_ops *shadow_init_emulation( > ? sizeof(sh_ctxt->insn_buf) : 0; > > return _shadow_emulator_ops; > +#else > +BUG(); I would usually use ASSERT_UNREACHABLE in such situations. And here I wonder whether this cannot be called from a PV path, AFAICT: do_page_fault -> fixup_page_fault -> paging_fault -> page_fault (sh_page_fault) -> shadow_init_emulation But maybe there are other conditions that make this path actually unreachable (or maybe something in your series changed this path). > +return NULL; > +#endif > } > > /* Update an initialized emulation context to prepare for the next > @@ -430,6 +437,7 @@ const struct x86_emulate_ops *shadow_init_emulation( > void shadow_continue_emulation(struct sh_emulate_ctxt *sh_ctxt, > struct cpu_user_regs *regs) > { > +#if CONFIG_HVM > unsigned long addr, diff; > > ASSERT(is_hvm_vcpu(current)); > @@ -451,6 +459,9 @@ void shadow_continue_emulation(struct sh_emulate_ctxt > *sh_ctxt, > ? sizeof(sh_ctxt->insn_buf) : 0; > sh_ctxt->insn_buf_eip = regs->rip; > } > +#else > +BUG(); > +#endif > } > > > @@ -1685,6 +1696,7 @@ static unsigned int shadow_get_allocation(struct domain > *d) > + ((pg & ((1 << (20 - PAGE_SHIFT)) - 1)) ? 1 : 0)); > } > > +#if CONFIG_HVM > /**/ > /* Handling guest writes to pagetables. */ > > @@ -1957,6 +1969,7 @@ static void sh_emulate_unmap_dest(struct vcpu *v, void > *addr, > > atomic_inc(>domain->arch.paging.shadow.gtable_dirty_version); > } > +#endif > > /**/ > /* Hash table for storing the guest->shadow mappings. > @@ -2723,12 +2736,13 @@ static int sh_remove_all_mappings(struct domain *d, > mfn_t gmfn, gfn_t gfn) > && (page->count_info & PGC_count_mask) <= 3 > && ((page->u.inuse.type_info & PGT_count_mask) > == (is_xen_heap_page(page) || > - is_ioreq_server_page(d, page ) > + (is_hvm_domain(d) && is_ioreq_server_page(d, > page) ) Isn't this a separate bugfix? is_ioreq_server_page shouldn't be called for PV domains at all (same below). > { > SHADOW_ERROR("can't find all mappings of mfn %lx (gfn %lx): " >"c=%lx t=%lx x=%d i=%d\n", mfn_x(gmfn), gfn_x(gfn), >page->count_info, page->u.inuse.type_info, > - !!is_xen_heap_page(page), is_ioreq_server_page(d, > page)); > + !!is_xen_heap_page(page), > + is_hvm_domain(d) && is_ioreq_server_page(d, page)); > } > } > > diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c > index 021ae25..ff7a20c 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > } > else > { > +#if CONFIG_HVM > /* Magic MMIO marker: extract gfn for MMIO address */ > ASSERT(sh_l1e_is_mmio(sl1e)); > +ASSERT(is_hvm_vcpu(v)); > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > << PAGE_SHIFT) > | (va & ~PAGE_MASK); > +perfc_incr(shadow_fault_fast_mmio); > +
Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM
>>> On 17.08.18 at 17:12, wrote: > @@ -423,6 +426,10 @@ const struct x86_emulate_ops *shadow_init_emulation( > ? sizeof(sh_ctxt->insn_buf) : 0; > > return _shadow_emulator_ops; > +#else > +BUG(); > +return NULL; > +#endif > } The sole invocation of the function sits right _after_ an is_hvm_...() conditional (which is odd enough, but presumably a result of the history of the code). Leaving aside a couple of labels (the goto-s to which are, I think, provable to be HVM-only as well), that is preceded by a shadow_mode_refcounts() check (right at the "emulate" label). I think shadow_mode_refcounts() wants to become constant false for !HVM, at which point the is_hvm_...() could even go away (but there's no strict need for this to happen right away). Bottom line - once again the entire function (not just its body) should be put inside the #ifdef, and then of course the same for shadow_continue_emulation(). > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -2926,18 +2926,25 @@ static int sh_page_fault(struct vcpu *v, > } > else > { > +#if CONFIG_HVM > /* Magic MMIO marker: extract gfn for MMIO address */ > ASSERT(sh_l1e_is_mmio(sl1e)); > +ASSERT(is_hvm_vcpu(v)); > gpa = (((paddr_t)(gfn_x(sh_l1e_mmio_get_gfn(sl1e > << PAGE_SHIFT) > | (va & ~PAGE_MASK); > +perfc_incr(shadow_fault_fast_mmio); > +SHADOW_PRINTK("fast path mmio %#"PRIpaddr"\n", gpa); > +sh_reset_early_unshadow(v); > +trace_shadow_gen(TRC_SHADOW_FAST_MMIO, va); > +return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, > +access) > +? EXCRET_fault_fixed : 0; > +#else > +/* When HVM is not enabled, there shouldn't be MMIO marker */ > +BUG(); At the example of this, while I agree we shouldn't reach here for PV, can this nevertheless be the less impactful domain_crash() please? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel