qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for insta


From: Auger Eric
Subject: Re: [Qemu-devel] [PATCH v3 4/4] hw/arm/sysbus-fdt: Add support for instantiating generic devices
Date: Wed, 8 Aug 2018 15:16:11 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0

Hi Geert,

On 08/08/2018 02:59 PM, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Tue, Aug 7, 2018 at 7:21 PM Auger Eric <address@hidden> wrote:
>> On 08/07/2018 05:32 PM, Geert Uytterhoeven wrote:
>>> On Tue, Aug 7, 2018 at 4:19 PM Auger Eric <address@hidden> wrote:
>>>> On 07/25/2018 04:34 PM, Geert Uytterhoeven wrote:
>>>>> Add a fallback for instantiating generic devices without a type-specific
>>>>> or compatible-specific instantation method.  This will be used when no
>>>>> other match is found.
>>>>>
>>>>> The generic instantation method creates a device node with "reg" and
>>>>> (optional) "interrupts" properties, and copies the "compatible"
>>>>> property and other optional properties from the host.
> 
>>>>> --- a/hw/arm/sysbus-fdt.c
>>>>> +++ b/hw/arm/sysbus-fdt.c
>>>>> @@ -415,6 +415,99 @@ static int add_amd_xgbe_fdt_node(SysBusDevice 
>>>>> *sbdev, void *opaque)
>>>>>      return 0;
>>>>>  }
>>>>>
>>>>> +static HostProperty generic_copied_properties[] = {
>>>>> +    {"compatible", false},
>>>>> +    {"#gpio-cells", true},
>>>>> +    {"gpio-controller", true},
>>>>> +    {"#interrupt-cells", true},
>>>>> +    {"interrupt-controller", true},
>>>>> +    {"interrupt-names", true},
>>>>
>>>> I think we would need to enumerate the other source properties which
>>>> were not copied to the guest fdt and either warn the userspace those
>>>> were omitted or fail. We may end up generating an incomplete guest dt
>>>> node that may not work properly.
>>>
>>> The list above is quite generic, so it is expected that some of the optional
>>> properties (marked "true") cannot be copied. Hence warning about them
>>> will be noisy, and confuse users.  Failing is definitely the wrong thing
>>> to do.
>>
>> I was not talking about those listed here and optional. Those ones are
>> taken care of. I was rather talking about potential other ones, present
>> on host side and not copied on guest side. For instance, let's say your
>> host dt node has a clocks property. You will attempt to create a guest
>> dt node without this property and obviously the device will not work
>> properly on guest side. The end user attempting this assignment does not
>> get any warning on guest launch. Maybe the driver on guest side will be
>> cool enough to issue a warning but we cannot really rely on this
>> assumption. So from a maintenance point of view, it looks not
>> manageable. I think we should checl all props found in the host dt node
>> are considered and copied into the guest node. Otherwise this means the
>> host dt node does not belong to the category of a "simple" one and thus
>> cannot use this creation function.
> 
> It depends. And that makes it difficult to come up with a sensible
> detection system for notifying the user, while avoiding too many false
> positives and negatives.
> 
> Properties like "clocks" typically use phandles, which means the node
> they're pointing to should be copied, too, possibly involving rewriting
> like with interrupts. Hence I think this should be left to a
> device-specific instantiation method.
Fully agreed. But with patch 4, an end-user can try to assign this
device without writing a specific dt node creation function. The device
on guest side will not run properly and he may report a bug. Then what
do we do?

Better emitting a false positive than nothing?
> 
> Furthermore, depending on the SoC, some DT properties should be ignored,
> and must not be copied.
> Examples are:
>   1. "power-domains", and optionally "clocks", when the device is part of a
>      power and/or clock domain.
>      Power management must be handled by the host, cfr. commit
>      415eb9fc0e23071f ("vfio: platform: Fix using devices in PM Domains",
>      
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=415eb9fc0e23071f).
>      That is another reason why we are replacing explicit clock handling
>      for power management by Runtime PM in the individual drivers, cfr.
>      e.g. commit 1ecd34ddf63ef1d4 ("ata: sata_rcar: Add rudimentary Runtime
>      PM support") in linux-next
>      
> (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=1ecd34ddf63ef1d4).
>      (the first reason is abstracting power management, which may differ
>       among SoCs using the same IP core).
>   2. "resets", pointing to a reset controller, as reset must be handled by
>      the host's vfio driver to restore the device to a pristine state
>      before/after virtualization.
>      See also "[PATCH v3 2/2] vfio: platform: Add generic DT reset support"
>      (https://www.spinics.net/lists/devicetree/msg223516.html).

Yes. maybe this should be handled by using a white list of properties
that are safe not to be copied? This is a vast topic.

Wouldn't it make sense to try getting 1-3 upstream separately as this
last patch seems more controversial and independent?

Thanks

Eric
> 
>>> If the host lacks a property that is mandatory for a specific device, the
>>> device won't have worked on the host before neither, right?
>>>
>>> The major issue remains that an incomplete guest DT node may be generated
>>> if the list above lacks certain needed properties, or if subnodes are
>>> needed.
>>> I expect the guest OS driver would complain about the missing parts, though.
>>> In that case, either the list should be extended, or a device-specific
>>> instantiation method should be written.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]