Re: [RFC PATCH 08/15] drivers/acrn: add VM memory management for ACRN char device

[Borislav Petkov]

On Mon, Aug 19, 2019 at 10:39:58AM +0300, Dan Carpenter wrote:
> On Mon, Aug 19, 2019 at 01:32:54PM +0800, Zhao, Yakui wrote:
> > In fact as this driver is mainly used for embedded IOT usage, it doesn't
> > handle the complex cleanup when such error is encountered. Instead the clean
> > up is handled in free_guest_vm.
> 
> A use after free here seems like a potential security problem.  Security
> matters for IoT...  :(

Yeah, the "S" in "IoT" stands for security.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

[Borislav Petkov]

On Mon, Aug 19, 2019 at 09:44:25AM +0800, Zhao, Yakui wrote:
> Not sure whether it can be sent in two patch sets?
> The first is to add the required APIs for ACRN driver.
> The second is to add the ACRN driver

One patchset adding the APIs and its user(s).

And make sure to refresh on

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

before sending.

Thx.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH 00/15] acrn: add the ACRN driver module

[Borislav Petkov]

On Fri, Aug 16, 2019 at 10:25:41AM +0800, Zhao Yakui wrote:
> The first three patches are the changes under x86/acrn, which adds the
> required APIs for the driver and reports the X2APIC caps. 
> The remaining patches add the ACRN driver module, which accepts the ioctl
> from user-space and then communicate with the low-level ACRN hypervisor
> by using hypercall.

I have a problem with that: you're adding interfaces to arch/x86/ and
its users go into staging. Why? Why not directly put the driver where
it belongs, clean it up properly and submit it like everything else is
submitted?

I don't want to have stuff in arch/x86/ which is used solely by code in
staging and the latter is lingering there indefinitely because no one is
cleaning it up...

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 3/6] kexec: export PG_offline to VMCOREINFO

[Borislav Petkov]

On Thu, Nov 15, 2018 at 01:01:17PM +0100, David Hildenbrand wrote:
> Just saying that "I'm not the first to do it, don't hit me with a stick" :)

:-)

> Indeed. And we still have without makedumpfile. I think you are aware of
> this, but I'll explain it just for consistency: PG_hwpoison

No, I appreciate an explanation very much! So thanks for that. :)

> At some point we detect a HW error and mask a page as PG_hwpoison.
> 
> makedumpfile knows how to treat that flag and can exclude it from the
> dump (== not access it). No crash.
> 
> kdump itself has no clue about old "struct pages". Especially:
> a) Where they are located in memory (e.g. SPARSE)
> b) What their format is ("where are the flags")
> c) What the meaning of flags is ("what does bit X mean")
> 
> In order to know such information, we would have to do parsing of quite
> some information inside the kernel in kdump. Basically what makedumpfile
> does just now. Is this feasible? I don't think so.
> 
> So we would need another approach to communicate such information as you
> said. I can't think of any, but if anybody reading this has an idea,
> please speak up. I am interested.

Yeah but that ship has sailed. And even if we had a great idea, we'd
have to support kdump before and after the idea. And that would be a
serious mess.

And if you have a huge box with gazillion piles of memory and an alpha
particle passes through a bunch of them on its way down to the earth's
core, and while doing so, flips a bunch of bits, you need to go and
collect all those regions and update some list which you then need to
shove into the second kernel.

And you probably need to do all that through perhaps a piece of memory
which is used for communication between first and second kernel and that
list better fit in there, or you need to realloc. And that piece of
memory's layout needs to be properly defined so that the second kernel
can parse it correctly.

And so on...

> The *only* way right now we would have to handle such scenarios:
> 
> 1. While dumping memory and we get a machine check, fake reading a zero
> page instead of crashing.
> 2. While dumping memory and we get a fault, fake reading a zero page
> instead of crashing.

Yap.

> Indeed, and the basic design is to export these flags. (let's say
> "unfortunately", being able to handle such stuff in kdump directly would
> be the dream).

Well, AFAICT, the minimum work you need to always do before starting the
dumping is somehow generate that list of pages or ranges to not dump.
And that work needs to be done by the first or the second kernel, I'd
say.

If the first kernel would do it, then you'd have to probably have
callbacks to certain operations which go and add ranges or pages to
exclude, to a list which is then readily accessible to the second
kernel. Which means, when you reserve memory for the second kernel,
you'd have to reserve memory also for such a list.

But then what do you do when that memory gets filled up...?

So I guess exporting those things in vmcoreinfo is probably the only
thing we *can* do in the end.

Oh well, enough rambling... :)

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 3/6] kexec: export PG_offline to VMCOREINFO

[Borislav Petkov]

