On Tue, 4 Feb 2020 12:56:10 +0100
Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
> On 04.02.20 11:57, Cornelia Huck wrote:
>
> >>
> >> /* 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...
>
> I will just use guest_handle as this guests assigned to and from such:
>
> arch/s390/include/asm/gmap.h: unsigned long se_handle;
> arch/s390/include/asm/uv.h: .guest_handle = gmap->se_handle,
> arch/s390/kvm/pv.c: WRITE_ONCE(kvm->arch.gmap->se_handle, 0);
> arch/s390/kvm/pv.c: kvm->arch.gmap->se_handle = uvcb.guest_handle;
> arch/s390/mm/fault.c: .guest_handle = gmap->se_handle,
Even better.
>
>
> >
> >> 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.
>
> I will use is "is_protected"
ok
> >
> >> /*
> >> * 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.
>
> Makes sense. I will wait a bit more but if you do not disagree I will remove
> the extra host config and bind things to CONFIG_KVM or CONFIG_PGSTE depending
> on context
>From the code I read so far, that makes sense.
