Re: [Qemu-devel] [PATCH V23 2/7] Add TPM (frontend) hardware interface (TPM TIS) to QEMU

[Andreas Färber]

Am 15.02.2013 20:39, schrieb Stefan Berger:
> diff --git a/tpm/tpm_tis.c b/tpm/tpm_tis.c
> new file mode 100644
> index 0000000..565e28d
> --- /dev/null
> +++ b/tpm/tpm_tis.c
[...]
> +/*
> + * This function is called when the machine starts, resets or due to
> + * S3 resume.
> + */
> +static void tpm_tis_reset(DeviceState *d)
> +{
> +    TPMState *s = DO_UPCAST(TPMState, busdev.qdev, d);

Please introduce a QOM cast macro in tpm_int.h (e.g., TPM_TIS() or
TPM(), preferably in this patch for better review) and use that instead
of DO_UPCAST().

> +    TPMTISEmuState *tis = &s->s.tis;
> +    int c;
> +
> +    s->be_driver->ops->reset(s->be_driver);
> +
> +    tis->active_locty = TPM_TIS_NO_LOCALITY;
> +    tis->next_locty = TPM_TIS_NO_LOCALITY;
> +    tis->aborting_locty = TPM_TIS_NO_LOCALITY;
> +
> +    for (c = 0; c < TPM_TIS_NUM_LOCALITIES; c++) {
> +        tis->loc[c].access = TPM_TIS_ACCESS_TPM_REG_VALID_STS;
> +        tis->loc[c].sts = 0;
> +        tis->loc[c].inte = TPM_TIS_INT_POLARITY_LOW_LEVEL;
> +        tis->loc[c].ints = 0;
> +        tis->loc[c].state = TPM_TIS_STATE_IDLE;
> +
> +        tis->loc[c].w_offset = 0;
> +        s->be_driver->ops->realloc_buffer(&tis->loc[c].w_buffer);
> +        tis->loc[c].r_offset = 0;
> +        s->be_driver->ops->realloc_buffer(&tis->loc[c].r_buffer);
> +    }
> +
> +    tpm_tis_do_startup_tpm(s);
> +}
> +
> +static int tpm_tis_init(ISADevice *dev)

ISADeviceClass::init has been obsoleted in the meantime. Please use
DeviceClass::realize now. It has the advantage of being able to report
errors to its caller, so that all the error_report()s below can be
refactored into error_setg().

Its semantics differ slightly from qdev initfn in that only things
failing or depending on properties should be in realize. Everything else
should go into instance_init. Cf. hw/qdev-core.h.

> +{
> +    TPMState *s = DO_UPCAST(TPMState, busdev, dev);

Dito.

> +    TPMTISEmuState *tis = &s->s.tis;
> +    int rc;
> +
> +    s->be_driver = qemu_find_tpm(s->backend);
> +    if (!s->be_driver) {
> +        error_report("tpm_tis: backend driver with id %s could not be "
> +                     "found.n\n", s->backend);
> +        goto err_exit;
> +    }
> +
> +    s->be_driver->fe_model = TPM_MODEL_TPM_TIS;
> +
> +    if (s->be_driver->ops->init(s->be_driver, s, tpm_tis_receive_cb)) {
> +        goto err_exit;
> +    }
> +
> +    tis->bh = qemu_bh_new(tpm_tis_receive_bh, s);
> +
> +    if (tis->irq_num > 15) {
> +        error_report("IRQ %d for TPM TIS is outside valid range of 0 to 
> 15.\n",
> +                     tis->irq_num);
> +        goto err_exit;
> +    }
> +
> +    isa_init_irq(dev, &tis->irq, tis->irq_num);
> +

> +    memory_region_init_io(&s->mmio, &tpm_tis_memory_ops, s, "tpm-tis-mmio",
> +                          TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);

This part at least should go into an instance_init function.

> +    memory_region_add_subregion(get_system_memory(), TPM_TIS_ADDR_BASE,
> +                                &s->mmio);

Why get_system_memory() and not isa_address_space()?

> +
> +    rc = tpm_tis_do_startup_tpm(s);
> +    if (rc != 0) {
> +        goto err_destroy_memory;
> +    }
> +
> +    return 0;
> +
> + err_destroy_memory:
> +    memory_region_del_subregion(get_system_memory(), &s->mmio);

> +    memory_region_destroy(&s->mmio);

This would go into instance_finalize if initialized in instance_init.

> +
> + err_exit:
> +    return -1;
> +}
> +
> +static const VMStateDescription vmstate_tpm_tis = {
> +    .name = "tpm",
> +    .unmigratable = 1,
> +};
> +
> +static Property tpm_tis_properties[] = {
> +    DEFINE_PROP_UINT32("irq", TPMState,
> +                       s.tis.irq_num, TPM_TIS_IRQ),
> +    DEFINE_PROP_STRING("tpmdev", TPMState, backend),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tpm_tis_class_initfn(ObjectClass *klass, void *data)

Rename to ..._class_init to distinguish from qdev initfn?

> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
> +
> +    ic->init = tpm_tis_init;
> +
> +    dc->props = tpm_tis_properties;
> +    dc->reset = tpm_tis_reset;
> +    dc->vmsd  = &vmstate_tpm_tis;
> +}
> +
> +static TypeInfo tpm_tis_info = {

static const please.

> +    .name        = "tpm-tis",
> +    .parent      = TYPE_ISA_DEVICE,
> +    .class_init  = tpm_tis_class_initfn,
> +    .instance_size = sizeof(TPMState),
> +};
> +
> +static void tpm_tis_register(void)
> +{
> +    type_register_static(&tpm_tis_info);
> +}
> +
> +type_init(tpm_tis_register)

Regards,
Andreas

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