KVM: arm64: Add support for hypercall services selection

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

include/linux/kvm_host.h +0 -0

index c310648cc8f1..d0bd8f7a026c 100644

include/linux/kvm_host.h +2 -0

 };
 
 #define kvm_err(fmt, ...) \

@@ -1666,6 +1667,8 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
        }
 }
 
+#define kvm_vm_has_started(kvm) (kvm->vm_started)
+
 extern bool kvm_rebooting;
 
 extern unsigned int halt_poll_ns;

virt/kvm/kvm_main.c +1 -0

index 72c4e6b39389..962b91ac2064 100644
@@ -3686,6 +3686,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
        int r;
        struct kvm_fpu *fpu = NULL;
        struct kvm_sregs *kvm_sregs = NULL;
+       struct kvm *kvm = vcpu->kvm;
 
        if (vcpu->kvm->mm != current->mm || vcpu->kvm->vm_dead)
                return -EIO;

virt/kvm/kvm_main.c +3 -1

+                       mutex_lock(&kvm->lock);
+                       kvm->vm_started = true;
+                       mutex_unlock(&kvm->lock);
                }
                r = kvm_arch_vcpu_ioctl_run(vcpu);
                trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
--

2.34.1.448.ga2b2bfdf31-goog

Hi Raghu,

Quoted 13 lines · Raghavendra Rao Ananta

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

include/linux/kvm_host.h struct kvm { · line 626

Quoted 9 lines

include/linux/kvm_host.h +1 -0

index c310648cc8f1..d0bd8f7a026c 100644
@@ -623,6 +623,7 @@ struct kvm {
        struct notifier_block pm_notifier;
 #endif
        char stats_id[KVM_STATS_NAME_SIZE];
+       bool vm_started;

Since KVM_RUN on any vCPUs doesn't necessarily mean that the VM started yet, the name might be a bit misleading IMHO. I would think 'has_run_once' or 'ran_once' might be more clear (?).

Quoted 23 lines

include/linux/kvm_host.h +2 -0

 };
 
 #define kvm_err(fmt, ...) \

@@ -1666,6 +1667,8 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
        }
 }
 
+#define kvm_vm_has_started(kvm) (kvm->vm_started)
+
 extern bool kvm_rebooting;
 
 extern unsigned int halt_poll_ns;

virt/kvm/kvm_main.c +1 -0

index 72c4e6b39389..962b91ac2064 100644
@@ -3686,6 +3686,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
        int r;
        struct kvm_fpu *fpu = NULL;
        struct kvm_sregs *kvm_sregs = NULL;
+       struct kvm *kvm = vcpu->kvm;
 

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3731

Quoted 11 lines

virt/kvm/kvm_main.c +5 -0

        if (vcpu->kvm->mm != current->mm || vcpu->kvm->vm_dead)
                return -EIO;

@@ -3723,6 +3724,14 @@ static long kvm_vcpu_ioctl(struct file *filp,
                        if (oldpid)
                                synchronize_rcu();
                        put_pid(oldpid);
+
+                       /*
+                        * Since we land here even on the first vCPU run,
+                        * we can mark that the VM has started running.
+                        */

It might be nicer to add a comment why the code below gets kvm->lock.

Anyway, the patch generally looks good to me, and thank you for making this change (it works for my purpose as well).

Reviewed-by Reiji Watanabe <[email protected]>

Thanks,
Reiji

Quoted 9 lines

virt/kvm/kvm_main.c +3 -1

+                       mutex_lock(&kvm->lock);
+                       kvm->vm_started = true;
+                       mutex_unlock(&kvm->lock);
                }
                r = kvm_arch_vcpu_ioctl_run(vcpu);
                trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
--

2.34.1.448.ga2b2bfdf31-goog

Hi Reiji,

Quoted 11 lines · Reiji Watanabe

Hi Raghu,

Quoted 6 lines · Raghavendra Rao Ananta

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Quoted 10 lines · Raghavendra Rao Ananta

Hi Reiji,

Quoted 6 lines · Reiji Watanabe

Hi Raghu,

Quoted 1 line · Raghavendra Rao Ananta

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

What about live migration, where the VM has already technically been started before the first call to KVM_RUN?

Quoted 13 lines · Jim Mattson

Quoted 10 lines · Raghavendra Rao Ananta

Hi Reiji,

Quoted 6 lines · Reiji Watanabe

Hi Raghu,

Quoted 1 line · Raghavendra Rao Ananta

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

What about live migration, where the VM has already technically been started before the first call to KVM_RUN?

My understanding is that a new 'struct kvm' is created on the target machine and this flag should be reset, which would allow the VMM to restore the firmware registers. However, we would be running KVM_RUN for the first time on the target machine, thus setting the flag. So, you are right; It's more of a resume operation from the guest's point of view. I guess the name of the variable is what's confusing here.

Thanks,
Raghavendra

Quoted 24 lines · Raghavendra Rao Ananta

Quoted 13 lines · Jim Mattson

Quoted 10 lines · Raghavendra Rao Ananta

Hi Reiji,

Quoted 6 lines · Reiji Watanabe

Hi Raghu,

Quoted 1 line · Raghavendra Rao Ananta

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

What about live migration, where the VM has already technically been started before the first call to KVM_RUN?

Quoted 7 lines

My understanding is that a new 'struct kvm' is created on the target machine and this flag should be reset, which would allow the VMM to restore the firmware registers. However, we would be running KVM_RUN for the first time on the target machine, thus setting the flag. So, you are right; It's more of a resume operation from the guest's point of view. I guess the name of the variable is what's confusing here.

I was actually thinking that live migration gives userspace an easy way to circumvent your restriction. You said, "This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running." However, if you don't ensure that these VM features are configured the same way on the target machine as they were on the source machine, you have not actually accomplished your stated goal.

Quoted 2 lines

Thanks,
Raghavendra

Quoted 35 lines · Jim Mattson

Quoted 24 lines · Raghavendra Rao Ananta

Quoted 13 lines · Jim Mattson

Quoted 10 lines · Raghavendra Rao Ananta

Hi Reiji,

Quoted 6 lines · Reiji Watanabe

Hi Raghu,

Quoted 1 line · Raghavendra Rao Ananta

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

What about live migration, where the VM has already technically been started before the first call to KVM_RUN?

Quoted 7 lines

My understanding is that a new 'struct kvm' is created on the target machine and this flag should be reset, which would allow the VMM to restore the firmware registers. However, we would be running KVM_RUN for the first time on the target machine, thus setting the flag. So, you are right; It's more of a resume operation from the guest's point of view. I guess the name of the variable is what's confusing here.

Quoted 8 lines

I was actually thinking that live migration gives userspace an easy way to circumvent your restriction. You said, "This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running." However, if you don't ensure that these VM features are configured the same way on the target machine as they were on the source machine, you have not actually accomplished your stated goal.

Isn't that up to the VMM to save/restore and validate the registers across migrations?

Perhaps I have to re-word my intentions for the patch- userspace should be able to configure the registers before issuing the first KVM_RUN.

Quoted 45 lines · Raghavendra Rao Ananta

Quoted 35 lines · Jim Mattson

Quoted 24 lines · Raghavendra Rao Ananta

Quoted 13 lines · Jim Mattson

Quoted 10 lines · Raghavendra Rao Ananta

Hi Reiji,

Quoted 6 lines · Reiji Watanabe

Hi Raghu,

Quoted 1 line · Raghavendra Rao Ananta

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

What about live migration, where the VM has already technically been started before the first call to KVM_RUN?

Quoted 7 lines

My understanding is that a new 'struct kvm' is created on the target machine and this flag should be reset, which would allow the VMM to restore the firmware registers. However, we would be running KVM_RUN for the first time on the target machine, thus setting the flag. So, you are right; It's more of a resume operation from the guest's point of view. I guess the name of the variable is what's confusing here.

Quoted 8 lines

I was actually thinking that live migration gives userspace an easy way to circumvent your restriction. You said, "This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running." However, if you don't ensure that these VM features are configured the same way on the target machine as they were on the source machine, you have not actually accomplished your stated goal.

Quoted 2 lines

Isn't that up to the VMM to save/restore and validate the registers across migrations?

Yes, just as it is up to userspace not to make bad configuration changes after the first VMRUN.

Quoted 3 lines

Perhaps I have to re-word my intentions for the patch- userspace should be able to configure the registers before issuing the first KVM_RUN.

Perhaps it would help if you explained *why* you are doing this. It sounds like you are either trying to protect against a malicious userspace, or you are trying to keep userspace from doing something stupid. In general, kvm only enforces constraints that are necessary to protect the host. If that's what you're doing, I don't understand why live migration doesn't provide an end-run around your protections.

Quoted 6 lines · Jim Mattson

Perhaps it would help if you explained *why* you are doing this. It sounds like you are either trying to protect against a malicious userspace, or you are trying to keep userspace from doing something stupid. In general, kvm only enforces constraints that are necessary to protect the host. If that's what you're doing, I don't understand why live migration doesn't provide an end-run around your protections.

It's mainly to safeguard the guests. With respect to migration, KVM and the userspace are collectively playing a role here. It's up to the userspace to ensure that the registers are configured the same across migrations and KVM ensures that the userspace doesn't modify the registers after KVM_RUN so that they don't see features turned OFF/ON during execution. I'm not sure if it falls into the definition of protecting the host. Do you see a value in adding this extra protection from KVM?

Quoted 7 lines · Raghavendra Rao Ananta

Quoted 6 lines · Jim Mattson

Perhaps it would help if you explained *why* you are doing this. It sounds like you are either trying to protect against a malicious userspace, or you are trying to keep userspace from doing something stupid. In general, kvm only enforces constraints that are necessary to protect the host. If that's what you're doing, I don't understand why live migration doesn't provide an end-run around your protections.

Quoted 8 lines

It's mainly to safeguard the guests. With respect to migration, KVM and the userspace are collectively playing a role here. It's up to the userspace to ensure that the registers are configured the same across migrations and KVM ensures that the userspace doesn't modify the registers after KVM_RUN so that they don't see features turned OFF/ON during execution. I'm not sure if it falls into the definition of protecting the host. Do you see a value in adding this extra protection from KVM?

Short answer: probably not?

There is precedent for disallowing userspace from doing stupid things, but that's either for KVM's protection (as Jim pointed out), or because KVM can't honor the change, e.g. x86 is currently in the process of disallowing most CPUID changes after KVM_RUN because KVM itself consumes the CPUID information and KVM doesn't support updating some of it's own internal state (because removing features like GB hugepage support is nonsensical and would require a large pile of complicated, messy code).

Restricing CPUID changes does offer some "protection" to the guest, but that's not the goal. E.g. KVM won't detect CPUID misconfiguration in the migration case, and trying to do so is a fool's errand.

If restricting updates in the arm64 is necessary to ensure KVM provides sane behavior, then it could be justified. But if it's purely a sanity check on behalf of the guest, then it's not justified.

Quoted 3 lines · Sean Christopherson

If restricting updates in the arm64 is necessary to ensure KVM provides sane behavior, then it could be justified. But if it's purely a sanity check on behalf of the guest, then it's not justified.

Agreed that KVM doesn't really safeguard the guests, but just curious, is there really a downside in adding this thin layer of safety check?

On the bright side, the guests would be safe, and it could save the developers some time in hunting down the bugs in this path, no?

Quoted 7 lines · Raghavendra Rao Ananta

Quoted 6 lines · Jim Mattson

Perhaps it would help if you explained *why* you are doing this. It sounds like you are either trying to protect against a malicious userspace, or you are trying to keep userspace from doing something stupid. In general, kvm only enforces constraints that are necessary to protect the host. If that's what you're doing, I don't understand why live migration doesn't provide an end-run around your protections.

Quoted 8 lines

It's mainly to safeguard the guests. With respect to migration, KVM and the userspace are collectively playing a role here. It's up to the userspace to ensure that the registers are configured the same across migrations and KVM ensures that the userspace doesn't modify the registers after KVM_RUN so that they don't see features turned OFF/ON during execution. I'm not sure if it falls into the definition of protecting the host. Do you see a value in adding this extra protection from KVM?

Short answer: probably not?

There is precedent for disallowing userspace from doing stupid things, but that's either for KVM's protection (as Jim pointed out), or because KVM can't honor the change, e.g. x86 is currently in the process of disallowing most CPUID changes after KVM_RUN because KVM itself consumes the CPUID information and KVM doesn't support updating some of it's own internal state (because removing features like GB hugepage support is nonsensical and would require a large pile of complicated, messy code).

Restricing CPUID changes does offer some "protection" to the guest, but that's not the goal. E.g. KVM won't detect CPUID misconfiguration in the migration case, and trying to do so is a fool's errand.

If restricting updates in the arm64 is necessary to ensure KVM provides sane behavior, then it could be justified. But if it's purely a sanity check on behalf of the guest, then it's not justified.

