Re: [Qemu-devel] [PATCH v10 3/3] sPAPR: Implement sPAPRPHBClass::eeh_handler

[Alex Williamson]

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 <address@hidden>
> >> ---
> >>  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 = {
> >
>