Re: [win-pv-devel] [PATCH 4/5] Implement new IOCTL handlers

[RafaÅ WojdyÅa <omeg@xxxxxxxxxxxxxxxxxxxxxx>]

On 2015-10-14 12:35, Paul Durrant wrote:
> 
> 
>> -----Original Message-----
>> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
>> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Rafal Wojdyla
>> Sent: 07 October 2015 05:49
>> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: [win-pv-devel] [PATCH 4/5] Implement new IOCTL handlers
>>
>> This patch implements new store, evtchn and gnttab IOCTLs.
>> Handlers are split into separate files for readability.
>>
>> Signed-off-by: Rafal Wojdyla <omeg@xxxxxxxxxxxxxxxxxxxxxx>

Again, I'll omit the code where there's no remarks or I generally agree
with your comments.

>> +    ProcessorCount =
>> KeQueryActiveProcessorCountEx(ALL_PROCESSOR_GROUPS);
>> +    RtlZeroMemory(Fdo->EvtchnDpc, sizeof(KDPC)*ProcessorCount);
> 
> Spaces around '*'. Also I don't think it's safe to use the active processor 
> count. What if a processor came online since the FDO was created?
> 
That's a fair point.

> 
> Since you're fixing whitespace, could you fix the above to have sane tabbing?
> 
I'll make a separate patch for un-tabify and such. Also on the general
subject of code formatting, what do you use for editing? VS has some
limitations (I douldn't find a way to make it insert spaces after sizeof
when formatting for example) so maybe I could at least partially
automate formatting to better fit your style.

>> +/* Copyright (c) Citrix Systems Inc.
>> + * All rights reserved.
> 
> You're not obliged to give Citrix copyright AFAIK. As long as it's BSD 
> licensed I think we're all happy.
> 
Yeah, I was not sure about all that copyright stuff :)

>> +    RtlZeroMemory(Context, sizeof(XENIFACE_GRANT_CONTEXT));
>> +    Context->Id.Type = XENIFACE_CONTEXT_GRANT;
>> +    Context->Id.Process = PsGetCurrentProcess();
> 
> Careful. I don't think Wndows guarantees that METHOD_BUFFERED ioctls will be 
> handled in the issuers process context. See the sentence:
> 
> " After the I/O manager has created a system-space buffer for the driver, the 
> requesting user-mode thread can be swapped out and its physical memory can be 
> reused by another thread, possibly by a thread belonging to another process."
> 
> at 
> https://msdn.microsoft.com/en-us/library/windows/hardware/ff565356%28v=vs.85%29.aspx.

I was pretty sure all user IOCTLs are delivered directly to the highest
level driver. Since IOCTL handlers run at PASSIVE_LEVEL they can page in
the user memory on completion I assume.
> 
> You should probably consider using METHOD_NEITHER. That may also allow you to 
> play tricks like injecting the results of the operation into the user-space 
> buffer without completing the ioctl possibly negating the need for a 'get 
> results' ioctl.
> 
This is an excellent point however. I'll try that.

-- 
Rafał Wojdyła
Qubes Tools for Windows developer
https://www.qubes-os.org/

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