Re: [PATCH v1 2/9] s390x: toplogy: adding drawers and books to smp parsing

[Pierre Morel <pmorel@linux.ibm.com>]

On 7/19/21 5:50 PM, Cornelia Huck wrote:
> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
>> On Fri, Jul 16, 2021 at 12:44:49PM +0200, Cornelia Huck wrote:
>>> On Fri, Jul 16 2021, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>
>>>> On Fri, Jul 16, 2021 at 11:10:04AM +0200, Cornelia Huck wrote:
>>>>> On Thu, Jul 15 2021, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>>> Pierre Morel <pmorel@linux.ibm.com> writes:
>>>>>>
>>>>>>> On 7/15/21 8:16 AM, Markus Armbruster wrote:
>>>>>>>> Pierre Morel <pmorel@linux.ibm.com> writes:
>>>>>>>>
>>>>>>>>> Drawers and Books are levels 4 and 3 of the S390 CPU
>>>>>>>>> topology.
>>>>>>>>> We allow the user to define these levels and we will
>>>>>>>>> store the values inside the S390CcwMachineState.
>>>>>>>>
>>>>>>>> Double-checking: are these members specific to S390?
>>>>>>>
>>>>>>> Yes AFAIK
>>>>>>
>>>>>> Makes me wonder whether they should be conditional on TARGET_S390X.
>>>>>>
>>>>>> What happens when you specify them for another target?  Silently
>>>>>> ignored, or error?
>>>>>
>>>>> I'm wondering whether we should include them in the base machine state
>>>>> and treat them as we treat 'dies' (i.e. the standard parser errors out
>>>>> if they are set, and only the s390x parser supports them.)
>>>>
>>>> To repeat what i just wrote in my reply to patch 1, I think we ought to
>>>> think  about a different approach to handling the usage constraints,
>>>> which doesn't require full re-implementation of the smp_parse method
>>>> each time.  There should be a way for each target to report topology
>>>> constraints, such the the single smp_parse method can do the right
>>>> thing, especially wrt error reporting for unsupported values.
>>>
>>> That would mean that all possible fields would need to go into common
>>> code, right?
>>
>> Yes, that is an implication of what i'm suggesting.
>>
>>> I'm wondering whether there are more architecture/cpu specific values
>>> lurking in the corner, it would get unwieldy if we need to go beyond the
>>> existing fields and drawers/books.
>>
>> Is the book/drawer thing architecture specific, or is it machine
>> type / CPU specific. ie do /all/ the s390x machine types / CPUS
>> QEMU support the book/drawer concept, or only a subset.
> 
> Should not be by machine type, but might be by cpu model (e.g. older
> hardware lacking the needed support for exposing this to the guest.) IBM
> folks, please correct me if I'm wrong.


Looks correct to me this is an information indicated by a facility 
introduced with Z10 if I do not make an error.

Regards,
Pierre

> 
>> If only a subset, then restricting it per target on QAPI doesn't
>> fully solve the root problem, and we instead are better focusing
>> on accurate runtime error reporting.
> 
> Nod. Runtime error reporting should also be more flexible.
> 

-- 
Pierre Morel
IBM Lab Boeblingen