From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49316) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0UkW-0008BI-74 for qemu-devel@nongnu.org; Fri, 27 Jun 2014 07:55:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0UkN-0000ur-5n for qemu-devel@nongnu.org; Fri, 27 Jun 2014 07:55:24 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:37930) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0UkM-0000uR-Pi for qemu-devel@nongnu.org; Fri, 27 Jun 2014 07:55:15 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 27 Jun 2014 12:55:13 +0100 Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 41A9A1B08075 for ; Fri, 27 Jun 2014 12:55:44 +0100 (BST) Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by b06cxnps3075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s5RBtBet34078830 for ; Fri, 27 Jun 2014 11:55:11 GMT Received: from d06av08.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s5RBtA6X011108 for ; Fri, 27 Jun 2014 05:55:11 -0600 Message-ID: <53AD5B9D.7030400@de.ibm.com> Date: Fri, 27 Jun 2014 13:55:09 +0200 From: Christian Borntraeger MIME-Version: 1.0 References: <1403868326-7718-1-git-send-email-cornelia.huck@de.ibm.com> <1403868326-7718-4-git-send-email-cornelia.huck@de.ibm.com> <53AD5947.2040704@suse.de> In-Reply-To: <53AD5947.2040704@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 03/10] pc-bios/s390-ccw: handle different sector sizes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Cornelia Huck , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, jfrei@linux.vnet.ibm.com, aliguori@amazon.com, "Eugene (jno) Dvurechenski" On 27/06/14 13:45, Alexander Graf wrote: > > On 27.06.14 13:25, Cornelia Huck wrote: >> From: "Eugene (jno) Dvurechenski" >> >> 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 >> Signed-off-by: Eugene (jno) Dvurechenski >> Signed-off-by: Jens Freimann >> Signed-off-by: Cornelia Huck >> --- >> 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.