On Tue, Dec 08, 2020 at 03:56:39PM +0000, Marc Zyngier wrote: > On 2020-12-08 14:24, David Brazdil wrote: > > PSCI driver exposes a struct containing the PSCI v0.1 function IDs > > configured in the DT. However, the struct does not convey the > > information whether these were set from DT or contain the default value > > zero. This could be a problem for PSCI proxy in KVM protected mode. > > > > Extend config passed to KVM with a bit mask with individual bits set > > depending on whether the corresponding function pointer in psci_ops is > > set, eg. set bit for PSCI_CPU_SUSPEND if psci_ops.cpu_suspend != NULL. > > > > Previously config was split into multiple global variables. Put > > everything into a single struct for convenience. > > > > Reported-by: Mark Rutland <mark.rutl...@arm.com> > > Signed-off-by: David Brazdil <dbraz...@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 20 +++++++++++ > > arch/arm64/kvm/arm.c | 14 +++++--- > > arch/arm64/kvm/hyp/nvhe/psci-relay.c | 53 +++++++++++++++++++++------- > > 3 files changed, 70 insertions(+), 17 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h > > b/arch/arm64/include/asm/kvm_host.h > > index 11beda85ee7e..828d50d40dc2 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -17,6 +17,7 @@ > > #include <linux/jump_label.h> > > #include <linux/kvm_types.h> > > #include <linux/percpu.h> > > +#include <linux/psci.h> > > #include <asm/arch_gicv3.h> > > #include <asm/barrier.h> > > #include <asm/cpufeature.h> > > @@ -240,6 +241,25 @@ struct kvm_host_data { > > struct kvm_pmu_events pmu_events; > > }; > > > > +#define KVM_HOST_PSCI_0_1_CPU_SUSPEND BIT(0) > > +#define KVM_HOST_PSCI_0_1_CPU_ON BIT(1) > > +#define KVM_HOST_PSCI_0_1_CPU_OFF BIT(2) > > +#define KVM_HOST_PSCI_0_1_MIGRATE BIT(3) > > + > > +struct kvm_host_psci_config { > > + /* PSCI version used by host. */ > > + u32 version; > > + > > + /* Function IDs used by host if version is v0.1. */ > > + struct psci_0_1_function_ids function_ids_0_1; > > + > > + /* Bitmask of functions enabled for v0.1, bits KVM_HOST_PSCI_0_1_*. */ > > + unsigned int enabled_functions_0_1; > > Nit: the conventional type for bitmaps is 'unsigned long'. > Also, "enabled" seems odd. Isn't it actually "available"?
Sure, that or "implemented" works here. Since there are only 4 functions here, it might make sense to use independent bools rather than a bitmap, which might make this a bit simpler... > > get_psci_0_1_function_ids(); > > + kvm_host_psci_config.version = psci_ops.get_version(); > > + > > + if (kvm_host_psci_config.version == PSCI_VERSION(0, 1)) { > > + kvm_host_psci_config.function_ids_0_1 = > > get_psci_0_1_function_ids(); > > + kvm_host_psci_config.enabled_functions_0_1 = > > + (psci_ops.cpu_suspend ? KVM_HOST_PSCI_0_1_CPU_SUSPEND : > > 0) | > > + (psci_ops.cpu_off ? KVM_HOST_PSCI_0_1_CPU_OFF : 0) | > > + (psci_ops.cpu_on ? KVM_HOST_PSCI_0_1_CPU_ON : 0) | > > + (psci_ops.migrate ? KVM_HOST_PSCI_0_1_MIGRATE : 0); ... since e.g. this could be roughly: kvm_host_psci_config.cpu_suspend_implemented = psci_ops.cpu_suspend; kvm_host_psci_config.cpu_off_implemented = psci_ops.cpu_off; kvm_host_psci_config.cpu_on_implemented = psci_ops.cpu_on; kvm_host_psci_config.migrate_implemented = psci_ops.migrate; > > +static inline bool is_psci_0_1_cpu_suspend(u64 func_id) > > +{ > > + return is_psci_0_1_function_enabled(KVM_HOST_PSCI_0_1_CPU_SUSPEND) && > > + (func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend); > > +} ...and similarly: return kvm_host_psci_config.cpu_suspend_implemented && func_id == kvm_host_psci_config.function_ids_0_1.cpu_suspend) > Otherwise looks OK. Don't bother respinning the series for my > comments, I can tidy things up as I apply it if there are no other > issues. FWIW, I'm happy with whatever choose to do here, so don't feel like you have to follow my suggestions above. Thanks, Mark. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm