Il sab 7 dic 2024, 16:38 Zhao Liu <zhao1....@intel.com> ha scritto:

> Thanks for pointing that out, I really didn't think of that, I
> understand how that would break the atomicity of the BQL lock, right?
>

Yes, but also the function seems unnecessary.

> impl fw_cfg_config {
> >     pub(crate) fn assign_hpet_id() -> usize {
> >         assert!(bql_locked());
> >         // SAFETY: all accesses go through these methods, which guarantee

>         // that the accesses are protected by the BQL.
> >         let fw_cfg = unsafe { &mut *hpet_fw_cfg };
>
> Nice idea!
>
> >         if self.count == u8::MAX {
> >             // first instance
> >             fw_cfg.count = 0;
> >         }
>
> Will something like “anything that releases bql_lock” happen here?


No, there are no function calls even.

There seems to be no atomicity guarantee here.
>

It's not beautiful but it's guaranteed to be atomic. For the rare case of
static mut, which is unsafe anyway, it makes sense.

>
> >         if fw_cfg.count == 8 {
> >             // TODO: Add error binding: error_setg()
> >             panic!("Only 8 instances of HPET is allowed");
> >         }
> >
> >         let id: usize = fw_cfg.count.into();
> >         fw_cfg.count += 1;
> >         id
> >     }
> > }
> >
> > and you can assert bql_locked by hand instead of using the BqlCell.
>
> Thanks! I can also add a line of doc for bql_locked that it can be used
> directly without BqlCell if necessary.
>

Good idea!

And if you also agree the Phillipe's idea, I also need to add BqlCell
> for fw_cfg field in HPETClass :-).
>

No, that also breaks compilation with CONFIG_HPET=n. The idea is nice but
it doesn't work. ¯⁠\⁠_⁠(⁠ツ⁠)⁠_⁠/⁠¯

Paolo


> Regards,
> Zhao
>
>
>

Reply via email to