Re: [Xen-devel] [PATCH 2/5] xen/arm: assign devices to boot domains

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 07/08/2019 23:46, Stefano Stabellini wrote:
> On Tue, 15 Jan 2019, Julien Grall wrote:
> > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > > > index cc6b464..d48f77e 100644
> > > > > --- a/xen/arch/arm/domain_build.c
> > > > > +++ b/xen/arch/arm/domain_build.c
> > > > > @@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain
> > > > > *d,
> > > > > struct kernel_info *kinfo)
> > > > >         return 0;
> > > > >     }
> > > > >     +static int __init scan_pt_node(const void *pfdt,
> > > > > +                               int nodeoff, const char *name, int
> > > > > depth,
> > > > > +                               u32 address_cells, u32 size_cells,
> > > > > +                               void *data)
> > > > Is it really necessary to parse the device-tree twice?
> > > I don't think I understand this comment. The device tree fragment is not
> > > scanned twice, just once I think. Am I missing something?
> > The previous patch is going through the DT to copy the properties over. This
> > patch introduce a new function to go over the DT to find the Device to attach.
> > So you are parsing the DT twice. Is there any reason for that?
> I got your question now.  I spent quite some time to merge the two
> paths, the first one used to copy the fragment, the second one used to
> parse it, into a single function. It is difficult because the
> information convenient to use for one case, it is not convenient for the
> other (specifically figuring out when to call fdt_end_node when called
> from device_tree_for_each_node.) I managed to do it though, in the next
> version there will be only one parsing of the fragment.
I wasn't necessarily asking to merge the two. I wanted to understand whether we 
can avoid two parsing yet keeping the code simple. Sorry if it wasn't clear enough.
[...]

> > > > Furthermore, this is assuming all the nodes in the fragment will be
> > > > under the GIC controller.  You may want to passthrough a interrupt
> > > > controller (i.e GPIO) to the guest and the related device.
> > > I don't think that the non-GIC scenario is supported today. I don't
> > > think it works even without dom0less. I wrote more about this as a reply
> > > to the other email.
> > I believe this works out-of-box with the guest. If we take the example of the
> > GPIO controller, it may be behind the GIC. You only care to route those
> > interrupts used by GPIO and MMIO associated. The rest can be describe normally
> > in the DT.
> >
> > Of course, this means that you need to passthrough all devices using the GPIO
> > controller to that guest.
> >
> > So I still think you need to check whether the interrupts belongs the GIC or
> > not.
> I think I understand what you meant now. I could add a check before
> routing any interrupts to the guest, to make sure that they are GIC
> interrupts. However, the way to do that check I believe would be using
> the "interrupt-parent" property, but we have just discussed about
> removing it.
I have suggested a way to keep it and avoid writing down the phandle value.

> So, if the user provides a fragment that doesn't have any
> "interrupt-parent" properties, how can I check that the interrupts are
> GIC interrupts?
The problem here is you are re-using the guest "interrupts" property for finding 
out the host interrupts. Firstly, this does not cater the case where guest DT is 
using the property "interrupt-extended".
It feels to me that we should look at the "interrupts" property from the host DT 
and map them 1:1 (i.e irq == virq). The property in the partial DT would just be 
replicating the values from the host DT.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel