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

[Anthony Liguori]

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.

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.

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