Fix arm64 issues highlighted by sparse and smatch

Sparse kicks up about a type mismatch for kvm_target_cpu:

arch/arm64/kvm/guest.c:271:25: error: symbol 'kvm_target_cpu' redeclared with different type (originally declared at ./arch/arm64/include/asm/kvm_host.h:45) - different modifiers

so fix this by adding the missing const attribute to the function declaration.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/include/asm/kvm_host.h   | 2 +-
 arch/arm64/include/asm/kvm_host.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/include/asm/kvm_host.h +1 -1

index 6dfb404f6c46..fcb12a6f7db5 100644
@@ -42,7 +42,7 @@
 
 struct kvm_vcpu;
 u32 *kvm_vcpu_reg(struct kvm_vcpu *vcpu, u8 reg_num, u32 mode);
-int kvm_target_cpu(void);
+int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
 void kvm_reset_coprocs(struct kvm_vcpu *vcpu);
 

arch/arm64/include/asm/kvm_host.h +1 -1

index e10c45a578e3..44094e559848 100644
@@ -42,7 +42,7 @@
 #define KVM_VCPU_MAX_FEATURES 3
 
 struct kvm_vcpu;
-int kvm_target_cpu(void);
+int __attribute_const__ kvm_target_cpu(void);
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu);
 int kvm_arch_dev_ioctl_check_extension(long ext);
 

Sparse kicks up about a type mismatch for kvm_target_cpu:

arch/arm64/kvm/guest.c:271:25: error: symbol 'kvm_target_cpu' redeclared with different type (originally declared at ./arch/arm64/include/asm/kvm_host.h:45) - different modifiers

so fix this by adding the missing const attribute to the function declaration.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Running sparse results in a bunch of noisy address space mismatches thanks to the broken __percpu annotation on kvm_get_running_vcpus.

This function returns a pcpu pointer to a pointer, not a pointer to a pcpu pointer. This patch fixes the annotation, which kills the warnings from sparse.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/kvm/arm.c                | 2 +-
 arch/arm64/include/asm/kvm_host.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/kvm/arm.c +1 -1

index a99e0cdf8ba2..00a5982ca5bc 100644
@@ -82,7 +82,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void)
 /**
  * kvm_arm_get_running_vcpus - get the per-CPU array of currently running vcpus.
  */
-struct kvm_vcpu __percpu **kvm_get_running_vcpus(void)
+struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void)
 {
 	return &kvm_arm_running_vcpu;
 }

arch/arm64/include/asm/kvm_host.h +1 -1

index 44094e559848..50431d36732b 100644
@@ -193,7 +193,7 @@ static inline int kvm_test_age_hva(struct kvm *kvm, unsigned long hva)
 }
 
 struct kvm_vcpu *kvm_arm_get_running_vcpu(void);
-struct kvm_vcpu __percpu **kvm_get_running_vcpus(void);
+struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void);
 
 u64 kvm_call_hyp(void *hypfn, ...);
 

is_valid_cache returns true if the specified cache is valid. Unfortunately, if the parameter passed it out of range, we return -ENOENT, which ends up as true leading to potential hilarity.

This patch returns false on the failure path instead.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/kvm/coproc.c     | 2 +-
 arch/arm64/kvm/sys_regs.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/kvm/coproc.c +1 -1

index 37a0fe1bb9bb..7928dbdf2102 100644
@@ -791,7 +791,7 @@ static bool is_valid_cache(u32 val)
 	u32 level, ctype;
 
 	if (val >= CSSELR_MAX)
-		return -ENOENT;
+		return false;
 
 	/* Bottom bit is Instruction or Data bit.  Next 3 bits are level. */
         level = (val >> 1);

arch/arm64/kvm/sys_regs.c +1 -1

index 5805e7c4a4dd..4cc3b719208e 100644
@@ -1218,7 +1218,7 @@ static bool is_valid_cache(u32 val)
 	u32 level, ctype;
 
 	if (val >= CSSELR_MAX)
-		return -ENOENT;
+		return false;
 
 	/* Bottom bit is Instruction or Data bit.  Next 3 bits are level. */
 	level = (val >> 1);

