[patch] make evtchn_upcall_pending arch-specific type

[patch] make evtchn_upcall_pending arch-specific type

[Hollis Blanchard]

evtchn_upcall_pending is another variable that bitops are used on, so PowerPC 
wants it to be a long. Unfortunately, it's also part of the guest/hypervisor 
interface, so we can't change it for architectures with existing guests.

Compile-tested on x86(-32). Please apply.

-- 
Hollis Blanchard
IBM Linux Technology Center

Have each architecture specify the type of vcpu_info->evtchn_upcall_pending.

Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff -r d102a30417a7 xen/common/keyhandler.c
--- a/xen/common/keyhandler.c	Wed Mar 29 16:50:59 2006 +0100
+++ b/xen/common/keyhandler.c	Thu Mar 30 13:07:56 2006 -0600
@@ -144,11 +144,11 @@ static void dump_domains(unsigned char k
                d->domain_id);
         for_each_vcpu ( d, v ) {
             printk("    VCPU%d: CPU%d [has=%c] flags=%lx "
-                   "upcall_pend = %02x, upcall_mask = %02x ",
+                   "upcall_pend = %02lx, upcall_mask = %02x ",
                    v->vcpu_id, v->processor,
                    test_bit(_VCPUF_running, &v->vcpu_flags) ? 'T':'F',
                    v->vcpu_flags,
-                   v->vcpu_info->evtchn_upcall_pending, 
+                   (ulong)v->vcpu_info->evtchn_upcall_pending, 
                    v->vcpu_info->evtchn_upcall_mask);
             cpuset_print(cpuset, sizeof(cpuset), v->vcpu_dirty_cpumask);
             printk("dirty_cpus=%s ", cpuset);
diff -r d102a30417a7 xen/include/public/arch-ia64.h
--- a/xen/include/public/arch-ia64.h	Wed Mar 29 16:50:59 2006 +0100
+++ b/xen/include/public/arch-ia64.h	Thu Mar 30 13:07:56 2006 -0600
@@ -322,6 +322,8 @@ typedef struct vcpu_guest_context {
 } vcpu_guest_context_t;
 DEFINE_GUEST_HANDLE(vcpu_guest_context_t);
 
+typedef uint8_t pending_upcall_t;
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __HYPERVISOR_IF_IA64_H__ */
diff -r d102a30417a7 xen/include/public/arch-x86_32.h
--- a/xen/include/public/arch-x86_32.h	Wed Mar 29 16:50:59 2006 +0100
+++ b/xen/include/public/arch-x86_32.h	Thu Mar 30 13:07:56 2006 -0600
@@ -168,6 +168,8 @@ typedef struct {
     unsigned long pad[5]; /* sizeof(vcpu_info_t) == 64 */
 } arch_vcpu_info_t;
 
+typedef uint8_t pending_upcall_t;
+
 #endif /* !__ASSEMBLY__ */
 
 /*
diff -r d102a30417a7 xen/include/public/arch-x86_64.h
--- a/xen/include/public/arch-x86_64.h	Wed Mar 29 16:50:59 2006 +0100
+++ b/xen/include/public/arch-x86_64.h	Thu Mar 30 13:07:56 2006 -0600
@@ -244,6 +244,8 @@ typedef struct {
     unsigned long pad; /* sizeof(vcpu_info_t) == 64 */
 } arch_vcpu_info_t;
 
+typedef uint8_t pending_upcall_t;
+
 #endif /* !__ASSEMBLY__ */
 
 /*
diff -r d102a30417a7 xen/include/public/xen.h
--- a/xen/include/public/xen.h	Wed Mar 29 16:50:59 2006 +0100
+++ b/xen/include/public/xen.h	Thu Mar 30 13:07:56 2006 -0600
@@ -312,7 +312,7 @@ typedef struct vcpu_info {
      * an upcall activation. The mask is cleared when the VCPU requests
      * to block: this avoids wakeup-waiting races.
      */
-    uint8_t evtchn_upcall_pending;
+    pending_upcall_t evtchn_upcall_pending;
     uint8_t evtchn_upcall_mask;
     unsigned long evtchn_pending_sel;
     arch_vcpu_info_t arch;

Re: [patch] make evtchn_upcall_pending arch-specific type

[Keir Fraser]

On 30 Mar 2006, at 20:13, Hollis Blanchard wrote:

> evtchn_upcall_pending is another variable that bitops are used on, so 
> PowerPC
> wants it to be a long. Unfortunately, it's also part of the 
> guest/hypervisor
> interface, so we can't change it for architectures with existing 
> guests.
>
> Compile-tested on x86(-32). Please apply.

Before going down this route, what about just casting the field to 
long, since it is actually aligned on a suitable boundary, as it 
happens? And what will your solution be for fields in grant_entry 
structure? That's another one where the fields to be atomically updated 
are at least 8-byte aligned, and where using longer types will bloat a 
structure that we'd prefer to pack nicely.

If this is the best way for ppc then I think atomic_bit_t would be a 
nicer typedef.

  -- Keir

Re: [patch] make evtchn_upcall_pending arch-specific type

[Jimi Xenidis]

On Mar 31, 2006, at 7:46 AM, Keir Fraser wrote:

>
> On 30 Mar 2006, at 20:13, Hollis Blanchard wrote:
>
>> evtchn_upcall_pending is another variable that bitops are used on,  
>> so PowerPC
>> wants it to be a long. Unfortunately, it's also part of the guest/ 
>> hypervisor
>> interface, so we can't change it for architectures with existing  
>> guests.
>>
>> Compile-tested on x86(-32). Please apply.
>
> Before going down this route, what about just casting the field to  
> long, since it is actually aligned on a suitable boundary, as it  
> happens?

We tried that, but to get the right bit we would have to use 56 not 0.
So this brings me to a possible solution:
Since both evtchn_upcall_pending and mask are paired and aligned and  
padded to long we could group them together, like:
	unsigned long evtchn_upcall_bits;
Then:
	#define EVTCHN_UPCALL_PENDING 0
	#define EVTCHN_UPCALL_MASK 8

use the macros to define the bit arg of the bitop_*().
I chose these values because they would be completely compatible with  
any assembler that exist for the itty bitty byte arches. :)  As for  
PPC the values don't matter to us, at this early stage.

We could also get into magical union/structs with more abstracted  
interfaces, but the above is rather simple.


> And what will your solution be for fields in grant_entry structure?
yeah, grant_entry.flags will be a pain because it is 16 bit and you  
also use cmpxchg() on it.
I suppose we could abstract those bit changes, but yeah, this one is  
gonna hurt :(

> That's another one where the fields to be atomically updated are at  
> least 8-byte aligned, and where using longer types will bloat a  
> structure that we'd prefer to pack nicely.
The I'd rather bloat (for PPC only) then come up with some nasty read/ 
calculate-the-actual-bit-and-modify/write.

>
> If this is the best way for ppc then I think atomic_bit_t would be  
> a nicer typedef.

I think a context specific typedef would be better, in most cases.
Also,
   - I'd like to see more use of DECLARE_BITMAP() for stuff like  
pirq_mask
   - more use of BITS_PER_LONG instead of (sizeof(long)*8) that  
occurs in many places like evtchn_pending[]

-JX

Re: [patch] make evtchn_upcall_pending arch-specific type

[Keir Fraser]

On 1 Apr 2006, at 04:55, Jimi Xenidis wrote:

> 	unsigned long evtchn_upcall_bits;
> Then:
> 	#define EVTCHN_UPCALL_PENDING 0
> 	#define EVTCHN_UPCALL_MASK 8
>
> use the macros to define the bit arg of the bitop_*().
> I chose these values because they would be completely compatible with 
> any assembler that exist for the itty bitty byte arches. :)  As for 
> PPC the values don't matter to us, at this early stage.

We'd have to group more than just the mask and pending flags into that 
long on x86 as otherwise we change the size of a public structure.

>> That's another one where the fields to be atomically updated are at 
>> least 8-byte aligned, and where using longer types will bloat a 
>> structure that we'd prefer to pack nicely.
> The I'd rather bloat (for PPC only) then come up with some nasty 
> read/calculate-the-actual-bit-and-modify/write.

Okay, I don't think it's so bad really, except you may want to round 
the structure size up to the next power of two (potentially) and that 
may halve MAX_VIRT_CPUS for you until we support allocating extra pages 
for higher order vCPU infos.

>>
>> If this is the best way for ppc then I think atomic_bit_t would be a 
>> nicer typedef.
>
> I think a context specific typedef would be better, in most cases.

