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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.10.2023 13:07, Julien Grall wrote:
> 
> 
> On 19/10/2023 12:01, Jan Beulich wrote:
>> 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".
> 
>  From previous discussion, I recalled that we seemed to agree that if 
> applies for most the architecture, then it should be considered common.

Hmm, not my recollection - a certain amount of "does this make sense from
an abstract perspective" should also be applied.

>> Avoiding duplication could e.g. be achieved by making RISC-V symlink Arm's
>> header.
> 
> Ewwwwww. Removing the fact I dislike it, I can see some issues with this 
> approach in term of review. Who is responsible to review for any changes 
> here? Surely, we don't only want to the Arm folks to review.

That could be achieved by an F: entry in the RISC-V section of ./MAINTAINERS.

Jan