On 6/19/19 7:09 PM, Jan Kiszka wrote: > On 19.06.19 18:52, Ralf Ramsauer wrote: >> MSI-X vector tables hold 64-bit entries. So far, we used mmio_write64 to >> set them. >> >> This conforms the PCI specification: "For all accesses to MSI-X Table >> and MSI-X >> PBA fields, software must use aligned full DWORD or aligned full QWORD >> transactions; otherwise the result is undefined" (PCI Local Bus >> Specification >> Rev 3.0, chapter 8.1.2). > > Chapter 6.8.2. > >> >> Nevertheless, some vendors don't support 64-bit writes, e.g., Broadcom >> ethernet >> cards (BCM5720). mmio_write64 stalls, and the transfer won't happen. >> >> Replace mmio_write64 with a wrapper mmio_write64_split that >> substitutes the >> 64-bit write with two 32-bit write operations. This accessor first >> writes the >> upper 32 bits and then the lower 32 bits. >> >> Credits go to Jan, the root cause of this bug was found in a private >> off-list >> discussion. >> >> Signed-off-by: Ralf Ramsauer <[email protected]> >> --- >> >> Only tested on x86, untested on ARM. I lack PCI devices with MSI-X >> suppport. > > I will throw this on our Seattle eventually. > >> >> hypervisor/arch/arm-common/pci.c | 4 ++-- >> hypervisor/arch/x86/pci.c | 8 ++++---- >> hypervisor/include/jailhouse/mmio.h | 15 +++++++++++++++ >> 3 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/hypervisor/arch/arm-common/pci.c >> b/hypervisor/arch/arm-common/pci.c >> index 60ffabb8..4f35175f 100644 >> --- a/hypervisor/arch/arm-common/pci.c >> +++ b/hypervisor/arch/arm-common/pci.c >> @@ -58,8 +58,8 @@ int arch_pci_update_msi(struct pci_device *device, >> int arch_pci_update_msix_vector(struct pci_device *device, unsigned >> int index) >> { >> /* NOTE: See arch_pci_update_msi. */ >> - mmio_write64(&device->msix_table[index].address, >> - device->msix_vectors[index].address); >> + mmio_write64_split(&device->msix_table[index].address, >> + device->msix_vectors[index].address); >> mmio_write32(&device->msix_table[index].data, >> device->msix_vectors[index].data); >> return 0; >> diff --git a/hypervisor/arch/x86/pci.c b/hypervisor/arch/x86/pci.c >> index 492e9859..560c5697 100644 >> --- a/hypervisor/arch/x86/pci.c >> +++ b/hypervisor/arch/x86/pci.c >> @@ -393,8 +393,8 @@ int arch_pci_update_msix_vector(struct pci_device >> *device, unsigned int index) >> irq_msg); >> // HACK for QEMU >> if (result == -ENOSYS) { >> - mmio_write64(&device->msix_table[index].address, >> - device->msix_vectors[index].address); >> + mmio_write64_split(&device->msix_table[index].address, >> + device->msix_vectors[index].address); > > Unlikely that QEMU shares this problem, but let's be consistent. Hope we > can remove that path one day... > >> mmio_write32(&device->msix_table[index].data, >> device->msix_vectors[index].data); >> return 0; >> @@ -402,8 +402,8 @@ int arch_pci_update_msix_vector(struct pci_device >> *device, unsigned int index) >> if (result < 0) >> return result; >> - mmio_write64(&device->msix_table[index].address, >> - pci_get_x86_msi_remap_address(result)); >> + mmio_write64_split(&device->msix_table[index].address, >> + pci_get_x86_msi_remap_address(result)); >> mmio_write32(&device->msix_table[index].data, 0); >> return 0; >> diff --git a/hypervisor/include/jailhouse/mmio.h >> b/hypervisor/include/jailhouse/mmio.h >> index 61b4647e..567901d0 100644 >> --- a/hypervisor/include/jailhouse/mmio.h >> +++ b/hypervisor/include/jailhouse/mmio.h >> @@ -73,6 +73,21 @@ DEFINE_MMIO_WRITE(32) >> DEFINE_MMIO_WRITE(64) >> /** @} */ >> +/** >> + * Write a 64-bit value to a memory-mapper I/O register. Perform two >> 32-bit >> + * write operations instead of one 64-bit write operation. >> + * >> + * @param address Virtual address of the register. >> + * @param value Value to write. >> + * @{ >> + */ >> +static inline void mmio_write64_split(void *address, u64 value) >> +{ >> + mmio_write32(address + sizeof(u32), (u32)(value >> (sizeof(u32) * >> 8))); > > Why do we need to that dance around the hot pot? u32 says in its name > that it's 32 bits.
mmio_write32(address + 4, (u32)(value >> 32)); Fair enough. You can fix that up during merge, if there are no other open issues. Otherwise I'll submit a V2. Ralf > >> + mmio_write32(address, (u32)value); >> +} >> +/** @} */ >> + >> /** >> * Read value from 32 or 64-bit MMIO register field. >> * @param address Virtual address of the register. >> > > Thanks, > Jan > -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/1cecd49c-d04d-de5d-110c-5396c233c6e1%40oth-regensburg.de. For more options, visit https://groups.google.com/d/optout.
