| Message ID | 20200214222658.12946-25-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: Add support for protected VMs | expand |
On 14.02.20 23:26, Christian Borntraeger wrote: > From: Janosch Frank <frankja@linux.ibm.com> > > Save response to sidad and disable address checking for protected > guests. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Cornelia Huck <cohuck@redhat.com> > [borntraeger@de.ibm.com: patch merging, splitting, fixing] > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/priv.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index ed52ffa8d5d4..b2de7dc5f58d 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) > > operand2 = kvm_s390_get_base_disp_s(vcpu, &ar); > > - if (operand2 & 0xfff) > + if (!kvm_s390_pv_is_protected(vcpu->kvm) && (operand2 & 0xfff)) > return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); Why is that needed? I'd assume the hardware handles this for us and this case can never happen for PV? (IOW, change is not necessary) > > switch (fc) { > @@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu) > handle_stsi_3_2_2(vcpu, (void *) mem); > break; > } > - > - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); > + if (kvm_s390_pv_is_protected(vcpu->kvm)) { > + memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, > + PAGE_SIZE); > + rc = 0; > + } else { > + rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); > + } > if (rc) { > rc = kvm_s390_inject_prog_cond(vcpu, rc); > goto out; > I'd pull the interrupt injection into the else case, makes things clearer. What about user_stsi? Will that still work? (I assume user space will write to the sida and it should work just fine) (I assume races regarding kvm_s390_pv_is_protected() will be dealt with in your next series) In general, looks good to me.
On 18.02.20 09:35, David Hildenbrand wrote: > On 14.02.20 23:26, Christian Borntraeger wrote: >> From: Janosch Frank <frankja@linux.ibm.com> >> >> Save response to sidad and disable address checking for protected >> guests. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >> [borntraeger@de.ibm.com: patch merging, splitting, fixing] >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/kvm/priv.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >> index ed52ffa8d5d4..b2de7dc5f58d 100644 >> --- a/arch/s390/kvm/priv.c >> +++ b/arch/s390/kvm/priv.c >> @@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >> >> operand2 = kvm_s390_get_base_disp_s(vcpu, &ar); >> >> - if (operand2 & 0xfff) >> + if (!kvm_s390_pv_is_protected(vcpu->kvm) && (operand2 & 0xfff)) >> return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); > > Why is that needed? I'd assume the hardware handles this for us and this > case can never happen for PV? (IOW, change is not necessary) Hardware is handling this for us AND we are not allowed to inject a specification exception. The ultravisor guards the program checks that we are allowed to inject. > >> >> switch (fc) { >> @@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >> handle_stsi_3_2_2(vcpu, (void *) mem); >> break; >> } >> - >> - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >> + if (kvm_s390_pv_is_protected(vcpu->kvm)) { >> + memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, >> + PAGE_SIZE); >> + rc = 0; >> + } else { >> + rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >> + } >> if (rc) { >> rc = kvm_s390_inject_prog_cond(vcpu, rc); >> goto out; >> > > I'd pull the interrupt injection into the else case, makes things clearer. Well, no. Thhe else case could set rc to 0. > > What about user_stsi? Will that still work? (I assume user space will > write to the sida and it should work just fine) will still work. > > (I assume races regarding kvm_s390_pv_is_protected() will be dealt with > in your next series) > > In general, looks good to me. >
On 18.02.20 09:44, Christian Borntraeger wrote: > > > On 18.02.20 09:35, David Hildenbrand wrote: >> On 14.02.20 23:26, Christian Borntraeger wrote: >>> From: Janosch Frank <frankja@linux.ibm.com> >>> >>> Save response to sidad and disable address checking for protected >>> guests. >>> >>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>> [borntraeger@de.ibm.com: patch merging, splitting, fixing] >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> arch/s390/kvm/priv.c | 11 ++++++++--- >>> 1 file changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >>> index ed52ffa8d5d4..b2de7dc5f58d 100644 >>> --- a/arch/s390/kvm/priv.c >>> +++ b/arch/s390/kvm/priv.c >>> @@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >>> >>> operand2 = kvm_s390_get_base_disp_s(vcpu, &ar); >>> >>> - if (operand2 & 0xfff) >>> + if (!kvm_s390_pv_is_protected(vcpu->kvm) && (operand2 & 0xfff)) >>> return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); >> >> Why is that needed? I'd assume the hardware handles this for us and this >> case can never happen for PV? (IOW, change is not necessary) > > Hardware is handling this for us AND we are not allowed to inject a specification > exception. The ultravisor guards the program checks that we are allowed to inject. > Yeah, but can this ever trigger without the check? AFAIKs, no. So why add it? (rather add a BUG_ON in kvm_s390_inject_program_int() in case we are in PV mode) >> >>> >>> switch (fc) { >>> @@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >>> handle_stsi_3_2_2(vcpu, (void *) mem); >>> break; >>> } >>> - >>> - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >>> + if (kvm_s390_pv_is_protected(vcpu->kvm)) { >>> + memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, >>> + PAGE_SIZE); >>> + rc = 0; >>> + } else { >>> + rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >>> + } >>> if (rc) { >>> rc = kvm_s390_inject_prog_cond(vcpu, rc); >>> goto out; >>> >> >> I'd pull the interrupt injection into the else case, makes things clearer. > > Well, no. Thhe else case could set rc to 0. Huh?! if (kvm_s390_pv_is_protected(vcpu->kvm)) { memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, rc = 0; } else { rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); if (rc) { rc = kvm_s390_inject_prog_cond(vcpu, rc); goto out; } } What am I missing?
On 18.02.20 10:08, David Hildenbrand wrote: > On 18.02.20 09:44, Christian Borntraeger wrote: >> >> >> On 18.02.20 09:35, David Hildenbrand wrote: >>> On 14.02.20 23:26, Christian Borntraeger wrote: >>>> From: Janosch Frank <frankja@linux.ibm.com> >>>> >>>> Save response to sidad and disable address checking for protected >>>> guests. >>>> >>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com> >>>> [borntraeger@de.ibm.com: patch merging, splitting, fixing] >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>>> --- >>>> arch/s390/kvm/priv.c | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >>>> index ed52ffa8d5d4..b2de7dc5f58d 100644 >>>> --- a/arch/s390/kvm/priv.c >>>> +++ b/arch/s390/kvm/priv.c >>>> @@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >>>> >>>> operand2 = kvm_s390_get_base_disp_s(vcpu, &ar); >>>> >>>> - if (operand2 & 0xfff) >>>> + if (!kvm_s390_pv_is_protected(vcpu->kvm) && (operand2 & 0xfff)) >>>> return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); >>> >>> Why is that needed? I'd assume the hardware handles this for us and this >>> case can never happen for PV? (IOW, change is not necessary) >> >> Hardware is handling this for us AND we are not allowed to inject a specification >> exception. The ultravisor guards the program checks that we are allowed to inject. >> > > Yeah, but can this ever trigger without the check? AFAIKs, no. So why > add it? It can. the GPRS can contain stale data and so can operand2. > > (rather add a BUG_ON in kvm_s390_inject_program_int() in case we are in > PV mode) > >>> >>>> >>>> switch (fc) { >>>> @@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >>>> handle_stsi_3_2_2(vcpu, (void *) mem); >>>> break; >>>> } >>>> - >>>> - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >>>> + if (kvm_s390_pv_is_protected(vcpu->kvm)) { >>>> + memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, >>>> + PAGE_SIZE); >>>> + rc = 0; >>>> + } else { >>>> + rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >>>> + } >>>> if (rc) { >>>> rc = kvm_s390_inject_prog_cond(vcpu, rc); >>>> goto out; >>>> >>> >>> I'd pull the interrupt injection into the else case, makes things clearer. >> >> Well, no. Thhe else case could set rc to 0. > > Huh?! > > if (kvm_s390_pv_is_protected(vcpu->kvm)) { > memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, > rc = 0; > } else { > rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); > if (rc) { > rc = kvm_s390_inject_prog_cond(vcpu, rc); > goto out; > } > } > Hmm, I find that one harder to read.
>> Yeah, but can this ever trigger without the check? AFAIKs, no. So why >> add it? > > It can. the GPRS can contain stale data and so can operand2. > Oh, ok. Makes sense. >> >> (rather add a BUG_ON in kvm_s390_inject_program_int() in case we are in >> PV mode) >> >>>> >>>>> >>>>> switch (fc) { >>>>> @@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu) >>>>> handle_stsi_3_2_2(vcpu, (void *) mem); >>>>> break; >>>>> } >>>>> - >>>>> - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >>>>> + if (kvm_s390_pv_is_protected(vcpu->kvm)) { >>>>> + memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, >>>>> + PAGE_SIZE); >>>>> + rc = 0; >>>>> + } else { >>>>> + rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >>>>> + } >>>>> if (rc) { >>>>> rc = kvm_s390_inject_prog_cond(vcpu, rc); >>>>> goto out; >>>>> >>>> >>>> I'd pull the interrupt injection into the else case, makes things clearer. >>> >>> Well, no. Thhe else case could set rc to 0. >> >> Huh?! >> >> if (kvm_s390_pv_is_protected(vcpu->kvm)) { >> memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, >> rc = 0; >> } else { >> rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); >> if (rc) { >> rc = kvm_s390_inject_prog_cond(vcpu, rc); >> goto out; >> } >> } >> > > Hmm, I find that one harder to read. It's clearer that you only inject a program check when not in pv mode ... ;) No strong feelings.
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index ed52ffa8d5d4..b2de7dc5f58d 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -872,7 +872,7 @@ static int handle_stsi(struct kvm_vcpu *vcpu) operand2 = kvm_s390_get_base_disp_s(vcpu, &ar); - if (operand2 & 0xfff) + if (!kvm_s390_pv_is_protected(vcpu->kvm) && (operand2 & 0xfff)) return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION); switch (fc) { @@ -893,8 +893,13 @@ static int handle_stsi(struct kvm_vcpu *vcpu) handle_stsi_3_2_2(vcpu, (void *) mem); break; } - - rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); + if (kvm_s390_pv_is_protected(vcpu->kvm)) { + memcpy((void *)sida_origin(vcpu->arch.sie_block), (void *)mem, + PAGE_SIZE); + rc = 0; + } else { + rc = write_guest(vcpu, operand2, ar, (void *)mem, PAGE_SIZE); + } if (rc) { rc = kvm_s390_inject_prog_cond(vcpu, rc); goto out;