Quoted 28 lines · Sean Christopherson

Quoted 7 lines · Raghavendra Rao Ananta

Quoted 6 lines · Jim Mattson

Perhaps it would help if you explained *why* you are doing this. It sounds like you are either trying to protect against a malicious userspace, or you are trying to keep userspace from doing something stupid. In general, kvm only enforces constraints that are necessary to protect the host. If that's what you're doing, I don't understand why live migration doesn't provide an end-run around your protections.

Quoted 8 lines

It's mainly to safeguard the guests. With respect to migration, KVM and the userspace are collectively playing a role here. It's up to the userspace to ensure that the registers are configured the same across migrations and KVM ensures that the userspace doesn't modify the registers after KVM_RUN so that they don't see features turned OFF/ON during execution. I'm not sure if it falls into the definition of protecting the host. Do you see a value in adding this extra protection from KVM?

Short answer: probably not?

There is precedent for disallowing userspace from doing stupid things, but that's either for KVM's protection (as Jim pointed out), or because KVM can't honor the change, e.g. x86 is currently in the process of disallowing most CPUID changes after KVM_RUN because KVM itself consumes the CPUID information and KVM doesn't support updating some of it's own internal state (because removing features like GB hugepage support is nonsensical and would require a large pile of complicated, messy code).

Quoted 7 lines

Restricing CPUID changes does offer some "protection" to the guest, but that's not the goal. E.g. KVM won't detect CPUID misconfiguration in the migration case, and trying to do so is a fool's errand.

If restricting updates in the arm64 is necessary to ensure KVM provides sane behavior, then it could be justified. But if it's purely a sanity check on behalf of the guest, then it's not justified.

The pseudo firmware hvc registers, which this series are adding, are used by KVM to identify available hvc features for the guest, and not directly exposed to the guest as registers.
The ways the KVM code in the series consumes the registers' values are very limited, and no KVM data/state is created based on their values. But, as the code that consumes the registers grows in the future, I wouldn't be surprised if KVM consumes them differently than it does now (e.g. create another data structure based on the register values). I'm not sure though :)

The restriction, with which KVM doesn't need to worry about the changes in the registers after KVM_RUN, could potentially protect or be useful to protect KVM and simplify future changes/maintenance of the KVM codes that consumes the values.
I thought this was one of the reasons for having the restriction.

Thanks,
Reiji

The restriction, with which KVM doesn't need to worry about the changes in the registers after KVM_RUN, could potentially protect or be useful to protect KVM and simplify future changes/maintenance of the KVM codes that consumes the values.

Quoted 4 lines · Reiji Watanabe

The restriction, with which KVM doesn't need to worry about the changes in the registers after KVM_RUN, could potentially protect or be useful to protect KVM and simplify future changes/maintenance of the KVM codes that consumes the values.

That sort of protection is definitely welcome, the previously mentioned CPUID mess on x86 would have benefit greatly by KVM being restrictive in the past. That said, hooking KVM_RUN is likely the wrong way to go about implementing any restrictions. Running a vCPU is where much of the vCPU's state is explicitly consumed, but it's all too easy for KVM to implicity/indirectly consume state via a different ioctl(), e.g. if there are side effects that are visible in other registers, than an update can also be visible to userspace via KVM_{G,S}ET_{S,}REGS, at which point disallowing modifying state after KVM_RUN but not after reading/writing regs is arbitrary and inconsitent.

If possible, preventing modification if kvm->created_vcpus > 0 is ideal as it's a relatively common pattern in KVM, and provides a clear boundary to userpace regarding what is/isn't allowed.

Quoted 7 lines · Sean Christopherson

Quoted 4 lines · Reiji Watanabe

The restriction, with which KVM doesn't need to worry about the changes in the registers after KVM_RUN, could potentially protect or be useful to protect KVM and simplify future changes/maintenance of the KVM codes that consumes the values.

Quoted 9 lines

That sort of protection is definitely welcome, the previously mentioned CPUID mess on x86 would have benefit greatly by KVM being restrictive in the past. That said, hooking KVM_RUN is likely the wrong way to go about implementing any restrictions. Running a vCPU is where much of the vCPU's state is explicitly consumed, but it's all too easy for KVM to implicity/indirectly consume state via a different ioctl(), e.g. if there are side effects that are visible in other registers, than an update can also be visible to userspace via KVM_{G,S}ET_{S,}REGS, at which point disallowing modifying state after KVM_RUN but not after reading/writing regs is arbitrary and inconsitent.

Thank you for your comments !
I think I understand your concern, and that's a great point. That's not the case for those pseudo registers though at least for now :) BTW, is this concern specific to hooking KVM_RUN ? (Wouldn't it be the same for the option with "if kvm->created_vcpus > 0" ?)

Quoted 3 lines

If possible, preventing modification if kvm->created_vcpus > 0 is ideal as it's a relatively common pattern in KVM, and provides a clear boundary to userpace regarding what is/isn't allowed.

Yes, I agree that would be better in general. For (pseudo) registers,

I would think preventing modification if kvm->created_vcpus > 0 might not be a very good option for KVM/ARM though considering usage of KVM_GET_REG_LIST and KVM_{G,S}ET_ONE_REG.

Quoted 18 lines · Reiji Watanabe

Quoted 7 lines · Sean Christopherson

Quoted 4 lines · Reiji Watanabe

The restriction, with which KVM doesn't need to worry about the changes in the registers after KVM_RUN, could potentially protect or be useful to protect KVM and simplify future changes/maintenance of the KVM codes that consumes the values.

Quoted 9 lines

That sort of protection is definitely welcome, the previously mentioned CPUID mess on x86 would have benefit greatly by KVM being restrictive in the past. That said, hooking KVM_RUN is likely the wrong way to go about implementing any restrictions. Running a vCPU is where much of the vCPU's state is explicitly consumed, but it's all too easy for KVM to implicity/indirectly consume state via a different ioctl(), e.g. if there are side effects that are visible in other registers, than an update can also be visible to userspace via KVM_{G,S}ET_{S,}REGS, at which point disallowing modifying state after KVM_RUN but not after reading/writing regs is arbitrary and inconsitent.

Quoted 5 lines

Thank you for your comments !
I think I understand your concern, and that's a great point. That's not the case for those pseudo registers though at least for now :) BTW, is this concern specific to hooking KVM_RUN ? (Wouldn't it be the same for the option with "if kvm->created_vcpus > 0" ?)

Not really? The goal with created_vcpus is to avoid having inconsistent state in "struct kvm_vcpu" with respect to the VM as whole. "struct kvm" obvioulsy can't be inconsistent with itself, e.g. even if userspace consumes some side effect, that's simply "the state". Did that make sense? Hard to explain in writing :-)

Quoted 4 lines

Quoted 3 lines

If possible, preventing modification if kvm->created_vcpus > 0 is ideal as it's a relatively common pattern in KVM, and provides a clear boundary to userpace regarding what is/isn't allowed.

Quoted 1 line

Yes, I agree that would be better in general. For (pseudo) registers,

What exactly are these pseudo registers? If it's something that's an immutable property of the (virtual) system, then it might make sense to use a separate, non-vCPU mechanism for setting/getting their values. Then you can easily restrict the <whatever> to pre-created_vcpus, e.g. see x86's KVM_SET_IDENTITY_MAP_ADDR.

Quoted 3 lines

I would think preventing modification if kvm->created_vcpus > 0 might not be a very good option for KVM/ARM though considering usage of KVM_GET_REG_LIST and KVM_{G,S}ET_ONE_REG.

Quoted 4 lines · Reiji Watanabe

The restriction, with which KVM doesn't need to worry about the changes in the registers after KVM_RUN, could potentially protect or be useful to protect KVM and simplify future changes/maintenance of the KVM codes that consumes the values.

That sort of protection is definitely welcome, the previously mentioned CPUID mess on x86 would have benefit greatly by KVM being restrictive in the past. That said, hooking KVM_RUN is likely the wrong way to go about implementing any restrictions. Running a vCPU is where much of the vCPU's state is explicitly consumed, but it's all too easy for KVM to implicity/indirectly consume state via a different ioctl(), e.g. if there are side effects that are visible in other registers, than an update can also be visible to userspace via KVM_{G,S}ET_{S,}REGS, at which point disallowing modifying state after KVM_RUN but not after reading/writing regs is arbitrary and inconsitent.

If possible, preventing modification if kvm->created_vcpus > 0 is ideal as it's a relatively common pattern in KVM, and provides a clear boundary to userpace regarding what is/isn't allowed.

Quoted 7 lines · Raghavendra Rao Ananta

Quoted 6 lines · Jim Mattson

Perhaps it would help if you explained *why* you are doing this. It sounds like you are either trying to protect against a malicious userspace, or you are trying to keep userspace from doing something stupid. In general, kvm only enforces constraints that are necessary to protect the host. If that's what you're doing, I don't understand why live migration doesn't provide an end-run around your protections.

Quoted 8 lines

It's mainly to safeguard the guests. With respect to migration, KVM and the userspace are collectively playing a role here. It's up to the userspace to ensure that the registers are configured the same across migrations and KVM ensures that the userspace doesn't modify the registers after KVM_RUN so that they don't see features turned OFF/ON during execution. I'm not sure if it falls into the definition of protecting the host. Do you see a value in adding this extra protection from KVM?

Short answer: probably not?

There is precedent for disallowing userspace from doing stupid things, but that's either for KVM's protection (as Jim pointed out), or because KVM can't honor the change, e.g. x86 is currently in the process of disallowing most CPUID changes after KVM_RUN because KVM itself consumes the CPUID information and KVM doesn't support updating some of it's own internal state (because removing features like GB hugepage support is nonsensical and would require a large pile of complicated, messy code).

Restricing CPUID changes does offer some "protection" to the guest, but that's not the goal. E.g. KVM won't detect CPUID misconfiguration in the migration case, and trying to do so is a fool's errand.

If restricting updates in the arm64 is necessary to ensure KVM provides sane behavior, then it could be justified. But if it's purely a sanity check on behalf of the guest, then it's not justified.

Hi Reiji,

Quoted 18 lines · Reiji Watanabe

Hi Raghu,

Quoted 13 lines · Raghavendra Rao Ananta

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

Quoted 14 lines

include/linux/kvm_host.h struct kvm { · line 626

Quoted 9 lines

include/linux/kvm_host.h +1 -0

index c310648cc8f1..d0bd8f7a026c 100644
@@ -623,6 +623,7 @@ struct kvm {
        struct notifier_block pm_notifier;
 #endif
        char stats_id[KVM_STATS_NAME_SIZE];
+       bool vm_started;

Since KVM_RUN on any vCPUs doesn't necessarily mean that the VM started yet, the name might be a bit misleading IMHO. I would think 'has_run_once' or 'ran_once' might be more clear (?).

I always struggle with the names; but if you feel that 'ran_once' makes more sense for a reader, I can change it.

Quoted 22 lines

Quoted 22 lines

include/linux/kvm_host.h +2 -0

 };
 
 #define kvm_err(fmt, ...) \

@@ -1666,6 +1667,8 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
        }
 }
 
+#define kvm_vm_has_started(kvm) (kvm->vm_started)
+
 extern bool kvm_rebooting;
 
 extern unsigned int halt_poll_ns;

virt/kvm/kvm_main.c +1 -0

index 72c4e6b39389..962b91ac2064 100644
@@ -3686,6 +3686,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
        int r;
        struct kvm_fpu *fpu = NULL;
        struct kvm_sregs *kvm_sregs = NULL;
+       struct kvm *kvm = vcpu->kvm;
 

Quoted 14 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3731

Quoted 11 lines

virt/kvm/kvm_main.c +5 -0

        if (vcpu->kvm->mm != current->mm || vcpu->kvm->vm_dead)
                return -EIO;

@@ -3723,6 +3724,14 @@ static long kvm_vcpu_ioctl(struct file *filp,
                        if (oldpid)
                                synchronize_rcu();
                        put_pid(oldpid);
+
+                       /*
+                        * Since we land here even on the first vCPU run,
+                        * we can mark that the VM has started running.
+                        */

It might be nicer to add a comment why the code below gets kvm->lock.

I've been going back and forth on this one. Initially I considered simply going with atomic_t, but the patch 4/11 (KVM: arm64: Setup a framework for hypercall bitmap firmware registers) kvm_arm_set_fw_reg_bmap()'s implementation felt like we need a lock to have the whole 'is the register busy?' operation atomic. But, that's just one of the applications.

Quoted 5 lines

Anyway, the patch generally looks good to me, and thank you for making this change (it works for my purpose as well).

Reviewed-by Reiji Watanabe <[email protected]>

Glad that it's helping you as well and thanks for the review.

Regards,
Raghavendra

Quoted 13 lines

Thanks,
Reiji

Quoted 9 lines

virt/kvm/kvm_main.c +3 -1

+                       mutex_lock(&kvm->lock);
+                       kvm->vm_started = true;
+                       mutex_unlock(&kvm->lock);
                }
                r = kvm_arch_vcpu_ioctl_run(vcpu);
                trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
