Re: [Qemu-devel] [PATCH 07/10] target-s390x: enable fully implemented facilities

[Alexander Graf]

On 26.05.15 08:02, Aurelien Jarno wrote:
> On 2015-05-25 23:47, Alexander Graf wrote:
>>
>>
>> On 25.05.15 23:13, Aurelien Jarno wrote:
>>> On 2015-05-25 23:04, Alexander Graf wrote:
>>>>
>>>>
>>>> On 25.05.15 23:02, Aurelien Jarno wrote:
>>>>> On 2015-05-25 22:39, Alexander Graf wrote:
>>>>>>
>>>>>>
>>>>>> On 25.05.15 01:47, Aurelien Jarno wrote:
>>>>>>> Cc: Alexander Graf <address@hidden>
>>>>>>> Cc: Richard Henderson <address@hidden>
>>>>>>> Signed-off-by: Aurelien Jarno <address@hidden>
>>>>>>
>>>>>> Shouldn't this get populated based on the selected -cpu type?
>>>>>
>>>>> In the long term yes, but given we only implement one CPU type (or
>>>>> rather none) in TCG mode, we can consider that's already the case.
>>>>
>>>> There are patches coming from IBM to at least add a list of a good
>>>> number of s390x cpu types. I'd really like to make use of that and have
>>>> actual CPU types selectable.
>>>
>>> I guess they are for the KVM mode. Do they provide the corresponding 
>>> facilities list? Probably otherwise that doesn't really differentiate
>>> various CPUs. Please make sure of that when reviewing these patches.
>>
>> I could definitely use help on review - it's probably my weakest point ;).
>>
>>>> At least let's move towards that model. So the code in question should
>>>> take the facility capabilities from the first cpu object (or the class?)
>>>> for example and we bump it to the currently supported feature set in there.
>>>
>>> Yes, that would work for STFL/STFLE, though we should have a list of
>>> facilities implemented by TCG so we can mask out the non-implemented
>>> facilities. This basically corresponds to the informations provided by
>>> the current patch.
>>
>> Ah, so you consider the current list the "these are the features TCG
>> knows about" list?
>>
>>> That said that won't work for actually disabling the corresponding
>>> instructions as we don't have a 1 to 1 mapping between the facilities
>>> and the group of instructions. Anyway we don't even check that right
>>> now.
>>
>> I agree, but the TCG code annotates which facility each opcode belongs
>> to which means actually limiting it should become trivial. That's really
>> all I'm asking for - I want to see the light at the end of the tunnel ;).
> 
> It's trivial doing so for the facilities annotated in TCG. Just that
> they don't match one to one with the facilities bits in STFL/STFLE. Some
> bits enable multiple facilities and QEMU has also grouped some
> facilities together. Also some bits do not actually concern instructions
> but rather MMU features. Some other gives additional properties to a
> facility: some facilities might be present by disabled, some other might 
> have a slow or fast implementation.
> 
> We therefore need a conversion function before being able to do that,
> and we need to know which format IBM is going to provide in their
> patches: list of facilities or STFL/STFLE bits?

Please check out this patch set:

  https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03436.html


Alex