Re: [PATCH v1 1/9] Add Intel RDT device to config.

[Jonathan Cameron]

On Fri, 19 Jul 2024 16:29:21 +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 Hendrik

Great to see emulation of this. Will be handy for testing
kernel changes etc.

Not convinced it's worth a separate patch just to add stubs.
Why not at least bring some real code in with this?

> ---
>  hw/i386/Kconfig       |  4 ++++
>  hw/i386/meson.build   |  1 +
>  hw/i386/rdt.c         | 49 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/rdt.h | 12 +++++++++++
>  4 files changed, 66 insertions(+)
>  create mode 100644 hw/i386/rdt.c
>  create mode 100644 include/hw/i386/rdt.h
> 
> diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
> index f4a33b6c08..4dd05ed6f2 100644
> --- a/hw/i386/Kconfig
> +++ b/hw/i386/Kconfig
> @@ -10,6 +10,9 @@ config SGX
>      bool
>      depends on KVM
>  
> +config RDT
> +    bool
> +
>  config PC
>      bool
>      imply APPLESMC
> @@ -26,6 +29,7 @@ config PC
>      imply QXL
>      imply SEV
>      imply SGX
> +    imply RDT
>      imply TEST_DEVICES
>      imply TPM_CRB
>      imply TPM_TIS_ISA
> diff --git a/hw/i386/meson.build b/hw/i386/meson.build
> index 03aad10df7..fdbf5962b5 100644
> --- a/hw/i386/meson.build
> +++ b/hw/i386/meson.build
> @@ -21,6 +21,7 @@ i386_ss.add(when: 'CONFIG_VMPORT', if_true: 
> files('vmport.c'))
>  i386_ss.add(when: 'CONFIG_VTD', if_true: files('intel_iommu.c'))
>  i386_ss.add(when: 'CONFIG_SGX', if_true: files('sgx-epc.c','sgx.c'),
>                                  if_false: files('sgx-stub.c'))
> +i386_ss.add(when: 'CONFIG_RDT', if_true: files('rdt.c'))
>  
>  i386_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-common.c'))
>  i386_ss.add(when: 'CONFIG_PC', if_true: files(
> diff --git a/hw/i386/rdt.c b/hw/i386/rdt.c
> new file mode 100644
> index 0000000000..0a5e95606b
> --- /dev/null
> +++ b/hw/i386/rdt.c

License etc missing.

> @@ -0,0 +1,49 @@
> +#include "qemu/osdep.h"
> +#include "hw/i386/rdt.h"
> +#include <stdint.h>
> +#include "hw/qdev-properties.h"
> +#include "qemu/typedefs.h"
> +#include "qom/object.h"
> +#include "target/i386/cpu.h"
> +#include "hw/isa/isa.h"

Ordering seems a bit random.  I don't really mind what order
they are in but it is easier to pick an option so it
becomes obvious where to put things later.

Also better to bring these in when they are needed so it
is obvious why they are here.


> +
> +#define TYPE_RDT "rdt"
> +
> +OBJECT_DECLARE_TYPE(RDTState, RDTStateClass, RDT);
> +
> +struct RDTState {
> +    ISADevice parent;
> +};
> +
> +struct RDTStateClass { };
I'd do
...Class {
};

As will reduce noise in later patches assuming this will have
content.


> +
> +OBJECT_DEFINE_TYPE(RDTState, rdt, RDT, ISA_DEVICE);
> +
> +static Property rdt_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void rdt_init(Object *obj)

Not used?

> +{
> +}
> +
> +static void rdt_realize(DeviceState *dev, Error **errp)
> +{
> +}
> +
> +static void rdt_finalize(Object *obj)
> +{
> +}

Not used?

> +
> +static void rdt_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->hotpluggable = false;
> +    dc->desc = "RDT";
> +    dc->user_creatable = true;
> +    dc->realize = rdt_realize;
> +
> +    device_class_set_props(dc, rdt_properties);
> +}
> +
> diff --git a/include/hw/i386/rdt.h b/include/hw/i386/rdt.h
> new file mode 100644
> index 0000000000..45e34d3103
> --- /dev/null
> +++ b/include/hw/i386/rdt.h
> @@ -0,0 +1,12 @@
> +#ifndef HW_RDT_H
> +#define HW_RDT_H
> +
> +#include <stdbool.h>
> +#include <stdint.h>

Not used so don't include them until needed.

> +
> +typedef struct RDTState RDTState;
> +typedef struct RDTStateInstance RDTStateInstance;
> +typedef struct RDTMonitor RDTMonitor;
> +typedef struct RDTAllocation RDTAllocation;
> +
> +#endif