On Fri, 13 Dec 2024 17:26:38 +0000
Hendrik Wuethrich <whend...@google.com> wrote:
> From: Hendrik Wüthrich <whend...@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 <whend...@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 */
