| Message ID | 20200326152438.6218-26-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add reassignable BARs and PCIE 1.1 support | expand |
On 26/03/2020 15:24, Alexandru Elisei wrote: > After writing to the device fd as part of the PCI configuration space > emulation, we read back from the device to make sure that the write > finished. The value is read back into the PCI configuration space and > afterwards, the same value is copied by the PCI emulation code. Let's > read from the device fd into a temporary variable, to prevent this > double write. > > The double write is harmless in itself. But when we implement > reassignable BARs, we need to keep track of the old BAR value, and the > VFIO code is overwritting it. > It seems still a bit fragile, since we rely on code in other places to limit "sz" to 4 or less, but in practice we should be covered. Can you maybe add an assert here to prevent accidents on the stack? > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> Cheers, Andre > --- > vfio/pci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/vfio/pci.c b/vfio/pci.c > index fe02574390f6..8b2a0c8dbac3 100644 > --- a/vfio/pci.c > +++ b/vfio/pci.c > @@ -470,7 +470,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd > struct vfio_region_info *info; > struct vfio_pci_device *pdev; > struct vfio_device *vdev; > - void *base = pci_hdr; > + u32 tmp; > > if (offset == PCI_ROM_ADDRESS) > return; > @@ -490,7 +490,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd > if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI) > vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz); > > - if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz) > + if (pread(vdev->fd, &tmp, sz, info->offset + offset) != sz) > vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x", > sz, offset); > } >
Hi, On 4/2/20 9:35 AM, André Przywara wrote: > On 26/03/2020 15:24, Alexandru Elisei wrote: >> After writing to the device fd as part of the PCI configuration space >> emulation, we read back from the device to make sure that the write >> finished. The value is read back into the PCI configuration space and >> afterwards, the same value is copied by the PCI emulation code. Let's >> read from the device fd into a temporary variable, to prevent this >> double write. >> >> The double write is harmless in itself. But when we implement >> reassignable BARs, we need to keep track of the old BAR value, and the >> VFIO code is overwritting it. >> > It seems still a bit fragile, since we rely on code in other places to > limit "sz" to 4 or less, but in practice we should be covered. > > Can you maybe add an assert here to prevent accidents on the stack? Sure, sounds reasonable. Thanks, Alex > >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > Cheers, > Andre > >> --- >> vfio/pci.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/vfio/pci.c b/vfio/pci.c >> index fe02574390f6..8b2a0c8dbac3 100644 >> --- a/vfio/pci.c >> +++ b/vfio/pci.c >> @@ -470,7 +470,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd >> struct vfio_region_info *info; >> struct vfio_pci_device *pdev; >> struct vfio_device *vdev; >> - void *base = pci_hdr; >> + u32 tmp; >> >> if (offset == PCI_ROM_ADDRESS) >> return; >> @@ -490,7 +490,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd >> if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI) >> vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz); >> >> - if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz) >> + if (pread(vdev->fd, &tmp, sz, info->offset + offset) != sz) >> vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x", >> sz, offset); >> } >>
diff --git a/vfio/pci.c b/vfio/pci.c index fe02574390f6..8b2a0c8dbac3 100644 --- a/vfio/pci.c +++ b/vfio/pci.c @@ -470,7 +470,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd struct vfio_region_info *info; struct vfio_pci_device *pdev; struct vfio_device *vdev; - void *base = pci_hdr; + u32 tmp; if (offset == PCI_ROM_ADDRESS) return; @@ -490,7 +490,7 @@ static void vfio_pci_cfg_write(struct kvm *kvm, struct pci_device_header *pci_hd if (pdev->irq_modes & VFIO_PCI_IRQ_MODE_MSI) vfio_pci_msi_cap_write(kvm, vdev, offset, data, sz); - if (pread(vdev->fd, base + offset, sz, info->offset + offset) != sz) + if (pread(vdev->fd, &tmp, sz, info->offset + offset) != sz) vfio_dev_warn(vdev, "Failed to read %d bytes from Configuration Space at 0x%x", sz, offset); }
After writing to the device fd as part of the PCI configuration space emulation, we read back from the device to make sure that the write finished. The value is read back into the PCI configuration space and afterwards, the same value is copied by the PCI emulation code. Let's read from the device fd into a temporary variable, to prevent this double write. The double write is harmless in itself. But when we implement reassignable BARs, we need to keep track of the old BAR value, and the VFIO code is overwritting it. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- vfio/pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)