Re: [Xen-devel] [PATCH v4 08/10] xen/arm: second irq injection while the first irq is still inflight

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-03-24 at 16:41 +0000, Stefano Stabellini wrote:
> On Mon, 24 Mar 2014, Ian Campbell wrote:
> > > > > > > We also need to force the first injection of evtchn_irq (call
> > > > > > > gic_vcpu_inject_irq) from vgic_enable_irqs because 
> > > > > > > evtchn_upcall_pending
> > > > > > > is already set by common code on vcpu creation.
> > > > > > 
> > > > > > This is because the common code expects that the guest is moving 
> > > > > > from
> > > > > > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, 
> > > > > > but
> > > > > > on ARM we start off that way anyway.
> > > > > > 
> > > > > > I suppose it's a minor wrinkle, but I wonder if we can get rid of 
> > > > > > it...
> > > > > 
> > > > > Do you mean getting rid of evtchn_upcall_pending being set at vcpu
> > > > > creation? Wouldn't that cause possible undesirable side effects to
> > > > > guests that came to rely on it somehow? It would be an ABI change.
> > > > 
> > > > I mean precisely for the boot cpu when it is brought online, there isn't
> > > > much sense in immediately taking an interrupt when that cpu enables
> > > > them.
> > > > 
> > > > The reason for setting it right now is only for the case of a cpu moving
> > > > its vcpu info, to ensure it can't miss anything.
> > > 
> > > What about secondary vcpus? Should we keep the spurious injection for
> > > them?
> > 
> > Not sure. No?
> > 
> > > In any case I agree with you that the current behaviour is not nice,
> > > however I am worried about changing a guest visible interface like this
> > > one, that would affect x86 guests too.
> > 
> > Oh, I certainly wouldn't change this for x86! Or maybe I would change it
> > but only for cpus which are not online at the time when the init happens
> > (which is effectively the difference between the x86 and arm cases)
>  
> Today on ARM and x86 PV on HVM VCPUOP_register_vcpu_info is called by
> each vcpu independently when is coming online, so secondary cpus would
> be already online at the time of the hypercall.
> 
> On x86 PV VCPUOP_register_vcpu_info is called in a loop by vcpu0 for all
> the possible vcpus before smp bringup.
> 
> Either way I don't think we can easily change this behaviour without
> affecting x86 guests.

OK, it was worth considering but I think you are right that it isn't
worth it. Hopefully any sane OS is going to be prepared to handle
interrupts before it enables them!

> Alternatively of course this (ugly) change in Xen would work:

Yeah, lets not ;-)

> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ad8a1b6..bd81f43 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -893,7 +893,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, 
> unsigned offset)
>      void *mapping;
>      vcpu_info_t *new_info;
>      struct page_info *page;
> +#ifdef CONFIG_X86
>      int i;
> +#endif
>  
>      if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
>          return -EINVAL;
> @@ -942,6 +944,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, 
> unsigned offset)
>      /* Set new vcpu_info pointer /before/ setting pending flags. */
>      smp_wmb();
>  
> +#ifdef CONFIG_X86
>      /*
>       * Mark everything as being pending just to make sure nothing gets
>       * lost.  The domain will get a spurious event, but it can cope.
> @@ -949,7 +952,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, 
> unsigned offset)
>      vcpu_info(v, evtchn_upcall_pending) = 1;
>      for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
>          set_bit(i, &vcpu_info(v, evtchn_pending_sel));
> -
> +#endif
>      return 0;
>  }



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