Re: [Qemu-devel] [Qemu-ppc] [PATCH RFC 04/10] virtio-pmem: Prototype

[David Hildenbrand]

>> +static void virtio_pmem_realize(DeviceState *dev, Error **errp)
>> +{
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> +    VirtIOPMEM *pmem = VIRTIO_PMEM(dev);
>> +
>> +    if (!pmem->memdev) {
>> +        error_setg(errp, "virtio-pmem memdev not set");
>> +        return;
>> +    } else if (host_memory_backend_is_mapped(pmem->memdev)) {
>> +        char *path = 
>> object_get_canonical_path_component(OBJECT(pmem->memdev));
>> +        error_setg(errp, "can't use already busy memdev: %s", path);
>> +        g_free(path);
>> +        return;
>> +    }
> 
> Perhaps splitting this if-else block could improve readability:

Makes sense, this was kept similar to from
hw/mem/pc-dimm.c:pc_dimm_realize()

> 
>     if (!pmem->memdev) {
>         error_setg(...);
>       return;
>     }
> 
>     if (host_memory_backend_is_mapped(pmem->memdev)) {
>        /* do stuff */
>        return;
>     }
> 
>     /* do other stuffs */
> 
>> +
>> +    host_memory_backend_set_mapped(pmem->memdev, true);
>> +    virtio_init(vdev, TYPE_VIRTIO_PMEM, VIRTIO_ID_PMEM,
>> +                                          sizeof(struct 
>> virtio_pmem_config));
> 
> I'm not quite sure what's the QEMU style for indenting this.  There
> are, for example, calls to warn_report() in other source files that
> are indented at the left side after the opening parenthesis.
> 
> Perhaps indenting it like this is preferable?
> 
>     virtio_init(vdev, TYPE_VIRTIO_PMEM, VIRTIO_ID_PMEM,
>                 sizeof(struct virtio_pmem_config));

Indeed, that looks weird.

> 
>> +    pmem->rq_vq = virtio_add_queue(vdev, 128, virtio_pmem_flush);
>> +}
>> +
>> +static void virtio_pmem_unrealize(DeviceState *dev, Error **errp)
>> +{
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> +    VirtIOPMEM *pmem = VIRTIO_PMEM(dev);
>> +
>> +    host_memory_backend_set_mapped(pmem->memdev, false);
>> +    pmem->rq_vq = virtio_add_queue(vdev, 128, virtio_pmem_flush);
>> +    virtio_cleanup(vdev);
>> +}
>> +
>> +static void virtio_pmem_fill_device_info(const VirtIOPMEM *pmem,
>> +                                         VirtioPMEMDeviceInfo *vi)
>> +{
>> +    vi->memaddr = pmem->start;
>> +    vi->size = pmem->memdev ? memory_region_size(&pmem->memdev->mr) : 0;
>> +    vi->memdev = object_get_canonical_path(OBJECT(pmem->memdev));
>> +}
>> +
>> +static MemoryRegion *virtio_pmem_get_memory_region(VirtIOPMEM *pmem,
>> +                                                   Error **errp)
>> +{
>> +    if (!pmem->memdev) {
>> +        error_setg(errp, "'%s' property must be set", 
>> VIRTIO_PMEM_MEMDEV_PROP);
>> +        return NULL;
>> +    }
>> +
>> +    return &pmem->memdev->mr;
>> +}
>> +
>> +static Property virtio_pmem_properties[] = {
>> +    DEFINE_PROP_UINT64(VIRTIO_PMEM_ADDR_PROP, VirtIOPMEM, start, 0),
>> +    DEFINE_PROP_LINK(VIRTIO_PMEM_MEMDEV_PROP, VirtIOPMEM, memdev,
>> +                     TYPE_MEMORY_BACKEND, HostMemoryBackend *),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void virtio_pmem_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
>> +    VirtIOPMEMClass *vpc = VIRTIO_PMEM_CLASS(klass);
>> +
>> +    dc->props = virtio_pmem_properties;
>> +
>> +    vdc->realize = virtio_pmem_realize;
>> +    vdc->unrealize = virtio_pmem_unrealize;
>> +    vdc->get_config = virtio_pmem_get_config;
>> +    vdc->get_features = virtio_pmem_get_features;
>> +
>> +    vpc->fill_device_info = virtio_pmem_fill_device_info;
>> +    vpc->get_memory_region = virtio_pmem_get_memory_region;
>> +}
>> +
>> +static TypeInfo virtio_pmem_info = {
>> +    .name          = TYPE_VIRTIO_PMEM,
>> +    .parent        = TYPE_VIRTIO_DEVICE,
>> +    .class_size    = sizeof(VirtIOPMEMClass),
>> +    .class_init    = virtio_pmem_class_init,
>> +    .instance_size = sizeof(VirtIOPMEM),
>> +};
>> +
>> +static void virtio_register_types(void)
>> +{
>> +    type_register_static(&virtio_pmem_info);
>> +}
>> +
>> +type_init(virtio_register_types)
>> diff --git a/include/hw/virtio/virtio-pmem.h 
>> b/include/hw/virtio/virtio-pmem.h
>> new file mode 100644
>> index 0000000000..85cee3ef39
>> --- /dev/null
>> +++ b/include/hw/virtio/virtio-pmem.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * Virtio pmem device
> 
> What if "pmem" was in upper case to match the header in virtio-pmem.c?
> 
>> + *
>> + * Copyright Red Hat, Inc. 2018
> 
> This is slightly different from what is in virtio-pmem.c header.
> Perhaps "(C)" is missing here.

Thanks for the hint, that is indeed not consistent.

[...]
>> +#define TYPE_VIRTIO_PMEM "virtio-pmem"
>> +
>> +#define VIRTIO_PMEM(obj) \
>> +        OBJECT_CHECK(VirtIOPMEM, (obj), TYPE_VIRTIO_PMEM)
> 
> This indentation is slightly different from the other two macros
> below.

Will be made consistent.

Thanks!

-- 

Thanks,

David / dhildenb