Re: [PATCH v3 6/8] s390/sclp: add extended-length sccb support for kvm guest

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 24 Jun 2020 10:49:57 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> On 6/24/20 8:55 AM, Cornelia Huck wrote:
> > On Wed, 24 Jun 2020 14:40:58 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 24/06/2020 14.36, Cornelia Huck wrote:  
> >>> On Thu, 18 Jun 2020 18:22:56 -0400
> >>> Collin Walling <walling@linux.ibm.com> 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 <walling@linux.ibm.com>
> >>>> ---
> >>>>   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;