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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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:


--- 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.