--

2.34.1.448.ga2b2bfdf31-goog

Quoted 36 lines · Raghavendra Rao Ananta

Hi Reiji,

Quoted 32 lines · Reiji Watanabe

Hi Raghu,

Quoted 18 lines · Raghavendra Rao Ananta

Capture the start of the KVM VM, which is basically the start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

include/linux/kvm_host.h +0 -0

index c310648cc8f1..d0bd8f7a026c 100644

include/linux/kvm_host.h struct kvm { · line 626

Quoted 4 lines

include/linux/kvm_host.h +1 -0

@@ -623,6 +623,7 @@ struct kvm {
        struct notifier_block pm_notifier;
 #endif
        char stats_id[KVM_STATS_NAME_SIZE];
+       bool vm_started;

Since KVM_RUN on any vCPUs doesn't necessarily mean that the VM started yet, the name might be a bit misleading IMHO. I would think 'has_run_once' or 'ran_once' might be more clear (?).

Quoted 2 lines

I always struggle with the names; but if you feel that 'ran_once' makes more sense for a reader, I can change it.

I would prefer 'ran_once'.

Quoted 37 lines

Quoted 23 lines

Quoted 23 lines

include/linux/kvm_host.h +2 -0

 };
 
 #define kvm_err(fmt, ...) \

@@ -1666,6 +1667,8 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
        }
 }
 
+#define kvm_vm_has_started(kvm) (kvm->vm_started)
+
 extern bool kvm_rebooting;
 
 extern unsigned int halt_poll_ns;

virt/kvm/kvm_main.c +1 -0

index 72c4e6b39389..962b91ac2064 100644
@@ -3686,6 +3686,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
        int r;
        struct kvm_fpu *fpu = NULL;
        struct kvm_sregs *kvm_sregs = NULL;
+       struct kvm *kvm = vcpu->kvm;
 

Quoted 14 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3731

Quoted 11 lines

virt/kvm/kvm_main.c +5 -0

        if (vcpu->kvm->mm != current->mm || vcpu->kvm->vm_dead)
                return -EIO;

@@ -3723,6 +3724,14 @@ static long kvm_vcpu_ioctl(struct file *filp,
                        if (oldpid)
                                synchronize_rcu();
                        put_pid(oldpid);
+
+                       /*
+                        * Since we land here even on the first vCPU run,
+                        * we can mark that the VM has started running.
+                        */

It might be nicer to add a comment why the code below gets kvm->lock.

Quoted 6 lines

I've been going back and forth on this one. Initially I considered simply going with atomic_t, but the patch 4/11 (KVM: arm64: Setup a framework for hypercall bitmap firmware registers) kvm_arm_set_fw_reg_bmap()'s implementation felt like we need a lock to have the whole 'is the register busy?' operation atomic. But, that's just one of the applications.

I understand why you need the code to get the lock here with the current implementation.
But, since the code just set the one field (vm_started) with the lock, I thought the intention of getting the lock might not be so obvious. (But, maybe clear enough looking at the code in the patch-4)

Thanks,
Reiji

Quoted 23 lines

Quoted 5 lines

Anyway, the patch generally looks good to me, and thank you for making this change (it works for my purpose as well).

Reviewed-by Reiji Watanabe <[email protected]>

Glad that it's helping you as well and thanks for the review.

Regards,
Raghavendra

Quoted 13 lines

Thanks,
Reiji

Quoted 9 lines

virt/kvm/kvm_main.c +3 -1

+                       mutex_lock(&kvm->lock);
+                       kvm->vm_started = true;
+                       mutex_unlock(&kvm->lock);
                }
                r = kvm_arch_vcpu_ioctl_run(vcpu);
                trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
--

2.34.1.448.ga2b2bfdf31-goog

Capture the start of the KVM VM, which is basically the

start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

include/linux/kvm_host.h +1 -0

index c310648cc8f1..d0bd8f7a026c 100644
@@ -623,6 +623,7 @@ struct kvm {
 	struct notifier_block pm_notifier;
 #endif
 	char stats_id[KVM_STATS_NAME_SIZE];
+	bool vm_started;
 };
 
 #define kvm_err(fmt, ...) \

virt/kvm/kvm_main.c +0 -0

 		}
 		r = kvm_arch_vcpu_ioctl_run(vcpu);
 		trace_kvm_userspace_exit(vcpu->run->exit_reason, r);

Signature · 6 lines

2.34.1.448.ga2b2bfdf31-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Quoted 1 line · Raghavendra Rao Ananta

Capture the start of the KVM VM, which is basically the

Please wrap at ~75 chars.

Quoted 4 lines

start of any vCPU run. This state of the VM is helpful in the upcoming patches to prevent user-space from configuring certain VM features after the VM has started running.

Please provide context of how the flag will be used. I glanced at the future patches, and knowing very little about arm, I was unable to glean useful info about exactly who is being prevented from doing what.

Quoted 23 lines

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 9 +++++++++
 2 files changed, 12 insertions(+)

include/linux/kvm_host.h +1 -0

index c310648cc8f1..d0bd8f7a026c 100644
@@ -623,6 +623,7 @@ struct kvm {
      struct notifier_block pm_notifier;
 #endif
      char stats_id[KVM_STATS_NAME_SIZE];
+     bool vm_started;
 };
 
 #define kvm_err(fmt, ...) \

@@ -1666,6 +1667,8 @@ static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
      }
 }
 

include/linux/kvm_host.h static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu) · line 1670

Quoted 1 line

include/linux/kvm_host.h +1 -0

+#define kvm_vm_has_started(kvm) (kvm->vm_started)

Needs parantheses around (kvm), but why bother with a macro? This is the same header that defines struct kvm.

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3689

Quoted 13 lines

include/linux/kvm_host.h +1 -0

+
 extern bool kvm_rebooting;
 
 extern unsigned int halt_poll_ns;

virt/kvm/kvm_main.c +1 -0

index 72c4e6b39389..962b91ac2064 100644
@@ -3686,6 +3686,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
      int r;
      struct kvm_fpu *fpu = NULL;
      struct kvm_sregs *kvm_sregs = NULL;
+     struct kvm *kvm = vcpu->kvm;

If you're going to bother grabbing kvm, replace the instances below that also do vcpu->kvm.

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3730

Quoted 11 lines

virt/kvm/kvm_main.c +4 -0

      if (vcpu->kvm->mm != current->mm || vcpu->kvm->vm_dead)
              return -EIO;

@@ -3723,6 +3724,14 @@ static long kvm_vcpu_ioctl(struct file *filp,
                      if (oldpid)
                              synchronize_rcu();
                      put_pid(oldpid);
+
+                     /*
+                      * Since we land here even on the first vCPU run,
+                      * we can mark that the VM has started running.

Please avoid "we", "us", etc..

"vm_started" is also ambiguous. If we end up with a flag, then I would prefer a much more literal name, a la created_vcpus, e.g. ran_vcpus or something.

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3732

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                      */
+                     mutex_lock(&kvm->lock);

This adds unnecessary lock contention when running vCPUs. The naive solution would be:

                        if (!kvm->vm_started) {
                                ...
                        }

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3734

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                     kvm->vm_started = true;
+                     mutex_unlock(&kvm->lock);

Lastly, why is this in generic KVM?

Quoted 10 lines

virt/kvm/kvm_main.c +0 -1

              }
              r = kvm_arch_vcpu_ioctl_run(vcpu);
              trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
--

2.34.1.448.ga2b2bfdf31-goog

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Quoted 7 lines · Sean Christopherson

Quoted 1 line · Raghavendra Rao Ananta

+#define kvm_vm_has_started(kvm) (kvm->vm_started)

Needs parantheses around (kvm), but why bother with a macro? This is the same header that defines struct kvm.

No specific reason for creating a macro as such. I can remove it if it feels noisy.

Quoted 9 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3732

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                      */
+                     mutex_lock(&kvm->lock);

This adds unnecessary lock contention when running vCPUs. The naive solution would be:

                        if (!kvm->vm_started) {
                                ...
                        }

Not sure if I understood the solution..

Quoted 5 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3734

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                     kvm->vm_started = true;
+                     mutex_unlock(&kvm->lock);

Lastly, why is this in generic KVM?

The v1 of the series originally had it in the arm specific code. However, I was suggested to move it to the generic code since the book keeping is not arch specific and could be helpful to others too [1].

Quoted 10 lines · Raghavendra Rao Ananta

Quoted 7 lines · Sean Christopherson

Quoted 1 line · Raghavendra Rao Ananta

+#define kvm_vm_has_started(kvm) (kvm->vm_started)

Needs parantheses around (kvm), but why bother with a macro? This is the same header that defines struct kvm.

No specific reason for creating a macro as such. I can remove it if it feels noisy.

Please do. In the future, don't use a macro unless there's a good reason to do so. Don't get me wrong, I love abusing macros, but for things like this they are completely inferior to

  static inline bool kvm_vm_has_started(struct kvm *kvm)
  {
        return kvm->vm_started;
  }

because a helper function gives us type safety, doesn't suffer from concatenation of tokens potentially doing weird things, is easier to extend to a multi-line implementation, etc...

An example of when it's ok to use a macro is x86's

  #define kvm_arch_vcpu_memslots_id(vcpu) ((vcpu)->arch.hflags & HF_SMM_MASK ? 1 : 0)

which uses a macro instead of a proper function to avoid a circular dependency due to arch/x86/include/asm/kvm_host.h being included by include/linux/kvm_host.h and thus x86's implementation of kvm_arch_vcpu_memslots_id() coming before the definition of struct kvm_vcpu. But that's very much an exception and done only because the alternatives suck more.

Quoted 10 lines

Quoted 9 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3732

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                      */
+                     mutex_lock(&kvm->lock);

This adds unnecessary lock contention when running vCPUs. The naive solution would be:

                        if (!kvm->vm_started) {
                                ...
                        }

Not sure if I understood the solution..

In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces unnecessary contention as it will serialize this bit of code if multiple vCPUs are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a VM will acquire kvm->lock and thus avoid contention once the VM is up and running. There's still a possibility that multiple vCPUs will contend for kvm->lock on their first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's a more elegant solution depending on the use case, e.g. a lockless approach might work (or it might not).

Quoted 8 lines

Quoted 5 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3734

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                     kvm->vm_started = true;
+                     mutex_unlock(&kvm->lock);

Lastly, why is this in generic KVM?

The v1 of the series originally had it in the arm specific code. However, I was suggested to move it to the generic code since the book keeping is not arch specific and could be helpful to others too [1].

I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, but I'm skeptical in this particular case. The code _is_ arch specific in that arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. versus a hypothetical x86 use case that might be completely ok with a lockless implementation. And it's not obvious that there's a plausible, safe use case outside of arm64, e.g. on x86, there is very, very little that is truly shared across the entire VM/system, most things are per-thread/core/package in some way, shape, or form. In other words, I'm a wary of providing something like this for x86 because odds are good that any use will be functionally incorrect.

Quoted 9 lines · Sean Christopherson

In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces unnecessary contention as it will serialize this bit of code if multiple vCPUs are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a VM will acquire kvm->lock and thus avoid contention once the VM is up and running. There's still a possibility that multiple vCPUs will contend for kvm->lock on their first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's a more elegant solution depending on the use case, e.g. a lockless approach might work (or it might not).

But is it safe to read kvm->vm_started without grabbing the lock in the first place?

use atomic_t maybe for this?

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3734

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                     kvm->vm_started = true;
+                     mutex_unlock(&kvm->lock);

Lastly, why is this in generic KVM?

The v1 of the series originally had it in the arm specific code. However, I was suggested to move it to the generic code since the book keeping is not arch specific and could be helpful to others too [1].

Quoted 9 lines

I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, but I'm skeptical in this particular case. The code _is_ arch specific in that arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. versus a hypothetical x86 use case that might be completely ok with a lockless implementation. And it's not obvious that there's a plausible, safe use case outside of arm64, e.g. on x86, there is very, very little that is truly shared across the entire VM/system, most things are per-thread/core/package in some way, shape, or form. In other words, I'm a wary of providing something like this for x86 because odds are good that any use will be functionally incorrect.

I've been going back and forth on this. I've seen a couple of variables declared in the generic struct and used only in the arch code. vcpu->valid_wakeup for instance, which is used only by s390 arch. Maybe I'm looking at it the wrong way as to what can and can't go in the generic kvm code.

Quoted 10 lines · Raghavendra Rao Ananta

Quoted 9 lines · Sean Christopherson

