* [PATCH 0/4] x86: replace plain numbers
@ 2015-01-22 13:53 Jan Beulich
2015-01-22 13:57 ` [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions() Jan Beulich
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 13:53 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
1: HVM: replace plain number in hvm_combine_hw_exceptions()
2: HVM: replace plain numbers
3: traps: replace plain numbers
4: VMX: replace plain numbers
Signed-off-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions()
2015-01-22 13:53 [PATCH 0/4] x86: replace plain numbers Jan Beulich
@ 2015-01-22 13:57 ` Jan Beulich
2015-01-22 14:12 ` Andrew Cooper
2015-01-22 14:42 ` Tim Deegan
2015-01-22 13:57 ` [PATCH 2/4] x86/HVM: replace plain numbers Jan Beulich
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 13:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]
While doing so also take care of #VE here (even if we don't make use of
it yet).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
*/
uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
{
+ const unsigned int contributory_exceptions =
+ (1 << TRAP_divide_error) |
+ (1 << TRAP_invalid_tss) |
+ (1 << TRAP_no_segment) |
+ (1 << TRAP_stack_error) |
+ (1 << TRAP_gp_fault);
+ const unsigned int page_faults =
+ (1 << TRAP_page_fault) |
+ (1 << TRAP_virtualisation);
+
/* Exception during double-fault delivery always causes a triple fault. */
if ( vec1 == TRAP_double_fault )
{
@@ -213,11 +223,12 @@ uint8_t hvm_combine_hw_exceptions(uint8_
}
/* Exception during page-fault delivery always causes a double fault. */
- if ( vec1 == TRAP_page_fault )
+ if ( (1u << vec1) & page_faults )
return TRAP_double_fault;
/* Discard the first exception if it's benign or if we now have a #PF. */
- if ( !((1u << vec1) & 0x7c01u) || (vec2 == TRAP_page_fault) )
+ if ( !((1u << vec1) & contributory_exceptions) ||
+ ((1u << vec2) & page_faults) )
return vec2;
/* Cannot combine the exceptions: double fault. */
[-- Attachment #2: x86-HVM-combine-exceptions.patch --]
[-- Type: text/plain, Size: 1465 bytes --]
x86/HVM: replace plain number in hvm_combine_hw_exceptions()
While doing so also take care of #VE here (even if we don't make use of
it yet).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
*/
uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
{
+ const unsigned int contributory_exceptions =
+ (1 << TRAP_divide_error) |
+ (1 << TRAP_invalid_tss) |
+ (1 << TRAP_no_segment) |
+ (1 << TRAP_stack_error) |
+ (1 << TRAP_gp_fault);
+ const unsigned int page_faults =
+ (1 << TRAP_page_fault) |
+ (1 << TRAP_virtualisation);
+
/* Exception during double-fault delivery always causes a triple fault. */
if ( vec1 == TRAP_double_fault )
{
@@ -213,11 +223,12 @@ uint8_t hvm_combine_hw_exceptions(uint8_
}
/* Exception during page-fault delivery always causes a double fault. */
- if ( vec1 == TRAP_page_fault )
+ if ( (1u << vec1) & page_faults )
return TRAP_double_fault;
/* Discard the first exception if it's benign or if we now have a #PF. */
- if ( !((1u << vec1) & 0x7c01u) || (vec2 == TRAP_page_fault) )
+ if ( !((1u << vec1) & contributory_exceptions) ||
+ ((1u << vec2) & page_faults) )
return vec2;
/* Cannot combine the exceptions: double fault. */
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/4] x86/HVM: replace plain numbers
2015-01-22 13:53 [PATCH 0/4] x86: replace plain numbers Jan Beulich
2015-01-22 13:57 ` [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions() Jan Beulich
@ 2015-01-22 13:57 ` Jan Beulich
2015-01-22 14:41 ` Andrew Cooper
2015-01-22 14:00 ` [PATCH 3/4] x86/traps: " Jan Beulich
2015-01-22 14:01 ` [PATCH 4/4] VMX: " Jan Beulich
3 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 13:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 2973 bytes --]
... making the code better document itself. No functional change
intended.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
switch ( vioapic->ioregsel )
{
case VIOAPIC_REG_VERSION:
- result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
- | (VIOAPIC_VERSION_ID & 0xff));
+ result = ((union IO_APIC_reg_01){
+ .bits = { .version = VIOAPIC_VERSION_ID,
+ .entries = VIOAPIC_NUM_PINS - 1 }
+ }).raw;
break;
case VIOAPIC_REG_APIC_ID:
+ /*
+ * Using union IO_APIC_reg_02 for the ID register too, as
+ * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
+ */
case VIOAPIC_REG_ARB_ID:
- result = ((vioapic->id & 0xf) << 24);
+ result = ((union IO_APIC_reg_02){
+ .bits = { .arbitration = vioapic->id }
+ }).raw;
break;
default:
{
- uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
+ uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
uint64_t redir_content;
if ( redir_index >= VIOAPIC_NUM_PINS )
@@ -173,7 +181,11 @@ static void vioapic_write_indirect(
break;
case VIOAPIC_REG_APIC_ID:
- vioapic->id = (val >> 24) & 0xf;
+ /*
+ * Using union IO_APIC_reg_02 for the ID register, as for some
+ * reason union IO_APIC_reg_00's ID field is 8 bits wide.
+ */
+ vioapic->id = ((union IO_APIC_reg_02){ .raw = val }).bits.arbitration;
break;
case VIOAPIC_REG_ARB_ID:
@@ -181,7 +193,7 @@ static void vioapic_write_indirect(
default:
{
- uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
+ uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -835,7 +835,7 @@ static int vlapic_write(struct vcpu *v,
unsigned int offset = address - vlapic_base_address(vlapic);
int rc = X86EMUL_OKAY;
- if ( offset != 0xb0 )
+ if ( offset != APIC_EOI )
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
"offset %#x with length %#lx, and value is %#lx",
offset, len, val);
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -47,6 +47,7 @@
#define VIOAPIC_REG_APIC_ID 0x00 /* x86 IOAPIC only */
#define VIOAPIC_REG_VERSION 0x01
#define VIOAPIC_REG_ARB_ID 0x02 /* x86 IOAPIC only */
+#define VIOAPIC_REG_RTE0 0x10
struct hvm_vioapic {
struct hvm_hw_vioapic hvm_hw_vioapic;
[-- Attachment #2: x86-HVM-literals.patch --]
[-- Type: text/plain, Size: 3001 bytes --]
x86/HVM: replace plain numbers
... making the code better document itself. No functional change
intended.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
switch ( vioapic->ioregsel )
{
case VIOAPIC_REG_VERSION:
- result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
- | (VIOAPIC_VERSION_ID & 0xff));
+ result = ((union IO_APIC_reg_01){
+ .bits = { .version = VIOAPIC_VERSION_ID,
+ .entries = VIOAPIC_NUM_PINS - 1 }
+ }).raw;
break;
case VIOAPIC_REG_APIC_ID:
+ /*
+ * Using union IO_APIC_reg_02 for the ID register too, as
+ * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
+ */
case VIOAPIC_REG_ARB_ID:
- result = ((vioapic->id & 0xf) << 24);
+ result = ((union IO_APIC_reg_02){
+ .bits = { .arbitration = vioapic->id }
+ }).raw;
break;
default:
{
- uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
+ uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
uint64_t redir_content;
if ( redir_index >= VIOAPIC_NUM_PINS )
@@ -173,7 +181,11 @@ static void vioapic_write_indirect(
break;
case VIOAPIC_REG_APIC_ID:
- vioapic->id = (val >> 24) & 0xf;
+ /*
+ * Using union IO_APIC_reg_02 for the ID register, as for some
+ * reason union IO_APIC_reg_00's ID field is 8 bits wide.
+ */
+ vioapic->id = ((union IO_APIC_reg_02){ .raw = val }).bits.arbitration;
break;
case VIOAPIC_REG_ARB_ID:
@@ -181,7 +193,7 @@ static void vioapic_write_indirect(
default:
{
- uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
+ uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -835,7 +835,7 @@ static int vlapic_write(struct vcpu *v,
unsigned int offset = address - vlapic_base_address(vlapic);
int rc = X86EMUL_OKAY;
- if ( offset != 0xb0 )
+ if ( offset != APIC_EOI )
HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
"offset %#x with length %#lx, and value is %#lx",
offset, len, val);
--- a/xen/include/asm-x86/hvm/vioapic.h
+++ b/xen/include/asm-x86/hvm/vioapic.h
@@ -47,6 +47,7 @@
#define VIOAPIC_REG_APIC_ID 0x00 /* x86 IOAPIC only */
#define VIOAPIC_REG_VERSION 0x01
#define VIOAPIC_REG_ARB_ID 0x02 /* x86 IOAPIC only */
+#define VIOAPIC_REG_RTE0 0x10
struct hvm_vioapic {
struct hvm_hw_vioapic hvm_hw_vioapic;
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/4] x86/traps: replace plain numbers
2015-01-22 13:53 [PATCH 0/4] x86: replace plain numbers Jan Beulich
2015-01-22 13:57 ` [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions() Jan Beulich
2015-01-22 13:57 ` [PATCH 2/4] x86/HVM: replace plain numbers Jan Beulich
@ 2015-01-22 14:00 ` Jan Beulich
2015-01-22 14:45 ` Andrew Cooper
2015-01-22 14:01 ` [PATCH 4/4] VMX: " Jan Beulich
3 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 14:00 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]
... making the code better document itself. No functional change
intended.
Note that for now (as we don't support RTM yet) DR_STATUS_RESERVED_ONE
and its users don't take DR_NOT_RTM into consideration yet.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -531,9 +531,9 @@ static void instruction_done(
regs->eflags &= ~X86_EFLAGS_RF;
if ( bpmatch || (regs->eflags & X86_EFLAGS_TF) )
{
- current->arch.debugreg[6] |= bpmatch | 0xffff0ff0;
+ current->arch.debugreg[6] |= bpmatch | DR_STATUS_RESERVED_ONE;
if ( regs->eflags & X86_EFLAGS_TF )
- current->arch.debugreg[6] |= 0x4000;
+ current->arch.debugreg[6] |= DR_STEP;
do_guest_trap(TRAP_debug, regs, 0);
}
}
@@ -3872,8 +3872,8 @@ long set_debugreg(struct vcpu *v, unsign
* DR6: Bits 4-11,16-31 reserved (set to 1).
* Bit 12 reserved (set to 0).
*/
- value &= 0xffffefff; /* reserved bits => 0 */
- value |= 0xffff0ff0; /* reserved bits => 1 */
+ value &= ~DR_STATUS_RESERVED_ZERO; /* reserved bits => 0 */
+ value |= DR_STATUS_RESERVED_ONE; /* reserved bits => 1 */
if ( v == curr )
write_debugreg(6, value);
break;
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -21,6 +21,8 @@
#define DR_STEP (0x4000) /* single-step */
#define DR_SWITCH (0x8000) /* task switch */
#define DR_NOT_RTM (0x10000) /* clear: #BP inside RTM region */
+#define DR_STATUS_RESERVED_ZERO (~0xffffeffful) /* Reserved, read as zero */
+#define DR_STATUS_RESERVED_ONE 0xffff0ff0ul /* Reserved, read as one */
/* Now define a bunch of things for manipulating the control register.
The top two bytes of the control register consist of 4 fields of 4
[-- Attachment #2: x86-traps-literals.patch --]
[-- Type: text/plain, Size: 1959 bytes --]
x86/traps: replace plain numbers
... making the code better document itself. No functional change
intended.
Note that for now (as we don't support RTM yet) DR_STATUS_RESERVED_ONE
and its users don't take DR_NOT_RTM into consideration yet.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -531,9 +531,9 @@ static void instruction_done(
regs->eflags &= ~X86_EFLAGS_RF;
if ( bpmatch || (regs->eflags & X86_EFLAGS_TF) )
{
- current->arch.debugreg[6] |= bpmatch | 0xffff0ff0;
+ current->arch.debugreg[6] |= bpmatch | DR_STATUS_RESERVED_ONE;
if ( regs->eflags & X86_EFLAGS_TF )
- current->arch.debugreg[6] |= 0x4000;
+ current->arch.debugreg[6] |= DR_STEP;
do_guest_trap(TRAP_debug, regs, 0);
}
}
@@ -3872,8 +3872,8 @@ long set_debugreg(struct vcpu *v, unsign
* DR6: Bits 4-11,16-31 reserved (set to 1).
* Bit 12 reserved (set to 0).
*/
- value &= 0xffffefff; /* reserved bits => 0 */
- value |= 0xffff0ff0; /* reserved bits => 1 */
+ value &= ~DR_STATUS_RESERVED_ZERO; /* reserved bits => 0 */
+ value |= DR_STATUS_RESERVED_ONE; /* reserved bits => 1 */
if ( v == curr )
write_debugreg(6, value);
break;
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -21,6 +21,8 @@
#define DR_STEP (0x4000) /* single-step */
#define DR_SWITCH (0x8000) /* task switch */
#define DR_NOT_RTM (0x10000) /* clear: #BP inside RTM region */
+#define DR_STATUS_RESERVED_ZERO (~0xffffeffful) /* Reserved, read as zero */
+#define DR_STATUS_RESERVED_ONE 0xffff0ff0ul /* Reserved, read as one */
/* Now define a bunch of things for manipulating the control register.
The top two bytes of the control register consist of 4 fields of 4
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/4] VMX: replace plain numbers
2015-01-22 13:53 [PATCH 0/4] x86: replace plain numbers Jan Beulich
` (2 preceding siblings ...)
2015-01-22 14:00 ` [PATCH 3/4] x86/traps: " Jan Beulich
@ 2015-01-22 14:01 ` Jan Beulich
2015-01-22 15:21 ` Andrew Cooper
2015-01-23 6:25 ` Tian, Kevin
3 siblings, 2 replies; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 14:01 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, Eddie Dong, Jun Nakajima
[-- Attachment #1: Type: text/plain, Size: 8635 bytes --]
... making the code better document itself. No functional change
intended.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1406,7 +1406,9 @@ static void __vmx_inject_exception(int t
* VM entry]", PRM Vol. 3, 22.6.1 (Interruptibility State).
*/
- intr_fields = (INTR_INFO_VALID_MASK | (type<<8) | trap);
+ intr_fields = INTR_INFO_VALID_MASK |
+ MASK_INSR(type, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(trap, INTR_INFO_VECTOR_MASK);
if ( error_code != HVM_DELIVER_NO_ERROR_CODE ) {
__vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
@@ -1430,7 +1432,9 @@ void vmx_inject_extint(int trap, uint8_t
PIN_BASED_VM_EXEC_CONTROL);
if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
nvmx_enqueue_n2_exceptions (v,
- INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) | trap,
+ INTR_INFO_VALID_MASK |
+ MASK_INSR(X86_EVENTTYPE_EXT_INTR, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(trap, INTR_INFO_VECTOR_MASK),
HVM_DELIVER_NO_ERROR_CODE, source);
return;
}
@@ -1449,7 +1453,9 @@ void vmx_inject_nmi(void)
PIN_BASED_VM_EXEC_CONTROL);
if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
nvmx_enqueue_n2_exceptions (v,
- INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) | TRAP_nmi,
+ INTR_INFO_VALID_MASK |
+ MASK_INSR(X86_EVENTTYPE_NMI, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(TRAP_nmi, INTR_INFO_VECTOR_MASK),
HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
return;
}
@@ -1487,7 +1493,7 @@ static void vmx_inject_trap(struct hvm_t
if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
{
__restore_debug_registers(curr);
- write_debugreg(6, read_debugreg(6) | 0x4000);
+ write_debugreg(6, read_debugreg(6) | DR_STEP);
}
if ( cpu_has_monitor_trap_flag )
break;
@@ -1502,7 +1508,8 @@ static void vmx_inject_trap(struct hvm_t
}
if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
- (((intr_info >> 8) & 7) == X86_EVENTTYPE_HW_EXCEPTION) )
+ (MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) ==
+ X86_EVENTTYPE_HW_EXCEPTION) )
{
_trap.vector = hvm_combine_hw_exceptions(
(uint8_t)intr_info, _trap.vector);
@@ -1517,7 +1524,9 @@ static void vmx_inject_trap(struct hvm_t
nvmx_intercepts_exception(curr, _trap.vector, _trap.error_code) )
{
nvmx_enqueue_n2_exceptions (curr,
- INTR_INFO_VALID_MASK | (_trap.type<<8) | _trap.vector,
+ INTR_INFO_VALID_MASK |
+ MASK_INSR(_trap.type, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(_trap.vector, INTR_INFO_VECTOR_MASK),
_trap.error_code, hvm_intsrc_none);
return;
}
@@ -1976,8 +1985,11 @@ static int vmx_cr_access(unsigned long e
}
case VMX_CONTROL_REG_ACCESS_TYPE_LMSW: {
unsigned long value = curr->arch.hvm_vcpu.guest_cr[0];
- /* LMSW can: (1) set bits 0-3; (2) clear bits 1-3. */
- value = (value & ~0xe) | ((exit_qualification >> 16) & 0xf);
+
+ /* LMSW can (1) set PE; (2) set or clear MP, EM, and TS. */
+ value = (value & ~(X86_CR0_MP|X86_CR0_EM|X86_CR0_TS)) |
+ (VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
+ (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
HVMTRACE_LONG_1D(LMSW, value);
return hvm_set_cr0(value);
}
@@ -2803,7 +2815,7 @@ void vmx_vmexit_handler(struct cpu_user_
*/
__vmread(EXIT_QUALIFICATION, &exit_qualification);
HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
- write_debugreg(6, exit_qualification | 0xffff0ff0);
+ write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
goto exit_and_crash;
domain_pause_for_debugger();
@@ -2872,8 +2884,8 @@ void vmx_vmexit_handler(struct cpu_user_
hvm_inject_page_fault(regs->error_code, exit_qualification);
break;
case TRAP_nmi:
- if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) !=
- (X86_EVENTTYPE_NMI << 8) )
+ if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
+ X86_EVENTTYPE_NMI )
goto exit_and_crash;
HVMTRACE_0D(NMI);
/* Already handled above. */
@@ -2924,7 +2936,8 @@ void vmx_vmexit_handler(struct cpu_user_
* - TSW is a vectored event due to a SW exception or SW interrupt.
*/
inst_len = ((source != 3) || /* CALL, IRET, or JMP? */
- (idtv_info & (1u<<10))) /* IntrType > 3? */
+ (MASK_EXTR(idtv_info, INTR_INFO_INTR_TYPE_MASK)
+ > 3)) /* IntrType > 3? */
? get_instruction_length() /* Safe: SDM 3B 23.2.4 */ : 0;
if ( (source == 3) && (idtv_info & INTR_INFO_DELIVER_CODE_MASK) )
__vmread(IDT_VECTORING_ERROR_CODE, &ecode);
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1272,7 +1272,7 @@ static void sync_exception_state(struct
if ( !(nvmx->intr.intr_info & INTR_INFO_VALID_MASK) )
return;
- switch ( (nvmx->intr.intr_info & INTR_INFO_INTR_TYPE_MASK) >> 8 )
+ switch ( MASK_EXTR(nvmx->intr.intr_info, INTR_INFO_INTR_TYPE_MASK) )
{
case X86_EVENTTYPE_EXT_INTR:
/* rename exit_reason to EXTERNAL_INTERRUPT */
@@ -1327,10 +1327,10 @@ static void nvmx_update_apicv(struct vcp
ppr = vlapic_set_ppr(vlapic);
WARN_ON((ppr & 0xf0) != (vector & 0xf0));
- status = vector << 8;
+ status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
rvi = vlapic_has_pending_irq(v);
if ( rvi != -1 )
- status |= rvi & 0xff;
+ status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
__vmwrite(GUEST_INTR_STATUS, status);
}
@@ -2161,7 +2161,8 @@ int nvmx_n2_vmexit_handler(struct cpu_us
case EXIT_REASON_EXCEPTION_NMI:
{
unsigned long intr_info;
- u32 valid_mask = (X86_EVENTTYPE_HW_EXCEPTION << 8) |
+ u32 valid_mask = MASK_INSR(X86_EVENTTYPE_HW_EXCEPTION,
+ INTR_INFO_INTR_TYPE_MASK) |
INTR_INFO_VALID_MASK;
u64 exec_bitmap;
int vector;
@@ -2350,8 +2351,8 @@ int nvmx_n2_vmexit_handler(struct cpu_us
u32 mask = 0;
__vmread(EXIT_QUALIFICATION, &exit_qualification);
- cr = exit_qualification & 0xf;
- write = (exit_qualification >> 4) & 3;
+ cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
+ write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
/* also according to guest exec_control */
ctrl = __n2_exec_control(v);
@@ -2443,8 +2444,9 @@ int nvmx_n2_vmexit_handler(struct cpu_us
u64 cr0_gh_mask = __get_vvmcs(nvcpu->nv_vvmcx, CR0_GUEST_HOST_MASK);
__vmread(CR0_READ_SHADOW, &old_val);
- old_val &= 0xf;
- val = (exit_qualification >> 16) & 0xf;
+ old_val &= X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS;
+ val = VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
+ (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS);
changed_bits = old_val ^ val;
if ( changed_bits & cr0_gh_mask )
nvcpu->nv_vmexit_pending = 1;
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -207,8 +207,10 @@ static inline unsigned long pi_get_pir(s
# define VMX_CONTROL_REG_ACCESS_TYPE_MOV_FROM_CR 1
# define VMX_CONTROL_REG_ACCESS_TYPE_CLTS 2
# define VMX_CONTROL_REG_ACCESS_TYPE_LMSW 3
- /* 10:8 - general purpose register operand */
+ /* 11:8 - general purpose register operand */
#define VMX_CONTROL_REG_ACCESS_GPR(eq) (((eq) >> 8) & 0xf)
+ /* 31:16 - LMSW source data */
+#define VMX_CONTROL_REG_ACCESS_DATA(eq) ((uint32_t)(eq) >> 16)
/*
* Access Rights
[-- Attachment #2: VMX-literals.patch --]
[-- Type: text/plain, Size: 8661 bytes --]
VMX: replace plain numbers
... making the code better document itself. No functional change
intended.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1406,7 +1406,9 @@ static void __vmx_inject_exception(int t
* VM entry]", PRM Vol. 3, 22.6.1 (Interruptibility State).
*/
- intr_fields = (INTR_INFO_VALID_MASK | (type<<8) | trap);
+ intr_fields = INTR_INFO_VALID_MASK |
+ MASK_INSR(type, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(trap, INTR_INFO_VECTOR_MASK);
if ( error_code != HVM_DELIVER_NO_ERROR_CODE ) {
__vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
@@ -1430,7 +1432,9 @@ void vmx_inject_extint(int trap, uint8_t
PIN_BASED_VM_EXEC_CONTROL);
if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
nvmx_enqueue_n2_exceptions (v,
- INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) | trap,
+ INTR_INFO_VALID_MASK |
+ MASK_INSR(X86_EVENTTYPE_EXT_INTR, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(trap, INTR_INFO_VECTOR_MASK),
HVM_DELIVER_NO_ERROR_CODE, source);
return;
}
@@ -1449,7 +1453,9 @@ void vmx_inject_nmi(void)
PIN_BASED_VM_EXEC_CONTROL);
if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
nvmx_enqueue_n2_exceptions (v,
- INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) | TRAP_nmi,
+ INTR_INFO_VALID_MASK |
+ MASK_INSR(X86_EVENTTYPE_NMI, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(TRAP_nmi, INTR_INFO_VECTOR_MASK),
HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
return;
}
@@ -1487,7 +1493,7 @@ static void vmx_inject_trap(struct hvm_t
if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
{
__restore_debug_registers(curr);
- write_debugreg(6, read_debugreg(6) | 0x4000);
+ write_debugreg(6, read_debugreg(6) | DR_STEP);
}
if ( cpu_has_monitor_trap_flag )
break;
@@ -1502,7 +1508,8 @@ static void vmx_inject_trap(struct hvm_t
}
if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
- (((intr_info >> 8) & 7) == X86_EVENTTYPE_HW_EXCEPTION) )
+ (MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) ==
+ X86_EVENTTYPE_HW_EXCEPTION) )
{
_trap.vector = hvm_combine_hw_exceptions(
(uint8_t)intr_info, _trap.vector);
@@ -1517,7 +1524,9 @@ static void vmx_inject_trap(struct hvm_t
nvmx_intercepts_exception(curr, _trap.vector, _trap.error_code) )
{
nvmx_enqueue_n2_exceptions (curr,
- INTR_INFO_VALID_MASK | (_trap.type<<8) | _trap.vector,
+ INTR_INFO_VALID_MASK |
+ MASK_INSR(_trap.type, INTR_INFO_INTR_TYPE_MASK) |
+ MASK_INSR(_trap.vector, INTR_INFO_VECTOR_MASK),
_trap.error_code, hvm_intsrc_none);
return;
}
@@ -1976,8 +1985,11 @@ static int vmx_cr_access(unsigned long e
}
case VMX_CONTROL_REG_ACCESS_TYPE_LMSW: {
unsigned long value = curr->arch.hvm_vcpu.guest_cr[0];
- /* LMSW can: (1) set bits 0-3; (2) clear bits 1-3. */
- value = (value & ~0xe) | ((exit_qualification >> 16) & 0xf);
+
+ /* LMSW can (1) set PE; (2) set or clear MP, EM, and TS. */
+ value = (value & ~(X86_CR0_MP|X86_CR0_EM|X86_CR0_TS)) |
+ (VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
+ (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
HVMTRACE_LONG_1D(LMSW, value);
return hvm_set_cr0(value);
}
@@ -2803,7 +2815,7 @@ void vmx_vmexit_handler(struct cpu_user_
*/
__vmread(EXIT_QUALIFICATION, &exit_qualification);
HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
- write_debugreg(6, exit_qualification | 0xffff0ff0);
+ write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
goto exit_and_crash;
domain_pause_for_debugger();
@@ -2872,8 +2884,8 @@ void vmx_vmexit_handler(struct cpu_user_
hvm_inject_page_fault(regs->error_code, exit_qualification);
break;
case TRAP_nmi:
- if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) !=
- (X86_EVENTTYPE_NMI << 8) )
+ if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
+ X86_EVENTTYPE_NMI )
goto exit_and_crash;
HVMTRACE_0D(NMI);
/* Already handled above. */
@@ -2924,7 +2936,8 @@ void vmx_vmexit_handler(struct cpu_user_
* - TSW is a vectored event due to a SW exception or SW interrupt.
*/
inst_len = ((source != 3) || /* CALL, IRET, or JMP? */
- (idtv_info & (1u<<10))) /* IntrType > 3? */
+ (MASK_EXTR(idtv_info, INTR_INFO_INTR_TYPE_MASK)
+ > 3)) /* IntrType > 3? */
? get_instruction_length() /* Safe: SDM 3B 23.2.4 */ : 0;
if ( (source == 3) && (idtv_info & INTR_INFO_DELIVER_CODE_MASK) )
__vmread(IDT_VECTORING_ERROR_CODE, &ecode);
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1272,7 +1272,7 @@ static void sync_exception_state(struct
if ( !(nvmx->intr.intr_info & INTR_INFO_VALID_MASK) )
return;
- switch ( (nvmx->intr.intr_info & INTR_INFO_INTR_TYPE_MASK) >> 8 )
+ switch ( MASK_EXTR(nvmx->intr.intr_info, INTR_INFO_INTR_TYPE_MASK) )
{
case X86_EVENTTYPE_EXT_INTR:
/* rename exit_reason to EXTERNAL_INTERRUPT */
@@ -1327,10 +1327,10 @@ static void nvmx_update_apicv(struct vcp
ppr = vlapic_set_ppr(vlapic);
WARN_ON((ppr & 0xf0) != (vector & 0xf0));
- status = vector << 8;
+ status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
rvi = vlapic_has_pending_irq(v);
if ( rvi != -1 )
- status |= rvi & 0xff;
+ status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
__vmwrite(GUEST_INTR_STATUS, status);
}
@@ -2161,7 +2161,8 @@ int nvmx_n2_vmexit_handler(struct cpu_us
case EXIT_REASON_EXCEPTION_NMI:
{
unsigned long intr_info;
- u32 valid_mask = (X86_EVENTTYPE_HW_EXCEPTION << 8) |
+ u32 valid_mask = MASK_INSR(X86_EVENTTYPE_HW_EXCEPTION,
+ INTR_INFO_INTR_TYPE_MASK) |
INTR_INFO_VALID_MASK;
u64 exec_bitmap;
int vector;
@@ -2350,8 +2351,8 @@ int nvmx_n2_vmexit_handler(struct cpu_us
u32 mask = 0;
__vmread(EXIT_QUALIFICATION, &exit_qualification);
- cr = exit_qualification & 0xf;
- write = (exit_qualification >> 4) & 3;
+ cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
+ write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
/* also according to guest exec_control */
ctrl = __n2_exec_control(v);
@@ -2443,8 +2444,9 @@ int nvmx_n2_vmexit_handler(struct cpu_us
u64 cr0_gh_mask = __get_vvmcs(nvcpu->nv_vvmcx, CR0_GUEST_HOST_MASK);
__vmread(CR0_READ_SHADOW, &old_val);
- old_val &= 0xf;
- val = (exit_qualification >> 16) & 0xf;
+ old_val &= X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS;
+ val = VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
+ (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS);
changed_bits = old_val ^ val;
if ( changed_bits & cr0_gh_mask )
nvcpu->nv_vmexit_pending = 1;
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -207,8 +207,10 @@ static inline unsigned long pi_get_pir(s
# define VMX_CONTROL_REG_ACCESS_TYPE_MOV_FROM_CR 1
# define VMX_CONTROL_REG_ACCESS_TYPE_CLTS 2
# define VMX_CONTROL_REG_ACCESS_TYPE_LMSW 3
- /* 10:8 - general purpose register operand */
+ /* 11:8 - general purpose register operand */
#define VMX_CONTROL_REG_ACCESS_GPR(eq) (((eq) >> 8) & 0xf)
+ /* 31:16 - LMSW source data */
+#define VMX_CONTROL_REG_ACCESS_DATA(eq) ((uint32_t)(eq) >> 16)
/*
* Access Rights
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions()
2015-01-22 13:57 ` [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions() Jan Beulich
@ 2015-01-22 14:12 ` Andrew Cooper
2015-01-22 14:36 ` Jan Beulich
2015-01-22 14:42 ` Tim Deegan
1 sibling, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2015-01-22 14:12 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 22/01/15 13:57, Jan Beulich wrote:
> While doing so also take care of #VE here (even if we don't make use of
> it yet).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
> */
> uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
> {
> + const unsigned int contributory_exceptions =
> + (1 << TRAP_divide_error) |
> + (1 << TRAP_invalid_tss) |
> + (1 << TRAP_no_segment) |
> + (1 << TRAP_stack_error) |
> + (1 << TRAP_gp_fault);
> + const unsigned int page_faults =
> + (1 << TRAP_page_fault) |
> + (1 << TRAP_virtualisation);
static as an extra hint?
I frankly hope that any decent compiler would turn these into
instruction immediate data.
Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
(FWIW the 0x7c01 constant was the inspiration for my vlapic patch,
although I felt that constructing the map at compile time was better
than an opaque number.)
> +
> /* Exception during double-fault delivery always causes a triple fault. */
> if ( vec1 == TRAP_double_fault )
> {
> @@ -213,11 +223,12 @@ uint8_t hvm_combine_hw_exceptions(uint8_
> }
>
> /* Exception during page-fault delivery always causes a double fault. */
> - if ( vec1 == TRAP_page_fault )
> + if ( (1u << vec1) & page_faults )
> return TRAP_double_fault;
>
> /* Discard the first exception if it's benign or if we now have a #PF. */
> - if ( !((1u << vec1) & 0x7c01u) || (vec2 == TRAP_page_fault) )
> + if ( !((1u << vec1) & contributory_exceptions) ||
> + ((1u << vec2) & page_faults) )
> return vec2;
>
> /* Cannot combine the exceptions: double fault. */
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions()
2015-01-22 14:12 ` Andrew Cooper
@ 2015-01-22 14:36 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 14:36 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 22.01.15 at 15:12, <andrew.cooper3@citrix.com> wrote:
> On 22/01/15 13:57, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
>> */
>> uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
>> {
>> + const unsigned int contributory_exceptions =
>> + (1 << TRAP_divide_error) |
>> + (1 << TRAP_invalid_tss) |
>> + (1 << TRAP_no_segment) |
>> + (1 << TRAP_stack_error) |
>> + (1 << TRAP_gp_fault);
>> + const unsigned int page_faults =
>> + (1 << TRAP_page_fault) |
>> + (1 << TRAP_virtualisation);
>
> static as an extra hint?
>
> I frankly hope that any decent compiler would turn these into
> instruction immediate data.
I think the static could actually misguide the compiler to in fact
allocate storage. I did verify with gcc 4.9.2 that the compiler
does translate the above to literal numbers.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] x86/HVM: replace plain numbers
2015-01-22 13:57 ` [PATCH 2/4] x86/HVM: replace plain numbers Jan Beulich
@ 2015-01-22 14:41 ` Andrew Cooper
2015-01-22 15:17 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2015-01-22 14:41 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 22/01/15 13:57, Jan Beulich wrote:
> ... making the code better document itself. No functional change
> intended.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
> switch ( vioapic->ioregsel )
> {
> case VIOAPIC_REG_VERSION:
> - result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
> - | (VIOAPIC_VERSION_ID & 0xff));
> + result = ((union IO_APIC_reg_01){
> + .bits = { .version = VIOAPIC_VERSION_ID,
> + .entries = VIOAPIC_NUM_PINS - 1 }
> + }).raw;
> break;
>
> case VIOAPIC_REG_APIC_ID:
> + /*
> + * Using union IO_APIC_reg_02 for the ID register too, as
> + * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
> + */
Having looked into this, Intel has a 4 bit wide ID with the top 4 bits
reserved, while AMD has the top 4 bits as the Extended ID which may be
used if an appropriate northbridge register has been set.
I think it might be better to use IO_APIC_reg_00 here and mask the write
operations, leaving a note about Intel vs AMD and the fact that emulate
an Intel IOAPIC to match the PIIX3 chipset in Qemu.
Modifying IO_APIC_reg_00 is not appropriate as Xens IOAPIC code needs to
deal with AMD systems as well.
~Andrew
> case VIOAPIC_REG_ARB_ID:
> - result = ((vioapic->id & 0xf) << 24);
> + result = ((union IO_APIC_reg_02){
> + .bits = { .arbitration = vioapic->id }
> + }).raw;
> break;
>
> default:
> {
> - uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
> + uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
> uint64_t redir_content;
>
> if ( redir_index >= VIOAPIC_NUM_PINS )
> @@ -173,7 +181,11 @@ static void vioapic_write_indirect(
> break;
>
> case VIOAPIC_REG_APIC_ID:
> - vioapic->id = (val >> 24) & 0xf;
> + /*
> + * Using union IO_APIC_reg_02 for the ID register, as for some
> + * reason union IO_APIC_reg_00's ID field is 8 bits wide.
> + */
> + vioapic->id = ((union IO_APIC_reg_02){ .raw = val }).bits.arbitration;
> break;
>
> case VIOAPIC_REG_ARB_ID:
> @@ -181,7 +193,7 @@ static void vioapic_write_indirect(
>
> default:
> {
> - uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
> + uint32_t redir_index = (vioapic->ioregsel - VIOAPIC_REG_RTE0) >> 1;
>
> HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
> redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -835,7 +835,7 @@ static int vlapic_write(struct vcpu *v,
> unsigned int offset = address - vlapic_base_address(vlapic);
> int rc = X86EMUL_OKAY;
>
> - if ( offset != 0xb0 )
> + if ( offset != APIC_EOI )
> HVM_DBG_LOG(DBG_LEVEL_VLAPIC,
> "offset %#x with length %#lx, and value is %#lx",
> offset, len, val);
> --- a/xen/include/asm-x86/hvm/vioapic.h
> +++ b/xen/include/asm-x86/hvm/vioapic.h
> @@ -47,6 +47,7 @@
> #define VIOAPIC_REG_APIC_ID 0x00 /* x86 IOAPIC only */
> #define VIOAPIC_REG_VERSION 0x01
> #define VIOAPIC_REG_ARB_ID 0x02 /* x86 IOAPIC only */
> +#define VIOAPIC_REG_RTE0 0x10
>
> struct hvm_vioapic {
> struct hvm_hw_vioapic hvm_hw_vioapic;
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions()
2015-01-22 13:57 ` [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions() Jan Beulich
2015-01-22 14:12 ` Andrew Cooper
@ 2015-01-22 14:42 ` Tim Deegan
2015-01-22 15:12 ` Jan Beulich
1 sibling, 1 reply; 18+ messages in thread
From: Tim Deegan @ 2015-01-22 14:42 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Andrew Cooper
At 13:57 +0000 on 22 Jan (1421931437), Jan Beulich wrote:
> While doing so also take care of #VE here (even if we don't make use of
> it yet).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
> */
> uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
> {
> + const unsigned int contributory_exceptions =
> + (1 << TRAP_divide_error) |
> + (1 << TRAP_invalid_tss) |
> + (1 << TRAP_no_segment) |
> + (1 << TRAP_stack_error) |
> + (1 << TRAP_gp_fault);
== 0x3c01, i.e. TRAP_page_fault has gone missing. AFAICS that's
correct, but please mention it in the patch description.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/4] x86/traps: replace plain numbers
2015-01-22 14:00 ` [PATCH 3/4] x86/traps: " Jan Beulich
@ 2015-01-22 14:45 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2015-01-22 14:45 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 22/01/15 14:00, Jan Beulich wrote:
> ... making the code better document itself. No functional change
> intended.
>
> Note that for now (as we don't support RTM yet) DR_STATUS_RESERVED_ONE
> and its users don't take DR_NOT_RTM into consideration yet.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -531,9 +531,9 @@ static void instruction_done(
> regs->eflags &= ~X86_EFLAGS_RF;
> if ( bpmatch || (regs->eflags & X86_EFLAGS_TF) )
> {
> - current->arch.debugreg[6] |= bpmatch | 0xffff0ff0;
> + current->arch.debugreg[6] |= bpmatch | DR_STATUS_RESERVED_ONE;
> if ( regs->eflags & X86_EFLAGS_TF )
> - current->arch.debugreg[6] |= 0x4000;
> + current->arch.debugreg[6] |= DR_STEP;
> do_guest_trap(TRAP_debug, regs, 0);
> }
> }
> @@ -3872,8 +3872,8 @@ long set_debugreg(struct vcpu *v, unsign
> * DR6: Bits 4-11,16-31 reserved (set to 1).
> * Bit 12 reserved (set to 0).
> */
> - value &= 0xffffefff; /* reserved bits => 0 */
> - value |= 0xffff0ff0; /* reserved bits => 1 */
> + value &= ~DR_STATUS_RESERVED_ZERO; /* reserved bits => 0 */
> + value |= DR_STATUS_RESERVED_ONE; /* reserved bits => 1 */
> if ( v == curr )
> write_debugreg(6, value);
> break;
> --- a/xen/include/asm-x86/debugreg.h
> +++ b/xen/include/asm-x86/debugreg.h
> @@ -21,6 +21,8 @@
> #define DR_STEP (0x4000) /* single-step */
> #define DR_SWITCH (0x8000) /* task switch */
> #define DR_NOT_RTM (0x10000) /* clear: #BP inside RTM region */
> +#define DR_STATUS_RESERVED_ZERO (~0xffffeffful) /* Reserved, read as zero */
> +#define DR_STATUS_RESERVED_ONE 0xffff0ff0ul /* Reserved, read as one */
>
> /* Now define a bunch of things for manipulating the control register.
> The top two bytes of the control register consist of 4 fields of 4
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions()
2015-01-22 14:42 ` Tim Deegan
@ 2015-01-22 15:12 ` Jan Beulich
2015-01-22 15:27 ` Tim Deegan
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 15:12 UTC (permalink / raw)
To: Tim Deegan; +Cc: Andrew Cooper, Keir Fraser, xen-devel
>>> On 22.01.15 at 15:42, <tim@xen.org> wrote:
> At 13:57 +0000 on 22 Jan (1421931437), Jan Beulich wrote:
>> While doing so also take care of #VE here (even if we don't make use of
>> it yet).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
>> */
>> uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
>> {
>> + const unsigned int contributory_exceptions =
>> + (1 << TRAP_divide_error) |
>> + (1 << TRAP_invalid_tss) |
>> + (1 << TRAP_no_segment) |
>> + (1 << TRAP_stack_error) |
>> + (1 << TRAP_gp_fault);
>
> == 0x3c01, i.e. TRAP_page_fault has gone missing. AFAICS that's
> correct, but please mention it in the patch description.
Since it's part of page_faults I don't think it has gone missing, but I'll
add an explaining sentence nevertheless.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] x86/HVM: replace plain numbers
2015-01-22 14:41 ` Andrew Cooper
@ 2015-01-22 15:17 ` Jan Beulich
2015-01-23 13:41 ` Andrew Cooper
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2015-01-22 15:17 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 22.01.15 at 15:41, <andrew.cooper3@citrix.com> wrote:
> On 22/01/15 13:57, Jan Beulich wrote:
>> ... making the code better document itself. No functional change
>> intended.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/vioapic.c
>> +++ b/xen/arch/x86/hvm/vioapic.c
>> @@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
>> switch ( vioapic->ioregsel )
>> {
>> case VIOAPIC_REG_VERSION:
>> - result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
>> - | (VIOAPIC_VERSION_ID & 0xff));
>> + result = ((union IO_APIC_reg_01){
>> + .bits = { .version = VIOAPIC_VERSION_ID,
>> + .entries = VIOAPIC_NUM_PINS - 1 }
>> + }).raw;
>> break;
>>
>> case VIOAPIC_REG_APIC_ID:
>> + /*
>> + * Using union IO_APIC_reg_02 for the ID register too, as
>> + * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
>> + */
>
> Having looked into this, Intel has a 4 bit wide ID with the top 4 bits
> reserved, while AMD has the top 4 bits as the Extended ID which may be
> used if an appropriate northbridge register has been set.
>
> I think it might be better to use IO_APIC_reg_00 here and mask the write
> operations, leaving a note about Intel vs AMD and the fact that emulate
> an Intel IOAPIC to match the PIIX3 chipset in Qemu.
>
> Modifying IO_APIC_reg_00 is not appropriate as Xens IOAPIC code needs to
> deal with AMD systems as well.
I had it that way first, but for the purpose of making very clear that
there is no functional change, I decided against doing such a conversion.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] VMX: replace plain numbers
2015-01-22 14:01 ` [PATCH 4/4] VMX: " Jan Beulich
@ 2015-01-22 15:21 ` Andrew Cooper
2015-01-23 6:25 ` Tian, Kevin
1 sibling, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2015-01-22 15:21 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Kevin Tian, Eddie Dong, Jun Nakajima
[-- Attachment #1.1: Type: text/plain, Size: 9052 bytes --]
On 22/01/15 14:01, Jan Beulich wrote:
> ... making the code better document itself. No functional change
> intended.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -1406,7 +1406,9 @@ static void __vmx_inject_exception(int t
> * VM entry]", PRM Vol. 3, 22.6.1 (Interruptibility State).
> */
>
> - intr_fields = (INTR_INFO_VALID_MASK | (type<<8) | trap);
> + intr_fields = INTR_INFO_VALID_MASK |
> + MASK_INSR(type, INTR_INFO_INTR_TYPE_MASK) |
> + MASK_INSR(trap, INTR_INFO_VECTOR_MASK);
> if ( error_code != HVM_DELIVER_NO_ERROR_CODE ) {
> __vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
> intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
> @@ -1430,7 +1432,9 @@ void vmx_inject_extint(int trap, uint8_t
> PIN_BASED_VM_EXEC_CONTROL);
> if ( pin_based_cntrl & PIN_BASED_EXT_INTR_MASK ) {
> nvmx_enqueue_n2_exceptions (v,
> - INTR_INFO_VALID_MASK | (X86_EVENTTYPE_EXT_INTR<<8) | trap,
> + INTR_INFO_VALID_MASK |
> + MASK_INSR(X86_EVENTTYPE_EXT_INTR, INTR_INFO_INTR_TYPE_MASK) |
> + MASK_INSR(trap, INTR_INFO_VECTOR_MASK),
> HVM_DELIVER_NO_ERROR_CODE, source);
> return;
> }
> @@ -1449,7 +1453,9 @@ void vmx_inject_nmi(void)
> PIN_BASED_VM_EXEC_CONTROL);
> if ( pin_based_cntrl & PIN_BASED_NMI_EXITING ) {
> nvmx_enqueue_n2_exceptions (v,
> - INTR_INFO_VALID_MASK | (X86_EVENTTYPE_NMI<<8) | TRAP_nmi,
> + INTR_INFO_VALID_MASK |
> + MASK_INSR(X86_EVENTTYPE_NMI, INTR_INFO_INTR_TYPE_MASK) |
> + MASK_INSR(TRAP_nmi, INTR_INFO_VECTOR_MASK),
> HVM_DELIVER_NO_ERROR_CODE, hvm_intsrc_nmi);
> return;
> }
> @@ -1487,7 +1493,7 @@ static void vmx_inject_trap(struct hvm_t
> if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF )
> {
> __restore_debug_registers(curr);
> - write_debugreg(6, read_debugreg(6) | 0x4000);
> + write_debugreg(6, read_debugreg(6) | DR_STEP);
> }
> if ( cpu_has_monitor_trap_flag )
> break;
> @@ -1502,7 +1508,8 @@ static void vmx_inject_trap(struct hvm_t
> }
>
> if ( unlikely(intr_info & INTR_INFO_VALID_MASK) &&
> - (((intr_info >> 8) & 7) == X86_EVENTTYPE_HW_EXCEPTION) )
> + (MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) ==
> + X86_EVENTTYPE_HW_EXCEPTION) )
> {
> _trap.vector = hvm_combine_hw_exceptions(
> (uint8_t)intr_info, _trap.vector);
> @@ -1517,7 +1524,9 @@ static void vmx_inject_trap(struct hvm_t
> nvmx_intercepts_exception(curr, _trap.vector, _trap.error_code) )
> {
> nvmx_enqueue_n2_exceptions (curr,
> - INTR_INFO_VALID_MASK | (_trap.type<<8) | _trap.vector,
> + INTR_INFO_VALID_MASK |
> + MASK_INSR(_trap.type, INTR_INFO_INTR_TYPE_MASK) |
> + MASK_INSR(_trap.vector, INTR_INFO_VECTOR_MASK),
> _trap.error_code, hvm_intsrc_none);
> return;
> }
> @@ -1976,8 +1985,11 @@ static int vmx_cr_access(unsigned long e
> }
> case VMX_CONTROL_REG_ACCESS_TYPE_LMSW: {
> unsigned long value = curr->arch.hvm_vcpu.guest_cr[0];
> - /* LMSW can: (1) set bits 0-3; (2) clear bits 1-3. */
> - value = (value & ~0xe) | ((exit_qualification >> 16) & 0xf);
> +
> + /* LMSW can (1) set PE; (2) set or clear MP, EM, and TS. */
> + value = (value & ~(X86_CR0_MP|X86_CR0_EM|X86_CR0_TS)) |
> + (VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
> + (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS));
> HVMTRACE_LONG_1D(LMSW, value);
> return hvm_set_cr0(value);
> }
> @@ -2803,7 +2815,7 @@ void vmx_vmexit_handler(struct cpu_user_
> */
> __vmread(EXIT_QUALIFICATION, &exit_qualification);
> HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
> - write_debugreg(6, exit_qualification | 0xffff0ff0);
> + write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
> if ( !v->domain->debugger_attached || cpu_has_monitor_trap_flag )
> goto exit_and_crash;
> domain_pause_for_debugger();
> @@ -2872,8 +2884,8 @@ void vmx_vmexit_handler(struct cpu_user_
> hvm_inject_page_fault(regs->error_code, exit_qualification);
> break;
> case TRAP_nmi:
> - if ( (intr_info & INTR_INFO_INTR_TYPE_MASK) !=
> - (X86_EVENTTYPE_NMI << 8) )
> + if ( MASK_EXTR(intr_info, INTR_INFO_INTR_TYPE_MASK) !=
> + X86_EVENTTYPE_NMI )
> goto exit_and_crash;
> HVMTRACE_0D(NMI);
> /* Already handled above. */
> @@ -2924,7 +2936,8 @@ void vmx_vmexit_handler(struct cpu_user_
> * - TSW is a vectored event due to a SW exception or SW interrupt.
> */
> inst_len = ((source != 3) || /* CALL, IRET, or JMP? */
> - (idtv_info & (1u<<10))) /* IntrType > 3? */
> + (MASK_EXTR(idtv_info, INTR_INFO_INTR_TYPE_MASK)
> + > 3)) /* IntrType > 3? */
> ? get_instruction_length() /* Safe: SDM 3B 23.2.4 */ : 0;
> if ( (source == 3) && (idtv_info & INTR_INFO_DELIVER_CODE_MASK) )
> __vmread(IDT_VECTORING_ERROR_CODE, &ecode);
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1272,7 +1272,7 @@ static void sync_exception_state(struct
> if ( !(nvmx->intr.intr_info & INTR_INFO_VALID_MASK) )
> return;
>
> - switch ( (nvmx->intr.intr_info & INTR_INFO_INTR_TYPE_MASK) >> 8 )
> + switch ( MASK_EXTR(nvmx->intr.intr_info, INTR_INFO_INTR_TYPE_MASK) )
> {
> case X86_EVENTTYPE_EXT_INTR:
> /* rename exit_reason to EXTERNAL_INTERRUPT */
> @@ -1327,10 +1327,10 @@ static void nvmx_update_apicv(struct vcp
> ppr = vlapic_set_ppr(vlapic);
> WARN_ON((ppr & 0xf0) != (vector & 0xf0));
>
> - status = vector << 8;
> + status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
> rvi = vlapic_has_pending_irq(v);
> if ( rvi != -1 )
> - status |= rvi & 0xff;
> + status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
>
> __vmwrite(GUEST_INTR_STATUS, status);
> }
> @@ -2161,7 +2161,8 @@ int nvmx_n2_vmexit_handler(struct cpu_us
> case EXIT_REASON_EXCEPTION_NMI:
> {
> unsigned long intr_info;
> - u32 valid_mask = (X86_EVENTTYPE_HW_EXCEPTION << 8) |
> + u32 valid_mask = MASK_INSR(X86_EVENTTYPE_HW_EXCEPTION,
> + INTR_INFO_INTR_TYPE_MASK) |
> INTR_INFO_VALID_MASK;
> u64 exec_bitmap;
> int vector;
> @@ -2350,8 +2351,8 @@ int nvmx_n2_vmexit_handler(struct cpu_us
> u32 mask = 0;
>
> __vmread(EXIT_QUALIFICATION, &exit_qualification);
> - cr = exit_qualification & 0xf;
> - write = (exit_qualification >> 4) & 3;
> + cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
> + write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
> /* also according to guest exec_control */
> ctrl = __n2_exec_control(v);
>
> @@ -2443,8 +2444,9 @@ int nvmx_n2_vmexit_handler(struct cpu_us
> u64 cr0_gh_mask = __get_vvmcs(nvcpu->nv_vvmcx, CR0_GUEST_HOST_MASK);
>
> __vmread(CR0_READ_SHADOW, &old_val);
> - old_val &= 0xf;
> - val = (exit_qualification >> 16) & 0xf;
> + old_val &= X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS;
> + val = VMX_CONTROL_REG_ACCESS_DATA(exit_qualification) &
> + (X86_CR0_PE|X86_CR0_MP|X86_CR0_EM|X86_CR0_TS);
> changed_bits = old_val ^ val;
> if ( changed_bits & cr0_gh_mask )
> nvcpu->nv_vmexit_pending = 1;
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -207,8 +207,10 @@ static inline unsigned long pi_get_pir(s
> # define VMX_CONTROL_REG_ACCESS_TYPE_MOV_FROM_CR 1
> # define VMX_CONTROL_REG_ACCESS_TYPE_CLTS 2
> # define VMX_CONTROL_REG_ACCESS_TYPE_LMSW 3
> - /* 10:8 - general purpose register operand */
> + /* 11:8 - general purpose register operand */
> #define VMX_CONTROL_REG_ACCESS_GPR(eq) (((eq) >> 8) & 0xf)
> + /* 31:16 - LMSW source data */
> +#define VMX_CONTROL_REG_ACCESS_DATA(eq) ((uint32_t)(eq) >> 16)
>
> /*
> * Access Rights
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 9880 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions()
2015-01-22 15:12 ` Jan Beulich
@ 2015-01-22 15:27 ` Tim Deegan
0 siblings, 0 replies; 18+ messages in thread
From: Tim Deegan @ 2015-01-22 15:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir Fraser, xen-devel
At 15:12 +0000 on 22 Jan (1421935935), Jan Beulich wrote:
> >>> On 22.01.15 at 15:42, <tim@xen.org> wrote:
> > At 13:57 +0000 on 22 Jan (1421931437), Jan Beulich wrote:
> >> While doing so also take care of #VE here (even if we don't make use of
> >> it yet).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -205,6 +205,16 @@ int hvm_event_needs_reinjection(uint8_t
> >> */
> >> uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2)
> >> {
> >> + const unsigned int contributory_exceptions =
> >> + (1 << TRAP_divide_error) |
> >> + (1 << TRAP_invalid_tss) |
> >> + (1 << TRAP_no_segment) |
> >> + (1 << TRAP_stack_error) |
> >> + (1 << TRAP_gp_fault);
> >
> > == 0x3c01, i.e. TRAP_page_fault has gone missing. AFAICS that's
> > correct, but please mention it in the patch description.
>
> Since it's part of page_faults I don't think it has gone missing, but I'll
> add an explaining sentence nevertheless.
Thanks. With that, Reviewed-by: TIm Deegan <tim@xen.org>
Cheers,
Tim.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/4] VMX: replace plain numbers
2015-01-22 14:01 ` [PATCH 4/4] VMX: " Jan Beulich
2015-01-22 15:21 ` Andrew Cooper
@ 2015-01-23 6:25 ` Tian, Kevin
1 sibling, 0 replies; 18+ messages in thread
From: Tian, Kevin @ 2015-01-23 6:25 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Dong, Eddie, Nakajima, Jun
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, January 22, 2015 10:01 PM
>
> ... making the code better document itself. No functional change
> intended.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] x86/HVM: replace plain numbers
2015-01-22 15:17 ` Jan Beulich
@ 2015-01-23 13:41 ` Andrew Cooper
2015-01-23 13:58 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2015-01-23 13:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 22/01/15 15:17, Jan Beulich wrote:
>>>> On 22.01.15 at 15:41, <andrew.cooper3@citrix.com> wrote:
>> On 22/01/15 13:57, Jan Beulich wrote:
>>> ... making the code better document itself. No functional change
>>> intended.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/vioapic.c
>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>> @@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
>>> switch ( vioapic->ioregsel )
>>> {
>>> case VIOAPIC_REG_VERSION:
>>> - result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
>>> - | (VIOAPIC_VERSION_ID & 0xff));
>>> + result = ((union IO_APIC_reg_01){
>>> + .bits = { .version = VIOAPIC_VERSION_ID,
>>> + .entries = VIOAPIC_NUM_PINS - 1 }
>>> + }).raw;
>>> break;
>>>
>>> case VIOAPIC_REG_APIC_ID:
>>> + /*
>>> + * Using union IO_APIC_reg_02 for the ID register too, as
>>> + * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
>>> + */
>> Having looked into this, Intel has a 4 bit wide ID with the top 4 bits
>> reserved, while AMD has the top 4 bits as the Extended ID which may be
>> used if an appropriate northbridge register has been set.
>>
>> I think it might be better to use IO_APIC_reg_00 here and mask the write
>> operations, leaving a note about Intel vs AMD and the fact that emulate
>> an Intel IOAPIC to match the PIIX3 chipset in Qemu.
>>
>> Modifying IO_APIC_reg_00 is not appropriate as Xens IOAPIC code needs to
>> deal with AMD systems as well.
> I had it that way first, but for the purpose of making very clear that
> there is no functional change, I decided against doing such a conversion.
Ok, but please adjust the comment.
"for some reason" is not helpful, whereas "because we emulate an Intel
IOAPIC which only has a 4 bit ID field, compared to 8 for AMD" would be
better.
~Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] x86/HVM: replace plain numbers
2015-01-23 13:41 ` Andrew Cooper
@ 2015-01-23 13:58 ` Jan Beulich
2015-01-23 13:59 ` Andrew Cooper
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2015-01-23 13:58 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 23.01.15 at 14:41, <andrew.cooper3@citrix.com> wrote:
> On 22/01/15 15:17, Jan Beulich wrote:
>>>>> On 22.01.15 at 15:41, <andrew.cooper3@citrix.com> wrote:
>>> On 22/01/15 13:57, Jan Beulich wrote:
>>>> ... making the code better document itself. No functional change
>>>> intended.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/xen/arch/x86/hvm/vioapic.c
>>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>>> @@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
>>>> switch ( vioapic->ioregsel )
>>>> {
>>>> case VIOAPIC_REG_VERSION:
>>>> - result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
>>>> - | (VIOAPIC_VERSION_ID & 0xff));
>>>> + result = ((union IO_APIC_reg_01){
>>>> + .bits = { .version = VIOAPIC_VERSION_ID,
>>>> + .entries = VIOAPIC_NUM_PINS - 1 }
>>>> + }).raw;
>>>> break;
>>>>
>>>> case VIOAPIC_REG_APIC_ID:
>>>> + /*
>>>> + * Using union IO_APIC_reg_02 for the ID register too, as
>>>> + * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
>>>> + */
>>> Having looked into this, Intel has a 4 bit wide ID with the top 4 bits
>>> reserved, while AMD has the top 4 bits as the Extended ID which may be
>>> used if an appropriate northbridge register has been set.
>>>
>>> I think it might be better to use IO_APIC_reg_00 here and mask the write
>>> operations, leaving a note about Intel vs AMD and the fact that emulate
>>> an Intel IOAPIC to match the PIIX3 chipset in Qemu.
>>>
>>> Modifying IO_APIC_reg_00 is not appropriate as Xens IOAPIC code needs to
>>> deal with AMD systems as well.
>> I had it that way first, but for the purpose of making very clear that
>> there is no functional change, I decided against doing such a conversion.
>
> Ok, but please adjust the comment.
>
> "for some reason" is not helpful, whereas "because we emulate an Intel
> IOAPIC which only has a 4 bit ID field, compared to 8 for AMD" would be
> better.
When I wrote the comment, I didn't have a clue why this was
inconsistent. Now we have at least a guess. I'll therefore use
your wording with "because" preceded by "presumably":
+ /*
+ * Presumably because we emulate an Intel IOAPIC which only has a
+ * 4 bit ID field (compared to 8 for AMD), using union IO_APIC_reg_02
+ * for the ID register (union IO_APIC_reg_00's ID field is 8 bits).
+ */
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/4] x86/HVM: replace plain numbers
2015-01-23 13:58 ` Jan Beulich
@ 2015-01-23 13:59 ` Andrew Cooper
0 siblings, 0 replies; 18+ messages in thread
From: Andrew Cooper @ 2015-01-23 13:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
On 23/01/15 13:58, Jan Beulich wrote:
>>>> On 23.01.15 at 14:41, <andrew.cooper3@citrix.com> wrote:
>> On 22/01/15 15:17, Jan Beulich wrote:
>>>>>> On 22.01.15 at 15:41, <andrew.cooper3@citrix.com> wrote:
>>>> On 22/01/15 13:57, Jan Beulich wrote:
>>>>> ... making the code better document itself. No functional change
>>>>> intended.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> --- a/xen/arch/x86/hvm/vioapic.c
>>>>> +++ b/xen/arch/x86/hvm/vioapic.c
>>>>> @@ -53,18 +53,26 @@ static uint32_t vioapic_read_indirect(co
>>>>> switch ( vioapic->ioregsel )
>>>>> {
>>>>> case VIOAPIC_REG_VERSION:
>>>>> - result = ((((VIOAPIC_NUM_PINS-1) & 0xff) << 16)
>>>>> - | (VIOAPIC_VERSION_ID & 0xff));
>>>>> + result = ((union IO_APIC_reg_01){
>>>>> + .bits = { .version = VIOAPIC_VERSION_ID,
>>>>> + .entries = VIOAPIC_NUM_PINS - 1 }
>>>>> + }).raw;
>>>>> break;
>>>>>
>>>>> case VIOAPIC_REG_APIC_ID:
>>>>> + /*
>>>>> + * Using union IO_APIC_reg_02 for the ID register too, as
>>>>> + * union IO_APIC_reg_00's ID field is 8 bits wide for some reason.
>>>>> + */
>>>> Having looked into this, Intel has a 4 bit wide ID with the top 4 bits
>>>> reserved, while AMD has the top 4 bits as the Extended ID which may be
>>>> used if an appropriate northbridge register has been set.
>>>>
>>>> I think it might be better to use IO_APIC_reg_00 here and mask the write
>>>> operations, leaving a note about Intel vs AMD and the fact that emulate
>>>> an Intel IOAPIC to match the PIIX3 chipset in Qemu.
>>>>
>>>> Modifying IO_APIC_reg_00 is not appropriate as Xens IOAPIC code needs to
>>>> deal with AMD systems as well.
>>> I had it that way first, but for the purpose of making very clear that
>>> there is no functional change, I decided against doing such a conversion.
>> Ok, but please adjust the comment.
>>
>> "for some reason" is not helpful, whereas "because we emulate an Intel
>> IOAPIC which only has a 4 bit ID field, compared to 8 for AMD" would be
>> better.
> When I wrote the comment, I didn't have a clue why this was
> inconsistent. Now we have at least a guess. I'll therefore use
> your wording with "because" preceded by "presumably":
>
> + /*
> + * Presumably because we emulate an Intel IOAPIC which only has a
> + * 4 bit ID field (compared to 8 for AMD), using union IO_APIC_reg_02
> + * for the ID register (union IO_APIC_reg_00's ID field is 8 bits).
> + */
Looks much better.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-01-23 13:59 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-22 13:53 [PATCH 0/4] x86: replace plain numbers Jan Beulich
2015-01-22 13:57 ` [PATCH 1/4] x86/HVM: replace plain number in hvm_combine_hw_exceptions() Jan Beulich
2015-01-22 14:12 ` Andrew Cooper
2015-01-22 14:36 ` Jan Beulich
2015-01-22 14:42 ` Tim Deegan
2015-01-22 15:12 ` Jan Beulich
2015-01-22 15:27 ` Tim Deegan
2015-01-22 13:57 ` [PATCH 2/4] x86/HVM: replace plain numbers Jan Beulich
2015-01-22 14:41 ` Andrew Cooper
2015-01-22 15:17 ` Jan Beulich
2015-01-23 13:41 ` Andrew Cooper
2015-01-23 13:58 ` Jan Beulich
2015-01-23 13:59 ` Andrew Cooper
2015-01-22 14:00 ` [PATCH 3/4] x86/traps: " Jan Beulich
2015-01-22 14:45 ` Andrew Cooper
2015-01-22 14:01 ` [PATCH 4/4] VMX: " Jan Beulich
2015-01-22 15:21 ` Andrew Cooper
2015-01-23 6:25 ` Tian, Kevin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.