Re: [PATCH v1 04/29] xen/asm-generic: introduce stub header device.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.10.2023 12:57, Julien Grall wrote:
> On 19/10/2023 11:53, Jan Beulich wrote:
>> On 19.10.2023 12:42, Julien Grall wrote:
>>> On 19/10/2023 10:14, Jan Beulich wrote:
>>>> On 14.09.2023 16:56, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/include/asm-generic/device.h
>>>>> @@ -0,0 +1,65 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>>> +#ifndef __ASM_GENERIC_DEVICE_H__
>>>>> +#define __ASM_GENERIC_DEVICE_H__
>>>>> +
>>>>> +struct dt_device_node;
>>>>> +
>>>>> +enum device_type
>>>>> +{
>>>>> +    DEV_DT,
>>>>> +    DEV_PCI,
>>>>> +};
>>>>
>>>> Are both of these really generic?
>>>
>>> I think can be re-used for RISC-V to have an abstract view a device.
>>> This is for instance used in the IOMMU code where both PCI and platform
>>> (here called DT) can be assigned to a domain. The driver will need to
>>> know the difference, but the common layer doesn't need to.
>>
>> Question to me is whether DT and PCI can be considered "common", which
>> is a prereq for being used here.
> 
> I think it can. See more below.
> 
>>
>>>>> +struct device {
>>>>> +    enum device_type type;
>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>> +    struct dt_device_node *of_node; /* Used by drivers imported from 
>>>>> Linux */
>>>>> +#endif
>>>>> +};
>>>>> +
>>>>> +enum device_class
>>>>> +{
>>>>> +    DEVICE_SERIAL,
>>>>> +    DEVICE_IOMMU,
>>>>> +    DEVICE_GIC,
>>>>
>>>> This one certainly is Arm-specific.
>>>
>>> This could be renamed to DEVICE_IC (or INTERRUPT_CONTROLLER)
>>>
>>>>
>>>>> +    DEVICE_PCI_HOSTBRIDGE,
>>>>
>>>> And this one's PCI-specific.
>>>
>>> Are you suggesting to #ifdef it? If so, I don't exactly see the value here.
>>
>> What to do with it is secondary to me. I was questioning its presence here.
>>
>>>> Overall same question as before: Are you expecting that RISC-V is going to
>>>> get away without a customized header? I wouldn't think so.
>>>
>>> I think it can be useful. Most likely you will have multiple drivers for
>>> a class and you may want to initialize certain device class early than
>>> others. See how it is used in device_init().
>>
>> I'm afraid I don't see how your reply relates to the question of such a
>> fallback header being sensible to have, or whether instead RISC-V will
>> need its own private header anyway.
> 
> My point is that RISC-V will most likely duplicate what Arm did (they 
> are already copying the dom0less code). So the header would end up to be 
> duplicated. This is not ideal and therefore we want to share the header.
> 
> I don't particularly care whether it lives in asm-generic or somewhere. 
> I just want to avoid the duplication.

Avoiding duplication is one goal, which I certainly appreciate. The header
as presented here is, however, only a subset of Arm's if I'm not mistaken.
If moving all of Arm's code here, I then wonder whether that really can
count as "generic".

Avoiding duplication could e.g. be achieved by making RISC-V symlink Arm's
header.

Jan