Re: [PULL v3 26/32] s390x/pci: use a PCI Group structure

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 17 Nov 2020 12:55:22 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 11/17/20 12:43 PM, Cornelia Huck wrote:
> > On Sun, 01 Nov 2020 14:02:46 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >   
> >> From: Pierre Morel <pmorel@linux.ibm.com>
> >>
> >> We use a S390PCIGroup structure to hold the information related to a
> >> zPCI Function group.
> >>
> >> This allows us to be ready to support multiple groups and to retrieve
> >> the group information from the host.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> >> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >> ---
> >>  hw/s390x/s390-pci-bus.c         |   42 +++++++++++++++++++++++++++++++++++++++
> >>  hw/s390x/s390-pci-inst.c        |   23 +++++++++++++--------
> >>  include/hw/s390x/s390-pci-bus.h |   10 +++++++++
> >>  3 files changed, 66 insertions(+), 9 deletions(-)  
> > 
> > I just bisected a regression down to this commit.
> > 
> > s390x tcg guest on x86, virtio-pci devices are not detected. The
> > relevant feature bits are visible to the guest. Same breakage with
> > different guest kernels.
> > 
> > KVM guests and s390x tcg guests on s390x are fine, so I assume an
> > endianness issue somewhere. Nothing jumps out to me, though.

(...)

> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >> index 4eadd9e79416..c25b2a67efe0 100644
> >> --- a/hw/s390x/s390-pci-inst.c
> >> +++ b/hw/s390x/s390-pci-inst.c
> >> @@ -298,21 +298,25 @@ int clp_service_call(S390CPU *cpu, uint8_t r2, uintptr_t ra)
> >>          stq_p(&resquery->edma, ZPCI_EDMA_ADDR);
> >>          stl_p(&resquery->fid, pbdev->fid);
> >>          stw_p(&resquery->pchid, 0);
> >> -        stw_p(&resquery->ug, 1);
> >> +        stw_p(&resquery->ug, ZPCI_DEFAULT_FN_GRP);
> >>          stl_p(&resquery->uid, pbdev->uid);
> >>          stw_p(&resquery->hdr.rsp, CLP_RC_OK);
> >>          break;
> >>      }
> >>      case CLP_QUERY_PCI_FNGRP: {
> >>          ClpRspQueryPciGrp *resgrp = (ClpRspQueryPciGrp *)resh;
> >> -        resgrp->fr = 1;
> >> -        stq_p(&resgrp->dasm, 0);
> >> -        stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
> >> -        stw_p(&resgrp->mui, DEFAULT_MUI);
> >> -        stw_p(&resgrp->i, 128);
> >> -        stw_p(&resgrp->maxstbl, 128);
> >> -        resgrp->version = 0;
> >>  
> >> +        ClpReqQueryPciGrp *reqgrp = (ClpReqQueryPciGrp *)reqh;
> >> +        S390PCIGroup *group;
> >> +
> >> +        group = s390_group_find(reqgrp->g);  
> 
> -        group = s390_group_find(reqgrp->g);
> +        group = s390_group_find(ldl_p(&reqgrp->g));

Yep, that's it :)

Do you want to send a proper patch, or should I do it?

> 
> >> +        if (!group) {
> >> +            /* We do not allow access to unknown groups */
> >> +            /* The group must have been obtained with a vfio device */
> >> +            stw_p(&resgrp->hdr.rsp, CLP_RC_QUERYPCIFG_PFGID);
> >> +            goto out;
> >> +        }
> >> +        memcpy(resgrp, &group->zpci_group, sizeof(ClpRspQueryPciGrp));
> >>          stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
> >>          break;
> >>      }