| Message ID | 20200711100434.46660-2-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: pvtime: Fixes and a new cap | expand |
Hi Andrew, On 2020-07-11 11:04, Andrew Jones wrote: > Don't confuse the guest by saying steal-time is supported when > it hasn't been configured by userspace and won't work. > > Signed-off-by: Andrew Jones <drjones@redhat.com> > --- > arch/arm64/kvm/pvtime.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c > index f7b52ce1557e..2b22214909be 100644 > --- a/arch/arm64/kvm/pvtime.c > +++ b/arch/arm64/kvm/pvtime.c > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu > *vcpu) > > switch (feature) { > case ARM_SMCCC_HV_PV_TIME_FEATURES: > - case ARM_SMCCC_HV_PV_TIME_ST: > val = SMCCC_RET_SUCCESS; > break; > + case ARM_SMCCC_HV_PV_TIME_ST: > + if (vcpu->arch.steal.base != GPA_INVALID) > + val = SMCCC_RET_SUCCESS; > + break; > } > > return val; I'm not so sure about this. I have always considered the discovery interface to be "do you know about this SMCCC function". And if you look at the spec, it says (4.2, PV_TIME_FEATURES): <quote> If PV_call_id identifies PV_TIME_FEATURES, this call returns: • NOT_SUPPORTED (-1) to indicate that all paravirtualized time functions in this specification are not supported. • SUCCESS (0) to indicate that all the paravirtualized time functions in this specification are supported. </quote> So the way I understand it, you cannot return "supported" for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for PV_TIME_ST. It applies to *all* features. Yes, this is very bizarre. But I don't think we can deviate from it. Thanks, M.
On Mon, Jul 27, 2020 at 06:25:50PM +0100, Marc Zyngier wrote: > Hi Andrew, > > On 2020-07-11 11:04, Andrew Jones wrote: > > Don't confuse the guest by saying steal-time is supported when > > it hasn't been configured by userspace and won't work. > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > --- > > arch/arm64/kvm/pvtime.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c > > index f7b52ce1557e..2b22214909be 100644 > > --- a/arch/arm64/kvm/pvtime.c > > +++ b/arch/arm64/kvm/pvtime.c > > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) > > > > switch (feature) { > > case ARM_SMCCC_HV_PV_TIME_FEATURES: > > - case ARM_SMCCC_HV_PV_TIME_ST: > > val = SMCCC_RET_SUCCESS; > > break; > > + case ARM_SMCCC_HV_PV_TIME_ST: > > + if (vcpu->arch.steal.base != GPA_INVALID) > > + val = SMCCC_RET_SUCCESS; > > + break; > > } > > > > return val; > > I'm not so sure about this. I have always considered the > discovery interface to be "do you know about this SMCCC > function". And if you look at the spec, it says (4.2, > PV_TIME_FEATURES): > > <quote> > If PV_call_id identifies PV_TIME_FEATURES, this call returns: > • NOT_SUPPORTED (-1) to indicate that all > paravirtualized time functions in this specification are not > supported. > • SUCCESS (0) to indicate that all the paravirtualized time > functions in this specification are supported. > </quote> > > So the way I understand it, you cannot return "supported" > for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for > PV_TIME_ST. It applies to *all* features. > > Yes, this is very bizarre. But I don't think we can deviate > from it. Ah, I see your point. But I wonder if we should drop this patch or if we should change the return of ARM_SMCCC_HV_PV_TIME_FEATURES to be dependant on all the pv calls? Discovery would look like this IF (SMCCC_ARCH_FEATURES, PV_TIME_FEATURES) == 0; THEN IF (PV_TIME_FEATURES, PV_TIME_FEATURES) == 0; THEN PV_TIME_ST is supported, as well as all other PV calls ELIF (PV_TIME_FEATURES, PV_TIME_ST) == 0; THEN PV_TIME_ST is supported ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN <another-pv-call> is supported ... ENDIF ELSE No PV calls are supported ENDIF I believe the above implements a reasonable interpretation of the specification, but the all feature (PV_TIME_FEATURES, PV_TIME_FEATURES) thing is indeed bizarre no matter how you look at it. Thanks, drew
On 2020-07-28 13:55, Andrew Jones wrote: > On Mon, Jul 27, 2020 at 06:25:50PM +0100, Marc Zyngier wrote: >> Hi Andrew, >> >> On 2020-07-11 11:04, Andrew Jones wrote: >> > Don't confuse the guest by saying steal-time is supported when >> > it hasn't been configured by userspace and won't work. >> > >> > Signed-off-by: Andrew Jones <drjones@redhat.com> >> > --- >> > arch/arm64/kvm/pvtime.c | 5 ++++- >> > 1 file changed, 4 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c >> > index f7b52ce1557e..2b22214909be 100644 >> > --- a/arch/arm64/kvm/pvtime.c >> > +++ b/arch/arm64/kvm/pvtime.c >> > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) >> > >> > switch (feature) { >> > case ARM_SMCCC_HV_PV_TIME_FEATURES: >> > - case ARM_SMCCC_HV_PV_TIME_ST: >> > val = SMCCC_RET_SUCCESS; >> > break; >> > + case ARM_SMCCC_HV_PV_TIME_ST: >> > + if (vcpu->arch.steal.base != GPA_INVALID) >> > + val = SMCCC_RET_SUCCESS; >> > + break; >> > } >> > >> > return val; >> >> I'm not so sure about this. I have always considered the >> discovery interface to be "do you know about this SMCCC >> function". And if you look at the spec, it says (4.2, >> PV_TIME_FEATURES): >> >> <quote> >> If PV_call_id identifies PV_TIME_FEATURES, this call returns: >> • NOT_SUPPORTED (-1) to indicate that all >> paravirtualized time functions in this specification are not >> supported. >> • SUCCESS (0) to indicate that all the paravirtualized time >> functions in this specification are supported. >> </quote> >> >> So the way I understand it, you cannot return "supported" >> for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for >> PV_TIME_ST. It applies to *all* features. >> >> Yes, this is very bizarre. But I don't think we can deviate >> from it. > > Ah, I see your point. But I wonder if we should drop this patch > or if we should change the return of ARM_SMCCC_HV_PV_TIME_FEATURES > to be dependant on all the pv calls? > > Discovery would look like this > > IF (SMCCC_ARCH_FEATURES, PV_TIME_FEATURES) == 0; THEN > IF (PV_TIME_FEATURES, PV_TIME_FEATURES) == 0; THEN > PV_TIME_ST is supported, as well as all other PV calls > ELIF (PV_TIME_FEATURES, PV_TIME_ST) == 0; THEN > PV_TIME_ST is supported > ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN > <another-pv-call> is supported > ... > ENDIF > ELSE > No PV calls are supported > ENDIF > > I believe the above implements a reasonable interpretation of the > specification, but the all feature (PV_TIME_FEATURES, PV_TIME_FEATURES) > thing is indeed bizarre no matter how you look at it. It it indeed true to the spec. Thankfully we only support PV_TIME as a feature for now, so we are (sort of) immune to the braindead aspect of the discovery protocol. I think returning a failure when PV_TIME isn't setup is a valid thing to do, as long as it applies to all functions (i.e. something like the below patch). Thanks, M. diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c index f7b52ce1557e..c3ef4ebd6846 100644 --- a/arch/arm64/kvm/pvtime.c +++ b/arch/arm64/kvm/pvtime.c @@ -43,7 +43,8 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) switch (feature) { case ARM_SMCCC_HV_PV_TIME_FEATURES: case ARM_SMCCC_HV_PV_TIME_ST: - val = SMCCC_RET_SUCCESS; + if (vcpu->arch.steal.base != GPA_INVALID) + val = SMCCC_RET_SUCCESS; break; }
On Tue, Jul 28, 2020 at 02:13:54PM +0100, Marc Zyngier wrote: > On 2020-07-28 13:55, Andrew Jones wrote: > > On Mon, Jul 27, 2020 at 06:25:50PM +0100, Marc Zyngier wrote: > > > Hi Andrew, > > > > > > On 2020-07-11 11:04, Andrew Jones wrote: > > > > Don't confuse the guest by saying steal-time is supported when > > > > it hasn't been configured by userspace and won't work. > > > > > > > > Signed-off-by: Andrew Jones <drjones@redhat.com> > > > > --- > > > > arch/arm64/kvm/pvtime.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c > > > > index f7b52ce1557e..2b22214909be 100644 > > > > --- a/arch/arm64/kvm/pvtime.c > > > > +++ b/arch/arm64/kvm/pvtime.c > > > > @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) > > > > > > > > switch (feature) { > > > > case ARM_SMCCC_HV_PV_TIME_FEATURES: > > > > - case ARM_SMCCC_HV_PV_TIME_ST: > > > > val = SMCCC_RET_SUCCESS; > > > > break; > > > > + case ARM_SMCCC_HV_PV_TIME_ST: > > > > + if (vcpu->arch.steal.base != GPA_INVALID) > > > > + val = SMCCC_RET_SUCCESS; > > > > + break; > > > > } > > > > > > > > return val; > > > > > > I'm not so sure about this. I have always considered the > > > discovery interface to be "do you know about this SMCCC > > > function". And if you look at the spec, it says (4.2, > > > PV_TIME_FEATURES): > > > > > > <quote> > > > If PV_call_id identifies PV_TIME_FEATURES, this call returns: > > > • NOT_SUPPORTED (-1) to indicate that all > > > paravirtualized time functions in this specification are not > > > supported. > > > • SUCCESS (0) to indicate that all the paravirtualized time > > > functions in this specification are supported. > > > </quote> > > > > > > So the way I understand it, you cannot return "supported" > > > for PV_TIME_FEATURES, and yet return NOT_SUPPORTED for > > > PV_TIME_ST. It applies to *all* features. > > > > > > Yes, this is very bizarre. But I don't think we can deviate > > > from it. > > > > Ah, I see your point. But I wonder if we should drop this patch > > or if we should change the return of ARM_SMCCC_HV_PV_TIME_FEATURES > > to be dependant on all the pv calls? > > > > Discovery would look like this > > > > IF (SMCCC_ARCH_FEATURES, PV_TIME_FEATURES) == 0; THEN > > IF (PV_TIME_FEATURES, PV_TIME_FEATURES) == 0; THEN > > PV_TIME_ST is supported, as well as all other PV calls > > ELIF (PV_TIME_FEATURES, PV_TIME_ST) == 0; THEN > > PV_TIME_ST is supported > > ELIF (PV_TIME_FEATURES, <another-pv-call>) == 0; THEN > > <another-pv-call> is supported > > ... > > ENDIF > > ELSE > > No PV calls are supported > > ENDIF > > > > I believe the above implements a reasonable interpretation of the > > specification, but the all feature (PV_TIME_FEATURES, PV_TIME_FEATURES) > > thing is indeed bizarre no matter how you look at it. > > It it indeed true to the spec. Thankfully we only support PV_TIME > as a feature for now, so we are (sort of) immune to the braindead > aspect of the discovery protocol. > > I think returning a failure when PV_TIME isn't setup is a valid thing > to do, as long as it applies to all functions (i.e. something like > the below patch). > > Thanks, > > M. > > diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c > index f7b52ce1557e..c3ef4ebd6846 100644 > --- a/arch/arm64/kvm/pvtime.c > +++ b/arch/arm64/kvm/pvtime.c > @@ -43,7 +43,8 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) > switch (feature) { > case ARM_SMCCC_HV_PV_TIME_FEATURES: > case ARM_SMCCC_HV_PV_TIME_ST: > - val = SMCCC_RET_SUCCESS; > + if (vcpu->arch.steal.base != GPA_INVALID) > + val = SMCCC_RET_SUCCESS; > break; > } Looks good to me. I'll do that for v2. Thanks, drew
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c index f7b52ce1557e..2b22214909be 100644 --- a/arch/arm64/kvm/pvtime.c +++ b/arch/arm64/kvm/pvtime.c @@ -42,9 +42,12 @@ long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu) switch (feature) { case ARM_SMCCC_HV_PV_TIME_FEATURES: - case ARM_SMCCC_HV_PV_TIME_ST: val = SMCCC_RET_SUCCESS; break; + case ARM_SMCCC_HV_PV_TIME_ST: + if (vcpu->arch.steal.base != GPA_INVALID) + val = SMCCC_RET_SUCCESS; + break; } return val;
Don't confuse the guest by saying steal-time is supported when it hasn't been configured by userspace and won't work. Signed-off-by: Andrew Jones <drjones@redhat.com> --- arch/arm64/kvm/pvtime.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)