diff mbox series

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

Message ID 20220706164304.1582687-16-maz@kernel.org (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: vgic-v3 userspace access consolidation (and other goodies) | expand

Commit Message

Marc Zyngier July 6, 2022, 4:43 p.m. UTC 
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(-)

Comments

Reiji Watanabe July 14, 2022, 6:37 a.m. UTC | #1 
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) ?

Thank you,
Reiji


> +       case KVM_VGIC_V2_ADDR_TYPE_DIST:
> +               r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
> +               if (!r)
> +                       r = vgic_check_iorange(kvm, vgic->vgic_dist_base, dev_addr->addr,
> +                                              SZ_4K, KVM_VGIC_V2_DIST_SIZE);
> +               if (!r)
> +                       vgic->vgic_dist_base = dev_addr->addr;
> +               break;
> +       case KVM_VGIC_V2_ADDR_TYPE_CPU:
> +               r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
> +               if (!r)
> +                       r = vgic_check_iorange(kvm, vgic->vgic_cpu_base, dev_addr->addr,
> +                                              SZ_4K, KVM_VGIC_V2_CPU_SIZE);
> +               if (!r)
> +                       vgic->vgic_cpu_base = dev_addr->addr;
> +               break;
> +       default:
> +               r = -ENODEV;
> +       }
> +
> +       mutex_unlock(&kvm->lock);
> +
> +       return r;
> +}
> +
>  /**
>   * kvm_vgic_addr - set or get vgic VM base addresses
>   * @kvm:   pointer to the vm struct
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 2d8f2e90edc2..f79cce67563e 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -365,6 +365,7 @@ extern struct static_key_false vgic_v2_cpuif_trap;
>  extern struct static_key_false vgic_v3_cpuif_trap;
>
>  int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
> +int kvm_set_legacy_vgic_v2_addr(struct kvm *kvm, struct kvm_arm_device_addr *dev_addr);
>  void kvm_vgic_early_init(struct kvm *kvm);
>  int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
>  int kvm_vgic_create(struct kvm *kvm, u32 type);
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Marc Zyngier July 14, 2022, 7:01 a.m. UTC | #2 
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 ;-). More
seriously, thanks for catching this one!

	M.
Reiji Watanabe July 15, 2022, 6:44 a.m. UTC | #3 
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
diff mbox series

Patch

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)) {
+	case KVM_VGIC_V2_ADDR_TYPE_DIST:
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
+		if (!r)
+			r = vgic_check_iorange(kvm, vgic->vgic_dist_base, dev_addr->addr,
+					       SZ_4K, KVM_VGIC_V2_DIST_SIZE);
+		if (!r)
+			vgic->vgic_dist_base = dev_addr->addr;
+		break;
+	case KVM_VGIC_V2_ADDR_TYPE_CPU:
+		r = vgic_check_type(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
+		if (!r)
+			r = vgic_check_iorange(kvm, vgic->vgic_cpu_base, dev_addr->addr,
+					       SZ_4K, KVM_VGIC_V2_CPU_SIZE);
+		if (!r)
+			vgic->vgic_cpu_base = dev_addr->addr;
+		break;
+	default:
+		r = -ENODEV;
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return r;
+}
+
 /**
  * kvm_vgic_addr - set or get vgic VM base addresses
  * @kvm:   pointer to the vm struct
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 2d8f2e90edc2..f79cce67563e 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -365,6 +365,7 @@  extern struct static_key_false vgic_v2_cpuif_trap;
 extern struct static_key_false vgic_v3_cpuif_trap;
 
 int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
+int kvm_set_legacy_vgic_v2_addr(struct kvm *kvm, struct kvm_arm_device_addr *dev_addr);
 void kvm_vgic_early_init(struct kvm *kvm);
 int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
 int kvm_vgic_create(struct kvm *kvm, u32 type);