Re: [V9 PATCH 7/8] pvh dom0: check for vioapic null ptr in vioapic_range

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Tue, 22 Apr 2014 08:33:29 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 22.04.14 at 02:59, <mukesh.rathor@oracle.com> wrote:
> > On Thu, 17 Apr 2014 07:54:55 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
.......
> I think I'm getting the idea, but the code neither refernces
> pvh_mmio_handlers[], nor is that array's initialization well done
> (should be using .<field> = <value> style instead, omitting the
> NULLs).

oops, forgot to add the if pvh check. 

> >> That aside - why is this coming up only now? The emulation path
> >> getting reached shouldn't really depend on Dom0 vs Domu?
> > 
> > The io emulation is handled by handle_pvh_io; there shouldn't be 
> > path for pvh domu leading to this function with all the  
> > restrictions and limitations it has at present.
> 
> In which case we're back to the initial question: Why is this patch
> needed in the first place? If there is a separate emulation path
> already, how do we manage to get to the point where you added the
> extra check?

As described in the patch description:

-----
pvh doesn't use apic emulation, as a result vioapic_init is not called
and vioapic ptr in struct hvm_domain is not initialized. One path that
would access the ptr for pvh is :

   hvm_hap_nested_page_fault -> handle_mmio -> hvmemul_do_io ->
          hvm_mmio_intercept -> vioapic_range
-----

The only caller of hvm_hap_nested_page_fault is ept_handle_violation.

Now, 3 calls to handle_mmio in hvm_hap_nested_page_fault:
  1st is for nested vcpu, so doesn't apply to PVH.
  2nd has is_hvm check, 

  So it must have been the third one that I had observed the
  vioapic_range crash in a while ago, and had made note of it. Looking
  at it:

    if ( (p2mt == p2m_mmio_dm) ||
         (access_w && (p2mt == p2m_ram_ro)) )
    {
        put_gfn(p2m->domain, gfn);
        if ( !handle_mmio() )

doesn't seem apply to domu. Unfortunately, I can't reproduce it now
so maybe it was an ept violation due to some bug, and a crash in 
vioapic_range before printing the gfn/mfns etc by ept_handle_violation
made me make a note to put a check in it.

Hope that makes sense, and I'll assume you are ok with
pvh_mmio_handlers[] change. Otherwise, please lmk.

thanks
mukesh