Re: [PATCH 16/19] KVM: x86: Explicitly track all possibilities for APIC map's logical modes

[Sean Christopherson]

On Wed, Aug 31, 2022, Maxim Levitsky wrote:
> On Wed, 2022-08-31 at 00:35 +0000, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 8209caffe3ab..3b6ef36b3963 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -168,7 +168,12 @@ static bool kvm_use_posted_timer_interrupt(struct kvm_vcpu *vcpu)
> >  
> >  static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
> >  		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
> > -	switch (map->mode) {
> > +	switch (map->logical_mode) {
> > +	case KVM_APIC_MODE_SW_DISABLED:
> > +		/* Arbitrarily use the flat map so that @cluster isn't NULL. */
> > +		*cluster = map->xapic_flat_map;
> > +		*mask = 0;
> > +		return true;
> >  	case KVM_APIC_MODE_X2APIC: {
> >  		u32 offset = (dest_id >> 16) * 16;
> >  		u32 max_apic_id = map->max_apic_id;
> > @@ -193,8 +198,10 @@ static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
> >  		*cluster = map->xapic_cluster_map[(dest_id >> 4) & 0xf];
> >  		*mask = dest_id & 0xf;
> >  		return true;
> > +	case KVM_APIC_MODE_MAP_DISABLED:
> > +		return false;
> >  	default:
> > -		/* Not optimized. */
> > +		WARN_ON_ONCE(1);
> 
> BTW unless I am mistaken, this warning is guest triggerable, and thus as you
> say when 'panic_on_warn=1', this will panic the host kernel.

If it's guest triggerable then it's a bug in this patch.  The "default" case was
reachable with the old approach of OR-ing in bits, but the intent of this patch
is to fully enumerate all values of map->logical_mode and make the "default" case
impossible.

And I don't think it's reachable.  The case statements are:

	case KVM_APIC_MODE_SW_DISABLED:
	case KVM_APIC_MODE_X2APIC:
	case KVM_APIC_MODE_XAPIC_FLAT:
	case KVM_APIC_MODE_XAPIC_CLUSTER:
	case KVM_APIC_MODE_MAP_DISABLED:
	default:

which covers all of the possible enum values.

	enum kvm_apic_logical_mode {
		KVM_APIC_MODE_SW_DISABLED,
		KVM_APIC_MODE_XAPIC_CLUSTER,
		KVM_APIC_MODE_XAPIC_FLAT,
		KVM_APIC_MODE_X2APIC,
		KVM_APIC_MODE_MAP_DISABLED,
	};

The map is explicitly initialized to KVM_APIC_MODE_SW_DISABLED (to avoid relying
on KVM_APIC_MODE_SW_DISABLED==0)

	new->logical_mode = KVM_APIC_MODE_SW_DISABLED;

so unless I've missed a "logical_mode |= ..." somewhere, reaching "default" should
be impossible.