xen/arch/x86/hvm/vlapic.c | 75 ++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 33 deletions(-)
From: Grygorii Strashko <grygorii_strashko@epam.com>
Rework LVTx registers masks usage in MMIO/WRMSR write emulation code:
- do LVTx masks renaming and rearranging to x_WR_MASK/x_RO_MASK
- rename "vlapic_lvt_mask[]" to "lvt_wr_masks[]" to indicate they define
writable LVTx regs bits
- add lvt_rsvdz_masks[] and use it in guest_wrmsr_x2apic() for "Reserved"
bits checking (RsvdZ, Non-zero writes to reserved bits should cause #GP
exception)
- add LVT_REG_IDX() macro to avoid open coding calculation of LVTx regs
indexes for lvt_wr_masks[]/lvt_rsvdz_masks[] in many places
Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
---
Hi
Patch created according to discussion [1] and based on [1].
I've seen patches from Jan [2] a bit late, so sending
it as is for now.
[1] https://patchwork.kernel.org/project/xen-devel/patch/20250930190550.1166875-1-grygorii_strashko@epam.com/
[2] https://patchwork.kernel.org/project/xen-devel/cover/265d5053-af61-42cb-a3b9-ef60df39c21b@suse.com/
xen/arch/x86/hvm/vlapic.c | 75 ++++++++++++++++++++++-----------------
1 file changed, 42 insertions(+), 33 deletions(-)
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 2ecba8163f48..1a00b224e033 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -34,27 +34,44 @@
#define VLAPIC_VERSION 0x00050014
#define VLAPIC_LVT_NUM 6
-#define LVT_MASK \
- (APIC_LVT_MASKED | APIC_SEND_PENDING | APIC_VECTOR_MASK)
-
-#define LINT_MASK \
- (LVT_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY |\
- APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
+#define LVT_REG_IDX(reg) (((reg) - APIC_LVTT) >> 4)
+
+#define LVT_WR_MASK (APIC_LVT_MASKED | APIC_VECTOR_MASK)
+#define LVT_RO_MASK (APIC_SEND_PENDING)
+
+#define LINT_RW_MASK \
+ (LVT_WR_MASK | APIC_DM_MASK | APIC_INPUT_POLARITY | APIC_LVT_LEVEL_TRIGGER)
+#define LINT_RO_MASK (LVT_RO_MASK | APIC_LVT_REMOTE_IRR)
+
+static const unsigned int lvt_wr_masks[VLAPIC_LVT_NUM] = {
+ /* LVTT */
+ LVT_WR_MASK | APIC_TIMER_MODE_MASK,
+ /* LVTTHMR */
+ LVT_WR_MASK | APIC_DM_MASK,
+ /* LVTPC */
+ LVT_WR_MASK | APIC_DM_MASK,
+ /* LVT0 */
+ LINT_RW_MASK,
+ /* LVT1 */
+ LINT_RW_MASK,
+ /* LVTERR */
+ LVT_WR_MASK,
+};
-static const unsigned int vlapic_lvt_mask[VLAPIC_LVT_NUM] =
-{
- /* LVTT */
- (LVT_MASK | APIC_TIMER_MODE_MASK) & ~APIC_SEND_PENDING,
- /* LVTTHMR */
- (LVT_MASK | APIC_DM_MASK) & ~APIC_SEND_PENDING,
- /* LVTPC */
- (LVT_MASK | APIC_DM_MASK) & ~APIC_SEND_PENDING,
- /* LVT0 */
- LINT_MASK & ~(APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING),
- /* LVT1 */
- LINT_MASK & ~(APIC_LVT_REMOTE_IRR | APIC_SEND_PENDING),
- /* LVTERR */
- LVT_MASK & ~APIC_SEND_PENDING,
+/* LVTx reserved (rsvdZ) bits masks */
+static const unsigned int lvt_rsvdz_masks[VLAPIC_LVT_NUM] = {
+ /* LVTT */
+ ~(LVT_WR_MASK | APIC_TIMER_MODE_MASK | LVT_RO_MASK),
+ /* LVTTHMR */
+ ~(LVT_WR_MASK | APIC_DM_MASK | LVT_RO_MASK),
+ /* LVTPC */
+ ~(LVT_WR_MASK | APIC_DM_MASK | LVT_RO_MASK),
+ /* LVT0 */
+ ~(LINT_RW_MASK | LINT_RO_MASK),
+ /* LVT1 */
+ ~(LINT_RW_MASK | LINT_RO_MASK),
+ /* LVTERR */
+ ~(LVT_WR_MASK | LVT_RO_MASK),
};
#define vlapic_lvtt_period(vlapic) \
@@ -881,7 +898,7 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, uint32_t val)
case APIC_LVTERR: /* LVT Error Reg */
if ( vlapic_sw_disabled(vlapic) )
val |= APIC_LVT_MASKED;
- val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4);
+ val &= array_access_nospec(lvt_wr_masks, LVT_REG_IDX(reg));
vlapic_set_reg(vlapic, reg, val);
if ( reg == APIC_LVT0 )
{
@@ -1013,26 +1030,18 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val)
return X86EMUL_EXCEPTION;
break;
- case APIC_LVTT:
- if ( val & ~(LVT_MASK | APIC_TIMER_MODE_MASK) )
+ case APIC_CMCI:
+ if ( val & ~(LVT_WR_MASK | LVT_RO_MASK | APIC_DM_MASK) )
return X86EMUL_EXCEPTION;
break;
+ case APIC_LVTT:
case APIC_LVTTHMR:
case APIC_LVTPC:
- case APIC_CMCI:
- if ( val & ~(LVT_MASK | APIC_DM_MASK) )
- return X86EMUL_EXCEPTION;
- break;
-
case APIC_LVT0:
case APIC_LVT1:
- if ( val & ~LINT_MASK )
- return X86EMUL_EXCEPTION;
- break;
-
case APIC_LVTERR:
- if ( val & ~LVT_MASK )
+ if ( val & lvt_rsvdz_masks[LVT_REG_IDX(offset)] )
return X86EMUL_EXCEPTION;
break;
--
2.34.1On 09.10.2025 13:42, Grygorii Strashko wrote: > From: Grygorii Strashko <grygorii_strashko@epam.com> > > Rework LVTx registers masks usage in MMIO/WRMSR write emulation code: > > - do LVTx masks renaming and rearranging to x_WR_MASK/x_RO_MASK > > - rename "vlapic_lvt_mask[]" to "lvt_wr_masks[]" to indicate they define > writable LVTx regs bits > > - add lvt_rsvdz_masks[] and use it in guest_wrmsr_x2apic() for "Reserved" > bits checking (RsvdZ, Non-zero writes to reserved bits should cause #GP > exception) Didn't we agree that this may better be done in two steps? > - add LVT_REG_IDX() macro to avoid open coding calculation of LVTx regs > indexes for lvt_wr_masks[]/lvt_rsvdz_masks[] in many places > > Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> > --- > Hi > > Patch created according to discussion [1] and based on [1]. > > I've seen patches from Jan [2] a bit late, so sending > it as is for now. Just wanted to ask, not the least because I'm increasingly of the opinion that putting your fix on top of that series might end up being better. For example, the special treatment of APIC_CMCI then shouldn't be needed anymore. > @@ -881,7 +898,7 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, uint32_t val) > case APIC_LVTERR: /* LVT Error Reg */ > if ( vlapic_sw_disabled(vlapic) ) > val |= APIC_LVT_MASKED; > - val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4); > + val &= array_access_nospec(lvt_wr_masks, LVT_REG_IDX(reg)); Just like it's here, ... > @@ -1013,26 +1030,18 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) > return X86EMUL_EXCEPTION; > break; > > - case APIC_LVTT: > - if ( val & ~(LVT_MASK | APIC_TIMER_MODE_MASK) ) > + case APIC_CMCI: > + if ( val & ~(LVT_WR_MASK | LVT_RO_MASK | APIC_DM_MASK) ) > return X86EMUL_EXCEPTION; > break; > > + case APIC_LVTT: > case APIC_LVTTHMR: > case APIC_LVTPC: > - case APIC_CMCI: > - if ( val & ~(LVT_MASK | APIC_DM_MASK) ) > - return X86EMUL_EXCEPTION; > - break; > - > case APIC_LVT0: > case APIC_LVT1: > - if ( val & ~LINT_MASK ) > - return X86EMUL_EXCEPTION; > - break; > - > case APIC_LVTERR: > - if ( val & ~LVT_MASK ) > + if ( val & lvt_rsvdz_masks[LVT_REG_IDX(offset)] ) ... array_access_nospec() also needs using here. Jan
Hi Jan, On 09.10.25 15:01, Jan Beulich wrote: > On 09.10.2025 13:42, Grygorii Strashko wrote: >> From: Grygorii Strashko <grygorii_strashko@epam.com> >> >> Rework LVTx registers masks usage in MMIO/WRMSR write emulation code: >> >> - do LVTx masks renaming and rearranging to x_WR_MASK/x_RO_MASK >> >> - rename "vlapic_lvt_mask[]" to "lvt_wr_masks[]" to indicate they define >> writable LVTx regs bits >> >> - add lvt_rsvdz_masks[] and use it in guest_wrmsr_x2apic() for "Reserved" >> bits checking (RsvdZ, Non-zero writes to reserved bits should cause #GP >> exception) > > Didn't we agree that this may better be done in two steps? Huh, sorry for misunderstanding. > >> - add LVT_REG_IDX() macro to avoid open coding calculation of LVTx regs >> indexes for lvt_wr_masks[]/lvt_rsvdz_masks[] in many places >> >> Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com> >> --- >> Hi >> >> Patch created according to discussion [1] and based on [1]. >> >> I've seen patches from Jan [2] a bit late, so sending >> it as is for now. > > Just wanted to ask, not the least because I'm increasingly of the opinion > that putting your fix on top of that series might end up being better. > For example, the special treatment of APIC_CMCI then shouldn't be needed > anymore. not sure what to answer here > >> @@ -881,7 +898,7 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, uint32_t val) >> case APIC_LVTERR: /* LVT Error Reg */ >> if ( vlapic_sw_disabled(vlapic) ) >> val |= APIC_LVT_MASKED; >> - val &= array_access_nospec(vlapic_lvt_mask, (reg - APIC_LVTT) >> 4); >> + val &= array_access_nospec(lvt_wr_masks, LVT_REG_IDX(reg)); > > Just like it's here, ... > >> @@ -1013,26 +1030,18 @@ int guest_wrmsr_x2apic(struct vcpu *v, uint32_t msr, uint64_t val) >> return X86EMUL_EXCEPTION; >> break; >> >> - case APIC_LVTT: >> - if ( val & ~(LVT_MASK | APIC_TIMER_MODE_MASK) ) >> + case APIC_CMCI: >> + if ( val & ~(LVT_WR_MASK | LVT_RO_MASK | APIC_DM_MASK) ) >> return X86EMUL_EXCEPTION; >> break; >> >> + case APIC_LVTT: >> case APIC_LVTTHMR: >> case APIC_LVTPC: >> - case APIC_CMCI: >> - if ( val & ~(LVT_MASK | APIC_DM_MASK) ) >> - return X86EMUL_EXCEPTION; >> - break; >> - >> case APIC_LVT0: >> case APIC_LVT1: >> - if ( val & ~LINT_MASK ) >> - return X86EMUL_EXCEPTION; >> - break; >> - >> case APIC_LVTERR: >> - if ( val & ~LVT_MASK ) >> + if ( val & lvt_rsvdz_masks[LVT_REG_IDX(offset)] ) > > ... array_access_nospec() also needs using here. ok -- Best regards, -grygorii
© 2016 - 2025 Red Hat, Inc.