Re: [win-pv-devel] [PATCH 10/10] Add support for the FIFO event channel ABI

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: David Vrabel
> Sent: 07 November 2014 14:37
> To: Paul Durrant; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 10/10] Add support for the FIFO event channel ABI
> 
> On 07/11/14 14:02, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Paul Durrant [mailto:paul.durrant@xxxxxxxxxx]
> >> Sent: 06 November 2014 14:31
> >> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Paul Durrant
> >> Subject: [PATCH 10/10] Add support for the FIFO event channel ABI
> >>
> >> If it is available then the fifo ABI will be used. If it is not then the
> >> two-level ABI will be used instead.
> >> The ABI is released and re-acquired across suspend/resume so this should
> >> allow moving between hosts with different capabilities.
> 
> Moving from FIFO to 2-level is only safe if you use fewer than 4095 (or
> 1023 on 32-bit) events, but this shouldn't be a problem for a Windows guest
> 

That's a good point. All event channels are essentially lost across a 
suspend/resume. The data structures are retained so that the clients can go 
through close/open cycle to re-establish their channels and if the open fails 
(because there are no longer as many ports as there were before) then the 
client needs to deal with that. As you say, such a high limit should never be 
hit in practice.

> >> diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
> >> new file mode 100644
> >> index 0000000..d37b1cd
> >> --- /dev/null
> >> +++ b/src/xenbus/evtchn_fifo.c
> >> @@ -0,0 +1,702 @@
> 
> >> +static event_word_t *
> >> +EvtchnFifoEventWord(
> >> +    IN  PXENBUS_EVTCHN_FIFO_CONTEXT Context,
> >> +    IN  ULONG                       Port
> >> +    )
> >> +{
> >> +    ULONG                           Index;
> >> +    PMDL                            Mdl;
> >> +    event_word_t                    *EventWord;
> >> +
> >> +    Index = Port / EVENT_WORDS_PER_PAGE;
> >> +    ASSERT3U(Index, <, Context->EventPageCount);
> >> +
> >> +    Mdl = Context->EventPageMdl[Index];
> >> +
> >> +    EventWord = MmGetSystemAddressForMdlSafe(Mdl,
> >> NormalPagePriority);
> >> +    ASSERT(EventWord != NULL);
> >> +
> >> +    ASSERT3U(Port, >=, Index * EVENT_WORDS_PER_PAGE);
> >> +    Port -= Index * EVENT_WORDS_PER_PAGE;
> >> +
> >> +    return &EventWord[Port];
> >> +}
> 
> I'm not at all familiar with Windows kernel memory management but is all
> this MDL stuff expensive?  Can you not store the VA instead of looking
> it up every time?
> 

It's actually a macro that pulls a field from the MDL if it's non-NULL and sets 
up a new PTE if its NULL, thus it's only costly on the first invocation.