In your proposed patch, KVM_RUN will take kvm->lock _every_ time. That introduces unnecessary contention as it will serialize this bit of code if multiple vCPUs are attempting KVM_RUN. By checking !vm_started, only the "first" KVM_RUN for a VM will acquire kvm->lock and thus avoid contention once the VM is up and running. There's still a possibility that multiple vCPUs will contend for kvm->lock on their first KVM_RUN, hence the quotes. I called it "naive" because it's possible there's a more elegant solution depending on the use case, e.g. a lockless approach might work (or it might not).

Quoted 2 lines

But is it safe to read kvm->vm_started without grabbing the lock in the first place?

Don't know, but that's my point. Without a consumer in generic KVM and due to my lack of arm64 knowledge, without a high-level description of how the flag will be used by arm64, it's really difficult to determine what's safe and what's not. For other architectures, it's an impossible question to answer because we don't know how the flag might be used.

Quoted 1 line

use atomic_t maybe for this?

No. An atomic_t is generally useful only if there are multiple writers that can possibly write different values. It's highly unlikely that simply switching to an atomic address the needs of arm64.

Quoted 18 lines

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *filp, · line 3734

Quoted 2 lines

virt/kvm/kvm_main.c +2 -0

+                     kvm->vm_started = true;
+                     mutex_unlock(&kvm->lock);

Lastly, why is this in generic KVM?

The v1 of the series originally had it in the arm specific code. However, I was suggested to move it to the generic code since the book keeping is not arch specific and could be helpful to others too [1].

Quoted 9 lines

I'm definitely in favor of moving/adding thing to generic KVM when it makes sense, but I'm skeptical in this particular case. The code _is_ arch specific in that arm64 apparently needs to acquire kvm->lock when checking if a vCPU has run, e.g. versus a hypothetical x86 use case that might be completely ok with a lockless implementation. And it's not obvious that there's a plausible, safe use case outside of arm64, e.g. on x86, there is very, very little that is truly shared across the entire VM/system, most things are per-thread/core/package in some way, shape, or form. In other words, I'm a wary of providing something like this for x86 because odds are good that any use will be functionally incorrect.

Quoted 5 lines

I've been going back and forth on this. I've seen a couple of variables declared in the generic struct and used only in the arch code. vcpu->valid_wakeup for instance, which is used only by s390 arch. Maybe I'm looking at it the wrong way as to what can and can't go in the generic kvm code.

Ya, valid_wakeup is an oddball, I don't know why it's in kvm_vcpu instead of arch code that's wrapped with e.g. kvm_arch_vcpu_valid_wakeup().

That said, valid_wakeup is consumed by generic KVM, i.e. has well defined semantics for how it is used, so it's purely a "this code is rather odd" issue. vm_started on the other hand is only produced by generic KVM, and so its required semantics are unclear.

Understood. I'll move it to arm64 and we can refactor it if there's a need for any other arch(s).

Quoted 2 lines · Raghavendra Rao Ananta

Understood. I'll move it to arm64 and we can refactor it if there's a need for any other arch(s).

Before you do that, can you answer Jim's question on _why_ arm64 needs this?

Introduce the KVM ARM64 capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, to indicate the support for psuedo-firmware bitmap extension. Each of these registers holds a feature-set exposed to the guest in the form of a bitmap. If supported, a simple 'read' of the capability should return the number of psuedo-firmware registers supported. User-space can utilize this to discover the registers. It can further explore or modify the features using the classical GET/SET_ONE_REG interface.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++++
 include/uapi/linux/kvm.h       |  1 +
 2 files changed, 22 insertions(+)

Documentation/virt/kvm/api.rst +0 -0

index aeeb071c7688..646176537f2c 100644

Documentation/virt/kvm/api.rst +4 -0

+
+This capability indicates that KVM for arm64 supports the psuedo-firmware
+register bitmap extension. Each of these registers represent the features
+supported by a particular type in the form of a bitmap. By default, these

Documentation/virt/kvm/api.rst +1 -0

+
 8. Other capabilities.
 ======================
 

include/uapi/linux/kvm.h +1 -1

index 1daa45268de2..209b43dbbc3c 100644
@@ -1131,6 +1131,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
 #define KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM 206
+#define KVM_CAP_ARM_HVC_FW_REG_BMAP 207
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
--

2.34.1.448.ga2b2bfdf31-goog

Hi Raghu,

Quoted 24 lines · Raghavendra Rao Ananta

Introduce the KVM ARM64 capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, to indicate the support for psuedo-firmware bitmap extension. Each of these registers holds a feature-set exposed to the guest in the form of a bitmap. If supported, a simple 'read' of the capability should return the number of psuedo-firmware registers supported. User-space can utilize this to discover the registers. It can further explore or modify the features using the classical GET/SET_ONE_REG interface.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++++
 include/uapi/linux/kvm.h       |  1 +
 2 files changed, 22 insertions(+)

Documentation/virt/kvm/api.rst +0 -0

index aeeb071c7688..646176537f2c 100644
@@ -6925,6 +6925,27 @@ indicated by the fd to the VM this is called on.
 This is intended to support intra-host migration of VMs between userspace VMMs,
 upgrading the VMM process without interrupting the guest.
 

Documentation/virt/kvm/api.rst indicated by the fd to the VM this is called on. · line 6928

Quoted 1 line

Documentation/virt/kvm/api.rst +1 -0

+7.30 KVM_CAP_ARM_HVC_FW_REG_BMAP

IMHO, instead of including its format of the register in the name, including its purpose/function in the name might be better. e.g. KVM_CAP_ARM_HVC_FEATURE_REG ?
(Feature fields don't necessarily have to be in a bitmap format
if they don't fit well although I'm not sure if we have such fields.)

Documentation/virt/kvm/api.rst indicated by the fd to the VM this is called on. · line 6933

Quoted 4 lines

Documentation/virt/kvm/api.rst +4 -0

+
+:Architectures: arm64
+:Parameters: None
+:Returns: Number of psuedo-firmware registers supported

Looking at patch-4, the return value of this would be the number of pseudo-firmware *bitmap* registers supported.
BTW, "4.68 KVM_SET_ONE_REG" in the doc uses the word "arm64 firmware pseudo-registers". It would be nicer to use the same term.

Quoted 10 lines

Documentation/virt/kvm/api.rst +10 -0

+
+This capability indicates that KVM for arm64 supports the psuedo-firmware
+register bitmap extension. Each of these registers represent the features
+supported by a particular type in the form of a bitmap. By default, these
+registers are set with the upper limit of the features that are supported.
+
+The registers can be accessed via the standard SET_ONE_REG and KVM_GET_ONE_REG
+interfaces. The user-space is expected to read the number of these registers
+available by reading KVM_CAP_ARM_HVC_FW_REG_BMAP, read the current bitmap
+configuration via GET_ONE_REG for each register, and then write back the

Documentation/virt/kvm/api.rst indicated by the fd to the VM this is called on. · line 6947

Quoted 4 lines

Documentation/virt/kvm/api.rst +4 -0

+desired bitmap of features that it wishes the guest to see via SET_ONE_REG.
+
+Note that KVM doesn't allow the user-space to modify these registers after
+the VM (any of the vCPUs) has started running.

Since even if KVM_RUN fails, and the VM hasn't started yet, it will get immutable. So, "after any of the vCPUs run KVM_RUN." might be more clear ?

Thanks,
Reiji

Quoted 19 lines

Documentation/virt/kvm/api.rst +1 -0

+
 8. Other capabilities.
 ======================
 

include/uapi/linux/kvm.h +1 -1

index 1daa45268de2..209b43dbbc3c 100644
@@ -1131,6 +1131,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
 #define KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM 206
+#define KVM_CAP_ARM_HVC_FW_REG_BMAP 207
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
--

2.34.1.448.ga2b2bfdf31-goog

Quoted 29 lines · Reiji Watanabe

Hi Raghu,

Quoted 24 lines · Raghavendra Rao Ananta

Introduce the KVM ARM64 capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, to indicate the support for psuedo-firmware bitmap extension. Each of these registers holds a feature-set exposed to the guest in the form of a bitmap. If supported, a simple 'read' of the capability should return the number of psuedo-firmware registers supported. User-space can utilize this to discover the registers. It can further explore or modify the features using the classical GET/SET_ONE_REG interface.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 Documentation/virt/kvm/api.rst | 21 +++++++++++++++++++++
 include/uapi/linux/kvm.h       |  1 +
 2 files changed, 22 insertions(+)

Documentation/virt/kvm/api.rst +0 -0

index aeeb071c7688..646176537f2c 100644
@@ -6925,6 +6925,27 @@ indicated by the fd to the VM this is called on.
 This is intended to support intra-host migration of VMs between userspace VMMs,
 upgrading the VMM process without interrupting the guest.
 

Quoted 8 lines

Documentation/virt/kvm/api.rst indicated by the fd to the VM this is called on. · line 6928

Quoted 1 line

Documentation/virt/kvm/api.rst +1 -0

+7.30 KVM_CAP_ARM_HVC_FW_REG_BMAP

IMHO, instead of including its format of the register in the name, including its purpose/function in the name might be better. e.g. KVM_CAP_ARM_HVC_FEATURE_REG ?
(Feature fields don't necessarily have to be in a bitmap format
if they don't fit well although I'm not sure if we have such fields.)

Well we do have registers, KVM_REG_ARM_PSCI_VERSION for instance, that's not covered by this CAP. But sure, I can explicitly add 'FEATURES' to the name. I also wanted to explicitly convey that we are covering the *bitmapped* firmware registers here. But not sure if appending 'BMAP' might give an impression that the CAP itself is bitmapped.
Do you think KVM_CAP_ARM_HVC_BMAP_FEAT_REG is better?

Quoted 10 lines

Documentation/virt/kvm/api.rst indicated by the fd to the VM this is called on. · line 6933

Quoted 4 lines

Documentation/virt/kvm/api.rst +4 -0

+
+:Architectures: arm64
+:Parameters: None
+:Returns: Number of psuedo-firmware registers supported

Looking at patch-4, the return value of this would be the number of pseudo-firmware *bitmap* registers supported.
BTW, "4.68 KVM_SET_ONE_REG" in the doc uses the word "arm64 firmware pseudo-registers". It would be nicer to use the same term.

Nice catch. I'll fix it here in apr.rst.

Quoted 10 lines

Quoted 10 lines

Documentation/virt/kvm/api.rst +10 -0

+
+This capability indicates that KVM for arm64 supports the psuedo-firmware
+register bitmap extension. Each of these registers represent the features
+supported by a particular type in the form of a bitmap. By default, these
+registers are set with the upper limit of the features that are supported.
+
+The registers can be accessed via the standard SET_ONE_REG and KVM_GET_ONE_REG
+interfaces. The user-space is expected to read the number of these registers
+available by reading KVM_CAP_ARM_HVC_FW_REG_BMAP, read the current bitmap
+configuration via GET_ONE_REG for each register, and then write back the

Quoted 9 lines

Documentation/virt/kvm/api.rst indicated by the fd to the VM this is called on. · line 6947

Quoted 4 lines

Documentation/virt/kvm/api.rst +4 -0

+desired bitmap of features that it wishes the guest to see via SET_ONE_REG.
+
+Note that KVM doesn't allow the user-space to modify these registers after
+the VM (any of the vCPUs) has started running.

Since even if KVM_RUN fails, and the VM hasn't started yet, it will get immutable. So, "after any of the vCPUs run KVM_RUN." might be more clear ?

Sure, that's probably more clear. I'll fix it.

Regards,
Raghavendra

Quoted 24 lines

Thanks,
Reiji

Quoted 19 lines

Documentation/virt/kvm/api.rst +1 -0

+
 8. Other capabilities.
 ======================
 

include/uapi/linux/kvm.h +1 -1

index 1daa45268de2..209b43dbbc3c 100644
@@ -1131,6 +1131,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_EXIT_ON_EMULATION_FAILURE 204
 #define KVM_CAP_ARM_MTE 205
 #define KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM 206
+#define KVM_CAP_ARM_HVC_FW_REG_BMAP 207
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
--

2.34.1.448.ga2b2bfdf31-goog

KVM regularly introduces new hypercall services to the guests without any consent from the Virtual Machine Manager (VMM). This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

As a result, there's a need for the VMM to elect the services that it wishes the guest to discover. VMM can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware psuedo-registers, such as KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.

These firmware registers are categorized based on the service call owners, and unlike the existing firmware psuedo-registers, they hold the features supported in the form of a bitmap.

The capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, is used to announce this extension, which returns the number of psuedo-firmware registers supported. During the VM initialization, the registers holds an upper-limit of the features supported by the corresponding registers. It's expected that the VMMs discover the features provided by each register via GET_ONE_REG, and writeback the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started.

Older VMMs can simply ignore the capability and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility.

In this patch, the framework adds the register only for ARM's standard secure services (owner value 4). Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0. Other services are momentarily added in the upcoming patches.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h |  12 ++++
 arch/arm64/include/uapi/asm/kvm.h |   4 ++
 arch/arm64/kvm/arm.c              |   4 ++
 arch/arm64/kvm/hypercalls.c       | 103 +++++++++++++++++++++++++++++-
 arch/arm64/kvm/trng.c             |   8 +--
 include/kvm/arm_hypercalls.h      |   6 ++
 6 files changed, 129 insertions(+), 8 deletions(-)

