Re: [PATCH-for-5.2] hw/s390x: Move S390_ADAPTER_SUPPRESSIBLE from 'css.h' to 's390_flic.h'

[Philippe Mathieu-Daudé]

On 7/16/20 1:11 PM, Cornelia Huck wrote:
> On Wed, 15 Jul 2020 15:18:45 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> Not all s390x devices require to have access to the CPU internals.
>>
>> To reduce the include dependencies on "s390x/css.h", move the
>> S390_ADAPTER_SUPPRESSIBLE definition to "s390x/s390_flic.h".
> 
> But css.h is not 'CPU internals', it is the main I/O subsystem?

I meant devices using the main I/O subsystem don't need to have
access to the CPU internals (CPUS390XState and s390_cpu* helpers).

> 
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  include/hw/s390x/css.h       | 7 -------
>>  include/hw/s390x/s390_flic.h | 7 +++++++
>>  2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index 08c869ab0a..7858666307 100644
>> --- a/include/hw/s390x/css.h
>> +++ b/include/hw/s390x/css.h
>> @@ -12,7 +12,6 @@
>>  #ifndef CSS_H
>>  #define CSS_H
>>  
>> -#include "cpu.h"
> 
> FWIW, we should just be able to remove this #include...

Odd it was not working yesterday (missing CONFIG_KVM) but today
it works.

> 
>>  #include "hw/s390x/adapter.h"
>>  #include "hw/s390x/s390_flic.h"
>>  #include "hw/s390x/ioinst.h"
>> @@ -233,12 +232,6 @@ uint32_t css_get_adapter_id(CssIoAdapterType type, 
>> uint8_t isc);
>>  void css_register_io_adapters(CssIoAdapterType type, bool swap, bool 
>> maskable,
>>                                uint8_t flags, Error **errp);
>>  
>> -#ifndef CONFIG_KVM
>> -#define S390_ADAPTER_SUPPRESSIBLE 0x01
>> -#else
>> -#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
>> -#endif
>> -
>>  #ifndef CONFIG_USER_ONLY
>>  SubchDev *css_find_subch(uint8_t m, uint8_t cssid, uint8_t ssid,
>>                           uint16_t schid);
>> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
>> index 4687ecfe83..6bf9d73728 100644
>> --- a/include/hw/s390x/s390_flic.h
>> +++ b/include/hw/s390x/s390_flic.h
>> @@ -17,6 +17,13 @@
>>  #include "hw/s390x/adapter.h"
>>  #include "hw/virtio/virtio.h"
>>  #include "qemu/queue.h"
>> +#include "cpu.h"
> 
> ...and we do not need it here AFAICS.
> 
>> +
>> +#ifndef CONFIG_KVM
>> +#define S390_ADAPTER_SUPPRESSIBLE 0x01
>> +#else
>> +#define S390_ADAPTER_SUPPRESSIBLE KVM_S390_ADAPTER_SUPPRESSIBLE
>> +#endif
>>  
>>  /*
>>   * Reserve enough gsis to accommodate all virtio devices.
> 
> Whether the definition of S390_ADAPTER_SUPPRESSIBLE belongs into css.h
> or s390_flic.h is probably a matter of taste: the definition of an I/O
> adapter is in css.h, and registration of an adapter is done via the
> FLIC.
> 
> I think removing the cpu.h include might already do what you wanted to
> do?

Yes :)

Thanks!