Re: [Qemu-ppc] [PATCH 5/6] xics: split to xics and xics-common

[Alexey Kardashevskiy]

On 08/07/2013 05:03 PM, Andreas Färber wrote:
> Am 07.08.2013 08:06, schrieb Alexey Kardashevskiy:
>> On 08/06/2013 07:53 PM, Andreas Färber wrote:
>>> Am 06.08.2013 10:27, schrieb Alexey Kardashevskiy:
>>>> +    }
>>>> +}
>>>> +
>>>> +void xics_prop_set_nr_irqs(Object *obj, struct Visitor *v,
>>>> +                           void *opaque, const char *name, struct Error 
>>>> **errp)
>>>
>>> You should be able to drop both "struct"s.
>>
>> Yeah, fixed. This is just how the ObjectPropertyAccessor type is defined.
> 
> Yeah, reason is some circular header dependencies. ;)
> 
>>>> +static void xics_common_initfn(Object *obj)
>>>> +{
>>>> +    object_property_add(obj, "nr_irqs", "int",
>>>> +                        NULL, xics_prop_set_nr_irqs, NULL, NULL, NULL);
>>>> +    object_property_add(obj, "nr_servers", "int",
>>>> +                        NULL, xics_prop_set_nr_servers, NULL, NULL, NULL);
>>>
>>> Since this property is visible in the QOM tree, it would be nice to
>>> implement trivial getters returns the value from the state fields.
>>
>> Added. How do I test it?
> 
> ./QMP/qom-list to find the path, if not fixed in code yet, and

Something is wrong. XICS does not have an id but it should not be a
problem, right (btw how to set it from the code?)?

address@hidden ~]$ ./qemu-impreza/QMP/qom-list -s ./qmp-sock
/
address@hidden ~]$

This is qtree:

(qemu) info qtree
bus: main-system-bus
  type System
  dev: spapr-pci-host-bridge, id ""
    index = 0
    buid = 0x800000020000000
    liobn = 0x80000000
    mem_win_addr = 0x100a0000000
    mem_win_size = 0x20000000
    io_win_addr = 0x10080000000
    io_win_size = 0x10000
    msi_win_addr = 0x10090000000
    irq 0
    bus: pci
      type PCI
  dev: spapr-vio-bridge, id ""
    irq 0
    bus: spapr-vio
      type spapr-vio-bus
      dev: spapr-vty, id "ser1"
        reg = 1895825409
        chardev = char1
        irq = 4102
      dev: spapr-nvram, id "address@hidden"
        reg = 1895825408
        drive = <null>
        irq = 4097
  dev: xics-kvm, id ""
    irq 0


> ./QMP/qom-get path.nr_servers to retrieve the value,
> ./QMP/qom-set path.nr_servers to verify it doesn't kill the process.
> 
> -qmp unix:./qmp-sock,server,nowait is what I use on the QEMU side.




>> "info qtree" prints only
>> DEVICE_CLASS(class)->props which is null in this case.
> 
> True, info qtree is from qdev times and deprecated. I recently attached
> a "qom-tree" script doing the equivalent that you could try.
> 
> I'll try to address -device xics,? showing them for 1.7. (Anthony
> indicated on a KVM call that the expected way to discover properties was
> to instantiate the type rather than looking at its class. Requires that
> types are instantiatable without asserting KVM accelerator, which gets
> initialized later.)
> 
>>>> @@ -651,47 +765,32 @@ static void rtas_int_on(PowerPCCPU *cpu, 
>>>> sPAPREnvironment *spapr,
>>>>  /*
>>>>   * XICS
>>>>   */
>>>> +void xics_set_nr_irqs(XICSState *icp, uint32_t nr_irqs)
>>>> +{
>>>> +    icp->ics = ICS(object_new(TYPE_ICS));
>>>> +    object_property_add_child(OBJECT(icp), "ics", OBJECT(icp->ics), NULL);
>>>
>>> Why create this single object in the setter? Can't you just create this
>>> in the common type's instance_init similar to before but using
>>> object_initialize() instead of object_new() and
>>> object_property_set_bool() in the realizefn?
>>
>> I cannot create in the common code as there are TYPE_ICS and TYPE_ICS_KVM
>> (oops, KVM is at the end, will fix it) types.
>>
>> And I would really want not to create it in instance_init() as I want to
>> have the object created and all its properties initialized in one place.
> 
> Doing it in instance_init facilitates improving the setter to allow
> multiple uses as a follow-up patch, since it must only be created once.
> If you don't, then the child will not be present in the composition tree
> before setting this seemingly unrelated property.

It will still be happening to ICP's and cannot be avoided. Why to make it
different for ICS and ICP? No IRQs number effectively means "no IRQ source".


> That seems worse to me
> than accessing a field in two places - instance_init will be run before
> any property setter.


> BTW these dynamic setters do not check automatically whether the object
> has already been realized. If you want the setter to return an Error in
> that case, you will need to check for DeviceState *dev = DEVICE(icp);
> dev->realized yourself. Not sure if that's the semantics you desire, but
> I guess so?


realize() will fail if a property is not set. Setters will fail if a
property is already set. By fail I mean that Error** thingy, not abort().
Do not really see why would we need something more here.



-- 
Alexey