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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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.

> 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?
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"?

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