arch/arm64/include/asm/kvm_host.h +12 -0

index 2a5f7f38006f..a32cded0371b 100644
@@ -102,6 +102,15 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+/**
+ * struct kvm_hvc_desc: KVM ARM64 hypercall descriptor
+ *
+ * @hvc_std_bmap: Bitmap of standard secure service calls
+ */
+struct kvm_hvc_desc {
+       u64 hvc_std_bmap;
+};
+
 struct kvm_arch {
        struct kvm_s2_mmu mmu;
 

@@ -137,6 +146,9 @@ struct kvm_arch {
 
        /* Memory Tagging Extension enabled for the guest */
        bool mte_enabled;
+
+       /* Hypercall firmware register' descriptor */
+       struct kvm_hvc_desc hvc_desc;
 };
 
 struct kvm_vcpu_fault_info {

arch/arm64/include/uapi/asm/kvm.h +4 -0

index b3edde68bc3e..0d6f29c58456 100644
@@ -281,6 +281,10 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED       3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED            (1U << 4)
 
+#define KVM_REG_ARM_STD_BMAP                   KVM_REG_ARM_FW_REG(3)
+#define KVM_REG_ARM_STD_BIT_TRNG_V1_0          BIT(0)
+#define KVM_REG_ARM_STD_BMAP_BIT_MAX           0       /* Last valid bit */
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE              (0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

arch/arm64/kvm/arm.c +1 -0

index e4727dc771bf..56fe81565235 100644
@@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
        kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
 
        set_default_spectre(kvm);
+       kvm_arm_init_hypercalls(kvm);
 
        return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/arm.c +0 -0

        default:
                r = 0;
        }

arch/arm64/kvm/hypercalls.c +12 -0

index 3c2fcf31ad3d..06243e4670eb 100644
@@ -58,6 +58,29 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
        val[3] = lower_32_bits(cycles);
 }
 
+static bool kvm_arm_fw_reg_feat_enabled(u64 reg_bmap, u64 feat_bit)
+{
+       return reg_bmap & feat_bit;
+}
+
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
+{
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
+
+       switch (func_id) {
+       case ARM_SMCCC_TRNG_VERSION:
+       case ARM_SMCCC_TRNG_FEATURES:

arch/arm64/kvm/hypercalls.c +27 -1

+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
        u32 func_id = smccc_get_function(vcpu);

@@ -65,6 +88,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
        u32 feature;
        gpa_t gpa;
 
+       if (!kvm_hvc_call_supported(vcpu, func_id))
+               goto out;
+
        switch (func_id) {
        case ARM_SMCCC_VERSION_FUNC_ID:
                val[0] = ARM_SMCCC_VERSION_1_1;

@@ -143,6 +169,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
                return kvm_psci_call(vcpu);
        }
 
+out:
        smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
        return 1;
 }

@@ -153,9 +180,25 @@ static const u64 kvm_arm_fw_reg_ids[] = {
        KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 };
 
+static const u64 kvm_arm_fw_reg_bmap_ids[] = {
+       KVM_REG_ARM_STD_BMAP,
+};
+
+void kvm_arm_init_hypercalls(struct kvm *kvm)
+{
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+       hvc_desc->hvc_std_bmap = ARM_SMCCC_STD_FEATURES;
+}
+
+int kvm_arm_num_fw_bmap_regs(void)
+{
+       return ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids);
+}
+
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
 {
-       return ARRAY_SIZE(kvm_arm_fw_reg_ids);
+       return ARRAY_SIZE(kvm_arm_fw_reg_ids) + kvm_arm_num_fw_bmap_regs();
 }
 
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

@@ -167,6 +210,11 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
                        return -EFAULT;
        }
 
+       for (i = 0; i < ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids); i++) {
+               if (put_user(kvm_arm_fw_reg_bmap_ids[i], uindices++))
+                       return -EFAULT;
+       }
+
        return 0;
 }
 

@@ -211,9 +259,20 @@ static int get_kernel_wa_level(u64 regid)
        return -EINVAL;

arch/arm64/kvm/hypercalls.c +45 -0

+}
+
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
        u64 val;
 
        switch (reg->id) {

@@ -224,6 +283,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
                val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
                break;
+       case KVM_REG_ARM_STD_BMAP:
+               kvm_arm_get_fw_reg_bmap(vcpu, hvc_desc->hvc_std_bmap, &val);
+               break;
        default:
                return -ENOENT;
        }

@@ -234,6 +296,43 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        return 0;
 }
 
+static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val)
+{
+       int ret = 0;
+       struct kvm *kvm = vcpu->kvm;
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+       u64 *fw_reg_bmap, fw_reg_features;
+
+       switch (reg_id) {
+       case KVM_REG_ARM_STD_BMAP:
+               fw_reg_bmap = &hvc_desc->hvc_std_bmap;
+               fw_reg_features = ARM_SMCCC_STD_FEATURES;
+               break;
+       default:
+               return -ENOENT;
+       }
+
+       /* Check for unsupported bit */
+       if (val & ~fw_reg_features)
+               return -EINVAL;
+
+       mutex_lock(&kvm->lock);
+
+       /*
+        * If the VM (any vCPU) has already started running, return success
+        * if there's no change in the value. Else, return -EBUSY.
+        */
+       if (kvm_vm_has_started(kvm)) {
+               ret = *fw_reg_bmap != val ? -EBUSY : 0;
+               goto out;
+       }
+
+       *fw_reg_bmap = val;
+out:
+       mutex_unlock(&kvm->lock);
+       return ret;
+}
+
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;

@@ -310,6 +409,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
                        return -EINVAL;
 
                return 0;
+       case KVM_REG_ARM_STD_BMAP:
+               return kvm_arm_set_fw_reg_bmap(vcpu, reg->id, val);
        default:
                return -ENOENT;
        }

arch/arm64/kvm/trng.c +0 -0

index 99bdd7103c9c..23f912514b06 100644
@@ -60,14 +60,8 @@ int kvm_trng_call(struct kvm_vcpu *vcpu)
                val = ARM_SMCCC_TRNG_VERSION_1_0;

arch/arm64/kvm/trng.c +0 -1

-               }
                break;
        case ARM_SMCCC_TRNG_GET_UUID:
                smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),

include/kvm/arm_hypercalls.h +6 -1

index 5d38628a8d04..8fe68d8d6d96 100644
@@ -6,6 +6,9 @@
 
 #include <asm/kvm_emulate.h>
 
+#define ARM_SMCCC_STD_FEATURES \
+       GENMASK_ULL(KVM_REG_ARM_STD_BMAP_BIT_MAX, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -42,9 +45,12 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 
 struct kvm_one_reg;
 
+void kvm_arm_init_hypercalls(struct kvm *kvm);
+int kvm_arm_num_fw_bmap_regs(void);
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id);
 
 #endif
--

2.34.1.448.ga2b2bfdf31-goog

Hi Raghu,

Quoted 107 lines · Raghavendra Rao Ananta

KVM regularly introduces new hypercall services to the guests without any consent from the Virtual Machine Manager (VMM). This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

As a result, there's a need for the VMM to elect the services that it wishes the guest to discover. VMM can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware psuedo-registers, such as KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.

These firmware registers are categorized based on the service call owners, and unlike the existing firmware psuedo-registers, they hold the features supported in the form of a bitmap.

The capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, is used to announce this extension, which returns the number of psuedo-firmware registers supported. During the VM initialization, the registers holds an upper-limit of the features supported by the corresponding registers. It's expected that the VMMs discover the features provided by each register via GET_ONE_REG, and writeback the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started.

Older VMMs can simply ignore the capability and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility.

In this patch, the framework adds the register only for ARM's standard secure services (owner value 4). Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0. Other services are momentarily added in the upcoming patches.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h |  12 ++++
 arch/arm64/include/uapi/asm/kvm.h |   4 ++
 arch/arm64/kvm/arm.c              |   4 ++
 arch/arm64/kvm/hypercalls.c       | 103 +++++++++++++++++++++++++++++-
 arch/arm64/kvm/trng.c             |   8 +--
 include/kvm/arm_hypercalls.h      |   6 ++
 6 files changed, 129 insertions(+), 8 deletions(-)

arch/arm64/include/asm/kvm_host.h +12 -0

index 2a5f7f38006f..a32cded0371b 100644
@@ -102,6 +102,15 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+/**
+ * struct kvm_hvc_desc: KVM ARM64 hypercall descriptor
+ *
+ * @hvc_std_bmap: Bitmap of standard secure service calls
+ */
+struct kvm_hvc_desc {
+       u64 hvc_std_bmap;
+};
+
 struct kvm_arch {
        struct kvm_s2_mmu mmu;
 

@@ -137,6 +146,9 @@ struct kvm_arch {
 
        /* Memory Tagging Extension enabled for the guest */
        bool mte_enabled;
+
+       /* Hypercall firmware register' descriptor */
+       struct kvm_hvc_desc hvc_desc;
 };
 
 struct kvm_vcpu_fault_info {

arch/arm64/include/uapi/asm/kvm.h +4 -0

index b3edde68bc3e..0d6f29c58456 100644
@@ -281,6 +281,10 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED       3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED            (1U << 4)
 
+#define KVM_REG_ARM_STD_BMAP                   KVM_REG_ARM_FW_REG(3)
+#define KVM_REG_ARM_STD_BIT_TRNG_V1_0          BIT(0)
+#define KVM_REG_ARM_STD_BMAP_BIT_MAX           0       /* Last valid bit */
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE              (0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

arch/arm64/kvm/arm.c +1 -0

index e4727dc771bf..56fe81565235 100644
@@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
        kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
 
        set_default_spectre(kvm);
+       kvm_arm_init_hypercalls(kvm);
 
        return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/arm.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 289

Quoted 6 lines

arch/arm64/kvm/arm.c +3 -0

@@ -283,6 +284,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
        case KVM_CAP_ARM_PTRAUTH_GENERIC:
                r = system_has_full_ptr_auth();
                break;
+       case KVM_CAP_ARM_HVC_FW_REG_BMAP:
+               r = kvm_arm_num_fw_bmap_regs();
+               break;

Looking at the discussion for the v2 series,

https://lore.kernel.org/kvmarm/[email protected]/

I assume that the number of the pseudo-firmware bitmap registers will be used to clear pseudo firmware bitmap registers that userspace doesn't know.
I'm wondering how userspace can identify which pseudo-firmware registers that KVM_GET_REG_LIST provides are the pseudo-firmware bitmap registers that it doesn't know.
For instance, suppose pseudo-firmware registers that KVM_GET_REG_LIST provides are KVM_REG_ARM_FW_REG(0) to KVM_REG_ARM_FW_REG(9), userspace doesn't knows KVM_REG_ARM_FW_REG(6) to KVM_REG_ARM_FW_REG(9), and KVM_CAP_ARM_HVC_FW_REG_BMAP returns 5, how can userspace identify remaining two bitmap registers from those 4 (fw-reg #6 to #9) firmware registers ?

Quoted 30 lines

arch/arm64/kvm/arm.c +0 -0

        default:
                r = 0;
        }

arch/arm64/kvm/hypercalls.c +18 -0

index 3c2fcf31ad3d..06243e4670eb 100644
@@ -58,6 +58,29 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
        val[3] = lower_32_bits(cycles);
 }
 
+static bool kvm_arm_fw_reg_feat_enabled(u64 reg_bmap, u64 feat_bit)
+{
+       return reg_bmap & feat_bit;
+}
+
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
+{
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
+
+       switch (func_id) {
+       case ARM_SMCCC_TRNG_VERSION:
+       case ARM_SMCCC_TRNG_FEATURES:
+       case ARM_SMCCC_TRNG_GET_UUID:
+       case ARM_SMCCC_TRNG_RND32:
+       case ARM_SMCCC_TRNG_RND64:
+               return kvm_arm_fw_reg_feat_enabled(hvc_desc->hvc_std_bmap,
+                                               KVM_REG_ARM_STD_BIT_TRNG_V1_0);
+       default:

arch/arm64/kvm/hypercalls.c static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val) · line 82

Quoted 4 lines

arch/arm64/kvm/hypercalls.c +4 -0

+               /* By default, allow the services that aren't listed here */
+               return true;
+       }
+}

kvm_hvc_call_supported() could return true even for @func_id that kvm_hvc_call_handler() returns -EINVAL for. Is this behavior what you really want ?

If so, IMHO the function name might be a bit mis-leading. "kvm_hvc_call_disabled" (and flip the return value) might be closer to what it does(?).

Quoted 66 lines

arch/arm64/kvm/hypercalls.c +27 -1

