Re: [Xen-devel] [PATCH] pci: constify domain parameter of pci_get_pdev_by_domain

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.09.17 at 20:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 08/09/17 18:33, Roger Pau Monné wrote:
>> On Fri, Sep 08, 2017 at 10:00:41AM -0600, Jan Beulich wrote:
>>>>>> On 08.09.17 at 16:30, <roger.pau@xxxxxxxxxx> wrote:
>>>> On Fri, Sep 08, 2017 at 03:15:40PM +0100, Andrew Cooper wrote:
>>>>> On 08/09/17 14:34, Roger Pau Monne wrote:
>>>>>> While there fix the indentation.
>>>>>>
>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>>> ---
>>>>>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>>>>>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>>> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>>>>>> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
>>>>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>>>>> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>>>>>> Cc: Tim Deegan <tim@xxxxxxx>
>>>>>> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
>>>>>> ---
>>>>>>  xen/drivers/passthrough/pci.c | 4 ++--
>>>>>>  xen/include/xen/pci.h         | 4 ++--
>>>>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/xen/drivers/passthrough/pci.c 
>>>>>> b/xen/drivers/passthrough/pci.c
>>>>>> index 74829e5748..469dfc6c3d 100644
>>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>>> @@ -532,8 +532,8 @@ struct pci_dev *pci_get_real_pdev(int seg, int bus, 
>>>>>> int 
>>>> devfn)
>>>>>>      return pdev;
>>>>>>  }
>>>>>>  
>>>>>> -struct pci_dev *pci_get_pdev_by_domain(
>>>>>> -    struct domain *d, int seg, int bus, int devfn)
>>>>>> +struct pci_dev *pci_get_pdev_by_domain(const struct domain *d, int seg,
>>>>>> +                                       int bus, int devfn)
>>>>> I know this isn't strictly related to the patch, but having 3 register
>>>>> parameters (and load of places elsewhere) here is extremely wasteful for
>>>>> register scheduling.  Could we introduce:
>>>>>
>>>>> typedef union {
>>>>>     uint32_t sbdf;
>>>>>     struct {
>>>>>         uint16_t  f : 3,  /* Function */
>>>>>                   d : 5,  /* Device   */
>>>>>                   b : 8;  /* Bus      */
>>>>>         uint16_t s; /* Segment  */
>>>>>     };
>>>>> } pci_sbdf_t;
>>>>>
>>>>> and try to start using it?
>>>> This is going to be kind of awkward to use with extended
>>>> functions...
>>>>
>>>> It's not like what we have now (devfn) is much better, but if we
>>>> switch to something else I would like to be able to get the correct
>>>> function value for both normal and extended function devices from the
>>>> same field.
>>> Why not unionize f and d with a uint8_t extfn (I'd also prefer the
>>> other two to be named func and dev)?
>> So something like:
> 
> As as bdf is also quite a common unit, how about:
> 
>>
>> typedef union {
>>     uint32_t sbdf;
>>     struct {
> 
> union {
>     uint16_t bdf;
>     struct {

Yes.

>>         union {
>>             struct {
>>                 uint8_t  func : 3, /* Function */
>>                          slot : 5; /* Device   */
> 
> dev or device, surely?

Yeah, comment and field name would better match (or the comments
could perhaps go away altogether). While "slot" is a common term
here, with this being inside something called "sbdf" I agree "device"
or "dev" (in the former case it should also be "function", but I prefer
the shorter variants) should be used here.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel