* Re: [PATCH] x86/MCE: Remove MCP_TIMESTAMP
From: Thomas Gleixner @ 2016-11-08 20:39 UTC (permalink / raw)
To: Borislav Petkov; +Cc: Luck, Tony, linux-edac, X86 ML, LKML
In-Reply-To: <20161108180932.wvvcbbzrzrai26eg@pd.tnic>
On Tue, 8 Nov 2016, Borislav Petkov wrote:
>
> Also, this fixes another bug where machine_check_poll() would clear
> mce.tsc unconditionally even if we requested precise MCP_TIMESTAMP
> logging.
> @@ -713,7 +713,6 @@ bool machine_check_poll(enum mcp_flags flags, mce_banks_t *b)
> m.misc = 0;
> m.addr = 0;
> m.bank = i;
> - m.tsc = 0;
That does not make any sense. Where is m.tsc initialized? I couldn't find
any place which does, except this and the conditional clear farther down in
that function.
Thanks,
tglx
^ permalink raw reply
* [Buildroot] [PATCH] configs: freescale_imx31_3stack: bump kernel version to 4.1.15_2.0.0_ga
From: Vincent Stehle @ 2016-11-08 20:41 UTC (permalink / raw)
To: buildroot
In-Reply-To: <20161105143516.00291507@free-electrons.com>
From: Vincent Stehl? <vincent.stehle@laposte.net>
Switch to the kernel of release 4.1.15_2.0.0_ga, as it builds properly with
gcc 5.x, which is now the default.
We add a linux config fragment to disable the framebuffer, to repair the
build for imx_v6_v7_defconfig.
Suggested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Vincent Stehl? <vincent.stehle@laposte.net>
Signed-off-by: Julien Olivain <juju@cotds.org>
Tested-by: Julien Olivain <juju@cotds.org>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
---
Hello Thomas and Fabio,
My friend Julien got the i.MX31 PDK and he could kindly test a few
buildroot configs!
His experiments show that mainline kernel will not boot "as is" on the PDK.
What he proposed instead is that we switch to a recent NXP/Freescale
release based on kernel 4.1, which does boot on the PDK.
Is that proposal fine with you, please?
Best regards,
Vincent.
board/freescale/imx31_3stack/linux.fragment | 1 +
configs/freescale_imx31_3stack_defconfig | 12 ++++++------
2 files changed, 7 insertions(+), 6 deletions(-)
create mode 100644 board/freescale/imx31_3stack/linux.fragment
diff --git a/board/freescale/imx31_3stack/linux.fragment b/board/freescale/imx31_3stack/linux.fragment
new file mode 100644
index 0000000..8d89e8e
--- /dev/null
+++ b/board/freescale/imx31_3stack/linux.fragment
@@ -0,0 +1 @@
+CONFIG_FB_MXS=n
diff --git a/configs/freescale_imx31_3stack_defconfig b/configs/freescale_imx31_3stack_defconfig
index bf1afad..0476fc1 100644
--- a/configs/freescale_imx31_3stack_defconfig
+++ b/configs/freescale_imx31_3stack_defconfig
@@ -3,18 +3,18 @@ BR2_arm=y
BR2_arm1136jf_s=y
BR2_ARM_EABIHF=y
-# Linux headers same as kernel, a 3.15 series
-BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_3_15=y
+# Linux headers same as kernel, a 4.1 series
+BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_4_1=y
# system
BR2_TARGET_GENERIC_GETTY_PORT="ttymxc0"
# kernel
BR2_LINUX_KERNEL=y
-# Note: sadly the Linux kernel will not boot on the i.MX31 PDK, starting with
-# v3.16 and at least up to v4.0-rc4; this is why we use v3.15.y here.
-BR2_LINUX_KERNEL_CUSTOM_VERSION=y
-BR2_LINUX_KERNEL_CUSTOM_VERSION_VALUE="3.15.10"
+BR2_LINUX_KERNEL_CUSTOM_GIT=y
+BR2_LINUX_KERNEL_CUSTOM_REPO_URL="git://git.freescale.com/imx/linux-imx.git"
+BR2_LINUX_KERNEL_CUSTOM_REPO_VERSION="rel_imx_4.1.15_2.0.0_ga"
BR2_LINUX_KERNEL_DEFCONFIG="imx_v6_v7"
+BR2_LINUX_KERNEL_CONFIG_FRAGMENT_FILES="board/freescale/imx31_3stack/linux.fragment"
BR2_TARGET_ROOTFS_CPIO_GZIP=y
BR2_TARGET_ROOTFS_INITRAMFS=y
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] [RFC] net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause
From: Timur Tabi @ 2016-11-08 20:43 UTC (permalink / raw)
To: Florian Fainelli, David Miller, netdev
In-Reply-To: <5820AC1D.4040408@codeaurora.org>
On 11/07/2016 10:30 AM, Timur Tabi wrote:
>
> I'm still don't understand 100% how these flags really work, because I
> just can't shake the feeling that they should not be set for every phy.
> If these flags are supposed to be turned on universally, then why are
> they even an option?
So I've been giving this more thought. Can you tell me if the following
is correct:
1) PHY drivers and/or phylib sets the SUPPORTED_Pause |
SUPPORTED_AsymPause bits in phydev->supported. This indicates that the
PHY supports pause frames.
2) The MAC driver checks phydev->supported before it calls phy_start().
If (SUPPORTED_Pause | SUPPORTED_AsymPause) is set, then it sets those
bits in phydev->advertising if it wants to enable pause frame support.
3) When the link state changes, the MAC driver checks
phydev->advertising, and if the bits are set, then it enables those
features in the MAC.
--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH] nfsd: constify reply_cache_stats_operations structure
From: J. Bruce Fields @ 2016-11-08 20:36 UTC (permalink / raw)
To: Jeff Layton; +Cc: Julia Lawall, kernel-janitors, linux-nfs, linux-kernel
In-Reply-To: <1472468267.2597.0.camel@poochiereds.net>
On Mon, Aug 29, 2016 at 06:57:47AM -0400, Jeff Layton wrote:
> On Sun, 2016-08-28 at 22:36 +0200, Julia Lawall wrote:
> > reply_cache_stats_operations, of type struct file_operations, is
> > never
> > modified, so declare it as const.
> >
> > Done with the help of Coccinelle.
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> >
> > ---
> > fs/nfsd/nfsctl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 65ad016..3bc9693 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -217,7 +217,7 @@ static const struct file_operations
> > pool_stats_operations = {
> > .release = nfsd_pool_stats_release,
> > };
> >
> > -static struct file_operations reply_cache_stats_operations = {
> > +static const struct file_operations reply_cache_stats_operations = {
> > .open = nfsd_reply_cache_stats_open,
> > .read = seq_read,
> > .llseek = seq_lseek,
> >
>
> I'm pretty sure Bruce will pick this up for v4.9:
Except, for some reason it's been languishing in my inbox. Apologies,
applying....
--b.
>
> Reviewed-by: Jeff Layton <jlayton@poochiereds.net>
^ permalink raw reply
* Re: Linux-4.X-rcY patches can't be applied with git?
From: Pavel Machek @ 2016-11-08 20:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jarod Wilson, Josh Boyer, Linux Kernel Mailing List,
Greg Kroah-Hartman
In-Reply-To: <CA+55aFwt3yoDkSY2TBo5uCYa2B_BhWXGVVBEN=9G-6WWXB__1w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]
Hi!
> > But in that case, what if your patch generation script used a filter to
> > exclude those binary files? No harm to that target audience, and it would
> > actually make them behave better for distro builds. Though that might be
> > counter to the goal of making them disappear entirely. :)
>
> Heh, I'd rather people get the warning that "oops, something is
> incomplete". They can still work with the end result, but at least
> they got some indication that hey, that patch didn't work wonderfully
> well...
>
> To be honest, I really would like to not do the tar-balls and patches at all.
>
> But maybe rather than saying "it's only for legacy 'patch' users", I
> could just say that it's getting phased out, and say "you have to use
> 'git apply' to apply them".
>
> Then I could just enable "--binary" and "-M", and see what happens.
>
> I suspect that these days, git is so ubiquitous that it's ok.
>
> And then in a few years, maybe I can just stop doing patches entirely,
> having proved the point that everybody already has git ;)
Well, having git is not quite the same thing as "having big enough
machine to run git on tree as big as kernel". Handheld zaurus was
powerful enough to compile kernel (in few hours) but git was pretty
painful to use there.
I'm now cross-compiling for n900, so problem is gone there, but there
is difference between downloading single release, and downloading full
history when you are on a slow line...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"
From: Jeff Cody @ 2016-11-08 20:44 UTC (permalink / raw)
To: Ashish Mittal
Cc: qemu-devel, pbonzini, kwolf, armbru, berrange, famz,
ashish.mittal, stefanha, Rakesh.Ranjan, Buddhi.Madhav,
Ketan.Nilangekar, Abhijit.Dey, Venkatesha.Mg
In-Reply-To: <1478566785-4002-3-git-send-email-ashish.mittal@veritas.com>
On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
> These changes use a vxhs test server that is a part of the following
> repository:
> https://github.com/MittalAshish/libqnio.git
>
> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> ---
> v6 changelog:
> (1) Added iotests for VxHS block device.
>
> tests/qemu-iotests/common | 6 ++++++
> tests/qemu-iotests/common.config | 13 +++++++++++++
> tests/qemu-iotests/common.filter | 1 +
> tests/qemu-iotests/common.rc | 19 +++++++++++++++++++
> 4 files changed, 39 insertions(+)
>
> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> index d60ea2c..41430d8 100644
> --- a/tests/qemu-iotests/common
> +++ b/tests/qemu-iotests/common
When using raw format, I was able to run the test successfully for all
supported test cases (26 of them).
With qcow2, they fail - but not the fault of this patch, I think; but
rather, the fault of the test server. Can qnio_server be modified so that
it does not work on just raw files?
> @@ -158,6 +158,7 @@ check options
> -nfs test nfs
> -archipelago test archipelago
> -luks test luks
> + -vxhs test vxhs
> -xdiff graphical mode diff
> -nocache use O_DIRECT on backing file
> -misalign misalign memory allocations
> @@ -261,6 +262,11 @@ testlist options
> xpand=false
> ;;
>
> + -vxhs)
> + IMGPROTO=vxhs
> + xpand=false
> + ;;
> +
> -ssh)
> IMGPROTO=ssh
> xpand=false
> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> index f6384fb..c7a80c0 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -105,6 +105,10 @@ if [ -z "$QEMU_NBD_PROG" ]; then
> export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
> fi
>
> +if [ -z "$QEMU_VXHS_PROG" ]; then
> + export QEMU_VXHS_PROG="`set_prog_path qnio_server /usr/local/bin`"
> +fi
> +
> _qemu_wrapper()
> {
> (
> @@ -156,10 +160,19 @@ _qemu_nbd_wrapper()
> )
> }
>
> +_qemu_vxhs_wrapper()
> +{
> + (
> + echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
> + exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
> + )
> +}
> +
> export QEMU=_qemu_wrapper
> export QEMU_IMG=_qemu_img_wrapper
> export QEMU_IO=_qemu_io_wrapper
> export QEMU_NBD=_qemu_nbd_wrapper
> +export QEMU_VXHS=_qemu_vxhs_wrapper
>
> QEMU_IMG_EXTRA_ARGS=
> if [ "$IMGOPTSSYNTAX" = "true" ]; then
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 240ed06..a8a4d0e 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -123,6 +123,7 @@ _filter_img_info()
> -e "s#$TEST_DIR#TEST_DIR#g" \
> -e "s#$IMGFMT#IMGFMT#g" \
> -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
> + -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
> -e "/encrypted: yes/d" \
> -e "/cluster_size: [0-9]\\+/d" \
> -e "/table_size: [0-9]\\+/d" \
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 3213765..06a3164 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -89,6 +89,9 @@ else
> TEST_IMG=$TEST_DIR/t.$IMGFMT
> elif [ "$IMGPROTO" = "archipelago" ]; then
> TEST_IMG="archipelago:at.$IMGFMT"
> + elif [ "$IMGPROTO" = "vxhs" ]; then
> + TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
> + TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
> else
> TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
> fi
> @@ -175,6 +178,12 @@ _make_test_img()
> eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT $TEST_IMG_FILE &"
> sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
> fi
> +
> + # Start QNIO server on image directory for vxhs protocol
> + if [ $IMGPROTO = "vxhs" ]; then
> + eval "$QEMU_VXHS -d $TEST_DIR &"
> + sleep 1 # Wait for server to come up.
> + fi
> }
>
> _rm_test_img()
> @@ -201,6 +210,16 @@ _cleanup_test_img()
> fi
> rm -f "$TEST_IMG_FILE"
> ;;
> + vxhs)
> + if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
> + local QEMU_VXHS_PID
> + read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
> + kill ${QEMU_VXHS_PID} >/dev/null 2>&1
> + rm -f "${TEST_DIR}/qemu-vxhs.pid"
> + fi
> + rm -f "$TEST_IMG_FILE"
> + ;;
> +
> file)
> _rm_test_img "$TEST_DIR/t.$IMGFMT"
> _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator
From: Peter Rosin @ 2016-11-08 20:44 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Rob Herring,
Mark Rutland, Daniel Baluta, Slawomir Stepien, linux-iio,
devicetree
In-Reply-To: <alpine.DEB.2.20.1611081809320.3501@nanos>
On 2016-11-08 19:38, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> On 2016-11-08 16:59, Thomas Gleixner wrote:
>
>>> So you need that whole dance including the delayed work because you cannot
>>> call iio_write_channel_raw() from hard interrupt context, right?
>>
>> It's not the "cannot call from hard irq context" that made me do that, it's..
>
> Well, what guarantees you that the DAC is writeable from IRQ context? It
> might be hanging off an i2c/spi bus as well....
Right, the DAC is actually on an i2c-bus so it's not possible to call
iio_write_channel_raw in hard irq context, I did not disagree with that.
But that was not the reason for not calling it in (hard or threaded) irq
context. The reason is that this was the simplest way to kill the race
with the timeout, and I wasn't in that much of a hurry to start the next
step (in the binary search) and was completely happy with just recording
if any interrupt did happen and then continue when the timeout eventually
happened. It's not like the delayed work connected to the timeout can be
left out anyway, since the design is to not get an interrupt before the
timeout in about half the cases. In some cases there might not be any
interrupts in eons...
>>> The core will mask the interrupt line until the threaded handler is
>>> finished. The threaded handler is invoked with preemption enabled, so you
>>> can sleep there as long as you want. So you can do everything in your
>>> handler and the above dance is just not required.
>>
>> ...that I couldn't work out how to reenable a oneshot irq once it had fired,
>> short of freeing the irq and requesting it again. That seemed entirely
>> bogus, the driver shouldn't risk losing a resource like that so I don't know
>> what I didn't see? Or maybe it was that I had a hard time resolving the race
>> between the irq and the timeout in a nice way. I honestly don't remember
>> why exactly I abandoned oneshot irqs, but this enable/sync/enable dance
>> was much nicer than what I came up with for the oneshot irq solution I
>> originally worked on.
>
> Threaded ONESHOT irqs work this way:
>
> interrupt fires
> mask interrupt
> handler thread is woken
>
> handler thread runs
> invokes isr
> unmask interrupt
>
> So if you rewrite the DAC to the new value in your ISR, then you should not
> get any spurious interrupt.
>
> Note, that this only works for level type interrupts.
>
> We do not mask edge type interrupts as we might lose an edge, but if that
> helps the cause of your problem it's simple enough to make it conditionally
> doing so in the core.
>
>> Or maybe I had problems with the possibly pending irq also when using a
>> oneshot irq, but didn't realize it? That was something I discovered quite
>> late in the process, some time after moving away from oneshot irqs. Are
>> pending irqs cleared when requesting (or reenabling, however that is done)
>> a oneshot irq?
>
> Pending irqs are only replayed, when you reenable an interrupt via
> enable_irq(). That can happen either by software or by hardware.
Ah, of course, the interrupt core does its best to not lose interrupts, but
in this case it is actually desired to lose the interrupts that happen
while the irq is disabled. Which means that the enable/sync/enable-dance is
needed for oneshot interrupts as well. Either that or make tweaks to the
core (yes, irqs are edge-triggered interrupts in my case).
>> Anyway, I do not want the interrupt to be serviced when no one is interested,
>> since I'm afraid that nasty input might generate a flood of interrupts that
>> might disturb other things that the cpu is doing. Which means that I need
>> to enable/disable the interrupt as needed.
>
> So the main issue I'm seing here, is that your comparator does not have
> means to prevent it from firing interrupts.
Right, it's just a discrete op-amp. Only way to "turn it off", is to feed
it input that makes it silent. But I'd rather not have code in this driver
that knows how to do that, since then the driver mutates from something
fairly generic to something that is very specific with hairy dependencies.
It would probably also require code to handle trailing interrupts caused
by setting up the silent state. It just sounds horrible compared to
simply disabling the interrupt (and doing a dance when enabling them).
>> However, what *I* thought Jonathan wanted input on was the part where the
>> interrupt edge/level is flipped when requesting "inverted" signals in
>> envelope_store_invert(). That could perhaps be seen as unorthodox and in
>> need of more eyes?
>
> Flipping the dectection level of the interrupt is fine, but what's the
> guarantee that it is correct in the first place? I don't see anything which
> makes that sure at all. Aside of that this bit does not makes sense:
That "guarantee" comes from devicetree. I.e. the person writing the
dts.
>> + env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_RISING)
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_FALLING;
>
> What's the |= about?
env->comp_irq_trigger_inv is zero from the start, the idea is to
- set FALLING for ..._trigger_inv if RISING is set for ..._trigger
- set RISING for ..._trigger_inv if FALLING is set for ..._trigger
- set LOW for ..._trigger_inv if HIGH is set for ..._trigger
- set HIGH for ..._trigger_inv if LOW is set for ..._trigger
That way, ..._trigger_inv will be the opposite of ..._trigger at all
times, whichever way ..._trigger is set up from devicetree.
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_FALLING)
>
> and this should be 'else if'. If the interrupt is configured for both
> edges, which is possible with some interrupt controllers then the whole
> thing does not work at all.
Right, if you do stupid thing in the devicetree, you get garbage. And
if ..._trigger is triggering on both edges, ..._trigger_inv might just
do the same stupid thing. That's only fair, methinks?
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_RISING;
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_HIGH)
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_LOW;
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_LOW)
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_HIGH;
So, to sum up, in order for this to work with threaded oneshot
interrupts, I still need to either keep the enable/sync/enable-dance
or tweak the irq core to handle my case better. The only gain would
be that I could fire the next step of the search from the threaded
irq handler directly (but it needs some new race-killing code).
Or am I missing something? If not, there's no pressing reason to
switch to threaded oneshot interrupts, right?
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH] mkefidisk.sh: add deprecation warning to the output
From: Randy Witt @ 2016-11-08 20:44 UTC (permalink / raw)
To: Khem Raj, ed.bartosh; +Cc: Hawley, John, openembedded-core
In-Reply-To: <32b3605d-9c79-d1be-c6cc-80309cad7a1a@gmail.com>
>>> We should also document, the wic steps in wiki pages e.g. http://wiki.minnowboard.org/Yocto_Project
>> It's already documented in README.hardware: https://git.yoctoproject.org/cgit/cgit.cgi/poky/tree/README.hardware
>>
>> And in Yocto manual:
>> http://www.yoctoproject.org/docs/2.2/mega-manual/mega-manual.html#building-an-image-for-hardware
>
> Thanks, now can you also nudge the minnowboard.org to do the same ?
>
Pinging you John. :)
>>
>> --
>> Regards,
>> Ed
>>
^ permalink raw reply
* Re: [PATCH v4 8/8] iio: envelope-detector: ADC driver based on a DAC and a comparator
From: Peter Rosin @ 2016-11-08 20:44 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, Daniel Baluta, Slawomir Stepien,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.DEB.2.20.1611081809320.3501@nanos>
On 2016-11-08 19:38, Thomas Gleixner wrote:
> On Tue, 8 Nov 2016, Peter Rosin wrote:
>> On 2016-11-08 16:59, Thomas Gleixner wrote:
>
>>> So you need that whole dance including the delayed work because you cannot
>>> call iio_write_channel_raw() from hard interrupt context, right?
>>
>> It's not the "cannot call from hard irq context" that made me do that, it's..
>
> Well, what guarantees you that the DAC is writeable from IRQ context? It
> might be hanging off an i2c/spi bus as well....
Right, the DAC is actually on an i2c-bus so it's not possible to call
iio_write_channel_raw in hard irq context, I did not disagree with that.
But that was not the reason for not calling it in (hard or threaded) irq
context. The reason is that this was the simplest way to kill the race
with the timeout, and I wasn't in that much of a hurry to start the next
step (in the binary search) and was completely happy with just recording
if any interrupt did happen and then continue when the timeout eventually
happened. It's not like the delayed work connected to the timeout can be
left out anyway, since the design is to not get an interrupt before the
timeout in about half the cases. In some cases there might not be any
interrupts in eons...
>>> The core will mask the interrupt line until the threaded handler is
>>> finished. The threaded handler is invoked with preemption enabled, so you
>>> can sleep there as long as you want. So you can do everything in your
>>> handler and the above dance is just not required.
>>
>> ...that I couldn't work out how to reenable a oneshot irq once it had fired,
>> short of freeing the irq and requesting it again. That seemed entirely
>> bogus, the driver shouldn't risk losing a resource like that so I don't know
>> what I didn't see? Or maybe it was that I had a hard time resolving the race
>> between the irq and the timeout in a nice way. I honestly don't remember
>> why exactly I abandoned oneshot irqs, but this enable/sync/enable dance
>> was much nicer than what I came up with for the oneshot irq solution I
>> originally worked on.
>
> Threaded ONESHOT irqs work this way:
>
> interrupt fires
> mask interrupt
> handler thread is woken
>
> handler thread runs
> invokes isr
> unmask interrupt
>
> So if you rewrite the DAC to the new value in your ISR, then you should not
> get any spurious interrupt.
>
> Note, that this only works for level type interrupts.
>
> We do not mask edge type interrupts as we might lose an edge, but if that
> helps the cause of your problem it's simple enough to make it conditionally
> doing so in the core.
>
>> Or maybe I had problems with the possibly pending irq also when using a
>> oneshot irq, but didn't realize it? That was something I discovered quite
>> late in the process, some time after moving away from oneshot irqs. Are
>> pending irqs cleared when requesting (or reenabling, however that is done)
>> a oneshot irq?
>
> Pending irqs are only replayed, when you reenable an interrupt via
> enable_irq(). That can happen either by software or by hardware.
Ah, of course, the interrupt core does its best to not lose interrupts, but
in this case it is actually desired to lose the interrupts that happen
while the irq is disabled. Which means that the enable/sync/enable-dance is
needed for oneshot interrupts as well. Either that or make tweaks to the
core (yes, irqs are edge-triggered interrupts in my case).
>> Anyway, I do not want the interrupt to be serviced when no one is interested,
>> since I'm afraid that nasty input might generate a flood of interrupts that
>> might disturb other things that the cpu is doing. Which means that I need
>> to enable/disable the interrupt as needed.
>
> So the main issue I'm seing here, is that your comparator does not have
> means to prevent it from firing interrupts.
Right, it's just a discrete op-amp. Only way to "turn it off", is to feed
it input that makes it silent. But I'd rather not have code in this driver
that knows how to do that, since then the driver mutates from something
fairly generic to something that is very specific with hairy dependencies.
It would probably also require code to handle trailing interrupts caused
by setting up the silent state. It just sounds horrible compared to
simply disabling the interrupt (and doing a dance when enabling them).
>> However, what *I* thought Jonathan wanted input on was the part where the
>> interrupt edge/level is flipped when requesting "inverted" signals in
>> envelope_store_invert(). That could perhaps be seen as unorthodox and in
>> need of more eyes?
>
> Flipping the dectection level of the interrupt is fine, but what's the
> guarantee that it is correct in the first place? I don't see anything which
> makes that sure at all. Aside of that this bit does not makes sense:
That "guarantee" comes from devicetree. I.e. the person writing the
dts.
>> + env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_RISING)
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_FALLING;
>
> What's the |= about?
env->comp_irq_trigger_inv is zero from the start, the idea is to
- set FALLING for ..._trigger_inv if RISING is set for ..._trigger
- set RISING for ..._trigger_inv if FALLING is set for ..._trigger
- set LOW for ..._trigger_inv if HIGH is set for ..._trigger
- set HIGH for ..._trigger_inv if LOW is set for ..._trigger
That way, ..._trigger_inv will be the opposite of ..._trigger at all
times, whichever way ..._trigger is set up from devicetree.
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_FALLING)
>
> and this should be 'else if'. If the interrupt is configured for both
> edges, which is possible with some interrupt controllers then the whole
> thing does not work at all.
Right, if you do stupid thing in the devicetree, you get garbage. And
if ..._trigger is triggering on both edges, ..._trigger_inv might just
do the same stupid thing. That's only fair, methinks?
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_RISING;
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_HIGH)
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_LOW;
>> + if (env->comp_irq_trigger & IRQF_TRIGGER_LOW)
>> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_HIGH;
So, to sum up, in order for this to work with threaded oneshot
interrupts, I still need to either keep the enable/sync/enable-dance
or tweak the irq core to handle my case better. The only gain would
be that I could fire the next step of the search from the threaded
irq handler directly (but it needs some new race-killing code).
Or am I missing something? If not, there's no pressing reason to
switch to threaded oneshot interrupts, right?
Cheers,
Peter
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v11 17/22] vfio_platform: Updated to use vfio_set_irqs_validate_and_prepare()
From: Kirti Wankhede @ 2016-11-08 20:41 UTC (permalink / raw)
To: Alexey Kardashevskiy, alex.williamson, pbonzini, kraxel, cjia
Cc: qemu-devel, kvm, kevin.tian, jike.song, bjsdjshi, linux-kernel
In-Reply-To: <e8d532f1-5724-9772-6a86-c48b50b7efdc@ozlabs.ru>
On 11/8/2016 2:22 PM, Alexey Kardashevskiy wrote:
> On 05/11/16 08:10, Kirti Wankhede wrote:
>> Updated vfio_platform_common.c file to use
>> vfio_set_irqs_validate_and_prepare()
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: Id87cd6b78ae901610b39bf957974baa6f40cd7b0
>> ---
>> drivers/vfio/platform/vfio_platform_common.c | 31 +++++++---------------------
>> 1 file changed, 8 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
>> index d78142830754..4c27f4be3c3d 100644
>> --- a/drivers/vfio/platform/vfio_platform_common.c
>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>> @@ -364,36 +364,21 @@ static long vfio_platform_ioctl(void *device_data,
>> struct vfio_irq_set hdr;
>> u8 *data = NULL;
>> int ret = 0;
>> + size_t data_size = 0;
>>
>> minsz = offsetofend(struct vfio_irq_set, count);
>>
>> if (copy_from_user(&hdr, (void __user *)arg, minsz))
>> return -EFAULT;
>>
>> - if (hdr.argsz < minsz)
>> - return -EINVAL;
>> -
>> - if (hdr.index >= vdev->num_irqs)
>> - return -EINVAL;
>> -
>> - if (hdr.flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
>> - VFIO_IRQ_SET_ACTION_TYPE_MASK))
>> - return -EINVAL;
>> -
>> - if (!(hdr.flags & VFIO_IRQ_SET_DATA_NONE)) {
>> - size_t size;
>> -
>> - if (hdr.flags & VFIO_IRQ_SET_DATA_BOOL)
>> - size = sizeof(uint8_t);
>> - else if (hdr.flags & VFIO_IRQ_SET_DATA_EVENTFD)
>> - size = sizeof(int32_t);
>> - else
>> - return -EINVAL;
>> -
>> - if (hdr.argsz - minsz < size)
>> - return -EINVAL;
>> + ret = vfio_set_irqs_validate_and_prepare(&hdr, vdev->num_irqs,
>> + vdev->num_irqs, &data_size);
>
> The patch does not change this but I am still curious:
>
> is not the second vdev->num_irqs supposed to be one of
> VFIO_PCI_INTX_IRQ_INDEX..VFIO_PCI_NUM_IRQS, not the actual number of
> interrupt vectors (as in vfio-pci)?
>
>
Those are PCI specific. I don't think those counts are applicable here.
If you see the prototype, second argument and third argument have
different meaning.
int vfio_set_irqs_validate_and_prepare(struct vfio_irq_set *hdr, int
num_irqs, int max_irq_type, size_t *data_size)
- num_irqs are number of irqs caller want to setup and
- max_irq_type is the one which is return to user in
VFIO_DEVICE_GET_INFO ioctl's info.num_irqs.
For platform these two are same.
Thanks,
Kirti
>
>
>> + if (ret)
>> + return ret;
>>
>> - data = memdup_user((void __user *)(arg + minsz), size);
>> + if (data_size) {
>> + data = memdup_user((void __user *)(arg + minsz),
>> + data_size);
>> if (IS_ERR(data))
>> return PTR_ERR(data);
>> }
>>
>
>
^ permalink raw reply
* nvmet_rdma crash - DISCONNECT event with NULL queue
From: Steve Wise @ 2016-11-08 20:45 UTC (permalink / raw)
In-Reply-To: <b499abb0-afc9-2985-c4c4-3ceba4ca6f33@grimberg.me>
> > Running the same test over mlx4/roce, I hit a warning in list_debug, and
then a
> > stuck CPU...
>
> Steve,
>
> Does this patch help?
> --
Hey Sagi,
With this patch applied, I haven't seen any more target side crashes. I say
this with a caveat, though, that I'm chasing a few other host side issues
still...
Steve.
^ permalink raw reply
* RE: nvmet_rdma crash - DISCONNECT event with NULL queue
From: Steve Wise @ 2016-11-08 20:45 UTC (permalink / raw)
To: 'Sagi Grimberg', 'Christoph Hellwig'
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <b499abb0-afc9-2985-c4c4-3ceba4ca6f33-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> > Running the same test over mlx4/roce, I hit a warning in list_debug, and
then a
> > stuck CPU...
>
> Steve,
>
> Does this patch help?
> --
Hey Sagi,
With this patch applied, I haven't seen any more target side crashes. I say
this with a caveat, though, that I'm chasing a few other host side issues
still...
Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [U-Boot] [PATCH 4/7] tools: sunxi: Add spl image builder
From: Maxime Ripard @ 2016-11-08 20:45 UTC (permalink / raw)
To: u-boot
In-Reply-To: <20161108172947.0d3cd546@bbrezillon>
On Tue, Nov 08, 2016 at 05:29:47PM +0100, Boris Brezillon wrote:
> On Tue, 8 Nov 2016 17:21:14 +0100
> Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>
> > This program generates raw SPL images that can be flashed on the NAND with
> > the ECC and randomizer properly set up.
> >
>
> Maybe you should mention that this file has been copied from the
> sunxi-tools project.
Definitely, I've amended my commit log.
> Otherwise,
>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161108/131f809c/attachment.sig>
^ permalink raw reply
* ✓ Fi.CI.BAT: success for drm/i915/gen9: fix the WM memory bandwidth WA for Y tiling cases
From: Patchwork @ 2016-11-08 20:46 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx
In-Reply-To: <1478636531-6081-1-git-send-email-paulo.r.zanoni@intel.com>
== Series Details ==
Series: drm/i915/gen9: fix the WM memory bandwidth WA for Y tiling cases
URL : https://patchwork.freedesktop.org/series/14990/
State : success
== Summary ==
Series 14990v1 drm/i915/gen9: fix the WM memory bandwidth WA for Y tiling cases
https://patchwork.freedesktop.org/api/1.0/series/14990/revisions/1/mbox/
Test drv_module_reload_basic:
dmesg-warn -> PASS (fi-skl-6770hq)
fi-bdw-5557u total:244 pass:229 dwarn:0 dfail:0 fail:0 skip:15
fi-bxt-t5700 total:244 pass:216 dwarn:0 dfail:0 fail:0 skip:28
fi-byt-j1900 total:244 pass:216 dwarn:0 dfail:0 fail:0 skip:28
fi-byt-n2820 total:244 pass:212 dwarn:0 dfail:0 fail:0 skip:32
fi-hsw-4770 total:244 pass:224 dwarn:0 dfail:0 fail:0 skip:20
fi-hsw-4770r total:244 pass:224 dwarn:0 dfail:0 fail:0 skip:20
fi-ilk-650 total:244 pass:191 dwarn:0 dfail:0 fail:0 skip:53
fi-ivb-3520m total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-ivb-3770 total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-kbl-7200u total:244 pass:222 dwarn:0 dfail:0 fail:0 skip:22
fi-skl-6260u total:244 pass:230 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:244 pass:223 dwarn:0 dfail:0 fail:0 skip:21
fi-skl-6700k total:244 pass:222 dwarn:1 dfail:0 fail:0 skip:21
fi-skl-6770hq total:244 pass:230 dwarn:0 dfail:0 fail:0 skip:14
fi-snb-2520m total:244 pass:212 dwarn:0 dfail:0 fail:0 skip:32
fi-snb-2600 total:244 pass:211 dwarn:0 dfail:0 fail:0 skip:33
0fa5820fd3cbd43e750ef57d8b272a731a04875c drm-intel-nightly: 2016y-11m-08d-19h-30m-38s UTC integration manifest
92970b0 drm/i915/gen9: fix the WM memory bandwidth WA for Y tiling cases
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2937/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v11 13/22] vfio: Introduce common function to add capabilities
From: Kirti Wankhede @ 2016-11-08 20:46 UTC (permalink / raw)
To: Alexey Kardashevskiy, alex.williamson, pbonzini, kraxel, cjia
Cc: qemu-devel, kvm, kevin.tian, jike.song, bjsdjshi, linux-kernel
In-Reply-To: <10cec4f6-0c0a-dcad-cb2f-4eb6a94cdbde@ozlabs.ru>
On 11/8/2016 12:59 PM, Alexey Kardashevskiy wrote:
> On 05/11/16 08:10, Kirti Wankhede wrote:
>> Vendor driver using mediated device framework should use
>> vfio_info_add_capability() to add capabilities.
>> Introduced this function to reduce code duplication in vendor drivers.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: I6fca329fa2291f37a2c859d0bc97574d9e2ce1a6
>> ---
>> drivers/vfio/vfio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/vfio.h | 3 +++
>> 2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 4ed1a6a247c6..9a03be0942a1 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1797,8 +1797,66 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset)
>> for (tmp = caps->buf; tmp->next; tmp = (void *)tmp + tmp->next - offset)
>> tmp->next += offset;
>> }
>> -EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
>> +EXPORT_SYMBOL(vfio_info_cap_shift);
>
>
> Why this change?
>
>
We want this symbol to be available to all drivers.
>>
>> +static int sparse_mmap_cap(struct vfio_info_cap *caps, void *cap_type)
>> +{
>> + struct vfio_info_cap_header *header;
>> + struct vfio_region_info_cap_sparse_mmap *sparse_cap, *sparse = cap_type;
>> + size_t size;
>> +
>> + size = sizeof(*sparse) + sparse->nr_areas * sizeof(*sparse->areas);
>> + header = vfio_info_cap_add(caps, size,
>> + VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1);
>> + if (IS_ERR(header))
>> + return PTR_ERR(header);
>> +
>> + sparse_cap = container_of(header,
>> + struct vfio_region_info_cap_sparse_mmap, header);
>> + sparse_cap->nr_areas = sparse->nr_areas;
>> + memcpy(sparse_cap->areas, sparse->areas,
>> + sparse->nr_areas * sizeof(*sparse->areas));
>> + return 0;
>> +}
>> +
>> +static int region_type_cap(struct vfio_info_cap *caps, void *cap_type)
>> +{
>> + struct vfio_info_cap_header *header;
>> + struct vfio_region_info_cap_type *type_cap, *cap = cap_type;
>> +
>> + header = vfio_info_cap_add(caps, sizeof(*cap),
>> + VFIO_REGION_INFO_CAP_TYPE, 1);
>> + if (IS_ERR(header))
>> + return PTR_ERR(header);
>> +
>> + type_cap = container_of(header, struct vfio_region_info_cap_type,
>> + header);
>> + type_cap->type = cap->type;
>> + type_cap->subtype = cap->subtype;
>> + return 0;
>> +}
>> +
>> +int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
>> + void *cap_type)
>> +{
>> + int ret = -EINVAL;
>> +
>> + if (!cap_type)
>> + return 0;
>> +
>> + switch (cap_type_id) {
>> + case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
>> + ret = sparse_mmap_cap(caps, cap_type);
>> + break;
>> +
>> + case VFIO_REGION_INFO_CAP_TYPE:
>> + ret = region_type_cap(caps, cap_type);
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(vfio_info_add_capability);
>>
>> /*
>> * Pin a set of guest PFNs and return their associated host PFNs for local
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index dcda8fccefab..cf90393a11e2 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -113,6 +113,9 @@ extern struct vfio_info_cap_header *vfio_info_cap_add(
>> struct vfio_info_cap *caps, size_t size, u16 id, u16 version);
>> extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
>>
>> +extern int vfio_info_add_capability(struct vfio_info_cap *caps,
>> + int cap_type_id, void *cap_type);
>> +
>
>
> It would make it easier to review and bisect if 14/22 was squashed into
> this one.
This was split based on Alex's suggestion on earlier version of this
patchset.
> In the resulting patch, vfio_info_cap_add() can be made static as
> it will only be used in drivers/vfio/vfio.c from now.
>
Not sure that vfio_info_cap_add() should be made static. If there are
any other drivers using this symbol outside kernel might break with that
change.
Thanks,
Kirti
>
>
>
>> struct pci_dev;
>> #ifdef CONFIG_EEH
>> extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>
>
>
^ permalink raw reply
* Re: [PATCH v2 22/23] staging: iio: tsl2583: updated copyright and MODULE_AUTHOR
From: Jonathan Cameron @ 2016-11-08 20:35 UTC (permalink / raw)
To: Brian Masney, linux-iio
Cc: devel, gregkh, lars, pmeerw, knaack.h, linux-kernel, Jon.Brenner
In-Reply-To: <1478600208-21153-23-git-send-email-masneyb@onstation.org>
On 08/11/16 10:16, Brian Masney wrote:
> Added Brian Masney's copyright to the header and to the MODULE_AUTHOR
> for all of the staging cleanups that has been done to this driver.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
> drivers/staging/iio/light/tsl2583.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 013e7f6..5cb4ae3 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -3,6 +3,7 @@
> * within the TAOS tsl258x family of devices (tsl2580, tsl2581).
> *
> * Copyright (c) 2011, TAOS Corporation.
> + * Copyright (c) 2016 Brian Masney <masneyb@onstation.org>
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -910,6 +911,6 @@ static struct i2c_driver tsl2583_driver = {
> };
> module_i2c_driver(tsl2583_driver);
>
> -MODULE_AUTHOR("J. August Brenner<jbrenner@taosinc.com>");
> +MODULE_AUTHOR("J. August Brenner <jbrenner@taosinc.com> and Brian Masney <masneyb@onstation.org>");
IIRC preferred method is multiple MODULE_AUTHOR statements.
> MODULE_DESCRIPTION("TAOS tsl2583 ambient light sensor driver");
> MODULE_LICENSE("GPL");
>
^ permalink raw reply
* Re: [Qemu-devel] [PATCH v11 13/22] vfio: Introduce common function to add capabilities
From: Kirti Wankhede @ 2016-11-08 20:46 UTC (permalink / raw)
To: Alexey Kardashevskiy, alex.williamson, pbonzini, kraxel, cjia
Cc: qemu-devel, kvm, kevin.tian, jike.song, bjsdjshi, linux-kernel
In-Reply-To: <10cec4f6-0c0a-dcad-cb2f-4eb6a94cdbde@ozlabs.ru>
On 11/8/2016 12:59 PM, Alexey Kardashevskiy wrote:
> On 05/11/16 08:10, Kirti Wankhede wrote:
>> Vendor driver using mediated device framework should use
>> vfio_info_add_capability() to add capabilities.
>> Introduced this function to reduce code duplication in vendor drivers.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: I6fca329fa2291f37a2c859d0bc97574d9e2ce1a6
>> ---
>> drivers/vfio/vfio.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/vfio.h | 3 +++
>> 2 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
>> index 4ed1a6a247c6..9a03be0942a1 100644
>> --- a/drivers/vfio/vfio.c
>> +++ b/drivers/vfio/vfio.c
>> @@ -1797,8 +1797,66 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset)
>> for (tmp = caps->buf; tmp->next; tmp = (void *)tmp + tmp->next - offset)
>> tmp->next += offset;
>> }
>> -EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
>> +EXPORT_SYMBOL(vfio_info_cap_shift);
>
>
> Why this change?
>
>
We want this symbol to be available to all drivers.
>>
>> +static int sparse_mmap_cap(struct vfio_info_cap *caps, void *cap_type)
>> +{
>> + struct vfio_info_cap_header *header;
>> + struct vfio_region_info_cap_sparse_mmap *sparse_cap, *sparse = cap_type;
>> + size_t size;
>> +
>> + size = sizeof(*sparse) + sparse->nr_areas * sizeof(*sparse->areas);
>> + header = vfio_info_cap_add(caps, size,
>> + VFIO_REGION_INFO_CAP_SPARSE_MMAP, 1);
>> + if (IS_ERR(header))
>> + return PTR_ERR(header);
>> +
>> + sparse_cap = container_of(header,
>> + struct vfio_region_info_cap_sparse_mmap, header);
>> + sparse_cap->nr_areas = sparse->nr_areas;
>> + memcpy(sparse_cap->areas, sparse->areas,
>> + sparse->nr_areas * sizeof(*sparse->areas));
>> + return 0;
>> +}
>> +
>> +static int region_type_cap(struct vfio_info_cap *caps, void *cap_type)
>> +{
>> + struct vfio_info_cap_header *header;
>> + struct vfio_region_info_cap_type *type_cap, *cap = cap_type;
>> +
>> + header = vfio_info_cap_add(caps, sizeof(*cap),
>> + VFIO_REGION_INFO_CAP_TYPE, 1);
>> + if (IS_ERR(header))
>> + return PTR_ERR(header);
>> +
>> + type_cap = container_of(header, struct vfio_region_info_cap_type,
>> + header);
>> + type_cap->type = cap->type;
>> + type_cap->subtype = cap->subtype;
>> + return 0;
>> +}
>> +
>> +int vfio_info_add_capability(struct vfio_info_cap *caps, int cap_type_id,
>> + void *cap_type)
>> +{
>> + int ret = -EINVAL;
>> +
>> + if (!cap_type)
>> + return 0;
>> +
>> + switch (cap_type_id) {
>> + case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
>> + ret = sparse_mmap_cap(caps, cap_type);
>> + break;
>> +
>> + case VFIO_REGION_INFO_CAP_TYPE:
>> + ret = region_type_cap(caps, cap_type);
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(vfio_info_add_capability);
>>
>> /*
>> * Pin a set of guest PFNs and return their associated host PFNs for local
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index dcda8fccefab..cf90393a11e2 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -113,6 +113,9 @@ extern struct vfio_info_cap_header *vfio_info_cap_add(
>> struct vfio_info_cap *caps, size_t size, u16 id, u16 version);
>> extern void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset);
>>
>> +extern int vfio_info_add_capability(struct vfio_info_cap *caps,
>> + int cap_type_id, void *cap_type);
>> +
>
>
> It would make it easier to review and bisect if 14/22 was squashed into
> this one.
This was split based on Alex's suggestion on earlier version of this
patchset.
> In the resulting patch, vfio_info_cap_add() can be made static as
> it will only be used in drivers/vfio/vfio.c from now.
>
Not sure that vfio_info_cap_add() should be made static. If there are
any other drivers using this symbol outside kernel might break with that
change.
Thanks,
Kirti
>
>
>
>> struct pci_dev;
>> #ifdef CONFIG_EEH
>> extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
>>
>
>
^ permalink raw reply
* Re: [PATCH lttng-tools v2] Fix: test cases now rely on explicit workloads
From: Francis Deslauriers @ 2016-11-08 20:46 UTC (permalink / raw)
To: Nathan Lynch; +Cc: lttng-dev@lists.lttng.org, jgalar
In-Reply-To: <e9f53bab-dbba-6ceb-3d7b-26f30a33433a@mentor.com>
[-- Attachment #1.1: Type: text/plain, Size: 1573 bytes --]
2016-11-08 15:06 GMT-05:00 Nathan Lynch <Nathan_Lynch@mentor.com>:
> On 11/08/2016 01:49 PM, Francis Deslauriers wrote:
> > Run a process explicitly in the tracing session to generate the enabled
> events
> > rather than relying on the events generated by the lttng CLI.
>
> [...]
>
> > diff --git a/tests/regression/kernel/test_all_events
> b/tests/regression/kernel/test_all_events
> > index 50002f4..80a8020 100755
> > --- a/tests/regression/kernel/test_all_events
> > +++ b/tests/regression/kernel/test_all_events
> > @@ -21,6 +21,8 @@ CURDIR=$(dirname $0)/
> > TESTDIR=$CURDIR/../..
> > NUM_TESTS=8
> >
> > +TESTCMD="grep -V -q > /dev/null"
> > +
>
> What was wrong with /bin/true, as in v1 of this patch?
I used grep because the configure script looks for it already
At the moment, we don't check if /bin/true is installed on the machine
using the configure script.
> FYI, Busybox grep doesn't know -V:
>
> # grep -V -q >/dev/null
> grep: invalid option -- 'V'
> BusyBox v1.24.1 (2016-11-08 12:59:13 CST) multi-call binary.
>
> Usage: grep [-HhnlLoqvsriwFE] [-m N] [-A/B/C N] PATTERN/-e PATTERN.../-f
> FILE [FILE]...
>
> # echo $?
> 1
>
> It looks like the testcases don't care about the exit status of TESTCMD,
> but you may want to redirect stderr to /dev/null as well.
>
Good point.
I could do: grep "foo" /dev/null
Thank you,
--
Francis Deslauriers
Software developer
EfficiOS inc.
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
[-- Attachment #1.2: Type: text/html, Size: 2892 bytes --]
[-- Attachment #2: Type: text/plain, Size: 156 bytes --]
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply
* Re: Richacl and stored but ignored permissions
From: Andreas Gruenbacher @ 2016-11-08 20:47 UTC (permalink / raw)
To: Steve French
Cc: linux-fsdevel, samba-technical, Anne Marie Merritt,
Weston Andros Adamson
In-Reply-To: <CAH2r5mvtWv6D4zmcFWL5Yowiy+wQ5mGgd1mRWNCT4bBGnrQLQA@mail.gmail.com>
Hi Steve,
On Tue, Nov 8, 2016 at 7:25 PM, Steve French <smfrench@gmail.com> wrote:
> I noticed that setrichacl (on ext4/xfs with richacl patches from your
> tree) allows setting some of the five "stored but ignored" permissions
>
> S synchronize
> W write named attributes
> R read named attributes
> e write retention
> E write retention hold
>
> but it brings up some questions:
> 1) why is 'S' the only one of those five that although allowed to be
> set, will not be displayed by getrichacl? Presumably if it can be
> set, you might as well display it on getrichacl and that might have
> been the original intent since there is a space for it when you do
> "getrichacl --full" but that implies (probably correctly) that
> 'Sychronize' permission is always granted.
the synchronize (S), read_attributes (a), and read_acl (c) permissions
are "always allowed". The read_named_attrs (R), write_named_attrs (W),
write_retention (e), and write_retention_hold (E) permissions have no
meaningful mapping locally. All of these permissions are stored but
ignored.
With setrichacl, when setting simple ACLs that can be fully
represented in the file permission bits, no actual ACL is stored. For
example, the ACL owner@:rwp::allow is stored as the rw------- file
permission bits. When checking if an ACL can be fully represented as
file permission bits, the "always allowed" permissions are ignored.
The ACL owner@:rwpS::allow (note the synchronize (S) permission) is
also stored as the rw------- file permission bits, and the synchronize
permission isn't explicitly preserved. This is likely why getrichacl
didn't seem to show the synchronize permission in your tests.
> 2) should we allow 'e' and 'E' to be set (I lean toward yes, but NFS
> rejected it when I tried, although xfs/ext4 accepted it).
Did you try with the appropriate NFS protocol version? These
permissions were added relatively late.
> 3) Shouldn't we actually do something with 'W' (and maybe 'R'
> permission but presumably that can be just implied to be on since some
> attributes always need to be readable) and actually enforce use of W
> permission to allow/forbid the setting of xattrs on the file?
The meaning of the R and W permissions is so vaguely defined that I
don't think it makes sense to map them to xattrs. Windows seems to use
them for something different than NFSv4 does according to
documentation, which may not even match what implementation do.
> 4) Shouldn't we display as enabled permissions those that are implicit
> rather than leaving them out (as if they are forbidden)? e.g. the
> 'owner' permission ('o') presumably can be displayed for root (as it
> is by default granted), also note the 'a' and 'S' permissions when
> you do "getrichacl --full" are displayed as unset even though they are
> implicitly granted. You can fix that by setting 'a' explicitly but it
> seems wrong to implicitly grant a permission, but not display it as
> granted in getrichacl
I've tried that, and things get confusing pretty fast:
* The resulting ACLs were rather confusing.
* What do you do with an ACL that doesn't have an entry for owner@ or the
actual owner? Do you add an owner@ entry just to show the implicitly
granted write_attributes (A) and write_acl (C) permissions?
* Since you're mentioning the root user, should root entries be added as well?
* Permissions might have to change when the file ownership changes, for entries
representing the actual file owner.
* Would implicitly granted permissions be added to DENY ACEs as well?
If not, will Windows still show DENY ACEs with well-known sets of
permissions such as Full Access correctly?
Thanks,
Andreas
^ permalink raw reply
* Re: [PATCH RFC 0/4] xfs: basic cow fork speculative preallocation
From: Darrick J. Wong @ 2016-11-08 20:48 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
In-Reply-To: <1478636856-7590-1-git-send-email-bfoster@redhat.com>
On Tue, Nov 08, 2016 at 03:27:32PM -0500, Brian Foster wrote:
> Hi all,
>
> This is an experiment based on an idea for COW fork speculative
> preallocation. This is experimental, lightly/barely tested and sent in
> RFC form to solicit thoughts, ideas or flames before I spend time taking
> it further.
>
> Patch 1 probably stands on its own. Patches 2 and 3 are some refactoring
> and patch 4 implements the basic idea, which is described in the commit
> log description. The testing I've done so far is basically similar to
> how one would test the effects of traditional speculative preallocation:
> write to multiple reflinked files in parallel and examine the resulting
> fragmentation. Specifically, I wrote sequentially to 16 different
> reflinked files of the same 8GB original (which has two data extents,
> completely shared). Without preallocation, the test results in ~248
> extents across the 16 files. With preallocation, the test results in 32
> extents across the 16 files (i.e., 2 extents per file, same as the
> source file).
>
> An obvious tradeoff is the unnecessarily aggressive allocation that
> might occur in the event of random writes to a large file (such as in
> the cloned VM disk image use case), but my thinking is that the
> cowblocks tagging and reclaim infrastructure should manage that
> sufficiently (lack of testing notwithstanding). In any event, I'm
> interested in any thoughts along the lines of whether this is useful at
> all, alternative algorithm ideas, etc.
Was about to step out to lunch when this came in, but...
Is there an xfstest for this, so I can play too? :)
As far as random writes go, some of the reflink tests look at fragmentation
behavior. generic/301 generic/302 xfs/180 xfs/182 xfs/184 xfs/192 xfs/193
xfs/198 xfs/200 xfs/204 xfs/208 xfs/208 xfs/211 xfs/215 xfs/218 xfs/219 xfs/221
xfs/223 xfs/224 xfs/225 xfs/226 xfs/228 xfs/230 xfs/231 xfs/232 xfs/344 xfs/345
xfs/346 xfs/347 are the ones that grep 'new extents:' picked up.
Will look at the patches when I get back.
--D
>
> Brian
>
> Brian Foster (4):
> xfs: clean up cow fork reservation and tag inodes correctly
> xfs: logically separate iomap range from allocation range
> xfs: reuse xfs_file_iomap_begin_delay() for cow fork delalloc
> xfs: implement basic COW fork speculative preallocation
>
> fs/xfs/xfs_iomap.c | 132 +++++++++++++++++++++++++++++++++++++++++----------
> fs/xfs/xfs_reflink.c | 28 ++---------
> 2 files changed, 111 insertions(+), 49 deletions(-)
>
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Linux-ima-devel] [PATCH v6 02/10] ima: on soft reboot, restore the measurement list
From: Mimi Zohar @ 2016-11-08 20:47 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: Thiago Jung Bauermann, linux-security-module, linuxppc-dev, kexec,
linux-kernel@vger.kernel.org, Eric W. Biederman, linux-ima-devel,
Andrew Morton
In-Reply-To: <CACE9dm87W1H0wJeBUce9_XxKz_W096r=T1KhQdFBYYhHnGcqGg@mail.gmail.com>
On Tue, 2016-11-08 at 21:46 +0200, Dmitry Kasatkin wrote:
> On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann
> <bauerman@linux.vnet.ibm.com> wrote:
> > From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >
> > The TPM PCRs are only reset on a hard reboot. In order to validate a
> > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > of the running kernel must be saved and restored on boot. This patch
> > restores the measurement list.
> >
> > Changelog v5:
> > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago)
> > - replace kexec_get_handover_buffer() with ima_get_kexec_buffer() (Thiago)
> > - replace kexec_free_handover_buffer() with ima_free_kexec_buffer() (Thiago)
> > - remove unnecessary includes from ima_kexec.c (Thiago)
> > - fix off-by-one error when checking hdr_v1->template_name_len (Colin King)
> >
> > Changelog v2:
> > - redefined ima_kexec_hdr to use types with well defined sizes (M. Ellerman)
> > - defined missing ima_load_kexec_buffer() stub function
> >
> > Changelog v1:
> > - call ima_load_kexec_buffer() (Thiago)
> >
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > ---
> > security/integrity/ima/Makefile | 1 +
> > security/integrity/ima/ima.h | 21 +++++
> > security/integrity/ima/ima_init.c | 2 +
> > security/integrity/ima/ima_kexec.c | 44 +++++++++
> > security/integrity/ima/ima_queue.c | 10 ++
> > security/integrity/ima/ima_template.c | 170 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 248 insertions(+)
> >
> > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> > index 9aeaedad1e2b..29f198bde02b 100644
> > --- a/security/integrity/ima/Makefile
> > +++ b/security/integrity/ima/Makefile
> > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> > ima_policy.o ima_template.o ima_template_lib.o
> > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > +ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
> > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index db25f54a04fe..51dc8d57d64d 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -28,6 +28,10 @@
> >
> > #include "../integrity.h"
> >
> > +#ifdef CONFIG_HAVE_IMA_KEXEC
> > +#include <asm/ima.h>
> > +#endif
> > +
> > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
> > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
> > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> > @@ -102,6 +106,21 @@ struct ima_queue_entry {
> > };
> > extern struct list_head ima_measurements; /* list of all measurements */
> >
> > +/* Some details preceding the binary serialized measurement list */
> > +struct ima_kexec_hdr {
> > + u16 version;
> > + u16 _reserved0;
> > + u32 _reserved1;
> > + u64 buffer_size;
> > + u64 count;
> > +};
> > +
> > +#ifdef CONFIG_HAVE_IMA_KEXEC
> > +void ima_load_kexec_buffer(void);
> > +#else
> > +static inline void ima_load_kexec_buffer(void) {}
> > +#endif /* CONFIG_HAVE_IMA_KEXEC */
> > +
> > /* Internal IMA function definitions */
> > int ima_init(void);
> > int ima_fs_init(void);
> > @@ -122,6 +141,8 @@ int ima_init_crypto(void);
> > void ima_putc(struct seq_file *m, void *data, int datalen);
> > void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
> > struct ima_template_desc *ima_template_desc_current(void);
> > +int ima_restore_measurement_entry(struct ima_template_entry *entry);
> > +int ima_restore_measurement_list(loff_t bufsize, void *buf);
> > int ima_init_template(void);
> >
> > /*
> > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> > index 32912bd54ead..3ba0ca49cba6 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -128,6 +128,8 @@ int __init ima_init(void)
> > if (rc != 0)
> > return rc;
> >
> > + ima_load_kexec_buffer();
> > +
> > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */
> > if (rc != 0)
> > return rc;
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > new file mode 100644
> > index 000000000000..36afd0fe9747
> > --- /dev/null
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Copyright (C) 2016 IBM Corporation
> > + *
> > + * Authors:
> > + * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> > + * Mimi Zohar <zohar@linux.vnet.ibm.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +#include "ima.h"
> > +
> > +/*
> > + * Restore the measurement list from the previous kernel.
> > + */
> > +void ima_load_kexec_buffer(void)
> > +{
> > + void *kexec_buffer = NULL;
> > + size_t kexec_buffer_size = 0;
> > + int rc;
> > +
> > + rc = ima_get_kexec_buffer(&kexec_buffer, &kexec_buffer_size);
> > + switch (rc) {
> > + case 0:
> > + rc = ima_restore_measurement_list(kexec_buffer_size,
> > + kexec_buffer);
> > + if (rc != 0)
> > + pr_err("Failed to restore the measurement list: %d\n",
> > + rc);
> > +
> > + ima_free_kexec_buffer();
> > + break;
> > + case -ENOTSUPP:
> > + pr_debug("Restoring the measurement list not supported\n");
> > + break;
> > + case -ENOENT:
> > + pr_debug("No measurement list to restore\n");
> > + break;
> > + default:
> > + pr_debug("Error restoring the measurement list: %d\n", rc);
> > + }
> > +}
> > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> > index 32f6ac0f96df..4b1bb7787839 100644
> > --- a/security/integrity/ima/ima_queue.c
> > +++ b/security/integrity/ima/ima_queue.c
> > @@ -149,3 +149,13 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
> > op, audit_cause, result, audit_info);
> > return result;
> > }
> > +
> > +int ima_restore_measurement_entry(struct ima_template_entry *entry)
> > +{
> > + int result = 0;
> > +
> > + mutex_lock(&ima_extend_list_mutex);
> > + result = ima_add_digest_entry(entry);
> > + mutex_unlock(&ima_extend_list_mutex);
> > + return result;
> > +}
> > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> > index febd12ed9b55..37f972cb05fe 100644
> > --- a/security/integrity/ima/ima_template.c
> > +++ b/security/integrity/ima/ima_template.c
> > @@ -37,6 +37,7 @@ static struct ima_template_field supported_fields[] = {
> > {.field_id = "sig", .field_init = ima_eventsig_init,
> > .field_show = ima_show_template_sig},
> > };
> > +#define MAX_TEMPLATE_NAME_LEN 15
> >
> > static struct ima_template_desc *ima_template;
> > static struct ima_template_desc *lookup_template_desc(const char *name);
> > @@ -205,3 +206,172 @@ int __init ima_init_template(void)
> >
> > return result;
> > }
> > +
> > +static int ima_restore_template_data(struct ima_template_desc *template_desc,
> > + void *template_data,
> > + int template_data_size,
> > + struct ima_template_entry **entry)
> > +{
> > + struct binary_field_data {
> > + u32 len;
> > + u8 data[0];
> > + } __packed;
> > +
> > + struct binary_field_data *field_data;
> > + int offset = 0;
> > + int ret = 0;
> > + int i;
> > +
> > + *entry = kzalloc(sizeof(**entry) +
> > + template_desc->num_fields * sizeof(struct ima_field_data),
> > + GFP_NOFS);
> > + if (!*entry)
> > + return -ENOMEM;
> > +
> > + (*entry)->template_desc = template_desc;
> > + for (i = 0; i < template_desc->num_fields; i++) {
> > + field_data = template_data + offset;
> > +
> > + /* Each field of the template data is prefixed with a length. */
> > + if (offset > (template_data_size - sizeof(field_data->len))) {
> > + pr_err("Restoring the template field failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + offset += sizeof(field_data->len);
> > +
> > + if (offset > (template_data_size - field_data->len)) {
> > + pr_err("Restoring the template field data failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + offset += field_data->len;
> > +
> > + (*entry)->template_data[i].len = field_data->len;
> > + (*entry)->template_data_len += sizeof(field_data->len);
> > +
> > + (*entry)->template_data[i].data =
> > + kzalloc(field_data->len + 1, GFP_KERNEL);
> > + if (!(*entry)->template_data[i].data) {
> > + ret = -ENOMEM;
> > + break;
> > + }
> > + memcpy((*entry)->template_data[i].data, field_data->data,
> > + field_data->len);
> > + (*entry)->template_data_len += field_data->len;
> > + }
> > +
> > + if (ret < 0) {
> > + ima_free_template_entry(*entry);
> > + *entry = NULL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/* Restore the serialized binary measurement list without extending PCRs. */
> > +int ima_restore_measurement_list(loff_t size, void *buf)
> > +{
> > + struct binary_hdr_v1 {
> > + u32 pcr;
> > + u8 digest[TPM_DIGEST_SIZE];
> > + u32 template_name_len;
> > + char template_name[0];
> > + } __packed;
> > + char template_name[MAX_TEMPLATE_NAME_LEN];
> > +
> > + struct binary_data_v1 {
> > + u32 template_data_size;
> > + char template_data[0];
> > + } __packed;
> > +
> > + struct ima_kexec_hdr *khdr = buf;
> > + struct binary_hdr_v1 *hdr_v1;
> > + struct binary_data_v1 *data_v1;
> > +
> > + void *bufp = buf + sizeof(*khdr);
> > + void *bufendp = buf + khdr->buffer_size;
> > + struct ima_template_entry *entry;
> > + struct ima_template_desc *template_desc;
> > + unsigned long count = 0;
> > + int ret = 0;
> > +
> > + if (!buf || size < sizeof(*khdr))
> > + return 0;
> > +
> > + if (khdr->version != 1) {
> > + pr_err("attempting to restore a incompatible measurement list");
> > + return 0;
> > + }
> > +
> > + /*
> > + * ima kexec buffer prefix: version, buffer size, count
> > + * v1 format: pcr, digest, template-name-len, template-name,
> > + * template-data-size, template-data
> > + */
> > + while ((bufp < bufendp) && (count++ < khdr->count)) {
> > + if (count > ULONG_MAX - 1) {
> > + pr_err("attempting to restore too many measurements");
> > + ret = -EINVAL;
> > + }
> > +
> > + hdr_v1 = bufp;
> > + if ((hdr_v1->template_name_len >= MAX_TEMPLATE_NAME_LEN) ||
> > + ((bufp + hdr_v1->template_name_len) > bufendp)) {
>
> based on following code template_name_len does not include header
> (sizeof(*hdr_v1))?
> If so the check is wrong???
Yes, good catch. In addition, before assigning hdr_v1 there should be a
size check as well.
>
> > + pr_err("attempting to restore a template name \
> > + that is too long\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + bufp += sizeof(*hdr_v1);
> > +
> > + /* template name is not null terminated */
> > + memcpy(template_name, bufp, hdr_v1->template_name_len);
> > + template_name[hdr_v1->template_name_len] = 0;
> > +
> > + if (strcmp(template_name, "ima") == 0) {
> > + pr_err("attempting to restore an unsupported \
> > + template \"%s\" failed\n", template_name);
> > + ret = -EINVAL;
> > + break;
> > + }
> > + data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
> > +
> > + /* get template format */
> > + template_desc = lookup_template_desc(template_name);
> > + if (!template_desc) {
> > + pr_err("template \"%s\" not found\n", template_name);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if (bufp > (bufendp - sizeof(data_v1->template_data_size))) {
> > + pr_err("restoring the template data size failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + bufp += (u_int8_t) sizeof(data_v1->template_data_size);
> > +
> > + if (bufp > (bufendp - data_v1->template_data_size)) {
> > + pr_err("restoring the template data failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
>
> It looks like a similar problem... sizeof(struct binary_data_v1) is
> missing in the check...
Following the template name, is the template data length and the
template data.
> > + ret = ima_restore_template_data(template_desc,
> > + data_v1->template_data,
> > + data_v1->template_data_size,
> > + &entry);
> > + if (ret < 0)
> > + break;
> > +
> > + memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE);
> > + entry->pcr = hdr_v1->pcr;
> > + ret = ima_restore_measurement_entry(entry);
> > + if (ret < 0)
> > + break;
> > +
> > + bufp += data_v1->template_data_size;
> > + }
> > + return ret;
> > +}
> > --
> > 2.7.4
> >
>
> In overall it is a bit hard to read this function somehow..
Ok, I'll see if there is any way of simplifying/cleaning up walking the
measurement list some more.
Mimi
^ permalink raw reply
* Re: [Linux-ima-devel] [PATCH v6 02/10] ima: on soft reboot, restore the measurement list
From: Mimi Zohar @ 2016-11-08 20:47 UTC (permalink / raw)
To: Dmitry Kasatkin
Cc: kexec, linux-kernel@vger.kernel.org, linux-ima-devel,
linux-security-module, Eric W. Biederman, Thiago Jung Bauermann,
Andrew Morton, linuxppc-dev
In-Reply-To: <CACE9dm87W1H0wJeBUce9_XxKz_W096r=T1KhQdFBYYhHnGcqGg@mail.gmail.com>
On Tue, 2016-11-08 at 21:46 +0200, Dmitry Kasatkin wrote:
> On Fri, Oct 21, 2016 at 5:44 AM, Thiago Jung Bauermann
> <bauerman@linux.vnet.ibm.com> wrote:
> > From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >
> > The TPM PCRs are only reset on a hard reboot. In order to validate a
> > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > of the running kernel must be saved and restored on boot. This patch
> > restores the measurement list.
> >
> > Changelog v5:
> > - replace CONFIG_KEXEC_FILE with architecture CONFIG_HAVE_IMA_KEXEC (Thiago)
> > - replace kexec_get_handover_buffer() with ima_get_kexec_buffer() (Thiago)
> > - replace kexec_free_handover_buffer() with ima_free_kexec_buffer() (Thiago)
> > - remove unnecessary includes from ima_kexec.c (Thiago)
> > - fix off-by-one error when checking hdr_v1->template_name_len (Colin King)
> >
> > Changelog v2:
> > - redefined ima_kexec_hdr to use types with well defined sizes (M. Ellerman)
> > - defined missing ima_load_kexec_buffer() stub function
> >
> > Changelog v1:
> > - call ima_load_kexec_buffer() (Thiago)
> >
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > ---
> > security/integrity/ima/Makefile | 1 +
> > security/integrity/ima/ima.h | 21 +++++
> > security/integrity/ima/ima_init.c | 2 +
> > security/integrity/ima/ima_kexec.c | 44 +++++++++
> > security/integrity/ima/ima_queue.c | 10 ++
> > security/integrity/ima/ima_template.c | 170 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 248 insertions(+)
> >
> > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> > index 9aeaedad1e2b..29f198bde02b 100644
> > --- a/security/integrity/ima/Makefile
> > +++ b/security/integrity/ima/Makefile
> > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> > ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> > ima_policy.o ima_template.o ima_template_lib.o
> > ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > +ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
> > obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index db25f54a04fe..51dc8d57d64d 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -28,6 +28,10 @@
> >
> > #include "../integrity.h"
> >
> > +#ifdef CONFIG_HAVE_IMA_KEXEC
> > +#include <asm/ima.h>
> > +#endif
> > +
> > enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
> > IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
> > enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> > @@ -102,6 +106,21 @@ struct ima_queue_entry {
> > };
> > extern struct list_head ima_measurements; /* list of all measurements */
> >
> > +/* Some details preceding the binary serialized measurement list */
> > +struct ima_kexec_hdr {
> > + u16 version;
> > + u16 _reserved0;
> > + u32 _reserved1;
> > + u64 buffer_size;
> > + u64 count;
> > +};
> > +
> > +#ifdef CONFIG_HAVE_IMA_KEXEC
> > +void ima_load_kexec_buffer(void);
> > +#else
> > +static inline void ima_load_kexec_buffer(void) {}
> > +#endif /* CONFIG_HAVE_IMA_KEXEC */
> > +
> > /* Internal IMA function definitions */
> > int ima_init(void);
> > int ima_fs_init(void);
> > @@ -122,6 +141,8 @@ int ima_init_crypto(void);
> > void ima_putc(struct seq_file *m, void *data, int datalen);
> > void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
> > struct ima_template_desc *ima_template_desc_current(void);
> > +int ima_restore_measurement_entry(struct ima_template_entry *entry);
> > +int ima_restore_measurement_list(loff_t bufsize, void *buf);
> > int ima_init_template(void);
> >
> > /*
> > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> > index 32912bd54ead..3ba0ca49cba6 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -128,6 +128,8 @@ int __init ima_init(void)
> > if (rc != 0)
> > return rc;
> >
> > + ima_load_kexec_buffer();
> > +
> > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */
> > if (rc != 0)
> > return rc;
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > new file mode 100644
> > index 000000000000..36afd0fe9747
> > --- /dev/null
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -0,0 +1,44 @@
> > +/*
> > + * Copyright (C) 2016 IBM Corporation
> > + *
> > + * Authors:
> > + * Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> > + * Mimi Zohar <zohar@linux.vnet.ibm.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +#include "ima.h"
> > +
> > +/*
> > + * Restore the measurement list from the previous kernel.
> > + */
> > +void ima_load_kexec_buffer(void)
> > +{
> > + void *kexec_buffer = NULL;
> > + size_t kexec_buffer_size = 0;
> > + int rc;
> > +
> > + rc = ima_get_kexec_buffer(&kexec_buffer, &kexec_buffer_size);
> > + switch (rc) {
> > + case 0:
> > + rc = ima_restore_measurement_list(kexec_buffer_size,
> > + kexec_buffer);
> > + if (rc != 0)
> > + pr_err("Failed to restore the measurement list: %d\n",
> > + rc);
> > +
> > + ima_free_kexec_buffer();
> > + break;
> > + case -ENOTSUPP:
> > + pr_debug("Restoring the measurement list not supported\n");
> > + break;
> > + case -ENOENT:
> > + pr_debug("No measurement list to restore\n");
> > + break;
> > + default:
> > + pr_debug("Error restoring the measurement list: %d\n", rc);
> > + }
> > +}
> > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> > index 32f6ac0f96df..4b1bb7787839 100644
> > --- a/security/integrity/ima/ima_queue.c
> > +++ b/security/integrity/ima/ima_queue.c
> > @@ -149,3 +149,13 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
> > op, audit_cause, result, audit_info);
> > return result;
> > }
> > +
> > +int ima_restore_measurement_entry(struct ima_template_entry *entry)
> > +{
> > + int result = 0;
> > +
> > + mutex_lock(&ima_extend_list_mutex);
> > + result = ima_add_digest_entry(entry);
> > + mutex_unlock(&ima_extend_list_mutex);
> > + return result;
> > +}
> > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> > index febd12ed9b55..37f972cb05fe 100644
> > --- a/security/integrity/ima/ima_template.c
> > +++ b/security/integrity/ima/ima_template.c
> > @@ -37,6 +37,7 @@ static struct ima_template_field supported_fields[] = {
> > {.field_id = "sig", .field_init = ima_eventsig_init,
> > .field_show = ima_show_template_sig},
> > };
> > +#define MAX_TEMPLATE_NAME_LEN 15
> >
> > static struct ima_template_desc *ima_template;
> > static struct ima_template_desc *lookup_template_desc(const char *name);
> > @@ -205,3 +206,172 @@ int __init ima_init_template(void)
> >
> > return result;
> > }
> > +
> > +static int ima_restore_template_data(struct ima_template_desc *template_desc,
> > + void *template_data,
> > + int template_data_size,
> > + struct ima_template_entry **entry)
> > +{
> > + struct binary_field_data {
> > + u32 len;
> > + u8 data[0];
> > + } __packed;
> > +
> > + struct binary_field_data *field_data;
> > + int offset = 0;
> > + int ret = 0;
> > + int i;
> > +
> > + *entry = kzalloc(sizeof(**entry) +
> > + template_desc->num_fields * sizeof(struct ima_field_data),
> > + GFP_NOFS);
> > + if (!*entry)
> > + return -ENOMEM;
> > +
> > + (*entry)->template_desc = template_desc;
> > + for (i = 0; i < template_desc->num_fields; i++) {
> > + field_data = template_data + offset;
> > +
> > + /* Each field of the template data is prefixed with a length. */
> > + if (offset > (template_data_size - sizeof(field_data->len))) {
> > + pr_err("Restoring the template field failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + offset += sizeof(field_data->len);
> > +
> > + if (offset > (template_data_size - field_data->len)) {
> > + pr_err("Restoring the template field data failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + offset += field_data->len;
> > +
> > + (*entry)->template_data[i].len = field_data->len;
> > + (*entry)->template_data_len += sizeof(field_data->len);
> > +
> > + (*entry)->template_data[i].data =
> > + kzalloc(field_data->len + 1, GFP_KERNEL);
> > + if (!(*entry)->template_data[i].data) {
> > + ret = -ENOMEM;
> > + break;
> > + }
> > + memcpy((*entry)->template_data[i].data, field_data->data,
> > + field_data->len);
> > + (*entry)->template_data_len += field_data->len;
> > + }
> > +
> > + if (ret < 0) {
> > + ima_free_template_entry(*entry);
> > + *entry = NULL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/* Restore the serialized binary measurement list without extending PCRs. */
> > +int ima_restore_measurement_list(loff_t size, void *buf)
> > +{
> > + struct binary_hdr_v1 {
> > + u32 pcr;
> > + u8 digest[TPM_DIGEST_SIZE];
> > + u32 template_name_len;
> > + char template_name[0];
> > + } __packed;
> > + char template_name[MAX_TEMPLATE_NAME_LEN];
> > +
> > + struct binary_data_v1 {
> > + u32 template_data_size;
> > + char template_data[0];
> > + } __packed;
> > +
> > + struct ima_kexec_hdr *khdr = buf;
> > + struct binary_hdr_v1 *hdr_v1;
> > + struct binary_data_v1 *data_v1;
> > +
> > + void *bufp = buf + sizeof(*khdr);
> > + void *bufendp = buf + khdr->buffer_size;
> > + struct ima_template_entry *entry;
> > + struct ima_template_desc *template_desc;
> > + unsigned long count = 0;
> > + int ret = 0;
> > +
> > + if (!buf || size < sizeof(*khdr))
> > + return 0;
> > +
> > + if (khdr->version != 1) {
> > + pr_err("attempting to restore a incompatible measurement list");
> > + return 0;
> > + }
> > +
> > + /*
> > + * ima kexec buffer prefix: version, buffer size, count
> > + * v1 format: pcr, digest, template-name-len, template-name,
> > + * template-data-size, template-data
> > + */
> > + while ((bufp < bufendp) && (count++ < khdr->count)) {
> > + if (count > ULONG_MAX - 1) {
> > + pr_err("attempting to restore too many measurements");
> > + ret = -EINVAL;
> > + }
> > +
> > + hdr_v1 = bufp;
> > + if ((hdr_v1->template_name_len >= MAX_TEMPLATE_NAME_LEN) ||
> > + ((bufp + hdr_v1->template_name_len) > bufendp)) {
>
> based on following code template_name_len does not include header
> (sizeof(*hdr_v1))?
> If so the check is wrong???
Yes, good catch. In addition, before assigning hdr_v1 there should be a
size check as well.
>
> > + pr_err("attempting to restore a template name \
> > + that is too long\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + bufp += sizeof(*hdr_v1);
> > +
> > + /* template name is not null terminated */
> > + memcpy(template_name, bufp, hdr_v1->template_name_len);
> > + template_name[hdr_v1->template_name_len] = 0;
> > +
> > + if (strcmp(template_name, "ima") == 0) {
> > + pr_err("attempting to restore an unsupported \
> > + template \"%s\" failed\n", template_name);
> > + ret = -EINVAL;
> > + break;
> > + }
> > + data_v1 = bufp += (u_int8_t)hdr_v1->template_name_len;
> > +
> > + /* get template format */
> > + template_desc = lookup_template_desc(template_name);
> > + if (!template_desc) {
> > + pr_err("template \"%s\" not found\n", template_name);
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
> > + if (bufp > (bufendp - sizeof(data_v1->template_data_size))) {
> > + pr_err("restoring the template data size failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > + bufp += (u_int8_t) sizeof(data_v1->template_data_size);
> > +
> > + if (bufp > (bufendp - data_v1->template_data_size)) {
> > + pr_err("restoring the template data failed\n");
> > + ret = -EINVAL;
> > + break;
> > + }
> > +
>
> It looks like a similar problem... sizeof(struct binary_data_v1) is
> missing in the check...
Following the template name, is the template data length and the
template data.
> > + ret = ima_restore_template_data(template_desc,
> > + data_v1->template_data,
> > + data_v1->template_data_size,
> > + &entry);
> > + if (ret < 0)
> > + break;
> > +
> > + memcpy(entry->digest, hdr_v1->digest, TPM_DIGEST_SIZE);
> > + entry->pcr = hdr_v1->pcr;
> > + ret = ima_restore_measurement_entry(entry);
> > + if (ret < 0)
> > + break;
> > +
> > + bufp += data_v1->template_data_size;
> > + }
> > + return ret;
> > +}
> > --
> > 2.7.4
> >
>
> In overall it is a bit hard to read this function somehow..
Ok, I'll see if there is any way of simplifying/cleaning up walking the
measurement list some more.
Mimi
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply
* Re: [PATCH] PCI: Don't attempt to claim shadow copies of ROM
From: Bjorn Helgaas @ 2016-11-08 20:49 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Vecu Bosseur
In-Reply-To: <20161107235158.32425.50331.stgit@bhelgaas-glaptop.roam.corp.google.com>
On Mon, Nov 07, 2016 at 05:51:58PM -0600, Bjorn Helgaas wrote:
> If we're using a shadow copy of a PCI device ROM, the shadow copy is in RAM
> and the device never sees accesses to it and doesn't respond to it. We
> don't have to route the shadow range to the PCI device, and the device
> doesn't have to claim the range.
>
> Previously we treated the shadow copy as though it were the ROM BAR, and we
> failed to claim it because the region wasn't routed to the device:
>
> pci 0000:01:00.0: Video device with shadowed ROM at [mem 0x000c0000-0x000dffff]
> pci_bus 0000:01: Allocating resources
> pci 0000:01:00.0: can't claim BAR 6 [mem 0x000c0000-0x000dffff]: no compatible bridge window
>
> The failure path of pcibios_allocate_dev_rom_resource() cleared out the
> resource start address, which caused the following ioremap() warning:
>
> WARNING: CPU: 0 PID: 116 at /build/linux-akdJXO/linux-4.8.0/arch/x86/mm/ioremap.c:121 __ioremap_caller+0x1ec/0x370
> ioremap on RAM at 0x0000000000000000 - 0x000000000001ffff
>
> Handle a shadow copy as RAM, without inserting it into the iomem tree.
>
> This fixes a regression caused by 0c0e0736acad ("PCI: Set ROM shadow
> location in arch code, not in PCI core"), which appeared in v4.6. The
> effect is failure to initialize video devices, reported on AMD Turks but
> likely to affect others as well.
>
> Fixes: 0c0e0736acad ("PCI: Set ROM shadow location in arch code, not in PCI core")
> Reported-and-tested-by: Vecu Bosseur <vecu.bosseur@gmail.com>
> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1627496
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=175391
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1352272
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> CC: stable@vger.kernel.org # v4.6+
I applied this to for-linus for v4.9.
I'm surprised we haven't seen more problems from this, given that it's
been there since v4.6, and anything using the shadow copy should be
broken.
> ---
> drivers/pci/setup-res.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 66c4d8f..9526e34 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -121,6 +121,14 @@ int pci_claim_resource(struct pci_dev *dev, int resource)
> return -EINVAL;
> }
>
> + /*
> + * If we have a shadow copy in RAM, the PCI device doesn't respond
> + * to the shadow range, so we don't need to claim it, and upstream
> + * bridges don't need to route the range to the device.
> + */
> + if (res->flags & IORESOURCE_ROM_SHADOW)
> + return 0;
> +
> root = pci_find_parent_resource(dev, res);
> if (!root) {
> dev_info(&dev->dev, "can't claim BAR %d %pR: no compatible bridge window\n",
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
From: Long Li @ 2016-11-08 20:49 UTC (permalink / raw)
To: Long Li, Greg KH
Cc: linux-pci@vger.kernel.org, Haiyang Zhang,
linux-kernel@vger.kernel.org, Bjorn Helgaas,
devel@linuxdriverproject.org
In-Reply-To: <BN3PR03MB2227A432B0288764BA9EE562CEA60@BN3PR03MB2227.namprd03.prod.outlook.com>
> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of Long Li
> Sent: Tuesday, November 8, 2016 8:57 AM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: linux-pci@vger.kernel.org; Haiyang Zhang <haiyangz@microsoft.com>;
> linux-kernel@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>;
> devel@linuxdriverproject.org
> Subject: RE: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hyperca=
ll
> params buffer
>=20
> This sender failed our fraud detection checks and may not be who they
> appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing
>=20
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Monday, November 7, 2016 11:00 PM
> > To: Long Li <longli@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org
> > Subject: Re: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate
> > hypercall params buffer
> >
> > On Tue, Nov 08, 2016 at 12:14:14AM -0800, Long Li wrote:
> > > From: Long Li <longli@microsoft.com>
> > >
> > > hv_do_hypercall assumes that we pass a segment from a physically
> > continuous buffer. Buffer allocated on the stack may not work if
> > CONFIG_VMAP_STACK=3Dy is set. Use kmalloc to allocate this buffer.
> >
> > Please wrap your changelog at 72 columns.
> >
> > >
> > > Signed-off-by: Long Li <longli@microsoft.com>
> > > Reported-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > > drivers/pci/host/pci-hyperv.c | 24 +++++++++++++-----------
> > > 1 file changed, 13 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/host/pci-hyperv.c
> > > b/drivers/pci/host/pci-hyperv.c index 763ff87..97e6daf 100644
> > > --- a/drivers/pci/host/pci-hyperv.c
> > > +++ b/drivers/pci/host/pci-hyperv.c
> > > @@ -378,6 +378,7 @@ struct hv_pcibus_device {
> > > struct msi_domain_info msi_info;
> > > struct msi_controller msi_chip;
> > > struct irq_domain *irq_domain;
> > > + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> >
> > Can you handle potentially unaligned accesses like this? Is there
> > some lock preventing you from using this structure more than once at th=
e
> same time?
> >
> > > };
> > >
> > > /*
> > > @@ -774,7 +775,7 @@ void hv_irq_unmask(struct irq_data *data) {
> > > struct msi_desc *msi_desc =3D irq_data_get_msi_desc(data);
> > > struct irq_cfg *cfg =3D irqd_cfg(data);
> > > - struct retarget_msi_interrupt params;
> > > + struct retarget_msi_interrupt *params;
> > > struct hv_pcibus_device *hbus;
> > > struct cpumask *dest;
> > > struct pci_bus *pbus;
> > > @@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
> > > pdev =3D msi_desc_to_pci_dev(msi_desc);
> > > pbus =3D pdev->bus;
> > > hbus =3D container_of(pbus->sysdata, struct hv_pcibus_device,
> > > sysdata);
> > > -
> > > - memset(¶ms, 0, sizeof(params));
> > > - params.partition_id =3D HV_PARTITION_ID_SELF;
> > > - params.source =3D 1; /* MSI(-X) */
> > > - params.address =3D msi_desc->msg.address_lo;
> > > - params.data =3D msi_desc->msg.data;
> > > - params.device_id =3D (hbus->hdev->dev_instance.b[5] << 24) |
> > > + params =3D &hbus->retarget_msi_interrupt_params;
> > > +
> > > + memset(params, 0, sizeof(*params));
> > > + params->partition_id =3D HV_PARTITION_ID_SELF;
> > > + params->source =3D 1; /* MSI(-X) */
> > > + params->address =3D msi_desc->msg.address_lo;
> > > + params->data =3D msi_desc->msg.data;
> > > + params->device_id =3D (hbus->hdev->dev_instance.b[5] << 24) |
> > > (hbus->hdev->dev_instance.b[4] << 16) |
> > > (hbus->hdev->dev_instance.b[7] << 8) |
> > > (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > > PCI_FUNC(pdev->devfn);
> > > - params.vector =3D cfg->vector;
> > > + params->vector =3D cfg->vector;
> > >
> > > for_each_cpu_and(cpu, dest, cpu_online_mask)
> > > - params.vp_mask |=3D (1ULL <<
> > vmbus_cpu_number_to_vp_number(cpu));
> > > + params->vp_mask |=3D (1ULL <<
> > vmbus_cpu_number_to_vp_number(cpu));
> > >
> > > - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, ¶ms, NULL);
> > > + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> >
> > As you only use this in one spot, why not just allocate it here and
> > then free it? Why add it to the pcibus device structure?
>=20
> Thanks Greg. I will send a V2.
Sorry forgot to address the reason why we don't just allocate the buffer he=
re. This function cannot fail. So it's better to pre-allocate the buffer.
>=20
> >
> > thanks,
> >
> > greg k-h
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> https://na01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fdriverd
> ev.linuxdriverproject.org%2Fmailman%2Flistinfo%2Fdriverdev-
> devel&data=3D02%7C01%7Clongli%40microsoft.com%7C6e28f5459da345cdb5e
> 408d407f836f0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636142
> 210041216500&sdata=3DvvVxGQUet7KMuRgs9%2BRbR8JE70rKF1AJo%2Fu3zx%
> 2FQNwY%3D&reserved=3D0
^ permalink raw reply
* RE: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall params buffer
From: Long Li @ 2016-11-08 20:49 UTC (permalink / raw)
To: Long Li, Greg KH
Cc: linux-pci@vger.kernel.org, Haiyang Zhang,
linux-kernel@vger.kernel.org, Bjorn Helgaas,
devel@linuxdriverproject.org
In-Reply-To: <BN3PR03MB2227A432B0288764BA9EE562CEA60@BN3PR03MB2227.namprd03.prod.outlook.com>
> -----Original Message-----
> From: devel [mailto:driverdev-devel-bounces@linuxdriverproject.org] On
> Behalf Of Long Li
> Sent: Tuesday, November 8, 2016 8:57 AM
> To: Greg KH <gregkh@linuxfoundation.org>
> Cc: linux-pci@vger.kernel.org; Haiyang Zhang <haiyangz@microsoft.com>;
> linux-kernel@vger.kernel.org; Bjorn Helgaas <bhelgaas@google.com>;
> devel@linuxdriverproject.org
> Subject: RE: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate hypercall
> params buffer
>
> This sender failed our fraud detection checks and may not be who they
> appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing
>
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Monday, November 7, 2016 11:00 PM
> > To: Long Li <longli@microsoft.com>
> > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>;
> > devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; linux-
> > pci@vger.kernel.org
> > Subject: Re: [Resend] [PATCH] pci-hyperv: use kmalloc to allocate
> > hypercall params buffer
> >
> > On Tue, Nov 08, 2016 at 12:14:14AM -0800, Long Li wrote:
> > > From: Long Li <longli@microsoft.com>
> > >
> > > hv_do_hypercall assumes that we pass a segment from a physically
> > continuous buffer. Buffer allocated on the stack may not work if
> > CONFIG_VMAP_STACK=y is set. Use kmalloc to allocate this buffer.
> >
> > Please wrap your changelog at 72 columns.
> >
> > >
> > > Signed-off-by: Long Li <longli@microsoft.com>
> > > Reported-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > > drivers/pci/host/pci-hyperv.c | 24 +++++++++++++-----------
> > > 1 file changed, 13 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/host/pci-hyperv.c
> > > b/drivers/pci/host/pci-hyperv.c index 763ff87..97e6daf 100644
> > > --- a/drivers/pci/host/pci-hyperv.c
> > > +++ b/drivers/pci/host/pci-hyperv.c
> > > @@ -378,6 +378,7 @@ struct hv_pcibus_device {
> > > struct msi_domain_info msi_info;
> > > struct msi_controller msi_chip;
> > > struct irq_domain *irq_domain;
> > > + struct retarget_msi_interrupt retarget_msi_interrupt_params;
> >
> > Can you handle potentially unaligned accesses like this? Is there
> > some lock preventing you from using this structure more than once at the
> same time?
> >
> > > };
> > >
> > > /*
> > > @@ -774,7 +775,7 @@ void hv_irq_unmask(struct irq_data *data) {
> > > struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> > > struct irq_cfg *cfg = irqd_cfg(data);
> > > - struct retarget_msi_interrupt params;
> > > + struct retarget_msi_interrupt *params;
> > > struct hv_pcibus_device *hbus;
> > > struct cpumask *dest;
> > > struct pci_bus *pbus;
> > > @@ -785,23 +786,24 @@ void hv_irq_unmask(struct irq_data *data)
> > > pdev = msi_desc_to_pci_dev(msi_desc);
> > > pbus = pdev->bus;
> > > hbus = container_of(pbus->sysdata, struct hv_pcibus_device,
> > > sysdata);
> > > -
> > > - memset(¶ms, 0, sizeof(params));
> > > - params.partition_id = HV_PARTITION_ID_SELF;
> > > - params.source = 1; /* MSI(-X) */
> > > - params.address = msi_desc->msg.address_lo;
> > > - params.data = msi_desc->msg.data;
> > > - params.device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > > + params = &hbus->retarget_msi_interrupt_params;
> > > +
> > > + memset(params, 0, sizeof(*params));
> > > + params->partition_id = HV_PARTITION_ID_SELF;
> > > + params->source = 1; /* MSI(-X) */
> > > + params->address = msi_desc->msg.address_lo;
> > > + params->data = msi_desc->msg.data;
> > > + params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > > (hbus->hdev->dev_instance.b[4] << 16) |
> > > (hbus->hdev->dev_instance.b[7] << 8) |
> > > (hbus->hdev->dev_instance.b[6] & 0xf8) |
> > > PCI_FUNC(pdev->devfn);
> > > - params.vector = cfg->vector;
> > > + params->vector = cfg->vector;
> > >
> > > for_each_cpu_and(cpu, dest, cpu_online_mask)
> > > - params.vp_mask |= (1ULL <<
> > vmbus_cpu_number_to_vp_number(cpu));
> > > + params->vp_mask |= (1ULL <<
> > vmbus_cpu_number_to_vp_number(cpu));
> > >
> > > - hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, ¶ms, NULL);
> > > + hv_do_hypercall(HVCALL_RETARGET_INTERRUPT, params, NULL);
> >
> > As you only use this in one spot, why not just allocate it here and
> > then free it? Why add it to the pcibus device structure?
>
> Thanks Greg. I will send a V2.
Sorry forgot to address the reason why we don't just allocate the buffer here. This function cannot fail. So it's better to pre-allocate the buffer.
>
> >
> > thanks,
> >
> > greg k-h
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdriverd
> ev.linuxdriverproject.org%2Fmailman%2Flistinfo%2Fdriverdev-
> devel&data=02%7C01%7Clongli%40microsoft.com%7C6e28f5459da345cdb5e
> 408d407f836f0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636142
> 210041216500&sdata=vvVxGQUet7KMuRgs9%2BRbR8JE70rKF1AJo%2Fu3zx%
> 2FQNwY%3D&reserved=0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.