| Message ID | 20190619094907.10131-5-pagupta@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Qemu virtio pmem device | expand |
On Wed, 19 Jun 2019 15:19:04 +0530 Pankaj Gupta <pagupta@redhat.com> wrote: > We need a proxy device for virtio-pmem, and this device has to be the > actual memory device so we can cleanly hotplug it. > > Forward memory device class functions either to the actual device or use > properties of the virtio-pmem device to implement these in the proxy. > > virtio-pmem will only be compiled for selected, supported architectures > (that can deal with virtio/pci devices being memory devices). An > architecture that is prepared for that can simply enable > CONFIG_VIRTIO_PMEM to make it work. > > As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM > will be enabled per supported architecture), we have to move the PCI proxy > to a separate file. > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > [ split up patches, memory-device changes, move pci proxy] > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/virtio/Makefile.objs | 1 + > hw/virtio/virtio-pmem-pci.c | 131 ++++++++++++++++++++++++++++++++++++++++++++ > hw/virtio/virtio-pmem-pci.h | 34 ++++++++++++ > include/hw/pci/pci.h | 1 + > 4 files changed, 167 insertions(+) > create mode 100644 hw/virtio/virtio-pmem-pci.c > create mode 100644 hw/virtio/virtio-pmem-pci.h (...) > +static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = { > + .base_name = TYPE_VIRTIO_PMEM_PCI, > + .generic_name = "virtio-pmem-pci", > + .transitional_name = "virtio-pmem-pci-transitional", Do we even have a transitional device for this? I.e., do we have a legacy version? I don't think that makes sense for new devices. > + .non_transitional_name = "virtio-pmem-pci-non-transitional", > + .instance_size = sizeof(VirtIOPMEMPCI), > + .instance_init = virtio_pmem_pci_instance_init, > + .class_init = virtio_pmem_pci_class_init, > + .interfaces = (InterfaceInfo[]) { > + { TYPE_MEMORY_DEVICE }, > + { } > + }, > +}; (...)
> > On Wed, 19 Jun 2019 15:19:04 +0530 > Pankaj Gupta <pagupta@redhat.com> wrote: > > > We need a proxy device for virtio-pmem, and this device has to be the > > actual memory device so we can cleanly hotplug it. > > > > Forward memory device class functions either to the actual device or use > > properties of the virtio-pmem device to implement these in the proxy. > > > > virtio-pmem will only be compiled for selected, supported architectures > > (that can deal with virtio/pci devices being memory devices). An > > architecture that is prepared for that can simply enable > > CONFIG_VIRTIO_PMEM to make it work. > > > > As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM > > will be enabled per supported architecture), we have to move the PCI proxy > > to a separate file. > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > [ split up patches, memory-device changes, move pci proxy] > > Signed-off-by: David Hildenbrand <david@redhat.com> > > --- > > hw/virtio/Makefile.objs | 1 + > > hw/virtio/virtio-pmem-pci.c | 131 > > ++++++++++++++++++++++++++++++++++++++++++++ > > hw/virtio/virtio-pmem-pci.h | 34 ++++++++++++ > > include/hw/pci/pci.h | 1 + > > 4 files changed, 167 insertions(+) > > create mode 100644 hw/virtio/virtio-pmem-pci.c > > create mode 100644 hw/virtio/virtio-pmem-pci.h > > (...) > > > +static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = { > > + .base_name = TYPE_VIRTIO_PMEM_PCI, > > + .generic_name = "virtio-pmem-pci", > > + .transitional_name = "virtio-pmem-pci-transitional", > > Do we even have a transitional device for this? I.e., do we have a > legacy version? I don't think that makes sense for new devices. No. Sure, will remove this. Thank you, Pankaj > > > + .non_transitional_name = "virtio-pmem-pci-non-transitional", > > + .instance_size = sizeof(VirtIOPMEMPCI), > > + .instance_init = virtio_pmem_pci_instance_init, > > + .class_init = virtio_pmem_pci_class_init, > > + .interfaces = (InterfaceInfo[]) { > > + { TYPE_MEMORY_DEVICE }, > > + { } > > + }, > > +}; > > (...) >
On Tue, Jul 02, 2019 at 01:55:19PM +0200, Cornelia Huck wrote: > On Wed, 19 Jun 2019 15:19:04 +0530 > Pankaj Gupta <pagupta@redhat.com> wrote: > > > We need a proxy device for virtio-pmem, and this device has to be the > > actual memory device so we can cleanly hotplug it. > > > > Forward memory device class functions either to the actual device or use > > properties of the virtio-pmem device to implement these in the proxy. > > > > virtio-pmem will only be compiled for selected, supported architectures > > (that can deal with virtio/pci devices being memory devices). An > > architecture that is prepared for that can simply enable > > CONFIG_VIRTIO_PMEM to make it work. > > > > As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM > > will be enabled per supported architecture), we have to move the PCI proxy > > to a separate file. > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > [ split up patches, memory-device changes, move pci proxy] > > Signed-off-by: David Hildenbrand <david@redhat.com> > > --- > > hw/virtio/Makefile.objs | 1 + > > hw/virtio/virtio-pmem-pci.c | 131 ++++++++++++++++++++++++++++++++++++++++++++ > > hw/virtio/virtio-pmem-pci.h | 34 ++++++++++++ > > include/hw/pci/pci.h | 1 + > > 4 files changed, 167 insertions(+) > > create mode 100644 hw/virtio/virtio-pmem-pci.c > > create mode 100644 hw/virtio/virtio-pmem-pci.h > > (...) > > > +static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = { > > + .base_name = TYPE_VIRTIO_PMEM_PCI, > > + .generic_name = "virtio-pmem-pci", > > + .transitional_name = "virtio-pmem-pci-transitional", > > Do we even have a transitional device for this? I.e., do we have a > legacy version? I don't think that makes sense for new devices. I agree - I applied so pls send a patch on top. Or if you end up having to respin pls include this. > > + .non_transitional_name = "virtio-pmem-pci-non-transitional", Neither do we need a non transitional name. > > + .instance_size = sizeof(VirtIOPMEMPCI), > > + .instance_init = virtio_pmem_pci_instance_init, > > + .class_init = virtio_pmem_pci_class_init, > > + .interfaces = (InterfaceInfo[]) { > > + { TYPE_MEMORY_DEVICE }, > > + { } > > + }, > > +}; > > (...)
> > On Tue, Jul 02, 2019 at 01:55:19PM +0200, Cornelia Huck wrote: > > On Wed, 19 Jun 2019 15:19:04 +0530 > > Pankaj Gupta <pagupta@redhat.com> wrote: > > > > > We need a proxy device for virtio-pmem, and this device has to be the > > > actual memory device so we can cleanly hotplug it. > > > > > > Forward memory device class functions either to the actual device or use > > > properties of the virtio-pmem device to implement these in the proxy. > > > > > > virtio-pmem will only be compiled for selected, supported architectures > > > (that can deal with virtio/pci devices being memory devices). An > > > architecture that is prepared for that can simply enable > > > CONFIG_VIRTIO_PMEM to make it work. > > > > > > As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM > > > will be enabled per supported architecture), we have to move the PCI > > > proxy > > > to a separate file. > > > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > > [ split up patches, memory-device changes, move pci proxy] > > > Signed-off-by: David Hildenbrand <david@redhat.com> > > > --- > > > hw/virtio/Makefile.objs | 1 + > > > hw/virtio/virtio-pmem-pci.c | 131 > > > ++++++++++++++++++++++++++++++++++++++++++++ > > > hw/virtio/virtio-pmem-pci.h | 34 ++++++++++++ > > > include/hw/pci/pci.h | 1 + > > > 4 files changed, 167 insertions(+) > > > create mode 100644 hw/virtio/virtio-pmem-pci.c > > > create mode 100644 hw/virtio/virtio-pmem-pci.h > > > > (...) > > > > > +static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = { > > > + .base_name = TYPE_VIRTIO_PMEM_PCI, > > > + .generic_name = "virtio-pmem-pci", > > > + .transitional_name = "virtio-pmem-pci-transitional", > > > > Do we even have a transitional device for this? I.e., do we have a > > legacy version? I don't think that makes sense for new devices. > > > I agree - I applied so pls send a patch on top. > Or if you end up having to respin pls include this. Sure. Thank you! > > > > + .non_transitional_name = "virtio-pmem-pci-non-transitional", > > Neither do we need a non transitional name. o.k Best regards, Pankaj > > > > + .instance_size = sizeof(VirtIOPMEMPCI), > > > + .instance_init = virtio_pmem_pci_instance_init, > > > + .class_init = virtio_pmem_pci_class_init, > > > + .interfaces = (InterfaceInfo[]) { > > > + { TYPE_MEMORY_DEVICE }, > > > + { } > > > + }, > > > +}; > > > > (...) >
On Tue, 2 Jul 2019 at 16:07, Michael S. Tsirkin <mst@redhat.com> wrote: > > From: Pankaj Gupta <pagupta@redhat.com> > > We need a proxy device for virtio-pmem, and this device has to be the > actual memory device so we can cleanly hotplug it. > > Forward memory device class functions either to the actual device or use > properties of the virtio-pmem device to implement these in the proxy. > > virtio-pmem will only be compiled for selected, supported architectures > (that can deal with virtio/pci devices being memory devices). An > architecture that is prepared for that can simply enable > CONFIG_VIRTIO_PMEM to make it work. > > As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM > will be enabled per supported architecture), we have to move the PCI proxy > to a separate file. > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > [ split up patches, memory-device changes, move pci proxy] > Signed-off-by: David Hildenbrand <david@redhat.com> > Message-Id: <20190619094907.10131-5-pagupta@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- Hi; Coverity spotted a bug here (CID 1403010): > +static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md, > + Error **errp) > +{ > + VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md); > + VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev); > + VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem); > + MemoryRegion *mr = vpc->get_memory_region(pmem, errp); > + > + /* the plugged size corresponds to the region size */ > + return mr ? 0 : memory_region_size(mr); This looks like maybe the arguments to ?: have been put the wrong way round? If mr is non-NULL we'll return 0 and if it is NULL then we'll crash because memory_region_size() dereferences mr... > +} thanks -- PMM
Hi Peter, > > From: Pankaj Gupta <pagupta@redhat.com> > > > > We need a proxy device for virtio-pmem, and this device has to be the > > actual memory device so we can cleanly hotplug it. > > > > Forward memory device class functions either to the actual device or use > > properties of the virtio-pmem device to implement these in the proxy. > > > > virtio-pmem will only be compiled for selected, supported architectures > > (that can deal with virtio/pci devices being memory devices). An > > architecture that is prepared for that can simply enable > > CONFIG_VIRTIO_PMEM to make it work. > > > > As not all architectures support memory devices (and CONFIG_VIRTIO_PMEM > > will be enabled per supported architecture), we have to move the PCI proxy > > to a separate file. > > > > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > > [ split up patches, memory-device changes, move pci proxy] > > Signed-off-by: David Hildenbrand <david@redhat.com> > > Message-Id: <20190619094907.10131-5-pagupta@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Hi; Coverity spotted a bug here (CID 1403010): > > > +static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState > > *md, > > + Error **errp) > > +{ > > + VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md); > > + VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev); > > + VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem); > > + MemoryRegion *mr = vpc->get_memory_region(pmem, errp); > > + > > + /* the plugged size corresponds to the region size */ > > + return mr ? 0 : memory_region_size(mr); > > This looks like maybe the arguments to ?: have been put > the wrong way round? If mr is non-NULL we'll return 0 > and if it is NULL then we'll crash because memory_region_size() > dereferences mr... Yes. I will send a patch to fix this. Thanks, Pankaj > > > +} > > thanks > -- PMM > >
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs index 5857e3b8e1..964ce78607 100644 --- a/hw/virtio/Makefile.objs +++ b/hw/virtio/Makefile.objs @@ -13,6 +13,7 @@ obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon.o obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += virtio-crypto-pci.o obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o +common-obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_VIRTIO_PCI)) += virtio-pmem-pci.o obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o ifeq ($(CONFIG_VIRTIO_PCI),y) diff --git a/hw/virtio/virtio-pmem-pci.c b/hw/virtio/virtio-pmem-pci.c new file mode 100644 index 0000000000..8b2d0dbccc --- /dev/null +++ b/hw/virtio/virtio-pmem-pci.c @@ -0,0 +1,131 @@ +/* + * Virtio PMEM PCI device + * + * Copyright (C) 2018-2019 Red Hat, Inc. + * + * Authors: + * Pankaj Gupta <pagupta@redhat.com> + * David Hildenbrand <david@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" + +#include "virtio-pmem-pci.h" +#include "hw/mem/memory-device.h" +#include "qapi/error.h" + +static void virtio_pmem_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp) +{ + VirtIOPMEMPCI *pmem_pci = VIRTIO_PMEM_PCI(vpci_dev); + DeviceState *vdev = DEVICE(&pmem_pci->vdev); + + qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus)); + object_property_set_bool(OBJECT(vdev), true, "realized", errp); +} + +static void virtio_pmem_pci_set_addr(MemoryDeviceState *md, uint64_t addr, + Error **errp) +{ + object_property_set_uint(OBJECT(md), addr, VIRTIO_PMEM_ADDR_PROP, errp); +} + +static uint64_t virtio_pmem_pci_get_addr(const MemoryDeviceState *md) +{ + return object_property_get_uint(OBJECT(md), VIRTIO_PMEM_ADDR_PROP, + &error_abort); +} + +static MemoryRegion *virtio_pmem_pci_get_memory_region(MemoryDeviceState *md, + Error **errp) +{ + VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md); + VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev); + VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem); + + return vpc->get_memory_region(pmem, errp); +} + +static uint64_t virtio_pmem_pci_get_plugged_size(const MemoryDeviceState *md, + Error **errp) +{ + VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md); + VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev); + VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem); + MemoryRegion *mr = vpc->get_memory_region(pmem, errp); + + /* the plugged size corresponds to the region size */ + return mr ? 0 : memory_region_size(mr); +} + +static void virtio_pmem_pci_fill_device_info(const MemoryDeviceState *md, + MemoryDeviceInfo *info) +{ + VirtioPMEMDeviceInfo *vi = g_new0(VirtioPMEMDeviceInfo, 1); + VirtIOPMEMPCI *pci_pmem = VIRTIO_PMEM_PCI(md); + VirtIOPMEM *pmem = VIRTIO_PMEM(&pci_pmem->vdev); + VirtIOPMEMClass *vpc = VIRTIO_PMEM_GET_CLASS(pmem); + DeviceState *dev = DEVICE(md); + + if (dev->id) { + vi->has_id = true; + vi->id = g_strdup(dev->id); + } + + /* let the real device handle everything else */ + vpc->fill_device_info(pmem, vi); + + info->u.virtio_pmem.data = vi; + info->type = MEMORY_DEVICE_INFO_KIND_VIRTIO_PMEM; +} + +static void virtio_pmem_pci_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass); + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass); + MemoryDeviceClass *mdc = MEMORY_DEVICE_CLASS(klass); + + k->realize = virtio_pmem_pci_realize; + set_bit(DEVICE_CATEGORY_MISC, dc->categories); + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET; + pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_PMEM; + pcidev_k->revision = VIRTIO_PCI_ABI_VERSION; + pcidev_k->class_id = PCI_CLASS_OTHERS; + + mdc->get_addr = virtio_pmem_pci_get_addr; + mdc->set_addr = virtio_pmem_pci_set_addr; + mdc->get_plugged_size = virtio_pmem_pci_get_plugged_size; + mdc->get_memory_region = virtio_pmem_pci_get_memory_region; + mdc->fill_device_info = virtio_pmem_pci_fill_device_info; +} + +static void virtio_pmem_pci_instance_init(Object *obj) +{ + VirtIOPMEMPCI *dev = VIRTIO_PMEM_PCI(obj); + + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev), + TYPE_VIRTIO_PMEM); +} + +static const VirtioPCIDeviceTypeInfo virtio_pmem_pci_info = { + .base_name = TYPE_VIRTIO_PMEM_PCI, + .generic_name = "virtio-pmem-pci", + .transitional_name = "virtio-pmem-pci-transitional", + .non_transitional_name = "virtio-pmem-pci-non-transitional", + .instance_size = sizeof(VirtIOPMEMPCI), + .instance_init = virtio_pmem_pci_instance_init, + .class_init = virtio_pmem_pci_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_MEMORY_DEVICE }, + { } + }, +}; + +static void virtio_pmem_pci_register_types(void) +{ + virtio_pci_types_register(&virtio_pmem_pci_info); +} +type_init(virtio_pmem_pci_register_types) diff --git a/hw/virtio/virtio-pmem-pci.h b/hw/virtio/virtio-pmem-pci.h new file mode 100644 index 0000000000..616abef093 --- /dev/null +++ b/hw/virtio/virtio-pmem-pci.h @@ -0,0 +1,34 @@ +/* + * Virtio PMEM PCI device + * + * Copyright (C) 2018-2019 Red Hat, Inc. + * + * Authors: + * Pankaj Gupta <pagupta@redhat.com> + * David Hildenbrand <david@redhat.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. + * See the COPYING file in the top-level directory. + */ + +#ifndef QEMU_VIRTIO_PMEM_PCI_H +#define QEMU_VIRTIO_PMEM_PCI_H + +#include "hw/virtio/virtio-pci.h" +#include "hw/virtio/virtio-pmem.h" + +typedef struct VirtIOPMEMPCI VirtIOPMEMPCI; + +/* + * virtio-pmem-pci: This extends VirtioPCIProxy. + */ +#define TYPE_VIRTIO_PMEM_PCI "virtio-pmem-pci-base" +#define VIRTIO_PMEM_PCI(obj) \ + OBJECT_CHECK(VirtIOPMEMPCI, (obj), TYPE_VIRTIO_PMEM_PCI) + +struct VirtIOPMEMPCI { + VirtIOPCIProxy parent_obj; + VirtIOPMEM vdev; +}; + +#endif /* QEMU_VIRTIO_PMEM_PCI_H */ diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index edf44de21d..097feb2cb7 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -85,6 +85,7 @@ extern bool pci_available; #define PCI_DEVICE_ID_VIRTIO_RNG 0x1005 #define PCI_DEVICE_ID_VIRTIO_9P 0x1009 #define PCI_DEVICE_ID_VIRTIO_VSOCK 0x1012 +#define PCI_DEVICE_ID_VIRTIO_PMEM 0x1013 #define PCI_VENDOR_ID_REDHAT 0x1b36 #define PCI_DEVICE_ID_REDHAT_BRIDGE 0x0001