Re: [Qemu-devel] [PATCH qemu v6 10/15] spapr_pci_vfio: Remove unnecessary cast to sPAPRPHBVFIOState

[Alexey Kardashevskiy]

On 04/16/2015 08:31 PM, Thomas Huth wrote:
> Am Sat, 11 Apr 2015 01:24:39 +1000
> schrieb Alexey Kardashevskiy <address@hidden>:
>
> > EEH handlers only need AddressSpace to get to the right VFIO container
> > to call ioctl() so remove unnecessary cast of sPAPRPHBState to
> > sPAPRPHBVFIOState.
> >
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > ---
> >   hw/ppc/spapr_pci_vfio.c | 12 ++++--------
> >   1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> > index a428166..a5b97d0 100644
> > --- a/hw/ppc/spapr_pci_vfio.c
> > +++ b/hw/ppc/spapr_pci_vfio.c
> > @@ -66,7 +66,6 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
> >   static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
> >                                            unsigned int addr, int option)
> >   {
> > -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> >       struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> >       int ret;
> >
> > @@ -103,7 +102,7 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState 
> > *sphb,
> >           return RTAS_OUT_PARAM_ERROR;
> >       }
> >
> > -    ret = vfio_container_ioctl(&svphb->phb.iommu_as,
> > +    ret = vfio_container_ioctl(&sphb->iommu_as,
> >                                  VFIO_EEH_PE_OP, &op);
> >       if (ret < 0) {
> >           return RTAS_OUT_HW_ERROR;
> > @@ -114,12 +113,11 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState 
> > *sphb,
> >
> >   static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
> >   {
> > -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> >       struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> >       int ret;
> >
> >       op.op = VFIO_EEH_PE_GET_STATE;
> > -    ret = vfio_container_ioctl(&svphb->phb.iommu_as,
> > +    ret = vfio_container_ioctl(&sphb->iommu_as,
> >                                  VFIO_EEH_PE_OP, &op);
> >       if (ret < 0) {
> >           return RTAS_OUT_PARAM_ERROR;
> > @@ -131,7 +129,6 @@ static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState 
> > *sphb, int *state)
> >
> >   static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
> >   {
> > -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> >       struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> >       int ret;
> >
> > @@ -149,7 +146,7 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, 
> > int option)
> >           return RTAS_OUT_PARAM_ERROR;
> >       }
> >
> > -    ret = vfio_container_ioctl(&svphb->phb.iommu_as,
> > +    ret = vfio_container_ioctl(&sphb->iommu_as,
> >                                  VFIO_EEH_PE_OP, &op);
> >       if (ret < 0) {
> >           return RTAS_OUT_HW_ERROR;
> > @@ -160,12 +157,11 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, 
> > int option)
> >
> >   static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
> >   {
> > -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> >       struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> >       int ret;
> >
> >       op.op = VFIO_EEH_PE_CONFIGURE;
> > -    ret = vfio_container_ioctl(&svphb->phb.iommu_as,
> > +    ret = vfio_container_ioctl(&sphb->iommu_as,
> >                                  VFIO_EEH_PE_OP, &op);
> >       if (ret < 0) {
> >           return RTAS_OUT_PARAM_ERROR;
>
> Patch looks fine to me, so:
>
> Reviewed-by: Thomas Huth <address@hidden>
>
> But maybe you could even squash this into the patch where you removed the
> iommugroupid parameters (patch #5) ... so that you don't have to touch all
> these lines twice?

May be... Patch #5 enables multiple groups per container, and this one 
prepares for spapr-vfio-phb merge so they solve different issues in general 
and I was told multiple times the keeping patches simpler is a good thing 
(to reasonable extent) :)

Anyway in the next respin I'll remove more of EEH stuff, coming soon!


--
Alexey