Hi Gautam,
Thanks for reviewing this patch. My responses to your review comments
inline below:
Gautam Menghani <gautam@xxxxxxxxxxxxx> writes:
> On Sun, Dec 22, 2024 at 07:32:33PM +0530, Vaibhav Jain wrote:
>> Implement and setup necessary structures to send a prepolulated
>> Guest-State-Buffer(GSB) requesting hostwide counters to L0-PowerVM and have
>> the returned GSB holding the values of these counters parsed. This is done
>> via existing GSB implementation and with the newly added support of
>> Hostwide elements in GSB.
>>
>> The request to L0-PowerVM to return Hostwide counters is done using a
>> pre-allocated GSB named 'gsb_l0_stats'. To be able to populate this GSB
>> with the needed Guest-State-Elements (GSIDs) a instance of 'struct
>> kvmppc_gs_msg' named 'gsm_l0_stats' is introduced. The 'gsm_l0_stats' is
>> tied to an instance of 'struct kvmppc_gs_msg_ops' named 'gsb_ops_l0_stats'
>> which holds various callbacks to be compute the size ( hostwide_get_size()
>> ), populate the GSB ( hostwide_fill_info() ) and
>> refresh ( hostwide_refresh_info() ) the contents of
>> 'l0_stats' that holds the Hostwide counters returned from L0-PowerVM.
>>
>> To protect these structures from simultaneous access a spinlock
>> 'lock_l0_stats' has been introduced. The allocation and initialization of
>> the above structures is done in newly introduced kvmppc_init_hostwide() and
>> similarly the cleanup is performed in newly introduced
>> kvmppc_cleanup_hostwide().
>>
>> Signed-off-by: Vaibhav Jain <vaibhav@xxxxxxxxxxxxx>
>> ---
>> arch/powerpc/kvm/book3s_hv_pmu.c | 189 +++++++++++++++++++++++++++++++
>> 1 file changed, 189 insertions(+)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv_pmu.c b/arch/powerpc/kvm/book3s_hv_pmu.c
>> index e72542d5e750..f7fd5190ecf7 100644
>> --- a/arch/powerpc/kvm/book3s_hv_pmu.c
>> +++ b/arch/powerpc/kvm/book3s_hv_pmu.c
>> @@ -27,10 +27,31 @@
>> #include <asm/plpar_wrappers.h>
>> #include <asm/firmware.h>
>>
>> +#include "asm/guest-state-buffer.h"
>> +
>> enum kvmppc_pmu_eventid {
>> KVMPPC_EVENT_MAX,
>> };
>>
>> +#define KVMPPC_PMU_EVENT_ATTR(_name, _id) \
>> + PMU_EVENT_ATTR_ID(_name, power_events_sysfs_show, _id)
>> +
>> +/* Holds the hostwide stats */
>> +static struct kvmppc_hostwide_stats {
>> + u64 guest_heap;
>> + u64 guest_heap_max;
>> + u64 guest_pgtable_size;
>> + u64 guest_pgtable_size_max;
>> + u64 guest_pgtable_reclaim;
>> +} l0_stats;
>> +
>> +/* Protect access to l0_stats */
>> +static DEFINE_SPINLOCK(lock_l0_stats);
>> +
>> +/* GSB related structs needed to talk to L0 */
>> +static struct kvmppc_gs_msg *gsm_l0_stats;
>> +static struct kvmppc_gs_buff *gsb_l0_stats;
>> +
>> static struct attribute *kvmppc_pmu_events_attr[] = {
>> NULL,
>> };
>> @@ -90,6 +111,167 @@ static void kvmppc_pmu_read(struct perf_event *event)
>> {
>> }
>>
>> +/* Return the size of the needed guest state buffer */
>> +static size_t hostwide_get_size(struct kvmppc_gs_msg *gsm)
>> +
>> +{
>> + size_t size = 0;
>> + const u16 ids[] = {
>> + KVMPPC_GSID_L0_GUEST_HEAP,
>> + KVMPPC_GSID_L0_GUEST_HEAP_MAX,
>> + KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE,
>> + KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE_MAX,
>> + KVMPPC_GSID_L0_GUEST_PGTABLE_RECLAIM
>> + };
>> +
>> + for (int i = 0; i < ARRAY_SIZE(ids); i++)
>> + size += kvmppc_gse_total_size(kvmppc_gsid_size(ids[i]));
>> + return size;
>> +}
>> +
>> +/* Populate the request guest state buffer */
>> +static int hostwide_fill_info(struct kvmppc_gs_buff *gsb,
>> + struct kvmppc_gs_msg *gsm)
>> +{
>> + struct kvmppc_hostwide_stats *stats = gsm->data;
>> +
>> + /*
>> + * It doesn't matter what values are put into request buffer as
>> + * they are going to be overwritten anyways. But for the sake of
>> + * testcode and symmetry contents of existing stats are put
>> + * populated into the request guest state buffer.
>> + */
>> + if (kvmppc_gsm_includes(gsm, KVMPPC_GSID_L0_GUEST_HEAP))
>> + kvmppc_gse_put_u64(gsb, KVMPPC_GSID_L0_GUEST_HEAP,
>> + stats->guest_heap);
>> + if (kvmppc_gsm_includes(gsm, KVMPPC_GSID_L0_GUEST_HEAP_MAX))
>> + kvmppc_gse_put_u64(gsb, KVMPPC_GSID_L0_GUEST_HEAP_MAX,
>> + stats->guest_heap_max);
>> + if (kvmppc_gsm_includes(gsm, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE))
>> + kvmppc_gse_put_u64(gsb, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE,
>> + stats->guest_pgtable_size);
>> + if (kvmppc_gsm_includes(gsm, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE_MAX))
>> + kvmppc_gse_put_u64(gsb, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE_MAX,
>> + stats->guest_pgtable_size_max);
>> + if (kvmppc_gsm_includes(gsm, KVMPPC_GSID_L0_GUEST_PGTABLE_RECLAIM))
>> + kvmppc_gse_put_u64(gsb, KVMPPC_GSID_L0_GUEST_PGTABLE_RECLAIM,
>> + stats->guest_pgtable_reclaim);
>> +
>> + return 0;
>> +}
>
> kvmppc_gse_put_u64() can return an error. I think we can handle it just
> like gs_msg_ops_vcpu_fill_info()
>
Good suggestion. Will incorporate that in v2.
>> +
>> +/* Parse and update the host wide stats from returned gsb */
>> +static int hostwide_refresh_info(struct kvmppc_gs_msg *gsm,
>> + struct kvmppc_gs_buff *gsb)
>> +{
>> + struct kvmppc_gs_parser gsp = { 0 };
>> + struct kvmppc_hostwide_stats *stats = gsm->data;
>> + struct kvmppc_gs_elem *gse;
>> + int rc;
>> +
>> + rc = kvmppc_gse_parse(&gsp, gsb);
>> + if (rc < 0)
>> + return rc;
>> +
>> + gse = kvmppc_gsp_lookup(&gsp, KVMPPC_GSID_L0_GUEST_HEAP);
>> + if (gse)
>> + stats->guest_heap = kvmppc_gse_get_u64(gse);
>> +
>> + gse = kvmppc_gsp_lookup(&gsp, KVMPPC_GSID_L0_GUEST_HEAP_MAX);
>> + if (gse)
>> + stats->guest_heap_max = kvmppc_gse_get_u64(gse);
>> +
>> + gse = kvmppc_gsp_lookup(&gsp, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE);
>> + if (gse)
>> + stats->guest_pgtable_size = kvmppc_gse_get_u64(gse);
>> +
>> + gse = kvmppc_gsp_lookup(&gsp, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE_MAX);
>> + if (gse)
>> + stats->guest_pgtable_size_max = kvmppc_gse_get_u64(gse);
>> +
>> + gse = kvmppc_gsp_lookup(&gsp, KVMPPC_GSID_L0_GUEST_PGTABLE_RECLAIM);
>> + if (gse)
>> + stats->guest_pgtable_reclaim = kvmppc_gse_get_u64(gse);
>> +
>> + return 0;
>> +}
>> +
>> +/* gsb-message ops for setting up/parsing */
>> +static struct kvmppc_gs_msg_ops gsb_ops_l0_stats = {
>> + .get_size = hostwide_get_size,
>> + .fill_info = hostwide_fill_info,
>> + .refresh_info = hostwide_refresh_info,
>> +};
>> +
>> +static int kvmppc_init_hostwide(void)
>> +{
>> + int rc = 0;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&lock_l0_stats, flags);
>> +
>> + /* already registered ? */
>> + if (gsm_l0_stats) {
>> + rc = 0;
>> + goto out;
>> + }
>> +
>> + /* setup the Guest state message/buffer to talk to L0 */
>> + gsm_l0_stats = kvmppc_gsm_new(&gsb_ops_l0_stats, &l0_stats,
>> + GSM_SEND, GFP_KERNEL);
>> + if (!gsm_l0_stats) {
>> + rc = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + /* Populate the Idents */
>> + kvmppc_gsm_include(gsm_l0_stats, KVMPPC_GSID_L0_GUEST_HEAP);
>> + kvmppc_gsm_include(gsm_l0_stats, KVMPPC_GSID_L0_GUEST_HEAP_MAX);
>> + kvmppc_gsm_include(gsm_l0_stats, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE);
>> + kvmppc_gsm_include(gsm_l0_stats, KVMPPC_GSID_L0_GUEST_PGTABLE_SIZE_MAX);
>> + kvmppc_gsm_include(gsm_l0_stats, KVMPPC_GSID_L0_GUEST_PGTABLE_RECLAIM);
>> +
>> + /* allocate GSB. Guest/Vcpu Id is ignored */
>> + gsb_l0_stats = kvmppc_gsb_new(kvmppc_gsm_size(gsm_l0_stats), 0, 0,
>> + GFP_KERNEL);
>> + if (!gsb_l0_stats) {
>> + rc = -ENOMEM;
>> + goto out;
>> + }
>> +
>> + /* ask the ops to fill in the info */
>> + rc = kvmppc_gsm_fill_info(gsm_l0_stats, gsb_l0_stats);
>> + if (rc)
>> + goto out;
>> +out:
>> + if (rc) {
>> + if (gsm_l0_stats)
>> + kvmppc_gsm_free(gsm_l0_stats);
>> + if (gsb_l0_stats)
>> + kvmppc_gsb_free(gsb_l0_stats);
>> + gsm_l0_stats = NULL;
>> + gsb_l0_stats = NULL;
>> + }
>> + spin_unlock_irqrestore(&lock_l0_stats, flags);
>> + return rc;
>> +}
>
> The error handling can probably be simplified to avoid multiple ifs:
>
> <snip>
>
> /* allocate GSB. Guest/Vcpu Id is ignored */
> gsb_l0_stats = kvmppc_gsb_new(kvmppc_gsm_size(gsm_l0_stats), 0, 0,
> GFP_KERNEL);
> if (!gsb_l0_stats) {
> rc = -ENOMEM;
> goto err_gsm;
> }
>
> /* ask the ops to fill in the info */
> rc = kvmppc_gsm_fill_info(gsm_l0_stats, gsb_l0_stats);
> if (!rc)
> goto out;
>
> err_gsb:
> kvmppc_gsb_free(gsb_l0_stats);
> gsb_l0_stats = NULL;
>
> err_gsm:
> kvmppc_gsm_free(gsm_l0_stats);
> gsm_l0_stats = NULL;
>
> out:
> spin_unlock_irqrestore(&lock_l0_stats, flags);
> return rc;
> }
>
Thats subjective opinion and I tend to prefer less number of goto jump
labels in the function hence the function is implemented the way it is.
>> +
>> +static void kvmppc_cleanup_hostwide(void)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&lock_l0_stats, flags);
>> +
>> + if (gsm_l0_stats)
>> + kvmppc_gsm_free(gsm_l0_stats);
>> + if (gsb_l0_stats)
>> + kvmppc_gsb_free(gsb_l0_stats);
>> + gsm_l0_stats = NULL;
>> + gsb_l0_stats = NULL;
>> +
>> + spin_unlock_irqrestore(&lock_l0_stats, flags);
>> +}
>> +
>> /* L1 wide counters PMU */
>> static struct pmu kvmppc_pmu = {
>> .task_ctx_nr = perf_sw_context,
>> @@ -108,6 +290,10 @@ int kvmppc_register_pmu(void)
>>
>> /* only support events for nestedv2 right now */
>> if (kvmhv_is_nestedv2()) {
>> + rc = kvmppc_init_hostwide();
>> + if (rc)
>> + goto out;
>> +
>> /* Setup done now register the PMU */
>> pr_info("Registering kvm-hv pmu");
>>
>> @@ -117,6 +303,7 @@ int kvmppc_register_pmu(void)
>> -1) : 0;
>> }
>>
>> +out:
>> return rc;
>> }
>> EXPORT_SYMBOL_GPL(kvmppc_register_pmu);
>> @@ -124,6 +311,8 @@ EXPORT_SYMBOL_GPL(kvmppc_register_pmu);
>> void kvmppc_unregister_pmu(void)
>> {
>> if (kvmhv_is_nestedv2()) {
>> + kvmppc_cleanup_hostwide();
>> +
>> if (kvmppc_pmu.type != -1)
>> perf_pmu_unregister(&kvmppc_pmu);
>>
>> --
>> 2.47.1
>>
--
Cheers
~ Vaibhav
