Re: [EXTERNAL] Issue in handle_icr_write

Thank you for the confirmation, I will send a PR for it.

Thanks,
Melody

On 8/14/25 9:48 AM, Jon Lange wrote:
> I think it would be reasonable to remove those checks.
> 
> -Jon
> 
> -----Original Message-----
> From: Melody Wang <[email protected]>
> Sent: Thursday, August 14, 2025 8:33 AM
> To: Jon Lange <[email protected]>
> Cc: Lendacky, Thomas <[email protected]>; Rödel, Jörg <[email protected]>; Kaplan, David <[email protected]>; [email protected]
> Subject: [EXTERNAL] Issue in handle_icr_write
> 
> Hi Jon,
> 
> I have been working on the PoC of Alternate Injection, I found there seems to be somewhat of a problem in handle_icr_write:
> 
> For the Message Type == Fixed you enforce
> 
>       trigger mode == 0 (edge triggered)
>       and
>       assert == 1
> 
> In practice, actually, those bits are largely ignored.
> 
> The Intel SDM says:
> 
> "Level:
> 
> For the INIT level de-assert delivery mode this flag must be set to 0; for all other delivery modes it must be set to 1. (This flag has no meaning in Pentium 4 and Intel Xeon processors, and will always be issued as a 1.)"
> 
> ...
> 
> Trigger mode:
> 
> Selects the trigger mode when using the INIT level de-assert delivery
> mode: edge (0) or level (1). It is ignored for all other delivery modes.
> (This flag has no meaning in Pentium 4 and Intel Xeon processors, and will always be issued as a 0."
> 
> Basically, the hw ignores those bits and this is how Linux has been doing it since forever.
> 
> Example from me tracing a Linux guest:
> 
> [SVSM] write_register: 0x830: 0x28
> 
> That's only the interrupt vector and neither of those two bits are set.
> 
> So I'm thinking we should simply remove those checks.
> 
> What do you think about it?
> 
> fn handle_icr_write(&mut self, value: u64) -> Result<(), SvsmError> {
>           let icr = ApicIcr::from(value);
> 
>           // Verify that this message type is supported.
>           let valid_type = match icr.message_type() {
>               IcrMessageType::Fixed => {
>                   // Only asserted edge-triggered interrupts can be handled.
>                   !icr.trigger_mode() && icr.assert()
>               }
>               IcrMessageType::Nmi => true,
>               _ => false,
>           };
> 
>           if !valid_type {
>               return Err(SvsmError::Apic(Emulation));
>           }
> 
>           self.send_ipi(icr);
> 
>           Ok(())
>       }
> 
> 
> --
> Thanks,
> Melody
> 

-- 
Thanks,
Melody