| Message ID | 20201015195913.101065-2-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DIAG 318 tests and fix | expand |
On 10/15/20 9:59 PM, Collin Walling wrote: > The DIAGNOSE 0x0318 instruction must be reset on a normal and clear > reset. However, this was missed for the clear reset case. > > Let's fix this by resetting the information during a normal reset. > Since clear reset is a superset of normal reset, the info will > still reset on a clear reset. The architecture really confuses me here but I think we don't want this in the kernel VCPU reset handlers at all. This needs to be reset per VM *NOT* per VCPU. Hence the resets are bound to diag308 and not SIGP. I.e. we need to clear it in QEMU's VM reset handler. It's still early and I have yet to consume my first coffee, am I missing something? > > Signed-off-by: Collin Walling <walling@linux.ibm.com> > --- > arch/s390/kvm/kvm-s390.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 6b74b92c1a58..b0cf8367e261 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu) > vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI; > vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; > memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb)); > + vcpu->run->s.regs.diag318 = 0; > > kvm_clear_async_pf_completion_queue(vcpu); > if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) > @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu) > > regs->etoken = 0; > regs->etoken_extension = 0; > - regs->diag318 = 0; > } > > int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) >
On 16.10.20 09:34, Janosch Frank wrote: > On 10/15/20 9:59 PM, Collin Walling wrote: >> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear >> reset. However, this was missed for the clear reset case. >> >> Let's fix this by resetting the information during a normal reset. >> Since clear reset is a superset of normal reset, the info will >> still reset on a clear reset. > > The architecture really confuses me here but I think we don't want this > in the kernel VCPU reset handlers at all. > > This needs to be reset per VM *NOT* per VCPU. > Hence the resets are bound to diag308 and not SIGP. > > I.e. we need to clear it in QEMU's VM reset handler. > It's still early and I have yet to consume my first coffee, am I missing > something? I agree with Janosch. architecture indicates that this should only be reset for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned to NOT reset the value. > >> >> Signed-off-by: Collin Walling <walling@linux.ibm.com> >> --- >> arch/s390/kvm/kvm-s390.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 6b74b92c1a58..b0cf8367e261 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu) >> vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI; >> vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; >> memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb)); >> + vcpu->run->s.regs.diag318 = 0; >> >> kvm_clear_async_pf_completion_queue(vcpu); >> if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) >> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu) >> >> regs->etoken = 0; >> regs->etoken_extension = 0; >> - regs->diag318 = 0; >> } >> >> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) >> > >
On 10/16/20 3:44 AM, Christian Borntraeger wrote: > > > On 16.10.20 09:34, Janosch Frank wrote: >> On 10/15/20 9:59 PM, Collin Walling wrote: >>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear >>> reset. However, this was missed for the clear reset case. >>> >>> Let's fix this by resetting the information during a normal reset. >>> Since clear reset is a superset of normal reset, the info will >>> still reset on a clear reset. >> >> The architecture really confuses me here but I think we don't want this >> in the kernel VCPU reset handlers at all. >> >> This needs to be reset per VM *NOT* per VCPU. >> Hence the resets are bound to diag308 and not SIGP. >> >> I.e. we need to clear it in QEMU's VM reset handler. >> It's still early and I have yet to consume my first coffee, am I missing >> something? > > I agree with Janosch. architecture indicates that this should only be reset > for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned > to NOT reset the value. > A few questions regarding how resets for diag318 should work here: The AR states that any copies retained by the diag318 should be set to 0 on a clear reset and load normal -- I thought both of those resets were implicitly called by diag308 as well? Should the register used to store diag318 info not be set to 0 *by KVM* then? Should the values be set *by QEMU* and a subsequent sync_regs will ensure things are sane on the KVM side? >> >>> >>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>> --- >>> arch/s390/kvm/kvm-s390.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index 6b74b92c1a58..b0cf8367e261 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu) >>> vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI; >>> vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; >>> memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb)); >>> + vcpu->run->s.regs.diag318 = 0; >>> >>> kvm_clear_async_pf_completion_queue(vcpu); >>> if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) >>> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu) >>> >>> regs->etoken = 0; >>> regs->etoken_extension = 0; >>> - regs->diag318 = 0; >>> } >>> >>> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) >>> >> >>
On 10/22/20 3:43 PM, Collin Walling wrote: > On 10/16/20 3:44 AM, Christian Borntraeger wrote: >> >> >> On 16.10.20 09:34, Janosch Frank wrote: >>> On 10/15/20 9:59 PM, Collin Walling wrote: >>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear >>>> reset. However, this was missed for the clear reset case. >>>> >>>> Let's fix this by resetting the information during a normal reset. >>>> Since clear reset is a superset of normal reset, the info will >>>> still reset on a clear reset. >>> >>> The architecture really confuses me here but I think we don't want this >>> in the kernel VCPU reset handlers at all. >>> >>> This needs to be reset per VM *NOT* per VCPU. >>> Hence the resets are bound to diag308 and not SIGP. >>> >>> I.e. we need to clear it in QEMU's VM reset handler. >>> It's still early and I have yet to consume my first coffee, am I missing >>> something? >> >> I agree with Janosch. architecture indicates that this should only be reset >> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned >> to NOT reset the value. >> > > A few questions regarding how resets for diag318 should work here: > > The AR states that any copies retained by the diag318 should be set to 0 > on a clear reset and load normal -- I thought both of those resets were > implicitly called by diag308 as well? > > Should the register used to store diag318 info not be set to 0 *by KVM* > then? Should the values be set *by QEMU* and a subsequent sync_regs will > ensure things are sane on the KVM side? Just a FYI for the non-IBMers reading in: I have spoken to the author of the architecture and cleared up our way forward. * We need to clear on diag 308 subcodes 0,1,3,4 * SIGP does not alter diag318 data in any way * We need to set the cpnc on all VCPUs of the VM > >>> >>>> >>>> Signed-off-by: Collin Walling <walling@linux.ibm.com> >>>> --- >>>> arch/s390/kvm/kvm-s390.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index 6b74b92c1a58..b0cf8367e261 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu) >>>> vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI; >>>> vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; >>>> memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb)); >>>> + vcpu->run->s.regs.diag318 = 0; >>>> >>>> kvm_clear_async_pf_completion_queue(vcpu); >>>> if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) >>>> @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu) >>>> >>>> regs->etoken = 0; >>>> regs->etoken_extension = 0; >>>> - regs->diag318 = 0; >>>> } >>>> >>>> int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs) >>>> >>> >>> > >
On 10/23/20 3:25 AM, Janosch Frank wrote: > On 10/22/20 3:43 PM, Collin Walling wrote: >> On 10/16/20 3:44 AM, Christian Borntraeger wrote: >>> >>> >>> On 16.10.20 09:34, Janosch Frank wrote: >>>> On 10/15/20 9:59 PM, Collin Walling wrote: >>>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear >>>>> reset. However, this was missed for the clear reset case. >>>>> >>>>> Let's fix this by resetting the information during a normal reset. >>>>> Since clear reset is a superset of normal reset, the info will >>>>> still reset on a clear reset. >>>> >>>> The architecture really confuses me here but I think we don't want this >>>> in the kernel VCPU reset handlers at all. >>>> >>>> This needs to be reset per VM *NOT* per VCPU. >>>> Hence the resets are bound to diag308 and not SIGP. >>>> >>>> I.e. we need to clear it in QEMU's VM reset handler. >>>> It's still early and I have yet to consume my first coffee, am I missing >>>> something? >>> >>> I agree with Janosch. architecture indicates that this should only be reset >>> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned >>> to NOT reset the value. >>> >> >> A few questions regarding how resets for diag318 should work here: >> >> The AR states that any copies retained by the diag318 should be set to 0 >> on a clear reset and load normal -- I thought both of those resets were >> implicitly called by diag308 as well? >> >> Should the register used to store diag318 info not be set to 0 *by KVM* >> then? Should the values be set *by QEMU* and a subsequent sync_regs will >> ensure things are sane on the KVM side? > > Just a FYI for the non-IBMers reading in: > > I have spoken to the author of the architecture and cleared up our way > forward. > > * We need to clear on diag 308 subcodes 0,1,3,4 > * SIGP does not alter diag318 data in any way Just to make sure I fully understand the changes required here (since the wording in the documentation is a bit tricky) -- this means I need to remove the kvm_arch_vcpu_ioctl_*_reset code for the diag318 data and instead implement a way for this to be reset via 308 only? If true, then resetting this data becomes tricky with the current implementation... > * We need to set the cpnc on all VCPUs of the VM > > This begs a few questions about the current design of the diag318 code. Since the diag318 info (CPNC and CPVC) are *attributes of the configuration*, then is the current implementation of the diag318 handling incorrect? Right now, the diag318 data is synced per-vcpu via a sync_regs call, and only the CPU involved in the instruction interception will have its diag318 register synced. However, if we shouldn't use the kvm_vcpu_ioctl functions to reset the diag318 data, then does it make sense to use the sync_regs interface? There are two approaches I can think of: 1. Modify the current implementation by "dirtying all the VCPU's registers" -- this will ensure the CPNC is set in all SIE blocks. 2. Fallback to the old implementation of this item and use an ioctl (only for setting the data) This new ioctl can be utilized to reset the diag318 data (including the copy retained by QEMU for migration) during the respective 308 resets. Any insights before I fall down the wrong rabbit hole? [...]
On 10/28/20 7:06 AM, Collin Walling wrote: > On 10/23/20 3:25 AM, Janosch Frank wrote: >> On 10/22/20 3:43 PM, Collin Walling wrote: >>> On 10/16/20 3:44 AM, Christian Borntraeger wrote: >>>> >>>> >>>> On 16.10.20 09:34, Janosch Frank wrote: >>>>> On 10/15/20 9:59 PM, Collin Walling wrote: >>>>>> The DIAGNOSE 0x0318 instruction must be reset on a normal and clear >>>>>> reset. However, this was missed for the clear reset case. >>>>>> >>>>>> Let's fix this by resetting the information during a normal reset. >>>>>> Since clear reset is a superset of normal reset, the info will >>>>>> still reset on a clear reset. >>>>> >>>>> The architecture really confuses me here but I think we don't want this >>>>> in the kernel VCPU reset handlers at all. >>>>> >>>>> This needs to be reset per VM *NOT* per VCPU. >>>>> Hence the resets are bound to diag308 and not SIGP. >>>>> >>>>> I.e. we need to clear it in QEMU's VM reset handler. >>>>> It's still early and I have yet to consume my first coffee, am I missing >>>>> something? >>>> >>>> I agree with Janosch. architecture indicates that this should only be reset >>>> for VM-wide resets, e.g. sigp orders 11 and 12 are explicitly mentioned >>>> to NOT reset the value. >>>> >>> >>> A few questions regarding how resets for diag318 should work here: >>> >>> The AR states that any copies retained by the diag318 should be set to 0 >>> on a clear reset and load normal -- I thought both of those resets were >>> implicitly called by diag308 as well? >>> >>> Should the register used to store diag318 info not be set to 0 *by KVM* >>> then? Should the values be set *by QEMU* and a subsequent sync_regs will >>> ensure things are sane on the KVM side? >> >> Just a FYI for the non-IBMers reading in: >> >> I have spoken to the author of the architecture and cleared up our way >> forward. >> >> * We need to clear on diag 308 subcodes 0,1,3,4 >> * SIGP does not alter diag318 data in any way > > Just to make sure I fully understand the changes required here (since > the wording in the documentation is a bit tricky) -- this means I need > to remove the kvm_arch_vcpu_ioctl_*_reset code for the diag318 data and > instead implement a way for this to be reset via 308 only? Yes > > If true, then resetting this data becomes tricky with the current > implementation... Not really, it's just a bit of a pain to iterate through all VCPUs > >> * We need to set the cpnc on all VCPUs of the VM >> >> > > This begs a few questions about the current design of the diag318 code. > > Since the diag318 info (CPNC and CPVC) are *attributes of the > configuration*, then is the current implementation of the diag318 > handling incorrect? > > Right now, the diag318 data is synced per-vcpu via a sync_regs call, and > only the CPU involved in the instruction interception will have its > diag318 register synced. However, if we shouldn't use the kvm_vcpu_ioctl > functions to reset the diag318 data, then does it make sense to use the > sync_regs interface? > > There are two approaches I can think of: > > 1. Modify the current implementation by "dirtying all the VCPU's > registers" -- this will ensure the CPNC is set in all SIE blocks. > > 2. Fallback to the old implementation of this item and use an ioctl > (only for setting the data) > > This new ioctl can be utilized to reset the diag318 data (including > the copy retained by QEMU for migration) during the respective 308 > resets. I don't think that we can tear down old IOCTLs that easily, we have to keep the API consistent. Luckily a diag 308 reset also means VCPU resets, so all VCPUs are out of SIE and QEMU will change a lot of the sync reg contents anyway. The tricky part is the instruction itself as synchronizing the CPNC change to all VCPUs will not be easy. I'd suggest you start with the d308 changes and I'll help you next week after the KVM Forum @Christian: Any other thoughts on this? At the end of the year I'll take a day to find out how we went so wrong with this item... > > Any insights before I fall down the wrong rabbit hole? > > [...] >
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 6b74b92c1a58..b0cf8367e261 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -3516,6 +3516,7 @@ static void kvm_arch_vcpu_ioctl_normal_reset(struct kvm_vcpu *vcpu) vcpu->arch.sie_block->gpsw.mask &= ~PSW_MASK_RI; vcpu->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; memset(vcpu->run->s.regs.riccb, 0, sizeof(vcpu->run->s.regs.riccb)); + vcpu->run->s.regs.diag318 = 0; kvm_clear_async_pf_completion_queue(vcpu); if (!kvm_s390_user_cpu_state_ctrl(vcpu->kvm)) @@ -3582,7 +3583,6 @@ static void kvm_arch_vcpu_ioctl_clear_reset(struct kvm_vcpu *vcpu) regs->etoken = 0; regs->etoken_extension = 0; - regs->diag318 = 0; } int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
The DIAGNOSE 0x0318 instruction must be reset on a normal and clear reset. However, this was missed for the clear reset case. Let's fix this by resetting the information during a normal reset. Since clear reset is a superset of normal reset, the info will still reset on a clear reset. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- arch/s390/kvm/kvm-s390.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)