Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
On Thu, Jul 04, 2019 at 02:42:08PM +0800, Wu Hao wrote:
> On Thu, Jul 04, 2019 at 07:37:19AM +0200, Greg KH wrote:
> > On Thu, Jul 04, 2019 at 07:38:22AM +0800, Wu Hao wrote:
> > > On Wed, Jul 03, 2019 at 07:59:26PM +0200, Greg KH wrote:
> > > > On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> > > > > From: Wu Hao
> > > > >
> > > > > This patch adds virtualization support description for DFL based
> > > > > FPGA devices (based on PCIe SRIOV), and introductions to new
> > > > > interfaces added by new dfl private feature drivers.
> > > > >
> > > > > [m...@kernel.org: Fixed up to make it work with new reStructuredText
> > > > > docs]
> > > > > Signed-off-by: Xu Yilun
> > > > > Signed-off-by: Wu Hao
> > > > > Acked-by: Alan Tull
> > > > > Signed-off-by: Moritz Fischer
> > > > > ---
> > > > > Documentation/fpga/dfl.rst | 100
> > > > > +
> > > > > 1 file changed, 100 insertions(+)
> > > >
> > > > This doesn't apply to my tree, where is this file created?
> > >
> > > Hi Greg,
> > >
> > > >From the cover-letter, Moritz mentioned, dfl.txt has been converted to
> > > >.rst
> > > in linux-next. I think this patch is created on top of that by Moritz.
> > >
> > > "Note: I've seen that Mauro touched Documentation/fpga/dfl.rst in
> > > linux-next
> > > commit c220a1fae6c5d ("docs: fpga: convert docs to ReST and rename to
> > > *.rst")"
> >
> > Ok, but I can't take this patch just because the file is converted in
> > someone else's tree :(
>
> Oh.. if that patch is merged from fpga tree, then we wont have this problem.
> :(
You better not be basing your tree on linux-next :)
> So do you have any suggestions on what should i do now?
> wait that patch goes to offical release, and then resubmit this patch?
Wait until the change hits Linus's tree and then resend it to me.
thanks,
greg k-h
Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
On Thu, Jul 04, 2019 at 02:31:06PM +0800, Wu Hao wrote:
> On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> > On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > > From: Wu Hao
> > > > >
> > > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > > assign back the port device. In order to safely turn Port from PF
> > > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > > interfaces, and then configure the PF/VF access register in FME.
> > > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > > ioctl to attach the port back to PF.
> > > > >
> > > > > Ioctl interfaces:
> > > > > * DFL_FPGA_FME_PORT_RELEASE
> > > > >Release platform device of given port, it deletes port platform
> > > > >device to remove related userspace interfaces on PF, then
> > > > >configures PF/VF access mode to VF.
> > > > >
> > > > > * DFL_FPGA_FME_PORT_ASSIGN
> > > > >Assign platform device of given port back to PF, it configures
> > > > >PF/VF access mode to PF, then adds port platform device back to
> > > > >re-enable related userspace interfaces on PF.
> > > >
> > > > Why are you not using the "generic" bind/unbind facility that userspace
> > > > already has for this with binding drivers to devices? Why a special
> > > > ioctl?
> > >
> > > Hi Greg,
> > >
> > > Actually we think it should be safer that making the device invisble than
> > > just unbinding its driver. Looks like user can try to rebind it at any
> > > time and we don't have any method to stop them.
> >
> > Why do you want to "stop" the user from doing something? They asked to
> > do it, why prevent it? If they ask to do something foolish, well, they
> > get to keep the pieces :)
>
> Actually this is for SRIOV support, as we are moving FPGA accelerator from
> PF to VF, so we don't want users to see the FPGA accelerator from PF any
> more. We can't allow user to touch same FPGA accelerator from both PF and
> VF side (it leads to hardware erros).
Ick, ok, this needs to be documented really well then.
> > > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > > >
> > > > > #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC, DFL_FME_BASE + 0)
> > > > >
> > > > > +/**
> > > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE + 1,
> > > > > + * struct
> > > > > dfl_fpga_fme_port_release)
> > > > > + *
> > > > > + * Driver releases the port per Port ID provided by caller.
> > > > > + * Return: 0 on success, -errno on failure.
> > > > > + */
> > > > > +struct dfl_fpga_fme_port_release {
> > > > > + /* Input */
> > > > > + __u32 argsz;/* Structure length */
> > > > > + __u32 flags;/* Zero for now */
> > > > > + __u32 port_id;
> > > > > +};
> > > >
> > > > meta-comment, why do all of your structures for ioctls have argsz? You
> > > > "know" the size of the structure already, it's part of the ioctl
> > > > definition. You shouldn't need to also set it again, right? Otherwise
> > > > ALL Linux ioctls would need something crazy like this.
> > >
> > > Actually we followed the same method as vfio.
> >
> > vfio is a protocol on "the wire", right? Not an ioctl.
> >
> > > The major purpose should be extendibility, as we really need this to
> > > be sth long term maintainable.
> >
> > You can't change ioctl structure sizes at any time.
> >
> > > It really helps, if we add some new members for extentions/enhancement
> > > under the same ioctl.
> >
> > You don't do that.
> >
> > > I don't think everybody needs this, but my consideration here is if
> > > newer generations of hardware/specs come with some extentions, I still
> > > hope we can resue these IOCTLs as much as we could, instead of
> > > creating more new ones.
> >
> > You create new ones, like everyone else does, as you can not change old
> > code. By trying to "version" structures like this, it's just going to
> > be a nightmare.
>
> Actually i learned this from vfio code here, it's not trying to "version"
> structures, let me copy the comments from vfio header file. It should be
> more clear than above short description from me.
>
> "include/uapi/linux/vfio.h"
>
> /*
> * The IOCTL interface is designed for extensibility by embedding the
> * structure length (argsz) and flags into structures passed between
> * kernel and userspace. We therefore use the _IO() macro for these
> * defines to avoid implicitly embedding a size into the ioctl request.
> * As structure fields are added, argsz will incr
Re: [PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization and new interfaces.
On Thu, Jul 04, 2019 at 10:17:13AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 02:42:08PM +0800, Wu Hao wrote:
> > On Thu, Jul 04, 2019 at 07:37:19AM +0200, Greg KH wrote:
> > > On Thu, Jul 04, 2019 at 07:38:22AM +0800, Wu Hao wrote:
> > > > On Wed, Jul 03, 2019 at 07:59:26PM +0200, Greg KH wrote:
> > > > > On Thu, Jun 27, 2019 at 05:49:41PM -0700, Moritz Fischer wrote:
> > > > > > From: Wu Hao
> > > > > >
> > > > > > This patch adds virtualization support description for DFL based
> > > > > > FPGA devices (based on PCIe SRIOV), and introductions to new
> > > > > > interfaces added by new dfl private feature drivers.
> > > > > >
> > > > > > [m...@kernel.org: Fixed up to make it work with new
> > > > > > reStructuredText docs]
> > > > > > Signed-off-by: Xu Yilun
> > > > > > Signed-off-by: Wu Hao
> > > > > > Acked-by: Alan Tull
> > > > > > Signed-off-by: Moritz Fischer
> > > > > > ---
> > > > > > Documentation/fpga/dfl.rst | 100
> > > > > > +
> > > > > > 1 file changed, 100 insertions(+)
> > > > >
> > > > > This doesn't apply to my tree, where is this file created?
> > > >
> > > > Hi Greg,
> > > >
> > > > >From the cover-letter, Moritz mentioned, dfl.txt has been converted to
> > > > >.rst
> > > > in linux-next. I think this patch is created on top of that by Moritz.
> > > >
> > > > "Note: I've seen that Mauro touched Documentation/fpga/dfl.rst in
> > > > linux-next
> > > > commit c220a1fae6c5d ("docs: fpga: convert docs to ReST and rename to
> > > > *.rst")"
> > >
> > > Ok, but I can't take this patch just because the file is converted in
> > > someone else's tree :(
> >
> > Oh.. if that patch is merged from fpga tree, then we wont have this
> > problem. :(
>
> You better not be basing your tree on linux-next :)
>
> > So do you have any suggestions on what should i do now?
> > wait that patch goes to offical release, and then resubmit this patch?
>
> Wait until the change hits Linus's tree and then resend it to me.
OK, get it! Thanks!
Hao
>
> thanks,
>
> greg k-h
Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
On Thu, Jul 04, 2019 at 10:20:13AM +0200, Greg KH wrote:
> On Thu, Jul 04, 2019 at 02:31:06PM +0800, Wu Hao wrote:
> > On Thu, Jul 04, 2019 at 07:39:27AM +0200, Greg KH wrote:
> > > On Thu, Jul 04, 2019 at 07:30:58AM +0800, Wu Hao wrote:
> > > > On Wed, Jul 03, 2019 at 08:07:53PM +0200, Greg KH wrote:
> > > > > On Thu, Jun 27, 2019 at 05:49:42PM -0700, Moritz Fischer wrote:
> > > > > > From: Wu Hao
> > > > > >
> > > > > > In order to support virtualization usage via PCIe SRIOV, this patch
> > > > > > adds two ioctls under FPGA Management Engine (FME) to release and
> > > > > > assign back the port device. In order to safely turn Port from PF
> > > > > > into VF and enable PCIe SRIOV, it requires user to invoke this
> > > > > > PORT_RELEASE ioctl to release port firstly to remove userspace
> > > > > > interfaces, and then configure the PF/VF access register in FME.
> > > > > > After disable SRIOV, it requires user to invoke this PORT_ASSIGN
> > > > > > ioctl to attach the port back to PF.
> > > > > >
> > > > > > Ioctl interfaces:
> > > > > > * DFL_FPGA_FME_PORT_RELEASE
> > > > > >Release platform device of given port, it deletes port platform
> > > > > >device to remove related userspace interfaces on PF, then
> > > > > >configures PF/VF access mode to VF.
> > > > > >
> > > > > > * DFL_FPGA_FME_PORT_ASSIGN
> > > > > >Assign platform device of given port back to PF, it configures
> > > > > >PF/VF access mode to PF, then adds port platform device back to
> > > > > >re-enable related userspace interfaces on PF.
> > > > >
> > > > > Why are you not using the "generic" bind/unbind facility that
> > > > > userspace
> > > > > already has for this with binding drivers to devices? Why a special
> > > > > ioctl?
> > > >
> > > > Hi Greg,
> > > >
> > > > Actually we think it should be safer that making the device invisble
> > > > than
> > > > just unbinding its driver. Looks like user can try to rebind it at any
> > > > time and we don't have any method to stop them.
> > >
> > > Why do you want to "stop" the user from doing something? They asked to
> > > do it, why prevent it? If they ask to do something foolish, well, they
> > > get to keep the pieces :)
> >
> > Actually this is for SRIOV support, as we are moving FPGA accelerator from
> > PF to VF, so we don't want users to see the FPGA accelerator from PF any
> > more. We can't allow user to touch same FPGA accelerator from both PF and
> > VF side (it leads to hardware erros).
>
> Ick, ok, this needs to be documented really well then.
Yes, we have add relateded descriptions for virtualization support in that
documentation patch.
[PATCH 05/15] Documentation: fpga: dfl: add descriptions for virtualization
and new interfaces.
>
> > > > > > --- a/include/uapi/linux/fpga-dfl.h
> > > > > > +++ b/include/uapi/linux/fpga-dfl.h
> > > > > > @@ -176,4 +176,36 @@ struct dfl_fpga_fme_port_pr {
> > > > > >
> > > > > > #define DFL_FPGA_FME_PORT_PR _IO(DFL_FPGA_MAGIC,
> > > > > > DFL_FME_BASE + 0)
> > > > > >
> > > > > > +/**
> > > > > > + * DFL_FPGA_FME_PORT_RELEASE - _IOW(DFL_FPGA_MAGIC, DFL_FME_BASE +
> > > > > > 1,
> > > > > > + * struct
> > > > > > dfl_fpga_fme_port_release)
> > > > > > + *
> > > > > > + * Driver releases the port per Port ID provided by caller.
> > > > > > + * Return: 0 on success, -errno on failure.
> > > > > > + */
> > > > > > +struct dfl_fpga_fme_port_release {
> > > > > > + /* Input */
> > > > > > + __u32 argsz;/* Structure length */
> > > > > > + __u32 flags;/* Zero for now */
> > > > > > + __u32 port_id;
> > > > > > +};
> > > > >
> > > > > meta-comment, why do all of your structures for ioctls have argsz?
> > > > > You
> > > > > "know" the size of the structure already, it's part of the ioctl
> > > > > definition. You shouldn't need to also set it again, right?
> > > > > Otherwise
> > > > > ALL Linux ioctls would need something crazy like this.
> > > >
> > > > Actually we followed the same method as vfio.
> > >
> > > vfio is a protocol on "the wire", right? Not an ioctl.
> > >
> > > > The major purpose should be extendibility, as we really need this to
> > > > be sth long term maintainable.
> > >
> > > You can't change ioctl structure sizes at any time.
> > >
> > > > It really helps, if we add some new members for extentions/enhancement
> > > > under the same ioctl.
> > >
> > > You don't do that.
> > >
> > > > I don't think everybody needs this, but my consideration here is if
> > > > newer generations of hardware/specs come with some extentions, I still
> > > > hope we can resue these IOCTLs as much as we could, instead of
> > > > creating more new ones.
> > >
> > > You create new ones, like everyone else does, as you can not change old
> > > code. By trying to "version" structures like this, it's just going to
> > > be a nightmare.
> >
> > Actually i learned this from vfio code here, it's not trying to "version"
>
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
On Sat, 2019-06-29 at 11:01 -0400, Sasha Levin wrote: > On Thu, Jun 27, 2019 at 02:31:35AM +0300, Jarkko Sakkinen wrote: > > On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote: > > > +static const uuid_t ftpm_ta_uuid = > > > + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4, > > > + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96); > > > + > > > +/** > > > + * ftpm_tee_tpm_op_recv - retrieve fTPM response. > > > + * > > > > Should not have an empty line here. > > > > > + * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h. > > > + * @buf: the buffer to store data. > > > + * @count: the number of bytes to read. > > Jarkko, w.r.t your comment above, there is an empty line between the > function name and variables in drivers/char/tpm, and in particular > tpm_crb.c which you authored and I used as reference. Do you want us to > diverge here? There is divergence and that was the first thing I've contributed to the TPM driver. I use this as the reference for formatting function descriptions these days: https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt According to that the legit way to format would be: * ftpm_tee_tpm_op_recv() - retrieve fTPM response. * @chip:the tpm_chip description as specified in driver/char/tpm/tpm.h. * @buf: the buffer to store data. * @count: the number of bytes to read. Since it is both a callback to an interface defined elsewhere and a static function and it does not document anything useful, I would just remove this comment. I'd do it for all callbacks that are part of tpm_call_ops. /Jarkko
Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
On Thu, Jul 04, 2019 at 04:58:55PM +0800, Wu Hao wrote: > > > Hope things could be more clear now. :) > > > > That's nice for the vfio stuff, but you are just a "normal" driver here. > > You want an ioctl that just does one thing, no arguments, no flags, no > > anything. No need for a size argument then at all. These ioctls don't > > even need a structure for them! > > > > Don't try to be fancy, it's not needed, it's not like you are running > > out of ioctl space... > > Thanks a lot for the comments and suggestions. > > That's true, it's a "normal" driver, maybe I overly considered the > extensibility of it. OK, Let me rework this patch to remove argsz from > these two ioctls. > > What about the existing ioctls for this driver, they have argsz too. > shall I prepare another patch to remove them as well? I am hoping you actually have users for those ioctls in userspace today? If not, and no one is using them, then yes, please fix those too. thanks, greg k-h
Re: [PATCH 06/15] fpga: dfl: fme: add DFL_FPGA_FME_PORT_RELEASE/ASSIGN ioctl support.
On Thu, Jul 04, 2019 at 01:04:49PM +0200, Greg KH wrote: > On Thu, Jul 04, 2019 at 04:58:55PM +0800, Wu Hao wrote: > > > > Hope things could be more clear now. :) > > > > > > That's nice for the vfio stuff, but you are just a "normal" driver here. > > > You want an ioctl that just does one thing, no arguments, no flags, no > > > anything. No need for a size argument then at all. These ioctls don't > > > even need a structure for them! > > > > > > Don't try to be fancy, it's not needed, it's not like you are running > > > out of ioctl space... > > > > Thanks a lot for the comments and suggestions. > > > > That's true, it's a "normal" driver, maybe I overly considered the > > extensibility of it. OK, Let me rework this patch to remove argsz from > > these two ioctls. > > > > What about the existing ioctls for this driver, they have argsz too. > > shall I prepare another patch to remove them as well? > > I am hoping you actually have users for those ioctls in userspace today? > If not, and no one is using them, then yes, please fix those too. Yes, we have a few users, not many, e.g. https://github.com/OPAE/opae-sdk I believe we may have more users as we are submitting code to make this driver more usable. Let me think about this, if we want to do this clean up, we have to increase the API version to tell everybody, things are changed. If finally we decide to do this clean up, that will be a new patch after this patchset. Many Thanks for your patient guide and suggestions. :) Hao > > thanks, > > greg k-h
[PATCH v2 1/3] x86/boot: Introduce the kernel_info
The relationships between the headers are analogous to the various data sections: setup_header = .data boot_params/setup_data = .bss What is missing from the above list? That's right: kernel_info = .rodata We have been (ab)using .data for things that could go into .rodata or .bss for a long time, for lack of alternatives and -- especially early on -- inertia. Also, the BIOS stub is responsible for creating boot_params, so it isn't available to a BIOS-based loader (setup_data is, though). setup_header is permanently limited to 144 bytes due to the reach of the 2-byte jump field, which doubles as a length field for the structure, combined with the size of the "hole" in struct boot_params that a protected-mode loader or the BIOS stub has to copy it into. It is currently 119 bytes long, which leaves us with 25 very precious bytes. This isn't something that can be fixed without revising the boot protocol entirely, breaking backwards compatibility. boot_params proper is limited to 4096 bytes, but can be arbitrarily extended by adding setup_data entries. It cannot be used to communicate properties of the kernel image, because it is .bss and has no image-provided content. kernel_info solves this by providing an extensible place for information about the kernel image. It is readonly, because the kernel cannot rely on a bootloader copying its contents anywhere, but that is OK; if it becomes necessary it can still contain data items that an enabled bootloader would be expected to copy into a setup_data chunk. This patch does not bump setup_header version in arch/x86/boot/header.S because it will be followed by additional changes coming into the Linux/x86 boot protocol. Suggested-by: H. Peter Anvin Signed-off-by: Daniel Kiper Reviewed-by: Eric Snowberg Reviewed-by: Ross Philipson --- v2 - suggestions/fixes: - rename setup_header2 to kernel_info, (suggested by H. Peter Anvin), - change kernel_info.header value to "InfO" (0x4f666e49), - new kernel_info description in Documentation/x86/boot.rst, (suggested by H. Peter Anvin), - drop kernel_info_offset_update() as an overkill and update kernel_info offset directly from main(), (suggested by Eric Snowberg), - new commit message (suggested by H. Peter Anvin), - fix some commit message misspellings (suggested by Eric Snowberg). --- Documentation/x86/boot.rst | 89 ++ arch/x86/boot/Makefile | 2 +- arch/x86/boot/compressed/Makefile | 4 +- arch/x86/boot/compressed/kernel_info.S | 12 + arch/x86/boot/header.S | 1 + arch/x86/boot/tools/build.c| 5 ++ arch/x86/include/uapi/asm/bootparam.h | 1 + 7 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 arch/x86/boot/compressed/kernel_info.S diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst index 08a2f100c0e6..a934a56f0516 100644 --- a/Documentation/x86/boot.rst +++ b/Documentation/x86/boot.rst @@ -68,8 +68,25 @@ Protocol 2.12(Kernel 3.8) Added the xloadflags field and extension fields Protocol 2.13 (Kernel 3.14) Support 32- and 64-bit flags being set in xloadflags to support booting a 64-bit kernel from 32-bit EFI + +Protocol 2.14: BURNT BY INCORRECT COMMIT ae7e1238e68f2a472a125673ab506d49158c1889 + (x86/boot: Add ACPI RSDP address to setup_header) + DO NOT USE!!! ASSUME SAME AS 2.13. + +Protocol 2.15: (Kernel 5.3) Added the kernel_info. = +.. note:: + The protocol version number should be changed only if the setup header + is changed. There is no need to update the version number if boot_params + or kernel_info are changed. Additionally, it is recommended to use + xloadflags (in this case the protocol version number should not be + updated either) or kernel_info to communicate supported Linux kernel + features to the boot loader. Due to very limited space available in + the original setup header every update to it should be considered + with great care. Starting from the protocol 2.15 the primary way to + communicate things to the boot loader is the kernel_info. + Memory Layout = @@ -207,6 +224,7 @@ Offset/Size Proto NameMeaning 0258/8 2.10+ pref_addressPreferred loading address 0260/4 2.10+ init_size Linear memory required during initialization 0264/4 2.11+ handover_offset Offset of handover entry point +0268/4 2.15+ kernel_info_offset Offset of the kernel_info ==== .. note:: @@ -855,6 +873,77 @@ Offset/size: 0x264/4 See EFI HANDOVER PROTOCOL below for more details. +=
[PATCH v2 3/3] x86/boot: Introduce the kernel_info.setup_type_max
This field contains maximal allowed type for setup_data and setup_indirect structs. And finally bump setup_header version in arch/x86/boot/header.S. Suggested-by: H. Peter Anvin Signed-off-by: Daniel Kiper Reviewed-by: Ross Philipson Reviewed-by: Eric Snowberg --- Documentation/x86/boot.rst | 10 +- arch/x86/boot/compressed/kernel_info.S | 4 arch/x86/boot/header.S | 2 +- arch/x86/include/uapi/asm/bootparam.h | 3 +++ 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst index 23d3726d54fc..63609fd0517f 100644 --- a/Documentation/x86/boot.rst +++ b/Documentation/x86/boot.rst @@ -73,7 +73,8 @@ Protocol 2.14:BURNT BY INCORRECT COMMIT ae7e1238e68f2a472a125673ab506d49158c188 (x86/boot: Add ACPI RSDP address to setup_header) DO NOT USE!!! ASSUME SAME AS 2.13. -Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect. +Protocol 2.15: (Kernel 5.3) Added the kernel_info, kernel_info.setup_type_max + and setup_indirect. = .. note:: @@ -980,6 +981,13 @@ Offset/size: 0x0004/4 This field contains the size of the kernel_info including kernel_info.header. It should be used by the boot loader to detect supported fields in the kernel_info. + == +Field name:setup_type_max +Offset/size: 0x0008/4 + == + + This field contains maximal allowed type for setup_data and setup_indirect structs. + The Image Checksum == diff --git a/arch/x86/boot/compressed/kernel_info.S b/arch/x86/boot/compressed/kernel_info.S index 3f1cb301b9ff..2f28aabf6558 100644 --- a/arch/x86/boot/compressed/kernel_info.S +++ b/arch/x86/boot/compressed/kernel_info.S @@ -1,5 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0 */ +#include + .section ".rodata.kernel_info", "a" .global kernel_info @@ -9,4 +11,6 @@ kernel_info: .ascii "InfO" /* Size. */ .long kernel_info_end - kernel_info +/* Maximal allowed type for setup_data and setup_indirect structs. */ + .long SETUP_TYPE_MAX kernel_info_end: diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S index ec6a25a43148..893a456663ab 100644 --- a/arch/x86/boot/header.S +++ b/arch/x86/boot/header.S @@ -300,7 +300,7 @@ _start: # Part 2 of the header, from the old setup.S .ascii "HdrS" # header signature - .word 0x020d # header version number (>= 0x0105) + .word 0x020f # header version number (>= 0x0105) # or else old loadlin-1.5 will fail) .globl realmode_swtch realmode_swtch:.word 0, 0# default_switch, SETUPSEG diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h index 17fa6ad6..2ba870dae6f3 100644 --- a/arch/x86/include/uapi/asm/bootparam.h +++ b/arch/x86/include/uapi/asm/bootparam.h @@ -12,6 +12,9 @@ #define SETUP_JAILHOUSE6 #define SETUP_INDIRECT 7 +/* max(SETUP_*) */ +#define SETUP_TYPE_MAX SETUP_INDIRECT + /* ram_size flags */ #define RAMDISK_IMAGE_START_MASK 0x07FF #define RAMDISK_PROMPT_FLAG0x8000 -- 2.11.0
[PATCH v2 2/3] x86/boot: Introduce the setup_indirect
The setup_data is a bit awkward to use for extremely large data objects,
both because the setup_data header has to be adjacent to the data object
and because it has a 32-bit length field. However, it is important that
intermediate stages of the boot process have a way to identify which
chunks of memory are occupied by kernel data.
Thus we introduce a uniform way to specify such indirect data as
setup_indirect struct and SETUP_INDIRECT type.
This patch does not bump setup_header version in arch/x86/boot/header.S
because it will be followed by additional changes coming into the
Linux/x86 boot protocol.
Suggested-by: H. Peter Anvin
Signed-off-by: Daniel Kiper
Reviewed-by: Eric Snowberg
Reviewed-by: Ross Philipson
---
v2 - suggestions/fixes:
- add setup_indirect usage example
(suggested by Eric Snowberg and Ross Philipson).
---
Documentation/x86/boot.rst| 38 ++-
arch/x86/include/uapi/asm/bootparam.h | 11 +-
2 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/boot.rst b/Documentation/x86/boot.rst
index a934a56f0516..23d3726d54fc 100644
--- a/Documentation/x86/boot.rst
+++ b/Documentation/x86/boot.rst
@@ -73,7 +73,7 @@ Protocol 2.14:BURNT BY INCORRECT COMMIT
ae7e1238e68f2a472a125673ab506d49158c188
(x86/boot: Add ACPI RSDP address to setup_header)
DO NOT USE!!! ASSUME SAME AS 2.13.
-Protocol 2.15: (Kernel 5.3) Added the kernel_info.
+Protocol 2.15: (Kernel 5.3) Added the kernel_info and setup_indirect.
=
.. note::
@@ -827,6 +827,42 @@ Protocol: 2.09+
sure to consider the case where the linked list already contains
entries.
+ The setup_data is a bit awkward to use for extremely large data objects,
+ both because the setup_data header has to be adjacent to the data object
+ and because it has a 32-bit length field. However, it is important that
+ intermediate stages of the boot process have a way to identify which
+ chunks of memory are occupied by kernel data.
+
+ Thus setup_indirect struct and SETUP_INDIRECT type were introduced in
+ protocol 2.15.
+
+ struct setup_indirect {
+__u32 type;
+__u32 reserved; /* Reserved, must be set to zero. */
+__u64 len;
+__u64 addr;
+ };
+
+ The type member is itself simply a SETUP_* type. However, it cannot be
+ SETUP_INDIRECT since making the setup_indirect a tree structure could
+ require a lot of stack space in something that needs to parse it and
+ stack space can be limited in boot contexts.
+
+ Let's give an example how to point to SETUP_E820_EXT data using
setup_indirect.
+ In this case setup_data and setup_indirect will look like this:
+
+ struct setup_data {
+__u64 next = 0 or ;
+__u32 type = SETUP_INDIRECT;
+__u32 len = sizeof(setup_data);
+__u8 data[sizeof(setup_indirect)] = struct setup_indirect {
+ __u32 type = SETUP_E820_EXT;
+ __u32 reserved = 0;
+ __u64 len = ;
+ __u64 addr = ;
+}
+ }
+
Field name:pref_address
Type: read (reloc)
diff --git a/arch/x86/include/uapi/asm/bootparam.h
b/arch/x86/include/uapi/asm/bootparam.h
index b05318112452..17fa6ad6 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -2,7 +2,7 @@
#ifndef _ASM_X86_BOOTPARAM_H
#define _ASM_X86_BOOTPARAM_H
-/* setup_data types */
+/* setup_data/setup_indirect types */
#define SETUP_NONE 0
#define SETUP_E820_EXT 1
#define SETUP_DTB 2
@@ -10,6 +10,7 @@
#define SETUP_EFI 4
#define SETUP_APPLE_PROPERTIES 5
#define SETUP_JAILHOUSE6
+#define SETUP_INDIRECT 7
/* ram_size flags */
#define RAMDISK_IMAGE_START_MASK 0x07FF
@@ -47,6 +48,14 @@ struct setup_data {
__u8 data[0];
};
+/* extensible setup indirect data node */
+struct setup_indirect {
+ __u32 type;
+ __u32 reserved; /* Reserved, must be set to zero. */
+ __u64 len;
+ __u64 addr;
+};
+
struct setup_header {
__u8setup_sects;
__u16 root_flags;
--
2.11.0
[PATCH v2 0/3] x86/boot: Introduce the kernel_info et consortes
Hi, Due to very limited space in the setup_header this patch series introduces new kernel_info struct which will be used to convey information from the kernel to the bootloader. This way the boot protocol can be extended regardless of the setup_header limitations. Additionally, the patch series introduces some convenience features like the setup_indirect struct and the kernel_info.setup_type_max field. Daniel Documentation/x86/boot.rst | 133 ++ arch/x86/boot/Makefile | 2 +- arch/x86/boot/compressed/Makefile | 4 +-- arch/x86/boot/compressed/kernel_info.S | 16 ++ arch/x86/boot/header.S | 3 +- arch/x86/boot/tools/build.c| 5 +++ arch/x86/include/uapi/asm/bootparam.h | 15 - 7 files changed, 173 insertions(+), 5 deletions(-) Daniel Kiper (3): x86/boot: Introduce the kernel_info x86/boot: Introduce the setup_indirect x86/boot: Introduce the kernel_info.setup_type_max
Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
Hi Thirupathaiah, [...] > > > > > I managed to do some quick testing in QEMU. > > > > > Everything works fine when i build this as a module (using IBM's TPM > > > > > 2.0 > > > > > TSS) > > > > > > > > > > - As module > > > > > # insmod > > > > > /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko > > > > > # getrandom -by 8 > > > > > randomBytes length 8 > > > > > 23 b9 3d c3 90 13 d9 6b > > > > > > > > > > - Built-in > > > > > # dmesg | grep optee > > > > > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session > > > > > failed, > > > > > err=0008 > > > > This (0x0008) translates to TEE_ERROR_ITEM_NOT_FOUND. > > > > > > > > Where is fTPM TA located in the your test setup? > > > > Is it stitched into TEE binary as an EARLY_TA or > > > > Is it expected to be loaded during run-time with the help of user mode > > > > OP- > > TEE supplicant? > > > > > > > > My guess is that you are trying to load fTPM TA through user mode OP-TEE > > supplicant. > > > > Can you confirm? > > > I tried both > > > > > > > Ok apparently there was a failure with my built-in binary which i > > didn't notice. I did a full rebuilt and checked the elf this time :) > > > > Built as an earlyTA my error now is: > > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session > > failed, err=3024 (translates to TEE_ERROR_TARGET_DEAD) > > Since you tested it on real hardware i guess you tried both > > module/built-in. Which TEE version are you using? > > I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after > stitching > fTPM TA as an EARLY_TA. > > Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are > using to test? QEMU, on armv7 > What is the preboot environment (UEFI or U-boot)? > Where is the secure storage in that HW platform? > I could think of two classes of secure storage. > 1. UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the > fTPM TA NV Storage, there should be no issue. > If fTPM TA NV storage is not initialized in pre-boot environment and you are > using > built-in fTPM Linux driver, you can run into this issue as TA will try to > initialize > NV store and fail. > > 2. other storage devices like QSPI accessible to only secure mode after > EBS/ReadyToBoot mile posts during boot. In this case, there should be no > issue at all > as there is no dependency on non-secure side services provided by supplicant. > Please check the previous mail from Sumit. It explains exaclty what's going on. The tl;dr version is that the storage is up only when the supplicant is running. > If you let me know the HW platform details, I am happy to work with you to > enable/integrate > fTPM TA on that HW platform. > Thanks, The hardware i am waiting for for has an eMMC RPMB. In theory the U-Boot supplicant support will be there so i'll be able to test it. Thanks /Ilias
[PATCH] Documentation: input: Add HID gadget driver's docs to Input subsystem
Convert gadget_hid file to ReST format, in order to allow it to
be parsed by Sphinx.
Also move the file in the Input subsystem documentation so as to
put it in the right place.
Signed-off-by: Shreeya Patel
---
.../devices/gadget_hid.rst}| 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
rename Documentation/{usb/gadget_hid.txt => input/devices/gadget_hid.rst} (96%)
diff --git a/Documentation/usb/gadget_hid.txt
b/Documentation/input/devices/gadget_hid.rst
similarity index 96%
rename from Documentation/usb/gadget_hid.txt
rename to Documentation/input/devices/gadget_hid.rst
index 098d563040cc..132a8d6719f0 100644
--- a/Documentation/usb/gadget_hid.txt
+++ b/Documentation/input/devices/gadget_hid.rst
@@ -1,3 +1,5 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
===
Linux USB HID gadget driver
===
@@ -8,15 +10,15 @@ Introduction
The HID Gadget driver provides emulation of USB Human Interface
Devices (HID). The basic HID handling is done in the kernel,
and HID reports can be sent/received through I/O on the
-/dev/hidgX character devices.
+:file:`/dev/hidgX` character devices.
For more details about HID, see the developer page on
-http://www.usb.org/developers/hidpage/
+`
Re: [RFC PATCH] binfmt_elf: Extract .note.gnu.property from an ELF file
Hi!
> > +static int scan(u8 *buf, u32 buf_size, int item_size, test_item_fn
> > test_item,
> > + next_item_fn next_item, u32 *arg, u32 type, u32 *pos)
> > +{
> > + int found = 0;
> > + u8 *p, *max;
> > +
> > + max = buf + buf_size;
> > + if (max < buf)
> > + return 0;
>
> How can this ever legitimately happen? If it can't, perhaps you meant
> to put a WARN_ON_ONCE() or something like that here?
> Also, computing out-of-bounds pointers is UB (section 6.5.6 of C99:
> "If both the pointer operand and the result point to elements of the
> same array object, or one past the last element of the array object,
> the evaluation shall not produce an overflow; otherwise, the behavior
> is undefined."), and if the addition makes the pointer wrap, that's
> certainly out of bounds; so I don't think this condition can trigger
> without UB.
Kernel assumes sane compiler. We pass flags to get it... C99 does not
quite apply here.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH] checkpatch: Added warnings in favor of strscpy().
On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> Added warnings in checkpatch.pl script to :
>
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
>
> Updated strncpy() section in Documentation/process/deprecated.rst
> to cover strscpy_pad() case.
>
> Signed-off-by: Nitin Gote
OK, for whatever reason, this when into a spam folder.
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -595,6 +595,11 @@ our %deprecated_apis = (
> "rcu_barrier_sched" => "rcu_barrier",
> "get_state_synchronize_sched" => "get_state_synchronize_rcu",
> "cond_synchronize_sched"=> "cond_synchronize_rcu",
> + "strcpy"=> "strscpy",
> + "strlcpy" => "strscpy",
> + "strncpy" => "strscpy, strscpy_pad or for
> + non-NUL-terminated strings, strncpy() can still be used, but
> + destinations should be marked with the __nonstring",
> );
$ git grep -w strcpy | wc -l
2239
$ git grep -w strlcpy | wc -l
1760
$ git grep -w strncpy | wc -l
839
These functions are _really_ commonly used in the kernel.
This should probably be a different %deprecated_string_api
and these should probably not be emitted at WARN level
when using command line option -f/--file but at CHECK level
so that novice script users just don't send bad patches.
Also, perhaps there could be some macro for the relatively
commonly used
strscpy(foo, bar, sizeof(foo))
and
strlcpy(foo, bar, sizeof(foo))
so argument 1 doesn't have to be repeated in the sizeof()
Something like:
#define stracpy(to, from) \
({ \
size_t size = ARRAY_SIZE(to); \
BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
\
strscpy(to, from, size);\
})
Re: [PATCH] docs: packing: move it to core-api book and adjust markups
On Sat, 29 Jun 2019 at 13:37, Mauro Carvalho Chehab
wrote:
>
> The packing.txt file was misplaced, as docs should be part of
> a documentation book, and not at the root dir.
>
> So, move it to the core-api directory and add to its index.
>
> Also, ensure that the file will be properly parsed and the bitmap
> ascii artwork will use a monotonic font.
>
> Fixes: 554aae35007e ("lib: Add support for generic packing operations")
> Signed-off-by: Mauro Carvalho Chehab
Reviewed-by: Vladimir Oltean
Tested-by: Vladimir Oltean
> ---
> Documentation/core-api/index.rst | 1 +
> .../{packing.txt => core-api/packing.rst} | 81 +++
> 2 files changed, 50 insertions(+), 32 deletions(-)
> rename Documentation/{packing.txt => core-api/packing.rst} (61%)
>
> diff --git a/Documentation/core-api/index.rst
> b/Documentation/core-api/index.rst
> index d1e5b95bf86d..aebb16d7771f 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -25,6 +25,7 @@ Core utilities
> librs
> genalloc
> errseq
> + packing
> printk-formats
> circular-buffers
> generic-radix-tree
> diff --git a/Documentation/packing.txt b/Documentation/core-api/packing.rst
> similarity index 61%
> rename from Documentation/packing.txt
> rename to Documentation/core-api/packing.rst
> index f830c98645f1..d8c341fe383e 100644
> --- a/Documentation/packing.txt
> +++ b/Documentation/core-api/packing.rst
> @@ -30,6 +30,7 @@ The solution
>
>
> This API deals with 2 basic operations:
> +
>- Packing a CPU-usable number into a memory buffer (with hardware
> constraints/quirks)
>- Unpacking a memory buffer (which has hardware constraints/quirks)
> @@ -49,10 +50,12 @@ What the examples show is where the logical bytes and
> bits sit.
>
> 1. Normally (no quirks), we would do it like this:
>
> -63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39
> 38 37 36 35 34 33 32
> -7 6 54
> -31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7
> 6 5 4 3 2 1 0
> -3 2 10
> +::
> +
> + 63 62 61 60 59 58 57 56 55 54 53 52 51 50 49 48 47 46 45 44 43 42 41 40 39
> 38 37 36 35 34 33 32
> + 7 6 54
> + 31 30 29 28 27 26 25 24 23 22 21 20 19 18 17 16 15 14 13 12 11 10 9 8 7
> 6 5 4 3 2 1 0
> + 3 2 10
>
> That is, the MSByte (7) of the CPU-usable u64 sits at memory offset 0, and
> the
> LSByte (0) of the u64 sits at memory offset 7.
> @@ -63,10 +66,12 @@ comments as "logical" notation.
>
> 2. If QUIRK_MSB_ON_THE_RIGHT is set, we do it like this:
>
> -56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32
> 33 34 35 36 37 38 39
> -7 65 4
> -24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0
> 1 2 3 4 5 6 7
> -3 21 0
> +::
If this is not too stylistically different from the rest of the kernel
docs, the RST syntax actually allows you to do "we do it like this::"
(with the two colons coming right after the text and not on their own
line, which looks more natural). The same comment applies to the other
changes below.
> +
> + 56 57 58 59 60 61 62 63 48 49 50 51 52 53 54 55 40 41 42 43 44 45 46 47 32
> 33 34 35 36 37 38 39
> + 7 65 4
> + 24 25 26 27 28 29 30 31 16 17 18 19 20 21 22 23 8 9 10 11 12 13 14 15 0
> 1 2 3 4 5 6 7
> + 3 21 0
>
> That is, QUIRK_MSB_ON_THE_RIGHT does not affect byte positioning, but
> inverts bit offsets inside a byte.
> @@ -74,10 +79,12 @@ inverts bit offsets inside a byte.
>
> 3. If QUIRK_LITTLE_ENDIAN is set, we do it like this:
>
> -39 38 37 36 35 34 33 32 47 46 45 44 43 42 41 40 55 54 53 52 51 50 49 48 63
> 62 61 60 59 58 57 56
> -4 5 6 7
> -7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8 23 22 21 20 19 18 17 16 31
> 30 29 28 27 26 25 24
> -0 1 2 3
> +::
> +
> + 39 38 37 36 35 34 33 32 47 46 45 44 43 42 41 40 55 54 53 52 51 50 49 48 63
> 62 61 60 59 58 57 56
> + 4 5 6 7
> + 7 6 5 4 3 2 1 0 15 14 13 12 11 10 9 8 23 22 21 20 19 18 17 16 31
> 30 29 28 27 26 25 24
> + 0 1 2 3
>
> Therefore, QUIRK_LITTLE_ENDIAN means that inside the memory region, every
> byte from each 4-byte word is placed at its
[RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)
On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > Added warnings in checkpatch.pl script to :
> >
> > 1. Deprecate strcpy() in favor of strscpy().
> > 2. Deprecate strlcpy() in favor of strscpy().
> > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> >
> > Updated strncpy() section in Documentation/process/deprecated.rst
> > to cover strscpy_pad() case.
[]
I sent a patch series for some strscpy/strlcpy misuses.
How about adding a macro helper to avoid the misuses like:
---
include/linux/string.h | 16
1 file changed, 16 insertions(+)
diff --git a/include/linux/string.h b/include/linux/string.h
index 4deb11f7976b..ef01bd6f19df 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
/* Wraps calls to strscpy()/memset(), no arch specific code required */
ssize_t strscpy_pad(char *dest, const char *src, size_t count);
+#define stracpy(to, from) \
+({ \
+ size_t size = ARRAY_SIZE(to); \
+ BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
+ \
+ strscpy(to, from, size);\
+})
+
+#define stracpy_pad(to, from) \
+({ \
+ size_t size = ARRAY_SIZE(to); \
+ BUILD_BUG_ON(!__same_type(typeof(*to), char)); \
+ \
+ strscpy_pad(to, from, size);\
+})
+
#ifndef __HAVE_ARCH_STRCAT
extern char * strcat(char *, const char *);
#endif
RE: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
> -Original Message- > From: Ilias Apalodimas > Sent: Thursday, July 4, 2019 11:11 AM > To: Thirupathaiah Annapureddy > Cc: Jarkko Sakkinen ; Sasha Levin > ; peterhu...@gmx.de; j...@ziepe.ca; cor...@lwn.net; linux- > ker...@vger.kernel.org; linux-doc@vger.kernel.org; linux- > integr...@vger.kernel.org; Microsoft Linux Kernel List ker...@microsoft.com>; Bryan Kelly (CSI) ; tee- > d...@lists.linaro.org; sumit.g...@linaro.org; rdun...@infradead.org; Joakim > Bech > > Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE > > Hi Thirupathaiah, > [...] > > > > > > I managed to do some quick testing in QEMU. > > > > > > Everything works fine when i build this as a module (using IBM's TPM > 2.0 > > > > > > TSS) > > > > > > > > > > > > - As module > > > > > > # insmod /lib/modules/5.2.0- > rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko > > > > > > # getrandom -by 8 > > > > > > randomBytes length 8 > > > > > > 23 b9 3d c3 90 13 d9 6b > > > > > > > > > > > > - Built-in > > > > > > # dmesg | grep optee > > > > > > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session > failed, > > > > > > err=0008 > > > > > This (0x0008) translates to TEE_ERROR_ITEM_NOT_FOUND. > > > > > > > > > > Where is fTPM TA located in the your test setup? > > > > > Is it stitched into TEE binary as an EARLY_TA or > > > > > Is it expected to be loaded during run-time with the help of user mode > OP- > > > TEE supplicant? > > > > > > > > > > My guess is that you are trying to load fTPM TA through user mode OP- > TEE > > > supplicant. > > > > > Can you confirm? > > > > I tried both > > > > > > > > > > Ok apparently there was a failure with my built-in binary which i > > > didn't notice. I did a full rebuilt and checked the elf this time :) > > > > > > Built as an earlyTA my error now is: > > > ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session > > > failed, err=3024 (translates to TEE_ERROR_TARGET_DEAD) > > > Since you tested it on real hardware i guess you tried both > > > module/built-in. Which TEE version are you using? > > > > I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after > stitching > > fTPM TA as an EARLY_TA. > > > > Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are > using to test? > > QEMU, on armv7 > > > What is the preboot environment (UEFI or U-boot)? > > Where is the secure storage in that HW platform? > > I could think of two classes of secure storage. > > 1. UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the > > fTPM TA NV Storage, there should be no issue. > > If fTPM TA NV storage is not initialized in pre-boot environment and you are > using > > built-in fTPM Linux driver, you can run into this issue as TA will try to > initialize > > NV store and fail. > > > > 2. other storage devices like QSPI accessible to only secure mode after > > EBS/ReadyToBoot mile posts during boot. In this case, there should be no > issue at all > > as there is no dependency on non-secure side services provided by > > supplicant. > > > > Please check the previous mail from Sumit. It explains exaclty what's going > on. > The tl;dr version is that the storage is up only when the supplicant is > running. I definitely know that OP-TEE can access storage only when the "user mode" supplicant is running :). But fTPM NV storage should have been initialized in in the preboot environment (UEFI/U-boot). It would also be helpful to understand the overall use case/scenario (Measured boot?)you are trying to exercise with the fTPM. I also want to emphasize that this discussion is turning into more of how fTPM gets integrated/enabled in a new HW platform. fTPM is hosted in github and you definitely bring any issues/feature requests there. > > > If you let me know the HW platform details, I am happy to work with you to > enable/integrate > > fTPM TA on that HW platform. > > > Thanks, > The hardware i am waiting for for has an eMMC RPMB. In theory the U-Boot > supplicant support will be there so i'll be able to test it. Can you give me the details of HW so that I can order one for myself? Is it one of the 96boards? The reason for the ask is that we have not upstreamd u-boot fTPM stack yet, although we have future plans for it. --Thiru
Re: [PATCH] Documentation: dmaengine: clean up description of dmatest usage
On 24-06-19, 18:35, Hook, Gary wrote: > Fix the formatting of the multi-channel test usage example. Call out > the note about parameter ordering and add detail on the settings of > parameters for the new version of dmatest. Applied, thanks -- ~Vinod
Re: [PATCH] Documentation: gpio: Fix reference to gpiod_get_array()
On Mon, Jul 1, 2019 at 4:10 PM Geert Uytterhoeven
wrote:
> The function is called gpiod_get_array(), not gpiod_array_get().
>
> Fixes: 77588c14ac868cae ("gpiolib: Pass array info to get/set array
> functions")
> Signed-off-by: Geert Uytterhoeven
Patch applied.
Yours,
Linus Walleij
Re: [PATCH] mm, slab: Extend slab/shrink to shrink all the memcg caches
On Wed 03-07-19 12:16:09, Waiman Long wrote: > On 7/3/19 11:53 AM, Michal Hocko wrote: > > On Wed 03-07-19 11:21:16, Waiman Long wrote: > >> On 7/2/19 5:33 PM, Andrew Morton wrote: > >>> On Tue, 2 Jul 2019 16:44:24 -0400 Waiman Long wrote: > >>> > On 7/2/19 4:03 PM, Andrew Morton wrote: > > On Tue, 2 Jul 2019 14:37:30 -0400 Waiman Long > > wrote: > > > >> Currently, a value of '1" is written to /sys/kernel/slab//shrink > >> file to shrink the slab by flushing all the per-cpu slabs and free > >> slabs in partial lists. This applies only to the root caches, though. > >> > >> Extends this capability by shrinking all the child memcg caches and > >> the root cache when a value of '2' is written to the shrink sysfs file. > > Why? > > > > Please fully describe the value of the proposed feature to or users. > > Always. > Sure. Essentially, the sysfs shrink interface is not complete. It allows > the root cache to be shrunk, but not any of the memcg caches. > >>> But that doesn't describe anything of value. Who wants to use this, > >>> and why? How will it be used? What are the use-cases? > >>> > >> For me, the primary motivation of posting this patch is to have a way to > >> make the number of active objects reported in /proc/slabinfo more > >> accurately reflect the number of objects that are actually being used by > >> the kernel. > > I believe we have been through that. If the number is inexact due to > > caching then lets fix slabinfo rather than trick around it and teach > > people to do a magic write to some file that will "solve" a problem. > > This is exactly what drop_caches turned out to be in fact. People just > > got used to drop caches because they were told so by $random web page. > > So really, think about the underlying problem and try to fix it. > > > > It is true that you could argue that this patch is actually fixing the > > existing interface because it doesn't really do what it is documented to > > do and on those grounds I would agree with the change. > > I do think that we should correct the shrink file to do what it is > designed to do to include the memcg caches as well. > > > > But do not teach > > people that they have to write to some file to get proper numbers. > > Because that is just a bad idea and it will kick back the same way > > drop_caches. > > The /proc/slabinfo file is a well-known file that is probably used > relatively extensively. Making it to scan through all the per-cpu > structures will probably cause performance issues as the slab_mutex has > to be taken during the whole duration of the scan. That could have > undesirable side effect. Please be more specific with some numbers ideally. Also if collecting data is too expensive, why cannot we simply account cached objects count in pcp manner? > Instead, I am thinking about extending the slab/objects sysfs file to > also show the number of objects hold up by the per-cpu structures and > thus we can get an accurate count by subtracting it from the reported > active objects. That will have a more limited performance impact as it > is just one kmem cache instead of all the kmem caches in the system. > Also the sysfs files are not as commonly used as slabinfo. That will be > another patch in the near future. Both are root only and once it is widespread that slabinfo doesn't provide precise data you can expect tools will try to fix that by adding another file(s) and we are back to square one, no? In other words slabinfo -- Michal Hocko SUSE Labs
