[RFC PATCH v4 04/26] KVM: arm64: Make ID_AA64PFR0_EL1 writable

[Reiji Watanabe]

Hi Fuad,

On Tue, Feb 1, 2022 at 6:14 AM Fuad Tabba <tabba at google.com> wrote:
>
> Hi Reiji,
>
> ...
>
> > > > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> > > > index 0a06d0648970..28d9bf0e178c 100644
> > > > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > > > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > > > @@ -116,6 +116,11 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
> > > >         else
> > > >                 INIT_LIST_HEAD(&kvm->arch.vgic.rd_regions);
> > > >
> > > > +       if (type == KVM_DEV_TYPE_ARM_VGIC_V3)
> > > > +               /* Set ID_AA64PFR0_EL1.GIC to 1 */
> > > > +               (void)kvm_set_id_reg_feature(kvm, SYS_ID_AA64PFR0_EL1,
> > > > +                                    ID_AA64PFR0_GIC3, ID_AA64PFR0_GIC_SHIFT);
> > > > +
> > >
> > > If this fails wouldn't it be better to return the error?
> >
> > This should never fail because kvm_vgic_create() prevents
> > userspace from running the first KVM_RUN for any vCPUs
> > while it calls kvm_set_id_reg_feature().
> > So, I am thinking of adding WARN_ON_ONCE() for the return value
> > rather than adding an unnecessary error handling.
>
> Consider this to be a nit from my part, as I don't have any strong
> feelings about this, but kvm_vgic_create() already returns an error if
> there's a problem. So I don't think that that would be imposing any
> additional error handling.

That's true.  But, since this failure case cannot be caused by userspace
(but KVM's bug), I would still prefer using WARN_ON_ONCE.

Thanks,
Reiji