[RFC,12/19] vfio-user: probe remote device's BARs

Commit Message

Elena Ufimtseva July 19, 2021, 6:27 a.m. UTC

From: John G Johnson <[e-mail]>

Signed-off-by: Elena Ufimtseva <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
Signed-off-by: Jagannathan Raman <[e-mail]>
---
 hw/vfio/pci.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

Comments

Alex Williamson July 19, 2021, 10:59 p.m. UTC | #1

On Sun, 18 Jul 2021 23:27:51 -0700
Elena Ufimtseva <[e-mail]> wrote:
> @@ -3442,6 +3448,22 @@ static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
>      /* QEMU can also add or extend BARs */
>      memset(vdev->emulated_config_bits + PCI_BASE_ADDRESS_0, 0xff, 6 * 4);
>  
> +    /*
> +     * Local QEMU overrides aren't allowed
> +     * They must be done in the device process
> +     */
> +    if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> +        error_setg(errp, "Multi-function must be specified by device process");
> +        goto error;
> +    }
> +    if (pdev->romfile) {
> +        error_setg(errp, "Romfile must be specified by device process");
> +        goto error;
> +    }

For what reason?  PCI functions can operate completely independently,
there could be different servers for different functions, a QEMU user
may wish to apply a different option ROM image than provided by the
server.  This all creates unnecessary incompatibilities.  Thanks,

Alex

John Johnson July 20, 2021, 1:39 a.m. UTC | #2

> On Jul 19, 2021, at 3:59 PM, Alex Williamson <[e-mail]> wrote:
> 
> On Sun, 18 Jul 2021 23:27:51 -0700
> Elena Ufimtseva <[e-mail]> wrote:
>> @@ -3442,6 +3448,22 @@ static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
>>     /* QEMU can also add or extend BARs */
>>     memset(vdev->emulated_config_bits + PCI_BASE_ADDRESS_0, 0xff, 6 * 4);
>> 
>> +    /*
>> +     * Local QEMU overrides aren't allowed
>> +     * They must be done in the device process
>> +     */
>> +    if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
>> +        error_setg(errp, "Multi-function must be specified by device process");
>> +        goto error;
>> +    }
>> +    if (pdev->romfile) {
>> +        error_setg(errp, "Romfile must be specified by device process");
>> +        goto error;
>> +    }
> 
> For what reason?  PCI functions can operate completely independently,
> there could be different servers for different functions, a QEMU user
> may wish to apply a different option ROM image than provided by the
> server.  This all creates unnecessary incompatibilities.  Thanks,
> 

	The idea is to have all the device options specified on the remote
server, and have the vfio client just be a pass-through device.  I thought
having them specified in 2 places would cause more confusion.

								JJ

Alex Williamson July 20, 2021, 3:01 a.m. UTC | #3

On Tue, 20 Jul 2021 01:39:21 +0000
John Johnson <[e-mail]> wrote:

> > On Jul 19, 2021, at 3:59 PM, Alex Williamson <[e-mail]> wrote:
> > 
> > On Sun, 18 Jul 2021 23:27:51 -0700
> > Elena Ufimtseva <[e-mail]> wrote:  
> >> @@ -3442,6 +3448,22 @@ static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
> >>     /* QEMU can also add or extend BARs */
> >>     memset(vdev->emulated_config_bits + PCI_BASE_ADDRESS_0, 0xff, 6 * 4);
> >> 
> >> +    /*
> >> +     * Local QEMU overrides aren't allowed
> >> +     * They must be done in the device process
> >> +     */
> >> +    if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
> >> +        error_setg(errp, "Multi-function must be specified by device process");
> >> +        goto error;
> >> +    }
> >> +    if (pdev->romfile) {
> >> +        error_setg(errp, "Romfile must be specified by device process");
> >> +        goto error;
> >> +    }  
> > 
> > For what reason?  PCI functions can operate completely independently,
> > there could be different servers for different functions, a QEMU user
> > may wish to apply a different option ROM image than provided by the
> > server.  This all creates unnecessary incompatibilities.  Thanks,
> >   
> 
> 	The idea is to have all the device options specified on the remote
> server, and have the vfio client just be a pass-through device.  I thought
> having them specified in 2 places would cause more confusion.

IMO, the server has no business making such configuration restrictions.
It's the client's decision if it wants to create multi-function
topologies or override the option rom.  Same for whether it wants to
override or virtualize capabilities.  All of this should just work
as-is; it's actually additional code required and knowledge through the
management stack to prevent it.  Thanks,

Alex

Patch

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 054e673552..a8d2e59470 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1619,11 +1619,17 @@  static void vfio_bar_prepare(VFIOPCIDevice *vdev, int nr)
     }
 
     /* Determine what type of BAR this is for registration */
-    ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
-                vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr));
-    if (ret != sizeof(pci_bar)) {
-        error_report("vfio: Failed to read BAR %d (%m)", nr);
-        return;
+    if (vdev->vbasedev.proxy != NULL) {
+        /* during setup, config space was initialized from remote */
+        memcpy(&pci_bar, vdev->pdev.config + PCI_BASE_ADDRESS_0 + (4 * nr),
+               sizeof(pci_bar));
+    } else {
+        ret = pread(vdev->vbasedev.fd, &pci_bar, sizeof(pci_bar),
+                    vdev->config_offset + PCI_BASE_ADDRESS_0 + (4 * nr));
+        if (ret != sizeof(pci_bar)) {
+            error_report("vfio: Failed to read BAR %d (%m)", nr);
+            return;
+        }
     }
 
     pci_bar = le32_to_cpu(pci_bar);
@@ -3442,6 +3448,22 @@  static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp)
     /* QEMU can also add or extend BARs */
     memset(vdev->emulated_config_bits + PCI_BASE_ADDRESS_0, 0xff, 6 * 4);
 
+    /*
+     * Local QEMU overrides aren't allowed
+     * They must be done in the device process
+     */
+    if (pdev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
+        error_setg(errp, "Multi-function must be specified by device process");
+        goto error;
+    }
+    if (pdev->romfile) {
+        error_setg(errp, "Romfile must be specified by device process");
+        goto error;
+    }
+
+    vfio_bars_prepare(vdev);
+
+
     return;
 
  error: