On 2/4/21 5:34 PM, Janosch Frank wrote:
> On 2/2/21 7:00 PM, Claudio Imbrenda wrote:
>> Extend kvm_s390_shadow_fault to return the pointer to the valid leaf
>> DAT table entry, or to the invalid entry.
>>
>> Also return some flags in the lower bits of the address:
>> DAT_PROT: indicates that DAT protection applies because of the
>> protection bit in the segment (or, if EDAT, region) tables
>> NOT_PTE: indicates that the address of the DAT table entry returned
>> does not refer to a PTE, but to a segment or region table.
>>
>> Signed-off-by: Claudio Imbrenda <imbre...@linux.ibm.com>
>> Cc: sta...@vger.kernel.org
>> ---
>> arch/s390/kvm/gaccess.c | 26 ++++++++++++++++++++++----
>> arch/s390/kvm/gaccess.h | 5 ++++-
>> arch/s390/kvm/vsie.c | 8 ++++----
>> 3 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
>> index 6d6b57059493..2d7bcbfb185e 100644
>> --- a/arch/s390/kvm/gaccess.c
>> +++ b/arch/s390/kvm/gaccess.c
>> @@ -1034,6 +1034,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg,
>> unsigned long saddr,
>> rfte.val = ptr;
>> goto shadow_r2t;
>> }
>> + *pgt = ptr + vaddr.rfx * 8;
>
> So pgt either is a table entry if rc > 0 or a pointer to the first pte
> on rc == 0 after this change?
>
> Hrm, if it is really based on RCs than I might be able to come to terms
> with having two things in a ptr with the name pgt. But it needs a
> comment change.
>
>> rc = gmap_read_table(parent, ptr + vaddr.rfx * 8, &rfte.val);
>> if (rc)
>> return rc;
>> @@ -1060,6 +1061,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg,
>> unsigned long saddr,
>> rste.val = ptr;
>> goto shadow_r3t;
>> }
>> + *pgt = ptr + vaddr.rsx * 8;
>> rc = gmap_read_table(parent, ptr + vaddr.rsx * 8, &rste.val);
>> if (rc)
>> return rc;
>> @@ -1087,6 +1089,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg,
>> unsigned long saddr,
>> rtte.val = ptr;
>> goto shadow_sgt;
>> }
>> + *pgt = ptr + vaddr.rtx * 8;
>> rc = gmap_read_table(parent, ptr + vaddr.rtx * 8, &rtte.val);
>> if (rc)
>> return rc;
>> @@ -1123,6 +1126,7 @@ static int kvm_s390_shadow_tables(struct gmap *sg,
>> unsigned long saddr,
>> ste.val = ptr;
>> goto shadow_pgt;
>> }
>> + *pgt = ptr + vaddr.sx * 8;
>> rc = gmap_read_table(parent, ptr + vaddr.sx * 8, &ste.val);
>> if (rc)
>> return rc;
>> @@ -1157,6 +1161,8 @@ static int kvm_s390_shadow_tables(struct gmap *sg,
>> unsigned long saddr,
>> * @vcpu: virtual cpu
>> * @sg: pointer to the shadow guest address space structure
>> * @saddr: faulting address in the shadow gmap
>> + * @pteptr: will contain the address of the faulting DAT table entry, or of
>> + * the valid leaf, plus some flags
>
> pteptr is not the right name if it can be two things
You use it for pei only, right?
>
>> *
>> * Returns: - 0 if the shadow fault was successfully resolved
>> * - > 0 (pgm exception code) on exceptions while faulting
>> @@ -1165,11 +1171,11 @@ static int kvm_s390_shadow_tables(struct gmap *sg,
>> unsigned long saddr,
>> * - -ENOMEM if out of memory
>> */
>> int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *sg,
>> - unsigned long saddr)
>> + unsigned long saddr, unsigned long *pteptr)
>> {
>> union vaddress vaddr;
>> union page_table_entry pte;
>> - unsigned long pgt;
>> + unsigned long pgt = 0;
>> int dat_protection, fake;
>> int rc;
>>
>> @@ -1191,8 +1197,20 @@ int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu,
>> struct gmap *sg,
>> pte.val = pgt + vaddr.px * PAGE_SIZE;
>> goto shadow_page;
>> }
>> - if (!rc)
>> - rc = gmap_read_table(sg->parent, pgt + vaddr.px * 8, &pte.val);
>> +
>> + switch (rc) {
>> + case PGM_SEGMENT_TRANSLATION:
>> + case PGM_REGION_THIRD_TRANS:
>> + case PGM_REGION_SECOND_TRANS:
>> + case PGM_REGION_FIRST_TRANS:
>> + pgt |= NOT_PTE;
>
> GACC_TRANSL_ENTRY_INV ?
>
>> + break;
>> + case 0:
>> + pgt += vaddr.px * 8;
>> + rc = gmap_read_table(sg->parent, pgt, &pte.val);
>> + }
>> + if (*pteptr)
>> + *pteptr = pgt | dat_protection * DAT_PROT;
>> if (!rc && pte.i)
>> rc = PGM_PAGE_TRANSLATION;
>> if (!rc && pte.z)
>> diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
>> index f4c51756c462..66a6e2cec97a 100644
>> --- a/arch/s390/kvm/gaccess.h
>> +++ b/arch/s390/kvm/gaccess.h
>> @@ -359,7 +359,10 @@ void ipte_unlock(struct kvm_vcpu *vcpu);
>> int ipte_lock_held(struct kvm_vcpu *vcpu);
>> int kvm_s390_check_low_addr_prot_real(struct kvm_vcpu *vcpu, unsigned long
>> gra);
>>
>> +#define DAT_PROT 2
>
> GACC_TRANSL_ENTRY_PROT
Ok after a second pass that's not what's going on here.
Those basically directly correspond to the MVPG PEI indication bits, right?
Do we also need to consider bit 63?
>
>> +#define NOT_PTE 4
>> +
>> int kvm_s390_shadow_fault(struct kvm_vcpu *vcpu, struct gmap *shadow,
>> - unsigned long saddr);
>> + unsigned long saddr, unsigned long *pteptr);
>>
>> #endif /* __KVM_S390_GACCESS_H */
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index c5d0a58b2c29..7db022141db3 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -619,10 +619,10 @@ static int map_prefix(struct kvm_vcpu *vcpu, struct
>> vsie_page *vsie_page)
>> /* with mso/msl, the prefix lies at offset *mso* */
>> prefix += scb_s->mso;
>>
>> - rc = kvm_s390_shadow_fault(vcpu, vsie_page->gmap, prefix);
>> + rc = kvm_s390_shadow_fault(vcpu, vsie_page->gmap, prefix, NULL);
>> if (!rc && (scb_s->ecb & ECB_TE))
>> rc = kvm_s390_shadow_fault(vcpu, vsie_page->gmap,
>> - prefix + PAGE_SIZE);
>> + prefix + PAGE_SIZE, NULL);
>> /*
>> * We don't have to mprotect, we will be called for all unshadows.
>> * SIE will detect if protection applies and trigger a validity.
>> @@ -913,7 +913,7 @@ static int handle_fault(struct kvm_vcpu *vcpu, struct
>> vsie_page *vsie_page)
>> current->thread.gmap_addr, 1);
>>
>> rc = kvm_s390_shadow_fault(vcpu, vsie_page->gmap,
>> - current->thread.gmap_addr);
>> + current->thread.gmap_addr, NULL);
>> if (rc > 0) {
>> rc = inject_fault(vcpu, rc,
>> current->thread.gmap_addr,
>> @@ -935,7 +935,7 @@ static void handle_last_fault(struct kvm_vcpu *vcpu,
>> {
>> if (vsie_page->fault_addr)
>> kvm_s390_shadow_fault(vcpu, vsie_page->gmap,
>> - vsie_page->fault_addr);
>> + vsie_page->fault_addr, NULL);
>
> Ok
>
>> vsie_page->fault_addr = 0;
>> }
>>
>>
>
