On Mon, May 26, 2025, Neeraj Upadhyay wrote:
>
>
> On 5/24/2025 5:42 PM, Borislav Petkov wrote:
> >
> > The previous patch is moving those *_POS() macros to arch/x86/kvm/lapic.c, now
> > this patch is doing rename-during-move to the new macros.
> >
> > Why can't you simply do the purely mechanical moves first and then do the
> > renames? Didn't I explain it the last time? Or is it still unclear?
> >
>
> I thought it was clear to me when you explained last time. However, I did this
> rename-during-move because of below reason. Please correct me if I am wrong here.
>
> VEC_POS, REG_POS are kvm-internal wrappers for APIC_VECTOR_TO_BIT_NUMBER/
> APIC_VECTOR_TO_REG_OFFSET macros which got defined in patch 01/32. Prior to patch
> 06/32, these macros were defined in kvm-internal header arch/x86/kvm/lapic.h. Using
> VEC_POS, REG_POS kvm-internal macros in x86 common header file (arch/x86/include/asm/apic.h)
> in this patch did not look correct to me and as APIC_VECTOR_TO_BIT_NUMBER/APIC_VECTOR_TO_REG_OFFSET
> are already defined in arch/x86/include/asm/apic.h, I used them.
>
> Is adding this information in commit log of this patch sufficient or do you have some
> other suggestion for doing this?
I agree that moving VEC_POS/REG_POS to common code would be weird/undesirable,
but I also agree with Boris' underlying point that doing renames as part of code
movement is also undesirable. And you're doing that all over this series.
So, just one patch at the beginning of the series to replace VEC_POS/REG_POS with
APIC_VECTOR_TO_BIT_NUMBER/APIC_VECTOR_TO_REG_OFFSET, but *only* in the functions
you intended to move out of KVM. That way you separate code movement and rename
patches.
Actually, looking at the end usage, just drop VEC_POS/REG_POS entirely. IIRC, I
suggested keeping the shorthand versions for KVM, but I didn't realize there would
literally be two helpers left. At that point, keeping VEC_POS and REG_POS is
pure stubborness :-)
1. Rename VEC_POS/REG_POS => APIC_VECTOR_TO_BIT_NUMBER/APIC_VECTOR_TO_REG_OFFSET
2. Rename all of the KVM helpers you intend to move out of KVM.
3. Move all of the helpers out of KVM.
That way #1 and #2 are pure KVM changes, and the code review movement is easy to
review because it'll be _just_ code movement.
Actually (redux), we should probably kill off __apic_test_and_set_vector() and
__apic_test_and_clear_vector(), because the _only_ register that's safe to modify
with a non-atomic operation is ISR, because KVM isn't running the vCPU, i.e.
hardware can't service an IRQ or process an EOI for the relevant (virtual) APIC.
So this on top somewhere? (completely untested)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 8ecc3e960121..95921e5c3eb2 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -104,16 +104,6 @@ bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
apic_test_vector(vector, apic->regs + APIC_IRR);
}
-static inline int __apic_test_and_set_vector(int vec, void *bitmap)
-{
- return __test_and_set_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
-}
-
-static inline int __apic_test_and_clear_vector(int vec, void *bitmap)
-{
- return __test_and_clear_bit(VEC_POS(vec), (bitmap) + REG_POS(vec));
-}
-
__read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
EXPORT_SYMBOL_GPL(kvm_has_noapic_vcpu);
@@ -706,9 +696,15 @@ void kvm_apic_clear_irr(struct kvm_vcpu *vcpu, int vec)
}
EXPORT_SYMBOL_GPL(kvm_apic_clear_irr);
+static void *apic_vector_to_isr(int vec, struct kvm_lapic *apic)
+{
+ return apic->regs + APIC_ISR + APIC_VECTOR_TO_REG_OFFSET(vec);
+}
+
static inline void apic_set_isr(int vec, struct kvm_lapic *apic)
{
- if (__apic_test_and_set_vector(vec, apic->regs + APIC_ISR))
+ if (__test_and_set_bit(APIC_VECTOR_TO_BIT_NUMBER(vec),
+ apic_vector_to_isr(vec, apic)))
return;
/*
@@ -751,7 +747,8 @@ static inline int apic_find_highest_isr(struct kvm_lapic *apic)
static inline void apic_clear_isr(int vec, struct kvm_lapic *apic)
{
- if (!__apic_test_and_clear_vector(vec, apic->regs + APIC_ISR))
+ if (!__test_and_clear_bit(APIC_VECTOR_TO_BIT_NUMBER(vec),
+ apic_vector_to_isr(vec, apic)))
return;
/*
