On 09/09/2016 12:23 PM, Borislav Petkov wrote:
> On Mon, Aug 22, 2016 at 05:37:49PM -0500, Tom Lendacky wrote:
>> This patch adds support to be change the memory encryption attribute for
>> one or more memory pages.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> ---
>> arch/x86/include/asm/cacheflush.h | 3 +
>> arch/x86/include/asm/mem_encrypt.h | 13 ++++++
>> arch/x86/mm/mem_encrypt.c | 43 +++++++++++++++++++++
>> arch/x86/mm/pageattr.c | 75 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 134 insertions(+)
>
> ...
>
>> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
>> index 72c292d..0ba9382 100644
>> --- a/arch/x86/mm/pageattr.c
>> +++ b/arch/x86/mm/pageattr.c
>> @@ -1728,6 +1728,81 @@ int set_memory_4k(unsigned long addr, int numpages)
>> __pgprot(0), 1, 0, NULL);
>> }
>>
>> +static int __set_memory_enc_dec(struct cpa_data *cpa)
>> +{
>> + unsigned long addr;
>> + int numpages;
>> + int ret;
>> +
>> + if (*cpa->vaddr & ~PAGE_MASK) {
>> + *cpa->vaddr &= PAGE_MASK;
>> +
>> + /* People should not be passing in unaligned addresses */
>> + WARN_ON_ONCE(1);
>
> Let's make this more user-friendly:
>
> if (WARN_ONCE(*cpa->vaddr & ~PAGE_MASK, "Misaligned address: 0x%lx\n", *cpa->vaddr))
> *cpa->vaddr &= PAGE_MASK;
Will do.
>
>> + }
>> +
>> + addr = *cpa->vaddr;
>> + numpages = cpa->numpages;
>> +
>> + /* Must avoid aliasing mappings in the highmem code */
>> + kmap_flush_unused();
>> + vm_unmap_aliases();
>> +
>> + ret = __change_page_attr_set_clr(cpa, 1);
>> +
>> + /* Check whether we really changed something */
>> + if (!(cpa->flags & CPA_FLUSHTLB))
>> + goto out;
>> +
>> + /*
>> + * On success we use CLFLUSH, when the CPU supports it to
>> + * avoid the WBINVD.
>> + */
>> + if (!ret && static_cpu_has(X86_FEATURE_CLFLUSH))
>> + cpa_flush_range(addr, numpages, 1);
>> + else
>> + cpa_flush_all(1);
>
> So if we fail (ret != 0) we do WBINVD unconditionally even if we don't
> have to?
Looking at __change_page_attr_set_clr() isn't it possible for some of
the pages to be changed before an error is encountered since it is
looping? If so, we may still need to flush. The CPA_FLUSHTLB flag
should take care of a failing case where no attributes have actually
been changed.
Thanks,
Tom
>
> Don't you want this instead:
>
> ret = __change_page_attr_set_clr(cpa, 1);
> if (ret)
> goto out;
>
> /* Check whether we really changed something */
> if (!(cpa->flags & CPA_FLUSHTLB))
> goto out;
>
> /*
> * On success we use CLFLUSH, when the CPU supports it to
> * avoid the WBINVD.
> */
> if (static_cpu_has(X86_FEATURE_CLFLUSH))
> cpa_flush_range(addr, numpages, 1);
> else
> cpa_flush_all(1);
>
> out:
> return ret;
> }
>
> ?
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
