Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support

[Cornelia Huck]

On Mon, 27 Jan 2020 11:39:02 -0500
Collin Walling <address@hidden> wrote:

> On 1/27/20 6:47 AM, Cornelia Huck wrote:
> > On Fri, 24 Jan 2020 17:14:04 -0500
> > Collin Walling <address@hidden> wrote:
> >   
> >> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
> >> be intercepted by SIE and handled via KVM. Let's introduce some
> >> functions to communicate between QEMU and KVM via ioctls. These
> >> will be used to get/set the diag318 information.  
> > 
> > Do you want to give a hint what diag 318 actually does?
> >   
> 
> For the sake of completeness, I'll have to get back to you on this.
> 
> >>
> >> The availability of this instruction is determined by byte 134, bit 0
> >> of the Read Info block. This coincidentally expands into the space used  
> > 
> > "SCLP Read Info"
> >   
> >> for CPU entries by taking away one byte, which means VMs running with
> >> the diag318 capability will not be able to retrieve information regarding
> >> the 248th CPU. This will not effect performance, and VMs can still be
> >> ran with 248 CPUs.  
> > 
> > Are there other ways in which that might affect guests? I assume Linux
> > can deal with it? Is it ok architecture-wise?
> > 
> > In any case, should go into the patch description :)
> >   
> 
> Same as above. I'll try to provide more information regarding what happens
> here in my next reply.

I think you can lift some stuff from the cover letter.

> 
> >>
> >> In order to simplify the migration and system reset requirements of
> >> the diag318 data, let's introduce it as a device class and include
> >> a VMStateDescription.
> >>
> >> Diag318 is set to 0 during modified clear and load normal resets.  
> > 
> > What exactly is set to 0? Stored values?
> >   
> 
> Correct. "The stored values set by DIAG318 are reset to 0 during..."

Sounds good.

> 
> >>
> >> Signed-off-by: Collin Walling <address@hidden>
> >> ---
> >>  hw/s390x/Makefile.objs              |  1 +
> >>  hw/s390x/diag318.c                  | 85 
> >> +++++++++++++++++++++++++++++++++++++
> >>  hw/s390x/diag318.h                  | 40 +++++++++++++++++
> >>  hw/s390x/s390-virtio-ccw.c          | 17 ++++++++
> >>  hw/s390x/sclp.c                     | 13 ++++++
> >>  include/hw/s390x/sclp.h             |  2 +
> >>  target/s390x/cpu_features.h         |  1 +
> >>  target/s390x/cpu_features_def.inc.h |  3 ++
> >>  target/s390x/gen-features.c         |  1 +
> >>  target/s390x/kvm-stub.c             | 10 +++++
> >>  target/s390x/kvm.c                  | 29 +++++++++++++
> >>  target/s390x/kvm_s390x.h            |  2 +
> >>  12 files changed, 204 insertions(+)
> >>  create mode 100644 hw/s390x/diag318.c
> >>  create mode 100644 hw/s390x/diag318.h
> >>  
> > (...)  

> >> +static bool diag318_needed(void *opaque)
> >> +{
> >> +    return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;  
> > 
> > Why do you need to guard this with kvm_enabled()? If tcg does not
> > enable the feature, we should be fine; and if it emulates this in the
> > future, we probably need to migrate something anyway.
> >   
> 
> Your explanation makes sense. My thoughts were to not even bother
> registering the state description if KVM isn't enabled (but I guess
> that thinking would mean that the other kvm_enabled fencing would
> be redundant? Doh.)

My thinking was along the lines how easy it would be to do some tcg
implementation (not sure if that even makes sense.)

> 
> I'll fix this.
> 

> >> @@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
> >>  
> >>      /* init the TOD clock */
> >>      s390_init_tod();
> >> +
> >> +    /* init object used for migrating diag318 info */
> >> +    s390_init_diag318();  
> > 
> > Doesn't that device do a bit more than 'migrating' info?
> > 
> > Also, it seems a bit useless unless you're running with kvm and the
> > feature bit switched on...
> >   
> 
> Right... I think this whole "diag318 device" thing needs some rethinking.
> 
> I made a comment on David's response regarding where to but the 
> VMStateDescription
> code for diag318. Perhaps including the related information within the 
> S390MachineState
> would be better? (I'm not sure).

Replied to David's mail.


> >> @@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, 
> >> CPUEntry *entry, int *count)
> >>  {
> >>      MachineState *ms = MACHINE(qdev_get_machine());
> >>      uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> >> +    int max_entries;
> >>      int i;
> >>  
> >> +    /* Calculate the max number of CPU entries that can be stored in the 
> >> SCCB */
> >> +    max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / 
> >> sizeof(CPUEntry);
> >> +
> >>      s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
> >>      for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
> >> +        if (*count == max_entries) {
> >> +            warn_report("Configuration only supports a max of %d CPU 
> >> entries.",
> >> +                        max_entries);  
> > 
> > IIUC, this only moans during Read Info... but you could previously add
> > more cpus than what could be serviced by Read Info. So, it looks to me
> > you get some messages when a guest is doing Read Info; that seems more
> > confusing than helpful to me. Can't we rather warn at cpu instantiation
> > time?
> >   
> 
> Ahh, I didn't think of that. For some reason, I was thinking that Read Info
> would only be queried once.

Linux probably only does it once, but there's nothing stopping a guest
from doing it more often :)

> 
> Yes, this makes sense. I'll relocate the warning message...
> 
> >> +            break;
> >> +        }
> >>          if (!ms->possible_cpus->cpus[i].cpu) {
> >>              continue;
> >>          }