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

[Gleb Natapov <gleb@redhat.com>]

On Fri, Apr 26, 2013 at 11:34:24PM +0200, Dominik Dingel wrote:
> On Fri, 26 Apr 2013 22:13:14 +0300
> Gleb Natapov <gleb@redhat.com> wrote:
> 
> > On Fri, Apr 26, 2013 at 01:55:23PM -0500, Anthony Liguori wrote:
> > > Dominik Dingel <dingel@linux.vnet.ibm.com> writes:
> > > 
> > > > On Fri, 26 Apr 2013 11:36:11 -0500
> > > > Anthony Liguori <anthony@codemonkey.ws> wrote:
> > > >
> > > >> Dominik Dingel <dingel@linux.vnet.ibm.com> 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 <dingel@linux.vnet.ibm.com>
> > > >> 
> > > >> 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.  
> 
That is up to firmware whether to exclude something from boot order or
not is "strict" boot option is not set. If "strict" is used excluding
devices without bootindex is a desirable behaviour. If docs/bootindex.txt
does not makes it clear (and I agree, it does not) it should be fixed.

> > > 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?
> 
You can implement whatever logic you like. Seabios default boot order is floppy,cdrom,disk,net.

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

> 
> 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
> 
No. This is not how it is now.

All devices with non negative bootindex are tries first in order of
bootindex. If strict option is specified boot process stops here, otherwise
firmware tries to boot from remaining bootable devices in whatever order
firmware deems best.

> 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.
> > 

--
			Gleb.