+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
        u32 func_id = smccc_get_function(vcpu);

@@ -65,6 +88,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
        u32 feature;
        gpa_t gpa;
 
+       if (!kvm_hvc_call_supported(vcpu, func_id))
+               goto out;
+
        switch (func_id) {
        case ARM_SMCCC_VERSION_FUNC_ID:
                val[0] = ARM_SMCCC_VERSION_1_1;

@@ -143,6 +169,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
                return kvm_psci_call(vcpu);
        }
 
+out:
        smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
        return 1;
 }

@@ -153,9 +180,25 @@ static const u64 kvm_arm_fw_reg_ids[] = {
        KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 };
 
+static const u64 kvm_arm_fw_reg_bmap_ids[] = {
+       KVM_REG_ARM_STD_BMAP,
+};
+
+void kvm_arm_init_hypercalls(struct kvm *kvm)
+{
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+       hvc_desc->hvc_std_bmap = ARM_SMCCC_STD_FEATURES;
+}
+
+int kvm_arm_num_fw_bmap_regs(void)
+{
+       return ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids);
+}
+
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
 {
-       return ARRAY_SIZE(kvm_arm_fw_reg_ids);
+       return ARRAY_SIZE(kvm_arm_fw_reg_ids) + kvm_arm_num_fw_bmap_regs();
 }
 
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

@@ -167,6 +210,11 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
                        return -EFAULT;
        }
 
+       for (i = 0; i < ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids); i++) {
+               if (put_user(kvm_arm_fw_reg_bmap_ids[i], uindices++))
+                       return -EFAULT;
+       }
+
        return 0;
 }
 

@@ -211,9 +259,20 @@ static int get_kernel_wa_level(u64 regid)
        return -EINVAL;
 }
 

arch/arm64/kvm/hypercalls.c static int get_kernel_wa_level(u64 regid) · line 269

Quoted 8 lines

arch/arm64/kvm/hypercalls.c +8 -0

+static void
+kvm_arm_get_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 fw_reg_bmap, u64 *val)
+{
+       struct kvm *kvm = vcpu->kvm;
+
+       mutex_lock(&kvm->lock);
+       *val = fw_reg_bmap;
+       mutex_unlock(&kvm->lock);

Why does it need to hold the lock ? (Wouldn't READ_ONCE be enough ?)

Quoted 86 lines

arch/arm64/kvm/hypercalls.c +45 -0

+}
+
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
        u64 val;
 
        switch (reg->id) {

@@ -224,6 +283,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
                val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
                break;
+       case KVM_REG_ARM_STD_BMAP:
+               kvm_arm_get_fw_reg_bmap(vcpu, hvc_desc->hvc_std_bmap, &val);
+               break;
        default:
                return -ENOENT;
        }

@@ -234,6 +296,43 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        return 0;
 }
 
+static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val)
+{
+       int ret = 0;
+       struct kvm *kvm = vcpu->kvm;
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+       u64 *fw_reg_bmap, fw_reg_features;
+
+       switch (reg_id) {
+       case KVM_REG_ARM_STD_BMAP:
+               fw_reg_bmap = &hvc_desc->hvc_std_bmap;
+               fw_reg_features = ARM_SMCCC_STD_FEATURES;
+               break;
+       default:
+               return -ENOENT;
+       }
+
+       /* Check for unsupported bit */
+       if (val & ~fw_reg_features)
+               return -EINVAL;
+
+       mutex_lock(&kvm->lock);
+
+       /*
+        * If the VM (any vCPU) has already started running, return success
+        * if there's no change in the value. Else, return -EBUSY.
+        */
+       if (kvm_vm_has_started(kvm)) {
+               ret = *fw_reg_bmap != val ? -EBUSY : 0;
+               goto out;
+       }
+
+       *fw_reg_bmap = val;
+out:
+       mutex_unlock(&kvm->lock);
+       return ret;
+}
+
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;

@@ -310,6 +409,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
                        return -EINVAL;
 
                return 0;
+       case KVM_REG_ARM_STD_BMAP:
+               return kvm_arm_set_fw_reg_bmap(vcpu, reg->id, val);
        default:
                return -ENOENT;
        }

arch/arm64/kvm/trng.c +0 -5

index 99bdd7103c9c..23f912514b06 100644
@@ -60,14 +60,8 @@ int kvm_trng_call(struct kvm_vcpu *vcpu)
                val = ARM_SMCCC_TRNG_VERSION_1_0;
                break;
        case ARM_SMCCC_TRNG_FEATURES:
-               switch (smccc_get_arg1(vcpu)) {
-               case ARM_SMCCC_TRNG_VERSION:
-               case ARM_SMCCC_TRNG_FEATURES:
-               case ARM_SMCCC_TRNG_GET_UUID:
-               case ARM_SMCCC_TRNG_RND32:

arch/arm64/kvm/trng.c int kvm_trng_call(struct kvm_vcpu *vcpu) · line 64

Quoted 3 lines

arch/arm64/kvm/trng.c +1 -1

-               case ARM_SMCCC_TRNG_RND64:
+               if (kvm_hvc_call_supported(vcpu, smccc_get_arg1(vcpu)))
                        val = TRNG_SUCCESS;

kvm_hvc_call_supported() returns true for any values that are not explicitly listed in kvm_hvc_call_supported() (i.e. it returns true even for @func_id that are not any of ARM_SMCCC_TRNG_*). So, I don't think it can simply use the current kvm_hvc_call_supported.

Thanks,
Reiji

Quoted 34 lines

arch/arm64/kvm/trng.c +0 -1

-               }
                break;
        case ARM_SMCCC_TRNG_GET_UUID:
                smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),

include/kvm/arm_hypercalls.h +6 -1

index 5d38628a8d04..8fe68d8d6d96 100644
@@ -6,6 +6,9 @@
 
 #include <asm/kvm_emulate.h>
 
+#define ARM_SMCCC_STD_FEATURES \
+       GENMASK_ULL(KVM_REG_ARM_STD_BMAP_BIT_MAX, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -42,9 +45,12 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 
 struct kvm_one_reg;
 
+void kvm_arm_init_hypercalls(struct kvm *kvm);
+int kvm_arm_num_fw_bmap_regs(void);
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id);
 
 #endif
--

2.34.1.448.ga2b2bfdf31-goog

Quoted 123 lines · Reiji Watanabe

Hi Raghu,

Quoted 107 lines · Raghavendra Rao Ananta

KVM regularly introduces new hypercall services to the guests without any consent from the Virtual Machine Manager (VMM). This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

As a result, there's a need for the VMM to elect the services that it wishes the guest to discover. VMM can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware psuedo-registers, such as KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.

These firmware registers are categorized based on the service call owners, and unlike the existing firmware psuedo-registers, they hold the features supported in the form of a bitmap.

The capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, is used to announce this extension, which returns the number of psuedo-firmware registers supported. During the VM initialization, the registers holds an upper-limit of the features supported by the corresponding registers. It's expected that the VMMs discover the features provided by each register via GET_ONE_REG, and writeback the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started.

Older VMMs can simply ignore the capability and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility.

In this patch, the framework adds the register only for ARM's standard secure services (owner value 4). Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0. Other services are momentarily added in the upcoming patches.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h |  12 ++++
 arch/arm64/include/uapi/asm/kvm.h |   4 ++
 arch/arm64/kvm/arm.c              |   4 ++
 arch/arm64/kvm/hypercalls.c       | 103 +++++++++++++++++++++++++++++-
 arch/arm64/kvm/trng.c             |   8 +--
 include/kvm/arm_hypercalls.h      |   6 ++
 6 files changed, 129 insertions(+), 8 deletions(-)

arch/arm64/include/asm/kvm_host.h +12 -0

index 2a5f7f38006f..a32cded0371b 100644
@@ -102,6 +102,15 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+/**
+ * struct kvm_hvc_desc: KVM ARM64 hypercall descriptor
+ *
+ * @hvc_std_bmap: Bitmap of standard secure service calls
+ */
+struct kvm_hvc_desc {
+       u64 hvc_std_bmap;
+};
+
 struct kvm_arch {
        struct kvm_s2_mmu mmu;
 

@@ -137,6 +146,9 @@ struct kvm_arch {
 
        /* Memory Tagging Extension enabled for the guest */
        bool mte_enabled;
+
+       /* Hypercall firmware register' descriptor */
+       struct kvm_hvc_desc hvc_desc;
 };
 
 struct kvm_vcpu_fault_info {

arch/arm64/include/uapi/asm/kvm.h +4 -0

index b3edde68bc3e..0d6f29c58456 100644
@@ -281,6 +281,10 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED       3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED            (1U << 4)
 
+#define KVM_REG_ARM_STD_BMAP                   KVM_REG_ARM_FW_REG(3)
+#define KVM_REG_ARM_STD_BIT_TRNG_V1_0          BIT(0)
+#define KVM_REG_ARM_STD_BMAP_BIT_MAX           0       /* Last valid bit */
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE              (0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

arch/arm64/kvm/arm.c +1 -0

index e4727dc771bf..56fe81565235 100644
@@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
        kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
 
        set_default_spectre(kvm);
+       kvm_arm_init_hypercalls(kvm);
 
        return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/arm.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 289

Quoted 6 lines

arch/arm64/kvm/arm.c +3 -0

@@ -283,6 +284,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
        case KVM_CAP_ARM_PTRAUTH_GENERIC:
                r = system_has_full_ptr_auth();
                break;
+       case KVM_CAP_ARM_HVC_FW_REG_BMAP:
+               r = kvm_arm_num_fw_bmap_regs();
+               break;

Looking at the discussion for the v2 series,

https://lore.kernel.org/kvmarm/[email protected]/

Quoted 13 lines

I assume that the number of the pseudo-firmware bitmap registers will be used to clear pseudo firmware bitmap registers that userspace doesn't know.
I'm wondering how userspace can identify which pseudo-firmware registers that KVM_GET_REG_LIST provides are the pseudo-firmware bitmap registers that it doesn't know.
For instance, suppose pseudo-firmware registers that KVM_GET_REG_LIST provides are KVM_REG_ARM_FW_REG(0) to KVM_REG_ARM_FW_REG(9), userspace doesn't knows KVM_REG_ARM_FW_REG(6) to KVM_REG_ARM_FW_REG(9), and KVM_CAP_ARM_HVC_FW_REG_BMAP returns 5, how can userspace identify remaining two bitmap registers from those 4 (fw-reg #6 to #9) firmware registers ?

In v3, we leave the decision upto the userspace. If the userspace encounters a register that it's unaware, it can choose either to clear it or let it get exposed to the guest as is (see the code snipped shared by Andrew in the link).
Trying to understand the question better- are you asking how would userspace distinguish between bitmap and regular fw registers with intermixed sequence numbers?

If yes, do you foresee a reason why they 'unaware' registers needed to be treated differently?

Quoted 30 lines

Quoted 30 lines

arch/arm64/kvm/arm.c +0 -0

        default:
                r = 0;
        }

arch/arm64/kvm/hypercalls.c +18 -0

index 3c2fcf31ad3d..06243e4670eb 100644
@@ -58,6 +58,29 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
        val[3] = lower_32_bits(cycles);
 }
 
+static bool kvm_arm_fw_reg_feat_enabled(u64 reg_bmap, u64 feat_bit)
+{
+       return reg_bmap & feat_bit;
+}
+
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
+{
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
+
+       switch (func_id) {
+       case ARM_SMCCC_TRNG_VERSION:
+       case ARM_SMCCC_TRNG_FEATURES:
+       case ARM_SMCCC_TRNG_GET_UUID:
+       case ARM_SMCCC_TRNG_RND32:
+       case ARM_SMCCC_TRNG_RND64:
+               return kvm_arm_fw_reg_feat_enabled(hvc_desc->hvc_std_bmap,
+                                               KVM_REG_ARM_STD_BIT_TRNG_V1_0);
+       default:

Quoted 8 lines

arch/arm64/kvm/hypercalls.c static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val) · line 82

Quoted 4 lines

arch/arm64/kvm/hypercalls.c +4 -0

+               /* By default, allow the services that aren't listed here */
+               return true;
+       }
+}

kvm_hvc_call_supported() could return true even for @func_id that kvm_hvc_call_handler() returns -EINVAL for. Is this behavior what you really want ?

Yes. My idea was to let kvm_hvc_call_supported() check for the support, while kvm_hvc_call_handler() does the real processing of the call.

Quoted 4 lines

If so, IMHO the function name might be a bit mis-leading. "kvm_hvc_call_disabled" (and flip the return value) might be closer to what it does(?).

Sorry, I'm unclear how flipping is helping. Wouldn't we return 'false' if we don't have a case for the func_id, indicating it's NOT disabled, but kvm_hvc_call_handler() can still return SMCCC_RET_NOT_SUPPORTED?

Quoted 66 lines

Quoted 66 lines

arch/arm64/kvm/hypercalls.c +27 -1

