Re: [PATCH 03/13] KVM: x86: Add emulation support for Extented LVT registers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 04, 2023 at 09:53:37AM +0000, Manali Shukla wrote:
>From: Santosh Shukla <santosh.shukla@xxxxxxx>
>
>The local interrupts are extended to include more LVT registers in
>order to allow additional interrupt sources, like Instruction Based
>Sampling (IBS) and many more.
>
>Currently there are four additional LVT registers defined and they are
>located at APIC offsets 500h-530h.
>
>AMD IBS driver is designed to use EXTLVT (Extended interrupt local
>vector table) by default for driver initialization.
>
>Extended LVT registers are required to be emulated to initialize the
>guest IBS driver successfully.
>
>Please refer to Section 16.4.5 in AMD Programmer's Manual Volume 2 at
>https://bugzilla.kernel.org/attachment.cgi?id=304653 for more details
>on EXTLVT.
>
>Signed-off-by: Santosh Shukla <santosh.shukla@xxxxxxx>
>Co-developed-by: Manali Shukla <manali.shukla@xxxxxxx>
>Signed-off-by: Manali Shukla <manali.shukla@xxxxxxx>
>---
> arch/x86/include/asm/apicdef.h | 14 ++++++++
> arch/x86/kvm/lapic.c           | 66 ++++++++++++++++++++++++++++++++--
> arch/x86/kvm/lapic.h           |  1 +
> arch/x86/kvm/svm/avic.c        |  4 +++
> 4 files changed, 83 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/include/asm/apicdef.h b/arch/x86/include/asm/apicdef.h
>index 4b125e5b3187..ac50919d10be 100644
>--- a/arch/x86/include/asm/apicdef.h
>+++ b/arch/x86/include/asm/apicdef.h
>@@ -139,6 +139,20 @@
> #define		APIC_EILVT_MSG_EXT	0x7
> #define		APIC_EILVT_MASKED	(1 << 16)
> 
>+/*
>+ * Initialize extended APIC registers to the default value when guest is started
>+ * and EXTAPIC feature is enabled on the guest.
>+ *
>+ * APIC_EFEAT is a read only Extended APIC feature register, whose default value
>+ * is 0x00040007.

bits 0/1/2 indicate other features. If KVM sets them to 1s, KVM needs to
enumerate the corresponding features.

>+ *
>+ * APIC_ECTRL is a read-write Extended APIC control register, whose default value
>+ * is 0x0.
>+ */
>+
>+#define		APIC_EFEAT_DEFAULT	0x00040007
>+#define		APIC_ECTRL_DEFAULT	0x0
>+
> #define APIC_BASE (fix_to_virt(FIX_APIC_BASE))
> #define APIC_BASE_MSR		0x800
> #define APIC_X2APIC_ID_MSR	0x802
>diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>index 7c1bd8594f1b..88985c481fe8 100644
>--- a/arch/x86/kvm/lapic.c
>+++ b/arch/x86/kvm/lapic.c
>@@ -1599,9 +1599,13 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
> }
> 
> #define APIC_REG_MASK(reg)	(1ull << ((reg) >> 4))
>+#define APIC_REG_EXT_MASK(reg)	(1ull << (((reg) >> 4) - 0x40))
> #define APIC_REGS_MASK(first, count) \
> 	(APIC_REG_MASK(first) * ((1ull << (count)) - 1))
> 
>+#define APIC_LAST_REG_OFFSET		0x3f0
>+#define APIC_EXT_LAST_REG_OFFSET	0x530
>+
> u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic)
> {
> 	/* Leave bits '0' for reserved and write-only registers. */
>@@ -1643,6 +1647,8 @@ EXPORT_SYMBOL_GPL(kvm_lapic_readable_reg_mask);
> static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> 			      void *data)
> {
>+	u64 valid_reg_ext_mask = 0;
>+	unsigned int last_reg = APIC_LAST_REG_OFFSET;
> 	unsigned char alignment = offset & 0xf;
> 	u32 result;
> 
>@@ -1652,13 +1658,44 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> 	 */
> 	WARN_ON_ONCE(apic_x2apic_mode(apic) && offset == APIC_ICR);
> 
>+	/*
>+	 * The local interrupts are extended to include LVT registers to allow
>+	 * additional interrupt sources when the EXTAPIC feature bit is enabled.
>+	 * The Extended Interrupt LVT registers are located at APIC offsets 400-530h.
>+	 */
>+	if (guest_cpuid_has(apic->vcpu, X86_FEATURE_EXTAPIC)) {
>+		valid_reg_ext_mask =
>+			APIC_REG_EXT_MASK(APIC_EFEAT) |
>+			APIC_REG_EXT_MASK(APIC_ECTRL) |
>+			APIC_REG_EXT_MASK(APIC_EILVTn(0)) |
>+			APIC_REG_EXT_MASK(APIC_EILVTn(1)) |
>+			APIC_REG_EXT_MASK(APIC_EILVTn(2)) |
>+			APIC_REG_EXT_MASK(APIC_EILVTn(3));
>+		last_reg = APIC_EXT_LAST_REG_OFFSET;
>+	}
>+
> 	if (alignment + len > 4)
> 		return 1;
> 
>-	if (offset > 0x3f0 ||
>-	    !(kvm_lapic_readable_reg_mask(apic) & APIC_REG_MASK(offset)))
>+	if (offset > last_reg)
> 		return 1;
> 
>+	switch (offset) {
>+	/*
>+	 * Section 16.3.2 in the AMD Programmer's Manual Volume 2 states:
>+	 * "APIC registers are aligned to 16-byte offsets and must be accessed
>+	 * using naturally-aligned DWORD size read and writes."
>+	 */
>+	case KVM_APIC_REG_SIZE ... KVM_APIC_EXT_REG_SIZE - 16:
>+		if (!(valid_reg_ext_mask & APIC_REG_EXT_MASK(offset)))
>+			return 1;
>+		break;
>+	default:
>+		if (!(kvm_lapic_readable_reg_mask(apic) & APIC_REG_MASK(offset)))
>+			return 1;
>+
>+	}
>+
> 	result = __apic_read(apic, offset & ~0xf);
> 
> 	trace_kvm_apic_read(offset, result);
>@@ -2386,6 +2423,12 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
> 		else
> 			kvm_apic_send_ipi(apic, APIC_DEST_SELF | val, 0);
> 		break;
>+	case APIC_EILVTn(0):
>+	case APIC_EILVTn(1):
>+	case APIC_EILVTn(2):
>+	case APIC_EILVTn(3):
>+		kvm_lapic_set_reg(apic, reg, val);
>+		break;

APIC_ECTRL is writable, so, I think it should be handled here.

> 	default:
> 		ret = 1;
> 		break;
>@@ -2664,6 +2707,25 @@ void kvm_inhibit_apic_access_page(struct kvm_vcpu *vcpu)
> 	kvm_vcpu_srcu_read_lock(vcpu);
> }
> 
>+/*
>+ * Initialize extended APIC registers to the default value when guest is
>+ * started. The extended APIC registers should only be initialized when the
>+ * EXTAPIC feature is enabled on the guest.
>+ */
>+void kvm_apic_init_eilvt_regs(struct kvm_vcpu *vcpu)
>+{
>+	struct kvm_lapic *apic = vcpu->arch.apic;
>+	int i;
>+
>+	if (guest_cpuid_has(vcpu, X86_FEATURE_EXTAPIC)) {

Do you need to check hardware support here?

>+		kvm_lapic_set_reg(apic, APIC_EFEAT, APIC_EFEAT_DEFAULT);
>+		kvm_lapic_set_reg(apic, APIC_ECTRL, APIC_ECTRL_DEFAULT);
>+		for (i = 0; i < APIC_EILVT_NR_MAX; i++)
>+			kvm_lapic_set_reg(apic, APIC_EILVTn(i), APIC_EILVT_MASKED);
>+	}
>+}
>+EXPORT_SYMBOL_GPL(kvm_apic_init_eilvt_regs);

looks Extended APIC space is generic to x86. Can you just call this function
from kvm_vcpu_after_set_cpuid()?  then there is no need to expose this function.

>+
> void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)

The extended APIC space should be re-initialized on reset. Right?

> {
> 	struct kvm_lapic *apic = vcpu->arch.apic;
>diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
>index ad6c48938733..b0c7393cd6af 100644
>--- a/arch/x86/kvm/lapic.h
>+++ b/arch/x86/kvm/lapic.h
>@@ -93,6 +93,7 @@ int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu);
> int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu);
> int kvm_apic_accept_events(struct kvm_vcpu *vcpu);
> void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event);
>+void kvm_apic_init_eilvt_regs(struct kvm_vcpu *vcpu);
> u64 kvm_lapic_get_cr8(struct kvm_vcpu *vcpu);
> void kvm_lapic_set_tpr(struct kvm_vcpu *vcpu, unsigned long cr8);
> void kvm_lapic_set_eoi(struct kvm_vcpu *vcpu);
>diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>index cfc8ab773025..081075674b1d 100644
>--- a/arch/x86/kvm/svm/avic.c
>+++ b/arch/x86/kvm/svm/avic.c
>@@ -679,6 +679,10 @@ static bool is_avic_unaccelerated_access_trap(u32 offset)
> 	case APIC_LVTERR:
> 	case APIC_TMICT:
> 	case APIC_TDCR:
>+	case APIC_EILVTn(0):
>+	case APIC_EILVTn(1):
>+	case APIC_EILVTn(2):
>+	case APIC_EILVTn(3):
> 		ret = true;
> 		break;
> 	default:
>-- 
>2.34.1
>



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux