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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2025 17:52, 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>
> 
> 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.

And hence (in part) why C++ gained "mutable" ages ago.

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

Okay, let's agree that we don't agree in our perspectives here.

Jan