> On 8 Oct 2026, at 1:18 PM, Eric Auger <[email protected]> wrote:
>
> !-------------------------------------------------------------------|
> CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Hi Khushit,
>
> On 9/30/26 9:34 AM, Khushit Shah wrote:
>>
>>> On 16 Sep 2026, at 8:15 PM, Eric Auger <[email protected]> wrote:
>>>
>>> !-------------------------------------------------------------------|
>>> CAUTION: External Email
>>>
>>> |-------------------------------------------------------------------!
>>>
>>> Since legacy composite options (such as SVE, virtualization, ...)
>>> and new SYSREG props coexist, add some basic consistency checks.
>>>
>>> Those checks are performed both in kvm_arch_init_vcpu() before
>>> applying the SYSREG props at the end of the initialization chain.
>>>
>>> Some ID reg fields corresponding to legacy composite option scope
>>> are not writable. In that case we just check that the field has
>>> not become writable.
>>>
>>> Signed-off-by: Eric Auger <[email protected]>
>> Hi Eric,
>>
>> Gave this another look. I suggest moving this to
>> kvm_arm_expose_idreg_properties().
>> What I suggest specifically is:
>> - Have a list of ID register fields which conflicts with legacy props.
> this depends on the definition of "conflict". For instance does
>
> PMUVer conflicts with has_pmu?
>
Yes, PMUVer = 0 is inconsistent with has_pmu=true
> Nevertheless your suggested approach could work for props below which in case
> they show up would trigger
> error_setg(errp, "%s is now exposed but qemu is not ready to support
> it",
> propname);
I was thinking of not exposing these props because we know it conflicts. So
instead of checking the set props in kvm_arm_vcpu_validate_sysreg, we never
expose conflicts in kvm_arm_expose_idreg_properties
I am suggesting this way because for named CPU models, we will have to handle
this conflicts differently.
I hope this makes sense.
Thanks,
Khushit
>
> Thanks
>
> Eric
>
>> - in kvm_arm_expose_idreg_properties() (the new field based approach) never
>> expose fields
>> which are part of the above list.
>>
>> This way the same consistency logic you have will carry over to named models
>> by just adding different
>> setters for the above field.
>>
>> What are your thoughts on this?
>>
>> Thanks,
>> Khushit
>>> —
>>> target/arm/kvm.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 105 insertions(+)
>>>
>>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>>> index ddf320d0e6..f52c03745e 100644
>>> --- a/target/arm/kvm.c
>>> +++ b/target/arm/kvm.c
>>> @@ -340,6 +340,107 @@ static ARM64SysRegField *get_field(int i, ARM64SysReg
>>> *reg)
>>> return NULL;
>>> }
>>>
>>> +
>>> +/**
>>> + * kvm_arm_vcpu_validate_sysreg:
>>> + *
>>> + * Validate a SYSREG field value is consistent with legacy composite
>>> options
>>> + * Some checks are not implemented because the corresponding sysreg field
>>> + * is not currently writable. In that case we make sure the field has not
>>> + * become writable, which would mean the user had the capability to set the
>>> + * corresponding property.
>>> + *
>>> + * return true if consistent. false if it is not, along with an error
>>> handle
>>> + */
>>> +static bool kvm_arm_vcpu_validate_sysreg(ARMCPU *cpu, ARM64SysRegField
>>> *field,
>>> + uint64_t value, Error **errp)
>>> +{
>>> + const char *fieldname = field->name;
>>> + ARM64SysReg *sysregdesc = &arm64_id_regs[field->index];
>>> + const char *regname = sysregdesc->name;
>>> + g_autofree char *propname = g_strdup_printf("SYSREG_%s_%s", regname,
>>> fieldname);
>>> +
>>> + /* virtualization */
>>> + if (!strcmp(propname, "SYSREG_ID_AA64PFR0_EL1_EL2")) {
>>> + /* consistency with machine virtualization property */
>>> + if (cpu->has_el2 && value == 0) {
>>> + error_setg(errp,
>>> + "Inconsistent -machine virtualization=on and "
>>> + "%s=0", propname);
>>> + return false;
>>> + } else if (!cpu->has_el2 && value > 0) {
>>> + error_setg(errp,
>>> + "Inconsistent -machine virtualization=off and "
>>> + "%s > 0", propname);
>>> + return false;
>>> + }
>>> + /* secure */
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64PFR0_EL1_EL3") && value >
>>> 0) {
>>> + /* consistency with machine secure property */
>>> + error_setg(errp, "%s is set but qemu is not ready to support it",
>>> + propname);
>>> + return false;
>>> + /* MTE */
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64PFR1_EL1_MTE")) {
>>> + error_setg(errp, "%s is now exposed but qemu is not ready to
>>> support it",
>>> + propname);
>>> + return false;
>>> + /* SVE */
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64PFR0_EL1_SVE")) {
>>> + error_setg(errp, "%s is now exposed but qemu is not ready to
>>> support it",
>>> + propname);
>>> + return false;
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64ZFR0_EL1_SVEver")) {
>>> + if (cpu_isar_feature(aa64_sve, cpu) && value == 0) {
>>> + error_setg(errp, "sve is set but %s is set to 0", propname);
>>> + return false;
>>> + } else if (!cpu_isar_feature(aa64_sve, cpu) && value > 0) {
>>> + error_setg(errp, "sve is not set but %s is greater than 0",
>>> + propname);
>>> + return false;
>>> + }
>>> + /* PAUTH */
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64ISAR1_EL1_APA")) {
>>> + /* PAUTH QARMA5 address authentification */
>>> + error_setg(errp, "%s is now exposed but qemu is not ready to
>>> support it",
>>> + propname);
>>> + return false;
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64ISAR1_EL1_API")) {
>>> + /* PAUTH Impl Defined address authentification */
>>> + error_setg(errp, "%s is now exposed but qemu is not ready to
>>> support it",
>>> + propname);
>>> + return false;
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64ISAR1_EL1_GPA")) {
>>> + /* QARMA5 generic code authentification */
>>> + error_setg(errp, "%s is now exposed but qemu is not ready to
>>> support it",
>>> + propname);
>>> + return false;
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64ISAR1_EL1_GPI")) {
>>> + /* QARMA5 generic code authentification */
>>> + error_setg(errp, "%s is now exposed but qemu is not ready to
>>> support it",
>>> + propname);
>>> + return false;
>>> + /* PMU */
>>> + } else if (!strcmp(propname, "SYSREG_ID_AA64DFR0_EL1_PMUVer")) {
>>> + if (cpu->has_pmu && value == 0) {
>>> + error_setg(errp, "%s is 0 whereas pmu is set", propname);
>>> + return false;
>>> + } else if (!cpu->has_pmu && value) {
>>> + error_setg(errp, "%s is non null whereas pmu is unset",
>>> propname);
>>> + return false;
>>> + }
>>> + /* aarch64 false */
>>> + } else if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>>> + if ((!strcmp(propname, "SYSREG_ID_AA64PFR0_EL1_EL0") ||
>>> + !strcmp(propname, "SYSREG_ID_AA64PFR0_EL1_EL1") ||
>>> + !strcmp(propname, "SYSREG_ID_AA64PFR0_EL1_EL2")) && value < 2)
>>> + error_setg(errp, "%s is < 2 while aarch64 is set to off",
>>> + propname);
>>> + return false;
>>> + }
>>> + return true;
>>> +}
>>> +
>>> #define MAKE_IDREG_KEY(reg_idx, field_shift) \
>>> (((uint64_t)(reg_idx) << 8) | ((uint64_t)(field_shift) & 0xFF))
>>>
>>> @@ -2241,6 +2342,10 @@ static int kvm_arm_apply_sysreg_props(ARMCPU *cpu,
>>> Error **errp)
>>> uint64_t oldfv;
>>> int ret;
>>>
>>> + if (!kvm_arm_vcpu_validate_sysreg(cpu, field, value, errp)) {
>>> + return -1;
>>> + }
>>> +
>>> mask = MAKE_64BIT_MASK(lower, length);
>>> value = value << lower;
>>>
>>> --
>>> 2.53.0
