On Mon, 27 Jan 2020 11:39:02 -0500
Collin Walling <wall...@linux.ibm.com> wrote:
> On 1/27/20 6:47 AM, Cornelia Huck wrote:
> > On Fri, 24 Jan 2020 17:14:04 -0500
> > Collin Walling <wall...@linux.ibm.com> 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 <wall...@linux.ibm.com>
> >> ---
> >> 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;
> >> }
