On Tue, 4 Feb 2020 11:57:38 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> 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?
correct.
(cc == 0) ⇔ (rc == 1) which is success.
any rc other than 1 sets cc = 1