On Thu, Nov 15, 2018 at 01:11:02PM +0100, Michal Hocko wrote:
> I am not familiar with kexec to judge this particular patch but we
> cannot simply define any range for these pages (same as for hwpoison
> ones) because they can be almost anywhere in the available memory range.
> Then there can be countless of them. There is no other way to rule them
> out but to check the page state.

I guess, especially if it is a monster box with a lot of memory in it.

> I am not really sure what is the concern here exactly. Kdump is so
> closly tight to the specific kernel version that the api exported
> specifically for its purpose cannot be seriously considered an ABI.
> Kdump has to adopt all the time.

Right...

Except, when people start ogling vmcoreinfo for other things and start
exporting all kinds of kernel internals in there, my alarm bells start
ringing.

But ok, kdump *is* special and I guess that's fine.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 3/6] kexec: export PG_offline to VMCOREINFO

[Borislav Petkov]

On Thu, Nov 15, 2018 at 12:20:40PM +0100, David Hildenbrand wrote:
> Sorry to say, but that is the current practice without which
> makedumpfile would not be able to work at all. (exclude user pages,
> exclude page cache, exclude buddy pages). Let's not reinvent the wheel
> here. This is how dumping works forever.

Sorry, but "we've always done this in the past" doesn't make it better.

> I don't see how there should be "set of pages which do not have
> PG_offline".

It doesn't have to be a set of pages. Think a (mmconfig perhaps) region
which the kdump kernel should completely skip because poking in it in
the kdump kernel, causes all kinds of havoc like machine checks. etc.
We've had and still have one issue like that.

But let me clarify my note: I don't want to be discussing with you the
design of makedumpfile and how it should or should not work - that ship
has already sailed. Apparently there are valid reasons to do it this
way.

I was *simply* stating that it feels wrong to export mm flags like that.

But as I said already, that is mm guys' call and looking at how we're
already exporting a bunch of stuff in the vmcoreinfo - including other
mm flags - I guess one more flag doesn't matter anymore.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH RFC 3/6] kexec: export PG_offline to VMCOREINFO

[Borislav Petkov]

On Thu, Nov 15, 2018 at 02:19:23PM +0800, Dave Young wrote:
> It would be good to copy some background info from cover letter to the
> patch description so that we can get better understanding why this is
> needed now.
> 
> BTW, Lianbo is working on a documentation of the vmcoreinfo exported
> fields. Ccing him so that he is aware of this.
> 
> Also cc Boris,  although I do not want the doc changes blocks this
> he might have different opinion :)

Yeah, my initial reaction is that exporting an mm-internal flag to
userspace is a no-no.

What would be better, IMHO, is having a general method of telling the
kdump kernel - maybe ranges of physical addresses - which to skip.

Because the moment there's a set of pages which do not have PG_offline
set but kdump would still like to skip, this breaks.

But that's mm guys' call.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

HELP WANTED: Fix -Wmissing-prototypes warnings

[Borislav Petkov]

Hi all,

gcc has a warning option

  -Wmissing-prototypes

which fires when a global function is missing a prototype declaration.
This warning is important as it catches cases when that global
function's prototype has been changed but its callers haven't been
updated. And they should be.

Currently, if enabled, this warning triggers ~1400 times for an
allmodconfig build and we would like to have 0 warnings and then add
that option to the main Makefile.

So helping out here would be lovely!

And it is very easy to do: you simply build the kernel with W=1:

make -j W=1 2>w.log

choose one -Wmissing-prototypes warning in the w.log build log and fix
it by including the header which has the function prototype. Or you
declare the function static. If that function prototype is missing, it
needs to be added, of course.

For every function, one should ask oneself, is it better to make the
function static and save ourselves the prototype and include file
dependency - that would be the preferred solution - or if not possible,
should one make the prototype visible by including the proper header so
that gcc sees the prototype.

This should be a good exercise for newbies who'd like to get involved
into kernel development.

Feel free to ask if there are any questions.

Thanks!

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/15] EDAC: make device_type const

[Borislav Petkov]

On Sat, Aug 19, 2017 at 01:52:12PM +0530, Bhumika Goyal wrote:
> Make these const as they are only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
> 
> Signed-off-by: Bhumika Goyal 
> ---
>  drivers/edac/edac_mc_sysfs.c | 8 
>  drivers/edac/i7core_edac.c   | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)

Applied, thanks.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 8/8] ACPI: Use recently introduced uuid_le_cmp_p{p}() helpers

[Borislav Petkov]

On Thu, Apr 27, 2017 at 04:09:56PM +0300, Andy Shevchenko wrote:
> Lukas pointed to this:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68725

