[PATCH] crypto, xor: Sanitize checksumming function selection output

[Borislav Petkov]

From: Borislav Petkov borislav.pet...@amd.com

Currently, it says

[1.015541] xor: automatically using best checksumming function: generic_sse
[1.040769]generic_sse:  6679.000 MB/sec
[1.045377] xor: using function: generic_sse (6679.000 MB/sec)

and repeats the function name three times unnecessarily. Change it into

[1.015115] xor: automatically using best checksumming function:
[1.040794]generic_sse:  6680.000 MB/sec

and save us a line in dmesg.

No functional change.

Cc: Herbert Xu herb...@gondor.apana.org.au
Signed-off-by: Borislav Petkov borislav.pet...@amd.com
---
 crypto/xor.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/crypto/xor.c b/crypto/xor.c
index b75182d8ab14..664b6dfa9e2c 100644
--- a/crypto/xor.c
+++ b/crypto/xor.c
@@ -129,9 +129,9 @@ calibrate_xor_blocks(void)
 
if (fastest) {
printk(KERN_INFO xor: automatically using best 
-   checksumming function: %s\n,
-   fastest-name);
+checksumming function:\n);
xor_speed(fastest);
+   goto out;
} else {
printk(KERN_INFO xor: measuring software checksum speed\n);
XOR_TRY_TEMPLATES;
@@ -146,6 +146,7 @@ calibrate_xor_blocks(void)
 
 #undef xor_speed
 
+ out:
free_pages((unsigned long)b1, 2);
 
active_template = fastest;
-- 
1.7.9.3.362.g71319

--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Wed, Aug 15, 2012 at 11:42:16AM +0300, Jussi Kivilinna wrote:
 I started thinking about the performance on AMD Bulldozer.
 vmovq/vmovd/vpextr*/vpinsr* between FPU and general purpose registers
 on AMD CPU is alot slower (latencies from 8 to 12 cycles) than on
 Intel sandy-bridge (where instructions have latency of 1 to 2). See:
 http://www.agner.org/optimize/instruction_tables.pdf

 It would be really good, if implementation could be tested on AMD CPU
 to determinate, if it causes performance regression. However I don't
 have access to machine with such CPU.

But I do. :)

And if you tell me exactly how to run the tests and on what kernel, I'll
try to do so.

HTH.

-- 
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

Ok, here we go. Raw data below.

On Wed, Aug 15, 2012 at 02:00:16PM +0300, Jussi Kivilinna wrote:
 And if you tell me exactly how to run the tests and on what kernel,
 I'll try to do so.

Ok, the box is a single-socket Bulldozer: AMD FX(tm)-8100 Eight-Core
Processor stepping 02; kernel is 3.6.0-rc1+ which is latest Linus +
tip/master merged ontop.

 Twofish-avx (CONFIG_TWOFISH_AVX_X86_64) is available in 3.6-rc1. For

I took CONFIG_CRYPTO_TWOFISH_AVX_X86_64 but I'm pretty sure you meant
that.

 testing you need CRYPTO_TEST build as module. You should turn off
 turbo-core, freq-scaling, etc.

$ for i in $(seq 0 7); do echo performance  
/sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor ; done
$ for i in $(seq 0 7); do echo 0  /sys/devices/system/cpu/cpu$i/cpufreq/cpb ; 
done

 Testing twofish-avx ('async twofish' speed test):
  modprobe twofish-avx-x86_64
  modprobe tcrypt mode=504 sec=1

$ modprobe twofish-avx-x86_64
$ modprobe tcrypt mode=504 sec=1

