| Message ID | 20250210030051.2562726-2-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Add HPET timer device | expand |
On Mon, Feb 10, 2025 at 3:41 AM Zhao Liu <zhao1.liu@intel.com> wrote: > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c > index d2cb08715a21..546de63123e6 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 must be kept for the case where HPET is not enabled at all in the build; removing the FW_CFG_HPET file changes the guest API and I'd prefer to merge the Rust HPET implementation without having to figure out the safety of that change. No need to do anything, I'll just make it #if !defined(CONFIG_HPET) && !defined(CONFIG_X_HPET_RUST) const struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX}; #endif Paolo
On Thu, Feb 13, 2025 at 12:25:55PM +0100, Paolo Bonzini wrote: > Date: Thu, 13 Feb 2025 12:25:55 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH v2 01/10] i386/fw_cfg: move hpet_cfg definition to > hpet.c > > On Mon, Feb 10, 2025 at 3:41 AM Zhao Liu <zhao1.liu@intel.com> wrote: > > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c > > index d2cb08715a21..546de63123e6 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 must be kept for the case where HPET is not enabled at all in the > build; removing the FW_CFG_HPET file changes the guest API and I'd > prefer to merge the Rust HPET implementation without having to figure > out the safety of that change. > > No need to do anything, I'll just make it > > #if !defined(CONFIG_HPET) && !defined(CONFIG_X_HPET_RUST) > const struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX}; > #endif > Thanks! This makes sense. Zhao
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c index d2cb08715a21..546de63123e6 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}; - const char *fw_cfg_arch_key_name(uint16_t key) { static const struct { @@ -153,7 +151,7 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms, PCMachineState *pcms = (PCMachineState *)object_dynamic_cast(OBJECT(ms), TYPE_PC_MACHINE); if (pcms && pcms->hpet_enabled) { - fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg)); + fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_fw_cfg, sizeof(hpet_fw_cfg)); } #endif diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c index 92b6d509edda..f2e2d83a3f67 100644 --- a/hw/timer/hpet.c +++ b/hw/timer/hpet.c @@ -40,6 +40,8 @@ #include "qom/object.h" #include "trace.h" +struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX}; + #define HPET_MSI_SUPPORT 0 OBJECT_DECLARE_SIMPLE_TYPE(HPETState, HPET) @@ -655,8 +657,8 @@ static void hpet_reset(DeviceState *d) s->hpet_counter = 0ULL; s->hpet_offset = 0ULL; s->config = 0ULL; - hpet_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability; - hpet_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr; + hpet_fw_cfg.hpet[s->hpet_id].event_timer_block_id = (uint32_t)s->capability; + hpet_fw_cfg.hpet[s->hpet_id].address = sbd->mmio[0].addr; /* to document that the RTC lowers its output on reset as well */ s->rtc_irq_level = 0; @@ -698,17 +700,17 @@ static void hpet_realize(DeviceState *dev, Error **errp) if (!s->intcap) { warn_report("Hpet's intcap not initialized"); } - if (hpet_cfg.count == UINT8_MAX) { + if (hpet_fw_cfg.count == UINT8_MAX) { /* first instance */ - hpet_cfg.count = 0; + hpet_fw_cfg.count = 0; } - if (hpet_cfg.count == 8) { + if (hpet_fw_cfg.count == 8) { error_setg(errp, "Only 8 instances of HPET is allowed"); return; } - s->hpet_id = hpet_cfg.count++; + s->hpet_id = hpet_fw_cfg.count++; for (i = 0; i < HPET_NUM_IRQ_ROUTES; i++) { sysbus_init_irq(sbd, &s->irqs[i]); diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h index 71e8c62453d1..c2656f7f0bef 100644 --- a/include/hw/timer/hpet.h +++ b/include/hw/timer/hpet.h @@ -73,7 +73,7 @@ struct hpet_fw_config struct hpet_fw_entry hpet[8]; } QEMU_PACKED; -extern struct hpet_fw_config hpet_cfg; +extern struct hpet_fw_config hpet_fw_cfg; #define TYPE_HPET "hpet"
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 | 14 ++++++++------ include/hw/timer/hpet.h | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-)