Re: [RFC PATCH v3 04/29] KVM: arm64: Make ID_AA64PFR0_EL1 writable

[Eric Auger <eauger@redhat.com>]

Hi Reiji,

On 12/4/21 8:59 AM, Reiji Watanabe wrote:
> Hi Eric,
> 
> On Thu, Dec 2, 2021 at 5:02 AM Eric Auger <eauger@redhat.com> wrote:
>>
>> Hi Reiji,
>>
>> On 11/30/21 2:29 AM, Reiji Watanabe wrote:
>>> Hi Eric,
>>>
>>> On Thu, Nov 25, 2021 at 7:35 AM Eric Auger <eauger@redhat.com> wrote:
>>>>
>>>> Hi Reiji,
>>>>
>>>> On 11/17/21 7:43 AM, Reiji Watanabe wrote:
>>>>> This patch adds id_reg_info for ID_AA64PFR0_EL1 to make it writable by
>>>>> userspace.
>>>>>
>>>>> The CSV2/CSV3 fields of the register were already writable and values
>>>>> that were written for them affected all vCPUs before. Now they only
>>>>> affect the vCPU.
>>>>> Return an error if userspace tries to set SVE/GIC field of the register
>>>>> to a value that conflicts with SVE/GIC configuration for the guest.
>>>>> SIMD/FP/SVE fields of the requested value are validated according to
>>>>> Arm ARM.
>>>>>
>>>>> Signed-off-by: Reiji Watanabe <reijiw@google.com>
>>>>> ---
>>>>>  arch/arm64/kvm/sys_regs.c | 159 ++++++++++++++++++++++++--------------
>>>>>  1 file changed, 103 insertions(+), 56 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>>> index 1552cd5581b7..35400869067a 100644
>>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>>> @@ -401,6 +401,92 @@ static void id_reg_info_init(struct id_reg_info *id_reg)
>>>>>               id_reg->init(id_reg);
>>>>>  }
>>>>>
>>>>> +#define      kvm_has_gic3(kvm)               \
>>>>> +     (irqchip_in_kernel(kvm) &&      \
>>>>> +      (kvm)->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3)
>>>> you may move this macro to kvm/arm_vgic.h as this may be used in
>>>> vgic/vgic-v3.c too
>>>
>>> Thank you for the suggestion. I will move that to kvm/arm_vgic.h.
>>>
>>>
>>>>> +
>>>>> +static int validate_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
>>>>> +                                 const struct id_reg_info *id_reg, u64 val)
>>>>> +{
>>>>> +     int fp, simd;
>>>>> +     bool vcpu_has_sve = vcpu_has_sve(vcpu);
>>>>> +     bool pfr0_has_sve = id_aa64pfr0_sve(val);
>>>>> +     int gic;
>>>>> +
>>>>> +     simd = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_ASIMD_SHIFT);
>>>>> +     fp = cpuid_feature_extract_signed_field(val, ID_AA64PFR0_FP_SHIFT);
>>>>> +     if (simd != fp)
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     /* fp must be supported when sve is supported */
>>>>> +     if (pfr0_has_sve && (fp < 0))
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     /* Check if there is a conflict with a request via KVM_ARM_VCPU_INIT */
>>>>> +     if (vcpu_has_sve ^ pfr0_has_sve)
>>>>> +             return -EPERM;
>>>>> +
>>>>> +     gic = cpuid_feature_extract_unsigned_field(val, ID_AA64PFR0_GIC_SHIFT);
>>>>> +     if ((gic > 0) ^ kvm_has_gic3(vcpu->kvm))
>>>>> +             return -EPERM;
>>>>
>>>> Sometimes from a given architecture version, some lower values are not
>>>> allowed. For instance from ARMv8.5 onlt 1 is permitted for CSV3.
>>>> Shouldn't we handle that kind of check?
>>>
>>> As far as I know, there is no way for guests to identify the
>>> architecture revision (e.g. v8.1, v8.2, etc).  It might be able
>>> to indirectly infer the revision though (from features that are
>>> available or etc).
>>
>> OK. That sounds weird to me as we do many checks accross different IDREG
>> settings but we may eventually have a wrong "CPU model" exposed by the
>> user space violating those spec revision minima. Shouldn't we introduce
>> some way for the userspace to provide his requirements? via new VCPU
>> targets for instance?
> 
> Thank you for sharing your thoughts and providing the suggestion !
> 
> Does the "new vCPU targets" mean Armv8.0, armv8.1, and so on ?

Yeah my suggestion probably is not a good idea, ie. introducing such
VCPU targets. I was simply confused by the fact we introduce in this
series quite intricate consistency checks but given the fact we miss the
spec rev information we are not exhaustive in terms of checking. So it
is sometimes difficult to review against the spec.

> 
> The ID registers' consistency checking in the series is to not
> promise more to userspace than what KVM (on the host) can provide,
> and to not expose ID register values that are not supported on
> any ARM v8 architecture for guests (I think those are what the
> current KVM is trying to assure).  I'm not trying to have KVM
> provide full consistency checking of ID registers to completely
> prevent userspace's bugs in setting ID registers.
> 
> I agree that it's quite possible that userspace exposes such wrong
> CPU models, and KVM's providing more consistency checking would be
> nicer in general.  But should it be KVM's responsibility to completely
> prevent such ID register issues due to userspace bugs ?
> 
> Honestly, I'm a bit reluctant to do that so far yet:)

understood. I will look at the spec in more details on my next review
cycle. Looking forward to reviewing the next version ;-)

Thanks

Eric
> If that is something useful that userspace or we (KVM developers)
> really want or need, or such userspace issue could affect KVM,
> I would be happy to add such extra consistency checking though.
> 
> Thanks,
> Reiji
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel