Re: [Xen-devel] [PATCH 25/34] x86/mm/shadow: make it build with !CONFIG_HVM

2018-08-27 Thread Wei Liu
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

2018-08-27 Thread Jan Beulich
>>> 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

2018-08-26 Thread Wei Liu
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

2018-08-24 Thread Wei Liu
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

2018-08-24 Thread Wei Liu
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

2018-08-24 Thread Wei Liu
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

2018-08-24 Thread Tim Deegan
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

2018-08-21 Thread Jan Beulich
>>> 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

2018-08-21 Thread Roger Pau Monné
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

2018-08-21 Thread Jan Beulich
>>> 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