diff mbox series

[v3,4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore

Message ID 20200527074920.43669-5-frankja@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series pc-bios: s390x: Cleanup part 1 | expand

Commit Message

Janosch Frank May 27, 2020, 7:49 a.m. UTC
If we have a lowcore struct that has members for offsets that we want
to touch, why not use it?

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 pc-bios/s390-ccw/cio.h  | 17 +++++++++++------
 pc-bios/s390-ccw/main.c |  8 +++-----
 2 files changed, 14 insertions(+), 11 deletions(-)

Comments

Christian Borntraeger May 27, 2020, 8:50 a.m. UTC | #1
On 27.05.20 09:49, Janosch Frank wrote:
> If we have a lowcore struct that has members for offsets that we want
> to touch, why not use it?
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  pc-bios/s390-ccw/cio.h  | 17 +++++++++++------
>  pc-bios/s390-ccw/main.c |  8 +++-----
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index aaa432dedd..1e5d4e92e1 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -122,12 +122,17 @@ typedef struct schib {
>  } __attribute__ ((packed, aligned(4))) Schib;
>  
>  typedef struct subchannel_id {
> -        __u32 cssid:8;
> -        __u32:4;
> -        __u32 m:1;
> -        __u32 ssid:2;
> -        __u32 one:1;
> -        __u32 sch_no:16;
> +    union {
> +        struct {
> +            __u16 cssid:8;
> +            __u16 reserved:4;
> +            __u16 m:1;
> +            __u16 ssid:2;
> +            __u16 one:1;
> +        };
> +        __u16 sch_id;
> +    };
> +    __u16 sch_no;
>  } __attribute__ ((packed, aligned(4))) SubChannelId;
>  >  struct chsc_header {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 4e65b411e1..8b912454c9 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>   */
>  void write_subsystem_identification(void)
>  {
> -    SubChannelId *schid = (SubChannelId *) 184;
> -    uint32_t *zeroes = (uint32_t *) 188;
> -
> -    *schid = blk_schid;
> -    *zeroes = 0;
> +    lowcore->subchannel_id = blk_schid.sch_id;
> +    lowcore->subchannel_nr = blk_schid.sch_no;
> +    lowcore->io_int_parm = 0;
>  }
>  
>  void write_iplb_location(void)

As an alternative, why not change the definition of Lowcore and replace both 16bit
fields lowcore->subchannel_id and lowcore->subchannel_nr with SubChannelID.
You can then simply assign lowcore->schid= blk_schid;

?
Janosch Frank May 27, 2020, 11:23 a.m. UTC | #2
On 5/27/20 10:50 AM, Christian Borntraeger wrote:
> 
> 
> On 27.05.20 09:49, Janosch Frank wrote:
>> If we have a lowcore struct that has members for offsets that we want
>> to touch, why not use it?
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>>  pc-bios/s390-ccw/cio.h  | 17 +++++++++++------
>>  pc-bios/s390-ccw/main.c |  8 +++-----
>>  2 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
>> index aaa432dedd..1e5d4e92e1 100644
>> --- a/pc-bios/s390-ccw/cio.h
>> +++ b/pc-bios/s390-ccw/cio.h
>> @@ -122,12 +122,17 @@ typedef struct schib {
>>  } __attribute__ ((packed, aligned(4))) Schib;
>>  
>>  typedef struct subchannel_id {
>> -        __u32 cssid:8;
>> -        __u32:4;
>> -        __u32 m:1;
>> -        __u32 ssid:2;
>> -        __u32 one:1;
>> -        __u32 sch_no:16;
>> +    union {
>> +        struct {
>> +            __u16 cssid:8;
>> +            __u16 reserved:4;
>> +            __u16 m:1;
>> +            __u16 ssid:2;
>> +            __u16 one:1;
>> +        };
>> +        __u16 sch_id;
>> +    };
>> +    __u16 sch_no;
>>  } __attribute__ ((packed, aligned(4))) SubChannelId;
>>  >  struct chsc_header {
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 4e65b411e1..8b912454c9 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>>   */
>>  void write_subsystem_identification(void)
>>  {
>> -    SubChannelId *schid = (SubChannelId *) 184;
>> -    uint32_t *zeroes = (uint32_t *) 188;
>> -
>> -    *schid = blk_schid;
>> -    *zeroes = 0;
>> +    lowcore->subchannel_id = blk_schid.sch_id;
>> +    lowcore->subchannel_nr = blk_schid.sch_no;
>> +    lowcore->io_int_parm = 0;
>>  }
>>  
>>  void write_iplb_location(void)
> 
> As an alternative, why not change the definition of Lowcore and replace both 16bit
> fields lowcore->subchannel_id and lowcore->subchannel_nr with SubChannelID.
> You can then simply assign lowcore->schid= blk_schid;
> 
> ?
> 
I think Pierre voted against that in the internal review if I remember
correctly.
diff mbox series

Patch

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index aaa432dedd..1e5d4e92e1 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -122,12 +122,17 @@  typedef struct schib {
 } __attribute__ ((packed, aligned(4))) Schib;
 
 typedef struct subchannel_id {
-        __u32 cssid:8;
-        __u32:4;
-        __u32 m:1;
-        __u32 ssid:2;
-        __u32 one:1;
-        __u32 sch_no:16;
+    union {
+        struct {
+            __u16 cssid:8;
+            __u16 reserved:4;
+            __u16 m:1;
+            __u16 ssid:2;
+            __u16 one:1;
+        };
+        __u16 sch_id;
+    };
+    __u16 sch_no;
 } __attribute__ ((packed, aligned(4))) SubChannelId;
 
 struct chsc_header {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 4e65b411e1..8b912454c9 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -36,11 +36,9 @@  LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
  */
 void write_subsystem_identification(void)
 {
-    SubChannelId *schid = (SubChannelId *) 184;
-    uint32_t *zeroes = (uint32_t *) 188;
-
-    *schid = blk_schid;
-    *zeroes = 0;
+    lowcore->subchannel_id = blk_schid.sch_id;
+    lowcore->subchannel_nr = blk_schid.sch_no;
+    lowcore->io_int_parm = 0;
 }
 
 void write_iplb_location(void)