Re: [qemu-s390x] [PATCH v2] target/s390x: Remove non-architected entries from struct LowCore

[David Hildenbrand]

On 05.03.19 08:14, Thomas Huth wrote:
> There are some fields in our struct LowCore which apparently have
> been copied from a very old version of the Linux kernel. These
> fields are not architected in the "Principles of Operation", and
> only used on these memory locations in Linux kernels older than
> 2.6.29. Newer Linux kernels moved the entries to different locations
> or are not using them at all anymore. Thus we should never access
> these fields from the QEMU side, so they should be removed.
> 
> While we're at it, also add a QEMU_BUILD_BUG_MSG() statement to
> assert that struct LowCore has the right size.
> 
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  v2: Now really added the QEMU_BUILD_BUG_MSG()
> 
>  target/s390x/internal.h | 41 ++---------------------------------------
>  1 file changed, 2 insertions(+), 39 deletions(-)
> 
> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> index 7baf0e2..84ce090 100644
> --- a/target/s390x/internal.h
> +++ b/target/s390x/internal.h
> @@ -63,45 +63,7 @@ typedef struct LowCore {
>      PSW             program_new_psw;          /* 0x1d0 */
>      PSW             mcck_new_psw;             /* 0x1e0 */
>      PSW             io_new_psw;               /* 0x1f0 */
> -    PSW             return_psw;               /* 0x200 */
> -    uint8_t         irb[64];                  /* 0x210 */
> -    uint64_t        sync_enter_timer;         /* 0x250 */
> -    uint64_t        async_enter_timer;        /* 0x258 */
> -    uint64_t        exit_timer;               /* 0x260 */
> -    uint64_t        last_update_timer;        /* 0x268 */
> -    uint64_t        user_timer;               /* 0x270 */
> -    uint64_t        system_timer;             /* 0x278 */
> -    uint64_t        last_update_clock;        /* 0x280 */
> -    uint64_t        steal_clock;              /* 0x288 */
> -    PSW             return_mcck_psw;          /* 0x290 */
> -    uint8_t         pad9[0xc00 - 0x2a0];      /* 0x2a0 */
> -    /* System info area */
> -    uint64_t        save_area[16];            /* 0xc00 */
> -    uint8_t         pad10[0xd40 - 0xc80];     /* 0xc80 */
> -    uint64_t        kernel_stack;             /* 0xd40 */
> -    uint64_t        thread_info;              /* 0xd48 */
> -    uint64_t        async_stack;              /* 0xd50 */
> -    uint64_t        kernel_asce;              /* 0xd58 */
> -    uint64_t        user_asce;                /* 0xd60 */
> -    uint64_t        panic_stack;              /* 0xd68 */
> -    uint64_t        user_exec_asce;           /* 0xd70 */
> -    uint8_t         pad11[0xdc0 - 0xd78];     /* 0xd78 */
> -
> -    /* SMP info area: defined by DJB */
> -    uint64_t        clock_comparator;         /* 0xdc0 */
> -    uint64_t        ext_call_fast;            /* 0xdc8 */
> -    uint64_t        percpu_offset;            /* 0xdd0 */
> -    uint64_t        current_task;             /* 0xdd8 */
> -    uint32_t        softirq_pending;          /* 0xde0 */
> -    uint32_t        pad_0x0de4;               /* 0xde4 */
> -    uint64_t        int_clock;                /* 0xde8 */
> -    uint8_t         pad12[0xe00 - 0xdf0];     /* 0xdf0 */
> -
> -    /* 0xe00 is used as indicator for dump tools */
> -    /* whether the kernel died with panic() or not */
> -    uint32_t        panic_magic;              /* 0xe00 */
> -
> -    uint8_t         pad13[0x11b0 - 0xe04];    /* 0xe04 */
> +    uint8_t         pad13[0x11b0 - 0x200];    /* 0x200 */
>  
>      uint64_t        mcesad;                    /* 0x11B0 */
>  
> @@ -130,6 +92,7 @@ typedef struct LowCore {
>  
>      uint8_t         pad18[0x2000 - 0x1400];    /* 0x1400 */
>  } QEMU_PACKED LowCore;
> +QEMU_BUILD_BUG_MSG(sizeof(LowCore) != 8192, "size of LowCore");

I don't consider this message any useful. It is literally self
documenting code what we have here.

Anyhow,

Reviewed-by: David Hildenbrand <address@hidden>


-- 

Thanks,

David / dhildenb