Well, I'm not too fussed. I expect atomic_bit_t would only get used in 
this one place ever anyway.

> Also,
>   - I'd like to see more use of DECLARE_BITMAP() for stuff like 
> pirq_mask
>   - more use of BITS_PER_LONG instead of (sizeof(long)*8) that occurs 
> in many places like evtchn_pending[]

Yes, for sure.

  -- Keir

Re: [patch] make evtchn_upcall_pending arch-specific type

[Jimi Xenidis]

On Apr 1, 2006, at 4:11 AM, Keir Fraser wrote:

>
> On 1 Apr 2006, at 04:55, Jimi Xenidis wrote:
>
>> 	unsigned long evtchn_upcall_bits;
>
> We'd have to group more than just the mask and pending flags into  
> that long on x86 as otherwise we change the size of a public  
> structure.

No size change, those two elements are padded to a long to  
accommodate the size and alignment of evtchn_pending_sel; so the size  
remains the same.

-JX

Re: [patch] make evtchn_upcall_pending arch-specific type

[Keir Fraser]

On 1 Apr 2006, at 20:45, Jimi Xenidis wrote:

>>> 	unsigned long evtchn_upcall_bits;
>>
>> We'd have to group more than just the mask and pending flags into 
>> that long on x86 as otherwise we change the size of a public 
>> structure.
>
> No size change, those two elements are padded to a long to accommodate 
> the size and alignment of evtchn_pending_sel; so the size remains the 
> same.

Well that is true. I'd still like to see how big the patch would be to 
hide accesses of the pending flag behind a macro, and how clean that 
would end up being.

There maybe also be the issue of non-atomic updates of the mask flag, 
but that mostly only happens in xenlinux. IA64 and x86 can update 
distinct bytes separately and atomically, but I'm not sure if powerpc 
guarantees writes are atomic at byte granularity? That flag gets 
touched quite a lot when running xenlinux, so we wouldn't want to make 
the accesses anything other than ordinary writes on x86 and ia64. 
That's one of the benefits for us of currently defining the mask flag 
in a separate uint8_t.

  -- Keir

Re: [patch] make evtchn_upcall_pending arch-specific type

[Keir Fraser]

On 1 Apr 2006, at 04:55, Jimi Xenidis wrote:

>> Before going down this route, what about just casting the field to 
>> long, since it is actually aligned on a suitable boundary, as it 
>> happens?
>
> We tried that, but to get the right bit we would have to use 56 not 0.

Actually, evtchn_upcall_pending is touched in very few places in Xen 
common code. Using bit 56 is not very pretty but should be easy to hide 
behind a macro? You can hide the cast to long inside the same macro. 
You already need to arch-dep the event_pending() macro, for your MSR.EE 
check, and that's one of the main ways in which common code accesses 
the pending flag.

Would that be nicer than adding another one-shot typedef in the public 
headers and bloating a struct on PPC?

  -- Keir

Re: [patch] make evtchn_upcall_pending arch-specific type

[Hollis Blanchard]

On Sat, 2006-04-01 at 10:30 +0100, Keir Fraser wrote:
> On 1 Apr 2006, at 04:55, Jimi Xenidis wrote:
> 
> >> Before going down this route, what about just casting the field to 
> >> long, since it is actually aligned on a suitable boundary, as it 
> >> happens?
> >
> > We tried that, but to get the right bit we would have to use 56 not 0.
> 
> Actually, evtchn_upcall_pending is touched in very few places in Xen 
> common code. Using bit 56 is not very pretty but should be easy to hide 
> behind a macro? You can hide the cast to long inside the same macro. 
> You already need to arch-dep the event_pending() macro, for your MSR.EE 
> check, and that's one of the main ways in which common code accesses 
> the pending flag.

For reference, here's the PPC implementation:

/* HACK: evtchn_upcall_pending is only a byte, but our atomic instructions only
 * store in 4/8 byte quantities. However, because evtchn_upcall_pending is part
 * of the guest ABI, we can't change its size without breaking backwards
 * compatibility. In this particular case, struct vcpu_info is big enough that
 * we can safely store a full long into it. However, note that bit 0 of that
 * byte is bit 56 when cast to a long.
 */
static inline int evtchn_test_and_set_pending(struct vcpu *v)
{
    unsigned long *l = (unsigned long *)&v->vcpu_info->evtchn_upcall_pending;
    return test_and_set_bit(56, l);
}

