Re: [qemu-s390x] [PATCH 07/15] s390-bios: Decouple channel i/o logic from virtio

[Cornelia Huck]

On Tue, 29 Jan 2019 08:29:14 -0500
"Jason J. Herne" <address@hidden> wrote:

> Create a separate library for channel i/o related code. This decouples
> channel i/o operations from virtio and allows us to make use of them for
> the real dasd boot path.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> ---
>  pc-bios/s390-ccw/Makefile        |  2 +-
>  pc-bios/s390-ccw/cio.c           | 41 
> ++++++++++++++++++++++++++++++++++++++++
>  pc-bios/s390-ccw/cio.h           |  3 +++
>  pc-bios/s390-ccw/main.c          |  1 +
>  pc-bios/s390-ccw/netboot.mak     |  2 +-
>  pc-bios/s390-ccw/netmain.c       |  1 +
>  pc-bios/s390-ccw/s390-ccw.h      |  1 -
>  pc-bios/s390-ccw/virtio-blkdev.c |  1 +
>  pc-bios/s390-ccw/virtio.c        | 27 ++------------------------
>  9 files changed, 51 insertions(+), 28 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/cio.c
> 

> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
> new file mode 100644
> index 0000000..095f79b
> --- /dev/null
> +++ b/pc-bios/s390-ccw/cio.c
> @@ -0,0 +1,41 @@
> +/*
> + * S390 Channel I/O
> + *
> + * Copyright (c) 2018 Jason J. Herne <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */

Not sure that copyright header is correct. You moved some code that
probably should be copyright IBM (although that was never added to the
header in the first place...) Also not sure if Alex has some copyrights
on the code you moved, or if that is only trivial stuff. (Don't want to
be difficult, but we should try to get this right.)

> +
> +#include "libc.h"
> +#include "s390-ccw.h"
> +#include "cio.h"
> +
> +static char chsc_page[PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
> +
> +int enable_mss_facility(void)
> +{
> +    int ret;
> +    ChscAreaSda *sda_area = (ChscAreaSda *) chsc_page;
> +
> +    memset(sda_area, 0, PAGE_SIZE);
> +    sda_area->request.length = 0x0400;
> +    sda_area->request.code = 0x0031;
> +    sda_area->operation_code = 0x2;
> +
> +    ret = chsc(sda_area);
> +    if ((ret == 0) && (sda_area->response.code == 0x0001)) {
> +        return 0;
> +    }
> +    return -EIO;
> +}
> +
> +void enable_subchannel(SubChannelId schid)
> +{
> +    Schib schib;
> +
> +    stsch_err(schid, &schib);
> +    schib.pmcw.ena = 1;
> +    msch(schid, &schib);
> +}

(...)

> diff --git a/pc-bios/s390-ccw/virtio-blkdev.c 
> b/pc-bios/s390-ccw/virtio-blkdev.c
> index 11c5626..d2e7fcd 100644
> --- a/pc-bios/s390-ccw/virtio-blkdev.c
> +++ b/pc-bios/s390-ccw/virtio-blkdev.c
> @@ -10,6 +10,7 @@
>  
>  #include "libc.h"
>  #include "s390-ccw.h"
> +#include "cio.h"

Not sure why you need to add this here?

>  #include "virtio.h"
>  #include "virtio-scsi.h"
>  

Otherwise, looks good.