+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
        u32 func_id = smccc_get_function(vcpu);

@@ -65,6 +88,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
        u32 feature;
        gpa_t gpa;
 
+       if (!kvm_hvc_call_supported(vcpu, func_id))
+               goto out;
+
        switch (func_id) {
        case ARM_SMCCC_VERSION_FUNC_ID:
                val[0] = ARM_SMCCC_VERSION_1_1;

@@ -143,6 +169,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
                return kvm_psci_call(vcpu);
        }
 
+out:
        smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
        return 1;
 }

@@ -153,9 +180,25 @@ static const u64 kvm_arm_fw_reg_ids[] = {
        KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 };
 
+static const u64 kvm_arm_fw_reg_bmap_ids[] = {
+       KVM_REG_ARM_STD_BMAP,
+};
+
+void kvm_arm_init_hypercalls(struct kvm *kvm)
+{
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+       hvc_desc->hvc_std_bmap = ARM_SMCCC_STD_FEATURES;
+}
+
+int kvm_arm_num_fw_bmap_regs(void)
+{
+       return ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids);
+}
+
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
 {
-       return ARRAY_SIZE(kvm_arm_fw_reg_ids);
+       return ARRAY_SIZE(kvm_arm_fw_reg_ids) + kvm_arm_num_fw_bmap_regs();
 }
 
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

@@ -167,6 +210,11 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
                        return -EFAULT;
        }
 
+       for (i = 0; i < ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids); i++) {
+               if (put_user(kvm_arm_fw_reg_bmap_ids[i], uindices++))
+                       return -EFAULT;
+       }
+
        return 0;
 }
 

@@ -211,9 +259,20 @@ static int get_kernel_wa_level(u64 regid)
        return -EINVAL;
 }
 

Quoted 11 lines

arch/arm64/kvm/hypercalls.c static int get_kernel_wa_level(u64 regid) · line 269

Quoted 8 lines

arch/arm64/kvm/hypercalls.c +8 -0

+static void
+kvm_arm_get_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 fw_reg_bmap, u64 *val)
+{
+       struct kvm *kvm = vcpu->kvm;
+
+       mutex_lock(&kvm->lock);
+       *val = fw_reg_bmap;
+       mutex_unlock(&kvm->lock);

Why does it need to hold the lock ? (Wouldn't READ_ONCE be enough ?)

I don't have much experience with READ_ONCE at this point, but do you think this read can be protected again the read/write in kvm_arm_set_fw_reg_bmap()?

Quoted 86 lines

Quoted 86 lines

arch/arm64/kvm/hypercalls.c +45 -0

+}
+
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
        u64 val;
 
        switch (reg->id) {

@@ -224,6 +283,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
                val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
                break;
+       case KVM_REG_ARM_STD_BMAP:
+               kvm_arm_get_fw_reg_bmap(vcpu, hvc_desc->hvc_std_bmap, &val);
+               break;
        default:
                return -ENOENT;
        }

@@ -234,6 +296,43 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        return 0;
 }
 
+static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val)
+{
+       int ret = 0;
+       struct kvm *kvm = vcpu->kvm;
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+       u64 *fw_reg_bmap, fw_reg_features;
+
+       switch (reg_id) {
+       case KVM_REG_ARM_STD_BMAP:
+               fw_reg_bmap = &hvc_desc->hvc_std_bmap;
+               fw_reg_features = ARM_SMCCC_STD_FEATURES;
+               break;
+       default:
+               return -ENOENT;
+       }
+
+       /* Check for unsupported bit */
+       if (val & ~fw_reg_features)
+               return -EINVAL;
+
+       mutex_lock(&kvm->lock);
+
+       /*
+        * If the VM (any vCPU) has already started running, return success
+        * if there's no change in the value. Else, return -EBUSY.
+        */
+       if (kvm_vm_has_started(kvm)) {
+               ret = *fw_reg_bmap != val ? -EBUSY : 0;
+               goto out;
+       }
+
+       *fw_reg_bmap = val;
+out:
+       mutex_unlock(&kvm->lock);
+       return ret;
+}
+
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;

@@ -310,6 +409,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
                        return -EINVAL;
 
                return 0;
+       case KVM_REG_ARM_STD_BMAP:
+               return kvm_arm_set_fw_reg_bmap(vcpu, reg->id, val);
        default:
                return -ENOENT;
        }

arch/arm64/kvm/trng.c +0 -5

index 99bdd7103c9c..23f912514b06 100644
@@ -60,14 +60,8 @@ int kvm_trng_call(struct kvm_vcpu *vcpu)
                val = ARM_SMCCC_TRNG_VERSION_1_0;
                break;
        case ARM_SMCCC_TRNG_FEATURES:
-               switch (smccc_get_arg1(vcpu)) {
-               case ARM_SMCCC_TRNG_VERSION:
-               case ARM_SMCCC_TRNG_FEATURES:
-               case ARM_SMCCC_TRNG_GET_UUID:
-               case ARM_SMCCC_TRNG_RND32:

Quoted 9 lines

arch/arm64/kvm/trng.c int kvm_trng_call(struct kvm_vcpu *vcpu) · line 64

Quoted 3 lines

arch/arm64/kvm/trng.c +1 -1

-               case ARM_SMCCC_TRNG_RND64:
+               if (kvm_hvc_call_supported(vcpu, smccc_get_arg1(vcpu)))
                        val = TRNG_SUCCESS;

kvm_hvc_call_supported() returns true for any values that are not explicitly listed in kvm_hvc_call_supported() (i.e. it returns true even for @func_id that are not any of ARM_SMCCC_TRNG_*). So, I don't think it can simply use the current kvm_hvc_call_supported.

You are right. Probably I should leave the case statements as is (or think of some better way).

Thanks for the review and suggestions.

Regards,
Raghavendra

Quoted 37 lines

Thanks,
Reiji

Quoted 34 lines

arch/arm64/kvm/trng.c +0 -1

-               }
                break;
        case ARM_SMCCC_TRNG_GET_UUID:
                smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),

include/kvm/arm_hypercalls.h +6 -1

index 5d38628a8d04..8fe68d8d6d96 100644
@@ -6,6 +6,9 @@
 
 #include <asm/kvm_emulate.h>
 
+#define ARM_SMCCC_STD_FEATURES \
+       GENMASK_ULL(KVM_REG_ARM_STD_BMAP_BIT_MAX, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -42,9 +45,12 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 
 struct kvm_one_reg;
 
+void kvm_arm_init_hypercalls(struct kvm *kvm);
+int kvm_arm_num_fw_bmap_regs(void);
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id);
 
 #endif
--

2.34.1.448.ga2b2bfdf31-goog

Quoted 138 lines · Raghavendra Rao Ananta

Quoted 123 lines · Reiji Watanabe

Hi Raghu,

Quoted 107 lines · Raghavendra Rao Ananta

KVM regularly introduces new hypercall services to the guests without any consent from the Virtual Machine Manager (VMM). This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

As a result, there's a need for the VMM to elect the services that it wishes the guest to discover. VMM can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware psuedo-registers, such as KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.

These firmware registers are categorized based on the service call owners, and unlike the existing firmware psuedo-registers, they hold the features supported in the form of a bitmap.

The capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, is used to announce this extension, which returns the number of psuedo-firmware registers supported. During the VM initialization, the registers holds an upper-limit of the features supported by the corresponding registers. It's expected that the VMMs discover the features provided by each register via GET_ONE_REG, and writeback the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started.

Older VMMs can simply ignore the capability and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility.

In this patch, the framework adds the register only for ARM's standard secure services (owner value 4). Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0. Other services are momentarily added in the upcoming patches.

Signed-off-by Raghavendra Rao Ananta <[email protected]>

---
 arch/arm64/include/asm/kvm_host.h |  12 ++++
 arch/arm64/include/uapi/asm/kvm.h |   4 ++
 arch/arm64/kvm/arm.c              |   4 ++
 arch/arm64/kvm/hypercalls.c       | 103 +++++++++++++++++++++++++++++-
 arch/arm64/kvm/trng.c             |   8 +--
 include/kvm/arm_hypercalls.h      |   6 ++
 6 files changed, 129 insertions(+), 8 deletions(-)

arch/arm64/include/asm/kvm_host.h +12 -0

index 2a5f7f38006f..a32cded0371b 100644
@@ -102,6 +102,15 @@ struct kvm_s2_mmu {
 struct kvm_arch_memory_slot {
 };
 
+/**
+ * struct kvm_hvc_desc: KVM ARM64 hypercall descriptor
+ *
+ * @hvc_std_bmap: Bitmap of standard secure service calls
+ */
+struct kvm_hvc_desc {
+       u64 hvc_std_bmap;
+};
+
 struct kvm_arch {
        struct kvm_s2_mmu mmu;
 

@@ -137,6 +146,9 @@ struct kvm_arch {
 
        /* Memory Tagging Extension enabled for the guest */
        bool mte_enabled;
+
+       /* Hypercall firmware register' descriptor */
+       struct kvm_hvc_desc hvc_desc;
 };
 
 struct kvm_vcpu_fault_info {

arch/arm64/include/uapi/asm/kvm.h +4 -0

index b3edde68bc3e..0d6f29c58456 100644
@@ -281,6 +281,10 @@ struct kvm_arm_copy_mte_tags {
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_NOT_REQUIRED       3
 #define KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2_ENABLED            (1U << 4)
 
+#define KVM_REG_ARM_STD_BMAP                   KVM_REG_ARM_FW_REG(3)
+#define KVM_REG_ARM_STD_BIT_TRNG_V1_0          BIT(0)
+#define KVM_REG_ARM_STD_BMAP_BIT_MAX           0       /* Last valid bit */
+
 /* SVE registers */
 #define KVM_REG_ARM64_SVE              (0x15 << KVM_REG_ARM_COPROC_SHIFT)
 

arch/arm64/kvm/arm.c +1 -0

index e4727dc771bf..56fe81565235 100644
@@ -156,6 +156,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
        kvm->arch.max_vcpus = kvm_arm_default_max_vcpus();
 
        set_default_spectre(kvm);
+       kvm_arm_init_hypercalls(kvm);
 
        return ret;
 out_free_stage2_pgd:

arch/arm64/kvm/arm.c int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext) · line 289

Quoted 6 lines

arch/arm64/kvm/arm.c +3 -0

@@ -283,6 +284,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
        case KVM_CAP_ARM_PTRAUTH_GENERIC:
                r = system_has_full_ptr_auth();
                break;
+       case KVM_CAP_ARM_HVC_FW_REG_BMAP:
+               r = kvm_arm_num_fw_bmap_regs();
+               break;

Looking at the discussion for the v2 series,

https://lore.kernel.org/kvmarm/[email protected]/

Quoted 13 lines

I assume that the number of the pseudo-firmware bitmap registers will be used to clear pseudo firmware bitmap registers that userspace doesn't know.
I'm wondering how userspace can identify which pseudo-firmware registers that KVM_GET_REG_LIST provides are the pseudo-firmware bitmap registers that it doesn't know.
For instance, suppose pseudo-firmware registers that KVM_GET_REG_LIST provides are KVM_REG_ARM_FW_REG(0) to KVM_REG_ARM_FW_REG(9), userspace doesn't knows KVM_REG_ARM_FW_REG(6) to KVM_REG_ARM_FW_REG(9), and KVM_CAP_ARM_HVC_FW_REG_BMAP returns 5, how can userspace identify remaining two bitmap registers from those 4 (fw-reg #6 to #9) firmware registers ?

Quoted 7 lines

In v3, we leave the decision upto the userspace. If the userspace encounters a register that it's unaware, it can choose either to clear it or let it get exposed to the guest as is (see the code snipped shared by Andrew in the link).
Trying to understand the question better- are you asking how would userspace distinguish between bitmap and regular fw registers with intermixed sequence numbers?

Yes, that's my question.

Quoted 2 lines

If yes, do you foresee a reason why they 'unaware' registers needed to be treated differently?

Since I'm not sure what the specification of 'unaware' (non-bitmap) registers will be, it would be safer for us to assume that they might need to be treated differently from the bitmap registers. Considering there is KVM_REG_ARM_PSCI_VERSION, which KVM doesn't allow userspace to set to 0, there might be similar registers that userspace cannot set to 0 in the future.

BTW, If you assume that all those 'unaware' firmware registers are treated in the same way, I don't think userspace needs the number of those bitmap registers from KVM_CAP_ARM_HVC_FW_REG_BMAP (Instead, I would think it can handle the 'unaware' registers with a list of firmware registers from KVM_GET_REG_LIST).

Quoted 42 lines

Quoted 30 lines

Quoted 30 lines

arch/arm64/kvm/arm.c +0 -0

        default:
                r = 0;
        }

arch/arm64/kvm/hypercalls.c +18 -0

index 3c2fcf31ad3d..06243e4670eb 100644
@@ -58,6 +58,29 @@ static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val)
        val[3] = lower_32_bits(cycles);
 }
 
+static bool kvm_arm_fw_reg_feat_enabled(u64 reg_bmap, u64 feat_bit)
+{
+       return reg_bmap & feat_bit;
+}
+
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id)
+{
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
+
+       switch (func_id) {
+       case ARM_SMCCC_TRNG_VERSION:
+       case ARM_SMCCC_TRNG_FEATURES:
+       case ARM_SMCCC_TRNG_GET_UUID:
+       case ARM_SMCCC_TRNG_RND32:
+       case ARM_SMCCC_TRNG_RND64:
+               return kvm_arm_fw_reg_feat_enabled(hvc_desc->hvc_std_bmap,
+                                               KVM_REG_ARM_STD_BIT_TRNG_V1_0);
+       default:

Quoted 8 lines

arch/arm64/kvm/hypercalls.c static void kvm_ptp_get_time(struct kvm_vcpu *vcpu, u64 *val) · line 82

Quoted 4 lines

arch/arm64/kvm/hypercalls.c +4 -0

+               /* By default, allow the services that aren't listed here */
+               return true;
+       }
+}

kvm_hvc_call_supported() could return true even for @func_id that kvm_hvc_call_handler() returns -EINVAL for. Is this behavior what you really want ?

Yes. My idea was to let kvm_hvc_call_supported() check for the support, while kvm_hvc_call_handler() does the real processing of the call.

Quoted 7 lines

Quoted 4 lines

If so, IMHO the function name might be a bit mis-leading. "kvm_hvc_call_disabled" (and flip the return value) might be closer to what it does(?).

Sorry, I'm unclear how flipping is helping. Wouldn't we return 'false' if we don't have a case for the func_id, indicating it's NOT disabled, but kvm_hvc_call_handler() can still return SMCCC_RET_NOT_SUPPORTED?

Yes, that's fine, too.
Since those services are disabled (because they are enabled by default), I just thought checking 'disabled' might be closer to what it does than checking 'enabled'. But, 'enabled' is also fine.

Quoted 77 lines

Quoted 66 lines

Quoted 66 lines

arch/arm64/kvm/hypercalls.c +27 -1

+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 {
        u32 func_id = smccc_get_function(vcpu);

@@ -65,6 +88,9 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
        u32 feature;
        gpa_t gpa;
 
+       if (!kvm_hvc_call_supported(vcpu, func_id))
+               goto out;
+
        switch (func_id) {
        case ARM_SMCCC_VERSION_FUNC_ID:
                val[0] = ARM_SMCCC_VERSION_1_1;

@@ -143,6 +169,7 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
                return kvm_psci_call(vcpu);
        }
 
+out:
        smccc_set_retval(vcpu, val[0], val[1], val[2], val[3]);
        return 1;
 }

@@ -153,9 +180,25 @@ static const u64 kvm_arm_fw_reg_ids[] = {
        KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2,
 };
 
+static const u64 kvm_arm_fw_reg_bmap_ids[] = {
+       KVM_REG_ARM_STD_BMAP,
+};
+
+void kvm_arm_init_hypercalls(struct kvm *kvm)
+{
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+
+       hvc_desc->hvc_std_bmap = ARM_SMCCC_STD_FEATURES;
+}
+
+int kvm_arm_num_fw_bmap_regs(void)
+{
+       return ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids);
+}
+
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu)
 {
-       return ARRAY_SIZE(kvm_arm_fw_reg_ids);
+       return ARRAY_SIZE(kvm_arm_fw_reg_ids) + kvm_arm_num_fw_bmap_regs();
 }
 
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)

@@ -167,6 +210,11 @@ int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
                        return -EFAULT;
        }
 
+       for (i = 0; i < ARRAY_SIZE(kvm_arm_fw_reg_bmap_ids); i++) {
+               if (put_user(kvm_arm_fw_reg_bmap_ids[i], uindices++))
+                       return -EFAULT;
+       }
+
        return 0;
 }
 

@@ -211,9 +259,20 @@ static int get_kernel_wa_level(u64 regid)
        return -EINVAL;
 }
 

Quoted 11 lines

arch/arm64/kvm/hypercalls.c static int get_kernel_wa_level(u64 regid) · line 269

Quoted 8 lines

arch/arm64/kvm/hypercalls.c +8 -0

+static void
+kvm_arm_get_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 fw_reg_bmap, u64 *val)
+{
+       struct kvm *kvm = vcpu->kvm;
+
+       mutex_lock(&kvm->lock);
+       *val = fw_reg_bmap;
+       mutex_unlock(&kvm->lock);

Why does it need to hold the lock ? (Wouldn't READ_ONCE be enough ?)

Quoted 3 lines

I don't have much experience with READ_ONCE at this point, but do you think this read can be protected again the read/write in kvm_arm_set_fw_reg_bmap()?

If kvm_arm_set_fw_reg_bmap is changed to use WRITE_ONCE to update hvc_desc->hvc_*_bmap (kvm_arm_set_fw_reg_bmap still needs to get the lock to prevent other vCPUs from running KVM_RUN), I would think using READ_ONCE in kvm_arm_get_fw_reg_bmap() without getting the lock should work (will see either old or new value).

Thanks,
Reiji

Quoted 136 lines

Quoted 86 lines

Quoted 86 lines

arch/arm64/kvm/hypercalls.c +45 -0

+}
+
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;
+       struct kvm_hvc_desc *hvc_desc = &vcpu->kvm->arch.hvc_desc;
        u64 val;
 
        switch (reg->id) {

@@ -224,6 +283,9 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        case KVM_REG_ARM_SMCCC_ARCH_WORKAROUND_2:
                val = get_kernel_wa_level(reg->id) & KVM_REG_FEATURE_LEVEL_MASK;
                break;
+       case KVM_REG_ARM_STD_BMAP:
+               kvm_arm_get_fw_reg_bmap(vcpu, hvc_desc->hvc_std_bmap, &val);
+               break;
        default:
                return -ENOENT;
        }

@@ -234,6 +296,43 @@ int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
        return 0;
 }
 
+static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val)
+{
+       int ret = 0;
+       struct kvm *kvm = vcpu->kvm;
+       struct kvm_hvc_desc *hvc_desc = &kvm->arch.hvc_desc;
+       u64 *fw_reg_bmap, fw_reg_features;
+
+       switch (reg_id) {
+       case KVM_REG_ARM_STD_BMAP:
+               fw_reg_bmap = &hvc_desc->hvc_std_bmap;
+               fw_reg_features = ARM_SMCCC_STD_FEATURES;
+               break;
+       default:
+               return -ENOENT;
+       }
+
+       /* Check for unsupported bit */
+       if (val & ~fw_reg_features)
+               return -EINVAL;
+
+       mutex_lock(&kvm->lock);
+
+       /*
+        * If the VM (any vCPU) has already started running, return success
+        * if there's no change in the value. Else, return -EBUSY.
+        */
+       if (kvm_vm_has_started(kvm)) {
+               ret = *fw_reg_bmap != val ? -EBUSY : 0;
+               goto out;
+       }
+
+       *fw_reg_bmap = val;
+out:
+       mutex_unlock(&kvm->lock);
+       return ret;
+}
+
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 {
        void __user *uaddr = (void __user *)(long)reg->addr;

@@ -310,6 +409,8 @@ int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
                        return -EINVAL;
 
                return 0;
+       case KVM_REG_ARM_STD_BMAP:
+               return kvm_arm_set_fw_reg_bmap(vcpu, reg->id, val);
        default:
                return -ENOENT;
        }

arch/arm64/kvm/trng.c +0 -5

index 99bdd7103c9c..23f912514b06 100644
@@ -60,14 +60,8 @@ int kvm_trng_call(struct kvm_vcpu *vcpu)
                val = ARM_SMCCC_TRNG_VERSION_1_0;
                break;
        case ARM_SMCCC_TRNG_FEATURES:
-               switch (smccc_get_arg1(vcpu)) {
-               case ARM_SMCCC_TRNG_VERSION:
-               case ARM_SMCCC_TRNG_FEATURES:
-               case ARM_SMCCC_TRNG_GET_UUID:
-               case ARM_SMCCC_TRNG_RND32:

Quoted 9 lines

arch/arm64/kvm/trng.c int kvm_trng_call(struct kvm_vcpu *vcpu) · line 64

Quoted 3 lines

arch/arm64/kvm/trng.c +1 -1

-               case ARM_SMCCC_TRNG_RND64:
+               if (kvm_hvc_call_supported(vcpu, smccc_get_arg1(vcpu)))
                        val = TRNG_SUCCESS;

kvm_hvc_call_supported() returns true for any values that are not explicitly listed in kvm_hvc_call_supported() (i.e. it returns true even for @func_id that are not any of ARM_SMCCC_TRNG_*). So, I don't think it can simply use the current kvm_hvc_call_supported.

You are right. Probably I should leave the case statements as is (or think of some better way).

Thanks for the review and suggestions.

Regards,
Raghavendra

Quoted 33 lines

Thanks,
Reiji

Quoted 30 lines

arch/arm64/kvm/trng.c +0 -1

-               }
                break;
        case ARM_SMCCC_TRNG_GET_UUID:
                smccc_set_retval(vcpu, le32_to_cpu(u[0]), le32_to_cpu(u[1]),

include/kvm/arm_hypercalls.h +6 -0

index 5d38628a8d04..8fe68d8d6d96 100644
@@ -6,6 +6,9 @@
 
 #include <asm/kvm_emulate.h>
 
+#define ARM_SMCCC_STD_FEATURES \
+       GENMASK_ULL(KVM_REG_ARM_STD_BMAP_BIT_MAX, 0)
+
 int kvm_hvc_call_handler(struct kvm_vcpu *vcpu);
 
 static inline u32 smccc_get_function(struct kvm_vcpu *vcpu)

@@ -42,9 +45,12 @@ static inline void smccc_set_retval(struct kvm_vcpu *vcpu,
 
 struct kvm_one_reg;
 
+void kvm_arm_init_hypercalls(struct kvm *kvm);
+int kvm_arm_num_fw_bmap_regs(void);
 int kvm_arm_get_fw_num_regs(struct kvm_vcpu *vcpu);
 int kvm_arm_copy_fw_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
 int kvm_arm_get_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
 int kvm_arm_set_fw_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+bool kvm_hvc_call_supported(struct kvm_vcpu *vcpu, u32 func_id);
 

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

include/kvm/arm_hypercalls.h +0 -1

 #endif
--

2.34.1.448.ga2b2bfdf31-goog

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

arch/arm64/kvm/hypercalls.c static int get_kernel_wa_level(u64 regid) · line 269

Quoted 8 lines

arch/arm64/kvm/hypercalls.c +8 -0

+static void
+kvm_arm_get_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 fw_reg_bmap, u64 *val)
+{
+       struct kvm *kvm = vcpu->kvm;
+
+       mutex_lock(&kvm->lock);
+       *val = fw_reg_bmap;
+       mutex_unlock(&kvm->lock);

KVM regularly introduces new hypercall services to the guests without any consent from the Virtual Machine Manager (VMM). This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

As a result, there's a need for the VMM to elect the services that it wishes the guest to discover. VMM can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware psuedo-registers, such as KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.

These firmware registers are categorized based on the service call owners, and unlike the existing firmware psuedo-registers, they hold the features supported in the form of a bitmap.

The capability, KVM_CAP_ARM_HVC_FW_REG_BMAP, is used to announce this extension, which returns the number of psuedo-firmware registers supported. During the VM initialization, the registers holds an upper-limit of the features supported by the corresponding registers. It's expected that the VMMs discover the features provided by each register via GET_ONE_REG, and writeback the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started.

Older VMMs can simply ignore the capability and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility.

In this patch, the framework adds the register only for ARM's standard secure services (owner value 4). Currently, this includes support only for ARM True Random Number Generator (TRNG) service, with bit-0 of the register representing mandatory features of v1.0. Other services are momentarily added in the upcoming patches.

Signed-off-by Raghavendra Rao Ananta<[email protected]>

Quoted 9 lines · Raghavendra Rao Ananta

KVM regularly introduces new hypercall services to the guests without any consent from the Virtual Machine Manager (VMM). This means, the guests can observe hypercall services in and out as they migrate across various host kernel versions. This could be a major problem if the guest discovered a hypercall, started using it, and after getting migrated to an older kernel realizes that it's no longer available. Depending on how the guest handles the change, there's a potential chance that the guest would just panic.

Quoted 5 lines

As a result, there's a need for the VMM to elect the services that it wishes the guest to discover. VMM can elect these services based on the kernels spread across its (migration) fleet. To remedy this, extend the existing firmware psuedo-registers, such as KVM_REG_ARM_PSCI_VERSION, for all the hypercall services available.

Haven't gone through the series but I wonder whether it's better to have a (e)BPF filter for this like seccomp.