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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-06-03 at 12:14 +0100, Julien Grall wrote:
> On 05/31/2013 04:54 PM, Ian Campbell wrote:
> 
> > 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?
> 
> 
> Except in this code we don't need the VFP macros. I don't see specific
> reason to use it here.
> 
> > Probably not.

As you can see I agree ;-)

> > 
> >>> 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).
> 
> I will not have time before 4.3 to implement lazy context switch. I
> think we can postpone this part and see later.

Yes, sorry I should have been clearer -- not worth it until/if we
implement lazy switching post 4.3 is what I meant to say.

> 
> >>
> >>>> +}
> >>>> +
> >>>> +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.
> 
> 
> For FPINST*:
> "Any value read from a Floating-Point Instruction Register can be
> written back to the same register. This means
> context switch and debugger software can save and restore Floating-Point
> Instruction Register values."

Good.

> For FPSCR:
> "Writes to the FPSCR can have side-effects on various aspects of
> processor operation. All of these side-effects are
> synchronous to the FPSCR write. This means they are guaranteed not to be
> visible to earlier instructions in the
> execution stream, and they are guaranteed to be visible to later
> instructions in the execution stream."
> 
> Except if I missed something in FPSCR encoding, we are safe during the
> restore step.

Yes, sounds like it. Thanks.

> >>> 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.
> 
> 
> I will try to write a patch for that when I will have time.


Thank you.

Ian.




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