Re: [PATCH 2/5] s390x/pv: Implement CGS check handler

[Cédric Le Goater]

On 1/5/23 14:58, Claudio Imbrenda wrote:
> On Thu, 5 Jan 2023 12:42:54 +0100
> Thomas Huth <thuth@redhat.com> wrote:
>
> > On 04/01/2023 12.51, Cédric Le Goater wrote:
> > > From: Cédric Le Goater <clg@redhat.com>
> > >
> > > When a protected VM is started with the maximum number of CPUs (248),
> > > the service call providing information on the CPUs requires more
> > > buffer space than allocated and QEMU disgracefully aborts :
> > >
> > >       LOADPARM=[........]
> > >       Using virtio-blk.
> > >       Using SCSI scheme.
> > >       
> > > ...................................................................................
> > >       qemu-system-s390x: KVM_S390_MEM_OP failed: Argument list too long
> > >
> > > Implement a test for this limitation in the ConfidentialGuestSupportClass
> > > check handler and provide some valid information to the user before the
> > > machine starts.
> > >
> > > Signed-off-by: Cédric Le Goater <clg@redhat.com>
> > > ---
> > >    hw/s390x/pv.c | 23 +++++++++++++++++++++++
> > >    1 file changed, 23 insertions(+)
> > >
> > > diff --git a/hw/s390x/pv.c b/hw/s390x/pv.c
> > > index 8dfe92d8df..3a7ec70634 100644
> > > --- a/hw/s390x/pv.c
> > > +++ b/hw/s390x/pv.c
> > > @@ -266,6 +266,26 @@ int s390_pv_kvm_init(ConfidentialGuestSupport *cgs, Error 
> > > **errp)
> > >        return 0;
> > >    }
> > >    
> > > +static bool s390_pv_check_cpus(Error **errp)
> > > +{
> > > +    MachineState *ms = MACHINE(qdev_get_machine());
> > > +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> > > +    uint32_t pv_max_cpus = mc->max_cpus - 1;
> >
> > Not sure whether "mc->max_cpus - 1" is the right approach here. I think it
> > would be better to calculate the amount of CPUs that we can support.
> >
> > So AFAIK the problem is that SCLP information that is gathered during
> > read_SCP_info() in hw/s390x/sclp.c. If protected virtualization is enabled,
> > everything has to fit in one page (i.e. 4096 bytes) there.
> >
> > So we have space for
> >
> >    (TARGET_PAGE_SIZE - offset_cpu) / sizeof(CPUEntry)
> >
> > CPUs.
> >
> > With S390_FEAT_EXTENDED_LENGTH_SCCB enabled, offset_cpu is 144 (see struct
> > ReadInfo in sclp.h), otherwise it is 128.
> >
> > That means, with S390_FEAT_EXTENDED_LENGTH_SCCB we can have a maximum of:
> >
> >    (4096 - 144) / 16 = 247 CPUs
> >
> > which is what you were trying to check with the mc->max_cpus - 1 here.

yes. That's much better.

> > But with "-cpu els=off", it sounds like we could fit all 248 also with
> > protected VMs? Could you please give it a try?

It runs. Unfortunately, QEMU also complains with :

  qemu-system-s390x: warning: 'diag318' requires 'els'.
  qemu-system-s390x: warning: 'diag318' requires 'els'.
  qemu-system-s390x: warning: 'diag318' requires 'els'.

when els is off.


> > Anyway, instead of using "pv_max_cpus = mc->max_cpus - 1" I'd suggest to use
> > something like this instead:
> >
> >    int offset_cpu = s390_has_feat(S390_FEAT_EXTENDED_LENGTH_SCCB) ?
> >                        offsetof(ReadInfo, entries) :
> >                        SCLP_READ_SCP_INFO_FIXED_CPU_OFFSET;
> >    pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);
>
> I agree with Thomas here


The problem is that QEMU doesn't know about the S390_FEAT_EXTENDED_LENGTH_SCCB
feature when the PV object link is first checked. So #248 CPUs is considered
valid, but when DIAG308_PV_START is called, it fails.

Let's simplify and use :

    int offset_cpu = offsetof(ReadInfo, entries);
    pv_max_cpus = (TARGET_PAGE_SIZE - offset_cpu) /sizeof(CPUEntry);

?

Thanks,

C.