Re: [patch 7/7] KVM: in-kernel ACPI C2 idle emulation

[Avi Kivity <avi@qumranet.com>]

Marcelo Tosatti wrote:
> Some guests fail to process the _CST notification which invalidates the C2 state.
>
>   

Which?

> Emulate C2 similarly to HLT for those cases.
>   


Please split the API change from actual addition of the ACPI emulation.

> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm/arch/x86/kvm/Makefile
> ===================================================================
> --- kvm.orig/arch/x86/kvm/Makefile
> +++ kvm/arch/x86/kvm/Makefile
> @@ -11,7 +11,7 @@ endif
>  EXTRA_CFLAGS += -Ivirt/kvm -Iarch/x86/kvm
>  
>  kvm-objs := $(common-objs) x86.o mmu.o x86_emulate.o i8259.o irq.o lapic.o \
> -	i8254.o
> +	i8254.o acpi.o
>  obj-$(CONFIG_KVM) += kvm.o
>  kvm-intel-objs = vmx.o
>  obj-$(CONFIG_KVM_INTEL) += kvm-intel.o
> Index: kvm/arch/x86/kvm/acpi.c
> ===================================================================
> --- /dev/null
> +++ kvm/arch/x86/kvm/acpi.c
> @@ -0,0 +1,82 @@
> +#include <linux/kvm_host.h>
> +#include <linux/kvm.h>
> +#include <linux/acpi_pmtmr.h>
> +#include <asm/io.h>
> +#include "iodev.h"
> +#include "irq.h"
> +
> +/*
> + * P_LVL2 = PBLK + 4h
> + *
> + * Note: matches BIOS (currently hardcoded) definition.
> + */
> +#define ACPI_PBLK 0xb010
>   

Please make it part of the API.

> +
> +struct kvm_acpi {
> +	struct kvm_io_device dev;
> +	struct kvm *kvm;
> +};
> +
> +static void acpi_cstate_halt(struct kvm_vcpu *vcpu)
> +{
> +	vcpu->arch.mp_state = KVM_MP_STATE_HALTED;
> +	mutex_unlock(&vcpu->kvm->lock);
> +	up_read(&vcpu->kvm->slots_lock);
> +	kvm_vcpu_block(vcpu);
> +	down_read(&vcpu->kvm->slots_lock);
> +	mutex_lock(&vcpu->kvm->lock);
> +	vcpu->arch.pio.size = 0;
> +}
> +
> +static void acpi_ioport_read(struct kvm_vcpu *vcpu, struct kvm_io_device *this,
> +			     gpa_t addr, int len, void *data)
> +{
> +
> +	if (addr == ACPI_PBLK + 0x4)
> +		acpi_cstate_halt(vcpu);
> +
>   

Don't you need to write to the data here?

> @@ -1725,6 +1727,10 @@ long kvm_arch_vm_ioctl(struct file *filp
>  		r = 0;
>  		break;
>  	}
> +	case KVM_ENABLE_ACPI_C2: {
> +		r = kvm_acpi_init(kvm);
> +		break;
> +	}
>   

Need a disable too.



-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.