Re: [Qemu-devel] [PATCH v5 7/9] s390x/sclp: properly guard pci-specific functions

[Halil Pasic]

On 08/23/2017 05:54 PM, Cornelia Huck wrote:
> If we do not provide zpci, pci reconfiguration via sclp is not available
> either. I/O adapter configuration, however, should always be present.
> 
> Rename the values that refer to I/O adapter configuration (instead of only
> pci) to make things clearer.
> 
> Move length checking of the sccb for I/O adapter configuration into the
> common sclp code (out of the pci code). This also fixes an issue that
> the pci code would refer to a field in the sccb before checking whether
> it was actually long enough.
> 
> Check for the adapter type in the sccb and return unrecognized adapter
> type if the guest tries to issue I/O adapter configure/deconfigure for
> a type other than pci or for pci if the zpci facility is not provided.
> 
> Signed-off-by: Cornelia Huck <address@hidden>
> ---
>  hw/s390x/s390-pci-bus.c  | 14 ++------------
>  hw/s390x/s390-pci-bus.h  |  8 --------
>  hw/s390x/s390-pci-stub.c |  2 ++
>  hw/s390x/sclp.c          | 39 ++++++++++++++++++++++++++++++++++-----
>  include/hw/s390x/sclp.h  | 17 +++++++++++++----
>  5 files changed, 51 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index c57f6ebae0..0a31a4ae88 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -122,16 +122,11 @@ S390PCIBusDevice *s390_pci_find_dev_by_fid(S390pciState 
> *s, uint32_t fid)
> 
>  void s390_pci_sclp_configure(SCCB *sccb)
>  {
> -    PciCfgSccb *psccb = (PciCfgSccb *)sccb;
> +    IoaCfgSccb *psccb = (IoaCfgSccb *)sccb;
>      S390PCIBusDevice *pbdev = s390_pci_find_dev_by_fid(s390_get_phb(),
>                                                         
> be32_to_cpu(psccb->aid));
>      uint16_t rc;
> 
> -    if (be16_to_cpu(sccb->h.length) < 16) {
> -        rc = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
> -        goto out;
> -    }
> -
>      if (!pbdev) {
>          DPRINTF("sclp config no dev found\n");
>          rc = SCLP_RC_ADAPTER_ID_NOT_RECOGNIZED;
> @@ -155,16 +150,11 @@ out:
> 
>  void s390_pci_sclp_deconfigure(SCCB *sccb)
>  {
> -    PciCfgSccb *psccb = (PciCfgSccb *)sccb;
> +    IoaCfgSccb *psccb = (IoaCfgSccb *)sccb;
>      S390PCIBusDevice *pbdev = s390_pci_find_dev_by_fid(s390_get_phb(),
>                                                         
> be32_to_cpu(psccb->aid));
>      uint16_t rc;
> 
> -    if (be16_to_cpu(sccb->h.length) < 16) {
> -        rc = SCLP_RC_INSUFFICIENT_SCCB_LENGTH;
> -        goto out;
> -    }
> -
>      if (!pbdev) {
>          DPRINTF("sclp deconfig no dev found\n");
>          rc = SCLP_RC_ADAPTER_ID_NOT_RECOGNIZED;
> diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> index 5df6292509..bd636abc28 100644
> --- a/hw/s390x/s390-pci-bus.h
> +++ b/hw/s390x/s390-pci-bus.h
> @@ -244,14 +244,6 @@ typedef struct ChscSeiNt2Res {
>      uint8_t ccdf[4016];
>  } QEMU_PACKED ChscSeiNt2Res;
> 
> -typedef struct PciCfgSccb {
> -    SCCBHeader header;
> -    uint8_t atype;
> -    uint8_t reserved1;
> -    uint16_t reserved2;
> -    uint32_t aid;
> -} QEMU_PACKED PciCfgSccb;
> -
>  typedef struct S390MsixInfo {
>      bool available;
>      uint8_t table_bar;
> diff --git a/hw/s390x/s390-pci-stub.c b/hw/s390x/s390-pci-stub.c
> index cc7278a865..7a642d376c 100644
> --- a/hw/s390x/s390-pci-stub.c
> +++ b/hw/s390x/s390-pci-stub.c
> @@ -20,10 +20,12 @@ int pci_chsc_sei_nt2_have_event(void)
>  /* hw/s390x/sclp.c */
>  void s390_pci_sclp_configure(SCCB *sccb)
>  {
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED);
>  }
> 
>  void s390_pci_sclp_deconfigure(SCCB *sccb)
>  {
> +    sccb->h.response_code = cpu_to_be16(SCLP_RC_ADAPTER_TYPE_NOT_RECOGNIZED);
>  }
> 
>  /* target/s390x/kvm.c */
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 9253dbbc64..7ae6a0e37a 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -80,7 +80,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>      prepare_cpu_entries(sclp, read_info->entries, cpu_count);
> 
>      read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> -                                        SCLP_HAS_PCI_RECONFIG);
> +                                        SCLP_HAS_IOA_RECONFIG);
> 
>      /* Memory Hotplug is only supported for the ccw machine type */
>      if (mhd) {
> @@ -354,6 +354,35 @@ static void sclp_read_cpu_info(SCLPDevice *sclp, SCCB 
> *sccb)
>      sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION);
>  }
> 
> +static void sclp_configure_io_adapter(SCLPDevice *sclp, SCCB *sccb,
> +                                      bool configure)

Just an idea. This could be called reconfigure instead
of configure (that's sclp_reconfigure_io_adapter) as the
facility providing both conf and deconf is called reconf.

It ain't important. Patch looks good!

Reviewed-by: Halil Pasic <address@hidden>