Re: [PATCH for-6.1 v2] i386: do not call cpudef-only models functions for max, host, base

[Claudio Fontana <cfontana@suse.de>]

On 11/29/21 4:11 PM, David Woodhouse wrote:
> On Mon, 2021-11-29 at 15:14 +0100, Claudio Fontana wrote:
>> On 11/29/21 12:39 PM, Woodhouse, David wrote:
>>> On Fri, 2021-07-23 at 13:29 +0200, Claudio Fontana wrote:
>>>>  static void kvm_cpu_instance_init(CPUState *cs)
>>>>  {
>>>>      X86CPU *cpu = X86_CPU(cs);
>>>> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
>>>>
>>>>      host_cpu_instance_init(cpu);
>>>>
>>>> -    if (!kvm_irqchip_in_kernel()) {
>>>> -        x86_cpu_change_kvm_default("x2apic", "off");
>>>> -    } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
>>>> -        x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>>>> -    }
>>>> -
>>>> -    /* Special cases not set in the X86CPUDefinition structs: */
>>>> +    if (xcc->model) {
>>>> +        /* only applies to builtin_x86_defs cpus */
>>>> +        if (!kvm_irqchip_in_kernel()) {
>>>> +            x86_cpu_change_kvm_default("x2apic", "off");
>>>> +        } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
>>>> +            x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
>>>> +        }
>>>>
>>>> -    x86_cpu_apply_props(cpu, kvm_default_props);
>>>> +        /* Special cases not set in the X86CPUDefinition structs: */
>>>> +        x86_cpu_apply_props(cpu, kvm_default_props);
>>>> +    }
>>>>
>>>
>>> I think this causes a regression in x2apic and kvm-msi-ext-dest-id
>>> support. If you start qemu thus:
>>
>> If I recall correctly, this change just tries to restore the behavior prior to
>> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448 ,
>>
>> fixing the issue introduced with the refactoring at that time.
>>
>> Can you try bisecting prior to
>> f5cc5a5c168674f84bf061cdb307c2d25fba5448 , to see if the actual
>> breakage comes from somewhere else?
> 
> Hm, so it looks like it never worked for '-cpu host' *until* commit
> f5cc5a5c16.

Right, so here we are talking about properly supporting this for the first time.

The fact that it works with f5cc5a5c16 is more an accident than anything else, that commit was clearly broken
(exemplified by reports of failed boots).

So we need to find the proper solution, ie, exactly which features should be enabled for which cpu classes and models.

> 
> It didn't matter before c1bb5418e3 because you couldn't enable that
> many vCPUs without an IOMMU, and the *IOMMU* setup would call
> kvm_enable_x2apic().
> 
> But after that, nothing ever called kvm_enable_x2apic() in the '-cpu
> host' case until commit f5cc5a5c16, which fixed it... until you
> restored the previous behaviour :)
> 
> This "works" to fix this case, but presumably isn't correct:

Right, we cannot just enable all this code, or the original refactor would have been right.

These kvm default properties have been as far as I know intended for the cpu actual models (builtin_x86_defs),
and not for the special cpu classes max, host and base. This is what the revert addresses.

I suspect what we actually need here is to review exactly in which specific cases kvm_enable_x2apic() should be called in the end.

The code there is mixing changes to the kvm_default_props that are then applied using x86_cpu_apply_props (and that part should be only for xcc->model != NULL),
with the actual enablement of the kvm x2apic using kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags) via kvm_enable_x2apic().

One way is to ignore this detail and just move out those checks, since changes to kvm_default_props are harmless once we skip the x86_cpu_apply_props call,
as such: 

------

static void kvm_cpu_instance_init(CPUState *cs)
{
    X86CPU *cpu = X86_CPU(cs);
    X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);

    host_cpu_instance_init(cpu);

    /* only applies to builtin_x86_defs cpus */
    if (!kvm_irqchip_in_kernel()) {
        x86_cpu_change_kvm_default("x2apic", "off");
    } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
        x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
    }

    if (xcc->model) {
        /* Special cases not set in the X86CPUDefinition structs: */
        x86_cpu_apply_props(cpu, kvm_default_props);
    }

    if (cpu->max_features) {
        kvm_cpu_max_instance_init(cpu);
    }

    kvm_cpu_xsave_init();
}

------

this might however cause further confusion later on, and I wonder if this is actually correct, should we _always_ enable x2apic when kvm_irqchip_is_split() returns true?
Even for cpu class "base"? I am not too sure.

Another option that comes to mind is to add a call to enable x2apic for max features cpus only ("host", "max") and not for base.

Thoughts? Paolo, Edoardo, anything comes to mind from your side?

Ciao,

Claudio


