| Message ID | 1533318808-10781-4-git-send-email-jollys@xilinx.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drivers: Introduce firmware dnd clock river for ZynqMP core | expand |
Hi, On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote: > From: Rajan Vaja <rajanv@xilinx.com> > > Add ZynqMP firmware IOCTL API to control and configure > devices like PLLs, SD, Gem, etc. > > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > Signed-off-by: Jolly Shah <jollys@xilinx.com> This patch worries me somewhat. It's a transparent pass-through ioctl driver. Is there a spec available for what the implemented IOCTLs are? Should some of them be proper drivers instead of an opaque pass-through like this? Could some of them have stability impact on the platform such that there are security concerns and the list of arguments should somehow be sanitized? What's the intended usecase anyway? Just a debug tool during development, or something that you expect heavy use of by some userspace middleware? -Olof
Hi Olof, On Sat, Sep 8, 2018 at 6:18 PM, Olof Johansson <olof@lixom.net> wrote: > Hi, > > On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote: >> From: Rajan Vaja <rajanv@xilinx.com> >> >> Add ZynqMP firmware IOCTL API to control and configure >> devices like PLLs, SD, Gem, etc. >> >> Signed-off-by: Rajan Vaja <rajanv@xilinx.com> >> Signed-off-by: Jolly Shah <jollys@xilinx.com> > > This patch worries me somewhat. It's a transparent pass-through ioctl > driver. Is there a spec available for what the implemented IOCTLs are? > > Should some of them be proper drivers instead of an opaque > pass-through like this? Could some of them have stability impact on > the platform such that there are security concerns and the list of > arguments should somehow be sanitized? I tend to agree with this, good catch. > What's the intended usecase anyway? Just a debug tool during > development, or something that you expect heavy use of by some > userspace middleware? I suspect it is another attempt to make userspace clocks/plls work? Scary. Cheers, Moritz
On 09/09/18 02:18, Olof Johansson wrote: > Hi, > > On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote: >> From: Rajan Vaja <rajanv@xilinx.com> >> >> Add ZynqMP firmware IOCTL API to control and configure >> devices like PLLs, SD, Gem, etc. >> >> Signed-off-by: Rajan Vaja <rajanv@xilinx.com> >> Signed-off-by: Jolly Shah <jollys@xilinx.com> > > This patch worries me somewhat. It's a transparent pass-through ioctl > driver. Is there a spec available for what the implemented IOCTLs are? > > Should some of them be proper drivers instead of an opaque > pass-through like this? Could some of them have stability impact on > the platform such that there are security concerns and the list of > arguments should somehow be sanitized? > > What's the intended usecase anyway? Just a debug tool during > development, or something that you expect heavy use of by some > userspace middleware? > Thanks for pointing this out. My earlier attempts were ignored[1] and I gave up.
On 9.9.2018 03:18, Olof Johansson wrote: > Hi, > > On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote: >> From: Rajan Vaja <rajanv@xilinx.com> >> >> Add ZynqMP firmware IOCTL API to control and configure >> devices like PLLs, SD, Gem, etc. >> >> Signed-off-by: Rajan Vaja <rajanv@xilinx.com> >> Signed-off-by: Jolly Shah <jollys@xilinx.com> > > This patch worries me somewhat. It's a transparent pass-through ioctl > driver. Is there a spec available for what the implemented IOCTLs are? https://www.xilinx.com/support/documentation/user_guides/ug1200-eemi-api.pdf > > Should some of them be proper drivers instead of an opaque > pass-through like this? Could some of them have stability impact on > the platform such that there are security concerns and the list of > arguments should somehow be sanitized? You can look at for example reset driver which is using this eemi interface. https://lkml.org/lkml/2018/9/5/144 > What's the intended usecase anyway? Just a debug tool during > development, or something that you expect heavy use of by some > userspace middleware? I am not an author of this interface but there is no intention to enable this interface for userspace as far as I see. All functions should be used by kernel drivers only. Jolly: Please answer all others concern in connection to this patchset. Thanks, Michal
On 9.9.2018 21:20, Moritz Fischer wrote: > Hi Olof, > > On Sat, Sep 8, 2018 at 6:18 PM, Olof Johansson <olof@lixom.net> wrote: >> Hi, >> >> On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote: >>> From: Rajan Vaja <rajanv@xilinx.com> >>> >>> Add ZynqMP firmware IOCTL API to control and configure >>> devices like PLLs, SD, Gem, etc. >>> >>> Signed-off-by: Rajan Vaja <rajanv@xilinx.com> >>> Signed-off-by: Jolly Shah <jollys@xilinx.com> >> >> This patch worries me somewhat. It's a transparent pass-through ioctl >> driver. Is there a spec available for what the implemented IOCTLs are? >> >> Should some of them be proper drivers instead of an opaque >> pass-through like this? Could some of them have stability impact on >> the platform such that there are security concerns and the list of >> arguments should somehow be sanitized? > > I tend to agree with this, good catch. > >> What's the intended usecase anyway? Just a debug tool during >> development, or something that you expect heavy use of by some >> userspace middleware? > > I suspect it is another attempt to make userspace clocks/plls work? Scary. none is trying to do that as far as I know. Thanks, Michal
Hi All, Adding more clarification on top of what Michal said: Here ioctl is not a system ioctl and just a eemi API like other interface APIs. It cannot be called from userspace. Only Linux drivers can use this API for defined ioctl operations. This API is meant for any platform specific operations which needs to be managed by firmware. Firmware will always validate the request for action being performed. Debugfs interface is just for debugging during development. We can remove debugfs support for ioctl API if you suggest. Thanks, Jolly Shah > -----Original Message----- > From: Olof Johansson [mailto:olof@lixom.net] > Sent: Saturday, September 08, 2018 6:19 PM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: ard.biesheuvel@linaro.org; Ingo Molnar <mingo@kernel.org>; Greg Kroah- > Hartman <gregkh@linuxfoundation.org>; matt@codeblueprint.co.uk; Sudeep > Holla <sudeep.holla@arm.com>; hkallweit1@gmail.com; Kees Cook > <keescook@chromium.org>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; > Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@codeaurora.org>; Michal Simek <michals@xilinx.com>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; linux-clk > <linux-clk@vger.kernel.org>; Rajan Vaja <RAJANV@xilinx.com>; Linux ARM > Mailing List <linux-arm-kernel@lists.infradead.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; DTML <devicetree@vger.kernel.org>; Jolly > Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH v11 03/11] firmware: xilinx: Add zynqmp IOCTL API for > device control > > Hi, > > On Fri, Aug 3, 2018 at 10:53 AM, Jolly Shah <jolly.shah@xilinx.com> wrote: > > From: Rajan Vaja <rajanv@xilinx.com> > > > > Add ZynqMP firmware IOCTL API to control and configure devices like > > PLLs, SD, Gem, etc. > > > > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > > Signed-off-by: Jolly Shah <jollys@xilinx.com> > > This patch worries me somewhat. It's a transparent pass-through ioctl driver. Is > there a spec available for what the implemented IOCTLs are? > > Should some of them be proper drivers instead of an opaque pass-through like > this? Could some of them have stability impact on the platform such that there > are security concerns and the list of arguments should somehow be sanitized? > > What's the intended usecase anyway? Just a debug tool during development, or > something that you expect heavy use of by some userspace middleware? > > > -Olof
On Mon, Sep 10, 2018 at 07:17:45PM +0000, Jolly Shah wrote: > Hi All, > > Adding more clarification on top of what Michal said: > Here ioctl is not a system ioctl and just a eemi API like other interface > APIs. It cannot be called from userspace. I get that these are not system ioctl and you keep assuming that the issue raised here is related to that. *NO*, the main issue is the way this so called EEMI ioctl interface is exposing the users low level accessors without much abstraction. IMO, it defeats the idea of having EEMI interface altogether. There's abstraction but the level is not right. Anyways, this gets worse with read/write debugfs interface you want to add. > Only Linux drivers can use this > API for defined ioctl operations. This API is meant for any platform > specific operations which needs to be managed by firmware. Firmware will > always validate the request for action being performed. > Debugfs interface is just for debugging during development. We can remove > debugfs support for ioctl API if you suggest. Yes please. I did suggest to remove them long back. You did only for the clock module but retained it in the core EEMI ioctl. But if you remove the debugfs, do you have any users of these ioctl in the series ? I couldn't find one, but if that's the case, drop this patch. I see only valid users for clock APIs in this series. -- Regards, Sudeep
Hi Jolly, On Mon, Sep 10, 2018 at 12:17 PM, Jolly Shah <JOLLYS@xilinx.com> wrote: > Hi All, > > Adding more clarification on top of what Michal said: > Here ioctl is not a system ioctl and just a eemi API like other interface APIs. It cannot be called from userspace. Only Linux drivers can use this API for defined ioctl operations. This API is meant for any platform specific operations which needs to be managed by firmware. Firmware will always validate the request for action being performed. > Debugfs interface is just for debugging during development. We can remove debugfs support for ioctl API if you suggest. Are there any pending patchsets that make use of the ioctl interface besides the debugfs piece? None of the ones in this pull request (besides the debugfs) seems to call into it. It might just be easier to leave it out until you have a use case for it. The rest of the patchset looks fine to me, and I'd be happy to take it. Once you have a need for it we can discuss specifics. -Olof
Hi Sudeep and Olof, Clock driver from same patch set uses ioctl API along with other clock eemi APIs. As clock patches' final review is pending by Stephen, Michal only created pull request for rest of the patches and that doesn't require ioctl api. I will remove it and submit new patch set. For future patches which requires ioctl api, would like to understand your suggestion so I can make required changes. For zynqmp, EEMI interface allows clock, reset, power etc management through firmware but apart from those there are some operations which needs secure access through firmware. Examples are accessing some storage registers for inter agent communication, configuring another agent(RPU) mode, setting PLL parameters, boot device configuration etc. Those operations are covered as ioctls as they are very platform specific. Do you suggest to handle them with individual EEMI APIs instead of ioctl API? Thanks, Jolly Shah > -----Original Message----- > From: Olof Johansson [mailto:olof@lixom.net] > Sent: Tuesday, September 11, 2018 8:03 AM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: ard.biesheuvel@linaro.org; Ingo Molnar <mingo@kernel.org>; Greg Kroah- > Hartman <gregkh@linuxfoundation.org>; matt@codeblueprint.co.uk; Sudeep > Holla <sudeep.holla@arm.com>; hkallweit1@gmail.com; Kees Cook > <keescook@chromium.org>; Dmitry Torokhov <dmitry.torokhov@gmail.com>; > Michael Turquette <mturquette@baylibre.com>; Stephen Boyd > <sboyd@codeaurora.org>; Michal Simek <michals@xilinx.com>; Rob Herring > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; linux-clk > <linux-clk@vger.kernel.org>; Rajan Vaja <RAJANV@xilinx.com>; Linux ARM > Mailing List <linux-arm-kernel@lists.infradead.org>; Linux Kernel Mailing List > <linux-kernel@vger.kernel.org>; DTML <devicetree@vger.kernel.org> > Subject: Re: [PATCH v11 03/11] firmware: xilinx: Add zynqmp IOCTL API for > device control > > Hi Jolly, > > On Mon, Sep 10, 2018 at 12:17 PM, Jolly Shah <JOLLYS@xilinx.com> wrote: > > Hi All, > > > > Adding more clarification on top of what Michal said: > > Here ioctl is not a system ioctl and just a eemi API like other interface APIs. It > cannot be called from userspace. Only Linux drivers can use this API for defined > ioctl operations. This API is meant for any platform specific operations which > needs to be managed by firmware. Firmware will always validate the request for > action being performed. > > Debugfs interface is just for debugging during development. We can remove > debugfs support for ioctl API if you suggest. > > Are there any pending patchsets that make use of the ioctl interface besides the > debugfs piece? None of the ones in this pull request (besides the debugfs) seems > to call into it. > > It might just be easier to leave it out until you have a use case for it. The rest of > the patchset looks fine to me, and I'd be happy to take it. Once you have a need > for it we can discuss specifics. > > > -Olof
Hi, Apologies for the slow responses here, I meant to follow up on this sooner. On Tue, Sep 11, 2018 at 8:20 PM, Jolly Shah <JOLLYS@xilinx.com> wrote: > Hi Sudeep and Olof, > > Clock driver from same patch set uses ioctl API along with other clock eemi APIs. As clock patches' final review is pending by Stephen, Michal only created pull request for rest of the patches and that doesn't require ioctl api. I will remove it and submit new patch set. > > For future patches which requires ioctl api, would like to understand your suggestion so I can make required changes. For zynqmp, EEMI interface allows clock, reset, power etc management through firmware but apart from those there are some operations which needs secure access through firmware. Examples are accessing some storage registers for inter agent communication, configuring another agent(RPU) mode, setting PLL parameters, boot device configuration etc. Those operations are covered as ioctls as they are very platform specific. Do you suggest to handle them with individual EEMI APIs instead of ioctl API? I'm personally less worried about whether the calls are through an ioctl API or an EEMI one, but if it is through ioctl, I'd prefer if it wasn't wide-open pass-through. I.e. that the ioctls you actually use are documented, and only those who are whitelisted are passed through (and not in general exported to userspace). Does that make sense? -Olof
Hi Olof, > -----Original Message----- > From: Olof Johansson [mailto:olof@lixom.net] > Sent: Sunday, September 23, 2018 6:11 AM > To: Jolly Shah <JOLLYS@xilinx.com> > Cc: Sudeep Holla <sudeep.holla@arm.com>; ard.biesheuvel@linaro.org; Ingo > Molnar <mingo@kernel.org>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; matt@codeblueprint.co.uk; > hkallweit1@gmail.com; Kees Cook <keescook@chromium.org>; Dmitry > Torokhov <dmitry.torokhov@gmail.com>; Michael Turquette > <mturquette@baylibre.com>; Stephen Boyd <sboyd@codeaurora.org>; Michal > Simek <michals@xilinx.com>; Rob Herring <robh+dt@kernel.org>; Mark Rutland > <mark.rutland@arm.com>; linux-clk <linux-clk@vger.kernel.org>; Rajan Vaja > <RAJANV@xilinx.com>; Linux ARM Mailing List <linux-arm- > kernel@lists.infradead.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; DTML <devicetree@vger.kernel.org> > Subject: Re: [PATCH v11 03/11] firmware: xilinx: Add zynqmp IOCTL API for > device control > > Hi, > > Apologies for the slow responses here, I meant to follow up on this sooner. > > On Tue, Sep 11, 2018 at 8:20 PM, Jolly Shah <JOLLYS@xilinx.com> wrote: > > Hi Sudeep and Olof, > > > > Clock driver from same patch set uses ioctl API along with other clock eemi > APIs. As clock patches' final review is pending by Stephen, Michal only created > pull request for rest of the patches and that doesn't require ioctl api. I will > remove it and submit new patch set. > > > > For future patches which requires ioctl api, would like to understand your > suggestion so I can make required changes. For zynqmp, EEMI interface allows > clock, reset, power etc management through firmware but apart from those > there are some operations which needs secure access through firmware. > Examples are accessing some storage registers for inter agent communication, > configuring another agent(RPU) mode, setting PLL parameters, boot device > configuration etc. Those operations are covered as ioctls as they are very > platform specific. Do you suggest to handle them with individual EEMI APIs > instead of ioctl API? > > I'm personally less worried about whether the calls are through an ioctl API or an > EEMI one, but if it is through ioctl, I'd prefer if it wasn't wide-open pass-through. > I.e. that the ioctls you actually use are documented, and only those who are > whitelisted are passed through (and not in general exported to userspace). > > Does that make sense? > Sounds perfect. I will make required changes in next incremental patchset. Thanks, Jolly Shah > > -Olof
diff --git a/drivers/firmware/xilinx/zynqmp.c b/drivers/firmware/xilinx/zynqmp.c index edbb84e..24cfd9e 100644 --- a/drivers/firmware/xilinx/zynqmp.c +++ b/drivers/firmware/xilinx/zynqmp.c @@ -241,8 +241,28 @@ static int get_set_conduit_method(struct device_node *np) return 0; } +/** + * zynqmp_pm_ioctl() - PM IOCTL API for device control and configs + * @node_id: Node ID of the device + * @ioctl_id: ID of the requested IOCTL + * @arg1: Argument 1 to requested IOCTL call + * @arg2: Argument 2 to requested IOCTL call + * @out: Returned output value + * + * This function calls IOCTL to firmware for device control and configuration. + * + * Return: Returns status, either success or error+reason + */ +static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, + u32 *out) +{ + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, ioctl_id, + arg1, arg2, out); +} + static const struct zynqmp_eemi_ops eemi_ops = { .get_api_version = zynqmp_pm_get_api_version, + .ioctl = zynqmp_pm_ioctl, }; /** diff --git a/include/linux/firmware/xlnx-zynqmp.h b/include/linux/firmware/xlnx-zynqmp.h index cb63bed..2eec6e7 100644 --- a/include/linux/firmware/xlnx-zynqmp.h +++ b/include/linux/firmware/xlnx-zynqmp.h @@ -34,6 +34,7 @@ enum pm_api_id { PM_GET_API_VERSION = 1, + PM_IOCTL = 34, }; /* PMU-FW return status codes */ @@ -49,6 +50,7 @@ enum pm_ret_status { struct zynqmp_eemi_ops { int (*get_api_version)(u32 *version); + int (*ioctl)(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, u32 *out); }; #if IS_REACHABLE(CONFIG_ARCH_ZYNQMP)