From: Cornelia Huck <cohuck@redhat.com> To: Thomas Huth <thuth@redhat.com>, jjherne@linux.ibm.com Cc: qemu-devel@nongnu.org, qemu-s390x@nongnu.org, pasic@linux.ibm.com, alifm@linux.ibm.com, borntraeger@de.ibm.com Subject: Re: [Qemu-devel] [qemu-s390x] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support Date: Fri, 5 Apr 2019 15:36:21 +0200 [thread overview] Message-ID: <20190405153621.79a0b995.cohuck@redhat.com> (raw) In-Reply-To: <ec4dd665-53c6-5731-8f36-0e0bef55e646@redhat.com> On Fri, 5 Apr 2019 15:26:24 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 05/04/2019 15.11, Jason J. Herne wrote: > > Your analysis of the problem matches what I'm seeing as well. Here is > > what I'm proposing to fix it. If you like it, let me know if you want me > > to re-send just the final patch, or the entire series again with a v7 tag. If you (Jason) resend just that patch, I'm happy to give it a tag :) > > > > > > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > > index 03e90e3..3d1e9a4 100644 > > --- a/pc-bios/s390-ccw/main.c > > +++ b/pc-bios/s390-ccw/main.c > > @@ -67,6 +67,7 @@ static bool find_subch(int dev_no) > > { > > Schib schib; > > int i, r; > > + bool is_virtio; > > > > for (i = 0; i < 0x10000; i++) { > > blk_schid.sch_no = i; > > @@ -80,12 +81,13 @@ static bool find_subch(int dev_no) > > > > enable_subchannel(blk_schid); > > cutype = cu_type(blk_schid); > > + is_virtio = virtio_is_supported(blk_schid); > > > > /* No specific devno given, just return 1st possibly bootable > > device */ > > if (dev_no < 0) { > > switch (cutype) { > > case CU_TYPE_VIRTIO: > > - if (virtio_is_supported(blk_schid)) { > > + if (is_virtio) { > > /* > > * Skip net devices since no IPLB is created and > > therefore > > * no network bootloader has been loaded > > > > > > Looks fine to me. But maybe we should also add a comment before the new > "virtio_is_supported" line, saying something like "Note: we always have > to run virtio_is_supported() here to make sure that the vdev.senseid > data gets pre-initialized"? Otherwise, we might accidentally "revert" > this patch when somebody thinks that the code might be optimizable by > removing the is_virtio variable again... Agreed. > > If you like, I can squash these changes in when I pick up the patches, > so you don't have to resend. > > Thomas >
WARNING: multiple messages have this Message-ID (diff)
From: Cornelia Huck <cohuck@redhat.com> To: Thomas Huth <thuth@redhat.com>, jjherne@linux.ibm.com Cc: pasic@linux.ibm.com, borntraeger@de.ibm.com, qemu-s390x@nongnu.org, alifm@linux.ibm.com, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [qemu-s390x] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support Date: Fri, 5 Apr 2019 15:36:21 +0200 [thread overview] Message-ID: <20190405153621.79a0b995.cohuck@redhat.com> (raw) Message-ID: <20190405133621.5RwXGDWEtmOUt6Lu-SSmbIg7OzhIRWFyeq0j9Ds-PVc@z> (raw) In-Reply-To: <ec4dd665-53c6-5731-8f36-0e0bef55e646@redhat.com> On Fri, 5 Apr 2019 15:26:24 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 05/04/2019 15.11, Jason J. Herne wrote: > > Your analysis of the problem matches what I'm seeing as well. Here is > > what I'm proposing to fix it. If you like it, let me know if you want me > > to re-send just the final patch, or the entire series again with a v7 tag. If you (Jason) resend just that patch, I'm happy to give it a tag :) > > > > > > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > > index 03e90e3..3d1e9a4 100644 > > --- a/pc-bios/s390-ccw/main.c > > +++ b/pc-bios/s390-ccw/main.c > > @@ -67,6 +67,7 @@ static bool find_subch(int dev_no) > > { > > Schib schib; > > int i, r; > > + bool is_virtio; > > > > for (i = 0; i < 0x10000; i++) { > > blk_schid.sch_no = i; > > @@ -80,12 +81,13 @@ static bool find_subch(int dev_no) > > > > enable_subchannel(blk_schid); > > cutype = cu_type(blk_schid); > > + is_virtio = virtio_is_supported(blk_schid); > > > > /* No specific devno given, just return 1st possibly bootable > > device */ > > if (dev_no < 0) { > > switch (cutype) { > > case CU_TYPE_VIRTIO: > > - if (virtio_is_supported(blk_schid)) { > > + if (is_virtio) { > > /* > > * Skip net devices since no IPLB is created and > > therefore > > * no network bootloader has been loaded > > > > > > Looks fine to me. But maybe we should also add a comment before the new > "virtio_is_supported" line, saying something like "Note: we always have > to run virtio_is_supported() here to make sure that the vdev.senseid > data gets pre-initialized"? Otherwise, we might accidentally "revert" > this patch when somebody thinks that the code might be optimizable by > removing the is_virtio variable again... Agreed. > > If you like, I can squash these changes in when I pick up the patches, > so you don't have to resend. > > Thomas >
next prev parent reply other threads:[~2019-04-05 13:37 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-04 14:34 [Qemu-devel] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 01/16] s390 vfio-ccw: Add bootindex property and IPLB data Jason J. Herne 2019-04-12 10:38 ` Thomas Huth 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 02/16] s390-bios: decouple cio setup from virtio Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 03/16] s390-bios: decouple common boot logic " Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 04/16] s390-bios: Clean up cio.h Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 05/16] s390-bios: Decouple channel i/o logic from virtio Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 06/16] s390-bios: Map low core memory Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 07/16] s390-bios: ptr2u32 and u32toptr Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 08/16] s390-bios: Support for running format-0/1 channel programs Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 09/16] s390-bios: cio error handling Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 10/16] s390-bios: Extend find_dev() for non-virtio devices Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 11/16] s390-bios: Factor finding boot device out of virtio code path Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 12/16] s390-bios: Refactor virtio to run channel programs via cio Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 13/16] s390-bios: Use control unit type to determine boot method Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 14/16] s390-bios: Add channel command codes/structs needed for dasd-ipl Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 15/16] s390-bios: Support booting from real dasd device Jason J. Herne 2019-04-04 14:34 ` [Qemu-devel] [PATCH v6 16/16] s390-bios: Use control unit type to find bootable devices Jason J. Herne 2019-04-05 5:55 ` Thomas Huth 2019-04-04 15:14 ` [Qemu-devel] [PATCH v6 00/16] s390: vfio-ccw dasd ipl support no-reply 2019-04-04 15:20 ` no-reply 2019-04-05 6:58 ` Thomas Huth 2019-04-05 7:52 ` [Qemu-devel] [qemu-s390x] " Thomas Huth 2019-04-05 13:11 ` Jason J. Herne 2019-04-05 13:26 ` Thomas Huth 2019-04-05 13:36 ` Cornelia Huck [this message] 2019-04-05 13:36 ` Cornelia Huck 2019-04-05 13:43 ` Jason J. Herne
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190405153621.79a0b995.cohuck@redhat.com \ --to=cohuck@redhat.com \ --cc=alifm@linux.ibm.com \ --cc=borntraeger@de.ibm.com \ --cc=jjherne@linux.ibm.com \ --cc=pasic@linux.ibm.com \ --cc=qemu-devel@nongnu.org \ --cc=qemu-s390x@nongnu.org \ --cc=thuth@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.