On 23.09.2021 12:31, Roger Pau Monné wrote:
> On Tue, Sep 21, 2021 at 09:20:00AM +0200, Jan Beulich wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3408,6 +3408,15 @@ enum hvm_translation_result hvm_copy_fro
>> PFEC_page_present | pfec, pfinfo);
>> }
>>
>> +enum hvm_translation_result hvm_copy_from_vcpu_linear(
>> + void *buf, unsigned long addr, unsigned int size, struct vcpu *v,
>> + unsigned int pfec)
>
> Even if your current use case doesn't need it, would it be worth
> adding a pagefault_info_t parameter?
I'd prefer to add such parameters only once they become necessary.
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -364,6 +364,71 @@ static void show_guest_stack(struct vcpu
>> printk("\n");
>> }
>>
>> +static void show_hvm_stack(struct vcpu *v, const struct cpu_user_regs *regs)
>> +{
>> +#ifdef CONFIG_HVM
>> + unsigned long sp = regs->rsp, addr;
>> + unsigned int i, bytes, words_per_line, pfec = PFEC_page_present;
>> + struct segment_register ss, cs;
>> +
>> + hvm_get_segment_register(v, x86_seg_ss, &ss);
>> + hvm_get_segment_register(v, x86_seg_cs, &cs);
>> +
>> + if ( hvm_long_mode_active(v) && cs.l )
>> + i = 16, bytes = 8;
>> + else
>> + {
>> + sp = ss.db ? (uint32_t)sp : (uint16_t)sp;
>> + i = ss.db ? 8 : 4;
>> + bytes = cs.db ? 4 : 2;
>> + }
>> +
>> + if ( bytes == 8 || (ss.db && !ss.base) )
>> + printk("Guest stack trace from sp=%0*lx:", i, sp);
>> + else
>> + printk("Guest stack trace from ss:sp=%04x:%0*lx:", ss.sel, i, sp);
>> +
>> + if ( !hvm_vcpu_virtual_to_linear(v, x86_seg_ss, &ss, sp, bytes,
>> + hvm_access_read, &cs, &addr) )
>> + {
>> + printk(" Guest-inaccessible memory\n");
>> + return;
>> + }
>> +
>> + if ( ss.dpl == 3 )
>> + pfec |= PFEC_user_mode;
>> +
>> + words_per_line = stack_words_per_line * (sizeof(void *) / bytes);
>> + for ( i = 0; i < debug_stack_lines * words_per_line; )
>> + {
>> + unsigned long val = 0;
>> +
>> + if ( (addr ^ (addr + bytes - 1)) & PAGE_SIZE )
>> + break;
>> +
>> + if ( !(i++ % words_per_line) )
>> + printk("\n ");
>> +
>> + if ( hvm_copy_from_vcpu_linear(&val, addr, bytes, v,
>> + pfec) != HVMTRANS_okay )
>
> I think I'm confused, but what about guests without paging enabled?
> Don't you need to use hvm_copy_from_guest_phys (likely transformed
> into hvm_copy_from_vcpu_phys)?
__hvm_copy() calls hvm_translate_get_page() telling it whether the
input is a linear or physical address. hvm_translate_get_page() will
use paging_gva_to_gfn() in this case. The HAP backing function
changes when the guest {en,dis}ables paging, while shadow code deals
with paging disabled by installing an identity mapping
(d->arch.paging.shadow.unpaged_pagetable) which it would then end up
(needlessly) walking.
It really is - afaict - intentional for callers to not have to deal
with the special case.
>> @@ -663,14 +728,22 @@ void vcpu_show_execution_state(struct vc
>> }
>> #endif
>>
>> - /* Prevent interleaving of output. */
>> - flags = console_lock_recursive_irqsave();
>> + /*
>> + * Prevent interleaving of output if possible. For HVM we can't do so,
>> as
>> + * the necessary P2M lookups involve locking, which has to occur with
>> IRQs
>> + * enabled.
>> + */
>> + if ( !is_hvm_vcpu(v) )
>> + flags = console_lock_recursive_irqsave();
>>
>> vcpu_show_registers(v);
>> - if ( guest_kernel_mode(v, &v->arch.user_regs) )
>> + if ( is_hvm_vcpu(v) )
>> + show_hvm_stack(v, &v->arch.user_regs);
>
> Would it make sense to unlock in show_hvm_stack, and thus keep the
> printing of vcpu_show_registers locked even when in HVM context?
Perhaps not _in_, but before calling it, yet - why not.
> TBH I've never found the guest stack dump to be helpful for debugging
> purposes, but maybe others do.
I can't count how many times I did use the stack dumps of Dom0 to
actually pinpoint problems.
Jan
