On Mon, 3 Feb 2020 08:19:25 -0500
Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
> From: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
>
> This provides the basic ultravisor calls and page table handling to cope
> with secure guests.
I'll leave reviewing the mm stuff to somebody actually familiar with
mm; only some other comments from me.
>
> Co-authored-by: Ulrich Weigand <Ulrich.Weigand@xxxxxxxxxx>
> Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>
> ---
> arch/s390/include/asm/gmap.h | 2 +
> arch/s390/include/asm/mmu.h | 2 +
> arch/s390/include/asm/mmu_context.h | 1 +
> arch/s390/include/asm/page.h | 5 +
> arch/s390/include/asm/pgtable.h | 34 +++++-
> arch/s390/include/asm/uv.h | 59 ++++++++++
> arch/s390/kernel/uv.c | 170 ++++++++++++++++++++++++++++
> 7 files changed, 268 insertions(+), 5 deletions(-)
>
> diff --git a/arch/s390/include/asm/gmap.h b/arch/s390/include/asm/gmap.h
> index 37f96b6f0e61..f2ab8b6d4b57 100644
> --- a/arch/s390/include/asm/gmap.h
> +++ b/arch/s390/include/asm/gmap.h
> @@ -9,6 +9,7 @@
> #ifndef _ASM_S390_GMAP_H
> #define _ASM_S390_GMAP_H
>
> +#include <linux/radix-tree.h>
> #include <linux/refcount.h>
>
> /* Generic bits for GMAP notification on DAT table entry changes. */
> @@ -61,6 +62,7 @@ struct gmap {
> spinlock_t shadow_lock;
> struct gmap *parent;
> unsigned long orig_asce;
> + unsigned long se_handle;
This is a deviation from the "protected virtualization" naming scheme
used in the previous patches, but I understand that the naming of this
whole feature is still in flux :) (Would still be nice to have it
consistent, though.)
However, I think I'd prefer something named based on protected
virtualization: the se_* stuff here just tends to make me think of
SELinux...
> int edat_level;
> bool removed;
> bool initialized;
> diff --git a/arch/s390/include/asm/mmu.h b/arch/s390/include/asm/mmu.h
> index bcfb6371086f..984026cb3608 100644
> --- a/arch/s390/include/asm/mmu.h
> +++ b/arch/s390/include/asm/mmu.h
> @@ -16,6 +16,8 @@ typedef struct {
> unsigned long asce;
> unsigned long asce_limit;
> unsigned long vdso_base;
> + /* The mmu context belongs to a secure guest. */
> + atomic_t is_se;
Here as well.
> /*
> * The following bitfields need a down_write on the mm
> * semaphore when they are written to. As they are only
(...)
> @@ -520,6 +521,15 @@ static inline int mm_has_pgste(struct mm_struct *mm)
> return 0;
> }
>
> +static inline int mm_is_se(struct mm_struct *mm)
> +{
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> + if (unlikely(atomic_read(&mm->context.is_se)))
> + return 1;
> +#endif
> + return 0;
> +}
I'm wondering how big of an overhead we actually have because we need
to check the flag here if the feature is enabled. We have an extra
check in a few functions anyway, even if protected virt is not
configured in. Given that distributions would likely want to enable the
feature in their kernels, I'm currently tending towards dropping the
extra config option.
> +
> static inline int mm_alloc_pgste(struct mm_struct *mm)
> {
> #ifdef CONFIG_PGSTE
(...)
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index f7778493e829..136c60a8e3ca 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -9,6 +9,8 @@
> #include <linux/sizes.h>
> #include <linux/bitmap.h>
> #include <linux/memblock.h>
> +#include <linux/pagemap.h>
> +#include <linux/swap.h>
> #include <asm/facility.h>
> #include <asm/sections.h>
> #include <asm/uv.h>
> @@ -98,4 +100,172 @@ void adjust_to_uv_max(unsigned long *vmax)
> if (prot_virt_host && *vmax > uv_info.max_sec_stor_addr)
> *vmax = uv_info.max_sec_stor_addr;
> }
> +
> +static int __uv_pin_shared(unsigned long paddr)
> +{
> + struct uv_cb_cfs uvcb = {
> + .header.cmd = UVC_CMD_PIN_PAGE_SHARED,
> + .header.len = sizeof(uvcb),
> + .paddr = paddr,
> + };
> +
> + if (uv_call(0, (u64)&uvcb))
> + return -EINVAL;
> + return 0;
I guess this call won't set an error rc in the control block, if it
does not set a condition code != 0?
(...)
