Hi,

Thanks for your review and patient.

On 2022/6/23 16:13, Marc Zyngier wrote:

On 2022-06-23 02:49, Chen Zhongjin wrote:

quoted

Using unreachable() at default of switch generates an extra branch at
end of the function, and compiler won't generate a ret to close this
branch because it knows it's unreachable.

If there's no instruction in this branch, compiler will generate a NOP,
And it will confuse objtool to warn this NOP as a fall through branch.

In fact these branches are actually unreachable, so we can replace
unreachable() with returning a -EINVAL value.

Signed-off-by: Chen Zhongjin <redacted>
---
 arch/arm64/kvm/hyp/vgic-v3-sr.c | 7 +++----
 drivers/irqchip/irq-gic-v3.c    | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

Basic courtesy would have been to Cc the maintainers of this code.

Sorry for that.

I'll cc everyone next time.

quoted

diff --git a/arch/arm64/kvm/hyp/vgic-v3-sr.c b/arch/arm64/kvm/hyp/vgic-v3-sr.c
index 4fb419f7b8b6..f3cee92c3038 100644
--- a/arch/arm64/kvm/hyp/vgic-v3-sr.c
+++ b/arch/arm64/kvm/hyp/vgic-v3-sr.c

@@ -6,7 +6,6 @@

 #include <hyp/adjust_pc.h>

-#include <linux/compiler.h>
 #include <linux/irqchip/arm-gic-v3.h>
 #include <linux/kvm_host.h>

@@ -55,7 +54,7 @@ static u64 __gic_v3_get_lr(unsigned int lr)

         return read_gicreg(ICH_LR15_EL2);
     }

-    unreachable();
+    return -EINVAL;

NAK. That's absolutely *wrong*, and will hide future bugs.
Nothing checks for -EINVAL, and we *never* expect to
reach this, hence the perfectly valid annotation.

If something needs fixing, it probably is your tooling.

        M.

You are right.

Essentially, this is because objtool does not anticipate that the compiler will
generate additional instructions when marking unreachable instructions.

I'll fix this problem or add a specific check for this state.

Best,
Chen