Re: [kvm-unit-tests PATCH 1/3] lib: s390x: hw: Provide early detect host

[Claudio Imbrenda <imbrenda@xxxxxxxxxxxxx>]

On Tue, 10 Oct 2023 13:03:08 +0200
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:

> On 10/10/23 12:40, Claudio Imbrenda wrote:
> > On Tue, 10 Oct 2023 07:38:53 +0000
> > Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
> >   
> >> For early sclp printing it's necessary to know if we're under LPAR or
> >> not so we can apply compat SCLP ASCII transformations.
> >>
> >> Signed-off-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
> >> ---
> >>   lib/s390x/hardware.c | 8 ++++++++
> >>   lib/s390x/hardware.h | 1 +
> >>   2 files changed, 9 insertions(+)
> >>
> >> diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
> >> index 2bcf9c4c..d5a752c0 100644
> >> --- a/lib/s390x/hardware.c
> >> +++ b/lib/s390x/hardware.c
> >> @@ -52,6 +52,14 @@ static enum s390_host do_detect_host(void *buf)
> >>   	return HOST_IS_UNKNOWN;
> >>   }
> >>   
> >> +enum s390_host detect_host_early(void)
> >> +{
> >> +	if (stsi_get_fc() == 2)
> >> +		return HOST_IS_LPAR;
> >> +
> >> +	return HOST_IS_UNKNOWN;
> >> +}
> >> +
> >>   enum s390_host detect_host(void)
> >>   {
> >>   	static enum s390_host host = HOST_IS_UNKNOWN;
> >> diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
> >> index 86fe873c..5e5a9d90 100644
> >> --- a/lib/s390x/hardware.h
> >> +++ b/lib/s390x/hardware.h
> >> @@ -24,6 +24,7 @@ enum s390_host {
> >>   };
> >>   
> >>   enum s390_host detect_host(void);
> >> +enum s390_host detect_host_early(void);  
> > 
> > I wonder if it weren't easier to fix detect_host so it can be used
> > early....
> >   
> 
> Problem is, where do you start and where do you end?
> 
> We could statically allocate a page but why did we add the current 
> version then? (The old version did that if I remember correctly).

the old version also allocated pages, and was a hodgepodge of various
functions replicating the same behaviour, many calling the others. the
main goal of the current version was to make it more readable and
maintainable. 

a possible fix could also involve allocating the buffer on the stack of
do_detect_host(), so it's not taking up memory permanently.

that said, I don't have a strong opinion about this, but maybe it's a
good idea to not replicate the same behaviour again :)

if you don't want to fix detect_host(), then maybe something like this
instead?

diff --git a/lib/s390x/hardware.h b/lib/s390x/hardware.h
index 86fe873c..5e5a9d90 100644
--- a/lib/s390x/hardware.h
+++ b/lib/s390x/hardware.h
@@ -24,6 +24,7 @@ enum s390_host {
 };
 
 enum s390_host detect_host(void);
+enum s390_host detect_host_early(void);
 
 static inline uint16_t get_machine_id(void)
 {
diff --git a/lib/s390x/hardware.c b/lib/s390x/hardware.c
index 2bcf9c4c..b281cf10 100644
--- a/lib/s390x/hardware.c
+++ b/lib/s390x/hardware.c
@@ -28,7 +28,7 @@ static enum s390_host do_detect_host(void *buf)
        if (stsi_get_fc() == 2)
                return HOST_IS_LPAR;
 
-       if (stsi_get_fc() != 3)
+       if (!buf || stsi_get_fc() != 3)
                return HOST_IS_UNKNOWN;
 
        if (!stsi(buf, 1, 1, 1)) {
@@ -67,3 +67,8 @@ enum s390_host detect_host(void)
        initialized = true;
        return host;
 }
+
+enum s390_host detect_host_early(void)
+{
+       return do_detect_host(NULL);
+}