Re: [Xen-devel] [PATCH 1/2] xen: event channel arrays are xen_ulong_t and not unsigned long

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-02-15 at 12:22 +0000, Stefano Stabellini wrote:
> On Thu, 14 Feb 2013, Ian Campbell wrote:
> > On ARM we want these to be the same size on 32- and 64-bit.
> > 
> > This is an ABI change on ARM. X86 does not change.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: Jan Beulich <JBeulich@xxxxxxxx>
> > Cc: Keir (Xen.org) <keir@xxxxxxx>
> > Cc: Tim Deegan <tim@xxxxxxx>
> > Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  arch/arm/include/asm/xen/events.h |   22 +++++++++++
> >  arch/x86/include/asm/xen/events.h |    2 +
> >  drivers/xen/events.c              |   75 
> > ++++++++++++++++++++----------------
> >  include/xen/interface/xen.h       |    8 ++--
> >  4 files changed, 70 insertions(+), 37 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/xen/events.h 
> > b/arch/arm/include/asm/xen/events.h
> > index 94b4e90..92dbb89 100644
> > --- a/arch/arm/include/asm/xen/events.h
> > +++ b/arch/arm/include/asm/xen/events.h
> > @@ -15,4 +15,26 @@ static inline int xen_irqs_disabled(struct pt_regs *regs)
> >     return raw_irqs_disabled_flags(regs->ARM_cpsr);
> >  }
> >  
> > +/*
> > + * We cannot use xchg because it does not support 8-byte
> > + * values. However it is safe to use {ldr,dtd}exd directly because all
> > + * platforms which Xen can run on support those instructions.
> > + */
> > +static inline xen_ulong_t xen_read_evtchn_pending_sel(struct vcpu_info 
> > *vcpu_info)
> > +{
> > +   xen_ulong_t val;
> > +   unsigned int tmp;
> > +
> > +   wmb();
> > +   asm volatile("@ read_evtchn_pending_sel\n"
> > +           "1:     ldrexd  %0, %H0, [%3]\n"
> > +           "       strexd  %1, %2, %H2, [%3]\n"
> > +           "       teq     %1, #0\n"
> > +           "       bne     1b"
> > +                   : "=&r" (val), "=&r" (tmp)
> > +                   : "r" (0), "r" (&vcpu_info->evtchn_pending_sel)
> > +                   : "memory", "cc");
> > +   return val;
> > +}
> 
> At this point we might as well introduce an xchg64, it is going to be
> more useful to us in the future and to other non-Xen people too.

The problem with that is the conditional nature of these instructions.
We are guaranteed to have them because LPAE and/or the virtualisation
extensions require them, but other ARMv7 implementations may not have
this.

> >  static inline int test_evtchn(int port)
> >  {
> >     struct shared_info *s = HYPERVISOR_shared_info;
> > -   return sync_test_bit(port, &s->evtchn_pending[0]);
> > +   return sync_test_bit(port, BM(&s->evtchn_pending[0]));
> >  }
> >  
> 
> Are you sure that the implementation of sync_test_bit, sync_set_bit,
> etc, can cope with a bit > 32?
> For example ____atomic_set_bit blatantly (bit & 31) at the beginning of
> the function.

I must confess I thought that these bit mask functions were designed to
work on arbitrary sized bitmaps. I was so sure I didn't even check...

        static inline void ____atomic_set_bit(unsigned int bit, volatile 
unsigned long *p)
        {
                unsigned long flags;
                unsigned long mask = 1UL << (bit & 31);
        
                p += bit >> 5;
        
                raw_local_irq_save(flags);
                *p |= mask;
                raw_local_irq_restore(flags);
        }
        
This is OK, it figures out the mask for the word containing @bit and
then increments @p to point to that word. The other bit ops are similar.

I think David V is right to be concerned about __ffs though, I'll need
to investigate that one further.

Ian.


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