Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c

[Zhao Liu <zhao1.liu@intel.com>]

Hi Paolo,

Sorry for late reply,

On Thu, Dec 05, 2024 at 04:30:15PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 16:30:15 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 09/13] i386/fw_cfg: move hpet_cfg definition to hpet.c
> 
> On 12/5/24 07:07, Zhao Liu wrote:
> > HPET device needs to access and update hpet_cfg variable, but now it is
> > defined in hw/i386/fw_cfg.c and Rust code can't access it.
> > 
> > Move hpet_cfg definition to hpet.c (and rename it to hpet_fw_cfg). This
> > allows Rust HPET device implements its own global hpet_fw_cfg variable,
> > and will further reduce the use of unsafe C code access and calls in the
> > Rust HPET implementation.
> > 
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/i386/fw_cfg.c        |  4 +---
> >   hw/timer/hpet.c         | 16 +++++++++-------
> >   include/hw/timer/hpet.h |  2 +-
> >   3 files changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> > index 0e4494627c21..965e6306838a 100644
> > --- a/hw/i386/fw_cfg.c
> > +++ b/hw/i386/fw_cfg.c
> > @@ -26,8 +26,6 @@
> >   #include CONFIG_DEVICES
> >   #include "target/i386/cpu.h"
> > -struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
> 
> This breaks if you disable HPET, which is why fw_cfg.c defines it.

Thanks! I did miss this case.

> You can do something like
> 
> diff --git a/include/hw/timer/hpet-fw-cfg.h b/include/hw/timer/hpet-fw-cfg.h
> new file mode 100644
> index 00000000000..234a49fc92e
> --- /dev/null
> +++ b/include/hw/timer/hpet-fw-cfg.h
> @@ -0,0 +1,16 @@
> +struct hpet_fw_entry
> +{
> +    uint32_t event_timer_block_id;
> +    uint64_t address;
> +    uint16_t min_tick;
> +    uint8_t page_prot;
> +} QEMU_PACKED;
> +
> +struct hpet_fw_config
> +{
> +    uint8_t count;
> +    struct hpet_fw_entry hpet[8];
> +} QEMU_PACKED;
> +
> +extern struct hpet_fw_config hpet_fw_cfg;
> +
> diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
> index d17a8d43199..6f7fcbc3c60 100644
> --- a/include/hw/timer/hpet.h
> +++ b/include/hw/timer/hpet.h
> @@ -60,26 +60,12 @@
>  #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
>  #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
> -struct hpet_fw_entry
> -{
> -    uint32_t event_timer_block_id;
> -    uint64_t address;
> -    uint16_t min_tick;
> -    uint8_t page_prot;
> -} QEMU_PACKED;
> -
> -struct hpet_fw_config
> -{
> -    uint8_t count;
> -    struct hpet_fw_entry hpet[8];
> -} QEMU_PACKED;
> -
> -extern struct hpet_fw_config hpet_cfg;
> -
>  #define TYPE_HPET "hpet"
>  #define HPET_INTCAP "hpet-intcap"
> +#include "hw/timer/hpet-fw-cfg.h"
> +
>  static inline bool hpet_find(void)
>  {
>      return object_resolve_path_type("", TYPE_HPET, NULL);
> diff --git a/rust/wrapper.h b/rust/wrapper.h
> index 285d0eb6ad0..82381e43472 100644
> --- a/rust/wrapper.h
> +++ b/rust/wrapper.h
> @@ -62,3 +62,4 @@ typedef enum memory_order {
>  #include "qapi/error.h"
>  #include "migration/vmstate.h"
>  #include "chardev/char-serial.h"
> +#include "hw/timer/hpet-fw-cfg.h"
> 

Thank you very much for this example!

> but you will have to use unsafe to access it since it's a "static mut".

I also noticed Philippe's version. And I prefer Phillippe's version,
although his version requires more changes to the rust version, it
provides an opportunity to add more field to the QOM class, so I feel
it's good to check current rust qom class support.

Regrads,
Zhao