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

[Cornelia Huck <cohuck@redhat.com>]

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-bus.c b/hw/s390x/s390-pci-bus.c
> index 218717397ae1..4c7f06d5cf95 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -738,6 +738,46 @@ static void s390_pci_iommu_free(S390pciState *s, PCIBus *bus, int32_t devfn)
>      object_unref(OBJECT(iommu));
>  }
>  
> +static S390PCIGroup *s390_group_create(int id)
> +{
> +    S390PCIGroup *group;
> +    S390pciState *s = s390_get_phb();
> +
> +    group = g_new0(S390PCIGroup, 1);
> +    group->id = id;
> +    QTAILQ_INSERT_TAIL(&s->zpci_groups, group, link);
> +    return group;
> +}
> +
> +S390PCIGroup *s390_group_find(int id)
> +{
> +    S390PCIGroup *group;
> +    S390pciState *s = s390_get_phb();
> +
> +    QTAILQ_FOREACH(group, &s->zpci_groups, link) {
> +        if (group->id == id) {
> +            return group;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static void s390_pci_init_default_group(void)
> +{
> +    S390PCIGroup *group;
> +    ClpRspQueryPciGrp *resgrp;
> +
> +    group = s390_group_create(ZPCI_DEFAULT_FN_GRP);
> +    resgrp = &group->zpci_group;
> +    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;
> +}
> +
>  static void s390_pcihost_realize(DeviceState *dev, Error **errp)
>  {
>      PCIBus *b;
> @@ -766,7 +806,9 @@ static void s390_pcihost_realize(DeviceState *dev, Error **errp)
>      QTAILQ_INIT(&s->pending_sei);
>      QTAILQ_INIT(&s->zpci_devs);
>      QTAILQ_INIT(&s->zpci_dma_limit);
> +    QTAILQ_INIT(&s->zpci_groups);
>  
> +    s390_pci_init_default_group();
>      css_register_io_adapters(CSS_IO_ADAPTER_PCI, true, false,
>                               S390_ADAPTER_SUPPRESSIBLE, errp);
>  }
> 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);
> +        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;
>      }
> @@ -787,7 +791,8 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      }
>      /* Length must be greater than 8, a multiple of 8 */
>      /* and not greater than maxstbl */
> -    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
> +    if ((len <= 8) || (len % 8) ||
> +        (len > pbdev->pci_group->zpci_group.maxstbl)) {
>          goto specification_error;
>      }
>      /* Do not cross a 4K-byte boundary */
> diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
> index 5f339e57fb68..869c0f254b7f 100644
> --- a/include/hw/s390x/s390-pci-bus.h
> +++ b/include/hw/s390x/s390-pci-bus.h
> @@ -316,6 +316,14 @@ typedef struct ZpciFmb {
>  } ZpciFmb;
>  QEMU_BUILD_BUG_MSG(offsetof(ZpciFmb, fmt0) != 48, "padding in ZpciFmb");
>  
> +#define ZPCI_DEFAULT_FN_GRP 0x20
> +typedef struct S390PCIGroup {
> +    ClpRspQueryPciGrp zpci_group;
> +    int id;
> +    QTAILQ_ENTRY(S390PCIGroup) link;
> +} S390PCIGroup;
> +S390PCIGroup *s390_group_find(int id);
> +
>  struct S390PCIBusDevice {
>      DeviceState qdev;
>      PCIDevice *pdev;
> @@ -333,6 +341,7 @@ struct S390PCIBusDevice {
>      uint16_t noi;
>      uint16_t maxstbl;
>      uint8_t sum;
> +    S390PCIGroup *pci_group;
>      S390MsixInfo msix;
>      AdapterRoutes routes;
>      S390PCIIOMMU *iommu;
> @@ -358,6 +367,7 @@ struct S390pciState {
>      QTAILQ_HEAD(, SeiContainer) pending_sei;
>      QTAILQ_HEAD(, S390PCIBusDevice) zpci_devs;
>      QTAILQ_HEAD(, S390PCIDMACount) zpci_dma_limit;
> +    QTAILQ_HEAD(, S390PCIGroup) zpci_groups;
>  };
>  
>  S390pciState *s390_get_phb(void);
>