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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.10.2023 13:27, Julien Grall wrote:
> Hi Jan,
> 
> On 19/10/2023 12:14, Jan Beulich wrote:
>> 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.
> 
> This works for one arch. But if PPC needs the same, then this is another 
> symbolic link.
> 
> At which point, how would this be different from asm-generic? We need to 
> have a way to share common headers

... which are sufficiently arch-agnostic.

> that doesn't involve one arch to symlink headers from another arch.

Whether to use symlinks or #include "../../arch/..." or yet something else is
a matter of mechanics.

Jan