Re: [RFC PATCH v2 10/16] qdev-monitor: allow adding any sysbus device before machine is ready

[Damien Hedde <damien.hedde@greensocs.com>]

On 9/23/21 13:55, Ani Sinha wrote:
> 
> 
> On Thu, 23 Sep 2021, Ani Sinha wrote:
> 
>>
>>
>> On Wed, 22 Sep 2021, Damien Hedde wrote:
>>
>>> Skip the sysbus device type per-machine allow-list check before the
>>> MACHINE_INIT_PHASE_READY phase.
>>>
>>> This patch permits adding any sysbus device (it still needs to be
>>> user_creatable) when using the -preconfig experimental option.
>>>
>>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>>> ---
>>>
>>> This commit is RFC. Depending on the condition to allow a device
>>> to be added, it may change.
>>> ---
>>>   softmmu/qdev-monitor.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>> index f1c9242855..73b991adda 100644
>>> --- a/softmmu/qdev-monitor.c
>>> +++ b/softmmu/qdev-monitor.c
>>> @@ -269,8 +269,13 @@ static DeviceClass *qdev_get_device_class(const char **driver, Error **errp)
>>>           return NULL;
>>>       }
>>>
>>> -    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE)) {
>>> -        /* sysbus devices need to be allowed by the machine */
>>> +    if (object_class_dynamic_cast(oc, TYPE_SYS_BUS_DEVICE) &&
>>> +        phase_check(MACHINE_INIT_PHASE_READY)) {
>>> +        /*
>>> +         * Sysbus devices need to be allowed by the machine.
>>> +         * We only check that after the machine is ready in order to let
>>> +         * us add any user_creatable sysbus device during machine creation.
>>> +         */
>>>           MachineClass *mc = MACHINE_CLASS(object_get_class(qdev_get_machine()));
>>>           if (!machine_class_is_dynamic_sysbus_dev_allowed(mc, *driver)) {
>>>               error_setg(errp, "'%s' is not an allowed pluggable sysbus device "
>>
>> Since now you are adding the state of the machine creation in the
>> valiation condition, the failure error message becomes misleading.
>> Better to do this I think :
>>
>> if (object class is TYPE_SYS_BUS_DEVICE)
>> {
>>    if (!phase_check(MACHINE_INIT_PHASE_READY))
>>      {
>>        // error out here saying the state of the machine creation is too
>> early
>>      }
>>
>>      // do the other check on dynamic sysbus
>>
>> }
> 
> The other thing to consider is whether we should put the machine phaze
> check at a higher level, at qdev_device_add() perhaps. One might envison
> that different device types may be allowed to be added at different
> stages of machine creation. So this check needs be more generalized for
> the future.
> 

Hi Ani,

I think moving out the allowance check from qdev_get_device_class is a 
good idea. The code will be more clear even if the check is not generalized.

Thanks,
--
Damien