Re: shrink ci_ilevel from 32 to 8 bits ?

[Jason Thorpe <thorpej%me.com@localhost>]

> On Apr 11, 2020, at 1:25 PM, Manuel Bouyer <bouyer%antioche.eu.org@localhost> wrote:
> 
> Hello,
> at some point, the x86's cpu_info was changed to pack the istate in a
> structure:
>        struct {
>                uint32_t        ipending;
>                int             ilevel;
>                uint32_t        imasked;
>        } ci_istate __aligned(8);
> #define ci_ipending     ci_istate.ipending
> #define ci_ilevel       ci_istate.ilevel
> #define ci_imasked      ci_istate.imasked
> 
> The point was to be able to change ipending and ilevel together
> atomically, with cmpxchg8b. This is used in spllower() and mutex_spin_exit().
> This avoid using sti/cli which is more expansive.
> 
> For Xen, a second interrupt pending integer exists, ci_xpending. It is
> separate because for PVHVM, and probably PVH, we need both.
> we can't use the native ipending mechanism because 32 events are really not
> enough, especially for dom0. It is not a pending interrupt maks, but
> a pending IPL mask.

Can you please give the field a better name to indicate this (or, at the very least, a comment)?

> With now, amd64's mutex_spin_exit checks the native pending interrupts
> and resets ilevel using cmpxchg8b, then checks xpending. This is racy.
> Also, as the goal is to get PVHVM support back in GENERIC, we can't really
> go back to sti/cli.
> 
> So I'ld like to change ci_istate to
>        struct {
>                uint32_t        ipending;
>                int8_t          ilevel;
>                uint8_t         xpending;
>                int16_t         ipad; /* free bits for futur use */
>                uint32_t        imasked;
>        } ci_istate __aligned(8);
> #define ci_ipending     ci_istate.ipending
> #define ci_ipendin     ci_istate.xpending

I guess you mean ci_xpending here?

> #define ci_ilevel       ci_istate.ilevel
> #define ci_imasked      ci_istate.imasked
> 
> and change the assembly code to access ilevel as byte instead of long
> (e.g. with movsbl instead of movl).
> 
> Does anyone see a problem with this approach ?

That seems reasonable, though I'm not an expert.

-- thorpej