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

[Cornelia Huck]

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.

> 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.