* Jan Kiszka <jan.kis...@siemens.com> [2018-01-12 12:58:43 +0100]:
> On 2018-01-12 01:22, Gustavo Lima Chaves wrote:
> > Before Jailhouse claims devices, disable AER reporting altogether on
> > them, because otherwise they could hit their root complexes, which would
> > be in the root cell. When the hypervisor is disabled, turn back whatever
> > values where on those previous configurations.
> >
> > Signed-off-by: Gustavo Lima Chaves <gustavo.lima.cha...@intel.com>
> > ---
> > hypervisor/include/jailhouse/pci.h | 15 +++++++++
> > hypervisor/pci.c | 63
> > +++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/hypervisor/include/jailhouse/pci.h
> > b/hypervisor/include/jailhouse/pci.h
> > index 720ecacd..6d256613 100644
> > --- a/hypervisor/include/jailhouse/pci.h
> > +++ b/hypervisor/include/jailhouse/pci.h
> > @@ -39,6 +39,17 @@
> > #define PCI_CAP_MSIX 0x11
> > #define PCI_CAP_EXPRESS 0x10
> >
> > +#define PCI_CAP_PCIE (0x10 | JAILHOUSE_PCI_EXT_CAP)
>
> I would OR the Jailhouse flag in on use and define PCI_CAP_PCIE purely
> according to the spec here.
>
> > +#define PCIE_CONTROL_REG 0x08
> > +
> > +#define PCIE_AER_FLAGS (PCIE_DEVCTL_CERE | PCIE_DEVCTL_NFERE | \
> > + PCIE_DEVCTL_FERE | PCIE_DEVCTL_URRE)
>
> That list could also be build on use, specifically as PCIE_AER_FLAGS is
> not telling the purpose of the list.
>
> > +
> > +#define PCIE_DEVCTL_CERE 0x0001 /* Correctable Error Reporting En. */
> > +#define PCIE_DEVCTL_NFERE 0x0002 /* Non-Fatal Error Reporting Enable */
> > +#define PCIE_DEVCTL_FERE 0x0004 /* Fatal Error Reporting Enable */
> > +#define PCIE_DEVCTL_URRE 0x0008 /* Unsupported Request Reporting En. */
> > +
> > #define PCI_IVSHMEM_NUM_MMIO_REGIONS 2
> >
> > struct cell;
> > @@ -131,6 +142,10 @@ struct pci_device {
> > /** Shadow BAR */
> > u32 bar[PCI_NUM_BARS];
> >
> > + /** Shadow state of Device Control Register. */
> > + u16 dev_ctrl_reg;
> > + bool aer_override;
> > +
> > /** Shadow state of MSI config space registers. */
> > union pci_msi_registers msi_registers;
> >
> > diff --git a/hypervisor/pci.c b/hypervisor/pci.c
> > index 39f36f5f..30b733c9 100644
> > --- a/hypervisor/pci.c
> > +++ b/hypervisor/pci.c
> > @@ -537,6 +537,64 @@ static void pci_restore_msix(struct pci_device *device,
> > pci_suppress_msix(device, cap, false);
> > }
> >
> > +static void pci_suppress_aer(struct pci_device *device)
> > +{
> > + const struct jailhouse_pci_capability *cap;
> > + u16 bdf = device->info->bdf;
> > + bool found = false;
> > + u16 dev_ctrl_reg;
> > + unsigned int n;
> > +
> > + if (device->info->type == JAILHOUSE_PCI_TYPE_IVSHMEM)
> > + return;
>
> Not needed because we do not call this function for physical devices.
>
> > +
> > + for_each_pci_cap(cap, device, n)
> > + if (cap->id != PCI_CAP_EXPRESS)
> > + continue;
> > + else {
>
> if () {
>
> } else {
>
> > + found = true;
> > + break;
> > + }
> > +
> > + if (!found)
> > + return;
>
> We can probably extract some pci_get_cap(device, cap_id) that returns a
> cap pointer of NULL. Would simplify the code here and avoid the
> duplication in pci_restore_aer.
Me gusta, done.
>
> > +
> > + device->dev_ctrl_reg = pci_read_config(bdf, cap->start +
> > + PCIE_CONTROL_REG,
> > + sizeof(device->dev_ctrl_reg));
> > + dev_ctrl_reg = device->dev_ctrl_reg;
>
> Style nit:
>
> dev_ctrl_reg = pci_read_config(bdf, cap->start + PCIE_CONTROL_REG,
> sizeof(device->dev_ctrl_reg));
> device->dev_ctrl_reg = dev_ctrl_reg;
But then we pass 80 cols there. Are you sure?
>
> > + dev_ctrl_reg &= ~PCIE_AER_FLAGS;
> > + device->aer_override = true;
> > +
> > + pci_write_config(bdf, cap->start + PCIE_CONTROL_REG, dev_ctrl_reg,
> > + sizeof(dev_ctrl_reg));
> > +}
> > +
> > +static void pci_restore_aer(struct pci_device *device)
> > +{
> > + const struct jailhouse_pci_capability *cap;
> > + bool found = false;
> > + unsigned int n;
> > +
> > + if (!device->aer_override ||
> > + device->info->type == JAILHOUSE_PCI_TYPE_IVSHMEM)
>
> aer_override will not be set for IVSHMEM devices, so testing for it
> alone is enough.
>
> > + return;
> > +
> > + for_each_pci_cap(cap, device, n)
> > + if (cap->id != PCI_CAP_EXPRESS)
> > + continue;
> > + else {
> > + found = true;
> > + break;
> > + }
> > +
> > + if (!found)
> > + return;
> > +
> > + pci_write_config(device->info->bdf, cap->start + PCIE_CONTROL_REG,
> > + device->dev_ctrl_reg, sizeof(device->dev_ctrl_reg));
> > +}
> > +
> > /**
> > * Prepare the handover of PCI devices to Jailhouse or back to Linux.
> > */
> > @@ -725,8 +783,10 @@ int pci_cell_init(struct cell *cell)
> >
> > root_device = pci_get_assigned_device(&root_cell,
> > dev_infos[ndev].bdf);
> > - if (root_device)
> > + if (root_device) {
> > + pci_suppress_aer(root_device);
> > pci_remove_physical_device(root_device);
> > + }
> >
> > err = pci_add_physical_device(cell, device);
> > if (err)
> > @@ -799,6 +859,7 @@ void pci_cell_exit(struct cell *cell)
> > pci_remove_physical_device(device);
> > pci_return_device_to_root_cell(device);
> > }
> > + pci_restore_aer(device);
>
> Shouldn't that be move into the block above?
With your suggested changes in the checks for the function, yes :)
Done.
>
> > }
> >
> > page_free(&mem_pool, cell->pci_devices, devlist_pages);
> >
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RDA IOT SES-DE
> Corporate Competence Center Embedded Linux
--
Gustavo Lima Chaves
Intel - Open Source Technology Center
--
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.