You'll notice I added an alignment attribute to that field because it
really really needs to be aligned for us. This has only been
build-tested on x86!

If it's acceptable, please apply.

Hide evtchn_upcall_pending test-and-set accesses behind a wrapper.
Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

diff -r aa088739693a xen/common/event_channel.c
--- a/xen/common/event_channel.c	Tue Jun 13 10:48:08 2006 -0500
+++ b/xen/common/event_channel.c	Tue Jun 13 14:52:05 2006 -0500
@@ -494,7 +494,7 @@ void evtchn_set_pending(struct vcpu *v, 
     if ( !test_bit        (port, s->evtchn_mask) &&
          !test_and_set_bit(port / BITS_PER_LONG,
                            &v->vcpu_info->evtchn_pending_sel) &&
-         !test_and_set_bit(0, &v->vcpu_info->evtchn_upcall_pending) )
+         !evtchn_test_and_set_pending(v) )
     {
         evtchn_notify(v);
     }
@@ -683,7 +683,7 @@ static long evtchn_unmask(evtchn_unmask_
          test_bit          (port, s->evtchn_pending) &&
          !test_and_set_bit (port / BITS_PER_LONG,
                             &v->vcpu_info->evtchn_pending_sel) &&
-         !test_and_set_bit (0, &v->vcpu_info->evtchn_upcall_pending) )
+         !evtchn_test_and_set_pending(v) )
     {
         evtchn_notify(v);
     }
diff -r aa088739693a xen/include/asm-ia64/event.h
--- a/xen/include/asm-ia64/event.h	Tue Jun 13 10:48:08 2006 -0500
+++ b/xen/include/asm-ia64/event.h	Tue Jun 13 14:52:05 2006 -0500
@@ -74,4 +74,9 @@ static inline int arch_virq_is_global(in
     return rc;
 }
 
+static inline int evtchn_test_and_set_pending(struct vcpu *v)
+{
+    return test_and_set_bit(0, &v->vcpu_info->evtchn_upcall_pending);
+}
+
 #endif
diff -r aa088739693a xen/include/asm-x86/event.h
--- a/xen/include/asm-x86/event.h	Tue Jun 13 10:48:08 2006 -0500
+++ b/xen/include/asm-x86/event.h	Tue Jun 13 14:52:05 2006 -0500
@@ -55,4 +55,9 @@ static inline int arch_virq_is_global(in
     return 1;
 }
 
+static inline int evtchn_test_and_set_pending(struct vcpu *v)
+{
+    return test_and_set_bit(0, &v->vcpu_info->evtchn_upcall_pending);
+}
+
 #endif
diff -r aa088739693a xen/include/public/xen.h
--- a/xen/include/public/xen.h	Tue Jun 13 10:48:08 2006 -0500
+++ b/xen/include/public/xen.h	Tue Jun 13 14:52:05 2006 -0500
@@ -363,7 +363,7 @@ struct vcpu_info {
      * an upcall activation. The mask is cleared when the VCPU requests
      * to block: this avoids wakeup-waiting races.
      */
-    uint8_t evtchn_upcall_pending;
+    uint8_t evtchn_upcall_pending __attribute__((aligned(sizeof(long))));
     uint8_t evtchn_upcall_mask;
     unsigned long evtchn_pending_sel;
     struct arch_vcpu_info arch;


-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [patch] make evtchn_upcall_pending arch-specific type

[Keir Fraser]

On 13 Jun 2006, at 21:02, Hollis Blanchard wrote:

> You'll notice I added an alignment attribute to that field because it
> really really needs to be aligned for us. This has only been
> build-tested on x86!
>
> If it's acceptable, please apply.
>
> Hide evtchn_upcall_pending test-and-set accesses behind a wrapper.
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

It would probably be better to move the TAS of the upcall_pending flag 
into evtchn_notify(). Avoids expanding the arch-specific interface 
further.

We have avoided adding gcc extensions to core public header files as it 
prevents easy use of the header files by some other projects (e.g., 
solaris port, I believe). You should instead add a suitable 
BUILD_BUG_ON() in arch/ppc -- in any case, if field offsets change in 
future you break backward compatibility, even if new field alignment 
does happen to still be 'okay' in that one case.

  -- Keir