Re: [PATCH v4 01/14] x86/pv: Don't assume that INT $imm8 instructions are two bytes long

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.03.2026 12:43, Andrew Cooper wrote:
> On 02/03/2026 11:03 am, Jan Beulich wrote:
>> On 28.02.2026 00:16, Andrew Cooper wrote:
>>> @@ -1401,6 +1402,53 @@ int pv_emulate_privileged_op(struct cpu_user_regs 
>>> *regs)
>>>      return 0;
>>>  }
>>>  
>>> +/*
>>> + * Hardware already decoded the INT $N instruction and determinted that 
>>> there
>>> + * was a DPL issue, hence the #GP.  Xen has already determined that the 
>>> guest
>>> + * kernel has permitted this software interrupt.
>>> + *
>>> + * All that is needed is the instruction length, to turn the fault into a
>>> + * trap.  All errors are turned back into the original #GP, as that's the
>>> + * action that really happened.
>>> + */
>>> +void pv_emulate_sw_interrupt(struct cpu_user_regs *regs)
>>> +{
>>> +    struct vcpu *curr = current;
>>> +    struct domain *currd = curr->domain;
>>> +    struct priv_op_ctxt ctxt = {
>>> +        .ctxt.regs = regs,
>>> +        .ctxt.lma = !is_pv_32bit_domain(currd),
>> The difference may not be overly significant here, but 64-bit guests can run
>> 32-bit code, so setting .lma seems wrong in that case. As it ought to be
>> largely benign, perhaps to code could even be left as is, just with a comment
>> to clarify things?
> 
> LMA must be set for a 64bit guest.  Are you confusing it with %cs.l ?

Indeed I am, sorry.

>>> +    struct x86_emulate_state *state;
>>> +    uint8_t vector = regs->error_code >> 3;
>>> +    unsigned int len, ar;
>>> +
>>> +    if ( !pv_emul_read_descriptor(regs->cs, curr, &ctxt.cs.base,
>>> +                                  &ctxt.cs.limit, &ar, 1) ||
>>> +         !(ar & _SEGMENT_S) ||
>>> +         !(ar & _SEGMENT_P) ||
>>> +         !(ar & _SEGMENT_CODE) )
>>> +        goto error;
>>> +
>>> +    state = x86_decode_insn(&ctxt.ctxt, insn_fetch);
>>> +    if ( IS_ERR_OR_NULL(state) )
>>> +        goto error;
>>> +
>>> +    len = x86_insn_length(state, &ctxt.ctxt);
>>> +    x86_emulate_free_state(state);
>>> +
>>> +    /* Note: Checked slightly late to simplify 'state' handling. */
>>> +    if ( ctxt.ctxt.opcode != 0xcd /* INT $imm8 */ )
>>> +        goto error;
>>> +
>>> +    regs->rip += len;
>>> +    pv_inject_sw_interrupt(vector);
>>> +    return;
>>> +
>>> + error:
>>> +    pv_inject_hw_exception(X86_EXC_GP, regs->entry_vector);
>> DYM regs->error_code here?
> 
> Oh.  I'm sure I fixed this bug already.  I wonder where the fix got lost.
> 
> Yes, it should be regs->error_code.

Then (plus with my confusion above sorted)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

>>  Might it alternatively make sense to return a
>> boolean here, for ...
>>
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -1379,8 +1379,7 @@ void do_general_protection(struct cpu_user_regs *regs)
>>>  
>>>          if ( permit_softint(TI_GET_DPL(ti), v, regs) )
>>>          {
>>> -            regs->rip += 2;
>>> -            pv_inject_sw_interrupt(vector);
>>> +            pv_emulate_sw_interrupt(regs);
>>>              return;
>> ... the return here to become conditional, leveraging the #GP injection at
>> the bottom of this function?
> 
> To make this bool, I need to insert a new label into the function.

Why would that be? Simply skipping the return and falling through will do,
afaics.

>  I
> considered that, but delayed it.  do_general_protection() wants a lot
> more cleaning up than just this, and proportionability is a concern.

Whatever you exactly mean with this.

Jan