| Message ID | 20220311044811.1980336-2-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Make CPU ID registers writable by userspace | expand |
Hi Reiji, On Thu, Mar 10, 2022 at 08:47:47PM -0800, Reiji Watanabe wrote: > Introduce arm64_check_features(), which does a basic validity checking > of an ID register value against the register's limit value, which is > generally the host's sanitized value. > > This function will be used by the following patches to check if an ID > register value that userspace tries to set for a guest can be supported > on the host. > > The validation is done using arm64_ftr_bits_kvm, which is created from > arm64_ftr_regs, with some entries overwritten by entries from > arm64_ftr_bits_kvm_override. > > Signed-off-by: Reiji Watanabe <reijiw@google.com> I have some concerns regarding the API between cpufeature and KVM that's being proposed here. It would appear that we are adding some of KVM's implementation details into the cpufeature code. In particular, I see that KVM's limitations on AA64DFR0 are being copied here. Additionally, I think it would be preferable to expose this in a manner that does not require CONFIG_KVM guards in other parts of the kernel. WDYT about having KVM keep its set of feature overrides and otherwise rely on the kernel's feature tables? I messed around with the idea a bit until I could get something workable (attached). I only compile tested this :) -- Thanks, Oliver
Hi Oliver, > On Thu, Mar 10, 2022 at 08:47:47PM -0800, Reiji Watanabe wrote: > > Introduce arm64_check_features(), which does a basic validity checking > > of an ID register value against the register's limit value, which is > > generally the host's sanitized value. > > > > This function will be used by the following patches to check if an ID > > register value that userspace tries to set for a guest can be supported > > on the host. > > > > The validation is done using arm64_ftr_bits_kvm, which is created from > > arm64_ftr_regs, with some entries overwritten by entries from > > arm64_ftr_bits_kvm_override. > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > I have some concerns regarding the API between cpufeature and KVM that's > being proposed here. It would appear that we are adding some of KVM's > implementation details into the cpufeature code. In particular, I see > that KVM's limitations on AA64DFR0 are being copied here. I assume "KVM's limitation details" you meant is about ftr_id_aa64dfr0_kvm. Entries in arm64_ftr_bits_kvm_override shouldn't be added based on KVM's implementation. When cpufeature.c doesn't handle lower level of (or fewer) features as the "safe" value for fields, the field should be added to arm64_ftr_bits_kvm_override. As PMUVER and DEBUGVER are not treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override. Having said that, although ftr_id_aa64dfr0 has PMUVER as signed field, I didn't fix that in ftr_id_aa64dfr0_kvm, and the code abused that.... I should make PMUVER unsigned field, and fix cpufeature.c to validate the field based on its special ID scheme for cleaner abstraction. (And KVM should skip the cpufeature.c's PMUVER validation using id_reg_desc's ignore_mask and have KVM validate it locally based on the KVM's special requirement) > Additionally, I think it would be preferable to expose this in a manner > that does not require CONFIG_KVM guards in other parts of the kernel. > > WDYT about having KVM keep its set of feature overrides and otherwise > rely on the kernel's feature tables? I messed around with the idea a > bit until I could get something workable (attached). I only compile > tested this :) Thanks for the proposal! But, providing the overrides to arm64_ftr_reg_valid() means that its consumer knows implementation details of cpufeture.c (values of entries in arm64_ftr_regs[]), which is a similar abstraction problem I want to avoid (the default validation by cpufeature.c should be purely based on ID schemes even with this option). Another option that I considered earlier was having a full set of arm64_ftr_bits in KVM for its validation. At the time, I thought statically) having a full set of arm64_ftr_bits in KVM is not good in terms of maintenance. But, considering that again, since most of fields are unsigned and lower safe fields, and KVM doesn't necessarily have to statically have a full set of arm64_ftr_bits (can dynamically generate during KVM's initialization), it may not be that bad. So, I am thinking of exploring this option. More specifically, changes in cpufeature.c from patch-1 will be below and remove all other newly added codes from cpufeature.c. (Need more changes in KVM) --- --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -3357,9 +3357,9 @@ static const struct arm64_ftr_bits *get_arm64_ftr_bits_kvm(u32 sys_id) * scheme, the function checks if values of the fields in @val are the same * as the ones in @limit. */ -int arm64_check_features_kvm(u32 sys_reg, u64 val, u64 limit) +int arm64_check_features(u32 sys_reg, const struct arm64_ftr_bits *ftrp, + u64 val, u64 limit) { - const struct arm64_ftr_bits *ftrp = get_arm64_ftr_bits_kvm(sys_reg); u64 exposed_mask = 0; if (!ftrp) --- Thanks, Reiji
On Tue, Mar 22, 2022 at 11:06:26PM -0700, Reiji Watanabe wrote: > Hi Oliver, > > > On Thu, Mar 10, 2022 at 08:47:47PM -0800, Reiji Watanabe wrote: > > > Introduce arm64_check_features(), which does a basic validity checking > > > of an ID register value against the register's limit value, which is > > > generally the host's sanitized value. > > > > > > This function will be used by the following patches to check if an ID > > > register value that userspace tries to set for a guest can be supported > > > on the host. > > > > > > The validation is done using arm64_ftr_bits_kvm, which is created from > > > arm64_ftr_regs, with some entries overwritten by entries from > > > arm64_ftr_bits_kvm_override. > > > > > > Signed-off-by: Reiji Watanabe <reijiw@google.com> > > > > I have some concerns regarding the API between cpufeature and KVM that's > > being proposed here. It would appear that we are adding some of KVM's > > implementation details into the cpufeature code. In particular, I see > > that KVM's limitations on AA64DFR0 are being copied here. > > I assume "KVM's limitation details" you meant is about > ftr_id_aa64dfr0_kvm. > Entries in arm64_ftr_bits_kvm_override shouldn't be added based > on KVM's implementation. When cpufeature.c doesn't handle lower level > of (or fewer) features as the "safe" value for fields, the field should > be added to arm64_ftr_bits_kvm_override. As PMUVER and DEBUGVER are not > treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override. I believe the fact that KVM is more permissive on PMUVER and DEBUGVER than cpufeature is in fact a detail of KVM, no? read_id_reg() already implicitly trusts the cpufeature code filtering and applies additional limitations on top of what we get back. Similarly, there are fields where KVM is more restrictive than cpufeature (ID_AA64DFR0_PMSVER). Each of those constraints could theoretically be expressed as an arm64_ftr_bits structure within KVM. > Having said that, although ftr_id_aa64dfr0 has PMUVER as signed field, > I didn't fix that in ftr_id_aa64dfr0_kvm, and the code abused that.... > I should make PMUVER unsigned field, and fix cpufeature.c to validate > the field based on its special ID scheme for cleaner abstraction. Good point. AA64DFR0 is an annoying register :) > (And KVM should skip the cpufeature.c's PMUVER validation using > id_reg_desc's ignore_mask and have KVM validate it locally based on > the KVM's special requirement) > > > > Additionally, I think it would be preferable to expose this in a manner > > that does not require CONFIG_KVM guards in other parts of the kernel. > > > > WDYT about having KVM keep its set of feature overrides and otherwise > > rely on the kernel's feature tables? I messed around with the idea a > > bit until I could get something workable (attached). I only compile > > tested this :) > > Thanks for the proposal! > But, providing the overrides to arm64_ftr_reg_valid() means that its > consumer knows implementation details of cpufeture.c (values of entries > in arm64_ftr_regs[]), which is a similar abstraction problem I want to > avoid (the default validation by cpufeature.c should be purely based on > ID schemes even with this option). It is certainly a matter of where you choose to draw those lines. We already do bank on the implementation details of cpufeature.c quite heavily, its just stuffed away behind read_sanitised_ftr_reg(). Now we have KVM bits and pieces in cpufeature.c and might wind up forcing others to clean up our dirty laundry in the future. It also seems to me that if I wanted to raise the permitted DEBUGVER for KVM, would I have to make a change outside of KVM. > Another option that I considered earlier was having a full set of > arm64_ftr_bits in KVM for its validation. At the time, I thought > statically) having a full set of arm64_ftr_bits in KVM is not good in > terms of maintenance. But, considering that again, since most of > fields are unsigned and lower safe fields, and KVM doesn't necessarily > have to statically have a full set of arm64_ftr_bits I think the argument could be made for KVM having its own static + verbose cpufeature tables. We've already been bitten by scenarios where cpufeature exposes a feature that we simply do not virtualize in KVM. That really can become a game of whack-a-mole. commit 96f4f6809bee ("KVM: arm64: Don't advertise FEAT_SPE to guests") is a good example, and I can really see no end to these sorts of issues without an overhaul. We'd need to also find a way to leverage the existing infrasturcture for working out a system-wide safe value, but this time with KVM's table of registers. KVM would then need to take a change to expose any new feature that has no involvement of EL2. Personally, I'd take that over the possibility of another unhandled feature slipping through and blowing up a guest kernel when running on newer hardware. > (dynamically generate during KVM's initialization) This was another one of my concerns with the current state of this patch. I found the register table construction at runtime hard to follow. I think it could be avoided with a helper that has a prescribed set of rules (caller-provided field definition takes precedence over the general one). Sorry it took me a bit to comment on everything, needed to really sit down and wrap my head around the series. -- Thanks, Oliver
Hi Oliver, > > > I have some concerns regarding the API between cpufeature and KVM that's > > > being proposed here. It would appear that we are adding some of KVM's > > > implementation details into the cpufeature code. In particular, I see > > > that KVM's limitations on AA64DFR0 are being copied here. > > > > I assume "KVM's limitation details" you meant is about > > ftr_id_aa64dfr0_kvm. > > Entries in arm64_ftr_bits_kvm_override shouldn't be added based > > on KVM's implementation. When cpufeature.c doesn't handle lower level > > of (or fewer) features as the "safe" value for fields, the field should > > be added to arm64_ftr_bits_kvm_override. As PMUVER and DEBUGVER are not > > treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override. > > I believe the fact that KVM is more permissive on PMUVER and DEBUGVER > than cpufeature is in fact a detail of KVM, no? read_id_reg() already What cpufeature knows is that consumers of the validation function needs the validation of each field based on ID register schemes that are described in Arm ARM (basically lower safe). As lower values of PMUVER/DEBUGVER indicates lower level of features or fewer level of features, those entries are to provide validation based on that. So, entries in arm64_ftr_bits_kvm_override will be added to adjust cpufeture's behavior based on ID register schemes, and KVM may or may not use them. I need to remove the word "kvm" from variable/function/structure names and put more clear comments:) > implicitly trusts the cpufeature code filtering and applies additional > limitations on top of what we get back. Similarly, there are fields > where KVM is more restrictive than cpufeature (ID_AA64DFR0_PMSVER). > > Each of those constraints could theoretically be expressed as an > arm64_ftr_bits structure within KVM. It's not impossible but it's a bit tricky (With __arm64_ftr_reg_valid(), it might look straight forward, but I don't think that treats FTR_EXACT correctly. Please see update_cpu_ftr_reg). > > Having said that, although ftr_id_aa64dfr0 has PMUVER as signed field, > > I didn't fix that in ftr_id_aa64dfr0_kvm, and the code abused that.... > > I should make PMUVER unsigned field, and fix cpufeature.c to validate > > the field based on its special ID scheme for cleaner abstraction. > > Good point. AA64DFR0 is an annoying register :) > > > (And KVM should skip the cpufeature.c's PMUVER validation using > > id_reg_desc's ignore_mask and have KVM validate it locally based on > > the KVM's special requirement) > > > > > > > Additionally, I think it would be preferable to expose this in a manner > > > that does not require CONFIG_KVM guards in other parts of the kernel. > > > > > > WDYT about having KVM keep its set of feature overrides and otherwise > > > rely on the kernel's feature tables? I messed around with the idea a > > > bit until I could get something workable (attached). I only compile > > > tested this :) > > > > Thanks for the proposal! > > But, providing the overrides to arm64_ftr_reg_valid() means that its > > consumer knows implementation details of cpufeture.c (values of entries > > in arm64_ftr_regs[]), which is a similar abstraction problem I want to > > avoid (the default validation by cpufeature.c should be purely based on > > ID schemes even with this option). > > It is certainly a matter of where you choose to draw those lines. We already > do bank on the implementation details of cpufeature.c quite heavily, its > just stuffed away behind read_sanitised_ftr_reg(). Now we have KVM bits and > pieces in cpufeature.c and might wind up forcing others to clean up our dirty > laundry in the future. As I mentioned above, they aren't KVM specific. > It also seems to me that if I wanted to raise the permitted DEBUGVER for KVM, > would I have to make a change outside of KVM. Could you elaborate this a little more? More specific concern I have about providing the override (with the existing arm64_ftr_bits) would be when field values of arm64_ftr_bits (i.e. LOWER_SAFE to EXACT) in cpufeature are changed due to kernel's implementation reasons, which might affect KVM (may need to pass extra override to arm64_ftr_reg_valid). But, by having cpufeature provide the validation based on the ID register schemes, cpufeature should be changed to provide the same validation in that case (i.e. if DFR0.PERFMON is changed from LOWER_SAFE to EXACT like AA64DFR0.PMUVER, DFR0.PERFMON should be added in arm64_ftr_bits_kvm_override with LOWER_SAFE). So, if I go with the option to provide override to cpufeature, IMHO it would be preferable for cpufeature to provide the validation based on ID schemes instead of with the current need-based policy (, which might get changed) for clear separation. > > Another option that I considered earlier was having a full set of > > arm64_ftr_bits in KVM for its validation. At the time, I thought > > statically) having a full set of arm64_ftr_bits in KVM is not good in > > terms of maintenance. But, considering that again, since most of > > fields are unsigned and lower safe fields, and KVM doesn't necessarily > > have to statically have a full set of arm64_ftr_bits > > I think the argument could be made for KVM having its own static + > verbose cpufeature tables. We've already been bitten by scenarios where What does "verbose cpufeature tables" mean ? > cpufeature exposes a feature that we simply do not virtualize in KVM. > That really can become a game of whack-a-mole. commit 96f4f6809bee > ("KVM: arm64: Don't advertise FEAT_SPE to guests") is a good example, > and I can really see no end to these sorts of issues without an > overhaul. We'd need to also find a way to leverage the existing > infrasturcture for working out a system-wide safe value, but this time > with KVM's table of registers. > KVM would then need to take a change to expose any new feature that has > no involvement of EL2. Personally, I'd take that over the possibility of > another unhandled feature slipping through and blowing up a guest kernel > when running on newer hardware. Userspace with configurable ID registers would eliminate such problems on known systems, but I agree that KVM itself should prevent it. It will be inconvenient for some people, but it would be safer in general. > > (dynamically generate during KVM's initialization) > > This was another one of my concerns with the current state of this > patch. I found the register table construction at runtime hard to > follow. I think it could be avoided with a helper that has a prescribed > set of rules (caller-provided field definition takes precedence over the > general one). Sure, I will improve that if I continue to keep the current way. With the option of having a separate KVM's arm64_ftr_bits, the code will be very different, but I will keep that in mind. Thanks, Reiji
Hi Reiji, On Wed, Mar 23, 2022 at 11:00:25PM -0700, Reiji Watanabe wrote: > Hi Oliver, > > > > > I have some concerns regarding the API between cpufeature and KVM that's > > > > being proposed here. It would appear that we are adding some of KVM's > > > > implementation details into the cpufeature code. In particular, I see > > > > that KVM's limitations on AA64DFR0 are being copied here. > > > > > > I assume "KVM's limitation details" you meant is about > > > ftr_id_aa64dfr0_kvm. > > > Entries in arm64_ftr_bits_kvm_override shouldn't be added based > > > on KVM's implementation. When cpufeature.c doesn't handle lower level > > > of (or fewer) features as the "safe" value for fields, the field should > > > be added to arm64_ftr_bits_kvm_override. As PMUVER and DEBUGVER are not > > > treated as LOWER_SAFE, they were added in arm64_ftr_bits_kvm_override. > > > > I believe the fact that KVM is more permissive on PMUVER and DEBUGVER > > than cpufeature is in fact a detail of KVM, no? read_id_reg() already > > What cpufeature knows is that consumers of the validation function > needs the validation of each field based on ID register schemes that > are described in Arm ARM (basically lower safe). > As lower values of PMUVER/DEBUGVER indicates lower level of features > or fewer level of features, those entries are to provide validation > based on that. So, entries in arm64_ftr_bits_kvm_override will be added > to adjust cpufeture's behavior based on ID register schemes, and KVM may > or may not use them. > > I need to remove the word "kvm" from variable/function/structure names > and put more clear comments:) I'll admit I definitely drilled down on the fact that KVM is the only actual user of these, and not the fact that it was realigning the fields with the Arm ARM :) > > implicitly trusts the cpufeature code filtering and applies additional > > limitations on top of what we get back. Similarly, there are fields > > where KVM is more restrictive than cpufeature (ID_AA64DFR0_PMSVER). > > > > Each of those constraints could theoretically be expressed as an > > arm64_ftr_bits structure within KVM. > > It's not impossible but it's a bit tricky (With __arm64_ftr_reg_valid(), > it might look straight forward, but I don't think that treats FTR_EXACT > correctly. Please see update_cpu_ftr_reg). > Ah right. __arm64_ftr_reg_valid() needs to trust either the value that comes from the boot CPU, or ->safe_val if the cores are different in the system. And what does it mean if the caller specified FTR_EXACT? I'll think on this more if I have any other suggestions. [...] > > It also seems to me that if I wanted to raise the permitted DEBUGVER for KVM, > > would I have to make a change outside of KVM. > > Could you elaborate this a little more? Urgh. Ignore me, I fixated to heavily on the SAFE_VAL you used for DEBUGVER, not the fact that it was LOWER_SAFE. > More specific concern I have about providing the override (with the > existing arm64_ftr_bits) would be when field values of arm64_ftr_bits > (i.e. LOWER_SAFE to EXACT) in cpufeature are changed due to kernel's > implementation reasons, which might affect KVM (may need to pass > extra override to arm64_ftr_reg_valid). > But, by having cpufeature provide the validation based on the ID > register schemes, cpufeature should be changed to provide the same > validation in that case (i.e. if DFR0.PERFMON is changed from LOWER_SAFE > to EXACT like AA64DFR0.PMUVER, DFR0.PERFMON should be added in > arm64_ftr_bits_kvm_override with LOWER_SAFE). > > So, if I go with the option to provide override to cpufeature, IMHO it > would be preferable for cpufeature to provide the validation based > on ID schemes instead of with the current need-based policy (, which > might get changed) for clear separation. Sounds good. Per your suggestion above, changing the naming/documentation around what is being added to cpufeature removes the confusion that it relates to KVM and really is a precise implementation of the rules in the Arm ARM. > > > Another option that I considered earlier was having a full set of > > > arm64_ftr_bits in KVM for its validation. At the time, I thought > > > statically) having a full set of arm64_ftr_bits in KVM is not good in > > > terms of maintenance. But, considering that again, since most of > > > fields are unsigned and lower safe fields, and KVM doesn't necessarily > > > have to statically have a full set of arm64_ftr_bits > > > > I think the argument could be made for KVM having its own static + > > verbose cpufeature tables. We've already been bitten by scenarios where > > What does "verbose cpufeature tables" mean ? Currently KVM implements a sparsely-defined denylist on top of whatever we get back from read_sanitised_ftr_reg(). We do not have an absolute upper bound for all fields in the feature registers, so there are times where unsupported features leak through to the guest much like the SPE commit I mentioned below. What I am suggesting is that KVM define an absolute limit on what it virtualizes for *all* fields, including what is presently RAZ. We have absolutely no idea whether or not we can virtualize new features that come in later revisions of the spec. It does mean we will need to raise those limits from time to time, but would rather do that than accidentally expose a feature we cannot virtualize. > > cpufeature exposes a feature that we simply do not virtualize in KVM. > > That really can become a game of whack-a-mole. commit 96f4f6809bee > > ("KVM: arm64: Don't advertise FEAT_SPE to guests") is a good example, > > and I can really see no end to these sorts of issues without an > > overhaul. We'd need to also find a way to leverage the existing > > infrasturcture for working out a system-wide safe value, but this time > > with KVM's table of registers. > > KVM would then need to take a change to expose any new feature that has > > no involvement of EL2. Personally, I'd take that over the possibility of > > another unhandled feature slipping through and blowing up a guest kernel > > when running on newer hardware. > > Userspace with configurable ID registers would eliminate such problems > on known systems, but I agree that KVM itself should prevent it. > It will be inconvenient for some people, but it would be safer in general. We cannot require userspace to write to these registers to run a guest given the fact that the present ABI doesn't. Given that fact, KVM is still responsible for having sane default values for these registers. If a field that we do not handle implies a feature we do not virtualize on newer hardware, invariably our guest will see it and likely panic when it realizes the vCPU is out of spec. Maybe the feature bits tables is a bit extreme given the fact that it does define the architected handling of each field. I think the upper bound on register values I mentioned above would do the trick and avoid copy/pasting a whole set of structures we don't desperately need. > > > (dynamically generate during KVM's initialization) > > > > This was another one of my concerns with the current state of this > > patch. I found the register table construction at runtime hard to > > follow. I think it could be avoided with a helper that has a prescribed > > set of rules (caller-provided field definition takes precedence over the > > general one). > > Sure, I will improve that if I continue to keep the current way. > With the option of having a separate KVM's arm64_ftr_bits, > the code will be very different, but I will keep that in mind. arm64_ftr_bits might be a bit extreme in KVM after all, I'll retract that suggestion in favor of what I said above :) Thanks for being patient working through all of this with me. -- Best, Oliver
Hi Oliver, > > > implicitly trusts the cpufeature code filtering and applies additional > > > limitations on top of what we get back. Similarly, there are fields > > > where KVM is more restrictive than cpufeature (ID_AA64DFR0_PMSVER). > > > > > > Each of those constraints could theoretically be expressed as an > > > arm64_ftr_bits structure within KVM. > > > > It's not impossible but it's a bit tricky (With __arm64_ftr_reg_valid(), > > it might look straight forward, but I don't think that treats FTR_EXACT > > correctly. Please see update_cpu_ftr_reg). > > > > Ah right. __arm64_ftr_reg_valid() needs to trust either the value that > comes from the boot CPU, or ->safe_val if the cores are different in the > system. And what does it mean if the caller specified FTR_EXACT? What I was trying to say is __arm64_ftr_reg_valid() should check if the 'bits' are the same as 'safe_bits' and should treat 'bits' as safe if they are the same instead of calling arm64_ftr_field_valid() as update_cpu_ftr_reg does. In my understanding, FTR_EXACT doesn't mean only 'ftrp->safe_val' is a safe value, but if two values are different, 'ftrp->safe_val' is the only safe value. > > More specific concern I have about providing the override (with the > > existing arm64_ftr_bits) would be when field values of arm64_ftr_bits > > (i.e. LOWER_SAFE to EXACT) in cpufeature are changed due to kernel's > > implementation reasons, which might affect KVM (may need to pass > > extra override to arm64_ftr_reg_valid). > > But, by having cpufeature provide the validation based on the ID > > register schemes, cpufeature should be changed to provide the same > > validation in that case (i.e. if DFR0.PERFMON is changed from LOWER_SAFE > > to EXACT like AA64DFR0.PMUVER, DFR0.PERFMON should be added in > > arm64_ftr_bits_kvm_override with LOWER_SAFE). > > > > So, if I go with the option to provide override to cpufeature, IMHO it > > would be preferable for cpufeature to provide the validation based > > on ID schemes instead of with the current need-based policy (, which > > might get changed) for clear separation. > > Sounds good. Per your suggestion above, changing the > naming/documentation around what is being added to cpufeature removes > the confusion that it relates to KVM and really is a precise > implementation of the rules in the Arm ARM. > > > > cpufeature exposes a feature that we simply do not virtualize in KVM. > > > That really can become a game of whack-a-mole. commit 96f4f6809bee > > > ("KVM: arm64: Don't advertise FEAT_SPE to guests") is a good example, > > > and I can really see no end to these sorts of issues without an > > > overhaul. We'd need to also find a way to leverage the existing > > > infrasturcture for working out a system-wide safe value, but this time > > > with KVM's table of registers. > > > KVM would then need to take a change to expose any new feature that has > > > no involvement of EL2. Personally, I'd take that over the possibility of > > > another unhandled feature slipping through and blowing up a guest kernel > > > when running on newer hardware. > > > > Userspace with configurable ID registers would eliminate such problems > > on known systems, but I agree that KVM itself should prevent it. > > It will be inconvenient for some people, but it would be safer in general. > > We cannot require userspace to write to these registers to run a guest > given the fact that the present ABI doesn't. Given that fact, KVM is > still responsible for having sane default values for these registers. > > If a field that we do not handle implies a feature we do not virtualize > on newer hardware, invariably our guest will see it and likely panic > when it realizes the vCPU is out of spec. I completely agree on this, and I will work on this separately from this series as it is a different issue from what this series tries to address. > Maybe the feature bits tables is a bit extreme given the fact that it > does define the architected handling of each field. I think the upper > bound on register values I mentioned above would do the trick and avoid > copy/pasting a whole set of structures we don't desperately need. Yes, I have been thinking the same (having a max register value for each register rather than having a max value for each field). > > > > (dynamically generate during KVM's initialization) > > > > > > This was another one of my concerns with the current state of this > > > patch. I found the register table construction at runtime hard to > > > follow. I think it could be avoided with a helper that has a prescribed > > > set of rules (caller-provided field definition takes precedence over the > > > general one). > > > > Sure, I will improve that if I continue to keep the current way. > > With the option of having a separate KVM's arm64_ftr_bits, > > the code will be very different, but I will keep that in mind. > > arm64_ftr_bits might be a bit extreme in KVM after all, I'll retract > that suggestion in favor of what I said above :) Having a full set of arm64_ftr_bits in KVM I meant here is the one that I mentioned earlier, and I am inclined to go with the option rather than having cpufeature provide ID scheme based validation and the override validation. With that, KVM will still use a validation function that cpufeture will provide, but as it is done only based on arm64_ftr_bits provided by KVM (FTR_UNSIGNED + LOWER_SAFE entries will be generated during KVM init, and the other entries will be statically defined in KVM). Compared to this separate arm64_ftr_bits option, there are two things that I don't like a bit about the ID scheme based validation option. One is special changes we will make in cpufeature to provide ID scheme based validation (like for ID_AA64DFR0_EL0.PMUVER) as needed won't be used by KVM when KVM needs another implementation (our efforts might be wasted). The other is (I think) it's pretty easy to forget to make changes in cpufeature to provide ID scheme based validation for new fields when needed. Along with future changes to set the maximum limit for each ID register field, I would think this separate arm64_ftr_bits option could provide a safer way for KVM to control/maintain feature visibilities for the guest more independently with smaller maintenance cost. The downside of having the separate arm64_ftr_bits will be that there are extra static arm64_ftr_bits entries that need to be defined for FTR_SIGNED + LOWER_SAFE. It's a so big deal though because there are just 6(?) of such entries, and I don't think we will newly have many of such entries. Thanks, Reiji
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index ef6be92b1921..a9edf1ca7dcb 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -631,6 +631,7 @@ void check_local_cpu_capabilities(void); u64 read_sanitised_ftr_reg(u32 id); u64 __read_sysreg_by_encoding(u32 sys_id); +int arm64_check_features_kvm(u32 sys_reg, u64 val, u64 limit); static inline bool cpu_supports_mixed_endian_el0(void) { diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index e5f23dab1c8d..bc0ed09aa1b5 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -928,6 +928,10 @@ static void init_32bit_cpu_features(struct cpuinfo_32bit *info) init_cpu_ftr_reg(SYS_MVFR2_EL1, info->reg_mvfr2); } +#ifdef CONFIG_KVM +static void init_arm64_ftr_bits_kvm(void); +#endif + void __init init_cpu_features(struct cpuinfo_arm64 *info) { /* Before we start using the tables, make sure it is sorted */ @@ -970,6 +974,14 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info) * after we have initialised the CPU feature infrastructure. */ setup_boot_cpu_capabilities(); + +#ifdef CONFIG_KVM + /* + * Initialize arm64_ftr_bits_kvm, which will be used to provide + * KVM with general feature checking for its guests. + */ + init_arm64_ftr_bits_kvm(); +#endif } static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new) @@ -3156,3 +3168,220 @@ ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, return sprintf(buf, "Vulnerable\n"); } } + +#ifdef CONFIG_KVM +/* + * arm64_ftr_bits_kvm[] is used for KVM to check if features that are + * indicated in an ID register value for the guest are available on the host. + * arm64_ftr_bits_kvm[] is created based on arm64_ftr_regs[]. But, for + * registers for which arm64_ftr_bits_kvm_override[] has a corresponding + * entry, replace arm64_ftr_bits entries in arm64_ftr_bits_kvm[] with the + * ones in arm64_ftr_bits_kvm_override[]. + * + * What to add to arm64_ftr_bits_kvm_override[] shouldn't be decided according + * to KVM's implementation, but according to schemes of ID register fields. + * (e.g. For ID_AA64DFR0_EL1.PMUVER, a higher value on the field indicates + * more features. So, the arm64_ftr_bits' type for the field can be + * FTR_LOWER_SAFE instead of FTR_EXACT unlike arm64_ftr_regs) + */ + +/* + * The number of __ftr_reg_bits_entry entries in arm64_ftr_bits_kvm must be + * the same as the number of __ftr_reg_entry entries in arm64_ftr_regs. + */ +static struct __ftr_reg_bits_entry { + u32 sys_id; + struct arm64_ftr_bits *ftr_bits; +} arm64_ftr_bits_kvm[ARRAY_SIZE(arm64_ftr_regs)]; + +/* + * Number of arm64_ftr_bits entries for each register. + * (Number of 4 bits fields in 64 bit register + 1 entry for ARM64_FTR_END) + */ +#define MAX_FTR_BITS_LEN 17 + +/* Use FTR_LOWER_SAFE for AA64DFR0_EL1.PMUVER and AA64DFR0_EL1.DEBUGVER. */ +static struct arm64_ftr_bits ftr_id_aa64dfr0_kvm[MAX_FTR_BITS_LEN] = { + S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_PMUVER_SHIFT, 4, 0), + ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_DEBUGVER_SHIFT, 4, 0x6), + ARM64_FTR_END, +}; + +#define ARM64_FTR_REG_BITS(id, table) { \ + .sys_id = id, \ + .ftr_bits = &((table)[0]), \ +} + +/* + * All entries in arm64_ftr_bits_kvm_override[] are used to override + * the corresponding entries in arm64_ftr_bits_kvm[]. + */ +static struct __ftr_reg_bits_entry arm64_ftr_bits_kvm_override[] = { + ARM64_FTR_REG_BITS(SYS_ID_AA64DFR0_EL1, ftr_id_aa64dfr0_kvm), +}; + +/* + * Override entries in @orig_ftrp with the ones in @new_ftrp when their shift + * fields match. The last entry of @orig_ftrp and @new_ftrp must be + * ARM64_FTR_END (.width == 0). + */ +static void arm64_ftr_reg_bits_override(struct arm64_ftr_bits *orig_ftrp, + const struct arm64_ftr_bits *new_ftrp) +{ + const struct arm64_ftr_bits *n_ftrp; + struct arm64_ftr_bits *o_ftrp; + + for (n_ftrp = new_ftrp; n_ftrp->width; n_ftrp++) { + for (o_ftrp = orig_ftrp; o_ftrp->width; o_ftrp++) { + if (o_ftrp->shift == n_ftrp->shift) { + *o_ftrp = *n_ftrp; + break; + } + } + } +} + +/* + * Copy arm64_ftr_bits entries from @src_ftrp to @dst_ftrp. The last entries + * of @dst_ftrp and @src_ftrp must be ARM64_FTR_END (.width == 0). + */ +static void copy_arm64_ftr_bits(struct arm64_ftr_bits *dst_ftrp, + const struct arm64_ftr_bits *src_ftrp) +{ + int i = 0; + + for (; src_ftrp[i].width; i++) { + if (WARN_ON_ONCE(i >= (MAX_FTR_BITS_LEN - 1))) + break; + + dst_ftrp[i] = src_ftrp[i]; + } + + dst_ftrp[i].width = 0; +} + +/* + * Initialize arm64_ftr_bits_kvm. Copy arm64_ftr_bits for each ID register + * from arm64_ftr_regs to arm64_ftr_bits_kvm, and then override entries in + * arm64_ftr_bits_kvm with ones in arm64_ftr_bits_kvm_override. + */ +static void init_arm64_ftr_bits_kvm(void) +{ + struct arm64_ftr_bits ftr_temp[MAX_FTR_BITS_LEN]; + static struct __ftr_reg_bits_entry *bits, *o_bits; + int i, j; + + /* Copy entries from arm64_ftr_regs to arm64_ftr_bits_kvm */ + for (i = 0; i < ARRAY_SIZE(arm64_ftr_bits_kvm); i++) { + bits = &arm64_ftr_bits_kvm[i]; + bits->sys_id = arm64_ftr_regs[i].sys_id; + bits->ftr_bits = (struct arm64_ftr_bits *)arm64_ftr_regs[i].reg->ftr_bits; + }; + + /* + * Override the entries in arm64_ftr_bits_kvm with the ones in + * arm64_ftr_bits_kvm_override. + */ + for (i = 0; i < ARRAY_SIZE(arm64_ftr_bits_kvm_override); i++) { + o_bits = &arm64_ftr_bits_kvm_override[i]; + for (j = 0; j < ARRAY_SIZE(arm64_ftr_bits_kvm); j++) { + bits = &arm64_ftr_bits_kvm[j]; + if (bits->sys_id != o_bits->sys_id) + continue; + + /* + * The code below tries to sustain the ordering of + * entries in bits even in o_bits, just in case + * arm64_ftr_bits entries in arm64_ftr_regs have + * any ordering requirements in the future (so that + * the ones in arm64_ftr_bits_kvm_override doesn't + * have to care). + * So, rather than directly copying them to empty + * slots in o_bits, the code simply copies entries + * from bits to o_bits first, then overrides them with + * original entries in o_bits. + */ + memset(ftr_temp, 0, sizeof(ftr_temp)); + + /* + * Temporary save all entries in o_bits->ftr_bits + * to ftr_temp. + */ + copy_arm64_ftr_bits(ftr_temp, o_bits->ftr_bits); + + /* + * Copy entries from bits->ftr_bits to o_bits->ftr_bits. + */ + copy_arm64_ftr_bits(o_bits->ftr_bits, bits->ftr_bits); + + /* + * Override entries in o_bits->ftr_bits with the + * saved ones, and update bits->ftr_bits with + * o_bits->ftr_bits. + */ + arm64_ftr_reg_bits_override(o_bits->ftr_bits, ftr_temp); + bits->ftr_bits = o_bits->ftr_bits; + break; + } + } +} + +static int search_cmp_ftr_reg_bits(const void *id, const void *regp) +{ + return ((int)(unsigned long)id - + (int)((const struct __ftr_reg_bits_entry *)regp)->sys_id); +} + +static const struct arm64_ftr_bits *get_arm64_ftr_bits_kvm(u32 sys_id) +{ + const struct __ftr_reg_bits_entry *ret; + + ret = bsearch((const void *)(unsigned long)sys_id, + arm64_ftr_bits_kvm, + ARRAY_SIZE(arm64_ftr_bits_kvm), + sizeof(arm64_ftr_bits_kvm[0]), + search_cmp_ftr_reg_bits); + if (ret) + return ret->ftr_bits; + + return NULL; +} + +/* + * Check if features (or levels of features) that are indicated in the ID + * register value @val are also indicated in @limit. + * This function is for KVM to check if features that are indicated in @val, + * which will be used as the ID register value for its guest, are supported + * on the host. + * For AA64MMFR0_EL1.TGranX_2 fields, which don't follow the standard ID + * scheme, the function checks if values of the fields in @val are the same + * as the ones in @limit. + */ +int arm64_check_features_kvm(u32 sys_reg, u64 val, u64 limit) +{ + const struct arm64_ftr_bits *ftrp = get_arm64_ftr_bits_kvm(sys_reg); + u64 exposed_mask = 0; + + if (!ftrp) + return -ENOENT; + + for (; ftrp->width; ftrp++) { + s64 ftr_val = arm64_ftr_value(ftrp, val); + s64 ftr_lim = arm64_ftr_value(ftrp, limit); + + exposed_mask |= arm64_ftr_mask(ftrp); + + if (ftr_val == ftr_lim) + continue; + + if (ftr_val != arm64_ftr_safe_value(ftrp, ftr_val, ftr_lim)) + return -E2BIG; + } + + /* Make sure that no unrecognized fields are set in @val. */ + if (val & ~exposed_mask) + return -E2BIG; + + return 0; +} +#endif /* CONFIG_KVM */
Introduce arm64_check_features(), which does a basic validity checking of an ID register value against the register's limit value, which is generally the host's sanitized value. This function will be used by the following patches to check if an ID register value that userspace tries to set for a guest can be supported on the host. The validation is done using arm64_ftr_bits_kvm, which is created from arm64_ftr_regs, with some entries overwritten by entries from arm64_ftr_bits_kvm_override. Signed-off-by: Reiji Watanabe <reijiw@google.com> --- arch/arm64/include/asm/cpufeature.h | 1 + arch/arm64/kernel/cpufeature.c | 229 ++++++++++++++++++++++++++++ 2 files changed, 230 insertions(+)