[  224.672094] 
[  224.672094] testing speed of async ecb(twofish) encryption
[  224.681444] test 0 (128 bit key, 16 byte blocks): 4862478 operations in 1 
seconds (77799648 bytes)
[  225.689190] test 1 (128 bit key, 64 byte blocks): 2040557 operations in 1 
seconds (130595648 bytes)
[  226.695864] test 2 (128 bit key, 256 byte blocks): 564098 operations in 1 
seconds (144409088 bytes)
[  227.702365] test 3 (128 bit key, 1024 byte blocks): 156553 operations in 1 
seconds (160310272 bytes)
[  228.708960] test 4 (128 bit key, 8192 byte blocks): 20128 operations in 1 
seconds (164888576 bytes)
[  229.715485] test 5 (192 bit key, 16 byte blocks): 4853879 operations in 1 
seconds (77662064 bytes)
[  230.722165] test 6 (192 bit key, 64 byte blocks): 2040187 operations in 1 
seconds (130571968 bytes)
[  231.729110] test 7 (192 bit key, 256 byte blocks): 564125 operations in 1 
seconds (144416000 bytes)
[  232.735600] test 8 (192 bit key, 1024 byte blocks): 156231 operations in 1 
seconds (159980544 bytes)
[  233.742205] test 9 (192 bit key, 8192 byte blocks): 19913 operations in 1 
seconds (163127296 bytes)
[  234.748777] test 10 (256 bit key, 16 byte blocks): 4880977 operations in 1 
seconds (78095632 bytes)
[  235.751405] test 11 (256 bit key, 64 byte blocks): 2045621 operations in 1 
seconds (130919744 bytes)
[  236.758079] test 12 (256 bit key, 256 byte blocks): 565273 operations in 1 
seconds (144709888 bytes)
[  237.764579] test 13 (256 bit key, 1024 byte blocks): 156625 operations in 1 
seconds (160384000 bytes)
[  238.771175] test 14 (256 bit key, 8192 byte blocks): 20125 operations in 1 
seconds (164864000 bytes)
[  239.26] 
[  239.26] testing speed of async ecb(twofish) decryption
[  239.787020] test 0 (128 bit key, 16 byte blocks): 4962193 operations in 1 
seconds (79395088 bytes)
[  240.792405] test 1 (128 bit key, 64 byte blocks): 2056765 operations in 1 
seconds (131632960 bytes)
[  241.799070] test 2 (128 bit key, 256 byte blocks): 559384 operations in 1 
seconds (143202304 bytes)
[  242.805568] test 3 (128 bit key, 1024 byte blocks): 153881 operations in 1 
seconds (157574144 bytes)
[  243.812191] test 4 (128 bit key, 8192 byte blocks): 19636 operations in 1 
seconds (160858112 bytes)
[  244.818718] test 5 (192 bit key, 16 byte blocks): 4917689 operations in 1 
seconds (78683024 bytes)
[  245.825408] test 6 (192 bit key, 64 byte blocks): 2056235 operations in 1 
seconds (131599040 bytes)
[  246.832070] test 7 (192 bit key, 256 byte blocks): 560579 operations in 1 
seconds (143508224 bytes)
[  247.838598] test 8 (192 bit key, 1024 byte blocks): 153813 operations in 1 
seconds (157504512 bytes)
[  248.845201] test 9 (192 bit key, 8192 byte blocks): 19411 operations in 1 
seconds (159014912 bytes)
[  249.851755] test 10 (256 bit key, 16 byte blocks): 4932508 operations in 1 
seconds (78920128 bytes)
[  250.858372] test 11 (256 bit key, 64 byte blocks): 2057244 operations in 1 
seconds (131663616 bytes)
[  251.865039] test 12 (256 bit key, 256 byte blocks): 559493 operations in 1 
seconds (143230208 bytes)
[  252.871554] test 13 (256 bit key, 1024 byte blocks): 153980 operations in 1 
seconds (157675520 bytes)
[  253.878159] test 14 (256 bit key, 8192 byte blocks): 19665 operations in 1 
seconds (161095680 bytes)
[  254.884711] 
[  254.884711] testing speed of async cbc(twofish) encryption
[  254.898925] test 0 (128 bit key, 16 byte blocks): 5194404 operations in 1 
seconds (83110464 bytes)
[  255.907087] test 1 (128 bit key, 64 byte blocks): 1916243 operations in 1 
seconds (122639552 bytes)
[  256.913758] test 2 (128 bit key, 256 byte blocks): 541282 operations in 1 
seconds (138568192 bytes)
[  257.916278] test 3 (128 bit key, 1024 byte blocks): 141389 operations in 1 
seconds (144782336 bytes)
[  258.918865] test 4 (128 bit key, 8192 byte blocks): 17811 operations in 1 
seconds (145907712 bytes)
[  259.925372] test 5 (192 bit key, 16 byte blocks): 5176387 operations in 1 
seconds (82822192 bytes)
[  260.932038] test 6 (192 bit key, 64 

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Wed, Aug 15, 2012 at 04:48:54PM +0300, Jussi Kivilinna wrote:
 I posted patch that optimize twofish-avx few weeks ago:
 http://marc.info/?l=linux-crypto-vgerm=134364845024825w=2

 I'd be interested to know, if this is patch helps on Bulldozer.

Sure, can you inline it here too please. The Download message RAW link
on marc.info gives me a diff but patch says:

patching file arch/x86/crypto/twofish-avx-x86_64-asm_64.S
patch unexpectedly ends in middle of line

Thanks.

-- 
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Wed, Aug 15, 2012 at 05:22:03PM +0300, Jussi Kivilinna wrote:

 Patch replaces 'movb' instructions with 'movzbl' to break false
 register dependencies and interleaves instructions better for
 out-of-order scheduling.

 Also move common round code to separate function to reduce object
 size.

Ok, redid the first test

$ modprobe twofish-avx-x86_64
$ modprobe tcrypt mode=504 sec=1

and from quickly juxtaposing the two results, I'd say the patch makes
things slightly worse but you'd need to run your scripts on it to get
the accurate results:

[   98.206067] testing speed of async ecb(twofish) encryption
[   98.214796] test 0 (128 bit key, 16 byte blocks): 4549296 operations in 1 
seconds (72788736 bytes)
[   99.221569] test 1 (128 bit key, 64 byte blocks): 1995934 operations in 1 
seconds (127739776 bytes)
[  100.228250] test 2 (128 bit key, 256 byte blocks): 535040 operations in 1 
seconds (136970240 bytes)
[  101.234751] test 3 (128 bit key, 1024 byte blocks): 148602 operations in 1 
seconds (152168448 bytes)
[  102.241345] test 4 (128 bit key, 8192 byte blocks): 19148 operations in 1 
seconds (156860416 bytes)
[  103.247880] test 5 (192 bit key, 16 byte blocks): 4558391 operations in 1 
seconds (72934256 bytes)
[  104.254547] test 6 (192 bit key, 64 byte blocks): 1997838 operations in 1 
seconds (127861632 bytes)
[  105.261202] test 7 (192 bit key, 256 byte blocks): 534396 operations in 1 
seconds (136805376 bytes)
[  106.267694] test 8 (192 bit key, 1024 byte blocks): 148199 operations in 1 
seconds (151755776 bytes)
[  107.274296] test 9 (192 bit key, 8192 byte blocks): 18913 operations in 1 
seconds (154935296 bytes)
[  108.280824] test 10 (256 bit key, 16 byte blocks): 4595524 operations in 1 
seconds (73528384 bytes)
[  109.287496] test 11 (256 bit key, 64 byte blocks): 1997893 operations in 1 
seconds (127865152 bytes)
[  110.294168] test 12 (256 bit key, 256 byte blocks): 533790 operations in 1 
seconds (136650240 bytes)
[  111.300679] test 13 (256 bit key, 1024 byte blocks): 148787 operations in 1 
seconds (152357888 bytes)
[  112.303561] test 14 (256 bit key, 8192 byte blocks): 19146 operations in 1 
seconds (156844032 bytes)
[  113.310104] 
[  113.310104] testing speed of async ecb(twofish) decryption
[  113.319419] test 0 (128 bit key, 16 byte blocks): 4754043 operations in 1 
seconds (76064688 bytes)
[  114.324768] test 1 (128 bit key, 64 byte blocks): 1831420 operations in 1 
seconds (117210880 bytes)
[  115.331441] test 2 (128 bit key, 256 byte blocks): 541170 operations in 1 
seconds (138539520 bytes)
[  116.337957] test 3 (128 bit key, 1024 byte blocks): 150538 operations in 1 
seconds (154150912 bytes)
[  117.344571] test 4 (128 bit key, 8192 byte blocks): 19397 operations in 1 
seconds (158900224 bytes)
[  118.351122] test 5 (192 bit key, 16 byte blocks): 4753957 operations in 1 
seconds (76063312 bytes)
[  119.357778] test 6 (192 bit key, 64 byte blocks): 1828676 operations in 1 
seconds (117035264 bytes)
[  120.364459] test 7 (192 bit key, 256 byte blocks): 540331 operations in 1 
seconds (138324736 bytes)
[  121.370969] test 8 (192 bit key, 1024 byte blocks): 150348 operations in 1 
seconds (153956352 bytes)
[  122.377573] test 9 (192 bit key, 8192 byte blocks): 19196 operations in 1 
seconds (157253632 bytes)
[  123.384080] test 10 (256 bit key, 16 byte blocks): 4664399 operations in 1 
seconds (74630384 bytes)
[  124.390782] test 11 (256 bit key, 64 byte blocks): 1839324 operations in 1 
seconds (117716736 bytes)
[  125.397463] test 12 (256 bit key, 256 byte blocks): 538735 operations in 1 
seconds (137916160 bytes)
[  126.403962] test 13 (256 bit key, 1024 byte blocks): 150489 operations in 1 
seconds (154100736 bytes)
[  127.410567] test 14 (256 bit key, 8192 byte blocks): 19397 operations in 1 
seconds (158900224 bytes)
[  128.417091] 
[  128.417091] testing speed of async cbc(twofish) encryption
[  128.431227] test 0 (128 bit key, 16 byte blocks): 4681239 operations in 1 
seconds (74899824 bytes)
[  129.439466] test 1 (128 bit key, 64 byte blocks): 1836636 operations in 1 
seconds (117544704 bytes)
[  130.446131] test 2 (128 bit key, 256 byte blocks): 536055 operations in 1 
seconds (137230080 bytes)
[  131.452631] test 3 (128 bit key, 1024 byte blocks): 140955 operations in 1 
seconds (144337920 bytes)
[  132.459243] test 4 (128 bit key, 8192 byte blocks): 17821 operations in 1 
seconds (145989632 bytes)
[  133.466124] test 5 (192 bit key, 16 byte blocks): 4674373 operations in 1 
seconds (74789968 bytes)
[  134.472728] test 6 (192 bit key, 64 byte blocks): 1835821 operations in 1 
seconds (117492544 bytes)
[  135.479374] test 7 (192 bit key, 256 byte blocks): 535882 operations in 1 
seconds (137185792 bytes)
[  136.485876] test 8 (192 bit key, 1024 byte blocks): 140917 operations in 1 
seconds (144299008 bytes)
[  137.492470] test 9 (192 bit key, 8192 byte blocks): 17707 operations in 1 
seconds (145055744 bytes)
[  138.498979] test 10 (256 bit key, 16 byte blocks): 4674648 

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Wed, Aug 15, 2012 at 08:34:25PM +0300, Jussi Kivilinna wrote:
 About ~5% slower, probably because I was tuning for sandy-bridge and
 introduced more FPU=CPU register moves.

 Here's new version of patch, with FPU=CPU moves from original
 implementation.

 (Note: also changes encryption function to inline all code in to main
 function, decryption still places common code to separate function to
 reduce object size. This is to measure the difference.)

Yep, looks better than the previous run and also a bit better or on par
with the initial run I did.

The thing is, I'm not sure whether optimizing the thing for each uarch
is a workable solution software-wise or maybe having a single version
which performs sufficiently ok on all uarches is easier/better to
maintain without causing code bloat. Hmmm...

4th:

ran like 1st.

[ 1014.074150] 
[ 1014.074150] testing speed of async ecb(twofish) encryption
[ 1014.083829] test 0 (128 bit key, 16 byte blocks): 4870055 operations in 1 
seconds (77920880 bytes)
[ 1015.092757] test 1 (128 bit key, 64 byte blocks): 2043828 operations in 1 
seconds (130804992 bytes)
[ 1016.099441] test 2 (128 bit key, 256 byte blocks): 606400 operations in 1 
seconds (155238400 bytes)
[ 1017.105939] test 3 (128 bit key, 1024 byte blocks): 168939 operations in 1 
seconds (172993536 bytes)
[ 1018.112517] test 4 (128 bit key, 8192 byte blocks): 21777 operations in 1 
seconds (178397184 bytes)
[ 1019.119035] test 5 (192 bit key, 16 byte blocks): 4882254 operations in 1 
seconds (78116064 bytes)
[ 1020.125716] test 6 (192 bit key, 64 byte blocks): 2043230 operations in 1 
seconds (130766720 bytes)
[ 1021.132391] test 7 (192 bit key, 256 byte blocks): 607477 operations in 1 
seconds (155514112 bytes)
[ 1022.138889] test 8 (192 bit key, 1024 byte blocks): 168743 operations in 1 
seconds (172792832 bytes)
[ 1023.145476] test 9 (192 bit key, 8192 byte blocks): 21442 operations in 1 
seconds (175652864 bytes)
[ 1024.152012] test 10 (256 bit key, 16 byte blocks): 4891863 operations in 1 
seconds (78269808 bytes)
[ 1025.158684] test 11 (256 bit key, 64 byte blocks): 2049390 operations in 1 
seconds (131160960 bytes)
[ 1026.165366] test 12 (256 bit key, 256 byte blocks): 606847 operations in 1 
seconds (155352832 bytes)
[ 1027.171841] test 13 (256 bit key, 1024 byte blocks): 169228 operations in 1 
seconds (173289472 bytes)
[ 1028.178436] test 14 (256 bit key, 8192 byte blocks): 21773 operations in 1 
seconds (178364416 bytes)
[ 1029.184981] 
[ 1029.184981] testing speed of async ecb(twofish) decryption
[ 1029.194508] test 0 (128 bit key, 16 byte blocks): 4931065 operations in 1 
seconds (78897040 bytes)
[ 1030.199640] test 1 (128 bit key, 64 byte blocks): 2056931 operations in 1 
seconds (131643584 bytes)
[ 1031.206303] test 2 (128 bit key, 256 byte blocks): 589409 operations in 1 
seconds (150888704 bytes)
[ 1032.212832] test 3 (128 bit key, 1024 byte blocks): 163681 operations in 1 
seconds (167609344 bytes)
[ 1033.219443] test 4 (128 bit key, 8192 byte blocks): 21062 operations in 1 
seconds (172539904 bytes)
[ 1034.225979] test 5 (192 bit key, 16 byte blocks): 4931537 operations in 1 
seconds (78904592 bytes)
[ 1035.232608] test 6 (192 bit key, 64 byte blocks): 2053989 operations in 1 
seconds (131455296 bytes)
[ 1036.239289] test 7 (192 bit key, 256 byte blocks): 589591 operations in 1 
seconds (150935296 bytes)
[ 1037.241784] test 8 (192 bit key, 1024 byte blocks): 163565 operations in 1 
seconds (167490560 bytes)
[ 1038.244387] test 9 (192 bit key, 8192 byte blocks): 20899 operations in 1 
seconds (171204608 bytes)
[ 1039.250923] test 10 (256 bit key, 16 byte blocks): 4937343 operations in 1 
seconds (78997488 bytes)
[ 1040.257589] test 11 (256 bit key, 64 byte blocks): 2050678 operations in 1 
seconds (131243392 bytes)
[ 1041.264262] test 12 (256 bit key, 256 byte blocks): 586869 operations in 1 
seconds (150238464 bytes)
[ 1042.270753] test 13 (256 bit key, 1024 byte blocks): 163548 operations in 1 
seconds (167473152 bytes)
[ 1043.277365] test 14 (256 bit key, 8192 byte blocks): 21053 operations in 1 
seconds (172466176 bytes)
[ 1044.283892] 
[ 1044.283892] testing speed of async cbc(twofish) encryption
[ 1044.293349] test 0 (128 bit key, 16 byte blocks): 5186240 operations in 1 
seconds (82979840 bytes)
[ 1045.298534] test 1 (128 bit key, 64 byte blocks): 1921034 operations in 1 
seconds (122946176 bytes)
[ 1046.305207] test 2 (128 bit key, 256 byte blocks): 542787 operations in 1 
seconds (138953472 bytes)
[ 1047.311699] test 3 (128 bit key, 1024 byte blocks): 141399 operations in 1 
seconds (144792576 bytes)
[ 1048.318312] test 4 (128 bit key, 8192 byte blocks): 17755 operations in 1 
seconds (145448960 bytes)
[ 1049.324829] test 5 (192 bit key, 16 byte blocks): 5196441 operations in 1 
seconds (83143056 bytes)
[ 1050.331485] test 6 (192 bit key, 64 byte blocks): 1921456 operations in 1 
seconds (122973184 bytes)
[ 1051.338157] test 7 (192 bit key, 256 byte blocks): 543581 

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Wed, Aug 22, 2012 at 07:35:12AM +0300, Jussi Kivilinna wrote:
 Looks that encryption lost ~0.4% while decryption gained ~1.8%.
 
 For 256 byte test, it's still slightly slower than twofish-3way (~3%). For 1k
 and 8k tests, it's ~5% faster.
 
 Here's very last test-patch, testing different ordering of fpu-cpu reg
 instructions at few places.

Hehe,

I don't mind testing patches, no worries there. Here are the results
this time, doesn't look better than the last run, AFAICT.

[  133.952723] 
[  133.952723] testing speed of async ecb(twofish) encryption
[  133.961946] test 0 (128 bit key, 16 byte blocks): 4768513 operations in 1 
seconds (76296208 bytes)
[  134.968388] test 1 (128 bit key, 64 byte blocks): 2033479 operations in 1 
seconds (130142656 bytes)
[  135.975070] test 2 (128 bit key, 256 byte blocks): 604754 operations in 1 
seconds (154817024 bytes)
[  136.981570] test 3 (128 bit key, 1024 byte blocks): 169578 operations in 1 
seconds (173647872 bytes)
[  137.988191] test 4 (128 bit key, 8192 byte blocks): 21847 operations in 1 
seconds (178970624 bytes)
[  138.994735] test 5 (192 bit key, 16 byte blocks): 4777481 operations in 1 
seconds (76439696 bytes)
[  140.001382] test 6 (192 bit key, 64 byte blocks): 2035352 operations in 1 
seconds (130262528 bytes)
[  141.008038] test 7 (192 bit key, 256 byte blocks): 603240 operations in 1 
seconds (154429440 bytes)
[  142.014591] test 8 (192 bit key, 1024 byte blocks): 169266 operations in 1 
seconds (173328384 bytes)
[  143.021169] test 9 (192 bit key, 8192 byte blocks): 21610 operations in 1 
seconds (177029120 bytes)
[  144.027703] test 10 (256 bit key, 16 byte blocks): 4798051 operations in 1 
seconds (76768816 bytes)
[  145.034341] test 11 (256 bit key, 64 byte blocks): 2036766 operations in 1 
seconds (130353024 bytes)
[  146.041015] test 12 (256 bit key, 256 byte blocks): 604216 operations in 1 
seconds (154679296 bytes)
[  147.047523] test 13 (256 bit key, 1024 byte blocks): 169594 operations in 1 
seconds (173664256 bytes)
[  148.054120] test 14 (256 bit key, 8192 byte blocks): 21889 operations in 1 
seconds (179314688 bytes)
[  149.060657] 
[  149.060657] testing speed of async ecb(twofish) decryption
[  149.069830] test 0 (128 bit key, 16 byte blocks): 4890581 operations in 1 
seconds (78249296 bytes)
[  150.075322] test 1 (128 bit key, 64 byte blocks): 2006891 operations in 1 
seconds (128441024 bytes)
[  151.081994] test 2 (128 bit key, 256 byte blocks): 586650 operations in 1 
seconds (150182400 bytes)
[  152.088522] test 3 (128 bit key, 1024 byte blocks): 164734 operations in 1 
seconds (168687616 bytes)
[  153.091153] test 4 (128 bit key, 8192 byte blocks): 2 operations in 1 
seconds (172941312 bytes)
[  154.097687] test 5 (192 bit key, 16 byte blocks): 4911365 operations in 1 
seconds (78581840 bytes)
[  155.104371] test 6 (192 bit key, 64 byte blocks): 2025363 operations in 1 
seconds (129623232 bytes)
[  156.54] test 7 (192 bit key, 256 byte blocks): 591229 operations in 1 
seconds (151354624 bytes)
[  157.117723] test 8 (192 bit key, 1024 byte blocks): 164381 operations in 1 
seconds (168326144 bytes)
[  158.124336] test 9 (192 bit key, 8192 byte blocks): 20714 operations in 1 
seconds (169689088 bytes)
[  159.130724] test 10 (256 bit key, 16 byte blocks): 4931938 operations in 1 
seconds (78911008 bytes)
[  160.137379] test 11 (256 bit key, 64 byte blocks): 2029741 operations in 1 
seconds (129903424 bytes)
[  161.144078] test 12 (256 bit key, 256 byte blocks): 589340 operations in 1 
seconds (150871040 bytes)
[  162.150580] test 13 (256 bit key, 1024 byte blocks): 164484 operations in 1 
seconds (168431616 bytes)
[  163.157174] test 14 (256 bit key, 8192 byte blocks): 21116 operations in 1 
seconds (172982272 bytes)
[  164.163694] 
[  164.163694] testing speed of async cbc(twofish) encryption
[  164.12] test 0 (128 bit key, 16 byte blocks): 5197069 operations in 1 
seconds (83153104 bytes)
[  165.186414] test 1 (128 bit key, 64 byte blocks): 1912975 operations in 1 
seconds (122430400 bytes)
[  166.193078] test 2 (128 bit key, 256 byte blocks): 540464 operations in 1 
seconds (138358784 bytes)
[  167.199587] test 3 (128 bit key, 1024 byte blocks): 140709 operations in 1 
seconds (144086016 bytes)
[  168.206209] test 4 (128 bit key, 8192 byte blocks): 17747 operations in 1 
seconds (145383424 bytes)
[  169.212768] test 5 (192 bit key, 16 byte blocks): 5184004 operations in 1 
seconds (82944064 bytes)
[  170.219372] test 6 (192 bit key, 64 byte blocks): 1913377 operations in 1 
seconds (122456128 bytes)
[  171.226028] test 7 (192 bit key, 256 byte blocks): 541385 operations in 1 
seconds (138594560 bytes)
[  172.232538] test 8 (192 bit key, 1024 byte blocks): 140867 operations in 1 
seconds (144247808 bytes)
[  173.239280] test 9 (192 bit key, 8192 byte blocks): 17642 operations in 1 
seconds (144523264 bytes)
[  174.245667] test 10 (256 bit key, 16 byte blocks): 5193804 operations in 1 
seconds (83100864 bytes)
[  

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Wed, Aug 22, 2012 at 10:20:03PM +0300, Jussi Kivilinna wrote:
 Actually it does look better, at least for encryption. Decryption had 
 different
 ordering for test, which appears to be bad on bulldozer as it is on
 sandy-bridge.
 
 So, yet another patch then :)

Here you go:

[  153.736745] 
[  153.736745] testing speed of async ecb(twofish) encryption
[  153.745806] test 0 (128 bit key, 16 byte blocks): 4832343 operations in 1 
seconds (77317488 bytes)
[  154.752525] test 1 (128 bit key, 64 byte blocks): 2049979 operations in 1 
seconds (131198656 bytes)
[  155.755195] test 2 (128 bit key, 256 byte blocks): 620439 operations in 1 
seconds (158832384 bytes)
[  156.761694] test 3 (128 bit key, 1024 byte blocks): 173900 operations in 1 
seconds (178073600 bytes)
[  157.768282] test 4 (128 bit key, 8192 byte blocks): 22366 operations in 1 
seconds (18372 bytes)
[  158.774815] test 5 (192 bit key, 16 byte blocks): 4850741 operations in 1 
seconds (77611856 bytes)
[  159.781498] test 6 (192 bit key, 64 byte blocks): 2046772 operations in 1 
seconds (130993408 bytes)
[  160.788163] test 7 (192 bit key, 256 byte blocks): 619915 operations in 1 
seconds (158698240 bytes)
[  161.794636] test 8 (192 bit key, 1024 byte blocks): 173442 operations in 1 
seconds (177604608 bytes)
[  162.801242] test 9 (192 bit key, 8192 byte blocks): 22083 operations in 1 
seconds (180903936 bytes)
[  163.807793] test 10 (256 bit key, 16 byte blocks): 4862951 operations in 1 
seconds (77807216 bytes)
[  164.814449] test 11 (256 bit key, 64 byte blocks): 2050036 operations in 1 
seconds (131202304 bytes)
[  165.821121] test 12 (256 bit key, 256 byte blocks): 620349 operations in 1 
seconds (158809344 bytes)
[  166.827621] test 13 (256 bit key, 1024 byte blocks): 173917 operations in 1 
seconds (178091008 bytes)
[  167.834218] test 14 (256 bit key, 8192 byte blocks): 22362 operations in 1 
seconds (183189504 bytes)
[  168.840798] 
[  168.840798] testing speed of async ecb(twofish) decryption
[  168.849968] test 0 (128 bit key, 16 byte blocks): 4889899 operations in 1 
seconds (78238384 bytes)
[  169.855439] test 1 (128 bit key, 64 byte blocks): 2052293 operations in 1 
seconds (131346752 bytes)
[  170.862113] test 2 (128 bit key, 256 byte blocks): 616979 operations in 1 
seconds (157946624 bytes)
[  171.868631] test 3 (128 bit key, 1024 byte blocks): 172773 operations in 1 
seconds (176919552 bytes)
[  172.875244] test 4 (128 bit key, 8192 byte blocks): 4 operations in 1 
seconds (182059008 bytes)
[  173.881777] test 5 (192 bit key, 16 byte blocks): 4893653 operations in 1 
seconds (78298448 bytes)
[  174.888451] test 6 (192 bit key, 64 byte blocks): 2048078 operations in 1 
seconds (131076992 bytes)
[  175.895131] test 7 (192 bit key, 256 byte blocks): 619204 operations in 1 
seconds (158516224 bytes)
[  176.901651] test 8 (192 bit key, 1024 byte blocks): 172569 operations in 1 
seconds (176710656 bytes)
[  177.908253] test 9 (192 bit key, 8192 byte blocks): 21888 operations in 1 
seconds (179306496 bytes)
[  178.914781] test 10 (256 bit key, 16 byte blocks): 4921751 operations in 1 
seconds (78748016 bytes)
[  179.917481] test 11 (256 bit key, 64 byte blocks): 2051219 operations in 1 
seconds (131278016 bytes)
[  180.920147] test 12 (256 bit key, 256 byte blocks): 618536 operations in 1 
seconds (158345216 bytes)
[  181.926637] test 13 (256 bit key, 1024 byte blocks): 172886 operations in 1 
seconds (177035264 bytes)
[  182.933249] test 14 (256 bit key, 8192 byte blocks): 2 operations in 1 
seconds (182042624 bytes)
[  183.939803] 
[  183.939803] testing speed of async cbc(twofish) encryption
[  183.953902] test 0 (128 bit key, 16 byte blocks): 5195403 operations in 1 
seconds (83126448 bytes)
[  184.962487] test 1 (128 bit key, 64 byte blocks): 1912010 operations in 1 
seconds (122368640 bytes)
[  185.969150] test 2 (128 bit key, 256 byte blocks): 540125 operations in 1 
seconds (138272000 bytes)
[  186.975650] test 3 (128 bit key, 1024 byte blocks): 140631 operations in 1 
seconds (144006144 bytes)
[  187.982411] test 4 (128 bit key, 8192 byte blocks): 17737 operations in 1 
seconds (145301504 bytes)
[  188.988782] test 5 (192 bit key, 16 byte blocks): 5182287 operations in 1 
seconds (82916592 bytes)
[  189.995435] test 6 (192 bit key, 64 byte blocks): 1912356 operations in 1 
seconds (122390784 bytes)
[  191.002093] test 7 (192 bit key, 256 byte blocks): 540991 operations in 1 
seconds (138493696 bytes)
[  192.008600] test 8 (192 bit key, 1024 byte blocks): 140791 operations in 1 
seconds (144169984 bytes)
[  193.015197] test 9 (192 bit key, 8192 byte blocks): 17609 operations in 1 
seconds (144252928 bytes)
[  194.021740] test 10 (256 bit key, 16 byte blocks): 5191521 operations in 1 
seconds (83064336 bytes)
[  195.028534] test 11 (256 bit key, 64 byte blocks): 1906226 operations in 1 
seconds (121998464 bytes)
[  196.035069] test 12 (256 bit key, 256 byte blocks): 540479 operations in 1 
seconds (138362624 bytes)

Re: [PATCH] crypto: twofish - add x86_64/avx assembler implementation

[Borislav Petkov]

On Tue, Aug 28, 2012 at 12:17:43PM +0300, Jussi Kivilinna wrote:
 With this patch twofish-avx is faster than twofish-3way for 256, 1k
 and 8k tests.
 
 sizeold-vs-new  new-vs-3way old-vs-3way
 ecb-enc ecb-dec ecb-enc ecb-dec ecb-enc ecb-dec
 256 1.10x   1.11x   1.01x   1.01x   0.92x   0.91x
 1k  1.11x   1.12x   1.08x   1.07x   0.97x   0.96x
 8k  1.11x   1.13x   1.10x   1.08x   0.99x   0.97x

Not bad, that's 10ish percent improvement, after all.

Thanks.

-- 
Regards/Gruss,
Boris.
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: randconfig build error with next-20131002, in crypto

[Borislav Petkov]

On Wed, Oct 02, 2013 at 03:19:51PM -0700, Jim Davis wrote:
 Building with the attached random configuration file,
 
   LD  init/built-in.o
 crypto/built-in.o: In function `RSA_verify_signature':
 rsa.c:(.text+0x2dc5e): undefined reference to `mpi_get_nbits'
 rsa.c:(.text+0x2dc68): undefined reference to `mpi_get_nbits'
 rsa.c:(.text+0x2dc89): undefined reference to `mpi_free'
 rsa.c:(.text+0x2dca4): undefined reference to `mpi_cmp_ui'
 rsa.c:(.text+0x2dcb7): undefined reference to `mpi_cmp'
 rsa.c:(.text+0x2dcc6): undefined reference to `mpi_alloc'
 rsa.c:(.text+0x2dce7): undefined reference to `mpi_powm'
 rsa.c:(.text+0x2dcff): undefined reference to `mpi_get_nbits'
 rsa.c:(.text+0x2dd1c): undefined reference to `mpi_get_buffer'
 rsa.c:(.text+0x2de07): undefined reference to `mpi_free'
 make: *** [vmlinux] Error 1

Looks like someone has forgotten this completely unused MPILIB_EXTRA
thing in there.

Does this totally untested but obvious patch help?

---
From: Borislav Petkov b...@suse.de
Date: Thu, 3 Oct 2013 01:51:15 +0200
Subject: [PATCH] crypto: Correct RSA MPI dependency

9e235dcaf4f6 (Revert crypto: GnuPG based MPI lib - additional sources
(part 4)) removed the MPI lib extra stuff but left RSA selecting it
while it should select CONFIG_MPILIB instead. Fix it.

Reported-by: Jim Davis jim.ep...@gmail.com
Cc: Herbert Xu herb...@gondor.apana.org.au
Cc: David S. Miller da...@davemloft.net
Cc: David Howells dhowe...@redhat.com
Signed-off-by: Borislav Petkov b...@suse.de
---
 crypto/asymmetric_keys/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
index 6d2c2ea12559..755f6174585a 100644
--- a/crypto/asymmetric_keys/Kconfig
+++ b/crypto/asymmetric_keys/Kconfig
@@ -21,7 +21,7 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
 config PUBLIC_KEY_ALGO_RSA
tristate RSA public-key algorithm
depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
-   select MPILIB_EXTRA
+   select MPILIB
help
  This option enables support for the RSA algorithm (PKCS#1, RFC3447).
 
-- 
1.8.4

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Crypto Update for 3.13

[Borislav Petkov]

On Wed, Nov 13, 2013 at 12:41:52AM +0800, Herbert Xu wrote:
 Hi Linus:
 
 Here is the crypto update for 3.13:
 
 * Made x86 ablk_helper generic for ARM.
 * Phase out chainiv in favour of eseqiv (affects IPsec).
 * Fixed aes-cbc IV corruption on s390.
 * Added constant-time crypto_memneq which replaces memcmp.
 
 * Fixed aes-ctr in omap-aes.
 * Added OMAP3 ROM RNG support.
 * Add PRNG support for MSM SoC's
 * Add and use Job Ring API in caam.
 
 * Misc fixes.

Maybe add this one to that:

http://marc.info/?l=linux-kernelm=138078878205385w=2

?

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: randconfig build error with next-20131223, in arch/x86/crypto/aesni-intel_avx.S

[Borislav Petkov]

On Mon, Dec 23, 2013 at 08:09:18AM -0700, Jim Davis wrote:
 Building with the attached random configuration file,
 
 arch/x86/crypto/aesni-intel_avx.S: Assembler messages:
 arch/x86/crypto/aesni-intel_avx.S:1449: Error: bad register name `%r12'
 [...]
 arch/x86/crypto/aesni-intel_avx.S:2807: Error: bad register name `%r12'
 make[2]: *** [arch/x86/crypto/aesni-intel_avx.o] Error 1

 #
 # Automatically generated file; DO NOT EDIT.
 # Linux/x86 3.13.0-rc5 Kernel Configuration
 #
 # CONFIG_64BIT is not set
 CONFIG_X86_32=y

Sounds like this aesni-intel_avx.S thing wasn't meant to be built for
32-bit.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] crypto: aesni - fix build on x86 (32bit)

[Borislav Petkov]

On Mon, Jan 06, 2014 at 10:10:55AM -0800, Tim Chen wrote:
 Yes, the code is in the file named aesni_intel_avx.S. So it should be
 clear that the code is meant for x86_64.

How do you deduce aesni_intel_avx.S is meant for x86_64 only from the
name?

Shouldn't it be called aesni_intel_avx-x86_64.S, as is the naming
convention in arch/x86/crypto/

?


-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/28] Remove MPILIB_EXTRA

[Borislav Petkov]

On Tue, Apr 15, 2014 at 11:06:34AM +0200, Paul Bolle wrote:
 On Sun, 2014-02-09 at 21:18 +0100, Paul Bolle wrote:
  On Sun, 2014-02-09 at 19:47 +0100, Richard Weinberger wrote:
   The symbol is an orphan, get rid of it.
   
   Signed-off-by: Richard Weinberger rich...@nod.at
  
  Acked-by: Paul Bolle pebo...@tiscali.nl
  
   ---
crypto/asymmetric_keys/Kconfig | 1 -
1 file changed, 1 deletion(-)
   
   diff --git a/crypto/asymmetric_keys/Kconfig 
   b/crypto/asymmetric_keys/Kconfig
   index 03a6eb9..0320c7d 100644
   --- a/crypto/asymmetric_keys/Kconfig
   +++ b/crypto/asymmetric_keys/Kconfig
   @@ -22,7 +22,6 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE

config PUBLIC_KEY_ALGO_RSA
 tristate RSA public-key algorithm
   - select MPILIB_EXTRA
 select MPILIB
 help
   This option enables support for the RSA algorithm (PKCS#1, RFC3447).
  
  I tried this a year ago (see https://lkml.org/lkml/2013/3/8/142 ).
 
 Did anyone else ever find some time to look at this oneliner?

Last time I asked about that part of the tree, Herbert said it should go
through James Morris' tree. CCed.

http://lkml.kernel.org/r/20131112182746.ga22...@gondor.apana.org.au

Unless akpm picks it up first. Also CCed.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/28] Remove MPILIB_EXTRA

[Borislav Petkov]

On Wed, Jun 18, 2014 at 10:09:54AM +0200, Paul Bolle wrote:
 This oneliner is neither part of v3.16-rc1 nor part of linux-next. It
 applies cleanly to next-20140618. Should I hope that Jiri Kosina wants
 to take it for trivial (CC-ed)?

Yeah, he will.

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] x86, crypto: Check if gas supports CRC32

[Borislav Petkov]

Building current kernel with some old toolchain (gcc 4.1.2 and gas 2.17)
chokes with:

arch/x86/crypto/crc32c-pcl-intel-asm_64.S: Assembler messages:
arch/x86/crypto/crc32c-pcl-intel-asm_64.S:128: Error: no such instruction: 
`crc32b %bl,%r8d'
arch/x86/crypto/crc32c-pcl-intel-asm_64.S:204: Error: no such instruction: 
`crc32q -i*8(%rcx),%r8'
...

due to the fact that gas doesn't know the CRC32 instruction. Check that
before building.

Cc: linux-crypto@vger.kernel.org
Signed-off-by: Borislav Petkov b...@suse.de
---

So this is one possibility to address this. Code already
has the ifdeffery around crc_pcl() which is implemented in
crc32c-pcl-intel-asm_64.S so we can piggyback on that and not build that
file if gas doesn't know CRC32.

If no CRC32 support, it still builds fine silently, however it would be
better to probably say that due to old toolchain, kernel doesn't include
fast CRC32 stuff in crc32c-intel.ko.

Hmmm.


 arch/x86/crypto/Makefile|  8 +++-
 arch/x86/crypto/crc32c-intel_glue.c |  7 ---
 scripts/gas-can-do-crc32.sh | 12 
 3 files changed, 23 insertions(+), 4 deletions(-)
 create mode 100755 scripts/gas-can-do-crc32.sh

diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
index 61d6e281898b..707bf7ecb903 100644
--- a/arch/x86/crypto/Makefile
+++ b/arch/x86/crypto/Makefile
@@ -83,7 +83,13 @@ ifeq ($(avx2_supported),yes)
 sha1-ssse3-y += sha1_avx2_x86_64_asm.o
 endif
 crc32c-intel-y := crc32c-intel_glue.o
-crc32c-intel-$(CONFIG_64BIT) += crc32c-pcl-intel-asm_64.o
+
+gas_can_crc32 := $(srctree)/scripts/gas-can-do-crc32.sh
+CONFIG_GAS_SUPPORTS_CRC32=$(shell $(CONFIG_SHELL) $(gas_can_crc32) $(AS))
+
+ifdef CONFIG_64BIT
+crc32c-intel-$(CONFIG_GAS_SUPPORTS_CRC32) += crc32c-pcl-intel-asm_64.o
+endif
 crc32-pclmul-y := crc32-pclmul_asm.o crc32-pclmul_glue.o
 sha256-ssse3-y := sha256-ssse3-asm.o sha256-avx-asm.o sha256-avx2-asm.o 
sha256_ssse3_glue.o
 sha512-ssse3-y := sha512-ssse3-asm.o sha512-avx-asm.o sha512-avx2-asm.o 
sha512_ssse3_glue.o
diff --git a/arch/x86/crypto/crc32c-intel_glue.c 
b/arch/x86/crypto/crc32c-intel_glue.c
index 6812ad98355c..4ce8e2db2984 100644
--- a/arch/x86/crypto/crc32c-intel_glue.c
+++ b/arch/x86/crypto/crc32c-intel_glue.c
@@ -46,7 +46,7 @@
 #define REX_PRE
 #endif
 
-#ifdef CONFIG_X86_64
+#if defined(CONFIG_X86_64)  defined(CONFIG_GAS_SUPPORTS_CRC32)
 /*
  * use carryless multiply version of crc32c when buffer
  * size is = 512 (when eager fpu is enabled) or
@@ -181,7 +181,7 @@ static int crc32c_intel_cra_init(struct crypto_tfm *tfm)
return 0;
 }
 
-#ifdef CONFIG_X86_64
+#if defined(CONFIG_X86_64)  defined(CONFIG_GAS_SUPPORTS_CRC32)
 static int crc32c_pcl_intel_update(struct shash_desc *desc, const u8 *data,
   unsigned int len)
 {
@@ -257,7 +257,8 @@ static int __init crc32c_intel_mod_init(void)
 {
if (!x86_match_cpu(crc32c_cpu_id))
return -ENODEV;
-#ifdef CONFIG_X86_64
+
+#if defined(CONFIG_X86_64)  defined(CONFIG_GAS_SUPPORTS_CRC32)
if (cpu_has_pclmulqdq) {
alg.update = crc32c_pcl_intel_update;
alg.finup = crc32c_pcl_intel_finup;
diff --git a/scripts/gas-can-do-crc32.sh b/scripts/gas-can-do-crc32.sh
new file mode 100755
index ..88ff476bd3cc
--- /dev/null
+++ b/scripts/gas-can-do-crc32.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+TMP=$(mktemp)
+
+echo crc32 %rax,%rbx | $* --warn - -o $TMP 2/dev/null
+if [ $? -eq 0 ]; then
+   echo y
+else
+   echo n
+fi
+
+rm -f $TMP
-- 
2.0.0


-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] x86, crypto: Check if gas supports CRC32

[Borislav Petkov]

On Wed, Jul 30, 2014 at 11:28:14PM +0200, Mathias Krause wrote:
 Gah, CONFIG_AS_CRC32 gets defined as a preprocessor symbol only so
 cannot be used in makefiles. So crc32c-pcl-intel-asm_64.S needs a
 #ifdef CONFIG_AS_CRC32 guard and still be compiled for CONFIG_64BIT,
 as it is now. It'll be an empty object for older binutils versions not
 supporting the crc32 instruction.

Yeah, that makes it all even simpler, thanks!

We're still b0rked though:

arch/x86/crypto/crct10dif-pcl-asm_64.S: Assembler messages:
arch/x86/crypto/crct10dif-pcl-asm_64.S:147: Error: no such instruction: 
`pclmulqdq $0x0,%xmm10,%xmm0'
arch/x86/crypto/crct10dif-pcl-asm_64.S:148: Error: no such instruction: 
`pclmulqdq $0x11,%xmm10,%xmm8'
arch/x86/crypto/crct10dif-pcl-asm_64.S:149: Error: no such instruction: 
`pclmulqdq $0x0,%xmm10,%xmm1'
...

and need checking for more instructions. I'll play with this more
tomorrow.

Good night :-)

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
To unsubscribe from this list: send the line unsubscribe linux-crypto in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/10] x86/cpufeature: Kill cpu_has_osxsave

[Borislav Petkov]

From: Borislav Petkov <b...@suse.de>

Use boot_cpu_has() instead.

Signed-off-by: Borislav Petkov <b...@suse.de>
Cc: linux-crypto@vger.kernel.org
---
 arch/x86/crypto/camellia_aesni_avx2_glue.c | 3 ++-
 arch/x86/crypto/camellia_aesni_avx_glue.c  | 2 +-
 arch/x86/crypto/serpent_avx2_glue.c| 2 +-
 arch/x86/include/asm/cpufeature.h  | 1 -
 arch/x86/include/asm/xor_avx.h | 4 ++--
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c 
b/arch/x86/crypto/camellia_aesni_avx2_glue.c
index d84456924563..c37f7028c85a 100644
--- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
@@ -562,7 +562,8 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx2 || !cpu_has_avx || !cpu_has_aes || !cpu_has_osxsave) {
+   if (!cpu_has_avx2 || !cpu_has_avx || !cpu_has_aes ||
+   !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
return -ENODEV;
}
diff --git a/arch/x86/crypto/camellia_aesni_avx_glue.c 
b/arch/x86/crypto/camellia_aesni_avx_glue.c
index 93d8f295784e..65f64556725b 100644
--- a/arch/x86/crypto/camellia_aesni_avx_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx_glue.c
@@ -554,7 +554,7 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx || !cpu_has_aes || !cpu_has_osxsave) {
+   if (!cpu_has_avx || !cpu_has_aes || !boot_cpu_has(X86_FEATURE_OSXSAVE)) 
{
pr_info("AVX or AES-NI instructions are not detected.\n");
return -ENODEV;
}
diff --git a/arch/x86/crypto/serpent_avx2_glue.c 
b/arch/x86/crypto/serpent_avx2_glue.c
index 6d198342e2de..408cae2b3543 100644
--- a/arch/x86/crypto/serpent_avx2_glue.c
+++ b/arch/x86/crypto/serpent_avx2_glue.c
@@ -538,7 +538,7 @@ static int __init init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx2 || !cpu_has_osxsave) {
+   if (!cpu_has_avx2 || !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX2 instructions are not detected.\n");
return -ENODEV;
}
diff --git a/arch/x86/include/asm/cpufeature.h 
b/arch/x86/include/asm/cpufeature.h
index 3aea54ecabfd..33c29aabc9aa 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -135,7 +135,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 #define cpu_has_x2apic boot_cpu_has(X86_FEATURE_X2APIC)
 #define cpu_has_xsave  boot_cpu_has(X86_FEATURE_XSAVE)
 #define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES)
-#define cpu_has_osxsaveboot_cpu_has(X86_FEATURE_OSXSAVE)
 /*
  * Do not add any more of those clumsy macros - use static_cpu_has() for
  * fast paths and boot_cpu_has() otherwise!
diff --git a/arch/x86/include/asm/xor_avx.h b/arch/x86/include/asm/xor_avx.h
index 7c0a517ec751..e45e556140af 100644
--- a/arch/x86/include/asm/xor_avx.h
+++ b/arch/x86/include/asm/xor_avx.h
@@ -167,12 +167,12 @@ static struct xor_block_template xor_block_avx = {
 
 #define AVX_XOR_SPEED \
 do { \
-   if (cpu_has_avx && cpu_has_osxsave) \
+   if (cpu_has_avx && boot_cpu_has(X86_FEATURE_OSXSAVE)) \
xor_speed(_block_avx); \
 } while (0)
 
 #define AVX_SELECT(FASTEST) \
-   (cpu_has_avx && cpu_has_osxsave ? _block_avx : FASTEST)
+   (cpu_has_avx && boot_cpu_has(X86_FEATURE_OSXSAVE) ? _block_avx : 
FASTEST)
 
 #else
 
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/10] x86/cpufeature: Remove cpu_has_avx

[Borislav Petkov]

From: Borislav Petkov <b...@suse.de>

Signed-off-by: Borislav Petkov <b...@suse.de>
Cc: linux-crypto@vger.kernel.org
---
 arch/x86/crypto/aesni-intel_glue.c | 2 +-
 arch/x86/crypto/camellia_aesni_avx2_glue.c | 3 ++-
 arch/x86/crypto/camellia_aesni_avx_glue.c  | 2 +-
 arch/x86/crypto/chacha20_glue.c| 3 ++-
 arch/x86/crypto/poly1305_glue.c| 3 ++-
 arch/x86/crypto/sha1_ssse3_glue.c  | 2 +-
 arch/x86/crypto/sha256_ssse3_glue.c| 2 +-
 arch/x86/crypto/sha512_ssse3_glue.c| 2 +-
 arch/x86/include/asm/cpufeature.h  | 1 -
 arch/x86/include/asm/xor_avx.h | 4 ++--
 10 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c 
b/arch/x86/crypto/aesni-intel_glue.c
index 064c7e2bd7c8..5b7fa1471007 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1477,7 +1477,7 @@ static int __init aesni_init(void)
}
aesni_ctr_enc_tfm = aesni_ctr_enc;
 #ifdef CONFIG_AS_AVX
-   if (cpu_has_avx) {
+   if (boot_cpu_has(X86_FEATURE_AVX)) {
/* optimize performance of ctr mode encryption transform */
aesni_ctr_enc_tfm = aesni_ctr_enc_avx_tfm;
pr_info("AES CTR mode by8 optimization enabled\n");
diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c 
b/arch/x86/crypto/camellia_aesni_avx2_glue.c
index c07f699826a0..60907c139c4e 100644
--- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
@@ -562,7 +562,8 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!boot_cpu_has(X86_FEATURE_AVX2) || !cpu_has_avx ||
+   if (!boot_cpu_has(X86_FEATURE_AVX) ||
+   !boot_cpu_has(X86_FEATURE_AVX2) ||
!boot_cpu_has(X86_FEATURE_AES) ||
!boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
diff --git a/arch/x86/crypto/camellia_aesni_avx_glue.c 
b/arch/x86/crypto/camellia_aesni_avx_glue.c
index 6d256d59c5fd..d96429da88eb 100644
--- a/arch/x86/crypto/camellia_aesni_avx_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx_glue.c
@@ -554,7 +554,7 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx ||
+   if (!boot_cpu_has(X86_FEATURE_AVX) ||
!boot_cpu_has(X86_FEATURE_AES) ||
!boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX or AES-NI instructions are not detected.\n");
diff --git a/arch/x86/crypto/chacha20_glue.c b/arch/x86/crypto/chacha20_glue.c
index cea061e137da..2d5c2e0bd939 100644
--- a/arch/x86/crypto/chacha20_glue.c
+++ b/arch/x86/crypto/chacha20_glue.c
@@ -129,7 +129,8 @@ static int __init chacha20_simd_mod_init(void)
return -ENODEV;
 
 #ifdef CONFIG_AS_AVX2
-   chacha20_use_avx2 = cpu_has_avx && boot_cpu_has(X86_FEATURE_AVX2) &&
+   chacha20_use_avx2 = boot_cpu_has(X86_FEATURE_AVX) &&
+   boot_cpu_has(X86_FEATURE_AVX2) &&
cpu_has_xfeatures(XFEATURE_MASK_SSE | 
XFEATURE_MASK_YMM, NULL);
 #endif
return crypto_register_alg();
diff --git a/arch/x86/crypto/poly1305_glue.c b/arch/x86/crypto/poly1305_glue.c
index ea21d2e440f7..e32142bc071d 100644
--- a/arch/x86/crypto/poly1305_glue.c
+++ b/arch/x86/crypto/poly1305_glue.c
@@ -183,7 +183,8 @@ static int __init poly1305_simd_mod_init(void)
return -ENODEV;
 
 #ifdef CONFIG_AS_AVX2
-   poly1305_use_avx2 = cpu_has_avx && boot_cpu_has(X86_FEATURE_AVX2) &&
+   poly1305_use_avx2 = boot_cpu_has(X86_FEATURE_AVX) &&
+   boot_cpu_has(X86_FEATURE_AVX2) &&
cpu_has_xfeatures(XFEATURE_MASK_SSE | 
XFEATURE_MASK_YMM, NULL);
alg.descsize = sizeof(struct poly1305_simd_desc_ctx);
if (poly1305_use_avx2)
diff --git a/arch/x86/crypto/sha1_ssse3_glue.c 
b/arch/x86/crypto/sha1_ssse3_glue.c
index dd14616b7739..1024e378a358 100644
--- a/arch/x86/crypto/sha1_ssse3_glue.c
+++ b/arch/x86/crypto/sha1_ssse3_glue.c
@@ -166,7 +166,7 @@ static struct shash_alg sha1_avx_alg = {
 static bool avx_usable(void)
 {
if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL)) {
-   if (cpu_has_avx)
+   if (boot_cpu_has(X86_FEATURE_AVX))
pr_info("AVX detected but unusable.\n");
return false;
}
diff --git a/arch/x86/crypto/sha256_ssse3_glue.c 
b/arch/x86/crypto/sha256_ssse3_glue.c
index 5f4d6086dc59..3ae0f43ebd37 100644
--- a/arch/x86/crypto/sha256_ssse3_glue.c
+++ b/arch/x86/crypto/sha256_ssse3_glue.c
@@ -201,7 +201,7 @@ static struct shash_alg sha256_avx_algs[] = { {
 static bool avx_usable(void)
 {
if (!cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL)) {

[PATCH 02/10] x86/cpufeature: Remove cpu_has_aes

[Borislav Petkov]

From: Borislav Petkov <b...@suse.de>

Signed-off-by: Borislav Petkov <b...@suse.de>
Cc: linux-crypto@vger.kernel.org
---
 arch/x86/crypto/camellia_aesni_avx2_glue.c | 3 ++-
 arch/x86/crypto/camellia_aesni_avx_glue.c  | 4 +++-
 arch/x86/include/asm/cpufeature.h  | 1 -
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c 
b/arch/x86/crypto/camellia_aesni_avx2_glue.c
index 39389662e29b..c07f699826a0 100644
--- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
@@ -562,7 +562,8 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!boot_cpu_has(X86_FEATURE_AVX2) || !cpu_has_avx || !cpu_has_aes ||
+   if (!boot_cpu_has(X86_FEATURE_AVX2) || !cpu_has_avx ||
+   !boot_cpu_has(X86_FEATURE_AES) ||
!boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
return -ENODEV;
diff --git a/arch/x86/crypto/camellia_aesni_avx_glue.c 
b/arch/x86/crypto/camellia_aesni_avx_glue.c
index 65f64556725b..6d256d59c5fd 100644
--- a/arch/x86/crypto/camellia_aesni_avx_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx_glue.c
@@ -554,7 +554,9 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx || !cpu_has_aes || !boot_cpu_has(X86_FEATURE_OSXSAVE)) 
{
+   if (!cpu_has_avx ||
+   !boot_cpu_has(X86_FEATURE_AES) ||
+   !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX or AES-NI instructions are not detected.\n");
return -ENODEV;
}
diff --git a/arch/x86/include/asm/cpufeature.h 
b/arch/x86/include/asm/cpufeature.h
index 810166530cbf..a6627b30bf45 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -123,7 +123,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 #define cpu_has_apic   boot_cpu_has(X86_FEATURE_APIC)
 #define cpu_has_fxsr   boot_cpu_has(X86_FEATURE_FXSR)
 #define cpu_has_xmmboot_cpu_has(X86_FEATURE_XMM)
-#define cpu_has_aesboot_cpu_has(X86_FEATURE_AES)
 #define cpu_has_avxboot_cpu_has(X86_FEATURE_AVX)
 #define cpu_has_xsave  boot_cpu_has(X86_FEATURE_XSAVE)
 #define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES)
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/10] x86/cpufeature: Remove cpu_has_avx2

[Borislav Petkov]

From: Borislav Petkov <b...@suse.de>

Signed-off-by: Borislav Petkov <b...@suse.de>
Cc: linux-crypto@vger.kernel.org
---
 arch/x86/crypto/camellia_aesni_avx2_glue.c | 2 +-
 arch/x86/crypto/chacha20_glue.c| 2 +-
 arch/x86/crypto/poly1305_glue.c| 2 +-
 arch/x86/crypto/serpent_avx2_glue.c| 2 +-
 arch/x86/include/asm/cpufeature.h  | 1 -
 5 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/crypto/camellia_aesni_avx2_glue.c 
b/arch/x86/crypto/camellia_aesni_avx2_glue.c
index c37f7028c85a..39389662e29b 100644
--- a/arch/x86/crypto/camellia_aesni_avx2_glue.c
+++ b/arch/x86/crypto/camellia_aesni_avx2_glue.c
@@ -562,7 +562,7 @@ static int __init camellia_aesni_init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx2 || !cpu_has_avx || !cpu_has_aes ||
+   if (!boot_cpu_has(X86_FEATURE_AVX2) || !cpu_has_avx || !cpu_has_aes ||
!boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX2 or AES-NI instructions are not detected.\n");
return -ENODEV;
diff --git a/arch/x86/crypto/chacha20_glue.c b/arch/x86/crypto/chacha20_glue.c
index 8baaff5af0b5..cea061e137da 100644
--- a/arch/x86/crypto/chacha20_glue.c
+++ b/arch/x86/crypto/chacha20_glue.c
@@ -129,7 +129,7 @@ static int __init chacha20_simd_mod_init(void)
return -ENODEV;
 
 #ifdef CONFIG_AS_AVX2
-   chacha20_use_avx2 = cpu_has_avx && cpu_has_avx2 &&
+   chacha20_use_avx2 = cpu_has_avx && boot_cpu_has(X86_FEATURE_AVX2) &&
cpu_has_xfeatures(XFEATURE_MASK_SSE | 
XFEATURE_MASK_YMM, NULL);
 #endif
return crypto_register_alg();
diff --git a/arch/x86/crypto/poly1305_glue.c b/arch/x86/crypto/poly1305_glue.c
index b283868acdf8..ea21d2e440f7 100644
--- a/arch/x86/crypto/poly1305_glue.c
+++ b/arch/x86/crypto/poly1305_glue.c
@@ -183,7 +183,7 @@ static int __init poly1305_simd_mod_init(void)
return -ENODEV;
 
 #ifdef CONFIG_AS_AVX2
-   poly1305_use_avx2 = cpu_has_avx && cpu_has_avx2 &&
+   poly1305_use_avx2 = cpu_has_avx && boot_cpu_has(X86_FEATURE_AVX2) &&
cpu_has_xfeatures(XFEATURE_MASK_SSE | 
XFEATURE_MASK_YMM, NULL);
alg.descsize = sizeof(struct poly1305_simd_desc_ctx);
if (poly1305_use_avx2)
diff --git a/arch/x86/crypto/serpent_avx2_glue.c 
b/arch/x86/crypto/serpent_avx2_glue.c
index 408cae2b3543..870f6d812a2d 100644
--- a/arch/x86/crypto/serpent_avx2_glue.c
+++ b/arch/x86/crypto/serpent_avx2_glue.c
@@ -538,7 +538,7 @@ static int __init init(void)
 {
const char *feature_name;
 
-   if (!cpu_has_avx2 || !boot_cpu_has(X86_FEATURE_OSXSAVE)) {
+   if (!boot_cpu_has(X86_FEATURE_AVX2) || 
!boot_cpu_has(X86_FEATURE_OSXSAVE)) {
pr_info("AVX2 instructions are not detected.\n");
return -ENODEV;
}
diff --git a/arch/x86/include/asm/cpufeature.h 
b/arch/x86/include/asm/cpufeature.h
index c594e04bf529..810166530cbf 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -125,7 +125,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
 #define cpu_has_xmmboot_cpu_has(X86_FEATURE_XMM)
 #define cpu_has_aesboot_cpu_has(X86_FEATURE_AES)
 #define cpu_has_avxboot_cpu_has(X86_FEATURE_AVX)
-#define cpu_has_avx2   boot_cpu_has(X86_FEATURE_AVX2)
 #define cpu_has_xsave  boot_cpu_has(X86_FEATURE_XSAVE)
 #define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES)
 /*
-- 
2.7.3

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 01/28] kvm: svm: Add support for additional SVM NPF error codes

[Borislav Petkov]

On Mon, Aug 22, 2016 at 07:23:44PM -0400, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lenda...@amd.com>
> 
> AMD hardware adds two additional bits to aid in nested page fault handling.
> 
> Bit 32 - NPF occurred while translating the guest's final physical address
> Bit 33 - NPF occurred while translating the guest page tables
> 
> The guest page tables fault indicator can be used as an aid for nested
> virtualization. Using V0 for the host, V1 for the first level guest and
> V2 for the second level guest, when both V1 and V2 are using nested paging
> there are currently a number of unnecessary instruction emulations. When
> V2 is launched shadow paging is used in V1 for the nested tables of V2. As
> a result, KVM marks these pages as RO in the host nested page tables. When
> V2 exits and we resume V1, these pages are still marked RO.
> 
> Every nested walk for a guest page table is treated as a user-level write
> access and this causes a lot of NPFs because the V1 page tables are marked
> RO in the V0 nested tables. While executing V1, when these NPFs occur KVM
> sees a write to a read-only page, emulates the V1 instruction and unprotects
> the page (marking it RW). This patch looks for cases where we get a NPF due
> to a guest page table walk where the page was marked RO. It immediately
> unprotects the page and resumes the guest, leading to far fewer instruction
> emulations when nested virtualization is used.
> 
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/include/asm/kvm_host.h |   11 ++-
>  arch/x86/kvm/mmu.c  |   20 ++--
>  arch/x86/kvm/svm.c      |2 +-
>  3 files changed, 29 insertions(+), 4 deletions(-)

FWIW: Reviewed-by: Borislav Petkov <b...@suse.de>

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
--
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 02/28] kvm: svm: Add kvm_fast_pio_in support

[Borislav Petkov]

On Mon, Aug 22, 2016 at 07:23:54PM -0400, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lenda...@amd.com>
> 
> Update the I/O interception support to add the kvm_fast_pio_in function
> to speed up the in instruction similar to the out instruction.
> 
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/include/asm/kvm_host.h |1 +
>  arch/x86/kvm/svm.c  |5 +++--
>  arch/x86/kvm/x86.c  |   43 
> +++
>  3 files changed, 47 insertions(+), 2 deletions(-)

FWIW: Reviewed-by: Borislav Petkov <b...@suse.de>

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 03/28] kvm: svm: Use the hardware provided GPA instead of page walk

[Borislav Petkov]

On Mon, Aug 22, 2016 at 07:24:07PM -0400, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lenda...@amd.com>
> 
> When a guest causes a NPF which requires emulation, KVM sometimes walks
> the guest page tables to translate the GVA to a GPA. This is unnecessary
> most of the time on AMD hardware since the hardware provides the GPA in
> EXITINFO2.
> 
> The only exception cases involve string operations involving rep or
> operations that use two memory locations. With rep, the GPA will only be
> the value of the initial NPF and with dual memory locations we won't know
> which memory address was translated into EXITINFO2.
> 
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |3 +++
>  arch/x86/include/asm/kvm_host.h|3 +++
>  arch/x86/kvm/svm.c |2 ++
>  arch/x86/kvm/x86.c |   17 -
>  4 files changed, 24 insertions(+), 1 deletion(-)

FWIW, LGTM. (Gotta love replying in acronyms :-))

Reviewed-by: Borislav Petkov <b...@suse.de>

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 09/28] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Mon, Aug 22, 2016 at 07:25:25PM -0400, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> EFI data is encrypted when the kernel is run under SEV. Update the
> page table references to be sure the EFI memory areas are accessed
> encrypted.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/platform/efi/efi_64.c |   14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 0871ea4..98363f3 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -213,7 +213,7 @@ void efi_sync_low_kernel_mappings(void)
>  
>  int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
>  {
> - unsigned long pfn, text;
> + unsigned long pfn, text, flags;
>   efi_memory_desc_t *md;
>   struct page *page;
>   unsigned npages;
> @@ -230,6 +230,10 @@ int __init efi_setup_page_tables(unsigned long 
> pa_memmap, unsigned num_pages)
>   efi_scratch.efi_pgt = (pgd_t *)__sme_pa(efi_pgd);
>   pgd = efi_pgd;
>  
> + flags = _PAGE_NX | _PAGE_RW;
> + if (sev_active)
> + flags |= _PAGE_ENC;

So this is confusing me. There's this patch which says EFI data is
accessed in the clear:

https://lkml.kernel.org/r/2016083738.29880.6909.st...@tlendack-t1.amdoffice.net

but now here it is encrypted when SEV is enabled.

Do you mean, it is encrypted here because we're in the guest kernel?

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 09/28] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Thu, Sep 22, 2016 at 08:23:36PM +0200, Paolo Bonzini wrote:
> Unless this is part of some spec, it's easier if things are the same in
> SME and SEV.

Yeah, I was pondering over how sprinkling sev_active checks might not be
so clean.

I'm wondering if we could make the EFI regions presented to the guest
unencrypted too, as part of some SEV-specific init routine so that the
guest kernel doesn't need to do anything different.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 09/28] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Thu, Sep 22, 2016 at 04:45:51PM +0200, Paolo Bonzini wrote:
> The main difference between the SME and SEV encryption, from the point
> of view of the kernel, is that real-mode always writes unencrypted in
> SME and always writes encrypted in SEV.  But UEFI can run in 64-bit mode
> and learn about the C bit, so EFI boot data should be unprotected in SEV
> guests.

Actually, it is different: you can start fully encrypted in SME, see:

https://lkml.kernel.org/r/2016083539.29880.96739.st...@tlendack-t1.amdoffice.net

The last paragraph alludes to a certain transparent mode where you're
already encrypted and only certain pieces like EFI is not encrypted. I
think the aim is to have the transparent mode be the default one, which
makes most sense anyway.

The EFI regions are unencrypted for obvious reasons and you need to
access them as such.

> Because the firmware volume is written to high memory in encrypted
> form, and because the PEI phase runs in 32-bit mode, the firmware
> code will be encrypted; on the other hand, data that is placed in low
> memory for the kernel can be unencrypted, thus limiting differences
> between SME and SEV.

When you run fully encrypted, you still need to access EFI tables in the
clear. That's why I'm confused about this patch here.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 04/28] x86: Secure Encrypted Virtualization (SEV) support

[Borislav Petkov]

On Mon, Aug 22, 2016 at 07:24:19PM -0400, Brijesh Singh wrote:
> From: Tom Lendacky 

Subject: [RFC PATCH v1 04/28] x86: Secure Encrypted Virtualization (SEV) support

Please start patch commit heading with a verb, i.e.:

"x86: Add AMD Secure Encrypted Virtualization (SEV) support"

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 09/28] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Thu, Sep 22, 2016 at 02:49:22PM -0500, Tom Lendacky wrote:
> > I thought that reduction is the reservation of bits for the SME mask.
> > 
> > What other reduction is there?
> 
> There is a reduction in physical address space for the SME mask and the
> bits used to aid in identifying the ASID associated with the memory
> request. This allows for the memory controller to determine the key to
> be used for the encryption operation (host/hypervisor key vs. an SEV
> guest key).

Ok, I think I see what you mean: you call SME mask the bit in CPUID
Fn8000_001F[EBX][5:0], i.e., the C-bit, i.e. sme_me_mask. And the other
reduction is the key ASID, i.e., CPUID Fn8000_001F[EBX][11:6], i.e.
sme_me_loss.

I think we're on the same page - I was simply calling everything SME
mask because both are together in the PTE:

"Additionally, in some implementations, the physical address size of the
processor may be reduced when memory encryption features are enabled,
for example from 48 to 43 bits."

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 09/28] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Thu, Sep 22, 2016 at 05:05:54PM +0200, Paolo Bonzini wrote:
> Which paragraph?

"Linux relies on BIOS to set this bit if BIOS has determined that the
reduction in the physical address space as a result of enabling memory
encryption..."

Basically, you can enable SME in the BIOS and you're all set.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v1 09/28] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Thu, Sep 22, 2016 at 07:08:50PM +0200, Paolo Bonzini wrote:
> That's not how I read it.  I just figured that the BIOS has some magic
> things high in the physical address space and if you reduce the physical
> address space the BIOS (which is called from e.g. EFI runtime services)
> would have problems with that.

Yeah, I had to ask about that myself and Tom will have it explained
better in the next version.

The reduction in physical address space happens when SME enabled because
you need a couple of bits in the PTE with which to say which key has
encrypted that page. So it is an indelible part of the SME machinery.

Btw, section "7.10 Secure Memory Encryption" has an initial writeup:

http://support.amd.com/TechDocs/24593.pdf

Now that I skim over it, it doesn't mention the BIOS thing but that'll
be updated.

HTH.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH v2 15/32] x86: Add support for changing memory encryption attribute in early boot

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:15:28AM -0500, Brijesh Singh wrote:
> Some KVM-specific custom MSRs shares the guest physical address with
> hypervisor. When SEV is active, the shared physical address must be mapped
> with encryption attribute cleared so that both hypervsior and guest can
> access the data.
> 
> Add APIs to change memory encryption attribute in early boot code.
> 
> Signed-off-by: Brijesh Singh 
> ---
>  arch/x86/include/asm/mem_encrypt.h |   15 +
>  arch/x86/mm/mem_encrypt.c  |   63 
> 
>  2 files changed, 78 insertions(+)
> 
> diff --git a/arch/x86/include/asm/mem_encrypt.h 
> b/arch/x86/include/asm/mem_encrypt.h
> index 9799835..95bbe4c 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -47,6 +47,9 @@ void __init sme_unmap_bootdata(char *real_mode_data);
>  
>  void __init sme_early_init(void);
>  
> +int __init early_set_memory_decrypted(void *addr, unsigned long size);
> +int __init early_set_memory_encrypted(void *addr, unsigned long size);
> +
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void);
>  
> @@ -110,6 +113,18 @@ static inline void __init sme_early_init(void)
>  {
>  }
>  
> +static inline int __init early_set_memory_decrypted(void *addr,
> + unsigned long size)
> +{
> + return 1;


return 1 when !CONFIG_AMD_MEM_ENCRYPT ?

The non-early variants return 0.

> +}
> +
> +static inline int __init early_set_memory_encrypted(void *addr,
> + unsigned long size)
> +{
> + return 1;
> +}
> +
>  #define __sme_pa __pa
>  #define __sme_pa_nodebug __pa_nodebug
>  
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 7df5f4c..567e0d8 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -258,6 +259,68 @@ static void sme_free(struct device *dev, size_t size, 
> void *vaddr,
>   swiotlb_free_coherent(dev, size, vaddr, dma_handle);
>  }
>  
> +static unsigned long __init get_pte_flags(unsigned long address)
> +{
> + int level;
> + pte_t *pte;
> + unsigned long flags = _KERNPG_TABLE_NOENC | _PAGE_ENC;
> +
> + pte = lookup_address(address, );
> + if (!pte)
> + return flags;
> +
> + switch (level) {
> + case PG_LEVEL_4K:
> + flags = pte_flags(*pte);
> + break;
> + case PG_LEVEL_2M:
> + flags = pmd_flags(*(pmd_t *)pte);
> + break;
> + case PG_LEVEL_1G:
> + flags = pud_flags(*(pud_t *)pte);
> + break;
> + default:
> + break;
> + }
> +
> + return flags;
> +}
> +
> +int __init early_set_memory_enc_dec(void *vaddr, unsigned long size,
> + unsigned long flags)
> +{
> + unsigned long pfn, npages;
> + unsigned long addr = (unsigned long)vaddr & PAGE_MASK;
> +
> + /* We are going to change the physical page attribute from C=1 to C=0.
> +  * Flush the caches to ensure that all the data with C=1 is flushed to
> +  * memory. Any caching of the vaddr after function returns will
> +  * use C=0.
> +  */

Kernel comments style is:

/*
 * A sentence ending with a full-stop.
 * Another sentence. ...
 * More sentences. ...
 */

> + clflush_cache_range(vaddr, size);
> +
> + npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> + pfn = slow_virt_to_phys((void *)addr) >> PAGE_SHIFT;
> +
> + return kernel_map_pages_in_pgd(init_mm.pgd, pfn, addr, npages,
> + flags & ~sme_me_mask);
> +
> +}
> +
> +int __init early_set_memory_decrypted(void *vaddr, unsigned long size)
> +{
> + unsigned long flags = get_pte_flags((unsigned long)vaddr);

So this does lookup_address()...

> + return early_set_memory_enc_dec(vaddr, size, flags & ~sme_me_mask);

... and this does it too in slow_virt_to_phys(). So you do it twice per
vaddr.

So why don't you define a __slow_virt_to_phys() helper - notice
the "__" - which returns flags in its second parameter and which
slow_virt_to_phys() calls with a NULL second parameter in the other
cases?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 18/32] kvm: svm: Use the hardware provided GPA instead of page walk

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:16:05AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lenda...@amd.com>
> 
> When a guest causes a NPF which requires emulation, KVM sometimes walks
> the guest page tables to translate the GVA to a GPA. This is unnecessary
> most of the time on AMD hardware since the hardware provides the GPA in
> EXITINFO2.
> 
> The only exception cases involve string operations involving rep or
> operations that use two memory locations. With rep, the GPA will only be
> the value of the initial NPF and with dual memory locations we won't know
> which memory address was translated into EXITINFO2.
> 
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> Reviewed-by: Borislav Petkov <b...@suse.de>

I think I already asked you to remove Revewed-by tags when you have to
change an already reviewed patch in non-trivial manner. Why does this
one still have my Reviewed-by tag?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 16/32] x86: kvm: Provide support to create Guest and HV shared per-CPU variables

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:15:36AM -0500, Brijesh Singh wrote:
> Some KVM specific MSR's (steal-time, asyncpf, avic_eio) allocates per-CPU
> variable at compile time and share its physical address with hypervisor.
> It presents a challege when SEV is active in guest OS. When SEV is active,
> guest memory is encrypted with guest key and hypervisor will no longer able
> to modify the guest memory. When SEV is active, we need to clear the
> encryption attribute of shared physical addresses so that both guest and
> hypervisor can access the data.
> 
> To solve this problem, I have tried these three options:
> 
> 1) Convert the static per-CPU to dynamic per-CPU allocation. When SEV is
> detected then clear the encryption attribute. But while doing so I found
> that per-CPU dynamic allocator was not ready when kvm_guest_cpu_init was
> called.
> 
> 2) Since the encryption attributes works on PAGE_SIZE hence add some extra
> padding to 'struct kvm-steal-time' to make it PAGE_SIZE and then at runtime
> clear the encryption attribute of the full PAGE. The downside of this was
> now we need to modify structure which may break the compatibility.

>From SEV-ES whitepaper:

"To facilitate this communication, the SEV-ES architecture defines
a Guest Hypervisor Communication Block (GHCB). The GHCB resides in
page of shared memory so it is accessible to both the guest VM and the
hypervisor."

So this is kinda begging to be implemented with a shared page between
guest and host. And then put steal-time, ... etc in there too. Provided
there's enough room in the single page for the GHCB *and* our stuff.

> 
> 3) Define a new per-CPU section (.data..percpu.hv_shared) which will be
> used to hold the compile time shared per-CPU variables. When SEV is
> detected we map this section with encryption attribute cleared.
> 
> This patch implements #3. It introduces a new DEFINE_PER_CPU_HV_SHAHRED
> macro to create a compile time per-CPU variable. When SEV is detected we
> map the per-CPU variable as decrypted (i.e with encryption attribute cleared).
> 
> Signed-off-by: Brijesh Singh 
> ---
>  arch/x86/kernel/kvm.c |   43 
> +++--
>  include/asm-generic/vmlinux.lds.h |3 +++
>  include/linux/percpu-defs.h   |9 
>  3 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 099fcba..706a08e 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -75,8 +75,8 @@ static int parse_no_kvmclock_vsyscall(char *arg)
>  
>  early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall);
>  
> -static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64);
> -static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64);
> +static DEFINE_PER_CPU_HV_SHARED(struct kvm_vcpu_pv_apf_data, apf_reason) 
> __aligned(64);
> +static DEFINE_PER_CPU_HV_SHARED(struct kvm_steal_time, steal_time) 
> __aligned(64);
>  static int has_steal_clock = 0;
>  
>  /*
> @@ -290,6 +290,22 @@ static void __init paravirt_ops_setup(void)
>  #endif
>  }
>  
> +static int kvm_map_percpu_hv_shared(void *addr, unsigned long size)
> +{
> + /* When SEV is active, the percpu static variables initialized
> +  * in data section will contain the encrypted data so we first
> +  * need to decrypt it and then map it as decrypted.
> +  */

Kernel comments style is:

/*
 * A sentence ending with a full-stop.
 * Another sentence. ...
 * More sentences. ...
 */

But you get the idea. Please check your whole patchset for this.

> + if (sev_active()) {
> + unsigned long pa = slow_virt_to_phys(addr);
> +
> + sme_early_decrypt(pa, size);
> + return early_set_memory_decrypted(addr, size);
> + }
> +
> + return 0;
> +}
> +
>  static void kvm_register_steal_time(void)
>  {
>   int cpu = smp_processor_id();
> @@ -298,12 +314,17 @@ static void kvm_register_steal_time(void)
>   if (!has_steal_clock)
>   return;
>  
> + if (kvm_map_percpu_hv_shared(st, sizeof(*st))) {
> + pr_err("kvm-stealtime: failed to map hv_shared percpu\n");
> + return;
> + }
> +
>   wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
>   pr_info("kvm-stealtime: cpu %d, msr %llx\n",
>   cpu, (unsigned long long) slow_virt_to_phys(st));
>  }
>  
> -static DEFINE_PER_CPU(unsigned long, kvm_apic_eoi) = KVM_PV_EOI_DISABLED;
> +static DEFINE_PER_CPU_HV_SHARED(unsigned long, kvm_apic_eoi) = 
> KVM_PV_EOI_DISABLED;
>  
>  static notrace void kvm_guest_apic_eoi_write(u32 reg, u32 val)
>  {
> @@ -327,25 +348,33 @@ static void kvm_guest_cpu_init(void)
>   if (kvm_para_has_feature(KVM_FEATURE_ASYNC_PF) && kvmapf) {
>   u64 pa = slow_virt_to_phys(this_cpu_ptr(_reason));
>  
> + if (kvm_map_percpu_hv_shared(this_cpu_ptr(_reason),
> +

Re: [RFC PATCH v2 14/32] x86: mm: Provide support to use memblock when spliting large pages

[Borislav Petkov]

On Fri, Mar 10, 2017 at 04:41:56PM -0600, Brijesh Singh wrote:
> I can take a look at fixing those warning. In my initial attempt was to create
> a new function to clear encryption bit but it ended up looking very similar to
> __change_page_attr_set_clr() hence decided to extend the exiting function to
> use memblock_alloc().

... except that having all that SEV-specific code in main code paths is
yucky and I'd like to avoid it, if possible.

> Early in boot process, guest kernel allocates some structure (its either
> statically allocated or dynamic allocated via memblock_alloc). And shares the 
> physical
> address of these structure with hypervisor. Since entire guest memory area is 
> mapped
> as encrypted hence those structure's are mapped as encrypted memory range. We 
> need
> a method to clear the encryption bit. Sometime these structure maybe part of 
> 2M pages
> and need to split into smaller pages.

So how hard would it be if the hypervisor allocated that memory for the
guest instead? It would allocate it decrypted and guest would need to
access it decrypted too. All in preparation for SEV-ES which will need a
block of unencrypted memory for the guest anyway...

> In most cases, guest and hypervisor communication starts as soon as guest 
> provides
> the physical address to hypervisor. So we must map the pages as decrypted 
> before
> sharing the physical address to hypervisor.

See above: so purely theoretically speaking, the hypervisor could prep
that decrypted range for the guest. I'd look in Paolo's direction,
though, for the feasibility of something like that.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

[Borislav Petkov]

On Thu, Mar 16, 2017 at 11:11:26AM -0500, Tom Lendacky wrote:
> Not quite. The guest still needs to understand about the encryption mask
> so that it can protect memory by setting the encryption mask in the
> pagetable entries.  It can also decide when to share memory with the
> hypervisor by not setting the encryption mask in the pagetable entries.

Ok, so the kernel - by that I mean both the baremetal and guest kernel -
needs to know whether we're encrypting stuff. So it needs to know about
SME.

> "Instruction fetches are always decrypted under SEV" means that,
> regardless of how a virtual address is mapped, encrypted or decrypted,
> if an instruction fetch is performed by the CPU from that address it
> will always be decrypted. This is to prevent the hypervisor from
> injecting executable code into the guest since it would have to be
> valid encrypted instructions.

Ok, so the guest needs to map its pages encrypted.

Which reminds me, KSM might be a PITA to enable with SEV but that's a
different story. :)

> There are many areas that use the same logic, but there are certain
> situations where we need to check between SME vs SEV (e.g. DMA operation
> setup or decrypting the trampoline area) and act accordingly.

Right, and I'd like to keep those areas where it differs at minimum and
nicely cordoned off from the main paths.

So looking back at the current patch in this subthread:

we do check

* CPUID 0x4000
* 8000_001F[EAX] for SME
* 8000_001F[EBX][5:0] for the encryption bits.

So how about we generate the following CPUID picture for the guest:

CPUID_Fn801F_EAX = ...10b

That is, SME bit is cleared, SEV is set. This will mean for the guest
kernel that SEV is enabled and you can avoid yourself the 0x4000
leaf check and the additional KVM feature bit glue.

10b configuration will be invalid for baremetal as - I'm assuming - you
can't have SEV=1b with SME=0b. It will be a virt-only configuration and
this way you can even avoid the hypervisor-specific detection but do
that for all.

Hmmm?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

[Borislav Petkov]

On Thu, Mar 16, 2017 at 09:28:58AM -0500, Tom Lendacky wrote:
> Because there are differences between how SME and SEV behave
> (instruction fetches are always decrypted under SEV, DMA to an
> encrypted location is not supported under SEV, etc.) we need to
> determine which mode we are in so that things can be setup properly
> during boot. For example, if SEV is active the kernel will already
> be encrypted and so we don't perform that step or the trampoline area
> for bringing up an AP must be decrypted for SME but encrypted for SEV.

So with SEV enabled, it seems to me a guest doesn't know anything about
encryption and can run as if SME is disabled. So sme_active() will be
false. And then the kernel can bypass all that code dealing with SME.

So a guest should simply run like on baremetal with no SME, IMHO.

But then there's that part: "instruction fetches are always decrypted
under SEV". What does that mean exactly? And how much of that code can
be reused so that

* SME on baremetal
* SEV on guest

use the same logic?

Having the larger SEV preparation part on the kvm host side is perfectly
fine. But I'd like to keep kernel initialization paths clean.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

[Borislav Petkov]

On Fri, Mar 17, 2017 at 07:47:33PM +0100, Dmitry Vyukov wrote:
> This problem is more general and is not specific to clang. It equally
> applies to different versions of gcc, different arches and different
> configs (namely, anything else than what a developer used for
> testing).

I guess. We do carry a bunch of gcc workarounds along with the cc-*
macros in scripts/Kbuild.include.

> A known, reasonably well working solution to this problem is
> a system of try bots that test patches before commit with different
> compilers/configs/archs. We already have such system in the form of
> 0-day bots. It would be useful to extend it with clang as soon as
> kernel builds.

Has someone actually already talked to Fengguang about it?

Oh, and the stupid question: why the effort to build the kernel
with clang at all? Just because or are there some actual, palpable
advantages?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

[Borislav Petkov]

On Fri, Mar 10, 2017 at 10:35:30AM -0600, Brijesh Singh wrote:
> We could update this patch to use the below logic:
> 
>  * CPUID(0) - Check for AuthenticAMD
>  * CPID(1) - Check if under hypervisor
>  * CPUID(0x8000) - Check for highest supported leaf
>  * CPUID(0x801F).EAX - Check for SME and SEV support
>  * rdmsr (MSR_K8_SYSCFG)[MemEncryptionModeEnc] - Check if SMEE is set

Actually, it is still not clear to me *why* we need to do anything
special wrt SEV in the guest.

Lemme clarify: why can't the guest boot just like a normal Linux on
baremetal and use the SME(!) detection code to set sme_enable and so
on? IOW, I'd like to avoid all those checks whether we're running under
hypervisor and handle all that like we're running on baremetal.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

[Borislav Petkov]

On Thu, Mar 09, 2017 at 05:13:33PM +0100, Paolo Bonzini wrote:
> This is not how you check if running under a hypervisor; you should
> check the HYPERVISOR bit, i.e. bit 31 of cpuid(1).ecx.  This in turn
> tells you if leaf 0x4000 is valid.

Ah, good point, I already do that in the microcode loader :)

/*
 * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
 * completely accurate as xen pv guests don't see that CPUID bit set but
 * that's good enough as they don't land on the BSP path anyway.
 */
if (native_cpuid_ecx(1) & BIT(31))
return *res;

> That said, the main issue with this function is that it hardcodes the
> behavior for KVM.  It is possible that another hypervisor defines its
> 0x4001 leaf in such a way that KVM_FEATURE_SEV has a different meaning.
> 
> Instead, AMD should define a "well-known" bit in its own space (i.e.
> 0x80xx) that is only used by hypervisors that support SEV.  This is
> similar to how Intel defined one bit in leaf 1 to say "is leaf
> 0x4000 valid".
> 
> > +   if (eax > 0x4000) {
> > +   eax = 0x4001;
> > +   ecx = 0;
> > +   native_cpuid(, , , );
> > +   if (!(eax & BIT(KVM_FEATURE_SEV)))
> > +   goto out;
> > +
> > +   eax = 0x801f;
> > +   ecx = 0;
> > +   native_cpuid(, , , );
> > +   if (!(eax & 1))

Right, so this is testing CPUID_0x801f_ECX(0)[0], SME. Why not
simply set that bit for the guest too, in kvm?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 13/32] KVM: SVM: Enable SEV by setting the SEV_ENABLE CPU feature

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:15:01AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Modify the SVM cpuid update function to indicate if Secure Encrypted
> Virtualization (SEV) is active in the guest by setting the SEV KVM CPU
> features bit. SEV is active if Secure Memory Encryption is enabled in
> the host and the SEV_ENABLE bit of the VMCB is set.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/kvm/cpuid.c |4 +++-
>  arch/x86/kvm/svm.c   |   18 ++
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 1639de8..e0c40a8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -601,7 +601,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 
> *entry, u32 function,
>   entry->edx = 0;
>   break;
>   case 0x8000:
> - entry->eax = min(entry->eax, 0x801a);
> + entry->eax = min(entry->eax, 0x801f);
>   break;
>   case 0x8001:
>   entry->edx &= kvm_cpuid_8000_0001_edx_x86_features;
> @@ -634,6 +634,8 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 
> *entry, u32 function,
>   break;
>   case 0x801d:
>   break;
> + case 0x801f:
> + break;

I guess those three case's can be unified:

case 0x801a:
case 0x801d:
case 0x801f:
break;

...

> + sev_info = kvm_find_cpuid_entry(vcpu, 0x801f, 0);
> + if (!sev_info)
> + return;
> +
> + if (ca->nested_ctl & SVM_NESTED_CTL_SEV_ENABLE) {
> + features->eax |= (1 << KVM_FEATURE_SEV);
> + cpuid(0x801f, _info->eax, _info->ebx,
> +   _info->ecx, _info->edx);
> + }

Right, as already mentioned in the previous mail: can we communicate SEV
status to the guest solely through the 0x801f leaf? Then we won't
need KVM_FEATURE_SEV and this way we'll be hypervisor-agnostic, as Paolo
suggested.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 01/32] x86: Add the Secure Encrypted Virtualization CPU feature

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:12:09AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Update the CPU features to include identifying and reporting on the
> Secure Encrypted Virtualization (SEV) feature.  SME is identified by
> CPUID 0x801f, but requires BIOS support to enable it (set bit 23 of
> MSR_K8_SYSCFG and set bit 0 of MSR_K7_HWCR).  Only show the SEV feature
> as available if reported by CPUID and enabled by BIOS.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/cpufeatures.h |1 +
>  arch/x86/include/asm/msr-index.h   |2 ++
>  arch/x86/kernel/cpu/amd.c  |   22 ++
>  arch/x86/kernel/cpu/scattered.c|1 +
>  4 files changed, 22 insertions(+), 4 deletions(-)

So this patchset is not really ontop of Tom's patchset because this
patch doesn't apply. The reason is, Tom did the SME bit this way:

https://lkml.kernel.org/r/20170216154236.19244.7580.st...@tlendack-t1.amdoffice.net

but it should've been in scattered.c.

> diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
> index cabda87..c3f58d9 100644
> --- a/arch/x86/kernel/cpu/scattered.c
> +++ b/arch/x86/kernel/cpu/scattered.c
> @@ -31,6 +31,7 @@ static const struct cpuid_bit cpuid_bits[] = {
>   { X86_FEATURE_CPB,  CPUID_EDX,  9, 0x8007, 0 },
>   { X86_FEATURE_PROC_FEEDBACK,CPUID_EDX, 11, 0x8007, 0 },
>   { X86_FEATURE_SME,  CPUID_EAX,  0, 0x801f, 0 },
> + { X86_FEATURE_SEV,  CPUID_EAX,  1, 0x801f, 0 },
>   { 0, 0, 0, 0, 0 }

... and here it is in scattered.c, as it should be. So you've used an
older version of the patch, it seems.

Please sync with Tom to see whether he's reworked the v4 version of that
patch already. If yes, then you could send only the SME and SEV adding
patches as a reply to this message so that I can continue reviewing in
the meantime.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 00/32] x86: Secure Encrypted Virtualization (AMD)

[Borislav Petkov]

On Fri, Mar 03, 2017 at 02:33:23PM -0600, Bjorn Helgaas wrote:
> On Thu, Mar 02, 2017 at 10:12:01AM -0500, Brijesh Singh wrote:
> > This RFC series provides support for AMD's new Secure Encrypted 
> > Virtualization
> > (SEV) feature. This RFC is build upon Secure Memory Encryption (SME) RFCv4 
> > [1].
> 
> What kernel version is this series based on?

Yeah, see that mail in [1]:

https://lkml.kernel.org/r/20170216154158.19244.66630.st...@tlendack-t1.amdoffice.net

"This patch series is based off of the master branch of tip.
  Commit a27cb9e1b2b4 ("Merge branch 'WIP.sched/core'")"

$ git describe a27cb9e1b2b4
v4.10-rc7-681-ga27cb9e1b2b4

So you need the SME pile first and then that SVE pile. But the first
patch needs refreshing as it is using a different base than the SME
pile. :-)

Tom, Brijesh, perhaps you guys could push a full tree somewhere - github
or so - for people to pull, in addition to the patchset on lkml.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 01/32] x86: Add the Secure Encrypted Virtualization CPU feature

[Borislav Petkov]

On Fri, Mar 03, 2017 at 03:01:23PM -0600, Brijesh Singh wrote:
> +merely enables SME (sets bit 23 of the MSR_K8_SYSCFG), then Linux can
> activate
> +memory encryption by default (CONFIG_AMD_MEM_ENCRYPT_ACTIVE_BY_DEFAULT=y)
> or
> +by supplying mem_encrypt=on on the kernel command line.  However, if BIOS
> does
> +not enable SME, then Linux will not be able to activate memory encryption,
> even
> +if configured to do so by default or the mem_encrypt=on command line
> parameter
> +is specified.

This looks like a wraparound...

$ test-apply.sh /tmp/brijesh.singh.delta
checking file Documentation/admin-guide/kernel-parameters.txt
Hunk #1 succeeded at 2144 (offset -9 lines).
checking file Documentation/x86/amd-memory-encryption.txt
patch:  malformed patch at line 23: DRAM from physical

Yap.

Looks like exchange or your mail client decided to do some patch editing
on its own.

Please send it to yourself first and try applying.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 02/32] x86: Secure Encrypted Virtualization (SEV) support

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:12:20AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Provide support for Secure Encyrpted Virtualization (SEV). This initial
> support defines a flag that is used by the kernel to determine if it is
> running with SEV active.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/mem_encrypt.h |   14 +-
>  arch/x86/mm/mem_encrypt.c  |3 +++
>  include/linux/mem_encrypt.h|6 ++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/mem_encrypt.h 
> b/arch/x86/include/asm/mem_encrypt.h
> index 1fd5426..9799835 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -20,10 +20,16 @@
>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>  
>  extern unsigned long sme_me_mask;
> +extern unsigned int sev_enabled;

So there's a function name sev_enabled() and an int sev_enabled too.

It looks to me like you want to call the function "sev_enable()" -
similar to sme_enable(), convert it to C code - i.e., I don't see what
would speak against it - and rename that sev_enc_bit to sev_enabled and
use it everywhere when testing SEV status.

