Re: [PATCH] xen: Don't cast away const-ness in vcpu_show_registers()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >> CC: Michal Orzel <michal.orzel@xxxxxxx>
> >> CC: Jan Beulich <jbeulich@xxxxxxxx>
> >> CC: Julien Grall <julien@xxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> >> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> >>
> >> 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 <jbeulich@xxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> 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.