[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN][PATCH v9 03/19] xen/arm/device: Remove __init from function type
Hi,
On 25/08/2023 01:52, Vikram Garhwal wrote:
Hi Julien,
On Tue, Aug 22, 2023 at 07:59:13PM +0100, Julien Grall wrote:
Hi Vikram,
On 19/08/2023 01:28, Vikram Garhwal wrote:
Remove __init from following function to access during runtime:
1. map_irq_to_domain()
2. handle_device_interrupts()
3. map_range_to_domain()
4. unflatten_dt_node()
We are at v9 now, so this is more a remark for the future. In general we are
trying to avoid modifying the same code multiple time within the series
because this makes it more difficult to review. In this case, you could have
removed the __init in patch #4 where you also export it.
Move map_irq_to_domain() prototype from domain_build.h to setup.h.
To avoid breaking the build, following changes are also done:
I am guessing by "breaking the build", you mean that you will get an error
because an non-init function is implemented in domain_build.c. Right? If so,
I think this should be spelled out.
Yeah, i will change the commit with right reasoning.
1. Move map_irq_to_domain(), handle_device_interrupts() and
map_range_to_domain()
to device.c. After removing __init type, these functions are not specific
to domain building, so moving them out of domain_build.c to device.c.
2. Remove static type from handle_device_interrupt().
Typo: s/interrupt/interrupts/ to match the function name. But I don't link
the name when it is exported as it leads to think this is dealing with real
interrupt.
With using handle_device() in overlay as your below suggestion will anyway need
this handle_device_interrupts() function here.
Ah I didn't notice it was used in handle_passthrough_prop(). So it
indeed needs to be exported. In which case, I would suggest to rename to
"map_device_irqs_to_domain()".
Looking at the overlay code, the caller (i.e. add_resources()) is very
similar to handle_device(). AFAICT the only differences are:
1/ add_resources() doesn't deal with mapping (you said it will in the
future)
2/ You need to update some rangeset
For 1/ it means this is a superset. For 2/, I think this could be abstracted
by adding a pointer to the rangesets in map_range_data. They could be NULL
for the domain build case.
So can you look at abstracting? This will make easier to maintain a single
place to parse a device node and map it.
A possible name for the function would be dt_map_resources_to_domain().
For this part of dynamic overlay programming this function can be used.
I updated the overlay code to use handle_device() as per your suggestions. Moved
handle_device() and other relevant function out of domain_build.
About renaming the function name to dt_map_resource_to_domain(): I will see if
i can come with better name else will keep your suggestion.
Now with this case, overlay device tree needs to have "xen,passthrough" enabled
else it will try to map and fail as Xen supports irq_route and mapping only at
the domain creation. In earlier patches this worked fine as we were always skip
the routing and map.
Right, but this also meant the behavior when parsing the DT overlay
would not have been consistent with parsing the host DT.
You would have then had to break backward compatibility to add support
for mapping the resources. If this was planned to be dealt before DT
Overlay becomes supported, then this would have been fine as we don't
guarantee stable interface for experimental/tech preview feature.
The new approach should at least prevent any backward compatibility
breakage and would allow us to support DT overlay even before the
resource mapping is supported. So I think this is better.
Cheers,
--
Julien Grall
|