>  static inline bool sme_active(void)
>  {
> - return (sme_me_mask) ? true : false;
> + return (sme_me_mask && !sev_enabled) ? true : false;
> +}
> +
> +static inline bool sev_active(void)
> +{
> + return (sme_me_mask && sev_enabled) ? true : false;

Then, those read strange: like SME and SEV are mutually exclusive. Why?
I might have an idea but I'd like for you to confirm it :-)

Then, you're calling sev_enabled in startup_32() but we can enter
in arch/x86/boot/compressed/head_64.S::startup_64() too, when we're
loaded by a 64-bit bootloader, which would then theoretically bypass
sev_enabled().

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 10/32] x86: DMA support for SEV memory encryption

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:14:25AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> DMA access to memory mapped as encrypted while SEV is active can not be
> encrypted during device write or decrypted during device read. In order
> for DMA to properly work when SEV is active, the swiotlb bounce buffers
> must be used.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/mm/mem_encrypt.c |   77 
> +
>  1 file changed, 77 insertions(+)
> 
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 090419b..7df5f4c 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -197,8 +197,81 @@ void __init sme_early_init(void)
>   /* Update the protection map with memory encryption mask */
>   for (i = 0; i < ARRAY_SIZE(protection_map); i++)
>   protection_map[i] = pgprot_encrypted(protection_map[i]);
> +
> + if (sev_active())
> + swiotlb_force = SWIOTLB_FORCE;
> +}
> +
> +static void *sme_alloc(struct device *dev, size_t size, dma_addr_t 
> *dma_handle,
> +gfp_t gfp, unsigned long attrs)
> +{
> + unsigned long dma_mask;
> + unsigned int order;
> + struct page *page;
> + void *vaddr = NULL;
> +
> + dma_mask = dma_alloc_coherent_mask(dev, gfp);
> + order = get_order(size);
> +
> + gfp &= ~__GFP_ZERO;

Please add a comment around here that swiotlb_alloc_coherent() will
memset(, 0, ) the memory. It took me a while to figure out what the
situation is.

Also, Joerg says the __GFP_ZERO is not absolutely necessary but it has
not been fixed in the other DMA alloc* functions because of fears that
something would break. That bit could also be part of the comment.

> +
> + page = alloc_pages_node(dev_to_node(dev), gfp, order);
> + if (page) {
> + dma_addr_t addr;
> +
> + /*
> +  * Since we will be clearing the encryption bit, check the
> +  * mask with it already cleared.
> +  */
> + addr = phys_to_dma(dev, page_to_phys(page)) & ~sme_me_mask;
> + if ((addr + size) > dma_mask) {
> + __free_pages(page, get_order(size));
> + } else {
> + vaddr = page_address(page);
> + *dma_handle = addr;
> + }
> + }
> +
> + if (!vaddr)
> + vaddr = swiotlb_alloc_coherent(dev, size, dma_handle, gfp);
> +
> + if (!vaddr)
> + return NULL;
> +
> + /* Clear the SME encryption bit for DMA use if not swiotlb area */
> + if (!is_swiotlb_buffer(dma_to_phys(dev, *dma_handle))) {
> + set_memory_decrypted((unsigned long)vaddr, 1 << order);
> + *dma_handle &= ~sme_me_mask;
> + }
> +
> + return vaddr;
>  }
>  
> +static void sme_free(struct device *dev, size_t size, void *vaddr,
> +  dma_addr_t dma_handle, unsigned long attrs)
> +{
> + /* Set the SME encryption bit for re-use if not swiotlb area */
> + if (!is_swiotlb_buffer(dma_to_phys(dev, dma_handle)))
> + set_memory_encrypted((unsigned long)vaddr,
> +  1 << get_order(size));
> +
> + swiotlb_free_coherent(dev, size, vaddr, dma_handle);
> +}
> +
> +static struct dma_map_ops sme_dma_ops = {

WARNING: struct dma_map_ops should normally be const
#112: FILE: arch/x86/mm/mem_encrypt.c:261:
+static struct dma_map_ops sme_dma_ops = {

Please integrate scripts/checkpatch.pl in your patch creation workflow.
Some of the warnings/errors *actually* make sense.


> + .alloc  = sme_alloc,
> + .free   = sme_free,
> + .map_page   = swiotlb_map_page,
> + .unmap_page = swiotlb_unmap_page,
> + .map_sg = swiotlb_map_sg_attrs,
> + .unmap_sg   = swiotlb_unmap_sg_attrs,
> + .sync_single_for_cpu= swiotlb_sync_single_for_cpu,
> + .sync_single_for_device = swiotlb_sync_single_for_device,
> + .sync_sg_for_cpu= swiotlb_sync_sg_for_cpu,
> + .sync_sg_for_device = swiotlb_sync_sg_for_device,
> + .mapping_error  = swiotlb_dma_mapping_error,
> +};
> +
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void)
>  {
> @@ -208,6 +281,10 @@ void __init mem_encrypt_init(void)
>   /* Call into SWIOTLB to update the SWIOTLB DMA buffers */
>   swiotlb_update_mem_attributes();
>  
> + /* Use SEV DMA operations if SEV is active */

That's obvious. The WHY is not.

> + if (sev_active())
> + dma_ops = _dma_ops;
> +
>   pr_info("AMD Secure Memory Encryption (SME) active\n");
>  }
>  
> 

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 12/32] x86: Add early boot support when running with SEV active

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:14:48AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Early in the boot process, add checks to determine if the kernel is
> running with Secure Encrypted Virtualization (SEV) active by issuing
> a CPUID instruction.
> 
> During early compressed kernel booting, if SEV is active the pagetables are
> updated so that data is accessed and decompressed with encryption.
> 
> During uncompressed kernel booting, if SEV is the memory encryption mask is
> set and a flag is set to indicate that SEV is enabled.

I don't know how many times I have to say this but I'm going to keep
doing it until it sticks: :-)

Please, no "WHAT" in the commit messages - I can see the "WHAT - but
"WHY".

Ok?

> diff --git a/arch/x86/boot/compressed/mem_encrypt.S 
> b/arch/x86/boot/compressed/mem_encrypt.S
> new file mode 100644
> index 000..8313c31
> --- /dev/null
> +++ b/arch/x86/boot/compressed/mem_encrypt.S
> @@ -0,0 +1,75 @@
> +/*
> + * AMD Memory Encryption Support
> + *
> + * Copyright (C) 2016 Advanced Micro Devices, Inc.
> + *
> + * Author: Tom Lendacky 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> + .text
> + .code32
> +ENTRY(sev_enabled)
> + xor %eax, %eax
> +
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> + push%ebx
> + push%ecx
> + push%edx
> +
> + /* Check if running under a hypervisor */
> + movl$0x4000, %eax
> + cpuid
> + cmpl$0x4001, %eax
> + jb  .Lno_sev
> +
> + movl$0x4001, %eax
> + cpuid
> + bt  $KVM_FEATURE_SEV, %eax
> + jnc .Lno_sev
> +
> + /*
> +  * Check for memory encryption feature:
> +  *   CPUID Fn8000_001F[EAX] - Bit 0
> +  */
> + movl$0x801f, %eax
> + cpuid
> + bt  $0, %eax
> + jnc .Lno_sev
> +
> + /*
> +  * Get memory encryption information:
> +  *   CPUID Fn8000_001F[EBX] - Bits 5:0
> +  * Pagetable bit position used to indicate encryption
> +  */
> + movl%ebx, %eax
> + andl$0x3f, %eax
> + movl%eax, sev_enc_bit(%ebp)
> + jmp .Lsev_exit
> +
> +.Lno_sev:
> + xor %eax, %eax
> +
> +.Lsev_exit:
> + pop %edx
> + pop %ecx
> + pop %ebx
> +
> +#endif   /* CONFIG_AMD_MEM_ENCRYPT */
> +
> + ret
> +ENDPROC(sev_enabled)

Right, as said in another mail earlier, this could be written in C. And
then the sme_enable() piece below looks the same as this one above. So
since you want to run it before kernel decompression and after, you
could extract this code into a separate .c file which you can link in
both places, similar to what we do with verify_cpu with the difference
that verify_cpu is getting included.

Alternatively, we still have some room in setup_header.xloadflags to
pass boot info to kernel proper from before the decompression stage.

But I'd prefer linking with both stages as it is cheaper and those flags
we can use for something which really wants to use a flag like that.

> diff --git a/arch/x86/kernel/mem_encrypt_init.c 
> b/arch/x86/kernel/mem_encrypt_init.c
> index 35c5e3d..5d514e6 100644
> --- a/arch/x86/kernel/mem_encrypt_init.c
> +++ b/arch/x86/kernel/mem_encrypt_init.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  static char sme_cmdline_arg_on[] __initdata = "mem_encrypt=on";
>  static char sme_cmdline_arg_off[] __initdata = "mem_encrypt=off";
> @@ -232,6 +233,29 @@ unsigned long __init sme_enable(void *boot_data)
>   void *cmdline_arg;
>   u64 msr;
>  
> + /* Check if running under a hypervisor */
> + eax = 0x4000;
> + ecx = 0;
> + native_cpuid(, , , );
> + if (eax > 0x4000) {
> + eax = 0x4001;
> + ecx = 0;
> + native_cpuid(, , , );
> + if (!(eax & BIT(KVM_FEATURE_SEV)))
> + goto out;
> +
> + eax = 0x801f;
> + ecx = 0;
> + native_cpuid(, , , );
> + if (!(eax & 1))
> + goto out;
> +
> + sme_me_mask = 1UL << (ebx & 0x3f);
> + sev_enabled = 1;
> +
> + goto out;
> + }
> +
>   /* Check for an AMD processor */
>   eax = 0;
>   ecx = 0;
> 

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 14/32] x86: mm: Provide support to use memblock when spliting large pages

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:15:15AM -0500, Brijesh Singh wrote:
> If kernel_maps_pages_in_pgd is called early in boot process to change the

kernel_map_pages_in_pgd()

> memory attributes then it fails to allocate memory when spliting large
> pages. The patch extends the cpa_data to provide the support to use
> memblock_alloc when slab allocator is not available.
> 
> The feature will be used in Secure Encrypted Virtualization (SEV) mode,
> where we may need to change the memory region attributes in early boot
> process.
> 
> Signed-off-by: Brijesh Singh 
> ---
>  arch/x86/mm/pageattr.c |   51 
> 
>  1 file changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> index 46cc89d..9e4ab3b 100644
> --- a/arch/x86/mm/pageattr.c
> +++ b/arch/x86/mm/pageattr.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -37,6 +38,7 @@ struct cpa_data {
>   int flags;
>   unsigned long   pfn;
>   unsignedforce_split : 1;
> + unsignedforce_memblock :1;
>   int curpage;
>   struct page **pages;
>  };
> @@ -627,9 +629,8 @@ try_preserve_large_page(pte_t *kpte, unsigned long 
> address,
>  
>  static int
>  __split_large_page(struct cpa_data *cpa, pte_t *kpte, unsigned long address,
> -struct page *base)
> +   pte_t *pbase, unsigned long new_pfn)
>  {
> - pte_t *pbase = (pte_t *)page_address(base);
>   unsigned long ref_pfn, pfn, pfninc = 1;
>   unsigned int i, level;
>   pte_t *tmp;
> @@ -646,7 +647,7 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte, 
> unsigned long address,
>   return 1;
>   }
>  
> - paravirt_alloc_pte(_mm, page_to_pfn(base));
> + paravirt_alloc_pte(_mm, new_pfn);
>  
>   switch (level) {
>   case PG_LEVEL_2M:
> @@ -707,7 +708,8 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte, 
> unsigned long address,
>* pagetable protections, the actual ptes set above control the
>* primary protection behavior:
>*/
> - __set_pmd_pte(kpte, address, mk_pte(base, __pgprot(_KERNPG_TABLE)));
> + __set_pmd_pte(kpte, address,
> + native_make_pte((new_pfn << PAGE_SHIFT) + _KERNPG_TABLE));
>  
>   /*
>* Intel Atom errata AAH41 workaround.
> @@ -723,21 +725,50 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte, 
> unsigned long address,
>   return 0;
>  }
>  
> +static pte_t *try_alloc_pte(struct cpa_data *cpa, unsigned long *pfn)
> +{
> + unsigned long phys;
> + struct page *base;
> +
> + if (cpa->force_memblock) {
> + phys = memblock_alloc(PAGE_SIZE, PAGE_SIZE);

Maybe there's a reason this fires:

WARNING: modpost: Found 2 section mismatch(es).
To see full details build your kernel with:
'make CONFIG_DEBUG_SECTION_MISMATCH=y'

WARNING: vmlinux.o(.text+0x48edc): Section mismatch in reference from the 
function __change_page_attr() to the function .init.text:memblock_alloc()
The function __change_page_attr() references
the function __init memblock_alloc().
This is often because __change_page_attr lacks a __init
annotation or the annotation of memblock_alloc is wrong.

WARNING: vmlinux.o(.text+0x491d1): Section mismatch in reference from the 
function __change_page_attr() to the function .meminit.text:memblock_free()
The function __change_page_attr() references
the function __meminit memblock_free().
This is often because __change_page_attr lacks a __meminit
annotation or the annotation of memblock_free is wrong.

Why do we need this whole early mapping? For the guest? I don't like
that memblock thing at all.

So I think the approach with the .data..percpu..hv_shared section is
fine and we should consider SEV-ES

http://support.amd.com/TechDocs/Protecting%20VM%20Register%20State%20with%20SEV-ES.pdf

and do this right from the get-go so that when SEV-ES comes along, we
should simply be ready and extend that mechanism to put the whole Guest
Hypervisor Communication Block in there.

But then the fact that you're mapping those decrypted in init_mm.pgd
makes me think you don't need that early mapping thing at all. Those are
the decrypted mappings of the hypervisor. And that you can do late.

Now, what would be better, IMHO (and I have no idea about virtualization
design so take with a grain of salt) is if the guest would allocate
enough memory for the GHCB and mark it decrypted from the very
beginning. It will be the communication vehicle with the hypervisor
anyway.

And we already do similar things in sme_map_bootdata() for the baremetal
kernel to map boot_data, initrd, EFI, ... and so on things decrypted.

And we should extend that mechanism to map the GHCB in the guest too and
then we can get rid of all that need for ->force_memblock which makes
the crazy mess in pageattr.c even crazier. And it would be lovely if 

Re: [RFC PATCH v2 01/32] x86: Add the Secure Encrypted Virtualization CPU feature

[Borislav Petkov]

On Mon, Mar 06, 2017 at 01:11:03PM -0500, Brijesh Singh wrote:
> Sending it through stg mail to avoid line wrapping. Please let me know if 
> something
> is still messed up. I have tried applying it and it seems to apply okay.

Yep, thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 04/32] KVM: SVM: Add SEV feature definitions to KVM

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:12:48AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Define a new KVM CPU feature for Secure Encrypted Virtualization (SEV).
> The kernel will check for the presence of this feature to determine if
> it is running with SEV active.
> 
> Define the SEV enable bit for the VMCB control structure. The hypervisor
> will use this bit to enable SEV in the guest.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/include/asm/svm.h   |1 +
>  arch/x86/include/uapi/asm/kvm_para.h |1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 2aca535..fba2a7b 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -137,6 +137,7 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define SVM_VM_CR_SVM_DIS_MASK  0x0010ULL
>  
>  #define SVM_NESTED_CTL_NP_ENABLE BIT(0)
> +#define SVM_NESTED_CTL_SEV_ENABLEBIT(1)
>  
>  struct __attribute__ ((__packed__)) vmcb_seg {
>   u16 selector;
> diff --git a/arch/x86/include/uapi/asm/kvm_para.h 
> b/arch/x86/include/uapi/asm/kvm_para.h
> index 1421a65..bc2802f 100644
> --- a/arch/x86/include/uapi/asm/kvm_para.h
> +++ b/arch/x86/include/uapi/asm/kvm_para.h
> @@ -24,6 +24,7 @@
>  #define KVM_FEATURE_STEAL_TIME   5
>  #define KVM_FEATURE_PV_EOI   6
>  #define KVM_FEATURE_PV_UNHALT7
> +#define KVM_FEATURE_SEV  8

This looks like it needs documenting in Documentation/virtual/kvm/cpuid.txt

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 07/32] x86/efi: Access EFI data as encrypted when SEV is active

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:13:21AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> EFI data is encrypted when the kernel is run under SEV. Update the
> page table references to be sure the EFI memory areas are accessed
> encrypted.
> 
> Signed-off-by: Tom Lendacky 
> Signed-off-by: Brijesh Singh 

This SOB chain looks good.

> ---
>  arch/x86/platform/efi/efi_64.c |   15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index 2d8674d..9a76ed8 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -45,6 +45,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * We allocate runtime services regions bottom-up, starting from -4G, i.e.
> @@ -286,7 +287,10 @@ int __init efi_setup_page_tables(unsigned long 
> pa_memmap, unsigned num_pages)
>* as trim_bios_range() will reserve the first page and isolate it away
>* from memory allocators anyway.
>*/
> - if (kernel_map_pages_in_pgd(pgd, 0x0, 0x0, 1, _PAGE_RW)) {
> + pf = _PAGE_RW;
> + if (sev_active())
> + pf |= _PAGE_ENC;
> + if (kernel_map_pages_in_pgd(pgd, 0x0, 0x0, 1, pf)) {
>   pr_err("Failed to create 1:1 mapping for the first page!\n");
>   return 1;
>   }
> @@ -329,6 +333,9 @@ static void __init __map_region(efi_memory_desc_t *md, 
> u64 va)
>   if (!(md->attribute & EFI_MEMORY_WB))
>   flags |= _PAGE_PCD;
>  
> + if (sev_active())
> + flags |= _PAGE_ENC;
> +

So I'm wondering if we could avoid this sprinkling of _PAGE_ENC in the
EFI code by defining something like __supported_pte_mask but called
__efi_base_page_flags or so which has _PAGE_ENC cleared in the SME case,
i.e., when baremetal and has it set in the SEV case.

Then we could simply OR in __efi_base_page_flags which the SME/SEV code
will set appropriately early enough.

Hmm.

Matt, what do you think?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 02/32] x86: Secure Encrypted Virtualization (SEV) support

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:12:20AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> Provide support for Secure Encyrpted Virtualization (SEV). This initial
> support defines a flag that is used by the kernel to determine if it is
> running with SEV active.
> 
> Signed-off-by: Tom Lendacky 

Btw,

you need to add your Signed-off-by here after Tom's to denote that
you're handing that patch forward.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 05/32] x86: Use encrypted access of BOOT related data with SEV

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:12:59AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> When Secure Encrypted Virtualization (SEV) is active, BOOT data (such as
> EFI related data, setup data) is encrypted and needs to be accessed as
> such when mapped. Update the architecture override in early_memremap to
> keep the encryption attribute when mapping this data.

This could also explain why persistent memory needs to be accessed
decrypted with SEV.

In general, what the difference in that aspect is in respect to SME. And
I'd write that in the comment over the function. And not say "E820 areas
are checked in making this determination." because that is visible but
say *why* we need to check those ranges and determine access depending
on their type.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 08/32] x86: Use PAGE_KERNEL protection for ioremap of memory page

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:13:32AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> In order for memory pages to be properly mapped when SEV is active, we
> need to use the PAGE_KERNEL protection attribute as the base protection.
> This will insure that memory mapping of, e.g. ACPI tables, receives the
> proper mapping attributes.
> 
> Signed-off-by: Tom Lendacky 
> ---

> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index c400ab5..481c999 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -151,7 +151,15 @@ static void __iomem *__ioremap_caller(resource_size_t 
> phys_addr,
> pcm = new_pcm;
> }
> 
> +   /*
> +* If the page being mapped is in memory and SEV is active then
> +* make sure the memory encryption attribute is enabled in the
> +* resulting mapping.
> +*/
> prot = PAGE_KERNEL_IO;
> +   if (sev_active() && page_is_mem(pfn))

Hmm, a resource tree walk per ioremap call. This could get expensive for
ioremap-heavy workloads.

__ioremap_caller() gets called here during boot 55 times so not a whole
lot but I wouldn't be surprised if there were some nasty use cases which
ioremap a lot.

...

> diff --git a/kernel/resource.c b/kernel/resource.c
> index 9b5f044..db56ba3 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -518,6 +518,46 @@ int __weak page_is_ram(unsigned long pfn)
>  }
>  EXPORT_SYMBOL_GPL(page_is_ram);
>  
> +/*
> + * This function returns true if the target memory is marked as
> + * IORESOURCE_MEM and IORESOUCE_BUSY and described as other than
> + * IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
> + */
> +static int walk_mem_range(unsigned long start_pfn, unsigned long nr_pages)
> +{
> + struct resource res;
> + unsigned long pfn, end_pfn;
> + u64 orig_end;
> + int ret = -1;
> +
> + res.start = (u64) start_pfn << PAGE_SHIFT;
> + res.end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1;
> + res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> + orig_end = res.end;
> + while ((res.start < res.end) &&
> + (find_next_iomem_res(, IORES_DESC_NONE, true) >= 0)) {
> + pfn = (res.start + PAGE_SIZE - 1) >> PAGE_SHIFT;
> + end_pfn = (res.end + 1) >> PAGE_SHIFT;
> + if (end_pfn > pfn)
> + ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
> + if (ret)
> + break;
> + res.start = res.end + 1;
> + res.end = orig_end;
> + }
> + return ret;
> +}

So the relevant difference between this one and walk_system_ram_range()
is this:

-   ret = (*func)(pfn, end_pfn - pfn, arg);
+   ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;

so it seems to me you can have your own *func() pointer which does that
IORES_DESC_NONE comparison. And then you can define your own workhorse
__walk_memory_range() which gets called by both walk_mem_range() and
walk_system_ram_range() instead of almost duplicating them.

And looking at walk_system_ram_res(), that one looks similar too except
the pfn computation. But AFAICT the pfn/end_pfn things are computed from
res.start and res.end so it looks to me like all those three functions
are crying for unification...

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC PATCH v2 09/32] x86: Change early_ioremap to early_memremap for BOOT data

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:13:53AM -0500, Brijesh Singh wrote:
> From: Tom Lendacky 
> 
> In order to map BOOT data with the proper encryption bit, the

Btw, what does that all-caps spelling "BOOT" denote? Something I'm
missing?

> early_ioremap() function calls are changed to early_memremap() calls.
> This allows the proper access for both SME and SEV.
> 
> Signed-off-by: Tom Lendacky 
> ---
>  arch/x86/kernel/acpi/boot.c |4 ++--
>  arch/x86/kernel/mpparse.c   |   10 +-
>  drivers/sfi/sfi_core.c  |6 +++---
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> index 35174c6..468c25a 100644
> --- a/arch/x86/kernel/acpi/boot.c
> +++ b/arch/x86/kernel/acpi/boot.c
> @@ -124,7 +124,7 @@ char *__init __acpi_map_table(unsigned long phys, 
> unsigned long size)
>   if (!phys || !size)
>   return NULL;
>  
> - return early_ioremap(phys, size);
> + return early_memremap(phys, size);

Right, the question will keep popping up why we can simply replace
memremap with ioremap and the general difference wrt to SME/SEV. So it
would be a good idea to have a comment in, say, arch/x86/mm/ioremap.c,
explaining the general situation.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5 12/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Wed, Oct 04, 2017 at 08:13:53AM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machines to be transparently encrypted with a key
> unique to the guest VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP), which exposes the
> commands for these tasks. The complete spec is available at:
> 
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> Extend the AMD-SP driver to provide the following support:
> 
>  - an in-kernel APIs to communicate with a SEV firmware. The APIs can be
>used by the hypervisor to create encryption context for the SEV guests.
> 
>  - a userspace IOCTL to manage the platform certificates

Change that commit message to:

"AMD's new Secure Encrypted Virtualization (SEV) feature allows the
memory contents of virtual machines to be transparently encrypted with a
key unique to the VM. The programming and management of the encryption
keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
commands for these tasks. The complete spec is available at:

http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf

Extend the AMD-SP driver to provide the following support:

 - an in-kernel API to communicate with the SEV firmware. The API can be
   used by the hypervisor to create encryption context for a SEV guest.

 - a userspace IOCTL to manage the platform certificates."

> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 734 
> +++
>  drivers/crypto/ccp/psp-dev.h |  17 +
>  include/linux/psp-sev.h  | 159 ++
>  include/uapi/linux/psp-sev.h | 116 +++
>  4 files changed, 1026 insertions(+)
>  create mode 100644 include/uapi/linux/psp-sev.h
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 7480d4316239..1b87a699bd3f 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -23,9 +23,20 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  #include "sp-dev.h"
>  #include "psp-dev.h"
>  
> +#define DEVICE_NAME  "sev"
> +
> +static unsigned int sev_poll;
> +module_param(sev_poll, uint, 0444);
> +MODULE_PARM_DESC(sev_poll, "Poll for sev command completion - any non-zero 
> value");

What is that used for? Some debugging leftover probably? If not, add a
comment for why it is useful.

> +
> +DEFINE_MUTEX(sev_cmd_mutex);
> +static bool sev_fops_registered;
> +
>  const struct psp_vdata psp_entry = {
>   .offset = 0x10500,
>  };
> @@ -49,9 +60,725 @@ static struct psp_device *psp_alloc_struct(struct 
> sp_device *sp)
>  
>  static irqreturn_t psp_irq_handler(int irq, void *data)
>  {
> + struct psp_device *psp = data;
> + unsigned int status;
> +
> + /* read the interrupt status */
> + status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS);
> +
> + /* check if its command completion */
> + if (status & (1 << PSP_CMD_COMPLETE_REG)) {
> + int reg;
> +
> + /* check if its SEV command completion */
> + reg = ioread32(psp->io_regs + PSP_CMDRESP);
> + if (reg & PSP_CMDRESP_RESP) {
> + psp->sev_int_rcvd = 1;
> + wake_up(>sev_int_queue);
> + }
> + }
> +
> + /* clear the interrupt status by writing 1 */
> + iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS);
> +
>   return IRQ_HANDLED;
>  }
>  
> +static int sev_wait_cmd_poll(struct psp_device *psp, unsigned int timeout,
> +  unsigned int *reg)
> +{
> + int wait = timeout * 10;/* 100ms sleep => timeout * 10 */

How is this a 100ms sleep?!

10 * 10 * 100ms = 1ms = 10 seconds max sleep is what my math says.

> +
> + while (--wait) {
> + msleep(100);
> +
> + *reg = ioread32(psp->io_regs + PSP_CMDRESP);
> + if (*reg & PSP_CMDRESP_RESP)
> + break;
> + }
> +
> + if (!wait) {
> + dev_err(psp->dev, "sev command timed out\n");
> + return -ETIMEDOUT;
&

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> During the device probe, sev_ops_init() will be called for every device
> instance which claims to support the SEV.  One of the device will be
> 'master' but we don't the master until we probe all the instances. Hence
> the probe for all SEV devices must return success.

I still am wondering whether that design with multiple devices - master
and non-master devices is optimal. Why isn't the security processor a
single driver which provides the whole functionality, PSP including? Why
do you need all that register and unregister glue and get_master bla if
you can simply put the whole thing in a single module?

And the fact that you need a global variable to mark that you've
registered the misc device should already tell you that something's
not optimal. Because if you had a single driver, it will go, detect
the whole functionality, initialize it, register services and be done
with it. No registering of devices, no finding of masters, no global
variables, no unnecessary glue.

IOW, in this diagram:

 +--- CCP
 |
AMD-SP --|
 |+--- SEV
 ||
 + PSP ---*
  |
  + TEE

why isn't the whole PSP functionality part of drivers/crypto/ccp/sp-dev.c ?

That driver is almost barebones minimal thing. You can very well add the
PSP/SEV stuff in there and if there's an *actual* reason to carve pieces
out, only then to do so, not to split it now unnecessarily and make your
life complicated for no reason.

Or am I missing some obvious and important reason?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 Patch v4.2] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Wed, Oct 04, 2017 at 12:06:42PM +0530, P J P wrote:
> Needs to kfree(sp->psp_data) before setting to NULL.

Not if it is allocated with devm_kzalloc().

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v4.1 07/29] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Wed, Oct 04, 2017 at 03:24:36PM +0530, P J P wrote:
> It appears to cross 80 columns limit, checkpatch.pl throws warnings. Adding 
> new line would be consistent with coding style.

The 80 cols rule is not a hard one and checkpatch should not override
common sense. This is a function which maps commands to buffer lengths
and it should be obvious at a *very* quick glance what it does. And that
is best done if it is written in a tabular manner as we do such things
in other places in the kernel too.

Btw, Brijesh, please space out those statements for even better
readability:

static int sev_cmd_buffer_len(int cmd)
{
   switch (cmd) {
   case SEV_CMD_INIT:   return sizeof(struct 
sev_data_init);
   case SEV_CMD_PLATFORM_STATUS:return sizeof(struct 
sev_data_status);
   case SEV_CMD_PEK_CSR:return sizeof(struct 
sev_data_pek_csr);
   case SEV_CMD_PEK_CERT_IMPORT:return sizeof(struct 
sev_data_pek_cert_import);
   case SEV_CMD_PDH_CERT_EXPORT:return sizeof(struct 
sev_data_pdh_cert_export);
   case SEV_CMD_LAUNCH_START:   return sizeof(struct 
sev_data_launch_start);
   case SEV_CMD_LAUNCH_UPDATE_DATA: return sizeof(struct 
sev_data_launch_update_data);
   case SEV_CMD_LAUNCH_UPDATE_VMSA: return sizeof(struct 
sev_data_launch_update_vmsa);
   case SEV_CMD_LAUNCH_FINISH:  return sizeof(struct 
sev_data_launch_finish);
   case SEV_CMD_LAUNCH_MEASURE: return sizeof(struct 
sev_data_launch_measure);
   case SEV_CMD_ACTIVATE:   return sizeof(struct 
sev_data_activate);
   case SEV_CMD_DEACTIVATE: return sizeof(struct 
sev_data_deactivate);
   case SEV_CMD_DECOMMISSION:   return sizeof(struct 
sev_data_decommission);
   case SEV_CMD_GUEST_STATUS:   return sizeof(struct 
sev_data_guest_status);
   case SEV_CMD_DBG_DECRYPT:return sizeof(struct 
sev_data_dbg);
   case SEV_CMD_DBG_ENCRYPT:return sizeof(struct 
sev_data_dbg);
   case SEV_CMD_SEND_START: return sizeof(struct 
sev_data_send_start);
   case SEV_CMD_SEND_UPDATE_DATA:   return sizeof(struct 
sev_data_send_update_data);
   case SEV_CMD_SEND_UPDATE_VMSA:   return sizeof(struct 
sev_data_send_update_vmsa);
   case SEV_CMD_SEND_FINISH:return sizeof(struct 
sev_data_send_finish);
   case SEV_CMD_RECEIVE_START:  return sizeof(struct 
sev_data_receive_start);
   case SEV_CMD_RECEIVE_FINISH: return sizeof(struct 
sev_data_receive_finish);
   case SEV_CMD_RECEIVE_UPDATE_DATA:return sizeof(struct 
sev_data_receive_update_data);
   case SEV_CMD_RECEIVE_UPDATE_VMSA:return sizeof(struct 
sev_data_receive_update_vmsa);
   case SEV_CMD_LAUNCH_UPDATE_SECRET:   return sizeof(struct 
sev_data_launch_secret);
   default: return 0;
   }

   return 0;
}

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v4.1 07/29] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Wed, Oct 04, 2017 at 12:26:11PM +0530, P J P wrote:
> Each return above needs to be on its own line.

... because?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Sun, Oct 08, 2017 at 07:11:04PM -0500, Brijesh Singh wrote:
> There is a single security processor driver (ccp) which provides the
> complete functionality including PSP.  But the driver should be able to
> work with multiple devices. e.g In my 2P EPYC configuration, security
> processor driver is used for the following devices
> 
> 02:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1456

So we have a lot of drivers which claim more than one PCI device. It is
not mandatory to have a driver per PCI device. Actually, the decisive
argument is what is the easiest way to manage those devices and what
makes the communication between them the easiest.

> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1468
> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1456

Btw, what do those PCI functions each do? Public PPR doesn't have them
documented.

> Some of the these devices support CCP only and some support both PSP and
> CCP. It's possible that multiple devices support the PSP functionality
> but only one of them is master which can be used for issuing SEV
> commands. There isn't a register which we can read to determine whether
> the device is master. We have to probe all the devices which supports
> the PSP to determine which one of them is a master.

Sure, and if you manage all the devices in a single driver, you can
simply keep them all in a linked list or in an array and iterating over
them is trivial.

Because right now you have

1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection

2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP

3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
sp->dev_vdata->psp_vdata which is nothing more than a simple offset
0x10500 which is where the PSP io regs are. For example, if this offset
is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
0x10500. No need for all that passing of structs around.

4. and finally, after that *whole* init has been done, you get to do
->set_psp_master_device(sp);

Or, you can save yourself all that jumping through hoops, merge sp-pci.c
and sp-dev.c into a single sp.c and put *everything* sp-related into
it. And then do the whole work of picking hw apart, detection and
configuration in sp_pci_probe() and have helper functions preparing and
configuring the device.

At the end, it adds it to the list of devices sp.c manages and done. You
actually have that list already:

static LIST_HEAD(sp_units);

in sp-dev.c.

You don't need the set_master thing either - you simply set the
sp_dev_master pointer inside sp.c

sp_init() can then go and you can replace it with its function body,
deciding whether it is a CCP or PSP and then call the respective
function which is also in sp.c or ccp-dev.c

And then all those separate compilation units and the interfaces between
them disappear - you have only the calls into the PSP and that's it.

Btw, the CCP thing could remain separate initially, I guess, with all
that ccp-* stuff in there.

> I was trying to follow the CCP  model -- in which sp-dev.c simply
> forwards the call to ccp-dev.c which does the real work.

And you don't really need that - you can do the real work directly in
sp-dev.c or sp.c or whatever.

> Currently, sev-dev.c contains barebone common code. IMO, keeping all
> the PSP private functions and data structure outside the sp-dev.c/.h
> is right thing.

By this model probably, but it causes all that init and registration
jump-through-hoops for no real reason. It is basically wasting cycles
and energy.

I'm all for splitting if it makes sense. But right now I don't see much
sense in this - it is basically a bunch of small compilation units
calling each other. And they could be merged into a single sp.c which
does it all in one go, without a lot of blabla.

> Additionally, I would like to highlight that if we decide to go with
> moving all the PSP functionality in sp-dev.c then we have to add #ifdef
> CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
> the sp-dev.c gets compiled for all architectures (including aarch64,
> i386 and x86_64).

That's fine. You can build it on 32-bit but add to the init function

if (IS_ENABLED(CONFIG_X86_32))
return -ENODEV;

and be done with it. No need for the ifdeffery.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Tue, Oct 10, 2017 at 01:43:22PM -0500, Tom Lendacky wrote:
> Maybe for the very first implementation we could do that and that was what
> was originally done for the CCP.  But as you can see the CCP does not have
> a set register offset between various iterations of the device and it can
> be expected the same will hold true for the PSP.  This just makes future
> changes easier in order to support newer devices.

Well, that's a simple switch-case statement which maps device iteration
to offset.

> I would prefer to keep things separated as they are.  The common code is
> one place and the pci/platform specific code resides in unique files. For
> sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined
> vs. adding #ifdefs into sp-dev.c or sp.c.  The code is working nicely and,
> at least to me, seems easily maintainable this way.

But you do see that you're doing a bunch of unnecessary things during
probing. And all those different devices: SP, CCP, PSP, TEE and whatnot,
they're just too granulary and it is a bunch of registration code and
one compilation unit calling into the other for no good reason. Or at
least I don't see it.

> If we really want to avoid the extra calls during probing, etc.
> then we can take a closer look afterwards and determine what is the
> best approach taking into account the CCP and some of the other PSP
> functionality that is coming.

And that coming functionality won't make things easier - you'll most
likely have more things wanting to talk to more other things. But ok,
your call. Just note that changing things later is always harder.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

[Borislav Petkov]

On Thu, Oct 12, 2017 at 11:13:44PM -0500, Brijesh Singh wrote:
> As per the spec, its perfectly legal to pass input.address = 0x0 and
> input.length = 0x0. If userspace wants to query CSR length then it will
> fill all the fields with. In response, FW will return error
> (LENGTH_TO_SMALL) and input.length will be filled with the expected
> length. Several command work very similar (e.g PEK_CSR,
> PEK_CERT_EXPORT). A typical usage from userspace will be:
> 
> - query the length of the blob (call command with all fields set to zero)
> - SEV FW will response with expected length
> - userspace allocate the blob and retries the command. 

Ok, let's make that a clearer and more precise by explicitly checking
the query case:

+   /* Userspace wants to query CSR length */
+   if (!input.address && !input.length)
+   goto cmd;

and commenting why we're doing this.

The rest is cleanups.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..e10f507f9a60 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -299,36 +299,37 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
struct sev_user_data_pek_csr input;
struct sev_data_pek_csr *data;
int do_shutdown = 0;
+   void *blob = NULL;
int ret, state;
-   void *blob;
 
-   if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
-  sizeof(struct sev_user_data_pek_csr)))
+   if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
 
-   /* allocate a temporary physical contigous buffer to store the CSR blob 
*/
-   blob = NULL;
-   if (input.address) {
-   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
-   input.length > SEV_FW_BLOB_MAX_SIZE) {
-   ret = -EFAULT;
-   goto e_free;
-   }
+   /* Userspace wants to query CSR length */
+   if (!input.address && !input.length)
+   goto cmd;
 
-   blob = kmalloc(input.length, GFP_KERNEL);
-   if (!blob) {
-   ret = -ENOMEM;
-   goto e_free;
-   }
+   /* allocate a physically contiguous buffer to store the CSR blob */
+   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+   input.length > SEV_FW_BLOB_MAX_SIZE) {
+   ret = -EFAULT;
+   goto e_free;
+   }
 
-   data->address = __psp_pa(blob);
-   data->len = input.length;
+   blob = kmalloc(input.length, GFP_KERNEL);
+   if (!blob) {
+   ret = -ENOMEM;
+   goto e_free;
}
 
+   data->address = __psp_pa(blob);
+   data->len = input.length;
+
+cmd:
ret = sev_platform_get_state(, >error);
if (ret)
goto e_free_blob;
@@ -349,25 +350,26 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
 
+   /*
+* If we query the CSR length, FW responded with the expected length.
+*/
input.length = data->len;
 
-   /* copy blob to userspace */
-   if (blob &&
-   copy_to_user((void __user *)(uintptr_t)input.address,
-   blob, input.length)) {
-   ret = -EFAULT;
-   goto e_shutdown;
+   if (blob) {
+   if (copy_to_user((void __user *)input.address, blob, 
input.length)) {
+   ret = -EFAULT;
+   goto e_shutdown;
+   }
}
 
-   if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
-sizeof(struct sev_user_data_pek_csr)))
+   if (copy_to_user((void __user *)argp->data, , sizeof(input)))
ret = -EFAULT;
 
 e_shutdown:
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
 e_free_blob:
kfree(blob);
 e_free:

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.8/31] crypto: ccp: Implement SEV_PEK_CERT_IMPORT ioctl command

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:06PM -0500, Brijesh Singh wrote:
> The SEV_PEK_CERT_IMPORT command can be used to import the signed PEK
> certificate. The command is defined in SEV spec section 5.8.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 97 
> 
>  1 file changed, 97 insertions(+)

Just minor cleanups, otherwise looks ok.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 25a437c8d532..e61758af288f 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -392,7 +392,7 @@ static void *copy_user_blob(u64 __user uaddr, u32 len)
if (!data)
return ERR_PTR(-ENOMEM);
 
-   if (copy_from_user(data, (void __user *)(uintptr_t)uaddr, len))
+   if (copy_from_user(data, (void __user *)uaddr, len))
goto e_free;
 
return data;
@@ -409,8 +409,7 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd 
*argp)
int ret, state, do_shutdown = 0;
void *pek_blob, *oca_blob;
 
-   if (copy_from_user(, (void __user *)(uintptr_t) argp->data,
-  sizeof(struct sev_user_data_pek_cert_import)))
+   if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
return -EFAULT;
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -456,10 +455,10 @@ static int sev_ioctl_pek_cert_import(struct sev_issue_cmd 
*argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_CERT_IMPORT, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_CERT_IMPORT, data, >error);
 
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
 
 e_free_oca:
kfree(oca_blob);
@@ -503,14 +502,14 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen();
break;
 
-   case SEV_PEK_CSR: {
+   case SEV_PEK_CSR:
ret = sev_ioctl_pek_csr();
break;
-   }
-   case SEV_PEK_CERT_IMPORT: {
+
+   case SEV_PEK_CERT_IMPORT:
ret = sev_ioctl_pek_cert_import();
break;
-   }
+
default:
ret = -EINVAL;
goto out;

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

[Borislav Petkov]

On Thu, Oct 12, 2017 at 09:24:01PM -0500, Brijesh Singh wrote:
> I assume you mean performing the SEV state check before allocating the
> memory for the CSR blob, right ?

I mean, do those first:

if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
return -EFAULT;

if (!input.address)
return -EINVAL;

/* allocate a physically contiguous buffer to store the CSR blob */
if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;


Because if you allocate the memory first and some of those checks fail,
you allocate in vain to free it immediately after. And you can save
yourself all that.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Thu, Sep 07, 2017 at 05:19:32PM -0500, Brijesh Singh wrote:
> At high level, AMD-SP (AMD Secure Processor) (i.e CCP driver) will provide the
> support for CCP, SEV and TEE FW commands.
> 
> 
>  +--- CCP
>  |
> AMD-SP --|
>  |+--- SEV
>  ||
>  + PSP ---*
>   |
>   + TEE

I still don't see the need for such finegrained separation, though.
There's no "this is a separate compilation unit because... ". At least
the PSP branch could be a single driver without the interface.

For example, psp_request_sev_irq() is called only by sev_dev_init(). So
why is sev-dev a separate compilation unit? Is anything else going to
use the PSP interface?

If not, just put it all in a psp-dev file and that's it. We have a
gazillion config options and having two more just because, is not a good
reason. You can always carve it out later if there's real need. But if
the SEV thing can't function without the PSP thing, then you can just as
well put it inside it.

This way you can save yourself a bunch of exported functions and the
like.

Another example for not optimal design is psp_request_tee_irq() - it
doesn't really request an irq by calling into the IRQ core but simply
assigns a handler. Which looks to me like you're simulating an interface
where one is not really needed. Ditto for the sev_irq version, btw.

And where are the psp_request_tee_irq() et al callers? Nothing calls
those functions. So you can save yourself all that needless glue if you
put them in a single psp-dev and have that functionality always present
when you enable the PSP.

Because this is what the PSP does - SEV and TEE services. Why would you
have CRYPTO_DEV_PSP_SEV depend on CRYPTO_DEV_SP_PSP where the SEV and
TEE functionality are integral part of it?

And so on and so on...

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Thu, Sep 07, 2017 at 06:15:55PM -0500, Gary R Hook wrote:
> I would prefer that we not shorten this. The prior incarnation,
> ccp_alloc_struct(), has/had been around for a while. And there are a
> number of similarly named allocation functions in the driver that we
> like to keep sorted. If anything, it should be more explanatory, IMO.

Well, looks like an AMD practice:

$ git grep --name-only alloc_struct
drivers/crypto/ccp/ccp-dev.c
drivers/crypto/ccp/ccp-dev.h
drivers/crypto/ccp/psp-dev.c
drivers/crypto/ccp/sev-dev.c
drivers/crypto/ccp/sp-dev.c
drivers/crypto/ccp/sp-dev.h
drivers/crypto/ccp/sp-pci.c
drivers/crypto/ccp/sp-platform.c
drivers/gpu/drm/amd/amdkfd/kfd_dbgmgr.c
drivers/gpu/drm/amd/amdkfd/kfd_priv.h
drivers/gpu/drm/amd/amdkfd/kfd_topology.c

But whatever, if you like it more that way... :)

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Wed, Sep 06, 2017 at 04:26:52PM -0500, Gary R Hook wrote:
> They were included in a pull request (for 4.14) from Herbert, dated Monday.

Right. I rebased the SEV pile ontop of latest upstream and now it applies much
better:

checking file drivers/crypto/ccp/Kconfig
Hunk #1 succeeded at 32 (offset 1 line).
checking file drivers/crypto/ccp/Makefile
checking file drivers/crypto/ccp/psp-dev.c
checking file drivers/crypto/ccp/psp-dev.h
checking file drivers/crypto/ccp/sp-dev.c
Hunk #3 succeeded at 225 (offset 1 line).
Hunk #4 FAILED at 243.
1 out of 4 hunks FAILED
checking file drivers/crypto/ccp/sp-dev.h
Hunk #1 succeeded at 42 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 75 (offset 1 line).
Hunk #3 succeeded at 114 (offset 1 line).
Hunk #4 succeeded at 143 (offset 1 line).
checking file drivers/crypto/ccp/sp-pci.c

The failed hunk is due to #ifdef CONFIG_PM guards but that's trivial to fix.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

[Borislav Petkov]

On Mon, Jul 24, 2017 at 03:02:40PM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machine to be transparently encrypted with a key
> unique to the guest VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP), which exposes the
> commands for these tasks. The complete spec for various commands are
> available at:
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> This patch extends AMD-SP driver to provide:
> 
>  - a in-kernel APIs to communicate with SEV device. The APIs can be used
>by the hypervisor to create encryption context for the SEV guests.
> 
>  - a userspace IOCTL to manage the platform certificates etc
> 
> Cc: Herbert Xu 
> Cc: David S. Miller 
> Cc: Gary Hook 
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Brijesh Singh 

...

