diff mbox

[PULL,03/10] pc-bios/s390-ccw: handle different sector sizes

Message ID 53AD5B9D.7030400@de.ibm.com
State New
Headers show

Commit Message

Christian Borntraeger June 27, 2014, 11:55 a.m. UTC 
On 27/06/14 13:45, Alexander Graf wrote:
> 
> On 27.06.14 13:25, Cornelia Huck wrote:
>> From: "Eugene (jno) Dvurechenski" <jno@linux.vnet.ibm.com>
>>
>> Use the virtio device's configuration to figure out the disk geometry
>> and use a sector size based upon the layout.
>>
>> [CH: s/SECTOR_SIZE/MAX_SECTOR_SIZE/g]
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Eugene (jno) Dvurechenski <jno@linux.vnet.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> ---
>>   pc-bios/s390-ccw/bootmap.c  |   12 +++---
>>   pc-bios/s390-ccw/s390-ccw.h |    2 +-
>>   pc-bios/s390-ccw/virtio.c   |   96 ++++++++++++++++++++++++++++++++++++++++---
>>   pc-bios/s390-ccw/virtio.h   |   48 ++++++++++++++++++++++
>>   4 files changed, 147 insertions(+), 11 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index c216030..fa2ca26 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -10,6 +10,7 @@
>>     #include "s390-ccw.h"
>>   #include "bootmap.h"
>> +#include "virtio.h"
>>     /* #define DEBUG_FALLBACK */
>>   @@ -22,7 +23,8 @@
>>   #endif
>>     /* Scratch space */
>> -static uint8_t sec[SECTOR_SIZE] __attribute__((__aligned__(SECTOR_SIZE)));
>> +static uint8_t sec[MAX_SECTOR_SIZE]
>> +__attribute__((__aligned__(MAX_SECTOR_SIZE)));
>>     typedef struct ResetInfo {
>>       uint32_t ipl_mask;
>> @@ -99,7 +101,7 @@ static inline bool unused_space(const void *p, unsigned int size)
>>     static int zipl_load_segment(ComponentEntry *entry)
>>   {
>> -    const int max_entries = (SECTOR_SIZE / sizeof(ScsiBlockPtr));
>> +    const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
> 
> Is this really safe to increase? Doesn't max_entries depend on the real sector size?

I think this is now covered by this if statement:
            if (bprs[i].blockct == 0 && unused_space(&bprs[i + 1],
                sizeof(ScsiBlockPtr))) {

which was introduced by commit c77cd87cf54f003748f29c14ea1ddaecfc5c653f (pc-bios/s390-ccw: fix for fragmented SCSI bootmap).

So strictly speaking this if statement might not be needed any more:
        if (i == (max_entries - 1)) {

Eugene, can you confirm?  If yes we could add this patch later on as a cleanup:

Comments

Eugene \"jno\" Dvurechenski June 27, 2014, 1:04 p.m. UTC | #1 
On 06/27/2014 03:55 PM, Christian Borntraeger wrote:
>>> -    const int max_entries = (SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>> +    const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>
>> Is this really safe to increase? Doesn't max_entries depend on the real sector size?
> 
> I think this is now covered by this if statement:
>             if (bprs[i].blockct == 0 && unused_space(&bprs[i + 1],
>                 sizeof(ScsiBlockPtr))) {
> 
> which was introduced by commit c77cd87cf54f003748f29c14ea1ddaecfc5c653f (pc-bios/s390-ccw: fix for fragmented SCSI bootmap).
> 
> So strictly speaking this if statement might not be needed any more:
>         if (i == (max_entries - 1)) {
> 
> Eugene, can you confirm?  If yes we could add this patch later on as a cleanup:

I'd preserve both checks.
In theory, we may catch a table that consumes all scratch space and
leave no unused entry.

Plus, this check for zero counter and last entry is for "continuation"
pointer, not for end-of-table by itself.

I think now, this code may need even few more checks to cover more cases...
Christian Borntraeger June 27, 2014, 1:11 p.m. UTC | #2 
On 27/06/14 15:04, Eugene "jno" Dvurechenski wrote:
> 
> 
> On 06/27/2014 03:55 PM, Christian Borntraeger wrote:
>>>> -    const int max_entries = (SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>>> +    const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>>
>>> Is this really safe to increase? Doesn't max_entries depend on the real sector size?
>>
>> I think this is now covered by this if statement:
>>             if (bprs[i].blockct == 0 && unused_space(&bprs[i + 1],
>>                 sizeof(ScsiBlockPtr))) {
>>
>> which was introduced by commit c77cd87cf54f003748f29c14ea1ddaecfc5c653f (pc-bios/s390-ccw: fix for fragmented SCSI bootmap).
>>
>> So strictly speaking this if statement might not be needed any more:
>>         if (i == (max_entries - 1)) {
>>
>> Eugene, can you confirm?  If yes we could add this patch later on as a cleanup:
> 
> I'd preserve both checks.
> In theory, we may catch a table that consumes all scratch space and
> leave no unused entry.
> 
> Plus, this check for zero counter and last entry is for "continuation"
> pointer, not for end-of-table by itself.
> 
> I think now, this code may need even few more checks to cover more cases...
> 
Ok. That means, that this patch as is, doesnt make anything worse. Correct?

I am expecting more fixes and cleanups for the bios code anyway, so as long as we dont add a regression here this should be good to go as it makes the whole code more flexible.

Christian
Eugene \"jno\" Dvurechenski June 27, 2014, 2:36 p.m. UTC | #3 
Yes, this patch doesn't make the code worse.

On 06/27/2014 05:11 PM, Christian Borntraeger wrote:
> On 27/06/14 15:04, Eugene "jno" Dvurechenski wrote:
>>
>>
>> On 06/27/2014 03:55 PM, Christian Borntraeger wrote:
>>>>> -    const int max_entries = (SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>>>> +    const int max_entries = (MAX_SECTOR_SIZE / sizeof(ScsiBlockPtr));
>>>>
>>>> Is this really safe to increase? Doesn't max_entries depend on the real sector size?
>>>
>>> I think this is now covered by this if statement:
>>>             if (bprs[i].blockct == 0 && unused_space(&bprs[i + 1],
>>>                 sizeof(ScsiBlockPtr))) {
>>>
>>> which was introduced by commit c77cd87cf54f003748f29c14ea1ddaecfc5c653f (pc-bios/s390-ccw: fix for fragmented SCSI bootmap).
>>>
>>> So strictly speaking this if statement might not be needed any more:
>>>         if (i == (max_entries - 1)) {
>>>
>>> Eugene, can you confirm?  If yes we could add this patch later on as a cleanup:
>>
>> I'd preserve both checks.
>> In theory, we may catch a table that consumes all scratch space and
>> leave no unused entry.
>>
>> Plus, this check for zero counter and last entry is for "continuation"
>> pointer, not for end-of-table by itself.
>>
>> I think now, this code may need even few more checks to cover more cases...
>>
> Ok. That means, that this patch as is, doesnt make anything worse. Correct?
> 
> I am expecting more fixes and cleanups for the bios code anyway, so as long as we dont add a regression here this should be good to go as it makes the whole code more flexible.
> 
> Christian
>
diff mbox

Patch

--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -303,7 +303,6 @@  static void ipl_eckd(ECKD_IPL_mode_t mode)
 
 static void zipl_load_segment(ComponentEntry *entry)
 {
-    const int max_entries = (SECTOR_SIZE / sizeof(ScsiBlockPtr));
     ScsiBlockPtr *bprs = (void *)sec;
     const int bprs_size = sizeof(sec);
     block_number_t blockno;
@@ -331,12 +330,6 @@  static void zipl_load_segment(ComponentEntry *entry)
                 break;
             }
 
-            /* we need the updated blockno for the next indirect entry in the
-               chain, but don't want to advance address */
-            if (i == (max_entries - 1)) {
-                break;
-            }
-
             if (bprs[i].blockct == 0 && unused_space(&bprs[i + 1],
                 sizeof(ScsiBlockPtr))) {
                 /* This is a "continue" pointer.