On 5/26/23 02:07, Sean Christopherson wrote:
> On Thu, May 25, 2023, Michal Luczaj wrote:
>> @@ -265,10 +265,14 @@ static int kvm_recalculate_phys_map(struct kvm_apic_map *new,
>> * mapped, i.e. is aliased to multiple vCPUs. The optimized
>> * map requires a strict 1:1 mapping between IDs and vCPUs.
>> */
>> - if (apic_x2apic_mode(apic))
>> + if (apic_x2apic_mode(apic)) {
>> + if (x2apic_id > new->max_apic_id)
>> + return -EINVAL;
>
> Hmm, disabling the optimized map just because userspace created a new vCPU is
> unfortunate and unnecessary. Rather than return -EINVAL and only perform the
> check when x2APIC is enabled, what if we instead do the check immediately and
> return -E2BIG? Then the caller can retry with a bigger array size. Preemption
> is enabled and retries are bounded by the number of possible vCPUs, so I don't
> see any obvious issues with retrying.
Right, that makes perfect sense.
Just a note, it changes the logic a bit:
- x2apic_format: an overflowing x2apic_id won't be silently ignored.
- !x2apic_format: -E2BIG even for !apic_x2apic_mode() leads to an realloc
instead of "new->phys_map[xapic_id] = apic" right away.
> @@ -228,6 +228,12 @@ static int kvm_recalculate_phys_map(struct kvm_apic_map *new,
> u32 xapic_id = kvm_xapic_id(apic);
> u32 physical_id;
>
> + if (WARN_ON_ONCE(xapic_id >= new->max_apic_id))
> + return -EINVAL;
Shouldn't it be ">" instead of ">="?
That said, xapic_id > new->max_apic_id means something terrible has happened as
kvm_xapic_id() returns u8 and max_apic_id should never be less than 255. Does
this qualify for KVM_BUG_ON?
> + if (x2apic_id >= new->max_apic_id)
> + return -E2BIG;
Probably ">"?
> @@ -366,6 +371,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> unsigned long i;
> u32 max_id = 255; /* enough space for any xAPIC ID */
> bool xapic_id_mismatch = false;
> + int r;
>
> /* Read kvm->arch.apic_map_dirty before kvm->arch.apic_map. */
> if (atomic_read_acquire(&kvm->arch.apic_map_dirty) == CLEAN)
> @@ -386,6 +392,7 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> return;
> }
>
> +retry:
> kvm_for_each_vcpu(i, vcpu, kvm)
> if (kvm_apic_present(vcpu))
> max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
> @@ -404,9 +411,13 @@ void kvm_recalculate_apic_map(struct kvm *kvm)
> if (!kvm_apic_present(vcpu))
> continue;
>
> - if (kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch)) {
> + r = kvm_recalculate_phys_map(new, vcpu, &xapic_id_mismatch);
> + if (r) {
> kvfree(new);
> new = NULL;
> + if (r == -E2BIG)
> + goto retry;
> +
> goto out;
> }
Maybe it's not important, but what about moving xapic_id_mismatch
(re)initialization after "retry:"?
