Re: [XEN][PATCH] x86: hvm: vlapic: rework WR/rsvdz masks for LVTx regs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2025 13:42, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> 
> 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@xxxxxxxx>
> ---
> 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