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