On Thu, Feb 20, 2025 at 4:41 PM Jonathan Cameron
<jonathan.came...@huawei.com> wrote:
>
> 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.
Because the init and finalize functions are needed by the macros
representing the RDT device, and the patch does not compile
if they are removed.
>
>
> > +
> > +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 */
>
