| Message ID | 1402365785-31620-4-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2014-06-10 at 12:03 +1000, 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 <gwshan@linux.vnet.ibm.com> > --- > hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > index 592d6a4..9750cf0 100644 > --- a/hw/ppc/spapr_pci_vfio.c > +++ b/hw/ppc/spapr_pci_vfio.c > @@ -85,6 +85,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), .flags = 0 }; FWIW, flags = 0 isn't actually necessary. I'm sure someone here can quote the C spec, but it's my understanding that if any field of a structure is initialized, the remaining fields are zero initialized. vfio.c has a mix of initializations depending on whether using an explicit value for flags adds to the code clarity. > + 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); > +} > + > static void spapr_phb_vfio_reset(DeviceState *qdev) > { > /* Do nothing */ > @@ -98,6 +153,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 = {
On Wed, Jun 11, 2014 at 02:26:51PM -0600, Alex Williamson wrote: >On Tue, 2014-06-10 at 12:03 +1000, 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 <gwshan@linux.vnet.ibm.com> >> --- >> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> index 592d6a4..9750cf0 100644 >> --- a/hw/ppc/spapr_pci_vfio.c >> +++ b/hw/ppc/spapr_pci_vfio.c >> @@ -85,6 +85,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), .flags = 0 }; > >FWIW, flags = 0 isn't actually necessary. I'm sure someone here can >quote the C spec, but it's my understanding that if any field of a >structure is initialized, the remaining fields are zero initialized. >vfio.c has a mix of initializations depending on whether using an >explicit value for flags adds to the code clarity. > Yes, but it's not harmful. Please let me know if you want me to remove it :-) I had a very quick experiment on x86 and Power Linux with following tiny program and the result is just what you think: With "struct test foo" in func2(): func2: foo.a=0xffffffff, foo.b=0xffffffff with "static struct test foo" in func2(). Here's the explaining about this: section 2.4.2.3 of http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members func2: foo.a=0x00000000, foo.b=0x00000000 with "struct test foo = { .a = 0 }" in func2(). func2: foo.a=0x00000000, foo.b=0x00000000 With "struct test foo = { 0 }" in func2(): func2: foo.a=0x00000000, foo.b=0x00000000 --- #include <stdio.h> struct test { int a; int b; }; static func1(void) { int var[1000]; int i; for (i = 0; i < 1000; i++) var[i] = 0xffffffff; } static func2(void) { struct test foo; printf("%s: foo.a=0x%08x, foo.b=0x%08x\n", __func__, foo.a, foo.b); } int main(int argc, char **argv) { func1(); func2(); return 0; } Thanks, Gavin >> + 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); >> +} >> + >> static void spapr_phb_vfio_reset(DeviceState *qdev) >> { >> /* Do nothing */ >> @@ -98,6 +153,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 = { >
On Thu, 2014-06-12 at 10:02 +1000, Gavin Shan wrote: > On Wed, Jun 11, 2014 at 02:26:51PM -0600, Alex Williamson wrote: > >On Tue, 2014-06-10 at 12:03 +1000, 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 <gwshan@linux.vnet.ibm.com> > >> --- > >> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 56 insertions(+) > >> > >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > >> index 592d6a4..9750cf0 100644 > >> --- a/hw/ppc/spapr_pci_vfio.c > >> +++ b/hw/ppc/spapr_pci_vfio.c > >> @@ -85,6 +85,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), .flags = 0 }; > > > >FWIW, flags = 0 isn't actually necessary. I'm sure someone here can > >quote the C spec, but it's my understanding that if any field of a > >structure is initialized, the remaining fields are zero initialized. > >vfio.c has a mix of initializations depending on whether using an > >explicit value for flags adds to the code clarity. > > > > Yes, but it's not harmful. Please let me know if you want me to remove > it :-) It's ok, explicit initialization doesn't hurt anything here. The series looks ok to me, but it depends on the header update, so it needs to wait for that to happen in the kernel. I provided my ack for the other series, but let me know if I need to push the vfio changes through my tree. Thanks, Alex > I had a very quick experiment on x86 > and Power Linux with following tiny program and the result is just > what you think: > > With "struct test foo" in func2(): > func2: foo.a=0xffffffff, foo.b=0xffffffff > with "static struct test foo" in func2(). Here's the explaining about > this: section 2.4.2.3 of http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members > func2: foo.a=0x00000000, foo.b=0x00000000 > with "struct test foo = { .a = 0 }" in func2(). > func2: foo.a=0x00000000, foo.b=0x00000000 > With "struct test foo = { 0 }" in func2(): > func2: foo.a=0x00000000, foo.b=0x00000000 > > --- > > #include <stdio.h> > > struct test { > int a; > int b; > }; > > static func1(void) > { > int var[1000]; > int i; > > for (i = 0; i < 1000; i++) > var[i] = 0xffffffff; > } > > static func2(void) > { > struct test foo; > > printf("%s: foo.a=0x%08x, foo.b=0x%08x\n", > __func__, foo.a, foo.b); > } > > int main(int argc, char **argv) > { > func1(); > func2(); > > return 0; > } > > Thanks, > Gavin > > >> + 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); > >> +} > >> + > >> static void spapr_phb_vfio_reset(DeviceState *qdev) > >> { > >> /* Do nothing */ > >> @@ -98,6 +153,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 = { > > >
On Wed, Jun 11, 2014 at 07:37:48PM -0600, Alex Williamson wrote: >On Thu, 2014-06-12 at 10:02 +1000, Gavin Shan wrote: >> On Wed, Jun 11, 2014 at 02:26:51PM -0600, Alex Williamson wrote: >> >On Tue, 2014-06-10 at 12:03 +1000, 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 <gwshan@linux.vnet.ibm.com> >> >> --- >> >> hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 56 insertions(+) >> >> >> >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c >> >> index 592d6a4..9750cf0 100644 >> >> --- a/hw/ppc/spapr_pci_vfio.c >> >> +++ b/hw/ppc/spapr_pci_vfio.c >> >> @@ -85,6 +85,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), .flags = 0 }; >> > >> >FWIW, flags = 0 isn't actually necessary. I'm sure someone here can >> >quote the C spec, but it's my understanding that if any field of a >> >structure is initialized, the remaining fields are zero initialized. >> >vfio.c has a mix of initializations depending on whether using an >> >explicit value for flags adds to the code clarity. >> > >> >> Yes, but it's not harmful. Please let me know if you want me to remove >> it :-) > >It's ok, explicit initialization doesn't hurt anything here. The series >looks ok to me, but it depends on the header update, so it needs to wait >for that to happen in the kernel. I provided my ack for the other >series, but let me know if I need to push the vfio changes through my >tree. Thanks, > Thanks, Alex. The kernel part should be merged firstly. All the stuff (kernel & QEMU part) depends on Alexey's VFIO stuff. So lets wait until Alexey's VFIO stuff gets merged. That time, I guess I probably have to rebase and send out a new revision (with your ack of course). Thanks, Gavin >> I had a very quick experiment on x86 >> and Power Linux with following tiny program and the result is just >> what you think: >> >> With "struct test foo" in func2(): >> func2: foo.a=0xffffffff, foo.b=0xffffffff >> with "static struct test foo" in func2(). Here's the explaining about >> this: section 2.4.2.3 of http://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Initializing-Structure-Members >> func2: foo.a=0x00000000, foo.b=0x00000000 >> with "struct test foo = { .a = 0 }" in func2(). >> func2: foo.a=0x00000000, foo.b=0x00000000 >> With "struct test foo = { 0 }" in func2(): >> func2: foo.a=0x00000000, foo.b=0x00000000 >> >> --- >> >> #include <stdio.h> >> >> struct test { >> int a; >> int b; >> }; >> >> static func1(void) >> { >> int var[1000]; >> int i; >> >> for (i = 0; i < 1000; i++) >> var[i] = 0xffffffff; >> } >> >> static func2(void) >> { >> struct test foo; >> >> printf("%s: foo.a=0x%08x, foo.b=0x%08x\n", >> __func__, foo.a, foo.b); >> } >> >> int main(int argc, char **argv) >> { >> func1(); >> func2(); >> >> return 0; >> } >> >> Thanks, >> Gavin >> >> >> + 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); >> >> +} >> >> + >> >> static void spapr_phb_vfio_reset(DeviceState *qdev) >> >> { >> >> /* Do nothing */ >> >> @@ -98,6 +153,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/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c index 592d6a4..9750cf0 100644 --- a/hw/ppc/spapr_pci_vfio.c +++ b/hw/ppc/spapr_pci_vfio.c @@ -85,6 +85,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), .flags = 0 }; + 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); +} + static void spapr_phb_vfio_reset(DeviceState *qdev) { /* Do nothing */ @@ -98,6 +153,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 = {
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 <gwshan@linux.vnet.ibm.com> --- hw/ppc/spapr_pci_vfio.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)