> 
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -161,7 +161,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
>  
>      host_cpu_instance_init(cpu);
>  
> -    if (xcc->model) {
> +    if (1 || xcc->model) {
>          /* only applies to builtin_x86_defs cpus */
>          if (!kvm_irqchip_in_kernel()) {
>              x86_cpu_change_kvm_default("x2apic", "off");
> 
> 
>>> Any image to specifically test out? Would an actual 9 sockets machine be required to reproduce this?
> 
> No, but the more CPUs you have in the host the less you have to wait
> for 288 vCPUs to spin up :)
> 
> My test is:
> 
> ./qemu-system-x86_64 -machine q35,accel=kvm,usb=off,kernel_irqchip=split -cpu host -m 2G -smp sockets=9,cores=16,threads=2 -drive file=/var/lib/libvirt/images/fedora.qcow2,if=virtio -serial mon:stdio -display none  -kernel ~/git/linux/arch/x86/boot/bzImage  -append "console=ttyS0,115200 root=/dev/vda1" 
> 
> 
> I then play with the affinity of the AHCI MSI. Pointing it at CPU 255
> should show the problem. 
> 
> [root@localhost ~]# cd /proc/irq/313
> [root@localhost 313]# echo 255 > smp_affinity_list 
> [root@localhost 313]#
> [   65.365821] Composed MSI for APIC 255 vector 0x22: 0/feeff000 22
> [root@localhost 313]# grep ahci /proc/interrupts 
> 
> 
> I also added some debugging into host and guest kernels to be a little
> more explicit:
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index b70344bf6600..53191db5145d 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1866,6 +1866,7 @@ static __init void try_to_enable_x2apic(int remap_mode)
>  		 * used for non-remapped IRQ domains.
>  		 */
>  		if (x86_init.hyper.msi_ext_dest_id()) {
> +			pr_info("x2apic: support extended destination ID\n");
>  			virt_ext_dest_id = 1;
>  			apic_limit = 32767;
>  		}
> @@ -2539,6 +2540,7 @@ void __irq_msi_compose_msg(struct irq_cfg *cfg, struct msi_msg *msg,
>  		msg->arch_addr_lo.virt_destid_8_14 = cfg->dest_apicid >> 8;
>  	else
>  		WARN_ON_ONCE(cfg->dest_apicid > 0xFF);
> +	printk("Composed MSI for APIC %d vector 0x%x: %x/%x %x\n", cfg->dest_apicid, cfg->vector, msg->address_hi, msg->address_lo, msg->data);
>  }
>  
>  u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 59abbdad7729..f0a7715763a2 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -856,6 +856,8 @@ static void __init kvm_apic_init(void)
>  
>  static bool __init kvm_msi_ext_dest_id(void)
>  {
> +	printk("dest id? %d (%x)\n", kvm_para_has_feature(KVM_FEATURE_MSI_EXT_DEST_ID),
> +	       kvm_arch_para_features());
>  	return kvm_para_has_feature(KVM_FEATURE_MSI_EXT_DEST_ID);
>  }
>  
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 759952dd1222..defe6a843780 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -894,15 +894,21 @@ static bool kvm_apic_is_broadcast_dest(struct kvm *kvm, struct kvm_lapic **src,
>  {
>  	if (kvm->arch.x2apic_broadcast_quirk_disabled) {
>  		if ((irq->dest_id == APIC_BROADCAST &&
> -				map->mode != KVM_APIC_MODE_X2APIC))
> +		     map->mode != KVM_APIC_MODE_X2APIC)) {
> +			printk("dest %d mode %d makes bcast\n", irq->dest_id, map->mode);
>  			return true;
> -		if (irq->dest_id == X2APIC_BROADCAST)
> +		}
> +		if (irq->dest_id == X2APIC_BROADCAST)  {
> +			printk("Sent to X2APIC bcast\n");
>  			return true;
> +		}
>  	} else {
>  		bool x2apic_ipi = src && *src && apic_x2apic_mode(*src);
>  		if (irq->dest_id == (x2apic_ipi ?
> -		                     X2APIC_BROADCAST : APIC_BROADCAST))
> +		                     X2APIC_BROADCAST : APIC_BROADCAST)) {
> +			printk("no quirk dest %x\n", irq->dest_id);
>  			return true;
> +		}
>  	}
>  
>  	return false;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d8f1d2169b45..5b0fd6d37a7e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5714,6 +5714,7 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>  		if (cap->args[0] & KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
>  			kvm->arch.x2apic_broadcast_quirk_disabled = true;
>  
> +		printk("X2APIC API: %x\n", cap->args[0]);
>  		r = 0;
>  		break;
>  	case KVM_CAP_X86_DISABLE_EXITS:
>