Re: [PATCH RFCv3 3/9] s390x: remove hypercall registration mechanism

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 27.07.20 11:24, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 16:37:44 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Nowadays, we only have a single machine type in QEMU, everything is based
>> on virtio-ccw and the traditional virtio machine does no longer exist. No
>> need to dynamically register diag500 handlers. Move the two existing
> 
> Hm, do we want to make certain subcodes available only if certain code
> has been configured? If yes, it might make sense to keep the mechanism.
> 

I think the registration really makes the code hard to read. So Removal
of that is
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


>> handlers into diag500.c.
> 
> In any case, that file does not exist :)
> 
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c   | 46 -----------------------------
>>  hw/s390x/s390-virtio-hcall.c | 56 ++++++++++++++++++++++++------------
>>  hw/s390x/s390-virtio-hcall.h |  2 --
>>  3 files changed, 38 insertions(+), 66 deletions(-)
>>
> 
> (...)
> 
>> diff --git a/hw/s390x/s390-virtio-hcall.c b/hw/s390x/s390-virtio-hcall.c
>> index ec7cf8beb3..5e14bd49b7 100644
>> --- a/hw/s390x/s390-virtio-hcall.c
>> +++ b/hw/s390x/s390-virtio-hcall.c
>> @@ -12,30 +12,50 @@
>>  #include "qemu/osdep.h"
>>  #include "cpu.h"
>>  #include "hw/s390x/s390-virtio-hcall.h"
>> +#include "hw/s390x/ioinst.h"
> 
> (Maybe you could remove the ioinst.h include from s390-virtio-ccw.c
> with this change?)
> 
>> +#include "hw/s390x/css.h"
>> +#include "virtio-ccw.h"
>>  
>> -#define MAX_DIAG_SUBCODES 255
>> -
>> -static s390_virtio_fn s390_diag500_table[MAX_DIAG_SUBCODES];
>> -
>> -void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn)
>> +static int handle_virtio_notify(uint64_t mem)
>>  {
>> -    assert(code < MAX_DIAG_SUBCODES);
>> -    assert(!s390_diag500_table[code]);
>> -
>> -    s390_diag500_table[code] = fn;
>> +    if (mem < ram_size) {
>> +        /* Tolerate early printk. */
> 
> I'm wondering if we still need this. Probably doesn't hurt too much to
> keep it around, though.
> 
>> +        return 0;
>> +    }
>> +    return -EINVAL;
>>  }
>>  
> 
> (...)
> 
>