Re: [PATCH for 4.6 07/13] xen: Introduce a generic way to describe device

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 17/12/14 10:46, Jan Beulich wrote:
>>>> On 17.12.14 at 11:30, <julien.grall@linaro.org> wrote:
>> On 17/12/2014 10:16, Jan Beulich wrote:
>>>>>> On 16.12.14 at 21:08, <julien.grall@linaro.org> wrote:
>>>> --- a/xen/common/Makefile
>>>> +++ b/xen/common/Makefile
>>>> @@ -2,6 +2,7 @@ obj-y += bitmap.o
>>>>   obj-y += core_parking.o
>>>>   obj-y += cpu.o
>>>>   obj-y += cpupool.o
>>>> +obj-y += device.o
>>>
>>> Shouldn't this instead be two lines, one using HAS_PCI and the second
>>> HAS_DEVICE_TREE?
>>
>> When ARM will gain PCI will, it will fail to compile because device.o is 
>> included twice.
> 
> Not necessarily: If we don't do this already, we should eliminate
> duplicates from $(obj-y) just like Linux does.

I will give a look.

>>>> @@ -75,8 +76,19 @@ struct pci_dev {
>>>>   #define PT_FAULT_THRESHOLD 10
>>>>       } fault;
>>>>       u64 vf_rlen[6];
>>>> +
>>>> +    struct device dev;
>>>>   };
>>>
>>> I'm not convinced yet that growing this structure (of which we have
>>> quite many instances on some systems) is really worth it, in particular
>>> on x86 where we (so far) only have one device type anyway.
>>
>> Actually this will growing by only sizeof (enum type) on x86.
> 
> No, by 8 bytes (due to padding).

>> Having a generic way to describe device will really help ARM code (see 
>> IOMMU).
>>
>> If we don't have a such thing, we may need to duplicate quite a lots of 
>> code. Which will make hard to maintain.
> 
> Not really, if e.g. "device" was simply an alias of "pci_dev" on x86.

How many pci_dev instance you could have on a platform? 1000? Though it
might be a high value but that mean we use 2k more of RAM.

It doesn't seem to bad for the benefit to have a clear code.

>>>> +#define pci_to_dev(pcidev)  (&(pcidev)->dev)
>>>> +
>>>> +static inline struct pci_dev *dev_to_pci(struct device *dev)
>>>> +{
>>>> +    ASSERT(dev->type == DEV_PCI);
>>>> +
>>>> +    return container_of(dev, struct pci_dev, dev);
>>>> +}
>>>
>>> While the former is const-correct, I dislike the inability of passing
>>> pointers to const into helper functions like the latter. I can't think
>>> of a good solution other than introducing a second const variant
>>> of it, but I suppose we should try to find alternatives before
>>> adding such a construct that moves us in a direction opposite to
>>> getting our code more const-correct.
>>
>> Oh right. I didn't though about that case. I will turn this inline 
>> function into a macro.
> 
> I'm afraid that won't help, as you still need to specify a type as
> 2nd argument to container_of(), and that type can't be both
> const and non-const at the same time, i.e. you can't easily
> inherit the const-ness of the passed in pointer.

I agree that we will drop the const-ness. But is it really an issue?

We won't have many place where we don't want to modify the pci_dev.

Regards,

-- 
Julien Grall