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

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

On Thu, 17 Apr 2014 07:54:55 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 17.04.14 at 03:44, <mukesh.rathor@oracle.com> wrote:
> > On Wed, 16 Apr 2014 17:05:57 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 16.04.14 at 02:12, <mukesh.rathor@oracle.com> wrote:
> >> > 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
> >> 
> >> Given this I'm not sure the guard belongs here. The majority of the
> >> handle_mmio() logic should never be used for Dom0. Perhaps you
> >> should simply have a pvh_mmio_handlers[] paralleling
> >> hvm_mmio_handlers[], but (presumably) only having HPET and MSI-X
> >> entries for now?
> > 
> > Well, there's already talk of adding vioapic support for PVH so it
> > could take advantage of the new features coming up. So, it'll prob 
> > converge in near future with hvm_mmio_handlers . I'm ok either way. 
> 
> From a conceptual pov I still think the separation of emulation paths
> should happen earlier and/or be more explicit, not the least because
> iirc PVH guests are expected to not have a qemu associated.

Correct. I've following for next version:

diff --git a/xen/arch/x86/hvm/intercept.c b/xen/arch/x86/hvm/intercept.c
index 7cc13b5..f89be28 100644
--- a/xen/arch/x86/hvm/intercept.c
+++ b/xen/arch/x86/hvm/intercept.c
@@ -42,6 +42,16 @@ hvm_mmio_handlers[HVM_MMIO_HANDLER_NR] =
     &iommu_mmio_handler
 };

+static const struct hvm_mmio_handler *const
+pvh_mmio_handlers[HVM_MMIO_HANDLER_NR] =
+{
+    &hpet_mmio_handler,
+    NULL,
+    NULL,
+    &msixtbl_mmio_handler,
+    NULL,
+};
+
 static int hvm_mmio_access(struct vcpu *v,
                            ioreq_t *p,
                            hvm_mmio_read_t read_handler,
@@ -169,11 +179,13 @@ int hvm_mmio_intercept(ioreq_t *p)
     int i;

     for ( i = 0; i < HVM_MMIO_HANDLER_NR; i++ )
-        if ( hvm_mmio_handlers[i]->check_handler(v, p->addr) )
-            return hvm_mmio_access(
-                v, p,
-                hvm_mmio_handlers[i]->read_handler,
-                hvm_mmio_handlers[i]->write_handler);
+    {
+        const struct hvm_mmio_handler *mmio_handler = hvm_mmio_handlers[i];
+
+        if ( mmio_handler && mmio_handler->check_handler(v, p->addr) )
+            return hvm_mmio_access(v, p, mmio_handler->read_handler,
+                                   mmio_handler->write_handler);
+    }

     return X86EMUL_UNHANDLEABLE;
 }



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

thanks
mukesh