[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device
From: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/7] bootindex: delete bootindex when device is removed |
Date: |
Mon, 4 Aug 2014 06:23:01 +0000 |
Hi,
> > >
> > > > + del_boot_device_path(dev);
> > >
> > > You can call this from device_finalize() instead of placing it into each
> > > individual device.
> > >
> > Maybe put this call in device_finalize is not a good idea.
> > I have three reasons:
> >
> > 1. the device's some memory have been freed before call device_finalize,
> > such as device->id. It is too later.
>
> I don't think you even need id. See my reply to v4 2/8.
>
> But you have a point about being too late: some devices call
> add_boot_device_path() on realize, so those would need to revert the
> operation on unrealize; others do it on init, so they need to do it on
> finalize.
>
> On either case, I believe an extra check inside device_finalize()
> wouldn't hurt, even if it becomes redundant on some devices.
>
>
OK. And I copy your review from v3 2/7, as follows:
>
> What if the device doesn't have any ID set? I don't see anything on
> add_boot_device_path() ensuring that dev->id is always set.
>
Yes, the id is not always set. So, I add a check in V4.
> Why you don't just check if i->dev == dev?
>
No, if we check directly i->dev == dev, we will not handle the virtio devices.
For example, the common user configure a virtio-net nic using command line
like " -device virtio-net-pci,netdev=net0,bootindex=3,id=nic1 ". Then the id
property
will be added for virtio-net-pci device, not virtio-net device which stored in
the global
fw_boot_order list. So, the i->dev
When common users want to change the bootindex of virtio-net. They only are
concerned
that they have configured an id for virtio-net nic card. So, they can pass the
id to QEMU. But
we should handle those scenes, meanwhile the device object gained by id is
virtio-net-pci device
not equals i->dev.
> > 2. not every kinds of device can configure bootindex property, such as usb
> > host adapters. It is a waste and useless for those devices. This is the
> > main reason.
>
> I would prefer to waste a few cycles scanning the boot index list every
> time a device is removed, than risking crashing QEMU in case somebody
> forget to add a del_boot_device_path() call.
>
OK, fine!
Maybe I should do this in device_finalize() as Gerd's previous suggestion,
like yours. Thanks.
>
> > 3. virtio-net device's parent is virtio-pci device, which configured id
> > property,
> > But the device saved in global fw_boot_order list is virtio-net device have
> not
> > id property. If we put call del_boot_device_path(dev) in
> virtio_net_device_unrealize
> > we can delete it from fw_boot_order directly.
>
> Sorry, I don't understand what you mean here. If virtio-net doesn't have
> an id property, would the current version of del_boot_device_path() even
> work?
>
Please see above comments.
Thanks for your review!
Best regards,
-Gonglei