Re: [Qemu-devel] [RFC PATCH v2 3/8] pc-bios/s390-ccw: Move virtio-block related functions into a separate file

[Thomas Huth <thuth@redhat.com>]

On 07.07.2017 14:54, Cornelia Huck wrote:
> On Fri,  7 Jul 2017 12:26:59 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> The netboot code is going to link against the code from virtio.c, too,
>> so we've got to move the virtio-block related code out of the way.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  pc-bios/s390-ccw/Makefile     |   2 +-
>>  pc-bios/s390-ccw/main.c       |   2 +-
>>  pc-bios/s390-ccw/s390-ccw.h   |   2 +-
>>  pc-bios/s390-ccw/virtio-blk.c | 296 ++++++++++++++++++++++++++++++++++++++++++
>>  pc-bios/s390-ccw/virtio.c     | 273 +-------------------------------------
>>  pc-bios/s390-ccw/virtio.h     |   4 +
>>  6 files changed, 307 insertions(+), 272 deletions(-)
>>  create mode 100644 pc-bios/s390-ccw/virtio-blk.c
>>
> 
>> diff --git a/pc-bios/s390-ccw/virtio-blk.c b/pc-bios/s390-ccw/virtio-blk.c
>> new file mode 100644
>> index 0000000..6cb77bc
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/virtio-blk.c
>> @@ -0,0 +1,296 @@
>> +/*
>> + * Virtio driver bits
>> + *
>> + * Copyright (c) 2013 Alexander Graf <agraf@suse.de>
> 
> The original code carries the same copyright notice, but there's a lot
> of IBM code in there. Just saying.

Then the IBM folks should maybe add their (c) statement to that file
first? Otherwise, I assume they simply do not care ;-)

[...]
>> +int virtio_read_many(ulong sector, void *load_addr, int sec_num)
>> +{
>> +    VDev *vdev = virtio_get_device();
>> +
>> +    switch (vdev->senseid.cu_model) {
>> +    case VIRTIO_ID_BLOCK:
>> +        return virtio_blk_read_many(vdev, sector, load_addr, sec_num);
>> +    case VIRTIO_ID_SCSI:
>> +        return virtio_scsi_read_many(vdev, sector, load_addr, sec_num);
> 
> This is scsi, not blk. Should virtio_read_many() stay in virtio.c?

No, that does not work since virtio_scsi_read_many() is defined in
virtio-scsi.c which is not linked to the netboot code.

>> +    }
>> +    panic("\n! No readable IPL device !\n");
>> +    return -1;
>> +}
> (...)
>> +void virtio_assume_scsi(void)
>> +{
>> +    VDev *vdev = virtio_get_device();
>> +
>> +    switch (vdev->senseid.cu_model) {
>> +    case VIRTIO_ID_BLOCK:
>> +        vdev->guessed_disk_nature = VIRTIO_GDN_SCSI;
>> +        vdev->config.blk.blk_size = VIRTIO_SCSI_BLOCK_SIZE;
>> +        vdev->config.blk.physical_block_exp = 0;
>> +        vdev->blk_factor = 1;
>> +        break;
>> +    case VIRTIO_ID_SCSI:
>> +        vdev->scsi_block_size = VIRTIO_SCSI_BLOCK_SIZE;
> 
> More scsi. Maybe the file just needs a different name :)

Do you have a better suggestion?

 Thomas