mbox series

[kvm-unit-tests,v2,0/1] s390x: do not enable PV dump support by default

Message ID 20221019145320.1228710-1-nrb@linux.ibm.com (mailing list archive)
Headers show
Series s390x: do not enable PV dump support by default | expand

Message

Nico Boehr Oct. 19, 2022, 2:53 p.m. UTC
v1->v2:
---
* add indent to CONFIG_DUMP if in Makefile (thanks Janosch)
* add comment (thanks Janosch)

Currently, dump support is always enabled by setting the respective
plaintext control flag (PCF). Unfortunately, older machines without
support for PV dump will not start the guest when this PCF is set.

Nico Boehr (1):
  s390x: do not enable PV dump support by default

 configure      | 11 +++++++++++
 s390x/Makefile | 26 +++++++++++++++++---------
 2 files changed, 28 insertions(+), 9 deletions(-)

Comments

Claudio Imbrenda Oct. 19, 2022, 3:19 p.m. UTC | #1
On Wed, 19 Oct 2022 16:53:19 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> v1->v2:
> ---
> * add indent to CONFIG_DUMP if in Makefile (thanks Janosch)
> * add comment (thanks Janosch)
> 
> Currently, dump support is always enabled by setting the respective
> plaintext control flag (PCF). Unfortunately, older machines without
> support for PV dump will not start the guest when this PCF is set.

maybe for the long term we could try to fix the stub generated by
genprotimg to check the plaintext flags and the available features and
refuse to try to start if the required features are missing.

ideally providing a custom message when generating the image, to be
shown if the required features are missing. e.g. for kvm unit test, the
custom message could be something like
SKIP: $TEST_NAME: Missing hardware features

once that is in place, we could revert this patch

> 
> Nico Boehr (1):
>   s390x: do not enable PV dump support by default
> 
>  configure      | 11 +++++++++++
>  s390x/Makefile | 26 +++++++++++++++++---------
>  2 files changed, 28 insertions(+), 9 deletions(-)
>
Nico Boehr Oct. 19, 2022, 3:24 p.m. UTC | #2
Quoting Claudio Imbrenda (2022-10-19 17:19:43)
> On Wed, 19 Oct 2022 16:53:19 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
> > v1->v2:
> > ---
> > * add indent to CONFIG_DUMP if in Makefile (thanks Janosch)
> > * add comment (thanks Janosch)
> > 
> > Currently, dump support is always enabled by setting the respective
> > plaintext control flag (PCF). Unfortunately, older machines without
> > support for PV dump will not start the guest when this PCF is set.
> 
> maybe for the long term we could try to fix the stub generated by
> genprotimg to check the plaintext flags and the available features and
> refuse to try to start if the required features are missing.
> 
> ideally providing a custom message when generating the image, to be
> shown if the required features are missing. e.g. for kvm unit test, the
> custom message could be something like
> SKIP: $TEST_NAME: Missing hardware features
> 
> once that is in place, we could revert this patch

But that would mean that on machines which don't support dumping, PV tests will never run, will they?

So we need some way of specifing at compile time whether you want dump support or not.
Janosch Frank Oct. 19, 2022, 3:47 p.m. UTC | #3
On 10/19/22 17:19, Claudio Imbrenda wrote:
> On Wed, 19 Oct 2022 16:53:19 +0200
> Nico Boehr <nrb@linux.ibm.com> wrote:
> 
>> v1->v2:
>> ---
>> * add indent to CONFIG_DUMP if in Makefile (thanks Janosch)
>> * add comment (thanks Janosch)
>>
>> Currently, dump support is always enabled by setting the respective
>> plaintext control flag (PCF). Unfortunately, older machines without
>> support for PV dump will not start the guest when this PCF is set.
> 
> maybe for the long term we could try to fix the stub generated by
> genprotimg to check the plaintext flags and the available features and
> refuse to try to start if the required features are missing.

That's not possible on multiple levels:
* Unsecure G2 does not have stfle 158
* Dump is a host feature so I'm unsure if it would even be indicated in 
the guest

> 
> ideally providing a custom message when generating the image, to be
> shown if the required features are missing. e.g. for kvm unit test, the
> custom message could be something like
> SKIP: $TEST_NAME: Missing hardware features
> 
> once that is in place, we could revert this patch

Also the host that's using genprotimg might not be PV enabled or even 
s390x so checking on image generation is no option either.

> 
>>
>> Nico Boehr (1):
>>    s390x: do not enable PV dump support by default
>>
>>   configure      | 11 +++++++++++
>>   s390x/Makefile | 26 +++++++++++++++++---------
>>   2 files changed, 28 insertions(+), 9 deletions(-)
>>
>
Claudio Imbrenda Oct. 19, 2022, 4:03 p.m. UTC | #4
On Wed, 19 Oct 2022 17:24:16 +0200
Nico Boehr <nrb@linux.ibm.com> wrote:

> Quoting Claudio Imbrenda (2022-10-19 17:19:43)
> > On Wed, 19 Oct 2022 16:53:19 +0200
> > Nico Boehr <nrb@linux.ibm.com> wrote:
> >   
> > > v1->v2:
> > > ---
> > > * add indent to CONFIG_DUMP if in Makefile (thanks Janosch)
> > > * add comment (thanks Janosch)
> > > 
> > > Currently, dump support is always enabled by setting the respective
> > > plaintext control flag (PCF). Unfortunately, older machines without
> > > support for PV dump will not start the guest when this PCF is set.  
> > 
> > maybe for the long term we could try to fix the stub generated by
> > genprotimg to check the plaintext flags and the available features and
> > refuse to try to start if the required features are missing.
> > 
> > ideally providing a custom message when generating the image, to be
> > shown if the required features are missing. e.g. for kvm unit test, the
> > custom message could be something like
> > SKIP: $TEST_NAME: Missing hardware features
> > 
> > once that is in place, we could revert this patch  
> 
> But that would mean that on machines which don't support dumping, PV tests will never run, will they?

no, the check would be done at run time, so the test would only be
skipped on machines that don't support dumping (or whatever other
feature)

but again, this is a long term idea, for now we'll take your patch
since it solves the problem :)

> 
> So we need some way of specifing at compile time whether you want dump support or not.
Claudio Imbrenda Oct. 19, 2022, 4:17 p.m. UTC | #5
On Wed, 19 Oct 2022 17:47:36 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 10/19/22 17:19, Claudio Imbrenda wrote:
> > On Wed, 19 Oct 2022 16:53:19 +0200
> > Nico Boehr <nrb@linux.ibm.com> wrote:
> >   
> >> v1->v2:
> >> ---
> >> * add indent to CONFIG_DUMP if in Makefile (thanks Janosch)
> >> * add comment (thanks Janosch)
> >>
> >> Currently, dump support is always enabled by setting the respective
> >> plaintext control flag (PCF). Unfortunately, older machines without
> >> support for PV dump will not start the guest when this PCF is set.  
> > 
> > maybe for the long term we could try to fix the stub generated by
> > genprotimg to check the plaintext flags and the available features and
> > refuse to try to start if the required features are missing.  
> 
> That's not possible on multiple levels:
> * Unsecure G2 does not have stfle 158
> * Dump is a host feature so I'm unsure if it would even be indicated in 
> the guest

fair enough. maybe those are problems that can be somehow fixed in the
(far) future.

for example when loading the blob with diag 308 subcode 8, the host
could reject it with an appropriate error code (since it would fail
to boot anyway)

not something we have to worry about now, though

> 
> > 
> > ideally providing a custom message when generating the image, to be
> > shown if the required features are missing. e.g. for kvm unit test, the
> > custom message could be something like
> > SKIP: $TEST_NAME: Missing hardware features
> > 
> > once that is in place, we could revert this patch  
> 
> Also the host that's using genprotimg might not be PV enabled or even 
> s390x so checking on image generation is no option either.

checking at image generation time is exactly what I did not want.

> 
> >   
> >>
> >> Nico Boehr (1):
> >>    s390x: do not enable PV dump support by default
> >>
> >>   configure      | 11 +++++++++++
> >>   s390x/Makefile | 26 +++++++++++++++++---------
> >>   2 files changed, 28 insertions(+), 9 deletions(-)
> >>  
> >   
>