Re: [Xen-devel] [PATCH v2 2/2] xen/arm32: implement VFP context switch

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-05-31 at 16:27 +0100, Julien Grall wrote:
> On 05/31/2013 03:12 PM, Ian Campbell wrote:
> 
> > On Thu, 2013-05-30 at 17:01 +0100, Julien Grall wrote:
> >> +void vfp_save_state(struct vcpu *v)
> >> +{
> >> +    uint32_t tmp;
> >> +
> >> +    v->arch.vfp.fpexc = READ_CP32(FPEXC);
> > 
> > The docs seem to call for reading this via an explicit VMRS
> > instruction. 
> > 
> > Looking at the ARM ARM this seems to be an alias for the encoding of an
> > MRC instruction corresponding to reading FPEXC as you have done. Did you
> > have a reference for that aliasing? (I'm not finding it in the ARM ARM).
> 
> 
> I didn't find anything on the aliasing. I looked at the linux VFP
> include (arch/arm/include/asm/vfp.h).

I wonder if it would be better to do this via VFP specific macros?

Probably not.

> > Are you avoiding the mnemonic to avoid issues with binutils providing
> > the instruction?
> 
> This mnemonic can only be used if VFP is enabled by gcc. I think it's
> safer to use mrc if we don't want to use VFP on the other part of XEN.

Agreed.

> > Do you know what happens to the values of d0..d31  and FPSCR etc when
> 
> > FPEXC.EN is disabled? I'm wondering if we can avoid saving the registers
> > in that case, we'd still need to restore to prevent leaking the last
> > guests state if the VCPU enabled exceptions. I suppose it's not worth it
> > unless we implement a more full featured lazy saving regime.
> 
> 
> I didn't find something about the state of the registers when VFP is
> disabled. I think the registers is not clobbered.
> Linux seems to save vfp registers at each context switch only if VFP is
> enabled. But we cannot rely on that for the other distributions.

Linux likely traps if the process then goes on to use VFP, at which
point it can clear/restore the registers as necessary. We could do
something similar using HCPTR -- that's what I meant by lazy saving (and
restore).

> 
> >> +}
> >> +
> >> +void vfp_restore_state(struct vcpu *v)
> >> +{
> > 
> > Some of the same comments (tmps, barriers etc) apply to restore as to
> > save.
> > 
> > Is there any chance that any of these loads could cause an FP fault?
> > e.g. if the guest had an FP exception pending when we saved it, could
> > reloading the register retrigger it?
> 
> I don't know.

Hrm :-/

If there are no invalid encodings for d0..d31 then it should just be a
case of checking the ARM ARM for FPINST* and FPSCR.

> >> @@ -106,6 +114,7 @@
> >>  #define NSACR           p15,0,c1,c1,2   /* Non-Secure Access Control 
> >> Register */
> >>  #define HSCTLR          p15,4,c1,c0,0   /* Hyp. System Control Register */
> >>  #define HCR             p15,4,c1,c1,0   /* Hyp. Configuration Register */
> >> +#define HCPTR           p15,4,c1,c1,2   /* Hyp. Coprocessor Trap Register 
> >> */
> > 
> > Is this intentionally unused? I don't mind adding unused register
> > definitions, just wanted to check there wasn't a hunk missing or
> > anything.
> 
> 
> Another lost when I have created the patch. I was using it to trap VFP
> when I tested the context switch.

OK.

> 
> > The register resets to 0x0 which is OK but it might be wise to trap all
> > accesses the coprocessors which we haven't implemented ctx switching
> > for? So at least we find out about missing ones instead of silently
> > leaking information between guests and/or corrupting their state.
> 
> 
> What about sending an undefined instruction to the guest if the
> coprocessor is not implemented?
> It seems that some software (sshd, ntpdate) which are using libcrypto,
> are trying to access to the cryptographic coprocessor even if it doesn't
> exist.

Urk. We should either ctx switch it properly or we should hide it
properly (from all the feature registers) and inject undef.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel