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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Rafał Wojdyła [mailto:omeg@xxxxxxxxxxxxxxxxxxxxxx]
> Sent: 14 October 2015 14:44
> To: Paul Durrant; win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [win-pv-devel] [PATCH 4/5] Implement new IOCTL handlers
> 
> 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.
> 

I just use emacs. I jump between hacking on the PV drivers, Xen, Linux and QEMU 
often enough that I need something ubiquitous. I know VS can be a pain... one 
of the other things it seems to do is randomly insert DOS line endings.

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

Yes, I think that's what happens in practice but it's one of those things that 
Microsoft might just decide to break... because they can :-/

  Paul

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