On 2018-01-12 01:22, Gustavo Lima Chaves wrote:
> Try our best to follow more closely what the specification mandates for
> function-level reset (section 6.6.2 of PCI Express Base Specification,
> Revision 3.0). The previous approach was writing blindly on the command
> register (setting int. disable bit), but that would lead to an
> unsupported request on functions that do not expect that right away.
>
> We now do checks for FLR support, proceed to the previous command
> register write, wait for any pending transactions for some cycles (used
> as a placeholder for the suggested 100ms timeout), raise the 'initiate
> FLR' bit on the right register, wait on the same kind of loop again and,
> finally, clear the transactions pending bit forcefully, as mandated.
>
> This should leave us with better support for PCI devices out there.
>
> Signed-off-by: Gustavo Lima Chaves <gustavo.lima.cha...@intel.com>
> ---
> hypervisor/include/jailhouse/pci.h | 5 +++
> hypervisor/pci.c | 82
> +++++++++++++++++++++++++++++++++++---
> 2 files changed, 82 insertions(+), 5 deletions(-)
>
> diff --git a/hypervisor/include/jailhouse/pci.h
> b/hypervisor/include/jailhouse/pci.h
> index 6d256613..00d52099 100644
> --- a/hypervisor/include/jailhouse/pci.h
> +++ b/hypervisor/include/jailhouse/pci.h
> @@ -40,7 +40,12 @@
> #define PCI_CAP_EXPRESS 0x10
>
> #define PCI_CAP_PCIE (0x10 | JAILHOUSE_PCI_EXT_CAP)
> +#define PCIE_DEV_CAPS_REG 0x04
> +# define PCI_DEV_CAPS_FLR (1 << 28)
> #define PCIE_CONTROL_REG 0x08
> +# define PCI_CONTROL_INIT_FLR (1 << 15)
> +#define PCIE_STATUS_REG 0x0a
> +# define PCI_STATUS_PENDING (1 << 5)
>
> #define PCIE_AER_FLAGS (PCIE_DEVCTL_CERE | PCIE_DEVCTL_NFERE | \
> PCIE_DEVCTL_FERE | PCIE_DEVCTL_URRE)
> diff --git a/hypervisor/pci.c b/hypervisor/pci.c
> index 30b733c9..f74f6399 100644
> --- a/hypervisor/pci.c
> +++ b/hypervisor/pci.c
> @@ -21,6 +21,8 @@
>
> #define MSIX_VECTOR_CTRL_DWORD 3
>
> +#define PCI_RESET_TIMEOUT 250
Where was this value derived from? What is its unit (it's apparently
"loops" or "cycles")?
Style: indention.
> +
> #define for_each_configured_pci_device(dev, cell) \
> for ((dev) = (cell)->pci_devices; \
> (dev) - (cell)->pci_devices < (cell)->config->num_pci_devices; \
> @@ -618,10 +620,13 @@ void pci_prepare_handover(void)
> }
> }
>
> -void pci_reset_device(struct pci_device *device)
> +static void pci_reset_device_internal(struct pci_device *device)
> {
> const struct jailhouse_pci_capability *cap;
> + unsigned timeout = PCI_RESET_TIMEOUT;
> + bool found = false;
> unsigned int n;
> + u32 val;
>
> memset(&device->msi_registers, 0, sizeof(device->msi_registers));
> for (n = 0; n < device->info->num_msix_vectors; n++) {
> @@ -635,13 +640,81 @@ void pci_reset_device(struct pci_device *device)
> return;
> }
>
> + for_each_pci_cap(cap, device, n) {
> + if (cap->id == PCI_CAP_PCIE) {
> + found = true;
> + break;
> + }
> + }
> +
> + if (!found)
> + return;
Another use case for pci_get_cap().
> +
> + val = pci_read_config(device->info->bdf,
> + cap->start + PCIE_DEV_CAPS_REG, 4);
> +
> + if ((val & PCI_DEV_CAPS_FLR) != PCI_DEV_CAPS_FLR) {
> + printk("Skipping function level reset for "
> + "device %02x:%02x.%x: no support\n",
> + PCI_BDF_PARAMS(device->info->bdf));
> + return;
> + }
> +
> /*
> - * Silence INTx of the physical device by setting the mask bit.
> - * This is a deviation from the specified reset state.
> + * "When a 0 is written to this register, the device is
> + * logically disconnected from the PCI bus for all accesses
> + * except configuration accesses."
> + *
> + * The bit that we set on that mask will silence INTx of the
> + * physical device.
> */
> pci_write_config(device->info->bdf, PCI_CFG_COMMAND,
> PCI_CMD_INTX_OFF, 2);
>
> + /*
> + * Wait for any pending transaction to end. Ideally we should
> + * count for 100ms, but we count arbitrary loops for now
> + * instead, by lack of timer notion in the hypervisor.
> + */
> + do {
> + val = pci_read_config(device->info->bdf,
> + cap->start + PCIE_STATUS_REG, 2);
> + cpu_relax();
> + timeout--;
> + } while ((val & PCI_STATUS_PENDING) == PCI_STATUS_PENDING
> + || timeout);
while (val & PCI_STATUS_PENDING || timeout > 0)
But what if timeout reaches 0? Just ignore STATUS_PENDING??
> +
> + /*
> + * Set "initiate function level reset" bit and wait a little
> + * bit more.
> + */
> + val = pci_read_config(device->info->bdf,
> + cap->start + PCIE_CONTROL_REG, 2);
Redundant, we have the current value already.
> + pci_write_config(device->info->bdf, cap->start + PCIE_CONTROL_REG,
> + val & PCI_CONTROL_INIT_FLR, 2);
> + timeout = PCI_RESET_TIMEOUT;
> + while (timeout) {
> + cpu_relax();
> + timeout--;
> + }
These two timeout loops have totally different time spans because one is
throttled by PCI accesses while the other is just a trivial counter. I
suspect we can either drop that second loop completely or have to
properly throttle it as well.
What does the spec say here?
> +
> + /*
> + * "The Transactions Pending bit must be cleared upon completion
> + * of the FLR."
> + */
> + val = pci_read_config(device->info->bdf,
> + cap->start + PCIE_STATUS_REG, 2);
Again: don't we have the current value already?
> + pci_write_config(device->info->bdf, cap->start + PCIE_STATUS_REG,
> + val & ~PCI_STATUS_PENDING, 2);
> +}
> +
> +void pci_reset_device(struct pci_device *device)
> +{
> + const struct jailhouse_pci_capability *cap;
> + unsigned int n;
> +
> + pci_reset_device_internal(device);
> +
> for_each_pci_cap(cap, device, n) {
> if (cap->id == PCI_CAP_MSI || cap->id == PCI_CAP_MSIX)
> /* Disable MSI/MSI-X by clearing the control word. */
> @@ -937,7 +1010,6 @@ void pci_shutdown(void)
> pci_restore_msix(device, cap);
>
> if (device->cell != &root_cell)
> - pci_write_config(device->info->bdf, PCI_CFG_COMMAND,
> - PCI_CMD_INTX_OFF, 2);
> + pci_reset_device_internal(device);
> }
> }
>
Jan
--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
--
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 jailhouse-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
