On Mon, 3 Mar 2025, Andrew Cooper wrote: > On 26/02/2025 7:33 am, Jan Beulich wrote: > > On 26.02.2025 00:02, Andrew Cooper wrote: > >> The final hunk is `(struct vcpu *)v` in disguise, expressed using a runtime > >> pointer chase through memory and a technicality of the C type system to > >> work > >> around the fact that get_hvm_registers() strictly requires a mutable > >> pointer. > >> > >> For anyone interested, this is one reason why C cannot optimise any reads > >> across sequence points, even for a function purporting to take a const > >> object. > >> > >> Anyway, have the function correctly state that it needs a mutable vcpu. > >> All > >> callers have a mutable vCPU to hand, and it removes the runtime pointer > >> chase > >> in x86. > >> > >> Make one style adjustment in ARM while adjusting the parameter type. > >> > >> Signed-off-by: Andrew Cooper <[email protected]> > >> --- > >> CC: Anthony PERARD <[email protected]> > >> CC: Michal Orzel <[email protected]> > >> CC: Jan Beulich <[email protected]> > >> CC: Julien Grall <[email protected]> > >> CC: Roger Pau Monné <[email protected]> > >> CC: Stefano Stabellini <[email protected]> > >> CC: Volodymyr Babchuk <[email protected]> > >> CC: Bertrand Marquis <[email protected]> > >> > >> RISC-V and PPC don't have this helper yet, not even in stub form. > >> > >> I expect there will be one objection to this patch. Since the last time we > >> fought over this, speculative vulnerabilities have demonstrated how > >> dangerous > >> pointer chases are, and this is a violation of Misra Rule 11.8, even if > >> it's > >> not reasonable for Eclair to be able to spot and reject it. > > On these grounds > > Acked-by: Jan Beulich <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]> > Thanks. > > > irrespective of the fact that a function of this name and purpose really, > > really > > should be taking a pointer-to-const. > > No - this is a perfect example of why most functions should *not* take > pointer-to-const for complex objects. > > There is no such thing as an actually-const vcpu or domain; they are all > mutable. The reason why x86 needs a strictly-mutable pointer is because > it needs to take a spinlock to negotiate for access to a hardware > resource to read some of the registers it needs. > > This is where there is a semantic gap between "logically doesn't modify" > and what the C keyword means. > > Anything except the-most-trivial trivial predates may reasonably need to > take a spinlock or some other safety primitive, even just to read > information. > > > Because this was gratuitously const in the first place, bad code was put > in place of making the prototype match reality. > > This demonstrates a bigger failing in how code is reviewed and > maintained. It is far too frequent that requests to const things don't > even compile. It is also far too frequent that requests to const things > haven't read the full patch series to realise why not. Both of these > are a source of friction during review. > > But more than that, even if something could technically be const right > now, the request to do so forces churn into a future patch to de-const > it in order to make a clean change. And for contributors who aren't > comfortable saying a firm no to a maintainer, this turns into a bad hack > instead. > > i.e. requests to const accessors for complexity objects are making Xen > worse, not better, and we should stop doing it. In general, functions like vcpu_show_registers, which have a clear usage pattern, would seem to be ideal candidates for using const parameters. However, as Andrew explained, when a function takes a struct vcpu as a parameter, complexities often arise. This example with the spinlock is a good way to explain the underlying risks. I don't know whether any general conclusions can be drawn from this example, but in this case Andrew's reasoning is correct.
