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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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

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

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

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

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

>> +    KeMemoryBarrier();
>> +}
>> +
>> +static FORCEINLINE VOID
>> +__EvtchnFifoClearFlag(
>> +    IN  event_word_t    *EventWord,
>> +    IN  ULONG           Flag
>> +    )
>> +{
>> +    *EventWord &= ~(1 << Flag);

This must be atomic as well.

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

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

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

David

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