All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: "Luck, Tony" <tony.luck@intel.com>, Borislav Petkov <bp@alien8.de>
Cc: Yazen Ghannam <Yazen.Ghannam@amd.com>, X86 ML <x86@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/4] x86/mce: Get rid of machine_check_vector
Date: Mon, 20 Sep 2021 09:42:22 +0200	[thread overview]
Message-ID: <5eb3ac0a-4887-08b2-82fa-0348e04ace95@rasmusvillemoes.dk> (raw)
In-Reply-To: <YUgUpXHciLMn4X20@agluck-desk2.amr.corp.intel.com>

On 20/09/2021 06.57, Luck, Tony wrote:
> On Fri, Sep 17, 2021 at 12:53:53PM +0200, Borislav Petkov wrote:
>> @@ -126,7 +123,9 @@ struct mca_config {
>>  	      ser			: 1,
>>  	      recovery			: 1,
>>  	      bios_cmci_threshold	: 1,
>> -	      __reserved		: 59;
>> +	      /* Proper #MC exception handler is set */
>> +	      initialized		: 1,
>> +	      __reserved		: 58;
> 
> Does this __reserved field do anything useful? It seems to
> just be an annoyance that must be updated each time a new
> bit is added. Surely the compiler will see that these bitfields
> are in a "u64" and do the math and skip to the right boundary
> without this.

Not at all. And it also seems that the current layout is not at all what
may have been intended (the bit fields do not start at an 8-byte boundary).

$ cat a.c
#include <string.h>
#include <stdint.h>
struct s1 {
        char x;
        uint64_t a:1,
                 b:1,
                 c:1,
                 d:61;
        char y;
};
struct s2 {
        char x;
        uint64_t a:1,
                 b:1,
                 c:1;
        char y;
};
struct s3 {
        uint64_t x;
        uint64_t a:1,
                 b:1,
                 c:1;
        char y;
};
// some dummy functions to make the structs appear used and make gcc
// actually emit debug info
void f1(struct s1 *s) { memset(s, 0xff, sizeof(*s)); }
void f2(struct s2 *s) { memset(s, 0xff, sizeof(*s)); }
void f3(struct s3 *s) { memset(s, 0xff, sizeof(*s)); }
$ gcc -o a.o -c a.c -O2 -g
$ pahole a.o
struct s1 {
    char                x;                    /*     0     1 */

    /* Bitfield combined with previous fields */

    uint64_t            a:1;                  /*     0: 8  8 */
    uint64_t            b:1;                  /*     0: 9  8 */
    uint64_t            c:1;                  /*     0:10  8 */

    /* XXX 53 bits hole, try to pack */

    /* Force alignment to the next boundary: */
    uint64_t            :0;

    uint64_t            d:61;                 /*     8: 0  8 */

    /* XXX 3 bits hole, try to pack */

    char                y;                    /*    16     1 */

    /* size: 24, cachelines: 1, members: 6 */
    /* sum members: 2 */
    /* sum bitfield members: 64 bits, bit holes: 2, sum bit holes: 56
bits */
    /* padding: 7 */
    /* last cacheline: 24 bytes */
};
struct s2 {
    char                x;                    /*     0     1 */

    /* Bitfield combined with previous fields */

    uint64_t            a:1;                  /*     0: 8  8 */
    uint64_t            b:1;                  /*     0: 9  8 */
    uint64_t            c:1;                  /*     0:10  8 */

    /* XXX 5 bits hole, try to pack */
    /* Bitfield combined with next fields */

    char                y;                    /*     2     1 */

    /* size: 8, cachelines: 1, members: 5 */
    /* sum members: 2 */
    /* sum bitfield members: 3 bits, bit holes: 1, sum bit holes: 5 bits */
    /* padding: 5 */
    /* last cacheline: 8 bytes */
};
struct s3 {
    uint64_t            x;                    /*     0     8 */
    uint64_t            a:1;                  /*     8: 0  8 */
    uint64_t            b:1;                  /*     8: 1  8 */
    uint64_t            c:1;                  /*     8: 2  8 */

    /* XXX 5 bits hole, try to pack */
    /* Bitfield combined with next fields */

    char                y;                    /*     9     1 */

    /* size: 16, cachelines: 1, members: 5 */
    /* sum members: 9 */
    /* sum bitfield members: 3 bits, bit holes: 1, sum bit holes: 5 bits */
    /* padding: 6 */
    /* last cacheline: 16 bytes */
};

And, since in the concrete case mca_config just has four bool members
before the bitfields, we see that the 1-bit bitfields are put within the
first 8 bytes of the struct, while the __reserved field gets an entire
u64 all to itself:

struct mca_config {
    _Bool               dont_log_ce;          /*     0     1 */
    _Bool               cmci_disabled;        /*     1     1 */
    _Bool               ignore_ce;            /*     2     1 */
    _Bool               print_all;            /*     3     1 */

    /* Bitfield combined with previous fields */

    long long unsigned int     lmce_disabled:1;      /*     0:32  8 */
    long long unsigned int     disabled:1;    /*     0:33  8 */
    long long unsigned int     ser:1;         /*     0:34  8 */
    long long unsigned int     recovery:1;    /*     0:35  8 */
    long long unsigned int     bios_cmci_threshold:1; /*     0:36  8 */

    /* XXX 27 bits hole, try to pack */

    /* Force alignment to the next boundary: */
    long long unsigned int     :0;

    long long unsigned int     __reserved:59;        /*     8: 0  8 */

    /* XXX 5 bits hole, try to pack */

    signed char         bootlog;              /*    16     1 */

    /* XXX 3 bytes hole, try to pack */

    int                 tolerant;             /*    20     4 */
    int                 monarch_timeout;      /*    24     4 */
    int                 panic_timeout;        /*    28     4 */
    unsigned int        rip_msr;              /*    32     4 */

    /* size: 40, cachelines: 1, members: 15 */
    /* sum members: 21, holes: 1, sum holes: 3 */
    /* sum bitfield members: 64 bits, bit holes: 2, sum bit holes: 32
bits */
    /* padding: 4 */
    /* last cacheline: 40 bytes */
};

But why the messy mix between 1-bit bitfields and _Bools in the first place?

Rasmus

  reply	other threads:[~2021-09-20  7:42 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-17 10:53 [PATCH 0/4] x86/mce: Remove indirect calls Borislav Petkov
2021-09-17 10:53 ` [PATCH 1/4] x86/mce: Get rid of the mce_severity function pointer Borislav Petkov
2021-09-17 10:53 ` [PATCH 2/4] x86/mce: Get rid of machine_check_vector Borislav Petkov
2021-09-20  4:57   ` Luck, Tony
2021-09-20  7:42     ` Rasmus Villemoes [this message]
2021-09-20  8:15       ` Borislav Petkov
2021-09-20  8:12     ` Borislav Petkov
2021-09-20 16:04       ` Luck, Tony
2021-09-20 16:32         ` Borislav Petkov
2021-09-17 10:53 ` [PATCH 3/4] x86/mce: Get rid of msr_ops Borislav Petkov
2021-09-20  4:47   ` Luck, Tony
2021-09-20  8:32     ` Borislav Petkov
2021-09-22 12:16       ` Borislav Petkov
2021-09-22 13:23         ` Luck, Tony
2021-09-22 13:55           ` Borislav Petkov
2021-09-22 15:22             ` Luck, Tony
2021-09-22 15:57               ` Borislav Petkov
2021-09-17 10:53 ` [PATCH 4/4] x86/mce: Get rid of the ->quirk_no_way_out() indirect call Borislav Petkov
2021-09-20  5:06   ` Luck, Tony
2021-09-20  8:34     ` Borislav Petkov
2021-09-23 14:51   ` Yazen Ghannam
2021-09-23 15:11     ` Borislav Petkov
2021-09-24 20:04       ` Yazen Ghannam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5eb3ac0a-4887-08b2-82fa-0348e04ace95@rasmusvillemoes.dk \
    --to=linux@rasmusvillemoes.dk \
    --cc=Yazen.Ghannam@amd.com \
    --cc=bp@alien8.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH 2/4] x86/mce: Get rid of machine_check_vector' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.