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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 27, 2018 at 01:46:10AM -0600, Jan Beulich wrote:
> >>> On 26.08.18 at 13:04, <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel