This supports SDEI_VERSION hypercall by returning v1.1, which is
the specification version we're following. The vendor is set to
'KVM'.
Signed-off-by: Gavin Shan <[email protected]>
---
arch/arm64/kvm/sdei.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/kvm/sdei.c b/arch/arm64/kvm/sdei.c
index 8a9b477b8977..5a3a64cd6e84 100644
--- a/arch/arm64/kvm/sdei.c
+++ b/arch/arm64/kvm/sdei.c
@@ -118,6 +118,14 @@ static bool remove_all_vcpu_events(struct kvm_vcpu *vcpu,
return pending;
}
+static unsigned long hypercall_version(struct kvm_vcpu *vcpu)
+{
+ /* v1.1 and the vendor is KVM */
+ return (1UL << SDEI_VERSION_MAJOR_SHIFT) |
+ (1UL << SDEI_VERSION_MINOR_SHIFT) |
+ 0x4b564d;
+}
+
int kvm_sdei_hypercall(struct kvm_vcpu *vcpu)
{
struct kvm *kvm = vcpu->kvm;
@@ -142,6 +150,8 @@ int kvm_sdei_hypercall(struct kvm_vcpu *vcpu)
switch (func) {
case SDEI_1_0_FN_SDEI_VERSION:
+ ret = hypercall_version(vcpu);
+ break;
case SDEI_1_0_FN_SDEI_EVENT_REGISTER:
case SDEI_1_0_FN_SDEI_EVENT_ENABLE:
case SDEI_1_0_FN_SDEI_EVENT_DISABLE:
--
2.23.0
On Tue, Mar 22, 2022 at 04:06:51PM +0800, Gavin Shan wrote:
> This supports SDEI_VERSION hypercall by returning v1.1, which is
> the specification version we're following. The vendor is set to
> 'KVM'.
>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> arch/arm64/kvm/sdei.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/kvm/sdei.c b/arch/arm64/kvm/sdei.c
> index 8a9b477b8977..5a3a64cd6e84 100644
> --- a/arch/arm64/kvm/sdei.c
> +++ b/arch/arm64/kvm/sdei.c
> @@ -118,6 +118,14 @@ static bool remove_all_vcpu_events(struct kvm_vcpu *vcpu,
> return pending;
> }
>
> +static unsigned long hypercall_version(struct kvm_vcpu *vcpu)
> +{
> + /* v1.1 and the vendor is KVM */
> + return (1UL << SDEI_VERSION_MAJOR_SHIFT) |
> + (1UL << SDEI_VERSION_MINOR_SHIFT) |
> + 0x4b564d;
It looks like the SDEI specification states that the vendor-defined
version number is 32 bits. Could we just use one of the
ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_{0,3} values instead?
ASCII 'KVM' is neat, but in reality guest software will just throw it in
a macro regardless. Might as well use one of the values we've already
trained it to use :-)
Also, it would appear that guest discovery of SDEI relies upon KVM
reporting a valid SDEI version. IMO, this patch should come at the very
end when KVM actually implements SDEI.
--
Thanks,
Oliver
Hi Oliver,
On 3/23/22 2:04 AM, Oliver Upton wrote:
> On Tue, Mar 22, 2022 at 04:06:51PM +0800, Gavin Shan wrote:
>> This supports SDEI_VERSION hypercall by returning v1.1, which is
>> the specification version we're following. The vendor is set to
>> 'KVM'.
>>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> arch/arm64/kvm/sdei.c | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sdei.c b/arch/arm64/kvm/sdei.c
>> index 8a9b477b8977..5a3a64cd6e84 100644
>> --- a/arch/arm64/kvm/sdei.c
>> +++ b/arch/arm64/kvm/sdei.c
>> @@ -118,6 +118,14 @@ static bool remove_all_vcpu_events(struct kvm_vcpu *vcpu,
>> return pending;
>> }
>>
>> +static unsigned long hypercall_version(struct kvm_vcpu *vcpu)
>> +{
>> + /* v1.1 and the vendor is KVM */
>> + return (1UL << SDEI_VERSION_MAJOR_SHIFT) |
>> + (1UL << SDEI_VERSION_MINOR_SHIFT) |
>> + 0x4b564d;
>
> It looks like the SDEI specification states that the vendor-defined
> version number is 32 bits. Could we just use one of the
> ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_{0,3} values instead?
>
> ASCII 'KVM' is neat, but in reality guest software will just throw it in
> a macro regardless. Might as well use one of the values we've already
> trained it to use :-)
>
> Also, it would appear that guest discovery of SDEI relies upon KVM
> reporting a valid SDEI version. IMO, this patch should come at the very
> end when KVM actually implements SDEI.
>
Yeah, I was sticky to the pattern of "KVM". However, I think it's good
to reuse the existing one. Lets use ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
if you agree. Its first two characters are "VM" at least.
It's fine to return the version here because the SDEI capability isn't
exposed yet. It means no events have been exposed and able to be
registered. However, It's also fine to move this patch after the
following one:
[PATCH v5 16/22] KVM: arm64: Support SDEI_EVENT_{COMPLETE, COMPLETE_AND_RESUME} hypercall
Thanks,
Gavin
Hi Oliver,
On 3/24/22 12:31 AM, Oliver Upton wrote:
> On Wed, Mar 23, 2022 at 08:46:40PM +0800, Gavin Shan wrote:
>> On 3/23/22 2:04 AM, Oliver Upton wrote:
>>> On Tue, Mar 22, 2022 at 04:06:51PM +0800, Gavin Shan wrote:
>>>> This supports SDEI_VERSION hypercall by returning v1.1, which is
>>>> the specification version we're following. The vendor is set to
>>>> 'KVM'.
>>>>
>>>> Signed-off-by: Gavin Shan <[email protected]>
>>>> ---
>>>> arch/arm64/kvm/sdei.c | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/sdei.c b/arch/arm64/kvm/sdei.c
>>>> index 8a9b477b8977..5a3a64cd6e84 100644
>>>> --- a/arch/arm64/kvm/sdei.c
>>>> +++ b/arch/arm64/kvm/sdei.c
>>>> @@ -118,6 +118,14 @@ static bool remove_all_vcpu_events(struct kvm_vcpu *vcpu,
>>>> return pending;
>>>> }
>>>> +static unsigned long hypercall_version(struct kvm_vcpu *vcpu)
>>>> +{
>>>> + /* v1.1 and the vendor is KVM */
>>>> + return (1UL << SDEI_VERSION_MAJOR_SHIFT) |
>>>> + (1UL << SDEI_VERSION_MINOR_SHIFT) |
>>>> + 0x4b564d;
>>>
>>> It looks like the SDEI specification states that the vendor-defined
>>> version number is 32 bits. Could we just use one of the
>>> ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_{0,3} values instead?
>>>
>>> ASCII 'KVM' is neat, but in reality guest software will just throw it in
>>> a macro regardless. Might as well use one of the values we've already
>>> trained it to use :-)
>>>
>>> Also, it would appear that guest discovery of SDEI relies upon KVM
>>> reporting a valid SDEI version. IMO, this patch should come at the very
>>> end when KVM actually implements SDEI.
>>>
>>
>> Yeah, I was sticky to the pattern of "KVM". However, I think it's good
>> to reuse the existing one. Lets use ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
>> if you agree. Its first two characters are "VM" at least.
>
> Sounds fine to me. The only other nit I'd say is we should define a
> macro for it too, something like:
>
> #define KVM_SDEI_VENDOR ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
>
Agreed, and the macro will be put into arch/arm64/include/asm/kvm_sdei.h.
arch/arm64/include/uapi/asm/kvm_sdei_state.h isn't the right place because
the dependent macro ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 isn't exposed by
ABI.
Thanks,
Gavin
On Thu, Mar 24, 2022 at 12:07:34PM +0800, Gavin Shan wrote:
[...]
> > > Yeah, I was sticky to the pattern of "KVM". However, I think it's good
> > > to reuse the existing one. Lets use ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
> > > if you agree. Its first two characters are "VM" at least.
> >
> > Sounds fine to me. The only other nit I'd say is we should define a
> > macro for it too, something like:
> >
> > #define KVM_SDEI_VENDOR ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
> >
>
> Agreed, and the macro will be put into arch/arm64/include/asm/kvm_sdei.h.
> arch/arm64/include/uapi/asm/kvm_sdei_state.h isn't the right place because
> the dependent macro ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 isn't exposed by
> ABI.
The argument could definitely be made that our vendor ID should be
promoted to UAPI. Even though linux is the only known user of our
vendor-specific hypercalls, nothing is stopping other software from
using them. Beyond that, these values should really never change anyway.
It isn't a big deal if you add it to internal headers, either, as the
only known consumer will be the kernel.
--
Thanks,
Oliver
Hi Oliver,
On 3/24/22 3:48 PM, Oliver Upton wrote:
> On Thu, Mar 24, 2022 at 12:07:34PM +0800, Gavin Shan wrote:
>
> [...]
>
>>>> Yeah, I was sticky to the pattern of "KVM". However, I think it's good
>>>> to reuse the existing one. Lets use ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
>>>> if you agree. Its first two characters are "VM" at least.
>>>
>>> Sounds fine to me. The only other nit I'd say is we should define a
>>> macro for it too, something like:
>>>
>>> #define KVM_SDEI_VENDOR ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
>>>
>>
>> Agreed, and the macro will be put into arch/arm64/include/asm/kvm_sdei.h.
>> arch/arm64/include/uapi/asm/kvm_sdei_state.h isn't the right place because
>> the dependent macro ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2 isn't exposed by
>> ABI.
>
> The argument could definitely be made that our vendor ID should be
> promoted to UAPI. Even though linux is the only known user of our
> vendor-specific hypercalls, nothing is stopping other software from
> using them. Beyond that, these values should really never change anyway.
>
> It isn't a big deal if you add it to internal headers, either, as the
> only known consumer will be the kernel.
>
Sure. Lets add it into include/asm/kvm_sdei.h in next respin. We can
expose it when it is needed. For now, I do think Linux is the only
user.
Thanks,
Gavin
On Wed, Mar 23, 2022 at 08:46:40PM +0800, Gavin Shan wrote:
> Hi Oliver,
>
> On 3/23/22 2:04 AM, Oliver Upton wrote:
> > On Tue, Mar 22, 2022 at 04:06:51PM +0800, Gavin Shan wrote:
> > > This supports SDEI_VERSION hypercall by returning v1.1, which is
> > > the specification version we're following. The vendor is set to
> > > 'KVM'.
> > >
> > > Signed-off-by: Gavin Shan <[email protected]>
> > > ---
> > > arch/arm64/kvm/sdei.c | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/arch/arm64/kvm/sdei.c b/arch/arm64/kvm/sdei.c
> > > index 8a9b477b8977..5a3a64cd6e84 100644
> > > --- a/arch/arm64/kvm/sdei.c
> > > +++ b/arch/arm64/kvm/sdei.c
> > > @@ -118,6 +118,14 @@ static bool remove_all_vcpu_events(struct kvm_vcpu *vcpu,
> > > return pending;
> > > }
> > > +static unsigned long hypercall_version(struct kvm_vcpu *vcpu)
> > > +{
> > > + /* v1.1 and the vendor is KVM */
> > > + return (1UL << SDEI_VERSION_MAJOR_SHIFT) |
> > > + (1UL << SDEI_VERSION_MINOR_SHIFT) |
> > > + 0x4b564d;
> >
> > It looks like the SDEI specification states that the vendor-defined
> > version number is 32 bits. Could we just use one of the
> > ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_{0,3} values instead?
> >
> > ASCII 'KVM' is neat, but in reality guest software will just throw it in
> > a macro regardless. Might as well use one of the values we've already
> > trained it to use :-)
> >
> > Also, it would appear that guest discovery of SDEI relies upon KVM
> > reporting a valid SDEI version. IMO, this patch should come at the very
> > end when KVM actually implements SDEI.
> >
>
> Yeah, I was sticky to the pattern of "KVM". However, I think it's good
> to reuse the existing one. Lets use ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
> if you agree. Its first two characters are "VM" at least.
Sounds fine to me. The only other nit I'd say is we should define a
macro for it too, something like:
#define KVM_SDEI_VENDOR ARM_SMCCC_VENDOR_HYP_UID_KVM_REG_2
--
Thanks,
Oliver