> >> +
> >> +static FORCEINLINE BOOLEAN
> >> +__EvtchnFifoTestFlag(
> >> +    IN  event_word_t    *EventWord,
> >> +    IN  ULONG           Flag
> >> +    )
> >> +{
> >> +    KeMemoryBarrier();
> 
> Do you need this barrier?  The doc has a section describing the minimum
> number of barriers needed.
> 
> >> +
> >> +    return !!(*EventWord & (1 << Flag));
> >> +}
> >> +
> >> +static FORCEINLINE BOOLEAN
> >> +__EvtchnFifoTestAndSetFlag(
> >> +    IN  event_word_t    *EventWord,
> >> +    IN  ULONG           Flag
> >> +    )
> >> +{
> >> +    KeMemoryBarrier();
> 
> Or this one?  The Interlocked*() functions are also already barriers.
> 

Yes, this me being too liberal with the barriers.

> >> +
> >> +    return !!InterlockedBitTestAndSet((LONG *)EventWord, Flag);
> >> +}
> >> +
> >> +static FORCEINLINE BOOLEAN
> >> +__EvtchnFifoTestAndClearFlag(
> >> +    IN  event_word_t    *EventWord,
> >> +    IN  ULONG           Flag
> >> +    )
> >> +{
> >> +    KeMemoryBarrier();
> 
> ditto.
> 
> >> +
> >> +    return !!InterlockedBitTestAndReset((LONG *)EventWord, Flag);
> >> +}
> >> +
> >> +static FORCEINLINE VOID
> >> +__EvtchnFifoSetFlag(
> >> +    IN  event_word_t    *EventWord,
> >> +    IN  ULONG           Flag
> >> +    )
> >> +{
> >> +    *EventWord |= (1 << Flag);
> 
> This must be atomic.  Remember that Xen may be simultaneously writing to
> this word from a different PCPU.
> 

Yes. Good point.

> You need an InterlockedBitSet() (which doesn't seem to exist.  Perhaps
> make one with InterlockedOr()?)
> 

Or just a TestAndSet, ignoring the result.

> >> +    KeMemoryBarrier();
> >> +}
> >> +
> >> +static FORCEINLINE VOID
> >> +__EvtchnFifoClearFlag(
> >> +    IN  event_word_t    *EventWord,
> >> +    IN  ULONG           Flag
> >> +    )
> >> +{
> >> +    *EventWord &= ~(1 << Flag);
> 
> This must be atomic as well.
> 

Ok.

> >> +static VOID
> >> +EvtchnFifoContract(
> >> +    IN  PXENBUS_EVTCHN_FIFO_CONTEXT Context
> >> +    )
> >> +{
> 
> You can't shrink the event array if it is in use.  I assume this is only
> called after an EVTCHNOP_reset hypercall?
> 

Yes. It's not a shrink in practice, it's a 'bin it and start again' :-)

> >> +static BOOLEAN
> >> +EvtchnFifoPortUnmask(
> >> +    IN  PXENBUS_EVTCHN_ABI_CONTEXT  _Context,
> >> +    IN  ULONG                       Port
> >> +    )
> >> +{
> >> +    PXENBUS_EVTCHN_FIFO_CONTEXT     Context = (PVOID)_Context;
> >> +    event_word_t                    *EventWord;
> >> +    LONG                            Old;
> >> +    LONG                            New;
> >> +
> >> +    EventWord = EvtchnFifoEventWord(Context, Port);
> >> +
> >> +    // Clear masked bit, spinning if busy
> >> +    do {
> >> +        Old = *EventWord & ~(1 << EVTCHN_FIFO_BUSY);
> >> +        New = Old & ~(1 << EVTCHN_FIFO_MASKED);
> >> +    } while (InterlockedCompareExchange((LONG *)EventWord, New,
> Old) !=
> >> Old);
> >> +
> >> +    // Check whether the port was masked
> >> +    if (~Old & (1 << EVTCHN_FIFO_MASKED))
> >> +        return FALSE;
> >> +
> >> +    // If we cleared the mask then check whether something is pending
> >> +    if (!__EvtchnFifoTestAndClearFlag(EventWord,
> EVTCHN_FIFO_PENDING))
> >> +        return FALSE;
> 
> The doc says you should do an EVTCHNOP_unmask hypercall here, but
> picking up any pending event here is fine.
> 
> You may want to be more careful if you unmask an event while running on
> a VCPU that is not the one the event is bound to.
> 

Yes - I do need to allow for that. It won't happen at the moment but nothing in 
the API prevents it.

> >> +static VOID
> >> +EvtchnFifoReset(
> >> +    IN  PXENBUS_EVTCHN_FIFO_CONTEXT Context
> >> +    )
> >> +{
> >> +    ULONGLONG                       Value;
> >> +    ULONG                           LocalPort;
> >> +    ULONG                           RemotePort;
> >> +    USHORT                          RemoteDomain;
> >> +    NTSTATUS                        status;
> >> +
> >> +    UNREFERENCED_PARAMETER(Context);
> >> +
> >> +    status = HvmGetParam(HVM_PARAM_STORE_EVTCHN, &Value);
> >> +    ASSERT(NT_SUCCESS(status));
> >> +
> >> +    LocalPort = (LONG)Value;
> 
> You may wish to consider the console event channel as well.  But if you
> don't use it, it's ok to leave it unbound.
> 

I think I may add it in. I do eventually want to use the console for debug 
output.

Thanks very much for the review,

  Paul

> David

_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel