Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges

[Andreas Färber]

(Dropping some borked CCs)

Am 05.07.2012 16:15, schrieb Michael S. Tsirkin:
> On Thu, Jul 05, 2012 at 08:54:21AM -0500, Anthony Liguori wrote:
>> On 07/05/2012 08:34 AM, Michael S. Tsirkin wrote:
>>> On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas Färber wrote:
>>>> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin:
>>>>> On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrote:
>>>>>> On Wed, Jul 04, 2012 at 07:19:33PM +0200, Andreas Färber wrote:
>>>>>>> Uglify the parent field to enforce QOM-style access via casts.
>>>>>>> Don't just typedef PCIHostState, either use it directly or embed it.
>>>>>>>
>>>>>>> Signed-off-by: Andreas Färber<address@hidden>
>>>>>>> ---
>>>>>>>  hw/alpha_typhoon.c |    4 ++--
>>>>>>>  hw/dec_pci.c       |    2 +-
>>>>>>>  hw/grackle_pci.c   |    2 +-
>>>>>>>  hw/gt64xxx.c       |   26 ++++++++++++++++----------
>>>>>>>  hw/piix_pci.c      |    6 ++++--
>>>>>>>  hw/ppc4xx_pci.c    |    8 +++++---
>>>>>>>  hw/ppce500_pci.c   |    2 +-
>>>>>>>  hw/prep_pci.c      |    8 +++++---
>>>>>>>  hw/spapr_pci.c     |   12 +++++++-----
>>>>>>>  hw/spapr_pci.h     |    2 +-
>>>>>>>  hw/unin_pci.c      |   14 +++++++-------
>>>>>>>  11 files changed, 50 insertions(+), 36 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c
>>>>>>> index 58025a3..955d628 100644
>>>>>>> --- a/hw/alpha_typhoon.c
>>>>>>> +++ b/hw/alpha_typhoon.c
>>>>>>> @@ -46,7 +46,7 @@ typedef struct TyphoonPchip {
>>>>>>>      OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BRIDGE)
>>>>>>>
>>>>>>>  typedef struct TyphoonState {
>>>>>>> -    PCIHostState host;
>>>>>>> +    PCIHostState parent_obj;
>>>>>>>
>>>>>>>      TyphoonCchip cchip;
>>>>>>>      TyphoonPchip pchip;
>>>>>>> @@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABus 
>>>>>>> **isa_bus,
>>>>>>>      b = pci_register_bus(dev, "pci",
>>>>>>>                           typhoon_set_irq, sys_map_irq, s,
>>>>>>>                           &s->pchip.reg_mem, addr_space_io, 0, 64);
>>>>>>> -    s->host.bus = b;
>>>>>>> +    PCI_HOST_BRIDGE(s)->bus = b;
>>>>>>>
>>>>>>>      /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, 
>>>>>>> 64MB.  */
>>>>>>>      memory_region_init_io(&s->pchip.reg_iack,&alpha_pci_iack_ops, b,
>>>>>>
>>>>>> Sorry I don't understand.
>>>>>> why are we making code ugly apparently intentionally?
>>>>>
>>>>> Just to clarify: replacing upcasts which are always safe
>>>>> with downcasts which can fail is what I consider especially ugly.
>>>>
>>>> As per Anthony the parent field in the QOM instance structs is not
>>>> supposed to be touched (cf. object.h). We mark it /*<  private>*/ so
>>>> that it doesn't even show up in gtk-doc documentation.
>>>
>>> PCIHostState is not private here. And if it were, it won't be
>>> of any use because compiler does not read gtk-doc documentation
>>> and neither do developers.
>>>
>>>> If it is unused,
>>>> its name becomes irrelevant and could even be "reserved" if we so
>>>> wanted. Renaming it to whatever proves that all old references are gone.
>>>
>>> Adding arbitrary rules like that only seems to make code future proof.
>>> People will not remember and will use the field anyway,
>>> then when you try to change it there will be work to be done.
>>> So let's do the work when we really need it.
>>>
>>>> Background is that qdev and QOM work differently with regards to
>>>> inheritance: as mentioned in the preceding patch, for qdev the parent
>>>> was (had to be) identified by name and could be anywhere in the struct;
>>>> for QOM the parent is a subset of the struct from the start and it's
>>>> supposed to be accessed through the struct type that provides the
>>>> fields, the usual way to get such a pointer is through
>>>> OBJECT_CHECK()-derived cast macros.
>>>
>>> It makes sense if you go from parent to child. Even in C++
>>> you don't use dynamic_cast to go from child to parent.
>>
>> But you use static cast.  Casting is not the same thing as
>> dereferencing a member.  IOW:
>>
>> Foo *foo = (Foo *)bar;
>>
>> Is very different than:
>>
>> Foo *foo = &bar.foo;
>>
>> Using cast macros makes the code an awful lot more readable because
>> it tells the reader more.
>>
>> Foo *foo = FOO(bar);
>>
>> Tells the user that we're casting bar to the a type Foo.  OTOH:
>>
>> Foo *foo = &bar.foo;
>>
>> Doesn't tell the user anything.  A FOO() macro is consistent
>> regardless of how the type is implemented too.  It gets even worse
>> when you are going up multiple levels:
>>
>> DeviceState *dev = &bar.host.qdev;
>>
>> Is unintelligible whereas:
>>
>> DeviceState *dev = DEVICE(bar);
>>
>> Tells you exactly what's happening.
>>
>> But really, this isn't the place to debate this.  QOM has been
>> around for a while.  Consistency is important and this is how things
>> are done in QOM.
> 
> It's important but not the most important thing.
> It does not make sense to do casts *everywhere*.
> Do it where it makes sense.
> 
>> If you want to revisit this style, you should start a separate
>> thread about it and we can talk about it.  But this patch is
>> consistent with the current infrastructure.
>>
>> Regards,
>>
>> Anthony Liguori
> 
> So far QOM was a win.
> You examples look better with a macro. Patch 13 looks better:
> -    PCIHostState *phb = FROM_SYSBUS(PCIHostState, 
> SYS_BUS_DEVICE(s->pcihost));
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s->pcihost);
> this is an improvement: devices do not want to deal with sysbus.
> 
> The code above that is being changed looks better without.

Do you want me to apologize for my macro misuse now? I apologize. :)
Replace PCI_HOST_BRIDGE(s)->bus = b; with
PCI_HOST_BRIDGE *phb = PCI_HOST_BRIDGE(s);
phb->bus = b;
in your mind and it matches exactly what you liked better above, no?

> This is why this thread is about the specific patch and not
> general QOM.

You're basically saying, QOM was a win but don't use it here. That's a
contradiction and thus a general QOM issue since that paradigm not only
applies here: Either we need to design all structs so that their fields
have nice names to access directly as you suggest, or nobody must access
the parent field as Anthony suggests. Having a wild mix of both at
maintainers' gusto is not a good solution.

Arguably we can just leave out this last patch if it is so
controversial. However, the split is arbitrary since I backwards-coded
this series, moving code snippets to earlier individual patches using
checkout -p, i.e. patches 1-11 have this final code design in mind and
thus went from SYS_BUS_DEVICE() to PCIHostState above rather than
through parent_obj. The reason I couldn't do it there directly is that
we didn't have the PCI_HOST_BRIDGE() macro before patch 13.
I also based patch 14 on the assumption that i440fx will get more state
fields when Anthony goes ahead with his QOM Pin series, otherwise we
could just scratch the I440FXState typedef (same discussion as for the
qtest herbivore test case).

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg