Re: [PATCH v1 2/9] Add state for RDT device.

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

On Fri, 19 Jul 2024 16:29:22 +0000
Hendrik Wuethrich <whendrik@google.com> wrote:

> From: ‪Hendrik Wüthrich <whendrik@google.com>
> 
> Add structures and variables needed to emulate Intel RDT, including
> module-internal sturctures and state in ArchCPU. No functionality yet.
> 
> Signed-off-by: Hendrik Wüthrich <whendrik@google.com>
A few general comments inline.

J
> ---
>  hw/i386/rdt.c     | 33 +++++++++++++++++++++++++++++++++
>  target/i386/cpu.h |  5 +++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/hw/i386/rdt.c b/hw/i386/rdt.c
> index 0a5e95606b..cf246ab835 100644
> --- a/hw/i386/rdt.c
> +++ b/hw/i386/rdt.c
> @@ -7,12 +7,44 @@
>  #include "target/i386/cpu.h"
>  #include "hw/isa/isa.h"
>  
> +/* Max counts for allocation masks or CBMs. In other words, the size of respective MSRs*/
> +#define MAX_L3_MASK_COUNT      128

If these are an architectural limitation rather than a qemu one good
to have a reference. If it's an RDT requirement namespace them with
RDT_*

> +#define MAX_L2_MASK_COUNT      48
> +#define MAX_MBA_THRTL_COUNT    31
> +
>  #define TYPE_RDT "rdt"
> +#define RDT_NUM_RMID_PROP "rmids"
>  
>  OBJECT_DECLARE_TYPE(RDTState, RDTStateClass, RDT);
>  
> +struct RDTMonitor {
> +    uint64_t count_local;
> +    uint64_t count_remote;
> +    uint64_t count_l3;
> +};
> +
> +struct RDTAllocation {
> +    uint32_t active_cos;
> +};
> +
> +struct RDTStateInstance {

I'd add some docs for this and RDTState to make it obvious
what the scope of each is.  What do they represent and how
is it different?
This seems like it might be the per core part.  If so can
you name it to make that obvious?  RDTPerCoreState or
something like that?

> +    uint32_t active_rmid;
> +    GArray *monitors;
> +
> +    RDTState *rdtstate;
> +};
> +
>  struct RDTState {
>      ISADevice parent;
> +
> +    uint32_t rmids;
> +
> +    GArray *rdtInstances;

Whilst naming makes it likely I'd add a comment to say this
an array of RDTStateInstance with (I think) one per core?

> +    GArray *allocations;
> +
> +    uint32_t msr_L3_ia32_mask_n[MAX_L3_MASK_COUNT];
> +    uint32_t msr_L2_ia32_mask_n[MAX_L2_MASK_COUNT];
> +    uint32_t ia32_L2_qos_ext_bw_thrtl_n[MAX_MBA_THRTL_COUNT];
>  };
>  
>  struct RDTStateClass { };
> @@ -20,6 +52,7 @@ 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(),
>  };
>  
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 1e121acef5..bd0bbb75f2 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1987,6 +1987,8 @@ typedef struct CPUArchState {
>  
>  struct kvm_msrs;
>  
> +struct RDTState;

Not used?

> +struct rdtStateInstance;
>  /**
>   * X86CPU:
>   * @env: #CPUX86State
> @@ -2143,6 +2145,9 @@ struct ArchCPU {
>      struct MemoryRegion *cpu_as_root, *cpu_as_mem, *smram;
>      Notifier machine_done;
>  
> +    /* Help the RDT MSRs find the RDT device */
> +    struct RDTStateInstance *rdt;
> +
>      struct kvm_msrs *kvm_msr_buf;
>  
>      int32_t node_id; /* NUMA node this CPU belongs to */