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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.04.14 at 02:59, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Thu, 17 Apr 2014 07:54:55 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> 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;
>  }

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

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

Jan


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