Re: [PATCH v1 0/3] vPCI capabilities filtering

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.08.2023 23:11, Stewart Hildebrand wrote:
> On 8/14/23 09:59, Jan Beulich wrote:
>> On 10.08.2023 21:12, Stewart Hildebrand wrote:
>>> This small series enables vPCI to filter which PCI capabilites we expose to 
>>> a
>>> domU. This series adds vPCI register handlers within
>>> xen/drivers/vpci/header.c:init_bars(), along with some supporting functions.
>>>
>>> Note there are minor rebase conflicts with the in-progress vPCI series [1].
>>> These conflicts fall into the category of functions and code being added
>>> adjacent to one another, so are easily resolved. I did not identify any
>>> dependency on the vPCI locking work, and the two series deal with different
>>> aspects of emulating the PCI header.
>>>
>>> Future work may involve adding handlers for more registers in the vPCI 
>>> header,
>>> such as STATUS, VID/DID, etc.
>>>
>>> [1] 
>>> https://lists.xenproject.org/archives/html/xen-devel/2023-07/msg01281.html
>>>
>>> Stewart Hildebrand (3):
>>>   xen/vpci: add vpci_hw_read8 helper
>>>   xen/vpci: add vpci_read_val helper
>>>   xen/vpci: header: filter PCI capabilities
>>
>> I'm not convinced of the split here: Seeing the new helpers in isolation
>> leaves entirely open what they're to be used for.
> 
> Our code review guide [2] (section "General Patterns") explicitly suggests 
> separating independent helper functions into (a) separate patch(es). Whether 
> it is one patch per helper, or all helpers in a single patch appears 
> ambiguous.
> That said, I'd still be happy to squash all these into a single patch to 
> avoid the transient dead code situation - please confirm.

I'm not the maintainer of this code, so my confirmation is of limited
value, but yes, in the case here I'm of the clear opinion that
separating out the helper functions is not helpful. Not doing so will
then also ...

>> Plus besides introducing
>> dead code (even if only transiently), you also introduce cf_check marked
>> code which isn't really called indirectly from anywhere. Yet we'd like to
>> keep the amount of these markings down (in the final binary, not so much
>> in source code).
> 
> The helper functions will be added to struct vpci_register objects, where 
> they will be called from vpci.c:vpci_read():
> 
>     val = r->read(pdev, r->offset, r->private);
> 
> Does this justify the cf_check attribute?

It not only justifies the attribute, it actually demands it, yes. By
the time these use sites appear.

> If so, should the cf_check attributes rather be added once the helpers are 
> actually used in the patch "xen/vpci: header: filter PCI capabilities"?

... deal with this, by rendering remark and question void.

> [2] http://xenbits.xenproject.org/governance/code-review-guide.html

I'll try to remember to add a topic to the next Community Call's
agenda. Actively suggesting to (even if just transiently) introduce
dead code is in direct conflict with the Misra work. Plus, if such
helpers were static, it also suggests actively breaking the build,
again just transiently of course.

Jan