Re: [Xen-devel] [PATCH v3] x86/hvm/viridian: flush remote tlbs by hypercall

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 20 November 2015 09:45
> To: Andrew Cooper; Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; Wei Liu; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Keir (Xen.org)
> Subject: RE: [PATCH v3] x86/hvm/viridian: flush remote tlbs by hypercall
> 
> >>> On 20.11.15 at 10:15, <Paul.Durrant@xxxxxxxxxx> wrote:
> >> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> >> Sent: 19 November 2015 17:09
> >> On 19/11/15 16:57, Paul Durrant wrote:
> >> >>> +        /*
> >> >>> +         * For each specified virtual CPU flush all ASIDs to 
> >> >>> invalidate
> >> >>> +         * TLB entries the next time it is scheduled and then, if it
> >> >>> +         * is currently running, add its physical CPU to a mask of
> >> >>> +         * those which need to be interrupted to force a flush.
> >> >>> +         */
> >> >>> +        for_each_vcpu ( currd, v )
> >> >>> +        {
> >> >>> +            if ( !(input_params.vcpu_mask & (1ul << v->vcpu_id)) )
> >> >>> +                continue;
> >> >> You need to cap this loop at a vcpu_id of 63, or the above conditional
> >> >> will become undefined.
> >> > The guest should not issue the hypercall if it has more than 64 vCPUs so
> to
> >> some extend I don't care what happens, as long as it is not harmful to the
> >> hypervisor in general, and I don't think that it is in this case.
> >>
> >> The compiler is free to do anything it wishes when encountering
> >> undefined behaviour, including crash the hypervisor.
> >>
> >
> > I don't believe so. See section 6.5.7 of the C99 spec. (top pf page 85):
> >
> > "The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits 
> > are
> > filled with
> > zeros. If E1 has an unsigned type, the value of the result is E1 x 2^E2,
> > reduced modulo
> > one more than the maximum value representable in the result type. If E1
> has
> > a signed
> > type and nonnegative value, and E1 x 2^E2 is representable in the result
> > type, then that is
> > the resulting value; otherwise, the behavior is undefined"
> >
> > So, the undefined behaviour you refer to is only in the case that E1 is
> > signed and in this case it is most definitely unsigned, so the modulo rule
> > applies.
> 
> Looks like you missed the earlier clause 3 (at the bottom of page 84):
> "If the value of the right operand is negative or is greater than or
>  equal to the width of the promoted left operand, the behavior is
>  undefined." And if you look at generated code, I think you'll find
> that the result is not zero for too large shift counts, as would need
> to be the case if that clause wasn't there.
> 

Hmm. That does seem contradictory. It does indeed say 'behavior' is undefined. 
If it was merely 'value' is undefined then I would not see that as a problem.

> I'll actually make my previous Reviewed-by dependent on this issue
> getting fixed.
> 

Since you've convinced me that the check is necessary, I'll post v5 with it in.

  Paul

> >> Any undefined-behaviour which can be triggered by a guest action
> >> warrants an XSA, because there is no telling what might happen.
> 
> Well, it warrants fixing, but I don't think it unconditionally would
> be XSA-worthy. After all, the compiler (normally) does sane
> things in response - in order to crash the hypervisor or produce
> truly undefined behavior, it would need to generate extra code
> (since other than in C, at the assembly level the instruction's
> behavior is defined), which I think we can take for granted it
> won't.
> 
> Jan


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