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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/03/2026 12:57 pm, Jan Beulich wrote:
> 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>

Thanks.

>
>>>  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.

Hmm.  That was supposed to say backportability, but I have no idea how
ended up like that.

The other advantage of being void functions is that they can be tailcalled.


Anyway, I have a plan for cleanup once FRED is settled, which looks a
little like this:

handle_GP_IDT()
    if ( guest_regs() )
        return handle_GP_guest()
    else
        return handle_GP_xen()

handle_GP_guest()
    ...

handle_GP_xen()
    ...

where the two FRED entrypoints can now call the context-specific
function rather than the generic one.

This does involve duplicating the X86_XEC_EXT check which is the only
common aspect in the #GP handler.  Next I need to figure out whether the
other handlers can be rearranged similarly.

~Andrew