Re: [Qemu-devel] Dangling change state handler while hot unplugging ahci adapter

[Ashijeet Acharya]

On Thu, Aug 11, 2016 at 1:50 PM, Ashijeet Acharya
<address@hidden> wrote:
> On Wed, Aug 10, 2016 at 11:12 AM, Ashijeet Acharya
> <address@hidden> wrote:
>> On Tue, Aug 9, 2016 at 11:48 PM, John Snow <address@hidden> wrote:
>>>
>>>
>>> On 08/09/2016 01:16 PM, Ashijeet Acharya wrote:
>>>>
>>>> Hi again,
>>>> I am still waiting for some guidance...Can I please get some help with
>>>> this?
>>>>
>>>> Also.. I tried hotplugging an AHCI adapter but got the following error:
>>>> Bus 'ahci.0' does not support hotplugging
>>>>
>>>> Steps I followed:
>>>>
>>>> 1. launch vm with ahci enabled
>>>> 2. (qemu) drive_add 0
>>>> file=test.qcow2,cache=none,if=none,id=disk2,format=qcow2
>>>> OK
>>>> 3. (qemu) device_add ide-hd,bus=ahci.0,id=ahci-disk,drive=disk2
>>>> Bus 'ahci.0' does not support hotplugging
>>>>
>>>> What am I doing wrong?
>>>>
>>>> Thanks
>>>> Ashijeet
>>>>
>>>
>>> I think you are confusing hotplugging the AHCI adapter with hotplugging an
>>> IDE drive into an AHCI adapter.
>>>
>>> IDE/ATA/PATA (A rose by any other name...) drives do not support hotplugging
>>> (hence "Bus 'ahci.0' does not support hotplugging").
>>>
>>> SATA drives do in theory, but it hasn't been implemented yet.
>>>
>>> That's probably a little more involved than a Bite Sized Task, but it's
>>> something that I'd be willing to review if you embarked upon such a task.
>
> Sure... I can work upon that after I complete this bitesized task and
> get more acquainted with this part of the code.
>
> Ashijeet Acharya
>>>
>>>> On Sat, Aug 6, 2016 at 11:21 PM, Ashijeet Acharya
>>>> <address@hidden> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I was working on a patch regarding a device lifecycle bitesize task
>>>>> and I wanted to clear my queries about what the task is exactly.
>>>>>
>>>>> Do I need to create a new function ahci_unrealize() in the
>>>>> hw/ide/ahci.c file which calls for qemu_del_vm_change_state_handler()
>>>>> to free the handler at the time of ahci hot unplug.
>>>>>
>>>
>>> Sounds about right. grep around for other instances of
>>> qemu_del_vm_change_state_handler and try to code by example.
>>
>> Alright..thanks!
>>
>> I found out that ahci_realize() calls for ide_register_restart_cb()
>> which has qemu_add_vm_change_state_handler() but the returned value
>> from qemu_add_vm_change_state_handler() does not get assigned to any
>> VMChangeStateEntry pointer. This should lead (I guess..) to a memory
>> leak! Maybe introducing a VMChangeStateEntry field in struct AHCIState
>> help and can be used here. This later can also be passed to
>> qemu_del_vm_change_state_handler() to free things up.
>>>

I think we should do

s->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, bus);
instead of
qemu_add_vm_change_state_handler(ide_restart_cb, bus);

in ide_register_restart_cb() in hw/ide/core.c to store the returned
pointer to memory to avoid a possible memory leak I guess and
introduce a VMChangeStateEntry field in struct AHCIState to handle
this. Same can then further be used with
qemu_del_vm_change_state_handler() in ahci_unrealize() to free things
up.

Ashijeet

>>>
>>>>> Please tell me if I am on the right path and correct me if I am wrong.
>>>>>
>>>>> Thanks
>>>>> Ashijeet
>>>
>>>
>>> --
>>> —js