Re: [PATCH 15/19] KVM: arm64: vgic-v2: Add helper for legacy dist/cpuif base address setting

From: Reiji Watanabe <reijiw@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	 Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Schspa Shi <schspa@gmail.com>,
	 kernel-team@android.com, Oliver Upton <oliver.upton@linux.dev>
Subject: Re: [PATCH 15/19] KVM: arm64: vgic-v2: Add helper for legacy dist/cpuif base address setting
Date: Thu, 14 Jul 2022 23:44:21 -0700	[thread overview]
Message-ID: <CAAeT=FwwO5=v3vLJ0qAw3V0NaPEnPeP1VmgLXXBL4jdm80aeew@mail.gmail.com> (raw)
In-Reply-To: <877d4gyy7y.wl-maz@kernel.org>

On Thu, Jul 14, 2022 at 12:01 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 14 Jul 2022 07:37:25 +0100,
> Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Hi Marc,
> >
> > On Wed, Jul 6, 2022 at 10:05 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > We carry a legacy interface to set the base addresses for GICv2.
> > > As this is currently plumbed into the same handling code as
> > > the modern interface, it limits the evolution we can make there.
> > >
> > > Add a helper dedicated to this handling, with a view of maybe
> > > removing this in the future.
> > >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  arch/arm64/kvm/arm.c                  | 11 ++-------
> > >  arch/arm64/kvm/vgic/vgic-kvm-device.c | 32 +++++++++++++++++++++++++++
> > >  include/kvm/arm_vgic.h                |  1 +
> > >  3 files changed, 35 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index 83a7f61354d3..bf39570c0aef 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -1414,18 +1414,11 @@ void kvm_arch_flush_remote_tlbs_memslot(struct kvm *kvm,
> > >  static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
> > >                                         struct kvm_arm_device_addr *dev_addr)
> > >  {
> > > -       unsigned long dev_id, type;
> > > -
> > > -       dev_id = (dev_addr->id & KVM_ARM_DEVICE_ID_MASK) >>
> > > -               KVM_ARM_DEVICE_ID_SHIFT;
> > > -       type = (dev_addr->id & KVM_ARM_DEVICE_TYPE_MASK) >>
> > > -               KVM_ARM_DEVICE_TYPE_SHIFT;
> > > -
> > > -       switch (dev_id) {
> > > +       switch (FIELD_GET(KVM_ARM_DEVICE_ID_MASK, dev_addr->id)) {
> > >         case KVM_ARM_DEVICE_VGIC_V2:
> > >                 if (!vgic_present)
> > >                         return -ENXIO;
> > > -               return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
> > > +               return kvm_set_legacy_vgic_v2_addr(kvm, dev_addr);
> > >         default:
> > >                 return -ENODEV;
> > >         }
> > > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > index fbbd0338c782..0dfd277b9058 100644
> > > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> > > @@ -41,6 +41,38 @@ static int vgic_check_type(struct kvm *kvm, int type_needed)
> > >                 return 0;
> > >  }
> > >
> > > +int kvm_set_legacy_vgic_v2_addr(struct kvm *kvm, struct kvm_arm_device_addr *dev_addr)
> > > +{
> > > +       struct vgic_dist *vgic = &kvm->arch.vgic;
> > > +       int r;
> > > +
> > > +       mutex_lock(&kvm->lock);
> > > +       switch (FIELD_GET(KVM_ARM_DEVICE_ID_MASK, dev_addr->id)) {
> >
> > Shouldn't this be KVM_ARM_DEVICE_TYPE_MASK (not KVM_ARM_DEVICE_ID_MASK) ?
>
> Damn, you just ruined my attempt at deprecating this API ;-).

Oops, I should have pretended not to notice:)

> More seriously, thanks for catching this one!

Thank you for cleaning up the code so much!
Reiji

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