> +int sev_issue_cmd(int cmd, void *data, int *psp_ret)
> +{
> + struct sev_device *sev = get_sev_master_device();
> + unsigned int phys_lsb, phys_msb;
> + unsigned int reg, ret;
> +
> + if (!sev)
> + return -ENODEV;
> +
> + if (psp_ret)
> + *psp_ret = 0;

So you set psp_ret to 0 here...

> +
> + /* Set the physical address for the PSP */
> + phys_lsb = data ? lower_32_bits(__psp_pa(data)) : 0;
> + phys_msb = data ? upper_32_bits(__psp_pa(data)) : 0;
> +
> + dev_dbg(sev->dev, "sev command id %#x buffer 0x%08x%08x\n",
> + cmd, phys_msb, phys_lsb);
> + print_hex_dump_debug("(in):  ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> +
> + /* Only one command at a time... */
> + mutex_lock(_cmd_mutex);
> +
> + iowrite32(phys_lsb, sev->io_regs + PSP_CMDBUFF_ADDR_LO);
> + iowrite32(phys_msb, sev->io_regs + PSP_CMDBUFF_ADDR_HI);
> + wmb();
> +
> + reg = cmd;
> + reg <<= PSP_CMDRESP_CMD_SHIFT;
> + reg |= sev_poll ? 0 : PSP_CMDRESP_IOC;
> + iowrite32(reg, sev->io_regs + PSP_CMDRESP);
> +
> + ret = sev_wait_cmd(sev, );
> + if (ret)
> + goto unlock;

... something fails here and you unlock...

> +
> + if (psp_ret)
> + *psp_ret = reg & PSP_CMDRESP_ERR_MASK;
> +
> + if (reg & PSP_CMDRESP_ERR_MASK) {
> + dev_dbg(sev->dev, "sev command %u failed (%#010x)\n",
> + cmd, reg & PSP_CMDRESP_ERR_MASK);
> + ret = -EIO;
> + }
> +
> +unlock:
> + mutex_unlock(_cmd_mutex);
> + print_hex_dump_debug("(out): ", DUMP_PREFIX_OFFSET, 16, 2, data,
> + sev_cmd_buffer_len(cmd), false);
> + return ret;

... and here you return psp_ret == 0 even though something failed.

What I think you should do is not touch @psp_ret when you return before
the SEV command executes and *when* you return, set @psp_ret accordingly
to denote the status of the command execution.

Or if you're touching it before you execute the SEV
command and you return early, it should say something like
PSP_CMDRESP_COMMAND_DIDNT_EXECUTE or so, to tell the caller exactly what
happened.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Mon, Jul 24, 2017 at 03:02:39PM -0500, Brijesh Singh wrote:
> Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP),
> PSP is a dedicated processor that provides the support for key management
> commands in a Secure Encrypted Virtualiztion (SEV) mode, along with
> software-based Tursted Executation Environment (TEE) to enable the
> third-party tursted applications.
> 
> Cc: Herbert Xu 
> Cc: David S. Miller 
> Cc: Gary Hook 
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/Kconfig   |   9 ++
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/psp-dev.c | 226 
> +++
>  drivers/crypto/ccp/psp-dev.h |  82 
>  drivers/crypto/ccp/sp-dev.c  |  43 
>  drivers/crypto/ccp/sp-dev.h  |  41 +++-
>  drivers/crypto/ccp/sp-pci.c  |  46 +
>  7 files changed, 447 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/crypto/ccp/psp-dev.c
>  create mode 100644 drivers/crypto/ccp/psp-dev.h

...

> diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
> index a017233..d263ba4 100644
> --- a/drivers/crypto/ccp/sp-dev.c
> +++ b/drivers/crypto/ccp/sp-dev.c

Hunk #1 succeeded at 24 (offset -7 lines).
checking file drivers/crypto/ccp/Makefile
Hunk #1 FAILED at 7.
1 out of 1 hunk FAILED
checking file drivers/crypto/ccp/psp-dev.c
checking file drivers/crypto/ccp/psp-dev.h
can't find file to patch at input line 414
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--
|diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c
|index a017233..d263ba4 100644
|--- a/drivers/crypto/ccp/sp-dev.c
|+++ b/drivers/crypto/ccp/sp-dev.c
--

What tree is that against? In any case, it doesn't apply here.

> This RFC is based on tip/master commit : 22db3de (Merge branch 'x86/mm').

$ git show 22db3de
fatal: ambiguous argument '22db3de': unknown revision or path not in the 
working tree.

Do you have updated version of the series which you can send out?

> @@ -67,6 +74,10 @@ struct sp_device {
>   /* DMA caching attribute support */
>   unsigned int axcache;
>  
> + /* get and set master device */
> + struct sp_device*(*get_psp_master_device)(void);
> + void(*set_psp_master_device)(struct sp_device *);

WARNING: missing space after return type
#502: FILE: drivers/crypto/ccp/sp-dev.h:79:
+   void(*set_psp_master_device)(struct sp_device *);

Don't forget to run all patches through checkpatch. Some of the warnings
make sense.

Thx.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

[Borislav Petkov]

Just a cursory review: more indepth after the redesign.

On Mon, Jul 24, 2017 at 03:02:40PM -0500, Brijesh Singh wrote:
> AMDs new Secure Encrypted Virtualization (SEV) feature allows the memory
> contents of a virtual machine to be transparently encrypted with a key
> unique to the guest VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP), which exposes the
> commands for these tasks. The complete spec for various commands are

s/ for various commands are/is/

> available at:
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> This patch extends AMD-SP driver to provide:

Never say "This patch" in a commit message of a patch. It is
tautologically useless.

>  - a in-kernel APIs to communicate with SEV device. The APIs can be used

s/a/an/  with a SEV ...

>by the hypervisor to create encryption context for the SEV guests.
> 
>  - a userspace IOCTL to manage the platform certificates etc
> 
> Cc: Herbert Xu 
> Cc: David S. Miller 
> Cc: Gary Hook 
> Cc: linux-crypto@vger.kernel.org
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/Kconfig   |  10 +
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/psp-dev.c |   4 +
>  drivers/crypto/ccp/psp-dev.h |  27 ++
>  drivers/crypto/ccp/sev-dev.c | 416 ++
>  drivers/crypto/ccp/sev-dev.h |  67 +
>  drivers/crypto/ccp/sev-ops.c | 457 +
>  drivers/crypto/ccp/sp-pci.c  |   2 +-
>  include/linux/psp-sev.h  | 683 
> +++
>  include/uapi/linux/psp-sev.h | 110 +++
>  10 files changed, 1776 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/crypto/ccp/sev-dev.c
>  create mode 100644 drivers/crypto/ccp/sev-dev.h
>  create mode 100644 drivers/crypto/ccp/sev-ops.c
>  create mode 100644 include/linux/psp-sev.h
>  create mode 100644 include/uapi/linux/psp-sev.h
> 
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index 41c0ff5..ae0ff1c 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -40,3 +40,13 @@ config CRYPTO_DEV_SP_PSP
>Provide the support for AMD Platform Security Processor (PSP) device
>which can be used for communicating with Secure Encryption 
> Virtualization
>(SEV) firmware.
> +
> +config CRYPTO_DEV_PSP_SEV
> + bool "Secure Encrypted Virtualization (SEV) interface"
> + default y
> + depends on CRYPTO_DEV_CCP_DD
> + depends on CRYPTO_DEV_SP_PSP
> + help
> +  Provide the kernel and userspace (/dev/sev) interface to communicate 
> with
> +  Secure Encrypted Virtualization (SEV) firmware running inside AMD 
> Platform
> +  Security Processor (PSP)
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 8aae4ff..94ca748 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -8,6 +8,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \
>   ccp-debugfs.o
>  ccp-$(CONFIG_PCI) += sp-pci.o
>  ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o
> +ccp-$(CONFIG_CRYPTO_DEV_PSP_SEV) += sev-dev.o sev-ops.o
>  
>  obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
>  ccp-crypto-objs := ccp-crypto-main.o \
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index bb0ea9a..0c9d25c 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -97,6 +97,7 @@ irqreturn_t psp_irq_handler(int irq, void *data)
>  static int psp_init(struct psp_device *psp)
>  {
>   psp_add_device(psp);
> + sev_dev_init(psp);
>  
>   return 0;
>  }
> @@ -166,17 +167,20 @@ void psp_dev_destroy(struct sp_device *sp)
>   struct psp_device *psp = sp->psp_data;
>  
>   sp_free_psp_irq(sp, psp);
> + sev_dev_destroy(psp);
>  
>   psp_del_device(psp);
>  }
>  
>  int psp_dev_resume(struct sp_device *sp)
>  {
> + sev_dev_resume(sp->psp_data);
>   return 0;
>  }
>  
>  int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
>  {
> + sev_dev_suspend(sp->psp_data, state);
>   return 0;
>  }
>  
> diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
> index 6e167b8..9334d87 100644
> --- a/drivers/crypto/ccp/psp-dev.h
> +++ b/drivers/crypto/ccp/psp-dev.h
> @@ -78,5 +78,32 @@ int psp_free_tee_irq(struct psp_device *psp, void *data);
>  struct psp_device *psp_get_master_device(void);
>  
>  extern const struct psp_vdata psp_entry;
> +#ifdef CONFIG_CRYPTO_DEV_PSP_SEV
> +
> +int sev_dev_init(struct psp_device *psp);
> +void sev_dev_destroy(struct psp_device *psp);
> +int sev_dev_resume(struct psp_device *psp);
> +int sev_dev_suspend(struct psp_device *psp, pm_message_t state);
> +
> +#else /* !CONFIG_CRYPTO_DEV_PSP_SEV */
> +
> +static inline int sev_dev_init(struct psp_device *psp)
> +{
> + return -ENODEV;
> +}

Re: [RFC Part2 PATCH v3 03/26] crypto: ccp: Add Secure Encrypted Virtualization (SEV) device support

[Borislav Petkov]

On Tue, Sep 12, 2017 at 10:32:13AM -0500, Brijesh Singh wrote:
> The debug statement is very helpful during development, it gives me the full
> view of what command we send to PSP, data dump of command buffer before and
> after the request completion.  e.g when dyndbg is enabled the output looks 
> like
> this:

I'm sure it is all very helpful but you have a bunch of code which is
always built-in and useful only to developers. Which means it could
be behind #ifdef DEBUG at least and disabled on production systems.
You don't have to do it immediately but once the stuff goes up and
everything stabilizes, you could ifdef it out... Something to think
about later, I'd say.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v4 05/29] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Sat, Sep 30, 2017 at 10:55:25AM -0500, Brijesh Singh wrote:
> CRYPTO_DEV_CCP_DD is supported on aarch64 and x86. Whereas the PSP
> interface I am adding is available on x86 only hence its safe to add add
> depend on CPU_SUP_AMD for CRYPTO_DEV_SP_PSP.

I think just from having CRYPTO_DEV_CCP_DD depend on CPU_SUP_AMD ||
ARM64, CRYPTO_DEV_SP_PSP gets almost the same dependency transitively.
But sure, let's make the PSP build only on x86. It should depend on
X86_64, to be precise.

> Yes its very much possible. The SEV FW provides two sets of commands 1)
> platform certificate management and 2) guest management
> 
> The platform certificate management commands is used outside the
> CONFIG_KVM_AMD.

Ok, please state that in the commit message so that it is written down
somewhere.

Thx.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] crypto: ccp: Build the AMD secure processor driver only with AMD CPU support

[Borislav Petkov]

On Sat, Sep 30, 2017 at 09:06:26AM -0500, Brijesh Singh wrote:
> > diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> > index 627f3e61dcac..f58a6521270b 100644
> > --- a/drivers/crypto/ccp/Kconfig
> > +++ b/drivers/crypto/ccp/Kconfig
> > @@ -1,5 +1,6 @@
> >  config CRYPTO_DEV_CCP_DD
> > tristate "Secure Processor device driver"
> > +   depends on CPU_SUP_AMD
> 
> Please note that ccp.ko is built on both x86 and aarch64 (AMD Seattle)
> architectures. I have not looked into details but I thought CPU_SUP_AMD
> is x64 specific config. I will look into it and verify that we don't
> break AMD Seattle platform builds.

Ah, then I guess that line needs to be:

depends on CPU_SUP_AMD || ARM64

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

[PATCH] crypto: ccp: Build the AMD secure processor driver only with AMD CPU support

[Borislav Petkov]

Hi,

just a small Kconfig correction. Feel free to add it to your patchset.

Thx.

---
From: Borislav Petkov <b...@suse.de>

This is AMD-specific hardware so present it in Kconfig only when AMD
CPU support is enabled.

Signed-off-by: Borislav Petkov <b...@suse.de>
Cc: Brijesh Singh <brijesh.si...@amd.com>
Cc: Tom Lendacky <thomas.lenda...@amd.com>
Cc: Gary Hook <gary.h...@amd.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: linux-crypto@vger.kernel.org
---
 drivers/crypto/ccp/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index 627f3e61dcac..f58a6521270b 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -1,5 +1,6 @@
 config CRYPTO_DEV_CCP_DD
tristate "Secure Processor device driver"
+   depends on CPU_SUP_AMD
default m
help
  Provides AMD Secure Processor device driver.
-- 
2.13.0

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v4 05/29] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Tue, Sep 19, 2017 at 03:46:03PM -0500, Brijesh Singh wrote:
> Platform Security Processor (PSP) is part of AMD Secure Processor (AMD-SP),

The Platform...

> PSP is a dedicated processor that provides the support for key management
> commands in a Secure Encrypted Virtualiztion (SEV) mode, along with

Virtualization

Is integrating that spellchecker hard? Because what I do, for example,
is press F7 in vim when I've written the commit message. And F7 is
mapped to:

map  :set spell! spelllang=en_us 
spellfile=~/.vim/spellfile.add:echo "spellcheck: " . strpart("offon", 
3 * , 3)

in my .vimrc

And I'm pretty sure you can do a similar thing with other editors.

> software-based Trusted Executation Environment (TEE) to enable the
> third-party trusted applications.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/Kconfig   |  11 +
>  drivers/crypto/ccp/Makefile  |   1 +
>  drivers/crypto/ccp/psp-dev.c | 111 
> +++
>  drivers/crypto/ccp/psp-dev.h |  61 
>  drivers/crypto/ccp/sp-dev.c  |  32 +
>  drivers/crypto/ccp/sp-dev.h  |  27 ++-
>  drivers/crypto/ccp/sp-pci.c  |  46 ++
>  7 files changed, 288 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/crypto/ccp/psp-dev.c
>  create mode 100644 drivers/crypto/ccp/psp-dev.h
> 
> diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
> index 6d626606b9c5..1d927e13bf31 100644
> --- a/drivers/crypto/ccp/Kconfig
> +++ b/drivers/crypto/ccp/Kconfig
> @@ -32,3 +32,14 @@ config CRYPTO_DEV_CCP_CRYPTO
> Support for using the cryptographic API with the AMD Cryptographic
> Coprocessor. This module supports offload of SHA and AES algorithms.
> If you choose 'M' here, this module will be called ccp_crypto.
> +
> +config CRYPTO_DEV_SP_PSP
> + bool "Platform Security Processor (PSP) device"
> + default y
> + depends on CRYPTO_DEV_CCP_DD

So this last symbol CRYPTO_DEV_CCP_DD is default m and it doesn't depend
on anything. And I'm pretty sure it should depend on CPU_SUP_AMD as this
is AMD-specific hw. You can add that dependency in a prepatch.

And what happened to adding dependencies on CONFIG_KVM_AMD? Or can you
use the PSP without virtualization in any sensible way?

> + help
> +  Provide the support for AMD Platform Security Processor (PSP). PSP is

... for the AMD ... The PSP 
... 

> +  a dedicated processor that provides the support for key management

that provides support for

> +  commands in in a Secure Encrypted Virtualiztion (SEV) mode, along with

... in Secure Encrypted Virtualization

> +  software-based Trusted Executation Environment (TEE) to enable the
> +  third-party trusted applications.
> diff --git a/drivers/crypto/ccp/Makefile b/drivers/crypto/ccp/Makefile
> index 57f8debfcfb3..008bae7e26ec 100644
> --- a/drivers/crypto/ccp/Makefile
> +++ b/drivers/crypto/ccp/Makefile
> @@ -7,6 +7,7 @@ ccp-$(CONFIG_CRYPTO_DEV_SP_CCP) += ccp-dev.o \
>   ccp-dmaengine.o \
>   ccp-debugfs.o
>  ccp-$(CONFIG_PCI) += sp-pci.o
> +ccp-$(CONFIG_CRYPTO_DEV_SP_PSP) += psp-dev.o
>  
>  obj-$(CONFIG_CRYPTO_DEV_CCP_CRYPTO) += ccp-crypto.o
>  ccp-crypto-objs := ccp-crypto-main.o \
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> new file mode 100644
> index ..e60e53272e71
> --- /dev/null
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -0,0 +1,111 @@
> +/*
> + * AMD Platform Security Processor (PSP) interface
> + *
> + * Copyright (C) 2016-2017 Advanced Micro Devices, Inc.
> + *
> + * Author: Brijesh Singh <brijesh.si...@amd.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "sp-dev.h"
> +#include "psp-dev.h"
> +
> +const struct psp_vdata psp_entry = {
> +

Re: [Part2 PATCH v4 05/29] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Sun, Oct 01, 2017 at 03:05:11PM -0500, Brijesh Singh wrote:
> I think theoretically a 32-bit host OS can invoke a PSP commands but
> currently PSP interface is exposing only the SEV FW command. And SEV

Let's cross that bridge when we get to it.

> feature is available when we are in 64-bit mode hence for now its okay
> to have depends on X86_64. I will add CRYPTO_DEV_CCP_DD depend on
> CPU_SUP_AMD || ARM64 and CRYPTO_DEV_SP_PSP depend on X86_64 and send you
> v4.2.

No, please add my patch below to your set for the CRYPTO_DEV_CCP_DD
dependency as it is a separate thing. Your patch should concentrate only
on adding the PSP and its dependencies.

Thx.

---
From: Borislav Petkov <b...@suse.de>
Date: Sat, 30 Sep 2017 10:06:27 +0200
Subject: [PATCH] crypto: ccp: Build the AMD secure processor driver only with
 AMD CPU support

This is AMD-specific hardware so present it in Kconfig only when AMD
CPU support is enabled or on ARM64 where it is also used.

Signed-off-by: Borislav Petkov <b...@suse.de>
Cc: Brijesh Singh <brijesh.si...@amd.com>
Cc: Tom Lendacky <thomas.lenda...@amd.com>
Cc: Gary Hook <gary.h...@amd.com>
Cc: Herbert Xu <herb...@gondor.apana.org.au>
Cc: "David S. Miller" <da...@davemloft.net>
Cc: linux-crypto@vger.kernel.org
---
 drivers/crypto/ccp/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/ccp/Kconfig b/drivers/crypto/ccp/Kconfig
index 627f3e61dcac..f19f57162225 100644
--- a/drivers/crypto/ccp/Kconfig
+++ b/drivers/crypto/ccp/Kconfig
@@ -1,5 +1,6 @@
 config CRYPTO_DEV_CCP_DD
tristate "Secure Processor device driver"
+   depends on CPU_SUP_AMD || ARM64
default m
help
  Provides AMD Secure Processor device driver.
-- 
2.13.0

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5 09/31] crypto: ccp: Build the AMD secure processor driver only with AMD CPU support

[Borislav Petkov]

On Wed, Oct 04, 2017 at 08:13:50AM -0500, Brijesh Singh wrote:
> From: Borislav Petkov <b...@suse.de>
> 
> This is AMD-specific hardware so present it in Kconfig only when AMD
> CPU support is enabled or on ARM64 where it is also used.
> 
> Signed-off-by: <brijesh.si...@amd.com>
> Signed-off-by: Borislav Petkov <b...@suse.de>

For the future, when you carry a patch from someone else, your SOB
should come after the author's. I.e.,

Signed-off-by: Borislav Petkov <b...@suse.de>
Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>

i.e., the first SOB is the author's and the second is yours which means,
you've handled the patch further on, like sending it upstream, for
example.

Thx.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:05:59PM -0500, Brijesh Singh wrote:
> AMD's new Secure Encrypted Virtualization (SEV) feature allows the
> memory contents of virtual machines to be transparently encrypted with a
> key unique to the VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
> commands for these tasks. The complete spec is available at:
> 
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> Extend the AMD-SP driver to provide the following support:
> 
>  - an in-kernel API to communicate with the SEV firmware. The API can be
>used by the hypervisor to create encryption context for a SEV guest.
> 
>  - a userspace IOCTL to manage the platform certificates.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
> 
> Based on Boris feedback split this patch in 9 logical patches, they are
> numbers from 12.1 to 12.9.
> 
>  drivers/crypto/ccp/psp-dev.c | 244 
> +++
>  drivers/crypto/ccp/psp-dev.h |  17 +++
>  include/linux/psp-sev.h  | 159 
>  3 files changed, 420 insertions(+)

A bunch of fixes ontop:

* sev_fops_registered is superfluous if you can use psp->has_sev_fops

* s/sev_handle_cmd/sev_do_cmd/g - it really is sev_do_cmd(). "handle" is
something else.

* Flip misc dev reg logic in sev_ops_init()

* PSP_P2CMSG needs arg eval

* text streamlining

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e9b776c3acb2..f0e0fc1fb512 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -31,7 +31,6 @@
 #define DEVICE_NAME"sev"
 
 static DEFINE_MUTEX(sev_cmd_mutex);
-static bool sev_fops_registered;
 
 static struct psp_device *psp_alloc_struct(struct sp_device *sp)
 {
@@ -121,7 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
return 0;
 }
 
-static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
+static int sev_do_cmd(int cmd, void *data, int *psp_ret)
 {
unsigned int phys_lsb, phys_msb;
struct psp_device *psp;
@@ -182,26 +181,26 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
return -ENOTTY;
 }
 
-const struct file_operations sev_fops = {
+static const struct file_operations sev_fops = {
.owner  = THIS_MODULE,
.unlocked_ioctl = sev_ioctl,
 };
 
 int sev_platform_init(struct sev_data_init *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_INIT, data, error);
+   return sev_do_cmd(SEV_CMD_INIT, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_init);
 
 int sev_platform_shutdown(int *error)
 {
-   return sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, error);
+   return sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_shutdown);
 
 int sev_platform_status(struct sev_data_status *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+   return sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_status);
 
@@ -211,58 +210,58 @@ int sev_issue_cmd_external_user(struct file *filep, 
unsigned int cmd,
if (!filep || filep->f_op != _fops)
return -EBADF;
 
-   return sev_handle_cmd(cmd, data, error);
+   return sev_do_cmd(cmd, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
 
 int sev_guest_deactivate(struct sev_data_deactivate *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_DEACTIVATE, data, error);
+   return sev_do_cmd(SEV_CMD_DEACTIVATE, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_deactivate);
 
 int sev_guest_activate(struct sev_data_activate *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_ACTIVATE, data, error);
+   return sev_do_cmd(SEV_CMD_ACTIVATE, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_activate);
 
 int sev_guest_decommission(struct sev_data_decommission *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_DECOMMISSION, data, error);
+   return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_decommission);
 
 int sev_guest_df_flush(int *error)
 {
-   return sev_handle_cmd(SEV_CMD_DF_FLUSH, 0, error);
+   return sev_do_cmd(SEV_CMD_DF_FLUSH, 0, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_df_flush);
 
 static int sev_ops_init(struct psp_device *psp)
 {
struct

Re: [Part2 PATCH v5.1 12.2/31] crypto: ccp: Define SEV userspace ioctl and command id

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:00PM -0500, Brijesh Singh wrote:
> Add a include file which defines the ioctl and command id used for
> issuing SEV platform management specific commands.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  include/uapi/linux/psp-sev.h | 115 
> +++
>  1 file changed, 115 insertions(+)
>  create mode 100644 include/uapi/linux/psp-sev.h

First of all, thanks for splitting the patch - it is much easier to
review this way.

Then, this patch should be 12.1, i.e., the first of the split because otherwise
the previous one - which should be the next - fails building due to

drivers/crypto/ccp/psp-dev.c:26:32: fatal error: uapi/linux/psp-sev.h: No such 
file or directory
 #include 
^
Just swap them in their order.

Also, those SEV commands should be __packed, see below.

With that addressed:

Reviewed-by: Borislav Petkov <b...@suse.de>

---
diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
index a385bf2b8d2a..b63e116f18c1 100644
--- a/include/uapi/linux/psp-sev.h
+++ b/include/uapi/linux/psp-sev.h
@@ -53,7 +53,7 @@ struct sev_user_data_status {
__u32 config;   /* Out */
__u8 build; /* Out */
__u32 guest_count;  /* Out */
-};
+} __packed;
 
 /**
  * struct sev_user_data_pek_csr - PEK_CSR command parameters
@@ -64,7 +64,7 @@ struct sev_user_data_status {
 struct sev_user_data_pek_csr {
__u64 address;  /* In */
__u32 length;   /* In/Out */
-};
+} __packed;
 
 /**
  * struct sev_user_data_cert_import - PEK_CERT_IMPORT command parameters
@@ -79,7 +79,7 @@ struct sev_user_data_pek_cert_import {
__u32 pek_cert_len; /* In */
__u64 oca_cert_address; /* In */
__u32 oca_cert_len; /* In */
-};
+} __packed;
 
 /**
  * struct sev_user_data_pdh_cert_export - PDH_CERT_EXPORT command parameters
@@ -94,7 +94,7 @@ struct sev_user_data_pdh_cert_export {
__u32 pdh_cert_len; /* In/Out */
__u64 cert_chain_address;   /* In */
__u32 cert_chain_len;   /* In/Out */
-};
+} __packed;
 
 /**
  * struct sev_issue_cmd - SEV ioctl parameters
@@ -107,7 +107,7 @@ struct sev_issue_cmd {
__u32 cmd;  /* In */
__u64 data; /* In */
__u32 error;/* Out */
-};
+} __packed;
 
 #define SEV_IOC_TYPE   'S'
 #define SEV_ISSUE_CMD  _IOWR(SEV_IOC_TYPE, 0x0, struct sev_issue_cmd)

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command

[Borislav Petkov]

On Wed, Oct 11, 2017 at 03:45:00PM -0500, Brijesh Singh wrote:
> OK, if userspace is going to pick bits apart then how about this:
> 
>  struct sev_data_status {
>  u8 api_major;   /* Out */
>  u8 api_minor;   /* Out */
>  u8 state;   /* Out */
>  u32 flags;  /* Out */
>  u8 build;   /* Out */

I guess. Except the spec marks those as 31:24 and belonging to the dword
starting at offset 4 and you've made "build" a separate u8 and the dword
starts at offset 3. I mean, not a big deal, I think you can change the
spec for a change :-)

Because it looks like the 4 bytes starting at offset 3 really are meant
for miscellaneous bits. And then CONFIG.ES could've been bit 1 of the
byte at offset 3. Oh well...

> Please let me know if you are okay with my above structure.

But yeah, in the end of the day it probably doesn't matter a whole lot.
The spec just counts those in 32-bit quantities.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command

[Borislav Petkov]

On Wed, Oct 11, 2017 at 03:10:49PM -0500, Brijesh Singh wrote:
> The current 'struct sev_data_status' matches with the firmware names and the
> bit fields. Only thing I did was the fields with no name is called as
> "reservedX"

Ok, I see it. So what you actually wanna do is:

struct sev_data_status {
u8 api_major;   /* Out */
u8 api_minor;   /* Out */
u8 state;   /* Out */
u8 flags;   /* Out */
u32 config; /* Out */
u32 guest_count;/* Out */
} __packed;

as this is exactly what the firwmare gives you. Theoretically, you
could've also done:

struct sev_data_status {
u64 first_qword;
u32 second_dword;
};

but you have the fields mostly defined already and that would be too
confusing.

What I mean is, once you've gotten the command buffer, then you can pick
fields apart in software.

owner = status.flags & 1;
config_es = status.config & 1;
build = (status.config >> 24) & 0xff;

This way, if new fields get added, you don't have to change the struct
definitions - *especially* if they're visible to userspace - and users
of that struct can be extended to understand the new fields.

And before you copy the struct to userspace, you can simply clear out
the reserved fields as nothing should rely on them having a particular
value, because, well, they're reserved, doh.

Makes sense?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:01PM -0500, Brijesh Singh wrote:
> The SEV_FACTORY_RESET command can be used by the platform owner to
> reset the non-volatile SEV related data. The command is defined in
> SEV spec section 5.4
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)

Some fixes ontop, like, for example, if you hit the default: label of
the switch due to input.cmd being one of the ones in the holes in enum
sev_cmd, you don't need to copy_to_user() in the end.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d3a50f1f737e..ed5a7404b5a5 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -192,19 +192,19 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
return -EINVAL;
 
switch (input.cmd) {
-
-   case SEV_FACTORY_RESET: {
-   ret = sev_handle_cmd(SEV_CMD_FACTORY_RESET, 0, );
+   case SEV_FACTORY_RESET:
+   ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, );
break;
-   }
+
default:
ret = -EINVAL;
-   break;
+   goto out;
}
 
if (copy_to_user(argp, , sizeof(struct sev_issue_cmd)))
ret = -EFAULT;
 
+out:
return ret;
 }

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> Basically we need some variable which is outside the per-device
> structure so that we don't end up creating multiple /dev/sev nodes. If
> needed, I think we can remove 'has_sev_fops' variable from struct
> psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
> struct psp_device. The 'has_sev_fops' is  mainly used in sev_exit(). If
> we decide to dynamic alloc sev_misc then in  sev_exit() we can use
> psp->sev_misc != NULL instead of psp->has_sev_ops.

Yap, that sounds better than an explicit variable.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:02PM -0500, Brijesh Singh wrote:
> The SEV_PLATFORM_STATUS command can be used by the platform owner to
> get the current status of the platform. The command is defined in
> SEV spec section 5.5.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 34 ++
>  1 file changed, 34 insertions(+)

...

> @@ -198,6 +228,10 @@ static long sev_ioctl(struct file *file, unsigned int 
> ioctl, unsigned long arg)
>   ret = sev_handle_cmd(SEV_CMD_FACTORY_RESET, 0, );
>   break;
>   }
> + case SEV_PLATFORM_STATUS: {
> + ret = sev_ioctl_platform_status();
> + break;
> + }

What's with the curly brackets around the case: statements?

Anyway, here are some more improvements:

* you can get rid of the struct copying into out and the bitfields by
doing something like this:

ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, >error);
if (ret)
goto e_free;

/* Clear out reserved fields: */
data->owner  &= BIT(0);
data->config &= BIT(0);

I'm not sure those are the ones you need to clear but you get
the idea - you simply poke holes in the reserved fields before
copying to userspace. If you need a more sophisticated mask, use
GENMASK/GENMASK_ULL.

And then you don't need struct sev_user_data_status and
simply remove the bitfields too.

* Also, a function should have a verb in the name, thus
sev_ioctl_do_platform_status().

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d668045956cb..1479db533da0 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -176,9 +176,8 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
return ret;
 }
 
-static int sev_ioctl_platform_status(struct sev_issue_cmd *argp)
+static int sev_ioctl_do_platform_status(struct sev_issue_cmd *argp)
 {
-   struct sev_user_data_status out;
struct sev_data_status *data;
int ret;
 
@@ -186,19 +185,15 @@ static int sev_ioctl_platform_status(struct sev_issue_cmd 
*argp)
if (!data)
return -ENOMEM;
 
-   ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, >error);
if (ret)
goto e_free;
 
-   out.api_major = data->api_major;
-   out.api_minor = data->api_minor;
-   out.state = data->state;
-   out.owner = data->owner;
-   out.config = data->config;
-   out.build = data->build;
-   out.guest_count = data->guest_count;
-   if (copy_to_user((void __user *)(uintptr_t) argp->data,
-, sizeof(struct sev_user_data_status)))
+   /* Clear out reserved fields: */
+   data->owner  &= BIT(0);
+   data->config &= BIT(0);
+
+   if (copy_to_user((void __user *)argp->data, data, sizeof(*data)))
ret = -EFAULT;
 
 e_free:
@@ -226,10 +221,10 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, );
break;
 
-   case SEV_PLATFORM_STATUS: {
-   ret = sev_ioctl_platform_status();
+   case SEV_PLATFORM_STATUS:
+   ret = sev_ioctl_do_platform_status();
break;
-   }
+
default:
ret = -EINVAL;
goto out;
diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
index 10b843cce75f..223942ba3e7e 100644
--- a/include/linux/psp-sev.h
+++ b/include/linux/psp-sev.h
@@ -144,11 +144,9 @@ struct sev_data_status {
u8 api_major;   /* Out */
u8 api_minor;   /* Out */
u8 state;   /* Out */
-   u8 owner : 1;   /* Out */
-   u8 reserved1 : 7;
-   u32 config : 1; /* Out */
-   u32 reserved2 : 23;
-   u32 build : 8;  /* Out */
+   u8 owner;   /* Out */
+   u32 config; /* Out */
+   u32 build;  /* Out */
u32 guest_count;/* Out */
 } __packed;
 

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command

[Borislav Petkov]

On Wed, Oct 11, 2017 at 02:49:55PM -0500, Brijesh Singh wrote:
> This is OK for now. But in future if FW steals another bit from reserved1
> field to expose a new flag then 'owner' name will no longer be valid. If you
> don't to use bit field then we have to use some generic name instead of
> naming the field as 'owner'. Please note that its not just userspace, KVM
> driver also uses the same fields and it may also need to know which bit
> position to use.

So what is this field called in the spec?

> This is a tricky one. The 32-bit are packed as:
> 
> BIT0 - config.es
> BIT1-23: reserved
> BIT24-31: build

Is that what the firmware gives?

Then it is easy:

 &= GENMASK(23, 1);

and then userspace can pick apart bit 0 and bit24-31.

Just use one raw struct which the firmware gives you and the rest is
done by the sw. Like clearing reserved fields before copying to user.

You don't want to be updating that struct layout later: think of old
qemu with new kernel and all those different configurations.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.4/31] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command

[Borislav Petkov]

On Wed, Oct 11, 2017 at 10:04:44PM +0200, Borislav Petkov wrote:
> Then it is easy:
> 
>&= GENMASK(23, 1);

[1:23] range cleared, of course:

 &= ~GENMASK(23, 1);

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Wed, Oct 11, 2017 at 11:50:30AM -0500, Brijesh Singh wrote:
> AMD's new Secure Encrypted Virtualization (SEV) feature allows the
> memory contents of virtual machines to be transparently encrypted with a
> key unique to the VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
> commands for these tasks. The complete spec is available at:
> 
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> Extend the AMD-SP driver to provide the following support:
> 
>  - an in-kernel API to communicate with the SEV firmware. The API can be
>used by the hypervisor to create encryption context for a SEV guest.
> 
>  - a userspace IOCTL to manage the platform certificates.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
> Make it as a second patch in the series (changes from 12.1 -> 12.2)
> 
> Changes since v5.1:
>  * text streamlining (from Boris)
>  * rename sev_handle_cmd -> sev_do_cmd (from Boris)
>  * PSP_P2CMSG needs arg eval (from Boris)
>  * use #ifdef instead of #if defined() (from Boris)
> 
>  drivers/crypto/ccp/psp-dev.c | 251 
> +++
>  drivers/crypto/ccp/psp-dev.h |  16 +++
>  include/linux/psp-sev.h  | 159 +++
>  3 files changed, 426 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b5789f878560..175cb3c3b8ef 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -23,9 +23,16 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  #include "sp-dev.h"
>  #include "psp-dev.h"
>  
> +#define DEVICE_NAME  "sev"
> +
> +static DEFINE_MUTEX(sev_cmd_mutex);
> +static bool sev_fops_registered;

Well, if you're going to have a global var, why not pull up the misc
device instead?

And mind you, I've moved out this assignments:

+   psp->sev_misc = psp_misc_dev;
+   init_waitqueue_head(>sev_int_queue);
+   dev_info(dev, "registered SEV device\n");

outside of the if-conditional as I'm assuming you want to do this for
each psp device for which sev_ops_init() is called.

Or am I wrong here?

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 175cb3c3b8ef..d50aaa1ca75b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -31,7 +31,7 @@
 #define DEVICE_NAME"sev"
 
 static DEFINE_MUTEX(sev_cmd_mutex);
-static bool sev_fops_registered;
+static struct miscdevice *psp_misc_dev;
 
 static struct psp_device *psp_alloc_struct(struct sp_device *sp)
 {
@@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush);
 static int sev_ops_init(struct psp_device *psp)
 {
struct device *dev = psp->dev;
-   struct miscdevice *misc;
int ret;
 
/*
@@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp)
 * sev_do_cmd() finds the right master device to which to issue the
 * command to the firmware.
 */
-   if (!sev_fops_registered) {
-
-   misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
-   if (!misc)
+   if (!psp_misc_dev) {
+   psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), 
GFP_KERNEL);
+   if (!psp_misc_dev)
return -ENOMEM;
 
-   misc->minor = MISC_DYNAMIC_MINOR;
-   misc->name = DEVICE_NAME;
-   misc->fops = _fops;
+   psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
+   psp_misc_dev->name = DEVICE_NAME;
+   psp_misc_dev->fops = _fops;
 
-   ret = misc_register(misc);
+   ret = misc_register(psp_misc_dev);
if (ret)
return ret;
-
-   sev_fops_registered = true;
-   psp->sev_misc = misc;
-   init_waitqueue_head(>sev_int_queue);
-   dev_info(dev, "registered SEV device\n");
}
 
+   psp->sev_misc = psp_misc_dev;
+   init_waitqueue_head(>sev_int_queue);
+   dev_info(dev, "registered SEV device\n");
+
return 0;
 }
 
@@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp)
 
 static void sev_exit(struct psp_device *psp)
 {
-   if (psp->sev_misc)
-   misc_deregister(psp->sev_misc);
+   if (psp_misc_dev)
+   misc_deregister(psp_misc_dev);
 }
 
 int psp_dev_init(struct sp_device *sp)

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.2 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command

[Borislav Petkov]

On Wed, Oct 11, 2017 at 11:55:21AM -0500, Brijesh Singh wrote:
> The SEV_FACTORY_RESET command can be used by the platform owner to
> reset the non-volatile SEV related data. The command is defined in
> SEV spec section 5.4
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
> 
> Changes since v5.1:
>  * rename sev_handle_cmd -> sev_do_cmd (from Boris)
>  * skip copy_to_user when invalid cmd id is passed (from Boris)
>  * use SEV_MAX instead of SEV_CMD_MAX to check for invalid command
> 
>  drivers/crypto/ccp/psp-dev.c | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 175cb3c3b8ef..a9c885a39910 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -179,7 +179,34 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>  
>  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long 
> arg)
>  {
> - return -ENOTTY;
> + void __user *argp = (void __user *)arg;
> + struct sev_issue_cmd input;
> + int ret = -EFAULT;
> +
> + if (ioctl != SEV_ISSUE_CMD)
> + return -EINVAL;
> +
> + if (copy_from_user(, argp, sizeof(struct sev_issue_cmd)))
> + return -EFAULT;
> +
> + if (input.cmd > SEV_MAX)
> + return -EINVAL;
> +
> + switch (input.cmd) {
> +
> + case SEV_FACTORY_RESET: {
> + ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, );
> + break;
> + }

Those curly braces are still here. Just use my diff I sent you by saving
the mail to a file and doing

$ patch -p1 --dry-run -i /tmp/boris.mail

ontop of this patch. If it applies ok, remove the --dry-run.

Then you can do changes ontop. This way you won't miss changes I've sent
you.

Thx.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:03PM -0500, Brijesh Singh wrote:
> The SEV_PEK_GEN command is used to generate a new Platform Endorsement
> Key (PEK). The command is defined in SEV spec section 5.6.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 68 
> 
>  1 file changed, 68 insertions(+)

Just small fixups. Worth noting is this:

if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);

I think we want to return the result of the *last* command
executed, which, in the do_shutdown=1 case is SEV_CMD_SHUTDOWN, not
SEV_CMD_PEK_GEN.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d02f48e356e8..31045ea7e798 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -211,7 +211,7 @@ static int sev_platform_get_state(int *state, int *error)
if (!data)
return -ENOMEM;
 
-   ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+   ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
if (!ret)
*state = data->state;
 
@@ -228,7 +228,7 @@ static int sev_firmware_init(int *error)
if (!data)
return -ENOMEM;
 
-   rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
+   rc = sev_do_cmd(SEV_CMD_INIT, data, error);
 
kfree(data);
return rc;
@@ -240,8 +240,8 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
int ret, state;
 
/*
-* PEK_GEN command can be issued only when firmware is in INIT state.
-* If firmware is in UNINIT state then we transition it in INIT state
+* The PEK_GEN command can be issued only when the firmware is in INIT
+* state. If it is in UNINIT state then we transition it in INIT state
 * and issue the command.
 */
ret = sev_platform_get_state(, >error);
@@ -258,10 +258,10 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_GEN, 0, >error);
 
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
 
return ret;
 }
@@ -291,10 +291,10 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_ioctl_do_platform_status();
break;
 
-   case SEV_PEK_GEN: {
+   case SEV_PEK_GEN:
ret = sev_ioctl_pek_gen();
break;
-   }
+
default:
ret = -EINVAL;
goto out;

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote:
> The SEV_PDH_GEN command is used to re-generate the Platform
> Diffie-Hellman (PDH) key. The command is defined in SEV spec section
> 5.9.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 03d7bd03ad58..28efb7a9245a 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
>   return ret;
>  }
>  
> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
> +{
> + int ret, state, do_shutdown = 0;
> +
> + /*
> +  * PDH_GEN command can be issued when platform is in INIT or WORKING
> +  * state. If we are in UNINIT state then transition in INIT state
> +  * before issuing the command.
> +  */
> + ret = sev_platform_get_state(, >error);
> + if (ret)
> + return ret;
> +

Why isn't this function doing:

if (state == SEV_STATE_WORKING) {
return -EBUSY;

like the PEK_GEN one?

Because if so, you can convert it and the PEK_GEN one into a single
function doing the work and wrappers handing in the command to avoid the
code duplication.

> + if (state == SEV_STATE_UNINIT) {
> + ret = sev_firmware_init(>error);
> + if (ret)
> + return ret;
> + do_shutdown = 1;
> + }
> +
> + ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, >error);
> +
> + if (do_shutdown)
> + sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +
> + return ret;
> +}
> +
>  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long 
> arg)
>  {
>   void __user *argp = (void __user *)arg;
> @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int 
> ioctl, unsigned long arg)
>   ret = sev_ioctl_pek_gen();
>   break;
>   }
> + case SEV_PDH_GEN: {
> + ret = sev_ioctl_pdh_gen();
> + break;
> + }

And those curly braces can go, as before.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:06:05PM -0500, Brijesh Singh wrote:
> The SEV_PEK_CSR command can be used to generate a PEK certificate
> signing request. The command is defined in SEV spec section 5.7.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 85 
> 
>  1 file changed, 85 insertions(+)

Ok, a couple of things here:

* Move the checks first and the allocations second so that you allocate
memory only after all checks have been passed and you don't allocate
pointlessly.

* That:

if (state == SEV_STATE_WORKING) {
ret = -EBUSY;
goto e_free_blob;
} else if (state == SEV_STATE_UNINIT) {
ret = sev_firmware_init(>error);
if (ret)
goto e_free_blob;
do_shutdown = 1;
}

is a repeating pattern. Perhaps it should be called
sev_firmware_reinit() and called by other functions.

* The rest is simplifications and streamlining.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..d41f5448a25b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
int ret, state;
void *blob;
 
-   if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
-  sizeof(struct sev_user_data_pek_csr)))
+   if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
+   return -EFAULT;
+
+   if (!input.address)
+   return -EINVAL;
+
+   /* allocate a physically contiguous buffer to store the CSR blob */
+   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+   input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
 
-   /* allocate a temporary physical contigous buffer to store the CSR blob 
*/
-   blob = NULL;
-   if (input.address) {
-   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
-   input.length > SEV_FW_BLOB_MAX_SIZE) {
-   ret = -EFAULT;
-   goto e_free;
-   }
-
-   blob = kmalloc(input.length, GFP_KERNEL);
-   if (!blob) {
-   ret = -ENOMEM;
-   goto e_free;
-   }
-
-   data->address = __psp_pa(blob);
-   data->len = input.length;
+   blob = kmalloc(input.length, GFP_KERNEL);
+   if (!blob) {
+   ret = -ENOMEM;
+   goto e_free;
}
 
+   data->address = __psp_pa(blob);
+   data->len = input.length;
+
ret = sev_platform_get_state(, >error);
if (ret)
goto e_free_blob;
@@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
 
input.length = data->len;
 
/* copy blob to userspace */
-   if (blob &&
-   copy_to_user((void __user *)(uintptr_t)input.address,
-   blob, input.length)) {
+   if (copy_to_user((void __user *)input.address, blob, input.length)) {
ret = -EFAULT;
goto e_shutdown;
}
 
-   if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
-sizeof(struct sev_user_data_pek_csr)))
+   if (copy_to_user((void __user *)argp->data, , sizeof(input)))
ret = -EFAULT;
 
 e_shutdown:
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
 e_free_blob:
kfree(blob);
 e_free:
@@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen();
break;
 
-   case SEV_PEK_CSR: {
+   case SEV_PEK_CSR:
ret = sev_ioctl_pek_csr();
break;
-   }
+
default:
ret = -EINVAL;
goto out;

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command

[Borislav Petkov]

On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
> Lets  consider this scenario
> 1- platform is in uninit state, we transition it to INIT
> 2- PEK_GEN command failed
> 3- since we have transitioned the platform in INIT state hence we must
> call the shutdown otherwise we will leave the system in wrong state. The
> shutdown command will most probably succeed and we will look the ret value

Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
the shutdown command fails?

You probably should carve out the whole shutdown order in separate
functions. I mean, the sequences do repeat in a couple of functions so
you could do:

ioctl:

case :

init_platform()
do_main_cmd()
shutdown_platform()
break;

and this way you have everything nicely separated and retvals properly
tracked...

Hmmm?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command

[Borislav Petkov]

On Thu, Oct 12, 2017 at 03:21:04PM -0500, Brijesh Singh wrote:
> We need to follow the platform state machine logic defined in SEV spec
> section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING
> state because the command actually re-generate the identity of the
> platform itself (in other words re-generate the Platform Endorsement
> Key). Whereas, the PDH_GEN command is used for re-generating Platform
> Diffie-Hellman Key which can be changed while the guest is running.

I see.

So the proposition to carve out and split the platform *init commands
might come in handy here too...

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote:
> The sev_exit() will be called for all the psp_device instance. we need
> to set psp_misc_dev = NULL after deregistering the device.
> 
> if (psp_misc_dev) {
>   misc_deregister(psp_misc_dev);
>    psp_misc_dev = NULL;

Right, except we assign that misc device to every psp device:

psp->sev_misc = psp_misc_dev;

so the question now is, how do you want this to work: the misc device
should be destroyed after the last psp device is destroyed or how?

Because after the first:

psp_misc_dev = NULL;

the respective psp->sev_misc will still keep references to that device
but the device itself will be already deregistered. And that sounds
strange.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.2 12.2/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Thu, Oct 12, 2017 at 04:52:32PM -0500, Brijesh Singh wrote:
> See my above comment, I think the simplest solution is remove psp->sev_misc

Ok, so far so good.

But now you still need to track which is the last psp device and to call
misc_deregister() only when the last device exits. Because if you do
that for the first psp device as it is now, all the following devices
will see a deregistered misc device. And I don't think we want that.

You probably could do something with reference counting: Documentation/kref.txt
to track that and have the last device deregister the misc device.

Or have the "enclosing" sp-dev deregister the misc device when it is
exiting and when it is sure that there are no more psp devices...

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v6 14/38] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command

[Borislav Petkov]

On Thu, Oct 19, 2017 at 09:33:49PM -0500, Brijesh Singh wrote:
> The SEV_FACTORY_RESET command can be used by the platform owner to
> reset the non-volatile SEV related data. The command is defined in
> SEV spec section 5.4
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 28 +++-
>  1 file changed, 27 insertions(+), 1 deletion(-)

Reviewed-by: Borislav Petkov <b...@suse.de>

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [Part2 PATCH v6 15/38] crypto: ccp: Implement SEV_PLATFORM_STATUS ioctl command

[Borislav Petkov]

On Thu, Oct 19, 2017 at 09:33:50PM -0500, Brijesh Singh wrote:
> The SEV_PLATFORM_STATUS command can be used by the platform owner to
> get the current status of the platform. The command is defined in
> SEV spec section 5.5.
> 
> Cc: Paolo Bonzini <pbonz...@redhat.com>
> Cc: "Radim Krčmář" <rkrc...@redhat.com>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Gary Hook <gary.h...@amd.com>
> Cc: Tom Lendacky <thomas.lenda...@amd.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov <b...@suse.de>
> Signed-off-by: Brijesh Singh <brijesh.si...@amd.com>
> ---
>  drivers/crypto/ccp/psp-dev.c | 24 
>  1 file changed, 24 insertions(+)

Reviewed-by: Borislav Petkov <b...@suse.de>

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Thu, Oct 19, 2017 at 09:33:48PM -0500, Brijesh Singh wrote:
> +static int __sev_platform_init(struct sev_data_init *data, int *error)
> +{
> + int rc = 0;
> +
> + mutex_lock(_init_mutex);
> +
> + if (!fw_init_count) {

I still don't like global semaphores. Can you get the status and check
for PSTATE.INIT state and do the init only if the platform is in
PSTATE.UNINIT?

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [Part2 PATCH v6 16/38] crypto: ccp: Implement SEV_PEK_GEN ioctl command

[Borislav Petkov]

On Mon, Oct 23, 2017 at 07:15:30AM -0500, Brijesh Singh wrote:
> I am not sure if I am able to understand your feedback. The
> sev_platform_shutdown() is called unconditionally.

How's that:

If sev_do_cmd() fails and sev_do_cmd(SEV_CMD_SHUTDOWN, ...) in
sev_platform_shutdown() fails, then the first ret from sev_do_cmd() is
gone.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--