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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.04.14 at 02:11, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Tue, 22 Apr 2014 08:33:29 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>> On 22.04.14 at 02:59, <mukesh.rathor@xxxxxxxxxx> wrote:
>> > 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

So in your earlier reply (still quoted above) you said emulation gets
handled by handle_pvh_io(), yet here you point out a path going
through handle_mmio(). That's all very inconsistent.

> 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.

Which makes me think that we don't need the patch at all.

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

I would be fine with it if it was proven that a change here is needed
at all.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel