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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 {

>         union {
>             struct {
>                 uint8_t  func : 3, /* Function */
>                          slot : 5; /* Device   */

dev or device, surely?

>             };
>             uint8_t      extfunc;  /* Extended function */
>         };
>         uint8_t          bus;      /* Bus      */

    };
};

>         uint16_t         seg;      /* Segment  */
>     };
> } pci_sbdf_t;

~Andrew

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