Re: [PATCH 3/3] hw/arm/virt: Implement kvm-steal-time

[Andrew Jones <drjones@redhat.com>]

On Fri, Jul 31, 2020 at 03:54:07PM +0100, Peter Maydell wrote:
> On Sat, 11 Jul 2020 at 11:10, Andrew Jones <drjones@redhat.com> wrote:
> > We add the kvm-steal-time CPU property and implement it for machvirt.
> > A tiny bit of refactoring was also done to allow pmu and pvtime to
> > use the same vcpu device helper functions.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> Hi; I'm forwarding a couple of comments here from Beata,
> (whose secondment with Linaro is coming to an end so she won't
> have access to her Linaro email address to continue the conversation):
> 
> 
> >  static void virt_cpu_post_init(VirtMachineState *vms)
> >  {
> > -    bool aarch64, pmu;
> > +    bool aarch64, pmu, steal_time;
> >      CPUState *cpu;
> >
> >      aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
> >      pmu = object_property_get_bool(OBJECT(first_cpu), "pmu", NULL);
> > +    steal_time = object_property_get_bool(OBJECT(first_cpu),
> > +                                          "kvm-steal-time", NULL);
> >
> >      if (kvm_enabled()) {
> > +        hwaddr pvtime_base = vms->memmap[VIRT_PVTIME].base;
> > +        hwaddr pvtime_size = vms->memmap[VIRT_PVTIME].size;
> > +
> > +        if (steal_time) {
> > +            MemoryRegion *pvtime = g_new(MemoryRegion, 1);
> > +
> > +            memory_region_init_ram(pvtime, NULL, "pvtime", pvtime_size, NULL);
> > +            memory_region_add_subregion(get_system_memory(), pvtime_base,
> > +                                        pvtime);
> > +        }
> 
> B: I'm not sure whether it wouldn't be useful to have the area
> allocated with size determined by number of VCPUs instead of having
> pre-defined size.

We can't go smaller than one host-sized page, so this 64k region is the
smallest we can go. The assert in the next hunk, which was snipped
out of the reply, ensures that 64k is large enough to cover the maximum
number of VCPUs that could ever be configured. I don't think there's
anything else we should do at this time. If the pvtime structure grows,
or if we increase the maximum number of VCPUs to be larger than 1024,
then we can revisit this in order to determine when additional 64k pages
should be allocated.

For now, if it would help, I could extend the comment (which was also
snipped) to mention that 64k was chosen because it's the maximum host
page size, and that at least one host-sized page must be allocated for
this region.

> 
> > +        if (vmc->kvm_no_steal_time &&
> > +            object_property_find(cpuobj, "kvm-steal-time", NULL)) {
> > +            object_property_set_bool(cpuobj, false, "kvm-steal-time", NULL);
> > +        }
> > +
> >          if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
> >              object_property_set_bool(cpuobj, "pmu", false, NULL);
> >          }
> > @@ -2528,6 +2558,7 @@ static void virt_machine_5_0_options(MachineClass *mc)
> >      mc->numa_mem_supported = true;
> >      vmc->acpi_expose_flash = true;
> >      mc->auto_enable_numa_with_memdev = false;
> > +    vmc->kvm_no_steal_time = true;
> >  }
> >  DEFINE_VIRT_MACHINE(5, 0)
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 54bcf17afd35..b5153afedcdf 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -80,6 +80,7 @@ enum {
> >      VIRT_PCDIMM_ACPI,
> >      VIRT_ACPI_GED,
> >      VIRT_NVDIMM_ACPI,
> > +    VIRT_PVTIME,
> >      VIRT_LOWMEMMAP_LAST,
> >  };
> >
> > @@ -126,6 +127,7 @@ typedef struct {
> >      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
> >      bool kvm_no_adjvtime;
> >      bool acpi_expose_flash;
> > +    bool kvm_no_steal_time;
> 
> B: It is slightly confusing : using kvm_no_steal_time vs kvm_steal_time
> 
> P: I have to admit I get confused about which sense this flag
> should have. I think the sense of the flags in this struct is
> "the false case is the one that the older virt boards had",
> so original virt didn't have an ITS or a PMU and so we have
> no_its and no_pmu. Similarly here old virt didn't have steal-time
> and so we want a no_ flag (ie the patch is correct). Why
> kvm_no_steal_time rather than no_kvm_steal_time, though ?

Correct. We want the default value of the boolean (false) to mean
"not disabled", so the boolean must have a negative name in order
to mean "disabled" when it is true. I'm not opposed to
no_kvm_steal_time vs. kvm_no_steal_time, since the property name
is "kvm-steal-time". I think I ended up with the 'kvm' and 'no'
swapped by copy+pasting kvm_no_adjvtime. However that one is
appropriately named because the property name is "kvm-no-adjvtime".
I'll change this for v2.

> 
> >  } VirtMachineClass;
> 
> > +void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
> > +{
> > +    struct kvm_device_attr attr = {
> > +        .group = KVM_ARM_VCPU_PVTIME_CTRL,
> > +        .attr = KVM_ARM_VCPU_PVTIME_IPA,
> > +        .addr = (uint64_t)&ipa,
> > +    };
> > +
> > +    if (!ARM_CPU(cs)->kvm_steal_time) {
> > +        return;
> > +    }
> > +    if (!kvm_arm_set_device_attr(cs, &attr, "PVTIME IPA")) {
> > +        error_report("failed to init PVTIME IPA");
> > +        abort();
> > +    }
> > +}
> 
> B: I am probably missing smth but .. there is a trigger missing to
> update the stats
> and write them back to pre-allocated guest memory.
> Looking at the kernel code the stats are updated upon pending
> VCPU request :
> in arch/arm64/kvm/arm.c:
> static void check_vcpu_requests(struct kvm_vcpu *vcpu) {
>         ...
>          if (kvm_check_request(KVM_REQ_RECORD_STEAL, vcpu))
>                 kvm_update_stolen_time(vcpu);
> }

kvm_arch_vcpu_load() unconditionally makes that request when pvtime
is enabled.

Thanks,
drew