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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


> > Specifically, see this
> > concrete example from ZynqMP:
> > 
> >          ethernet@ff0e0000 {
> >              compatible = "cdns,zynqmp-gem";
> >              status = "okay";
> >              interrupt-parent = <0xfde8>;
> >              interrupts = <0x0 0x3f 0x4 0x0 0x3f 0x4>;
> >              reg = <0x0 0xff0e0000 0x1000>;
> >              clock-names = "pclk", "hclk", "tx_clk", "rx_clk";
> >              #address-cells = <0x1>;
> >              #size-cells = <0x0>;
> >              clocks = <0x1 0x1 0x1 0x1>;
> >              phy-mode = "rgmii-id";
> >              xlnx,ptp-enet-clock = <0x0>;
> >              local-mac-address = [00 0a 35 00 22 01];
> >              phy-handle = <0x2>;
> >              xen,path = "/amba/ethernet@ff0e0000";
> > 
> >              phy@c {
> >                  reg = <0xc>;
> >                  ti,rx-internal-delay = <0x8>;
> >                  ti,tx-internal-delay = <0xa>;
> >                  ti,fifo-depth = <0x1>;
> >                  ti,rxctrl-strap-worka;
> >                  linux,phandle = <0x2>;
> >                  phandle = <0x2>;
> >              };
> >          };
> > 
> > We want to map the first reg property under ethernet@ff0e0000, not the
> > reg property under phy@c, which it doesn't even refer to a memory
> > region. This problem goes away completely if we start using a new Xen
> > specific "xen,reg" property, instead of trying to figure out which
> > existing reg to map. With that, we can safely remove the artificial
> > depth <= 2 restriction.
> 
> Your proposition for "xen,reg" is a bit different from what I was thinking.
> Ideally we only want the user to specify once the guest physical address (i.e
> only in reg). But, I understand that it may require more work.
> 
> I think I would be happy with this solution providing there are an action to
> investigate whether "range" can be used.

OK, I'll add a note to the commit message.


> > > > +            if ( rc < 0 )
> > > > +            {
> > > > +                dprintk(XENLOG_ERR, "Failed to map %"PRIpaddr" to the
> > > > guest\n", start);
> > > > +                return -EFAULT;
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > > +    prop = fdt_get_property_namelen(pfdt, nodeoff, "path",
> > > > strlen("path"),
> > > > &len);
> > > > +    if ( prop != NULL ) {
> > > > +        node = dt_find_node_by_path((char *)&prop->data[0]);
> > > 
> > > What's wrong with giving directly prop->data?
> > 
> > Nothing wrong with it, just a matter of code style. I don't care about
> > this, I can change it.
> 
> My issue is with the cast, not &prop->data[0] itself (though I dislike it ;)).
> Casts are dangerous and should be avoided as much as possible.

I'll remove the cast, I don't know why I added it.


> > > 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.

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?

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