Re: [PATCH v5 1/8] i386: Add Intel RDT device and State to config.

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Fri, 13 Dec 2024 17:26:38 +0000
Hendrik Wuethrich <whendrik@google.com> wrote:

> From: ‪Hendrik Wüthrich <whendrik@google.com>
> 
> Change config to show RDT, add minimal code to the rdt.c module to make
> sure things still compile.
> 
> Signed-off-by: Hendrik Wüthrich <whendrik@google.com>
Hi,

A few drive by comments.

> ---
>  hw/i386/Kconfig       |  4 ++
>  hw/i386/meson.build   |  1 +
>  hw/i386/rdt.c         | 99 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/rdt.h | 25 +++++++++++
>  target/i386/cpu.h     |  3 ++
>  5 files changed, 132 insertions(+)
>  create mode 100644 hw/i386/rdt.c
>  create mode 100644 include/hw/i386/rdt.h

> diff --git a/hw/i386/rdt.c b/hw/i386/rdt.c
> new file mode 100644
> index 0000000000..b2203197e3
> --- /dev/null
> +++ b/hw/i386/rdt.c
> @@ -0,0 +1,99 @@
> +/*
> + * Intel Resource Director Technology (RDT).
> + *
> + * Copyright 2024 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "hw/i386/rdt.h"
> +#include "qemu/osdep.h" /* Needs to be included before isa.h */
> +#include "hw/isa/isa.h"
> +#include "hw/qdev-properties.h"
> +#include "qom/object.h"
> +
> +/* Max counts for allocation masks or CBMs. In other words, the size of
> + * respective MSRs.
> + * L3_MASK and L3_mask are architectural limitations. THRTL_COUNT is just
> + * the space left until the next MSR.
> + * */


Should match multiline comment style in qemu style guide.

> +
> +/*One instance of RDT-internal state to be shared by all cores*/
> +struct RDTState {
> +    ISADevice parent;
> +
> +    /*Max amount of RMIDs*/

Spaces typically after * and before * I think are most common
syntax for comments in qEMU

> +    uint32_t rmids;
> +
> +    /*Per core state*/
> +    RDTStatePerCore *rdtInstances;
> +    RDTAllocation *allocations;
> +
> +    /*RDT Allocation bitmask MSRs*/
> +    uint32_t msr_L3_ia32_mask_n[RDT_MAX_L3_MASK_COUNT];
> +    uint32_t msr_L2_ia32_mask_n[RDT_MAX_L2_MASK_COUNT];
> +    uint32_t ia32_L2_qos_ext_bw_thrtl_n[RDT_MAX_MBA_THRTL_COUNT];
> +};
> +
> +struct RDTStateClass {
> +};
> +
> +OBJECT_DEFINE_TYPE(RDTState, rdt, RDT, ISA_DEVICE);
> +
> +static Property rdt_properties[] = {
> +    DEFINE_PROP_UINT32(RDT_NUM_RMID_PROP, RDTState, rmids, 256),
> +    DEFINE_PROP_END_OF_LIST(),

You'll see this in a rebase but this terminator is no longer needed
(or defined I think)
> +};
> +
> +static void rdt_init(Object *obj)
> +{
> +}
> +
> +static void rdt_finalize(Object *obj)
> +{
> +}

Why introduce this pair as empty and as far as I can see not called?
init is called in patch 2 so bring it in there.  I'm struggling to
spot finalize being called.


> +
> +static void rdt_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->hotpluggable = false;
> +    dc->desc = "RDT";
> +    dc->user_creatable = true;
> +
> +    device_class_set_props(dc, rdt_properties);
> +}

>      int32_t node_id; /* NUMA node this CPU belongs to */