Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
On 24/06/2020 16.57, Cornelia Huck wrote:
On Wed, 24 Jun 2020 10:49:57 -0400
Collin Walling wrote:
On 6/24/20 8:55 AM, Cornelia Huck wrote:
On Wed, 24 Jun 2020 14:40:58 +0200
Thomas Huth wrote:
On 24/06/2020 14.36, Cornelia Huck wrote:
On Thu, 18 Jun 2020 18:22:56 -0400
Collin Walling wrote:
[...]
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 0dfbe6e5ec..f7c49e339e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr,
uint32_t code,
uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
switch (code & SCLP_CMD_CODE_MASK) {
+case SCLP_CMDW_READ_SCP_INFO:
+case SCLP_CMDW_READ_SCP_INFO_FORCED:
+case SCLP_CMDW_READ_CPU_INFO:
+/*
+ * An extended-length SCCB is only allowed for Read SCP/CPU Info and
+ * is allowed to exceed the 4k boundary. The respective commands will
+ * set the length field to the required length if an insufficient
+ * SCCB length is provided.
+ */
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+return true;
+}
Add a fallthrough annotation?
... otherwise Coverity and friends will complain later.
Nod.
Something simple like...
/* without this feature, these commands must respect the 4k boundary */
?
No, I meant something that is parsed by static checkers (/* fallthrough */
seems to be the common marker for that in QEMU). I think what the
fallthrough does is already clear enough to humans.
See also the "-Wimplicit-fallthrough" compiler option ... which we do
not have enabled for QEMU yet, but maybe will be enabled one day. It can
e.g. check for "/* fallthrough */" comments.
Thomas
Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
On Wed, 24 Jun 2020 10:49:57 -0400
Collin Walling wrote:
> On 6/24/20 8:55 AM, Cornelia Huck wrote:
> > On Wed, 24 Jun 2020 14:40:58 +0200
> > Thomas Huth wrote:
> >
> >> On 24/06/2020 14.36, Cornelia Huck wrote:
> >>> On Thu, 18 Jun 2020 18:22:56 -0400
> >>> Collin Walling wrote:
> >>>
> As more features and facilities are added to the Read SCP Info (RSCPI)
> response, more space is required to store them. The space used to store
> these new features intrudes on the space originally used to store CPU
> entries. This means as more features and facilities are added to the
> RSCPI response, less space can be used to store CPU entries.
>
> With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
> the RSCPI command and determine if the SCCB is large enough to store a
> complete reponse. If it is not large enough, then the required length
> will be set in the SCCB header.
>
> The caller of the SCLP command is responsible for creating a
> large-enough SCCB to store a complete response. Proper checking should
> be in place, and the caller should execute the command once-more with
> the large-enough SCCB.
>
> This facility also enables an extended SCCB for the Read CPU Info
> (RCPUI) command.
>
> When this facility is enabled, the boundary violation response cannot
> be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
>
> In order to tolerate kernels that do not yet have full support for this
> feature, a "fixed" offset to the start of the CPU Entries within the
> Read SCP Info struct is set to allow for the original 248 max entries
> when this feature is disabled.
>
> Additionally, this is introduced as a CPU feature to protect the guest
> from migrating to a machine that does not support storing an extended
> SCCB. This could otherwise hinder the VM from being able to read all
> available CPU entries after migration (such as during re-ipl).
>
> Signed-off-by: Collin Walling
> ---
> hw/s390x/sclp.c | 21 -
> include/hw/s390x/sclp.h | 1 +
> target/s390x/cpu_features_def.inc.h | 1 +
> target/s390x/gen-features.c | 1 +
> target/s390x/kvm.c | 8
> 5 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 0dfbe6e5ec..f7c49e339e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t
> sccb_addr, uint32_t code,
> uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>
> switch (code & SCLP_CMD_CODE_MASK) {
> +case SCLP_CMDW_READ_SCP_INFO:
> +case SCLP_CMDW_READ_SCP_INFO_FORCED:
> +case SCLP_CMDW_READ_CPU_INFO:
> +/*
> + * An extended-length SCCB is only allowed for Read SCP/CPU
> Info and
> + * is allowed to exceed the 4k boundary. The respective
> commands will
> + * set the length field to the required length if an
> insufficient
> + * SCCB length is provided.
> + */
> +if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
> +return true;
> +}
> >>>
> >>> Add a fallthrough annotation?
> >>
> >> ... otherwise Coverity and friends will complain later.
> >
> > Nod.
> >
>
> Something simple like...
>
> /* without this feature, these commands must respect the 4k boundary */
>
> ?
No, I meant something that is parsed by static checkers (/* fallthrough */
seems to be the common marker for that in QEMU). I think what the
fallthrough does is already clear enough to humans.
>
> >>
> default:
> if (sccb_max_addr < sccb_boundary) {
> return true;
Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
On 6/24/20 8:55 AM, Cornelia Huck wrote:
> On Wed, 24 Jun 2020 14:40:58 +0200
> Thomas Huth wrote:
>
>> On 24/06/2020 14.36, Cornelia Huck wrote:
>>> On Thu, 18 Jun 2020 18:22:56 -0400
>>> Collin Walling wrote:
>>>
As more features and facilities are added to the Read SCP Info (RSCPI)
response, more space is required to store them. The space used to store
these new features intrudes on the space originally used to store CPU
entries. This means as more features and facilities are added to the
RSCPI response, less space can be used to store CPU entries.
With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
the RSCPI command and determine if the SCCB is large enough to store a
complete reponse. If it is not large enough, then the required length
will be set in the SCCB header.
The caller of the SCLP command is responsible for creating a
large-enough SCCB to store a complete response. Proper checking should
be in place, and the caller should execute the command once-more with
the large-enough SCCB.
This facility also enables an extended SCCB for the Read CPU Info
(RCPUI) command.
When this facility is enabled, the boundary violation response cannot
be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
In order to tolerate kernels that do not yet have full support for this
feature, a "fixed" offset to the start of the CPU Entries within the
Read SCP Info struct is set to allow for the original 248 max entries
when this feature is disabled.
Additionally, this is introduced as a CPU feature to protect the guest
from migrating to a machine that does not support storing an extended
SCCB. This could otherwise hinder the VM from being able to read all
available CPU entries after migration (such as during re-ipl).
Signed-off-by: Collin Walling
---
hw/s390x/sclp.c | 21 -
include/hw/s390x/sclp.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/gen-features.c | 1 +
target/s390x/kvm.c | 8
5 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 0dfbe6e5ec..f7c49e339e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr,
uint32_t code,
uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
switch (code & SCLP_CMD_CODE_MASK) {
+case SCLP_CMDW_READ_SCP_INFO:
+case SCLP_CMDW_READ_SCP_INFO_FORCED:
+case SCLP_CMDW_READ_CPU_INFO:
+/*
+ * An extended-length SCCB is only allowed for Read SCP/CPU Info
and
+ * is allowed to exceed the 4k boundary. The respective commands
will
+ * set the length field to the required length if an insufficient
+ * SCCB length is provided.
+ */
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+return true;
+}
>>>
>>> Add a fallthrough annotation?
>>
>> ... otherwise Coverity and friends will complain later.
>
> Nod.
>
Something simple like...
/* without this feature, these commands must respect the 4k boundary */
?
>>
default:
if (sccb_max_addr < sccb_boundary) {
return true;
@@ -72,6 +84,10 @@ static bool sccb_sufficient_len(SCCB *sccb, int
num_cpus, int data_len)
if (be16_to_cpu(sccb->h.length) < required_len) {
sccb->h.response_code =
cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
+sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
+sccb->h.length = required_len;
+}
return false;
}
return true;
@@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms,
CPUEntry *entry, int *count)
*/
static inline int get_read_scp_info_data_len(void)
{
-return offsetof(ReadInfo, entries);
+return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
+ offsetof(ReadInfo, entries) :
+ SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
}
/* Provide information about the configuration, CPUs and storage */
@@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
CPUEntry *entries_start = (void *)sccb + data_len;
if (!sccb_sufficient_len(sccb, machine->possible_cpus->len,
data_len)) {
+warn_report("insufficient sccb size to store read scp info
response");
>>>
>>> Hm, this warning is triggered by a guest
Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
On Wed, 24 Jun 2020 14:40:58 +0200
Thomas Huth wrote:
> On 24/06/2020 14.36, Cornelia Huck wrote:
> > On Thu, 18 Jun 2020 18:22:56 -0400
> > Collin Walling wrote:
> >
> >> As more features and facilities are added to the Read SCP Info (RSCPI)
> >> response, more space is required to store them. The space used to store
> >> these new features intrudes on the space originally used to store CPU
> >> entries. This means as more features and facilities are added to the
> >> RSCPI response, less space can be used to store CPU entries.
> >>
> >> With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
> >> the RSCPI command and determine if the SCCB is large enough to store a
> >> complete reponse. If it is not large enough, then the required length
> >> will be set in the SCCB header.
> >>
> >> The caller of the SCLP command is responsible for creating a
> >> large-enough SCCB to store a complete response. Proper checking should
> >> be in place, and the caller should execute the command once-more with
> >> the large-enough SCCB.
> >>
> >> This facility also enables an extended SCCB for the Read CPU Info
> >> (RCPUI) command.
> >>
> >> When this facility is enabled, the boundary violation response cannot
> >> be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
> >>
> >> In order to tolerate kernels that do not yet have full support for this
> >> feature, a "fixed" offset to the start of the CPU Entries within the
> >> Read SCP Info struct is set to allow for the original 248 max entries
> >> when this feature is disabled.
> >>
> >> Additionally, this is introduced as a CPU feature to protect the guest
> >> from migrating to a machine that does not support storing an extended
> >> SCCB. This could otherwise hinder the VM from being able to read all
> >> available CPU entries after migration (such as during re-ipl).
> >>
> >> Signed-off-by: Collin Walling
> >> ---
> >> hw/s390x/sclp.c | 21 -
> >> include/hw/s390x/sclp.h | 1 +
> >> target/s390x/cpu_features_def.inc.h | 1 +
> >> target/s390x/gen-features.c | 1 +
> >> target/s390x/kvm.c | 8
> >> 5 files changed, 31 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> >> index 0dfbe6e5ec..f7c49e339e 100644
> >> --- a/hw/s390x/sclp.c
> >> +++ b/hw/s390x/sclp.c
> >> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr,
> >> uint32_t code,
> >> uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
> >>
> >> switch (code & SCLP_CMD_CODE_MASK) {
> >> +case SCLP_CMDW_READ_SCP_INFO:
> >> +case SCLP_CMDW_READ_SCP_INFO_FORCED:
> >> +case SCLP_CMDW_READ_CPU_INFO:
> >> +/*
> >> + * An extended-length SCCB is only allowed for Read SCP/CPU Info
> >> and
> >> + * is allowed to exceed the 4k boundary. The respective commands
> >> will
> >> + * set the length field to the required length if an insufficient
> >> + * SCCB length is provided.
> >> + */
> >> +if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
> >> +return true;
> >> +}
> >
> > Add a fallthrough annotation?
>
> ... otherwise Coverity and friends will complain later.
Nod.
>
> >> default:
> >> if (sccb_max_addr < sccb_boundary) {
> >> return true;
> >> @@ -72,6 +84,10 @@ static bool sccb_sufficient_len(SCCB *sccb, int
> >> num_cpus, int data_len)
> >>
> >> if (be16_to_cpu(sccb->h.length) < required_len) {
> >> sccb->h.response_code =
> >> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> >> +if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
> >> +sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
> >> +sccb->h.length = required_len;
> >> +}
> >> return false;
> >> }
> >> return true;
> >> @@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms,
> >> CPUEntry *entry, int *count)
> >>*/
> >> static inline int get_read_scp_info_data_len(void)
> >> {
> >> -return offsetof(ReadInfo, entries);
> >> +return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
> >> + offsetof(ReadInfo, entries) :
> >> + SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
> >> }
> >>
> >> /* Provide information about the configuration, CPUs and storage */
> >> @@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> >> CPUEntry *entries_start = (void *)sccb + data_len;
> >>
> >> if (!sccb_sufficient_len(sccb, machine->possible_cpus->len,
> >> data_len)) {
> >> +warn_report("insufficient sccb size to store read scp info
> >> response");
> >
> > Hm, this warning is triggered by a guest action, isn't it? Not sure how
> > helpful it is.
>
> I think this should be qemu_log_mask(LOG_GUEST_ERROR, ...) instead?
Yes, that sounds better.
Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
On 24/06/2020 14.36, Cornelia Huck wrote:
On Thu, 18 Jun 2020 18:22:56 -0400
Collin Walling wrote:
As more features and facilities are added to the Read SCP Info (RSCPI)
response, more space is required to store them. The space used to store
these new features intrudes on the space originally used to store CPU
entries. This means as more features and facilities are added to the
RSCPI response, less space can be used to store CPU entries.
With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
the RSCPI command and determine if the SCCB is large enough to store a
complete reponse. If it is not large enough, then the required length
will be set in the SCCB header.
The caller of the SCLP command is responsible for creating a
large-enough SCCB to store a complete response. Proper checking should
be in place, and the caller should execute the command once-more with
the large-enough SCCB.
This facility also enables an extended SCCB for the Read CPU Info
(RCPUI) command.
When this facility is enabled, the boundary violation response cannot
be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
In order to tolerate kernels that do not yet have full support for this
feature, a "fixed" offset to the start of the CPU Entries within the
Read SCP Info struct is set to allow for the original 248 max entries
when this feature is disabled.
Additionally, this is introduced as a CPU feature to protect the guest
from migrating to a machine that does not support storing an extended
SCCB. This could otherwise hinder the VM from being able to read all
available CPU entries after migration (such as during re-ipl).
Signed-off-by: Collin Walling
---
hw/s390x/sclp.c | 21 -
include/hw/s390x/sclp.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/gen-features.c | 1 +
target/s390x/kvm.c | 8
5 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 0dfbe6e5ec..f7c49e339e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr,
uint32_t code,
uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
switch (code & SCLP_CMD_CODE_MASK) {
+case SCLP_CMDW_READ_SCP_INFO:
+case SCLP_CMDW_READ_SCP_INFO_FORCED:
+case SCLP_CMDW_READ_CPU_INFO:
+/*
+ * An extended-length SCCB is only allowed for Read SCP/CPU Info and
+ * is allowed to exceed the 4k boundary. The respective commands will
+ * set the length field to the required length if an insufficient
+ * SCCB length is provided.
+ */
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+return true;
+}
Add a fallthrough annotation?
... otherwise Coverity and friends will complain later.
default:
if (sccb_max_addr < sccb_boundary) {
return true;
@@ -72,6 +84,10 @@ static bool sccb_sufficient_len(SCCB *sccb, int num_cpus,
int data_len)
if (be16_to_cpu(sccb->h.length) < required_len) {
sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
+sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
+sccb->h.length = required_len;
+}
return false;
}
return true;
@@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry
*entry, int *count)
*/
static inline int get_read_scp_info_data_len(void)
{
-return offsetof(ReadInfo, entries);
+return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
+ offsetof(ReadInfo, entries) :
+ SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
}
/* Provide information about the configuration, CPUs and storage */
@@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
CPUEntry *entries_start = (void *)sccb + data_len;
if (!sccb_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
+warn_report("insufficient sccb size to store read scp info response");
Hm, this warning is triggered by a guest action, isn't it? Not sure how
helpful it is.
I think this should be qemu_log_mask(LOG_GUEST_ERROR, ...) instead?
Thomas
Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
On Thu, 18 Jun 2020 18:22:56 -0400
Collin Walling wrote:
> As more features and facilities are added to the Read SCP Info (RSCPI)
> response, more space is required to store them. The space used to store
> these new features intrudes on the space originally used to store CPU
> entries. This means as more features and facilities are added to the
> RSCPI response, less space can be used to store CPU entries.
>
> With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
> the RSCPI command and determine if the SCCB is large enough to store a
> complete reponse. If it is not large enough, then the required length
> will be set in the SCCB header.
>
> The caller of the SCLP command is responsible for creating a
> large-enough SCCB to store a complete response. Proper checking should
> be in place, and the caller should execute the command once-more with
> the large-enough SCCB.
>
> This facility also enables an extended SCCB for the Read CPU Info
> (RCPUI) command.
>
> When this facility is enabled, the boundary violation response cannot
> be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
>
> In order to tolerate kernels that do not yet have full support for this
> feature, a "fixed" offset to the start of the CPU Entries within the
> Read SCP Info struct is set to allow for the original 248 max entries
> when this feature is disabled.
>
> Additionally, this is introduced as a CPU feature to protect the guest
> from migrating to a machine that does not support storing an extended
> SCCB. This could otherwise hinder the VM from being able to read all
> available CPU entries after migration (such as during re-ipl).
>
> Signed-off-by: Collin Walling
> ---
> hw/s390x/sclp.c | 21 -
> include/hw/s390x/sclp.h | 1 +
> target/s390x/cpu_features_def.inc.h | 1 +
> target/s390x/gen-features.c | 1 +
> target/s390x/kvm.c | 8
> 5 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 0dfbe6e5ec..f7c49e339e 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr,
> uint32_t code,
> uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
>
> switch (code & SCLP_CMD_CODE_MASK) {
> +case SCLP_CMDW_READ_SCP_INFO:
> +case SCLP_CMDW_READ_SCP_INFO_FORCED:
> +case SCLP_CMDW_READ_CPU_INFO:
> +/*
> + * An extended-length SCCB is only allowed for Read SCP/CPU Info and
> + * is allowed to exceed the 4k boundary. The respective commands will
> + * set the length field to the required length if an insufficient
> + * SCCB length is provided.
> + */
> +if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
> +return true;
> +}
Add a fallthrough annotation?
> default:
> if (sccb_max_addr < sccb_boundary) {
> return true;
> @@ -72,6 +84,10 @@ static bool sccb_sufficient_len(SCCB *sccb, int num_cpus,
> int data_len)
>
> if (be16_to_cpu(sccb->h.length) < required_len) {
> sccb->h.response_code =
> cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
> +if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
> +sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
> +sccb->h.length = required_len;
> +}
> return false;
> }
> return true;
> @@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms,
> CPUEntry *entry, int *count)
> */
> static inline int get_read_scp_info_data_len(void)
> {
> -return offsetof(ReadInfo, entries);
> +return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
> + offsetof(ReadInfo, entries) :
> + SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
> }
>
> /* Provide information about the configuration, CPUs and storage */
> @@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> CPUEntry *entries_start = (void *)sccb + data_len;
>
> if (!sccb_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
> +warn_report("insufficient sccb size to store read scp info
> response");
Hm, this warning is triggered by a guest action, isn't it? Not sure how
helpful it is.
> return;
> }
>
(...)
Otherwise, looks good to me.
[PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest
As more features and facilities are added to the Read SCP Info (RSCPI)
response, more space is required to store them. The space used to store
these new features intrudes on the space originally used to store CPU
entries. This means as more features and facilities are added to the
RSCPI response, less space can be used to store CPU entries.
With the Extended-Length SCCB (ELS) facility, a KVM guest can execute
the RSCPI command and determine if the SCCB is large enough to store a
complete reponse. If it is not large enough, then the required length
will be set in the SCCB header.
The caller of the SCLP command is responsible for creating a
large-enough SCCB to store a complete response. Proper checking should
be in place, and the caller should execute the command once-more with
the large-enough SCCB.
This facility also enables an extended SCCB for the Read CPU Info
(RCPUI) command.
When this facility is enabled, the boundary violation response cannot
be a result from the RSCPI, RSCPI Forced, or RCPUI commands.
In order to tolerate kernels that do not yet have full support for this
feature, a "fixed" offset to the start of the CPU Entries within the
Read SCP Info struct is set to allow for the original 248 max entries
when this feature is disabled.
Additionally, this is introduced as a CPU feature to protect the guest
from migrating to a machine that does not support storing an extended
SCCB. This could otherwise hinder the VM from being able to read all
available CPU entries after migration (such as during re-ipl).
Signed-off-by: Collin Walling
---
hw/s390x/sclp.c | 21 -
include/hw/s390x/sclp.h | 1 +
target/s390x/cpu_features_def.inc.h | 1 +
target/s390x/gen-features.c | 1 +
target/s390x/kvm.c | 8
5 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index 0dfbe6e5ec..f7c49e339e 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -56,6 +56,18 @@ static bool sccb_has_valid_boundary(uint64_t sccb_addr,
uint32_t code,
uint64_t sccb_boundary = (sccb_addr & PAGE_MASK) + PAGE_SIZE;
switch (code & SCLP_CMD_CODE_MASK) {
+case SCLP_CMDW_READ_SCP_INFO:
+case SCLP_CMDW_READ_SCP_INFO_FORCED:
+case SCLP_CMDW_READ_CPU_INFO:
+/*
+ * An extended-length SCCB is only allowed for Read SCP/CPU Info and
+ * is allowed to exceed the 4k boundary. The respective commands will
+ * set the length field to the required length if an insufficient
+ * SCCB length is provided.
+ */
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB)) {
+return true;
+}
default:
if (sccb_max_addr < sccb_boundary) {
return true;
@@ -72,6 +84,10 @@ static bool sccb_sufficient_len(SCCB *sccb, int num_cpus,
int data_len)
if (be16_to_cpu(sccb->h.length) < required_len) {
sccb->h.response_code = cpu_to_be16(SCLP_RC_INSUFFICIENT_SCCB_LENGTH);
+if (s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) &&
+sccb->h.control_mask[2] & SCLP_VARIABLE_LENGTH_RESPONSE) {
+sccb->h.length = required_len;
+}
return false;
}
return true;
@@ -101,7 +117,9 @@ static void prepare_cpu_entries(MachineState *ms, CPUEntry
*entry, int *count)
*/
static inline int get_read_scp_info_data_len(void)
{
-return offsetof(ReadInfo, entries);
+return s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
+ offsetof(ReadInfo, entries) :
+ SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
}
/* Provide information about the configuration, CPUs and storage */
@@ -116,6 +134,7 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
CPUEntry *entries_start = (void *)sccb + data_len;
if (!sccb_sufficient_len(sccb, machine->possible_cpus->len, data_len)) {
+warn_report("insufficient sccb size to store read scp info response");
return;
}
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index 822eff4396..ef2d63eae9 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -110,6 +110,7 @@ typedef struct CPUEntry {
uint8_t reserved1;
} QEMU_PACKED CPUEntry;
+#define SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET 128
typedef struct ReadInfo {
SCCBHeader h;
uint16_t rnmax;
diff --git a/target/s390x/cpu_features_def.inc.h
b/target/s390x/cpu_features_def.inc.h
index 5942f81f16..1c04cc18f4 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -97,6 +97,7 @@ DEF_FEAT(GUARDED_STORAGE, "gs", STFL, 133, "Guarded-storage
facility")
DEF_FEAT(VECTOR_PACKED_DECIMAL, "vxpd", STFL, 134, "Vector packed decimal
facility")
DEF_FEAT(VECTOR_ENH, "vxeh", STFL, 135, "Vector enhancements facility")
DEF_FEAT(MULTIPLE_EPOCH, "mepoch", STFL, 139, "Multiple-epoch facility")
+DEF_FEAT(EXTENDED_LENGTH_SCCB, "els", STFL, 140, "Extended-length SCCB
