On Wed, Jan 14, 2015 at 12:41:59PM +1100, David Gibson wrote: >On Mon, Jan 05, 2015 at 11:26:28AM +1100, Gavin Shan wrote: >> The patch implements sPAPRPHBClass::eeh_handler so that the >> EEH RTAS requests can be routed to VFIO for further handling. >> >> Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci_vfio.c | 56 >> +++++++++++++++++++++++++++++++++++++++++++++++++ >> hw/vfio/common.c | 1 + >> 2 files changed, 57 insertions(+) >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index 144912b..73652a9 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -71,6 +71,61 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState >> *sphb, Error **errp) >> spapr_tce_get_iommu(tcet)); >> } >> >> +static int spapr_phb_vfio_eeh_handler(sPAPRPHBState *sphb, int req, int opt) >> +{ >> + sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); >> + struct vfio_eeh_pe_op op = { .argsz = sizeof(op) }; > >This is a local variable, which means it won't be initialized. You >never memset() it and it's not obvious that all fields get >initialized, which makes it dangerous to pass to an ioctl(). >
Yeah, we might get uncertain "flags" passed to ioctl(). I'll change the code like this if you agree: struct vfio_eeh_pe_op op; memset(&op, 0, sizeof(op)); op.argsz = sizeof(op); >> + int cmd; >> + >> + switch (req) { >> + case RTAS_EEH_REQ_SET_OPTION: >> + switch (opt) { >> + case RTAS_EEH_DISABLE: >> + cmd = VFIO_EEH_PE_DISABLE; >> + break; >> + case RTAS_EEH_ENABLE: >> + cmd = VFIO_EEH_PE_ENABLE; >> + break; >> + case RTAS_EEH_THAW_IO: >> + cmd = VFIO_EEH_PE_UNFREEZE_IO; >> + break; >> + case RTAS_EEH_THAW_DMA: >> + cmd = VFIO_EEH_PE_UNFREEZE_DMA; >> + break; >> + default: >> + return -EINVAL; >> + } >> + break; >> + case RTAS_EEH_REQ_GET_STATE: >> + cmd = VFIO_EEH_PE_GET_STATE; >> + break; >> + case RTAS_EEH_REQ_RESET: >> + switch (opt) { >> + case RTAS_SLOT_RESET_DEACTIVATE: >> + cmd = VFIO_EEH_PE_RESET_DEACTIVATE; >> + break; >> + case RTAS_SLOT_RESET_HOT: >> + cmd = VFIO_EEH_PE_RESET_HOT; >> + break; >> + case RTAS_SLOT_RESET_FUNDAMENTAL: >> + cmd = VFIO_EEH_PE_RESET_FUNDAMENTAL; >> + break; >> + default: >> + return -EINVAL; >> + } >> + break; >> + case RTAS_EEH_REQ_CONFIGURE: >> + cmd = VFIO_EEH_PE_CONFIGURE; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + op.op = cmd; >> + return vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, >> + VFIO_EEH_PE_OP, &op); > >Don't you need some sort of translation from the errnos the ioctl() >returns into RTAS error codes? > Currently, errors from ioctl() is simply translated to RTAS_OUT_HW_ERROR if the RTAS call supports it. Otherwise, RTAS_OUT_PARAM_ERROR will be returned. Thanks, Gavin >> +} >> + >> static void spapr_phb_vfio_reset(DeviceState *qdev) >> { >> /* Do nothing */ >> @@ -84,6 +139,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, >> void *data) >> dc->props = spapr_phb_vfio_properties; >> dc->reset = spapr_phb_vfio_reset; >> spc->finish_realize = spapr_phb_vfio_finish_realize; >> + spc->eeh_handler = spapr_phb_vfio_eeh_handler; >> } >> >> static const TypeInfo spapr_phb_vfio_info = { >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index cf483ff..8a10c8b 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -948,6 +948,7 @@ int vfio_container_ioctl(AddressSpace *as, int32_t >> groupid, >> switch (req) { >> case VFIO_CHECK_EXTENSION: >> case VFIO_IOMMU_SPAPR_TCE_GET_INFO: >> + case VFIO_EEH_PE_OP: >> break; >> default: >> /* Return an error on unknown requests */ > >-- >David Gibson | I'll have my music baroque, and my code >david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! >http://www.ozlabs.org/~dgibson