Re: [Xen-devel] [PATCH V2] x86/vm_event: block interrupt injection for sync vm_events

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 11/12/2018 10:21, Razvan Cojocaru wrote:
> On 12/11/18 12:14 PM, Roger Pau Monné wrote:
> > On Tue, Dec 11, 2018 at 12:01:53PM +0200, Razvan Cojocaru wrote:
> > > On 12/10/18 6:59 PM, Razvan Cojocaru wrote:
> > > > On 12/10/18 6:49 PM, Roger Pau Monné wrote:
> > > > > On Mon, Dec 10, 2018 at 06:01:49PM +0200, Razvan Cojocaru wrote:
> > > > > > diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
> > > > > > index 66f2474..b63249e 100644
> > > > > > --- a/xen/include/asm-arm/vm_event.h
> > > > > > +++ b/xen/include/asm-arm/vm_event.h
> > > > > > @@ -52,4 +52,10 @@ void vm_event_emulate_check(struct vcpu *v, 
> > > > > > vm_event_response_t *rsp)
> > > > > >       /* Not supported on ARM. */
> > > > > >   }
> > > > > >   
> > > > > > +static inline
> > > > > > +void vm_event_block_interrupts(struct vcpu *v, bool value)
> > > > > > +{
> > > > > > +    /* Not supported on ARM. */
> > > > > ASSERT_UNREACHABLE?
> > > > Will do (although if you look at the rest of the function in that header
> > > > it'll break what appears to be the prior convention).
> > > Sorry, on second thought we can't do that, because that function is
> > > being called from the common code - which is why the function became
> > > necessary. Specifically, this it unconditionally called in
> > > monitor_traps(), which is used for all events (ARM and otherwise).
> > >
> > > So it's valid to call monitor_traps() for ARM vm_events and expect it to
> > > run without issue, which ASSERT_UNREACHABLE() would of course break.
> > But then the functionality that makes use of vm_event_block_interrupts
> > cannot work reliably on ARM and should not be used?
> Well, it's currently a no-op on ARM so it doesn't make anything worse.
Can an vm-event app rely on the interrupts to be blocked?

> I don't have access to ARM hardware and am unfamiliar with the specifics
> of handling interrupts on ARM with regard to vm_events (or even if this
> specific problem applies to ARM) - so it's the best that I am able to do
> at the moment.
At the first, the name of the function looks quite wrong for Arm. If you block 
interrupts, you will never receive them again. I read the commit message and I 
am not sure to understand the exact behavior of this function.
Do you mind to provide more explanation what you are trying to achieve?

> Of course, this patch can be the basis of a future one for ARM if that
> work makes sense (perhaps Tamas has more to say about this), or if an
> ARM maintaner can point out what modifications should be done I can
> compile-test for ARM with a cross-compiler, _hope_ it works, and
> re-submit the patch.
Before pointing out the modifications, I need to understand what you exactly 
want to achieve with it. But then, I think such code should be tested before 
getting merged.
That's fine by me if you don't want to implement it for Arm. However, we need to 
make sure that vm-event app does not expect that behavior.
In any case, I think you want to rename the function and/or document more that 
expected behavior.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel