From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:55533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hEfTz-0001y2-V0 for qemu-devel@nongnu.org; Thu, 11 Apr 2019 15:35:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hEfTy-0003jl-Nu for qemu-devel@nongnu.org; Thu, 11 Apr 2019 15:35:35 -0400 References: <20190411135602.21725-1-armbru@redhat.com> <20190411135602.21725-2-armbru@redhat.com> From: Laszlo Ersek Message-ID: <0eea5ddb-1efb-8fdf-4a37-9bc2daf86e2b@redhat.com> Date: Thu, 11 Apr 2019 21:35:19 +0200 MIME-Version: 1.0 In-Reply-To: <20190411135602.21725-2-armbru@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] pflash_cfi01: New pflash_cfi01_legacy_drive() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, peter.maydell@linaro.org, qemu-block@nongnu.org, mreitz@redhat.com, qemu-arm@nongnu.org On 04/11/19 15:56, Markus Armbruster wrote: > Factored out of pc_system_firmware_init() so the next commit can reuse > it in hw/arm/virt.c. > > Signed-off-by: Markus Armbruster > --- > hw/block/pflash_cfi01.c | 30 ++++++++++++++++++++++++++++++ > hw/i386/pc_sysfw.c | 19 ++----------------- > include/hw/block/flash.h | 1 + > 3 files changed, 33 insertions(+), 17 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 16dfae14b8..eba01b1447 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -44,9 +44,12 @@ > #include "qapi/error.h" > #include "qemu/timer.h" > #include "qemu/bitops.h" > +#include "qemu/error-report.h" > #include "qemu/host-utils.h" > #include "qemu/log.h" > +#include "qemu/option.h" > #include "hw/sysbus.h" > +#include "sysemu/blockdev.h" > #include "sysemu/sysemu.h" > #include "trace.h" > > @@ -968,6 +971,33 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl) > return &fl->mem; > } > > +/* > + * Handle -drive if=pflash for machines that use machine properties. (1) Can we improve readability with quotes? "-drive if=pflash" > + * Map unit#i (0 < i <= @ndev) to @dev[i]'s property "drive". (2) I think you meant (0 <= i < @ndev) > + */ > +void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev) > +{ > + int i; (3) For utter pedantry, "ndev" and "i" should have type "size_t" (in particular because we expect the caller to fill "ndev" with ARRAY_SIZE(). But, this would go beyond refactoring -- the original "int"s have served us just fine, and we're usually counting up (exclusively) to the huge number... two. :) > + DriveInfo *dinfo; > + Location loc; > + > + for (i = 0; i < ndev; i++) { > + dinfo = drive_get(IF_PFLASH, 0, i); > + if (!dinfo) { > + continue; > + } > + loc_push_none(&loc); > + qemu_opts_loc_restore(dinfo->opts); > + if (dev[i]->blk) { (4) Is this really identical to the code being removed? The above controlling expression amounts to: pcms->flash[i]->blk but the original boils down to pflash_cfi01_get_blk(pcms->flash[i]) Hmm... looking at pflash_cfi01_get_blk(), they are the same. Is there a particular reason for not using the wrapper function any longer? As in: pflash_cfi01_get_blk(dev[i]) > + error_report("clashes with -machine"); > + exit(1); > + } > + qdev_prop_set_drive(DEVICE(dev[i]), "drive", > + blk_by_legacy_dinfo(dinfo), &error_fatal); > + loc_pop(&loc); > + } > +} > + > static void postload_update_cb(void *opaque, int running, RunState state) > { > PFlashCFI01 *pfl = opaque; > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index c628540774..d58e47184c 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -269,32 +269,17 @@ void pc_system_firmware_init(PCMachineState *pcms, > { > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > int i; > - DriveInfo *pflash_drv; > BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)]; > - Location loc; > > if (!pcmc->pci_enabled) { > old_pc_system_rom_init(rom_memory, true); > return; > } > > - /* Map legacy -drive if=pflash to machine properties */ > + pflash_cfi01_legacy_drive(pcms->flash, ARRAY_SIZE(pcms->flash)); > + > for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { > pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); > - pflash_drv = drive_get(IF_PFLASH, 0, i); > - if (!pflash_drv) { > - continue; > - } > - loc_push_none(&loc); > - qemu_opts_loc_restore(pflash_drv->opts); > - if (pflash_blk[i]) { > - error_report("clashes with -machine"); > - exit(1); > - } > - pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); > - qdev_prop_set_drive(DEVICE(pcms->flash[i]), > - "drive", pflash_blk[i], &error_fatal); > - loc_pop(&loc); > } (5) I think you deleted too much here. After this loop, post-patch, for all "i", we'll have pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); But pre-patch, for all "i" where the "continue" didn't fire, we'd end up with: pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); IOW the original loop both verifies and *collects*, for the gap check that comes just below. IIRC, this allows for mixing "-drive if=pflash,unit=N" with the machine properties, as long as you have neither conflicts, nor gaps. Post-patch however, this kind of mixing would break, because we'd report gaps for the legacy ("-drive if=pflash") options. In addition, it could break the "use ROM mode" branch below, which is based on pflash_blk[0]. I think we should extend pflash_cfi01_legacy_drive() to populate "pflash_blk[0..(ndev-1)]" on output (assuming pflash_blk is not NULL on input). (Because, I'm assuming that qdev_prop_set_drive() -- which now occurs in the factored-out loop *before* the loop that we preserve here -- has no effect on pflash_cfi01_get_blk(pcms->flash[i]).) Thanks, Laszlo > > /* Reject gaps */ > diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h > index a0f488732a..f6a68c2a4c 100644 > --- a/include/hw/block/flash.h > +++ b/include/hw/block/flash.h > @@ -24,6 +24,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base, > int be); > BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl); > MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl); > +void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev); > > /* pflash_cfi02.c */ > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A2BDBC10F13 for ; Thu, 11 Apr 2019 19:36:35 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6070F206BA for ; Thu, 11 Apr 2019 19:36:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6070F206BA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([127.0.0.1]:53916 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hEfUw-0002ax-MI for qemu-devel@archiver.kernel.org; Thu, 11 Apr 2019 15:36:34 -0400 Received: from eggs.gnu.org ([209.51.188.92]:55533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hEfTz-0001y2-V0 for qemu-devel@nongnu.org; Thu, 11 Apr 2019 15:35:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hEfTy-0003jl-Nu for qemu-devel@nongnu.org; Thu, 11 Apr 2019 15:35:35 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49202) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1hEfTs-0003dc-VB; Thu, 11 Apr 2019 15:35:29 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id A34BC309E9A7; Thu, 11 Apr 2019 19:35:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-126-53.rdu2.redhat.com [10.10.126.53]) by smtp.corp.redhat.com (Postfix) with ESMTP id C0BB95D6A9; Thu, 11 Apr 2019 19:35:25 +0000 (UTC) To: Markus Armbruster , qemu-devel@nongnu.org References: <20190411135602.21725-1-armbru@redhat.com> <20190411135602.21725-2-armbru@redhat.com> From: Laszlo Ersek Message-ID: <0eea5ddb-1efb-8fdf-4a37-9bc2daf86e2b@redhat.com> Date: Thu, 11 Apr 2019 21:35:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190411135602.21725-2-armbru@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Thu, 11 Apr 2019 19:35:27 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] [PATCH 1/2] pflash_cfi01: New pflash_cfi01_legacy_drive() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, peter.maydell@linaro.org, qemu-arm@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" Message-ID: <20190411193519.P9s7ba2u_ul-AOpGfysYMSkS8_bZVQwmRkVfOkWirAw@z> On 04/11/19 15:56, Markus Armbruster wrote: > Factored out of pc_system_firmware_init() so the next commit can reuse > it in hw/arm/virt.c. > > Signed-off-by: Markus Armbruster > --- > hw/block/pflash_cfi01.c | 30 ++++++++++++++++++++++++++++++ > hw/i386/pc_sysfw.c | 19 ++----------------- > include/hw/block/flash.h | 1 + > 3 files changed, 33 insertions(+), 17 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index 16dfae14b8..eba01b1447 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -44,9 +44,12 @@ > #include "qapi/error.h" > #include "qemu/timer.h" > #include "qemu/bitops.h" > +#include "qemu/error-report.h" > #include "qemu/host-utils.h" > #include "qemu/log.h" > +#include "qemu/option.h" > #include "hw/sysbus.h" > +#include "sysemu/blockdev.h" > #include "sysemu/sysemu.h" > #include "trace.h" > > @@ -968,6 +971,33 @@ MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl) > return &fl->mem; > } > > +/* > + * Handle -drive if=pflash for machines that use machine properties. (1) Can we improve readability with quotes? "-drive if=pflash" > + * Map unit#i (0 < i <= @ndev) to @dev[i]'s property "drive". (2) I think you meant (0 <= i < @ndev) > + */ > +void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev) > +{ > + int i; (3) For utter pedantry, "ndev" and "i" should have type "size_t" (in particular because we expect the caller to fill "ndev" with ARRAY_SIZE(). But, this would go beyond refactoring -- the original "int"s have served us just fine, and we're usually counting up (exclusively) to the huge number... two. :) > + DriveInfo *dinfo; > + Location loc; > + > + for (i = 0; i < ndev; i++) { > + dinfo = drive_get(IF_PFLASH, 0, i); > + if (!dinfo) { > + continue; > + } > + loc_push_none(&loc); > + qemu_opts_loc_restore(dinfo->opts); > + if (dev[i]->blk) { (4) Is this really identical to the code being removed? The above controlling expression amounts to: pcms->flash[i]->blk but the original boils down to pflash_cfi01_get_blk(pcms->flash[i]) Hmm... looking at pflash_cfi01_get_blk(), they are the same. Is there a particular reason for not using the wrapper function any longer? As in: pflash_cfi01_get_blk(dev[i]) > + error_report("clashes with -machine"); > + exit(1); > + } > + qdev_prop_set_drive(DEVICE(dev[i]), "drive", > + blk_by_legacy_dinfo(dinfo), &error_fatal); > + loc_pop(&loc); > + } > +} > + > static void postload_update_cb(void *opaque, int running, RunState state) > { > PFlashCFI01 *pfl = opaque; > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c > index c628540774..d58e47184c 100644 > --- a/hw/i386/pc_sysfw.c > +++ b/hw/i386/pc_sysfw.c > @@ -269,32 +269,17 @@ void pc_system_firmware_init(PCMachineState *pcms, > { > PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms); > int i; > - DriveInfo *pflash_drv; > BlockBackend *pflash_blk[ARRAY_SIZE(pcms->flash)]; > - Location loc; > > if (!pcmc->pci_enabled) { > old_pc_system_rom_init(rom_memory, true); > return; > } > > - /* Map legacy -drive if=pflash to machine properties */ > + pflash_cfi01_legacy_drive(pcms->flash, ARRAY_SIZE(pcms->flash)); > + > for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) { > pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); > - pflash_drv = drive_get(IF_PFLASH, 0, i); > - if (!pflash_drv) { > - continue; > - } > - loc_push_none(&loc); > - qemu_opts_loc_restore(pflash_drv->opts); > - if (pflash_blk[i]) { > - error_report("clashes with -machine"); > - exit(1); > - } > - pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); > - qdev_prop_set_drive(DEVICE(pcms->flash[i]), > - "drive", pflash_blk[i], &error_fatal); > - loc_pop(&loc); > } (5) I think you deleted too much here. After this loop, post-patch, for all "i", we'll have pflash_blk[i] = pflash_cfi01_get_blk(pcms->flash[i]); But pre-patch, for all "i" where the "continue" didn't fire, we'd end up with: pflash_blk[i] = blk_by_legacy_dinfo(pflash_drv); IOW the original loop both verifies and *collects*, for the gap check that comes just below. IIRC, this allows for mixing "-drive if=pflash,unit=N" with the machine properties, as long as you have neither conflicts, nor gaps. Post-patch however, this kind of mixing would break, because we'd report gaps for the legacy ("-drive if=pflash") options. In addition, it could break the "use ROM mode" branch below, which is based on pflash_blk[0]. I think we should extend pflash_cfi01_legacy_drive() to populate "pflash_blk[0..(ndev-1)]" on output (assuming pflash_blk is not NULL on input). (Because, I'm assuming that qdev_prop_set_drive() -- which now occurs in the factored-out loop *before* the loop that we preserve here -- has no effect on pflash_cfi01_get_blk(pcms->flash[i]).) Thanks, Laszlo > > /* Reject gaps */ > diff --git a/include/hw/block/flash.h b/include/hw/block/flash.h > index a0f488732a..f6a68c2a4c 100644 > --- a/include/hw/block/flash.h > +++ b/include/hw/block/flash.h > @@ -24,6 +24,7 @@ PFlashCFI01 *pflash_cfi01_register(hwaddr base, > int be); > BlockBackend *pflash_cfi01_get_blk(PFlashCFI01 *fl); > MemoryRegion *pflash_cfi01_get_memory(PFlashCFI01 *fl); > +void pflash_cfi01_legacy_drive(PFlashCFI01 *dev[], int ndev); > > /* pflash_cfi02.c */ > >