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

[Gavin Shan]

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 <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,
>

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