| Message ID | c65908895de707a0532aa9dd09932bff329ea416.1626675354.git.elena.ufimtseva@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-user implementation | expand |
On Sun, Jul 18, 2021 at 11:27:43PM -0700, Elena Ufimtseva wrote: > From: John G Johnson <john.g.johnson@oracle.com> > > New class for vfio-user with its class and instance > constructors and destructors. > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > hw/vfio/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index bea95efc33..554b562769 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -42,6 +42,7 @@ > #include "qapi/error.h" > #include "migration/blocker.h" > #include "migration/qemu-file.h" > +#include "hw/vfio/user.h" > > #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug" > > @@ -3326,3 +3327,51 @@ static void register_vfio_pci_dev_type(void) > } > > type_init(register_vfio_pci_dev_type) > + > +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp) > +{ > + ERRP_GUARD(); > + VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); > + > + if (!udev->sock_name) { > + error_setg(errp, "No socket specified"); > + error_append_hint(errp, "Use -device vfio-user-pci,socket=<name>\n"); > + return; > + } > +} > + > +static void vfio_user_instance_finalize(Object *obj) > +{ > +} > + > +static Property vfio_user_pci_dev_properties[] = { > + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), Please use SocketAddress so that alternative socket connection details can be supported without inventing custom syntax for vfio-user-pci. For example, file descriptor passing should be possible. I think this requires a bit of command-line parsing work, so don't worry about it for now, but please add a TODO comment. When the -device vfio-user-pci syntax is finalized (i.e. when the code is merged and the device name doesn't start with the experimental x- prefix), then it needs to be solved. > + DEFINE_PROP_BOOL("secure-dma", VFIOUserPCIDevice, secure, false), I'm not sure what "secure-dma" means and the "secure" variable name is even more inscrutable. Does this mean don't share memory so that each DMA access is checked individually? > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void vfio_user_pci_dev_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass); > + > + device_class_set_props(dc, vfio_user_pci_dev_properties); > + dc->desc = "VFIO over socket PCI device assignment"; > + pdc->realize = vfio_user_pci_realize; > +} > + > +static const TypeInfo vfio_user_pci_dev_info = { > + .name = TYPE_VFIO_USER_PCI, > + .parent = TYPE_VFIO_PCI_BASE, > + .instance_size = sizeof(VFIOUserPCIDevice), > + .class_init = vfio_user_pci_dev_class_init, > + .instance_init = vfio_instance_init, > + .instance_finalize = vfio_user_instance_finalize, > +}; > + > +static void register_vfio_user_dev_type(void) > +{ > + type_register_static(&vfio_user_pci_dev_info); > +} > + > +type_init(register_vfio_user_dev_type) > -- > 2.25.1 >
> On Jul 28, 2021, at 3:16 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Sun, Jul 18, 2021 at 11:27:43PM -0700, Elena Ufimtseva wrote: >> From: John G Johnson <john.g.johnson@oracle.com> >> >> New class for vfio-user with its class and instance >> constructors and destructors. >> >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> >> --- >> hw/vfio/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 49 insertions(+) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index bea95efc33..554b562769 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -42,6 +42,7 @@ >> #include "qapi/error.h" >> #include "migration/blocker.h" >> #include "migration/qemu-file.h" >> +#include "hw/vfio/user.h" >> >> #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug" >> >> @@ -3326,3 +3327,51 @@ static void register_vfio_pci_dev_type(void) >> } >> >> type_init(register_vfio_pci_dev_type) >> + >> +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp) >> +{ >> + ERRP_GUARD(); >> + VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); >> + >> + if (!udev->sock_name) { >> + error_setg(errp, "No socket specified"); >> + error_append_hint(errp, "Use -device vfio-user-pci,socket=<name>\n"); >> + return; >> + } >> +} >> + >> +static void vfio_user_instance_finalize(Object *obj) >> +{ >> +} >> + >> +static Property vfio_user_pci_dev_properties[] = { >> + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), > > Please use SocketAddress so that alternative socket connection details > can be supported without inventing custom syntax for vfio-user-pci. For > example, file descriptor passing should be possible. > > I think this requires a bit of command-line parsing work, so don't worry > about it for now, but please add a TODO comment. When the -device > vfio-user-pci syntax is finalized (i.e. when the code is merged and the > device name doesn't start with the experimental x- prefix), then it > needs to be solved. > What do you want the options to look like at the endgame? I’d rather work backward from that than have several different flavors of options as new socket options are added. I did look at -chardev socket, and it was confusing enough that I went for the simple string. >> + DEFINE_PROP_BOOL("secure-dma", VFIOUserPCIDevice, secure, false), > > I'm not sure what "secure-dma" means and the "secure" variable name is > even more inscrutable. Does this mean don't share memory so that each > DMA access is checked individually? > Yes. Do you have another name you’d prefer? “no-shared-mem”? JJ >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void vfio_user_pci_dev_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass); >> + >> + device_class_set_props(dc, vfio_user_pci_dev_properties); >> + dc->desc = "VFIO over socket PCI device assignment"; >> + pdc->realize = vfio_user_pci_realize; >> +} >> + >> +static const TypeInfo vfio_user_pci_dev_info = { >> + .name = TYPE_VFIO_USER_PCI, >> + .parent = TYPE_VFIO_PCI_BASE, >> + .instance_size = sizeof(VFIOUserPCIDevice), >> + .class_init = vfio_user_pci_dev_class_init, >> + .instance_init = vfio_instance_init, >> + .instance_finalize = vfio_user_instance_finalize, >> +}; >> + >> +static void register_vfio_user_dev_type(void) >> +{ >> + type_register_static(&vfio_user_pci_dev_info); >> +} >> + >> +type_init(register_vfio_user_dev_type) >> -- >> 2.25.1 >>
On Thu, Jul 29, 2021 at 12:55:08AM +0000, John Johnson wrote: > > > > On Jul 28, 2021, at 3:16 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > > > > On Sun, Jul 18, 2021 at 11:27:43PM -0700, Elena Ufimtseva wrote: > >> From: John G Johnson <john.g.johnson@oracle.com> > >> > >> New class for vfio-user with its class and instance > >> constructors and destructors. > >> > >> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > >> Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > >> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > >> --- > >> hw/vfio/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 49 insertions(+) > >> > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >> index bea95efc33..554b562769 100644 > >> --- a/hw/vfio/pci.c > >> +++ b/hw/vfio/pci.c > >> @@ -42,6 +42,7 @@ > >> #include "qapi/error.h" > >> #include "migration/blocker.h" > >> #include "migration/qemu-file.h" > >> +#include "hw/vfio/user.h" > >> > >> #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug" > >> > >> @@ -3326,3 +3327,51 @@ static void register_vfio_pci_dev_type(void) > >> } > >> > >> type_init(register_vfio_pci_dev_type) > >> + > >> +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp) > >> +{ > >> + ERRP_GUARD(); > >> + VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); > >> + > >> + if (!udev->sock_name) { > >> + error_setg(errp, "No socket specified"); > >> + error_append_hint(errp, "Use -device vfio-user-pci,socket=<name>\n"); > >> + return; > >> + } > >> +} > >> + > >> +static void vfio_user_instance_finalize(Object *obj) > >> +{ > >> +} > >> + > >> +static Property vfio_user_pci_dev_properties[] = { > >> + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), > > > > Please use SocketAddress so that alternative socket connection details > > can be supported without inventing custom syntax for vfio-user-pci. For > > example, file descriptor passing should be possible. > > > > I think this requires a bit of command-line parsing work, so don't worry > > about it for now, but please add a TODO comment. When the -device > > vfio-user-pci syntax is finalized (i.e. when the code is merged and the > > device name doesn't start with the experimental x- prefix), then it > > needs to be solved. > > > > What do you want the options to look like at the endgame? I’d > rather work backward from that than have several different flavors of > options as new socket options are added. I did look at -chardev socket, > and it was confusing enough that I went for the simple string. The standard socket syntax is present in qemu-storage-daemon's --export and --nbd-server options: addr.type=inet,addr.host=<host>,addr.port=<port> addr.type=unix,addr.path=<socket-path> addr.type=fd,addr.str=<fd> --export and --nbd-server use QAPI to generate parsers for these options (they use 'SocketAddress' from qapi/sockets.json). I'm not sure whether it's easier to reuse the QAPI parser or to simply add qdev properties mimicking the same syntax. Either way, there should probably be a common qdev property API for SocketAddress values. > >> + DEFINE_PROP_BOOL("secure-dma", VFIOUserPCIDevice, secure, false), > > > > I'm not sure what "secure-dma" means and the "secure" variable name is > > even more inscrutable. Does this mean don't share memory so that each > > DMA access is checked individually? > > > > Yes. Do you have another name you’d prefer? “no-shared-mem”? I'm not sure other property names are much clearer, so feel free to stick with "secure-dma". Renaming the "secure" field to "secure_dma" and adding a comment that clarifies its purpose would be enough. Here are some options: - The vfio-user protocol message for sharing memory is called VFIO_USER_DMA_MAP. The option could be dma-map=on|off (default on). But this is based on protocol internals and may not be clear to users. - shared-mem=on|off - shared-ram=on|off Thanks, Stefan
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index bea95efc33..554b562769 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -42,6 +42,7 @@ #include "qapi/error.h" #include "migration/blocker.h" #include "migration/qemu-file.h" +#include "hw/vfio/user.h" #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug" @@ -3326,3 +3327,51 @@ static void register_vfio_pci_dev_type(void) } type_init(register_vfio_pci_dev_type) + +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp) +{ + ERRP_GUARD(); + VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); + + if (!udev->sock_name) { + error_setg(errp, "No socket specified"); + error_append_hint(errp, "Use -device vfio-user-pci,socket=<name>\n"); + return; + } +} + +static void vfio_user_instance_finalize(Object *obj) +{ +} + +static Property vfio_user_pci_dev_properties[] = { + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), + DEFINE_PROP_BOOL("secure-dma", VFIOUserPCIDevice, secure, false), + DEFINE_PROP_END_OF_LIST(), +}; + +static void vfio_user_pci_dev_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass); + + device_class_set_props(dc, vfio_user_pci_dev_properties); + dc->desc = "VFIO over socket PCI device assignment"; + pdc->realize = vfio_user_pci_realize; +} + +static const TypeInfo vfio_user_pci_dev_info = { + .name = TYPE_VFIO_USER_PCI, + .parent = TYPE_VFIO_PCI_BASE, + .instance_size = sizeof(VFIOUserPCIDevice), + .class_init = vfio_user_pci_dev_class_init, + .instance_init = vfio_instance_init, + .instance_finalize = vfio_user_instance_finalize, +}; + +static void register_vfio_user_dev_type(void) +{ + type_register_static(&vfio_user_pci_dev_info); +} + +type_init(register_vfio_user_dev_type)