Re: [PATCH 03/12] pc-bios/s390-ccw/bootmap: Improve the guessing logic in zipl_load_vblk()

[Thomas Huth]

On 01/07/2022 17.36, Eric Farman wrote:
> On Tue, 2022-06-28 at 15:10 +0200, Thomas Huth wrote:
> > The logic of trying an final ISO or ECKD boot on virtio-block devices
> > is
> > very weird: Since the geometry hardly ever matches in
> > virtio_disk_is_scsi(),
> > virtio_blk_setup_device() always sets a "guessed" disk geometry via
> > virtio_assume_scsi() (which is certainly also wrong in a lot of
> > cases).
> >
> > zipl_load_vblk() then sees that there's been a
> > "virtio_guessed_disk_nature"
> > and tries to fix up the geometry again via virtio_assume_iso9660()
> > before
> > always trying to do ipl_iso_el_torito(). That's a very brain-twisting
> > way of attempting to boot from ISO images, which won't work anymore
> > after
> > the following patches that will clean up the virtio_assume_scsi()
> > mess
> > (and thus get rid of the "virtio_guessed_disk_nature" here).
> >
> > Let's try a better approach instead: ISO files always have a magic
> > string "CD001" at offset 0x8001 (see e.g. the ECMA-119 specification)
> > which we can use to decide whether we should try to boot in ISO 9660
> > mode (which we should also try if we see a sector size of 2048).
> >
> > And if we were not able to boot in ISO mode here, the final boot
> > attempt
> > before panicking is to boot in ECKD mode. Since this is our last boot
> > attempt anyway, simply always assume the ECKD geometry here (if the
> > sector
> > size was not 4096 yet), so that we also do not depend on the guessed
> > disk
> > geometry from virtio_blk_setup_device() here anymore.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   pc-bios/s390-ccw/bootmap.c | 25 +++++++++++++++++++++----
> >   1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> > index 56411ab3b6..3181b05382 100644
> > --- a/pc-bios/s390-ccw/bootmap.c
> > +++ b/pc-bios/s390-ccw/bootmap.c
> > @@ -780,18 +780,35 @@ static void ipl_iso_el_torito(void)
> >       }
> >   }
> >   
> > +/**
> > + * Detect whether we're trying to boot from an .ISO image.
> > + * These always have a signature string "CD001" at offset 0x8001.
> > + */
> > +static bool has_iso_signature(void)
> > +{
> > +    if (virtio_read(0x8000 / virtio_get_block_size(), sec)) {
>
> Certainly unlikely, but virtio_get_block_size is able to return zero.

Right ... I guess I'd better add a check for this here.

 Thanks,
  Thomas