Yap, the same thing.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Thu, Apr 06, 2017 at 01:37:41PM -0500, Brijesh Singh wrote:
> I did thought about prot idea but ran into another corner case which may 
> require
> us changing the signature of phys_pud_init and phys_pmd_init. The paddr_start
> and paddr_end args into kernel_physical_mapping_init() should be aligned on 
> PMD
> level down (see comment [1]). So, if we encounter a case where our address 
> range
> is part of large page but we need to clear only one entry (i.e asked to clear 
> just
> one page into 2M region). In that case, now we need to pass additional 
> arguments
> into kernel_physical_mapping, phys_pud_init and phys_pmd_init to hint the 
> splitting
> code that it should use our prot for specific entries and all other entries 
> will use
> the old_prot.

Ok, but your !4K case:

+   /*
+* virtual address is part of large page, create the page
+* table mapping to use smaller pages (4K). The virtual and
+* physical address must be aligned to PMD level.
+*/
+   kernel_physical_mapping_init(__pa(vaddr & PMD_MASK),
+__pa((vaddr_end & PMD_MASK) + 
PMD_SIZE),
+0);


would map a 2M page as encrypted by default. What if we want to map a 2M page
frame as ~_PAGE_ENC?

IOW, if you're adding a new interface, it should be generic enough, like
with prot argument.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

Hi Brijesh,

On Thu, Apr 06, 2017 at 09:05:03AM -0500, Brijesh Singh wrote:
> I looked into arch/x86/mm/init_{32,64}.c and as you pointed the file contains
> routines to do basic page splitting. I think it sufficient for our usage.

Good :)

> I should be able to drop the memblock patch from the series and update the
> Patch 15 [1] to use the kernel_physical_mapping_init().
> 
> The kernel_physical_mapping_init() creates the page table mapping using
> default KERNEL_PAGE attributes, I tried to extend the function by passing
> 'bool enc' flags to hint whether to clr or set _PAGE_ENC when splitting the
> pages. The code did not looked clean hence I dropped that idea.

Or, you could have a

__kernel_physical_mapping_init_prot(..., prot)

helper which gets a protection argument and hands it down. The lower
levels already hand down prot which is good.

The interface kernel_physical_mapping_init() will then itself call:

__kernel_physical_mapping_init_prot(..., PAGE_KERNEL);

for the normal cases.

That in a pre-patch of course.

How does that sound?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Wed, Mar 29, 2017 at 05:21:13PM +0200, Paolo Bonzini wrote:
> The GHCB would have to be allocated much earlier, possibly even by
> firmware depending on how things will be designed.

How about a statically allocated page like we do with the early
pagetable pages in head_64.S?

> I think it's premature to consider SEV-ES requirements.

My only concern is not to have to redo a lot when SEV-ES gets enabled.
So it would be prudent to design with SEV-ES in the back of our minds.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Fri, Mar 17, 2017 at 03:45:26PM +0100, Paolo Bonzini wrote:
> Yes, and I'd like that to be done with a new data section rather than a
> special KVM hook.

Can you give more details about how pls? Or is there already an example for that
somewhere in the kvm code?

> I have no idea.  SEV-ES seems to be very hard to set up at the beginning
> of the kernel bootstrap.  There's all sorts of chicken and egg problems,
> as well as complicated handshakes between the firmware and the guest,
> and the way to do it also depends on the trust and threat models.
> 
> A much simpler way is to just boot under a trusted hypervisor, do
> "modprobe sev-es" and save a snapshot of the guest.  Then you sign the
> snapshot and pass it to your cloud provider.

Right, especially the early trapping could be a pain. I don't think this
is cast in stone yet, though...

We'll see.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Fri, Mar 17, 2017 at 12:03:31PM +0100, Paolo Bonzini wrote:

> If it is possible to do it in a fairly hypervisor-independent manner,
> I'm all for it.  That is, only by looking at AMD-specified CPUID leaves
> and at kernel ELF sections.

Not even that.

What that needs to be able to do is:

kvm_map_percpu_hv_shared(st, sizeof(*st)))

where st is the percpu steal time ptr:

struct kvm_steal_time *st = _cpu(steal_time, cpu);

Underneath, what it does basically is it clears the encryption mask from
the pte, see patch 16/32.

And I keep talking about SEV-ES because this is going to expand on the
need of having a shared memory region which the hypervisor and the guest
needs to access, thus unencrypted. See

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

This is where you come in and say what would be the best approach there...

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Fri, Mar 17, 2017 at 11:47:16AM +0100, Paolo Bonzini wrote:
> Theoretically or practically?

In the sense, it needs to be tried first to see how ugly it can get.

> It only looks at the E820 map, doesn't it?  Why does it have to do
> anything with percpu memory areas?

That's irrelevant. What we want to do is take what's in init_mm.pgd and
modify it. And use the facilities in arch/x86/mm/init_{32,64}.c because
they already know about early/late pagetable pages allocation and they
deal with the kernel pagetable anyway.

