On 12/17/20 1:18 PM, Claudio Imbrenda wrote:
> On Fri, 11 Dec 2020 05:00:34 -0500
> Janosch Frank <frankja@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
>> ---
>> 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?
Leftover from last version
>
>> #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)
This is the way.
Thomas already asked me that. I had a look what the kernel does and
that's what they are doing. QEMU writes the same feature bits to all
cpus and I haven't found an explanation for that code yet but I figured
there might (have) be(en) one.
>
>> + }
>> + }
>> +}
>> +
>> /* 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);
>
