| Message ID | 1531415537-26037-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/bcm2836: Mark the bcm2836 / bcm2837 devices with user_creatable = false | expand |
On 12 July 2018 at 18:12, Thomas Huth <thuth@redhat.com> wrote: > These devices are currently causing some problems when a user is trying > to hot-plug or introspect them during runtime. Since these devices can > not be instantiated by the user at all (they need to be wired up in code > instead), we should mark them with user_creatable = false anyway, then we > avoid at least the crashes with the hot-plugging. The introspection problem > will be handled by a separate patch. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- I thought we'd changed things so all sysbus devices defaulted to not-user-createable? Am I confusing that with something else? (The set of user-creatable sysbus devices must be very small...) thanks -- PMM
On 16.07.2018 15:39, Peter Maydell wrote: > On 12 July 2018 at 18:12, Thomas Huth <thuth@redhat.com> wrote: >> These devices are currently causing some problems when a user is trying >> to hot-plug or introspect them during runtime. Since these devices can >> not be instantiated by the user at all (they need to be wired up in code >> instead), we should mark them with user_creatable = false anyway, then we >> avoid at least the crashes with the hot-plugging. The introspection problem >> will be handled by a separate patch. > > I thought we'd changed things so all sysbus devices defaulted > to not-user-createable? Am I confusing that with something else? > (The set of user-creatable sysbus devices must be very small...) You remember it right, but TYPE_BCM283X is not a sysbus device, it is declared with ".parent = TYPE_DEVICE" instead. Thomas
On 16 July 2018 at 14:44, Thomas Huth <thuth@redhat.com> wrote: > On 16.07.2018 15:39, Peter Maydell wrote: >> On 12 July 2018 at 18:12, Thomas Huth <thuth@redhat.com> wrote: >>> These devices are currently causing some problems when a user is trying >>> to hot-plug or introspect them during runtime. Since these devices can >>> not be instantiated by the user at all (they need to be wired up in code >>> instead), we should mark them with user_creatable = false anyway, then we >>> avoid at least the crashes with the hot-plugging. The introspection problem >>> will be handled by a separate patch. >> >> I thought we'd changed things so all sysbus devices defaulted >> to not-user-createable? Am I confusing that with something else? >> (The set of user-creatable sysbus devices must be very small...) > > You remember it right, but TYPE_BCM283X is not a sysbus device, it is > declared with ".parent = TYPE_DEVICE" instead. So it is. (Which sort of makes sense I suppose, though we're not entirely consistent about whether SoC containers are sysbus or plain device.) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> and I'll apply this to target-arm.next for 3.0. thanks -- PMM
Thomas Huth <thuth@redhat.com> writes: > These devices are currently causing some problems when a user is trying > to hot-plug or introspect them during runtime. Since these devices can > not be instantiated by the user at all (they need to be wired up in code > instead), we should mark them with user_creatable = false anyway, then we > avoid at least the crashes with the hot-plugging. The introspection problem > will be handled by a separate patch. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/arm/bcm2836.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 6805a7d..45d9e40 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -185,6 +185,8 @@ static void bcm283x_class_init(ObjectClass *oc, void *data) > bc->info = data; > dc->realize = bcm2836_realize; > dc->props = bcm2836_props; > + /* Reason: Must be wired up in code (see raspi_init() function) */ > + dc->user_creatable = false; > } > > static const TypeInfo bcm283x_type_info = { A more common way to phrase this is /* Reason: needs to be wired-up by raspi_init() */ If you you expect other functions to wire this one up in the future, insert ", e.g." before "by". I like consistency in such things, but it's matter of taste, thus: Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 16.07.2018 16:18, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> These devices are currently causing some problems when a user is trying >> to hot-plug or introspect them during runtime. Since these devices can >> not be instantiated by the user at all (they need to be wired up in code >> instead), we should mark them with user_creatable = false anyway, then we >> avoid at least the crashes with the hot-plugging. The introspection problem >> will be handled by a separate patch. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/arm/bcm2836.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >> index 6805a7d..45d9e40 100644 >> --- a/hw/arm/bcm2836.c >> +++ b/hw/arm/bcm2836.c >> @@ -185,6 +185,8 @@ static void bcm283x_class_init(ObjectClass *oc, void *data) >> bc->info = data; >> dc->realize = bcm2836_realize; >> dc->props = bcm2836_props; >> + /* Reason: Must be wired up in code (see raspi_init() function) */ >> + dc->user_creatable = false; >> } >> >> static const TypeInfo bcm283x_type_info = { > > A more common way to phrase this is > > /* Reason: needs to be wired-up by raspi_init() */ > > If you you expect other functions to wire this one up in the future, > insert ", e.g." before "by". $ grep -r Reason.*wire hw/ hw/acpi/piix4.c: * Reason: part of PIIX4 southbridge, needs to be wired up, hw/core/or-irq.c: /* Reason: Needs to be wired up to work, e.g. see stm32f205_soc.c */ hw/core/register.c: /* Reason: needs to be wired up to work */ hw/core/split-irq.c: /* Reason: Needs to be wired up to work */ hw/dma/i8257.c: /* Reason: needs to be wired up by isa_bus_dma() to work */ hw/i2c/smbus_ich9.c: * Reason: part of ICH9 southbridge, needs to be wired up by hw/ide/microdrive.c: /* Reason: Needs to be wired-up in code, see dscm1xxxx_init() */ hw/intc/apic_common.c: * Reason: APIC and CPU need to be wired up by hw/intc/nios2_iic.c: /* Reason: needs to be wired up, e.g. by nios2_10m50_ghrd_init() */ hw/isa/piix4.c: * Reason: part of PIIX4 southbridge, needs to be wired up, hw/isa/vt82c686.c: * Reason: part of VIA VT82C686 southbridge, needs to be wired up, hw/isa/lpc_ich9.c: * Reason: part of ICH9 southbridge, needs to be wired up by hw/pci-host/piix.c: * Reason: part of PIIX3 southbridge, needs to be wired up by hw/pci-host/piix.c: /* Reason: needs to be wired up by pc_init1 */ hw/pci-host/q35.c: /* Reason: needs to be wired up by pc_q35_init */ hw/timer/mc146818rtc.c: /* Reason: needs to be wired up by rtc_init() */ ... does not look very consistent to me. But if Peter feels like it should be changed, too, please feel free to modify the comment when picking up the patch (I won't resend it again just because of this). > I like consistency in such things, but it's matter of taste, thus: > Reviewed-by: Markus Armbruster <armbru@redhat.com> Thanks, Thomas
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 6805a7d..45d9e40 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -185,6 +185,8 @@ static void bcm283x_class_init(ObjectClass *oc, void *data) bc->info = data; dc->realize = bcm2836_realize; dc->props = bcm2836_props; + /* Reason: Must be wired up in code (see raspi_init() function) */ + dc->user_creatable = false; } static const TypeInfo bcm283x_type_info = {
These devices are currently causing some problems when a user is trying to hot-plug or introspect them during runtime. Since these devices can not be instantiated by the user at all (they need to be wired up in code instead), we should mark them with user_creatable = false anyway, then we avoid at least the crashes with the hot-plugging. The introspection problem will be handled by a separate patch. Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/arm/bcm2836.c | 2 ++ 1 file changed, 2 insertions(+)