Re: [kvm-unit-tests PATCH v3 3/8] s390x: SCLP feature checking

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri, 11 Dec 2020 05:00:34 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Availability of SIE is announced via a feature bit in a SCLP info CPU
> entry. Let's add a framework that allows us to easily check for such
> facilities.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  lib/s390x/io.c   |  1 +
>  lib/s390x/sclp.c | 19 +++++++++++++++++++
>  lib/s390x/sclp.h | 13 ++++++++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/s390x/io.c b/lib/s390x/io.c
> index 6a1da63..ef9f59e 100644
> --- a/lib/s390x/io.c
> +++ b/lib/s390x/io.c
> @@ -35,6 +35,7 @@ void setup(void)
>  	setup_args_progname(ipl_args);
>  	setup_facilities();
>  	sclp_read_info();
> +	sclp_facilities_setup();
>  	sclp_console_setup();
>  	sclp_memory_setup();
>  	smp_setup();
> diff --git a/lib/s390x/sclp.c b/lib/s390x/sclp.c
> index bf1d9c0..cf6ea7c 100644
> --- a/lib/s390x/sclp.c
> +++ b/lib/s390x/sclp.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <libcflat.h>
> +#include <bitops.h>

you add this include, but it seems you are not actually using it?

>  #include <asm/page.h>
>  #include <asm/arch_def.h>
>  #include <asm/interrupt.h>
> @@ -25,6 +26,7 @@ static uint64_t max_ram_size;
>  static uint64_t ram_size;
>  char _read_info[PAGE_SIZE] __attribute__((__aligned__(4096)));
>  static ReadInfo *read_info;
> +struct sclp_facilities sclp_facilities;
>  
>  char _sccb[PAGE_SIZE] __attribute__((__aligned__(4096)));
>  static volatile bool sclp_busy;
> @@ -128,6 +130,23 @@ CPUEntry *sclp_get_cpu_entries(void)
>  	return (void *)read_info + read_info->offset_cpu;
>  }
>  
> +void sclp_facilities_setup(void)
> +{
> +	unsigned short cpu0_addr = stap();
> +	CPUEntry *cpu;
> +	int i;
> +
> +	assert(read_info);
> +
> +	cpu = (void *)read_info + read_info->offset_cpu;

another void* arithmetic. consider using well-defined constructs, like

cpu = (CPUEntry *)(_read_info + read_info->offset_cpu);

> +	for (i = 0; i < read_info->entries_cpu; i++, cpu++) {
> +		if (cpu->address == cpu0_addr) {
> +			sclp_facilities.has_sief2 = cpu->feat_sief2;
> +			break;

this only checks CPU 0. I wonder if you shouldn't check all CPUs? Or if
we assume that all CPUs have the same facilities, isn't it enough to
check the first CPU in the list? (i.e. avoid the loop)

> +		}
> +	}
> +}
> +
>  /* Perform service call. Return 0 on success, non-zero otherwise. */
>  int sclp_service_call(unsigned int command, void *sccb)
>  {
> diff --git a/lib/s390x/sclp.h b/lib/s390x/sclp.h
> index acd86d5..6c86037 100644
> --- a/lib/s390x/sclp.h
> +++ b/lib/s390x/sclp.h
> @@ -92,12 +92,22 @@ typedef struct SCCBHeader {
>  typedef struct CPUEntry {
>      uint8_t address;
>      uint8_t reserved0;
> -    uint8_t features[SCCB_CPU_FEATURE_LEN];
> +    uint8_t : 4;
> +    uint8_t feat_sief2 : 1;
> +    uint8_t : 3;
> +    uint8_t features_res2 [SCCB_CPU_FEATURE_LEN - 1];
>      uint8_t reserved2[6];
>      uint8_t type;
>      uint8_t reserved1;
>  } __attribute__((packed)) CPUEntry;
>  
> +extern struct sclp_facilities sclp_facilities;
> +
> +struct sclp_facilities {
> +	uint64_t has_sief2 : 1;
> +	uint64_t : 63;
> +};
> +
>  typedef struct ReadInfo {
>      SCCBHeader h;
>      uint16_t rnmax;
> @@ -271,6 +281,7 @@ void sclp_print(const char *str);
>  void sclp_read_info(void);
>  int sclp_get_cpu_num(void);
>  CPUEntry *sclp_get_cpu_entries(void);
> +void sclp_facilities_setup(void);
>  int sclp_service_call(unsigned int command, void *sccb);
>  void sclp_memory_setup(void);
>  uint64_t get_ram_size(void);