| Message ID | 1548768562-20007-5-git-send-email-jjherne@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390: vfio-ccw dasd ipl support | expand |
On Tue, 29 Jan 2019 08:29:11 -0500 "Jason J. Herne" <jjherne@linux.ibm.com> wrote: > We need a method for finding the subchannel of a dasd device. Let's > modify find_dev to handle this since it mostly does what we need. Up to > this point find_dev has been specific to only virtio devices. > > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > pc-bios/s390-ccw/main.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 2019-01-29 14:29, Jason J. Herne wrote: > We need a method for finding the subchannel of a dasd device. Let's > modify find_dev to handle this since it mostly does what we need. Up to > this point find_dev has been specific to only virtio devices. > > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > pc-bios/s390-ccw/main.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c > index 67df421..7e3f65e 100644 > --- a/pc-bios/s390-ccw/main.c > +++ b/pc-bios/s390-ccw/main.c > @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void) > return atoui(loadparm_str); > } > > +/* > + * Find the subchannel connected to the given device (dev_no) and fill in the > + * subchannel information block (schib) with the connected subchannel's info. > + * NOTE: The global variable blk_schid is updated to contain the subchannel > + * information. > + */ > static bool find_dev(Schib *schib, int dev_no) > { > int i, r; > @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no) > if (!schib->pmcw.dnv) { > continue; > } > - if (!virtio_is_supported(blk_schid)) { > - continue; > - } > + > /* Skip net devices since no IPLB is created and therefore no > - * no network bootloader has been loaded > + * network bootloader has been loaded > */ > - if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { > + if (virtio_is_supported(blk_schid) && > + virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { > continue; > } > + > if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) { > return true; > } > Not sure whether this really works as expected? If dev_no is -1, this used to return the first supported virtio device. Now it returns the first device that could be found - but how are we sure that we can boot from that device? Thomas
On 2/11/19 11:38 AM, Thomas Huth wrote: > On 2019-01-29 14:29, Jason J. Herne wrote: >> We need a method for finding the subchannel of a dasd device. Let's >> modify find_dev to handle this since it mostly does what we need. Up to >> this point find_dev has been specific to only virtio devices. >> >> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> >> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> pc-bios/s390-ccw/main.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >> index 67df421..7e3f65e 100644 >> --- a/pc-bios/s390-ccw/main.c >> +++ b/pc-bios/s390-ccw/main.c >> @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void) >> return atoui(loadparm_str); >> } >> >> +/* >> + * Find the subchannel connected to the given device (dev_no) and fill in the >> + * subchannel information block (schib) with the connected subchannel's info. >> + * NOTE: The global variable blk_schid is updated to contain the subchannel >> + * information. >> + */ >> static bool find_dev(Schib *schib, int dev_no) >> { >> int i, r; >> @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no) >> if (!schib->pmcw.dnv) { >> continue; >> } >> - if (!virtio_is_supported(blk_schid)) { >> - continue; >> - } >> + >> /* Skip net devices since no IPLB is created and therefore no >> - * no network bootloader has been loaded >> + * network bootloader has been loaded >> */ >> - if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { >> + if (virtio_is_supported(blk_schid) && >> + virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { >> continue; >> } >> + >> if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) { >> return true; >> } >> > > Not sure whether this really works as expected? If dev_no is -1, this > used to return the first supported virtio device. Now it returns the > first device that could be found - but how are we sure that we can boot > from that device? > How could we ever be sure we could boot from the first virtio device? The dev_no=1 case means we don't know which device to boot from so we're guessing. The only thing that is changing here is that we're allowing non-virtio devices as well. We do this because we now support booting from a device type that is not virtio.
On 13/02/2019 14.59, Jason J. Herne wrote: > On 2/11/19 11:38 AM, Thomas Huth wrote: >> On 2019-01-29 14:29, Jason J. Herne wrote: >>> We need a method for finding the subchannel of a dasd device. Let's >>> modify find_dev to handle this since it mostly does what we need. Up to >>> this point find_dev has been specific to only virtio devices. >>> >>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com> >>> Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com> >>> --- >>> pc-bios/s390-ccw/main.c | 16 +++++++++++----- >>> 1 file changed, 11 insertions(+), 5 deletions(-) >>> >>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c >>> index 67df421..7e3f65e 100644 >>> --- a/pc-bios/s390-ccw/main.c >>> +++ b/pc-bios/s390-ccw/main.c >>> @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void) >>> return atoui(loadparm_str); >>> } >>> +/* >>> + * Find the subchannel connected to the given device (dev_no) and >>> fill in the >>> + * subchannel information block (schib) with the connected >>> subchannel's info. >>> + * NOTE: The global variable blk_schid is updated to contain the >>> subchannel >>> + * information. >>> + */ >>> static bool find_dev(Schib *schib, int dev_no) >>> { >>> int i, r; >>> @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no) >>> if (!schib->pmcw.dnv) { >>> continue; >>> } >>> - if (!virtio_is_supported(blk_schid)) { >>> - continue; >>> - } >>> + >>> /* Skip net devices since no IPLB is created and therefore no >>> - * no network bootloader has been loaded >>> + * network bootloader has been loaded >>> */ >>> - if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { >>> + if (virtio_is_supported(blk_schid) && >>> + virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { >>> continue; >>> } >>> + >>> if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) { >>> return true; >>> } >>> >> >> Not sure whether this really works as expected? If dev_no is -1, this >> used to return the first supported virtio device. Now it returns the >> first device that could be found - but how are we sure that we can boot >> from that device? > > How could we ever be sure we could boot from the first virtio device? > The dev_no=1 case means we don't know which device to boot from so we're > guessing. The only thing that is changing here is that we're allowing > non-virtio devices as well. We do this because we now support booting > from a device type that is not virtio. For example, this command line used to work fine in the past: s390x-softmmu/qemu-system-s390x -enable-kvm -nographic \ -device x-terminal3270 -device virtio-blk-ccw,drive=dr1 \ -drive if=none,id=dr1,format=qcow2,file=guest.qcow2 With this patch applied, the guest is now not bootable anymore. So I'm sorry, but you break setups here that worked fine in the past. Isn't there an easy way to determine whether a device is a bootable block device or not? Thomas
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c index 67df421..7e3f65e 100644 --- a/pc-bios/s390-ccw/main.c +++ b/pc-bios/s390-ccw/main.c @@ -49,6 +49,12 @@ unsigned int get_loadparm_index(void) return atoui(loadparm_str); } +/* + * Find the subchannel connected to the given device (dev_no) and fill in the + * subchannel information block (schib) with the connected subchannel's info. + * NOTE: The global variable blk_schid is updated to contain the subchannel + * information. + */ static bool find_dev(Schib *schib, int dev_no) { int i, r; @@ -62,15 +68,15 @@ static bool find_dev(Schib *schib, int dev_no) if (!schib->pmcw.dnv) { continue; } - if (!virtio_is_supported(blk_schid)) { - continue; - } + /* Skip net devices since no IPLB is created and therefore no - * no network bootloader has been loaded + * network bootloader has been loaded */ - if (virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { + if (virtio_is_supported(blk_schid) && + virtio_get_device_type() == VIRTIO_ID_NET && dev_no < 0) { continue; } + if ((dev_no < 0) || (schib->pmcw.dev == dev_no)) { return true; }