RE: [Xen-ia64-devel] [Fwd: [Xen-bugs] [Bug 1392] New: Problems with denormalized floating point numbers on XEN-virtualized Linux/IA64]

["Zhang, Xiantao" <xiantao.zhang@xxxxxxxxx>]

Isaku Yamahata wrote:
> For those who want to test it, here is the slightly update patch.
> NOTE: this version doesn't solve the potential infinite loop
>       which Alex is suspecting about.
> 
> IA64: fix emulation of fp emulation
> 
> This patch fixes bug reported as
> http://bugzilla.xensource.com/bugzilla/show_bug.cgi?id=1392
> 
> When pv domain case, FPSWA hypercall isn't implemented properly.
> So avoid the injecting floating point fault/trap at this moment.
> However this may cause infinite loop depending on dtlb cache.
> The right fix is to implement the hypercall properly however
> it wouldn't be very straight forward because the argument
> of fpswa is large and includes pointers.
> 
> When hvm domain case, floating point trap case iip was incremented
> improperly. removed the increment
> 
> Signed-off-by: Isaku Yamahata <yamahata@xxxxxxxxxxxxx>
> 
> diff --git a/xen/arch/ia64/vmx/vmx_fault.c
> b/xen/arch/ia64/vmx/vmx_fault.c --- a/xen/arch/ia64/vmx/vmx_fault.c
> +++ b/xen/arch/ia64/vmx/vmx_fault.c
> @@ -130,10 +130,8 @@ void vmx_reflect_interruption(u64 ifa, u
>          status = handle_fpu_swa(0, regs, isr);
>          if (!status)
>              return;
> -        else if (IA64_RETRY == status) {
> -            vcpu_decrement_iip(vcpu);
> +        else if (IA64_RETRY == status)
>              return;
> -        }
>          break;
> 
>      case 29: // IA64_DEBUG_VECTOR

Hi, Isaku
   Why do you think the decrement is useless ? For trap case, the iip should 
point to the next instruction instead of the one which results in the trap. So 
once need retry, the iip should be back to the privious one ?  
Thanks
Xiantao


> diff --git a/xen/arch/ia64/xen/faults.c b/xen/arch/ia64/xen/faults.c
> --- a/xen/arch/ia64/xen/faults.c
> +++ b/xen/arch/ia64/xen/faults.c
> @@ -318,6 +318,7 @@ handle_fpu_swa(int fp_fault, struct pt_r
>       IA64_BUNDLE bundle;
>       unsigned long fault_ip;
>       fpswa_ret_t ret;
> +     unsigned long rc;
> 
>       fault_ip = regs->cr_iip;
>       /*
> @@ -328,16 +329,15 @@ handle_fpu_swa(int fp_fault, struct pt_r
>       if (!fp_fault && (ia64_psr(regs)->ri == 0))
>               fault_ip -= 16;
> 
> -     if (VMX_DOMAIN(current)) {
> -             if (IA64_RETRY == __vmx_get_domain_bundle(fault_ip, &bundle))
> -                     return IA64_RETRY;
> -     } else
> -             bundle = __get_domain_bundle(fault_ip);
> -
> -     if (!bundle.i64[0] && !bundle.i64[1]) {
> -             printk("%s: floating-point bundle at 0x%lx not mapped\n",
> -                    __FUNCTION__, fault_ip);
> -             return -1;
> +     if (VMX_DOMAIN(current))
> +             rc = __vmx_get_domain_bundle(fault_ip, &bundle);
> +     else
> +             rc = __get_domain_bundle(fault_ip, &bundle);
> +     if (rc == IA64_RETRY) {
> +             gdprintk(XENLOG_DEBUG,
> +                      "%s(%s): floating-point bundle at 0x%lx not mapped\n",
> +                      __FUNCTION__, fp_fault ? "fault" : "trap", fault_ip);
> +             return IA64_RETRY;
>       }
> 
>       ret = fp_emulate(fp_fault, &bundle, &regs->cr_ipsr, &regs->ar_fpsr,
> diff --git a/xen/arch/ia64/xen/vcpu.c b/xen/arch/ia64/xen/vcpu.c
> --- a/xen/arch/ia64/xen/vcpu.c
> +++ b/xen/arch/ia64/xen/vcpu.c
> @@ -1325,6 +1325,16 @@ static TR_ENTRY *vcpu_tr_lookup(VCPU * v
>       return NULL;
>  }
> 
> +unsigned long
> +__get_domain_bundle(unsigned long iip, IA64_BUNDLE *bundle)
> +{
> +     *bundle = __get_domain_bundle_asm(iip);
> +     if (!bundle->i64[0] && !bundle->i64[1])
> +             return IA64_RETRY;
> +
> +     return 0;
> +}
> +
>  // return value
>  // 0: failure
>  // 1: success
> @@ -1335,6 +1345,7 @@ vcpu_get_domain_bundle(VCPU * vcpu, REGS
>       u64 gpip;               // guest pseudo phyiscal ip
>       unsigned long vaddr;
>       struct page_info *page;
> +     unsigned long rc;
> 
>   again:
>  #if 0
> @@ -1387,11 +1398,11 @@ vcpu_get_domain_bundle(VCPU * vcpu, REGS
>               if (swap_rr0) {
>                       set_virtual_rr0();
>               }
> -             *bundle = __get_domain_bundle(gip);
> +             rc = __get_domain_bundle(gip, bundle);
>               if (swap_rr0) {
>                       set_metaphysical_rr0();
>               }
> -             if (bundle->i64[0] == 0 && bundle->i64[1] == 0) {
> +             if (rc == IA64_RETRY) {
>                       dprintk(XENLOG_INFO, "%s gip 0x%lx\n", __func__, gip);
>                       return 0;
>               }
> diff --git a/xen/arch/ia64/xen/xenasm.S b/xen/arch/ia64/xen/xenasm.S
> --- a/xen/arch/ia64/xen/xenasm.S
> +++ b/xen/arch/ia64/xen/xenasm.S
> @@ -389,7 +389,7 @@ END(ia64_prepare_handle_reflection)
>  END(ia64_prepare_handle_reflection)
>  #endif
> 
> -GLOBAL_ENTRY(__get_domain_bundle)
> +GLOBAL_ENTRY(__get_domain_bundle_asm)
>       EX(.failure_in_get_bundle,ld8 r8=[r32],8)
>       ;;
>       EX(.failure_in_get_bundle,ld8 r9=[r32])
> @@ -403,7 +403,7 @@ GLOBAL_ENTRY(__get_domain_bundle)
>       ;;
>       br.ret.sptk.many rp
>       ;;
> -END(__get_domain_bundle)
> +END(__get_domain_bundle_asm)
> 
>  /* derived from linux/arch/ia64/hp/sim/boot/boot_head.S */
>  GLOBAL_ENTRY(pal_emulator_static)
> diff --git a/xen/include/asm-ia64/bundle.h
> b/xen/include/asm-ia64/bundle.h --- a/xen/include/asm-ia64/bundle.h
> +++ b/xen/include/asm-ia64/bundle.h
> @@ -225,7 +225,8 @@ typedef union U_INST64 {
> 
>  #ifdef __XEN__
>  extern unsigned long __vmx_get_domain_bundle(unsigned long iip,
> IA64_BUNDLE *pbundle); -extern IA64_BUNDLE
> __get_domain_bundle(unsigned long iip); +extern IA64_BUNDLE
> __get_domain_bundle_asm(unsigned long iip); +extern unsigned long
>  __get_domain_bundle(unsigned long iip, IA64_BUNDLE *bundle); #endif
> 
>  #define MASK_41 ((unsigned long)0x1ffffffffff)
> 
> 
> On Fri, Dec 05, 2008 at 02:29:32PM +0900, Isaku Yamahata wrote:
>> On Thu, Dec 04, 2008 at 09:10:48AM -0700, Alex Williamson wrote:
>>> On Thu, 2008-12-04 at 15:29 +0900, Isaku Yamahata wrote:
>>>> Although I also replied by the bugzilla,
>>>> I also send the patch to the list for those who doesn't watch on
>>>> the bug report. I hope this patch fixes it, please try this.
>>> 
>>> Hi Isaku,
>>> 
>>> Thanks for looking at this.  With your patch, it doesn't fail, but I
>>> regularly see cases where it doesn't complete, at least not in a
>>> reasonable time frame.  I imagine it's getting stuck in an endless
>>> loop of retries.
>> 
>> Hmm, I have been afraid of that. But I haven't observed it myself
>> yet. It's probably because I've tested it without any other
>> workloads. Injecting floating point fault/trap into pv guest and
>> letting the guest retrieve the bundle/issue fpswa hypercall would
>> solve that issue. 
>> 
>> However FPSWA hypercall isn't implemented properly which
>> currently returns random value. This is the root cause.
>> Please see fw_hypercall_fpswa()/ in xen/arch/ia64/xen/hypercall.c
>> which was implemented by Kanno-san as 10158:9d52a66c7499.
>> I can't find any place which sets arch_vcpu::fpswa_ret.
>> Kanno-san, Any comment?
>> 
>> from 10158:9d52a66c7499
>> 
>> +static fpswa_ret_t
>> +fw_hypercall_fpswa (struct vcpu *v)
>> +{
>> +       return PSCBX(v, fpswa_ret);
>> +}
>> +
>> ...
>> @@ -109,6 +114,7 @@ struct arch_vcpu {
>>      //for phycial  emulation
>>      unsigned long old_rsc;
>>      int mode_flags;
>> +    fpswa_ret_t fpswa_ret;     /* save return values of FPSWA
>>      emulation */ struct arch_vmx_struct arch_vmx; /* Virtual
>> Machine Extensions */  }; 
>> 
>> 
>>> On bare metal the test takes about 1.3s (1.66GHz Montvale).
>>> The few cases I've seen it complete running in dom0, it takes about
>>> 3.1s (1.6GHz Montecito).  Thanks,
>> 
>> It's quite slow. more optimization is necessary?


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel