Re: [Qemu-devel] [PATCH 01/10] Common: Add a default bootindex for all applicable devices

[Dominik Dingel]

On Fri, 26 Apr 2013 22:13:14 +0300
Gleb Natapov <address@hidden> wrote:

> On Fri, Apr 26, 2013 at 01:55:23PM -0500, Anthony Liguori wrote:
> > Dominik Dingel <address@hidden> writes:
> > 
> > > On Fri, 26 Apr 2013 11:36:11 -0500
> > > Anthony Liguori <address@hidden> wrote:
> > >
> > >> Dominik Dingel <address@hidden> writes:
> > >> 
> > >> > Currently only devices with a positive boot index will be pushed in the
> > >> > fw_boot_order queue, so if no boot index at all will be specified,
> > >> > the queue ends up empty.
> > >> >
> > >> > Instead we push exactly as docs/bootindex.txt says the devices with
> > >> > the lowest possible boot priority at the tail of the queue,
> > >> > because we give them the highest available boot index.
> > >> >
> > >> > Signed-off-by: Dominik Dingel <address@hidden>
> > >> 
> > >> Wouldn't this break the ability to say: "don't every try to boot from
> > >> this device?"
> > >> 
> > >> As an example, some people want to force PXE boot to not be tried on
> > >> certain networks.
> > >> 
> > >> Regards,
> > >> 
> > >> Anthony Liguori
> > >
> > > That is correct, hmm. The thing is, if we don't submit a bootindex, we
> > > will assign -1 in virtio-blk and virtio-net. This would forbid that
> > > the device would be booted from. 
> > > Where docs/bootindex.txt says: if a device got no bootindex, it gets
> > > the lowest possibly priority... 
> > >
> > > One way to fix this, would be to change the behaviour for virtio-blk
> > > and virtio-net, if there is no boot value assigned to, give it the
> > > possible highest number. 
> > >
> > > Would be this okay for you Anthony?
> > 
> > CC'ing Gleb since he introduced bootindex.
> > 
> This will break "strict" boot option. It makes firmware boot only
> from a device with specified boot priority.
> 
> What problem the patch is trying to fix?

virtio-blk and virtio-net devices will be excluded from the boot order if they 
don't get a boot index on commandline, which is exactly the opposite of what 
docs/bootindex.txt describes.  

> > The challenge here is we have two mechanisms on x86.
> > 
> > If there are no devices specified by boot index, then we follow the CMOS
> > bits that include "boot from first hard disk".  The firmware can
> > enumerate hard disks on its own (including virtio-blk devices) and the
> > user can reorder this list within the firmware.
> > 
> > So we don't need to explicitly set bootindex on x86 to get the behavior
> > your trying to replicate on s390 (boot from first hard disk).  We get it
> > through our CMOS fallback.
> IIRC even if CMOS will not specify the boot order Seabios will use its
> own defaults. Firmware enumerates bootable devices anyway and may have
> its own logic to find the one to boot from. For instance the first
> bootable device that is found.

But wouldn't that prevent any kind of boot protection for like pxe boot? If the 
network would be the first boot device?

Maybe someone should check if docs/bootindex.txt is up to date? With all the 
changes in the code flow.

If I see it correctly we have three cases for a boot device: 
- boot index <= 0, should mean from 0 to x this is your position in the boot 
ordering
- boot index > 0, should mean don't you dare to try to boot from this device

- no boot index at all, the doc says: boot, but with no priority 
                            code says: don't boot

Both ways are fine, but should be in sync. This is currently a complete focus 
on s390 as for x and p I'm not quite sure what the firmware can do after all 
happend.

Dominik

> > 
> > I think defaulting all bootable devices to something other than -1 (for
> > instance, UINT_MAX) would be okay provided that we used globals to use
> > the old behavior with older machine types.
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >
> > > Dominik
> > >  
> > >> >
> > >> > diff --git a/vl.c b/vl.c
> > >> > index 6caa5f4..84d7031 100644
> > >> > --- a/vl.c
> > >> > +++ b/vl.c
> > >> > @@ -248,7 +248,7 @@ struct FWBootEntry {
> > >> >      char *suffix;
> > >> >  };
> > >> >  
> > >> > -static QTAILQ_HEAD(, FWBootEntry) fw_boot_order =
> > >> > +static QTAILQ_HEAD(FWBootOrder, FWBootEntry) fw_boot_order =
> > >> >      QTAILQ_HEAD_INITIALIZER(fw_boot_order);
> > >> >  
> > >> >  int nb_numa_nodes;
> > >> > @@ -1213,10 +1213,21 @@ void add_boot_device_path(int32_t bootindex, 
> > >> > DeviceState *dev,
> > >> >      FWBootEntry *node, *i;
> > >> >  
> > >> >      if (bootindex < 0) {
> > >> > -        return;
> > >> > +        bootindex = INT32_MAX;
> > >> > +        if (!QTAILQ_EMPTY(&fw_boot_order) &&
> > >> > +           (QTAILQ_LAST(&fw_boot_order, FWBootOrder)->bootindex == 
> > >> > INT32_MAX)) {
> > >> > +            /* there is a device at the end of the queue, so we need 
> > >> > to walk
> > >> > +               the queue reverse to get the next free bootindex */
> > >> > +            QTAILQ_FOREACH_REVERSE(i, &fw_boot_order, FWBootOrder, 
> > >> > link) {
> > >> > +                if (i->bootindex != bootindex) {
> > >> > +                    break;
> > >> > +                }
> > >> > +                bootindex--;
> > >> > +            }
> > >> > +        }
> > >> >      }
> > >> >  
> > >> > -    assert(dev != NULL || suffix != NULL);
> > >> > +    assert(dev != NULL || suffix != NULL || bootindex >=  0);
> > >> >  
> > >> >      node = g_malloc0(sizeof(FWBootEntry));
> > >> >      node->bootindex = bootindex;
> > >> > -- 
> > >> > 1.7.9.5
> > >> 
> 
> --
>                       Gleb.
>