And *not* teach pageattr.c about memblock because that can be misused,
as tglx pointed out on IRC.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Thu, Mar 16, 2017 at 11:25:36PM +0100, Paolo Bonzini wrote:
> The kvmclock memory is initially zero so there is no need for the
> hypervisor to allocate anything; the point of these patches is just to
> access the data in a natural way from Linux source code.

I realize that.

> I also don't really like the patch as is (plus it fails modpost), but
> IMO reusing __change_page_attr and __split_large_page is the right thing
> to do.

Right, so teaching pageattr.c about memblock could theoretically come
around and bite us later when a page allocated with memblock gets freed
with free_page().

And looking at this more, we have all this kernel pagetable preparation
code down the init_mem_mapping() call and the pagetable setup in
arch/x86/mm/init_{32,64}.c

And that code even does some basic page splitting. Oh and it uses
alloc_low_pages() which knows whether to do memblock reservation or the
common __get_free_pages() when slabs are up.

So what would be much cleaner, IMHO, is if one would reuse that code to
change init_mm.pgd mappings early without copying pageattr.c.

init_mem_mapping() gets called before kvm_guest_init() in setup_arch()
so the guest would simply fixup its pagetable right there.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Borislav Petkov]

On Fri, Sep 23, 2016 at 09:33:00PM +1200, Kai Huang wrote:
> How is this even possible? The spec clearly says under SEV only in long mode
> or PAE mode guest can control whether memory is encrypted via c-bit, and in
> other modes guest will be always in encrypted mode.

I was suggesting the hypervisor supplies the EFI ranges unencrypted. But
that is not a good idea because firmware data is exposed then, see same
thread from yesterday.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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:04:27PM -0500, Tom Lendacky wrote:
> That's not what I mean here.  If the BIOS sets the SMEE bit in the
> SYS_CFG msr then, even if the encryption bit is never used, there is
> still a reduction in physical address space.

I thought that reduction is the reservation of bits for the SME mask.

What other reduction is there?

> Transparent SME (TSME) will be a BIOS option that will result in the
> memory controller performing encryption no matter what. In this case
> all data will be encrypted without a reduction in physical address
> space.

Now I'm confused: aren't we reducing the address space with the SME
mask?

Or what reduction do you mean?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH v1 05/28] KVM: SVM: prepare for new bit definition in nested_ctl

[Borislav Petkov]

On Mon, Aug 22, 2016 at 07:24:32PM -0400, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lenda...@amd.com>
> 
> Currently the nested_ctl variable in the vmcb_control_area structure is
> used to indicate nested paging support. The nested paging support field
> is actually defined as bit 0 of the this field. In order to support a new
> feature flag the usage of the nested_ctl and nested paging support must
> be converted to operate on a single bit.
> 
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
>  arch/x86/include/asm/svm.h |2 ++
>  arch/x86/kvm/svm.c |7 ---
>  2 files changed, 6 insertions(+), 3 deletions(-)

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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)
-- 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH - resend] checkpatch: Remove unnecessary + after {8,8}

[Borislav Petkov]

Hey Andrew,

can we get this one applied already - I'm seeing this warning at least
since before my summer vacation started! :-P

Thanks.

On Mon, Sep 01, 2014 at 09:55:06AM -0700, Joe Perches wrote:
 There's a useless + use that needs to be removed as perl 5.20
 emits a Useless use of greediness modifier '+' message each
 time it's hit.
 
 Reported-by: Greg KH gre...@linuxfoundation.org
 Signed-off-by: Joe Perches j...@perches.com
 ---
 
 Resending, maybe Andrew's all-seeing eye blinked...
 
 On Fri, 2014-07-11 at 19:05 -0700, Greg KH wrote:
  Ok, with linux-next I get the same thing:
 
 Thanks Greg.
 
  scripts/checkpatch.pl | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
 index d5ac001..370a974 100755
 --- a/scripts/checkpatch.pl
 +++ b/scripts/checkpatch.pl
 @@ -2376,7 +2376,7 @@ sub process {
   please, no space before tabs\n . $herevet) 
   $fix) {
   while ($fixed[$fixlinenr] =~
 -s/(^\+.*) {8,8}+\t/$1\t\t/) {}
 +s/(^\+.*) {8,8}\t/$1\t\t/) {}
   while ($fixed[$fixlinenr] =~
  s/(^\+.*) +\t/$1\t/) {}
   }
 
 
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH - resend] checkpatch: Remove unnecessary + after {8,8}

[Borislav Petkov]

On Thu, Oct 30, 2014 at 12:12:04PM +0100, Borislav Petkov wrote:
 Hey Andrew,
 
 can we get this one applied already - I'm seeing this warning at least
 since before my summer vacation started! :-P

Oh ok, so it did go in after 3.17 but when using checkpatch on a 3.17
tree, it fires of course. Maybe the first checkpatch fix to tag for
stable?

:-)

-- 
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel