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 <[email protected]>
>> 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
