Re: [Qemu-devel] Hot-pluggable device without ->unrealize() is highly suspect

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Fri, 30 Nov 2018 at 07:40, Markus Armbruster <address@hidden> wrote:
>> Peter Maydell <address@hidden> writes:
>> > Add an assert somewhere and catch it with the usual
>> > "instantiate everything" qtest?
>
>> The troublemaker is (3), where we may end up with an overridden
>> realize-like method and an non-matching, un-overridden unrealize-like
>> method.  Got any smart ideas?
>
> Mmm, I see the difficulty. I suppose in theory we could have
> a custom linter like clang-tidy and check that anything that
> sets a realize pointer also sets an unrealize, but that's
> pie-in-the-sky territory at the moment.
>
> Asserting that there is an unrealize would catch at least
> some cases of "forgot to handle hotplug", though, right?

Yes.  I figure it's worth doing.

Another idea: track down all the instances of "parent class implements
::realize(), ::unrealize(), which in turn call methods its children may
implement", and rename the children's methods to be called realize(),
unrealize(), so that we can find offenders with a git-grep -E
'->(un)?realize *='.

>> > More generally, what is causing dc390 to be hotpluggable?
>> > I can't see anything obvious in the class init.
>>
>> It's a PCI device.  These are all hot-pluggable by default.
>
> Maybe we should change that? Most people writing a
> PCI device are probably not thinking about hotplug.

More on that below.

> Though in the dc390 case it would probably not help to have
> to set a dc->hotpluggable flag, because it would inherit that
> from am53c974 too.
>
> I wonder if the dc390 could be structured in a way that
> it doesn't subclass a complete non-abstract pci device like
> that...

Yes, a common abstract base class would be cleaner.

>> >                                                 If we
>> > have APIs where the default is "you get this weird thing
>> > you weren't thinking about but it's broken because
>> > you weren't thinking about it" then we will have a whole
>> > class of bugs that we then need to squash device by device[*].
>> > I think it would be better for devices to have to explicitly
>> > opt in to implementing things like hotplug -- that way the
>> > failure mode is just "this isn't hotpluggable", we can
>> > report that helpfully to the user, and if anybody cares
>> > they can add the support.
>>
>> I tend to agree, although for PCI devices, not being hot-pluggable is
>> kind of weird, except for the ones that only occur soldered onto
>> mainboards.
>
> Real hardware PCI devices are not hot-pluggable by default
> as far as I'm aware; PCIe may be hot-pluggable but I think
> need to be designed to support it.
> This is a random non-hotplug-safe PCIe card:
> https://i.stack.imgur.com/tXug5.jpg
> PRSNT2# (pin 17 side B) is connected to Ground on pin 18
> (these are the rightmost two connections visible); for
> hotplug it must be connected to PRSNT1# (which is pin 1 side A),
> AIUI.
>
> But the hardware situation is kind of a tangent from how
> we try to design our APIs.

It's best not to deviate from real hardware without a good reason.
Making it harder to create the same stupid bugs over and over again is a
good reason.