is_valid_cache returns true if the specified cache is valid. Unfortunately, if the parameter passed it out of range, we return -ENOENT, which ends up as true leading to potential hilarity.

This patch returns false on the failure path instead.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

We currently return the number of bytes not copied if set_timer_reg fails, which is almost certainly not what userspace would like.

This patch returns -EFAULT instead.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm/kvm/guest.c   | 2 +-
 arch/arm64/kvm/guest.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/kvm/guest.c +1 -1

index 813e49258690..cc0b78769bd8 100644
@@ -163,7 +163,7 @@ static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
 	if (ret != 0)
-		return ret;
+		return -EFAULT;
 
 	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
 }

arch/arm64/kvm/guest.c +1 -1

index 8d1ec2887a26..76794692c20b 100644
@@ -174,7 +174,7 @@ static int set_timer_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 
 	ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
 	if (ret != 0)
-		return ret;
+		return -EFAULT;
 
 	return kvm_arm_timer_set_reg(vcpu, reg->id, val);
 }

We currently return the number of bytes not copied if set_timer_reg fails, which is almost certainly not what userspace would like.

This patch returns -EFAULT instead.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

vgic_ioaddr_overlap claims to return a bool, but in reality it returns an int. Shut sparse up by fixing the type signature.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 virt/kvm/arm/vgic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

virt/kvm/arm/vgic.c +1 -1

index 73eba793b17f..d1cfe672b9d7 100644
@@ -1690,7 +1690,7 @@ out:
 	return ret;
 }
 
-static bool vgic_ioaddr_overlap(struct kvm *kvm)
+static int vgic_ioaddr_overlap(struct kvm *kvm)
 {
 	phys_addr_t dist = kvm->arch.vgic.vgic_dist_base;
 	phys_addr_t cpu = kvm->arch.vgic.vgic_cpu_base;

vgic_ioaddr_overlap claims to return a bool, but in reality it returns an int. Shut sparse up by fixing the type signature.

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

We extract the vgic probe function from the of_device_id data pointer, which is const. Kill the sparse warning by ensuring that the local function pointer is also marked as const.

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 virt/kvm/arm/vgic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

virt/kvm/arm/vgic.c +2 -2

index d1cfe672b9d7..efe6eee2e7eb 100644
@@ -1557,8 +1557,8 @@ static const struct of_device_id vgic_ids[] = {
 int kvm_vgic_hyp_init(void)
 {
 	const struct of_device_id *matched_id;
-	int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
-			  const struct vgic_params **);
+	const int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
+				const struct vgic_params **);
 	struct device_node *vgic_node;
 	int ret;
 

We extract the vgic probe function from the of_device_id data pointer, which is const. Kill the sparse warning by ensuring that the local function pointer is also marked as const.

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

The internal irq domain ops for the GIC are not used directly anywhere else, so make them static. This gets rid of a sparse warning on the file.

Cc Jason Cooper <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 drivers/irqchip/irq-gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-gic.c +1 -1

index 4b959e606fe8..dda6dbc23565 100644
@@ -867,7 +867,7 @@ static int gic_routable_irq_domain_xlate(struct irq_domain *d,
 	return 0;
 }
 
-const struct irq_domain_ops gic_default_routable_irq_domain_ops = {
+static const struct irq_domain_ops gic_default_routable_irq_domain_ops = {
 	.map = gic_routable_irq_domain_map,
 	.unmap = gic_routable_irq_domain_unmap,
 	.xlate = gic_routable_irq_domain_xlate,

The __percpu __iomem annotations on the rdist base are contradictory and confuse static checkers such as sparse.

This patch fixes the anotations so that rdist is described as a __percpu pointer to an __iomem pointer.

Cc Jason Cooper <[email protected]>

Cc Marc Zyngier <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 drivers/irqchip/irq-gic-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/irqchip/irq-gic-v3.c +1 -1

index 57eaa5a0b1e3..4d7b7c28c554 100644
@@ -36,7 +36,7 @@
 struct gic_chip_data {
 	void __iomem		*dist_base;
 	void __iomem		**redist_base;
-	void __percpu __iomem	**rdist;
+	void __iomem * __percpu	*rdist;
 	struct irq_domain	*domain;
 	u64			redist_stride;
 	u32			redist_regions;

The current perf_regs code relies on sp and pc sitting just off the end of the pt_regs->regs array. This is ugly and fragile, so this patch checks for these register explicitly and returns the appropriate field.

Cc Jean Pihet <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/kernel/perf_regs.c | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm64/kernel/perf_regs.c +6 -1

index 422ebd63b619..6762ad705587 100644
@@ -24,6 +24,12 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
                        return regs->compat_lr;
        }
 
+       if ((u32)idx == PERF_REG_ARM64_SP)
+               return regs->sp;
+
+       if ((u32)idx == PERF_REG_ARM64_PC)
+               return regs->pc;
+
        return regs->regs[idx];
 }
 
--

2.1.0.rc1

Hi all,

After the static checking talk at kernel summit, I felt empowered to go and run sparse and smatch over the arm64 defconfig. As I anticipated, this spotted a few issues in the arch code, so I've fixed up the ones that aren't just noise in this series. Most are simply incorrect address space annotations, but fixing these is useful for spotting real issues in the future.

I don't think any of these fix critical bugs, but they all deserve to be addressed. I tagged the compat watchpoint fix for stable, as that code has been broken since we merged the architecture port.

Quoted 9 lines · Will Deacon

Hi all,

After the static checking talk at kernel summit, I felt empowered to go and run sparse and smatch over the arm64 defconfig. As I anticipated, this spotted a few issues in the arch code, so I've fixed up the ones that aren't just noise in this series. Most are simply incorrect address space annotations, but fixing these is useful for spotting real issues in the future.

Quoted 4 lines

I don't think any of these fix critical bugs, but they all deserve to be addressed. I tagged the compat watchpoint fix for stable, as that code has been broken since we merged the architecture port.

Do you want me to queue the kvm ones or did you have other plans to upstream this entire series as one?

Also, do you consider these fixes for 3.17 (what about cc for stable in that case), or for 3.18?

Hi Christoffer,

Quoted 10 lines · Christoffer Dall

Quoted 9 lines · Will Deacon

Hi all,

After the static checking talk at kernel summit, I felt empowered to go and run sparse and smatch over the arm64 defconfig. As I anticipated, this spotted a few issues in the arch code, so I've fixed up the ones that aren't just noise in this series. Most are simply incorrect address space annotations, but fixing these is useful for spotting real issues in the future.

Quoted 7 lines

Quoted 4 lines

I don't think any of these fix critical bugs, but they all deserve to be addressed. I tagged the compat watchpoint fix for stable, as that code has been broken since we merged the architecture port.

Do you want me to queue the kvm ones or did you have other plans to upstream this entire series as one?

They're all independent, so please take the kvm ones via your tree. I can send a separate pull if you prefer? Jason -- would you like an irqchip-specific pull for those?

Quoted 2 lines

Also, do you consider these fixes for 3.17 (what about cc for stable in that case), or for 3.18?

The only one I CC'd stable for was the compat ptrace watchpoint patch, because that's something that we've been getting wrong and will cause breakage with a 32-bit GDB. I don't consider the others urgent (i.e. happy if you wait until 3.18).

Hi all,

After the static checking talk at kernel summit, I felt empowered to go and run sparse and smatch over the arm64 defconfig. As I anticipated, this spotted a few issues in the arch code, so I've fixed up the ones that aren't just noise in this series. Most are simply incorrect address space annotations, but fixing these is useful for spotting real issues in the future.

I don't think any of these fix critical bugs, but they all deserve to be addressed. I tagged the compat watchpoint fix for stable, as that code has been broken since we merged the architecture port.

Cheers,

Will

Cc Christoffer Dall <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Jason Cooper <[email protected]>

Cc Jean Pihet <[email protected]>

--->8

Will Deacon (11):
irqchip: gic: make gic_default_routable_irq_domain_ops static
irqchip: gic-v3: declare rdist as __percpu pointer to __iomem pointer

 drivers/irqchip/irq-gic-v3.c           |  2 +-
 drivers/irqchip/irq-gic.c              |  2 +-