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

[Gavin Shan]

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 :-)

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 = {
>