lkml.org 
[lkml]   [2021]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv2 2/4] arm64: add guest pvstate support
    On (21/07/12 16:42), Marc Zyngier wrote:
    > >
    > > PV-vcpu-state is a per-CPU struct, which, for the time being,
    > > holds boolean `preempted' vCPU state. During the startup,
    > > given that host supports PV-state, each guest vCPU sends
    > > a pointer to its per-CPU variable to the host as a payload
    >
    > What is the expected memory type for this memory region? What is its
    > life cycle? Where is it allocated from?

    Guest per-CPU area, which physical addresses is shared with the host.

    > > with the SMCCC HV call, so that host can update vCPU state
    > > when it puts or loads vCPU.
    > >
    > > This has impact on the guest's scheduler:
    > >
    > > [..]
    > > wake_up_process()
    > > try_to_wake_up()
    > > select_task_rq_fair()
    > > available_idle_cpu()
    > > vcpu_is_preempted()
    > >
    > > Some sched benchmarks data is available on the github page [0].
    > >
    > > [0] https://github.com/sergey-senozhatsky/arm64-vcpu_is_preempted
    >
    > Please include these results in the cover letter. I tend to reply to
    > email while offline, and I can't comment on GH.

    ACK.

    > > +struct vcpu_state {
    >
    > If this is KVM specific (which it most likely is), please name-space
    > it correctly, and move it to a KVM-specific location.

    ACK.

    > > + bool preempted;
    > > + u8 reserved[63];
    >
    > Why 63? Do you attach any particular meaning to a 64byte structure
    > (and before you say "cache line size", please look at some of the
    > cache line sizes we have to deal with...).

    We do have some future plans to share some bits of the guest's context
    with the host.

    > This should also be versioned from day-1, one way or another.

    Makes sense.

    > > +};
    > > +
    > > #ifdef CONFIG_PARAVIRT
    > > #include <linux/static_call_types.h>
    > >
    > > @@ -20,8 +25,22 @@ static inline u64 paravirt_steal_clock(int cpu)
    > >
    > > int __init pv_time_init(void);
    > >
    > > +bool dummy_vcpu_is_preempted(unsigned int cpu);
    > > +
    > > +extern struct static_key pv_vcpu_is_preempted_enabled;
    > > +DECLARE_STATIC_CALL(pv_vcpu_is_preempted, dummy_vcpu_is_preempted);
    > > +
    > > +static inline bool paravirt_vcpu_is_preempted(unsigned int cpu)
    > > +{
    > > + return static_call(pv_vcpu_is_preempted)(cpu);
    > > +}
    > > +
    > > +int __init pv_vcpu_state_init(void);
    > > +
    > > #else
    > >
    > > +#define pv_vcpu_state_init() do {} while (0)
    > > +
    > > #define pv_time_init() do {} while (0)
    > >
    > > #endif // CONFIG_PARAVIRT
    > > diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
    > > index 75fed4460407..d8fc46795d94 100644
    > > --- a/arch/arm64/kernel/paravirt.c
    > > +++ b/arch/arm64/kernel/paravirt.c
    > > @@ -40,6 +40,11 @@ struct pv_time_stolen_time_region {
    > >
    > > static DEFINE_PER_CPU(struct pv_time_stolen_time_region, stolen_time_region);
    > >
    > > +static DEFINE_PER_CPU(struct vcpu_state, vcpus_states);
    >
    > nit: there is only one 'state' structure per CPU, so I'd prefer the
    > singular form.

    ACK.

    > > +struct static_key pv_vcpu_is_preempted_enabled;
    > > +
    > > +DEFINE_STATIC_CALL(pv_vcpu_is_preempted, dummy_vcpu_is_preempted);
    > > +
    > > static bool steal_acc = true;
    > > static int __init parse_no_stealacc(char *arg)
    > > {
    > > @@ -165,3 +170,92 @@ int __init pv_time_init(void)
    > >
    > > return 0;
    > > }
    > > +
    > > +bool dummy_vcpu_is_preempted(unsigned int cpu)
    >
    > Why does this have to be global?

    I think this can be moved away from the header, so then we don't need
    to DECLARE_STATIC_CALL() with a dummy function.

    > > +static bool has_pv_vcpu_state(void)
    > > +{
    > > + struct arm_smccc_res res;
    > > +
    > > + /* To detect the presence of PV time support we require SMCCC 1.1+ */
    > > + if (arm_smccc_1_1_get_conduit() == SMCCC_CONDUIT_NONE)
    > > + return false;
    > > +
    > > + arm_smccc_1_1_invoke(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
    > > + ARM_SMCCC_HV_PV_VCPU_STATE_FEATURES,
    > > + &res);
    > > +
    > > + if (res.a0 != SMCCC_RET_SUCCESS)
    > > + return false;
    > > + return true;
    >
    > Please move all this over the the KVM-specific discovery mechanism.

    Will take a look.

    > > +static int __pv_vcpu_state_hook(unsigned int cpu, int event)
    > > +{
    > > + struct arm_smccc_res res;
    > > + struct vcpu_state *st;
    > > +
    > > + st = &per_cpu(vcpus_states, cpu);
    > > + arm_smccc_1_1_invoke(event, virt_to_phys(st), &res);
    > > + if (res.a0 != SMCCC_RET_SUCCESS)
    > > + return -EINVAL;
    > > + return 0;
    > > +}
    > > +
    > > +static int vcpu_state_init(unsigned int cpu)
    > > +{
    > > + int ret = __pv_vcpu_state_hook(cpu, ARM_SMCCC_HV_PV_VCPU_STATE_INIT);
    > > +
    > > + if (ret)
    > > + pr_warn("Unable to ARM_SMCCC_HV_PV_STATE_INIT\n");
    >
    > pr_warn_once(), please.

    ACK.

    \
     
     \ /
      Last update: 2021-07-21 04:07    [W:3.535 / U:0.200 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site