Re: [XEN][PATCH v7 15/19] xen/arm: Implement device tree node removal functionalities

[Julien Grall <julien@xxxxxxx>]

Hi,

On 17/08/2023 01:31, Vikram Garhwal wrote:
> On Mon, Jun 05, 2023 at 10:07:48PM +0100, Julien Grall wrote:
> > > +{
> > > +    int rc = 0;
> > > +    struct domain *d = hardware_domain;
> > > +    domid_t domid;
> > > +    unsigned int naddr, len;
> > > +    unsigned int i, nirq;
> > > +
> > > +    domid = dt_device_used_by(device_node);
> > Looking at the caller, it is not clear to me which lock is preventing the
> > device to be assigned whilst you remove it.
> So, this is on user to make sure the domain is not using the device.
It is not clear what you mean by user. Is it someone external to Xen? If 
not, which function is responsible? If yes, then we can't rely on an 
entity outside of Xen to do the right thing.
> > > +    if ( domid != 0 && domid != DOMID_IO )
> > > +    {
> > > +        printk(XENLOG_ERR "Device %s is being used by domain %u. Removing nodes 
> > > failed\n",
> > > +               device_node->full_name, domid);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    dt_dprintk("Removing node: %s\n", device_node->full_name);
> > > +
> > > +    nirq = dt_number_of_irq(device_node); > +
> > > +    /* Remove IRQ permission */
> > > +    for ( i = 0; i < nirq; i++ )
> > > +    {
> > > +        rc = platform_get_irq(device_node, i);
> > As I mentioned in [1], I think that parsing the Device-Tree again when
> > removing any interrupts/mappings is a bit odd as there are more possible
> > failures and is more complex than necessary. I have proposed a way to do it
> > with rangeset, but I can't find any reason why this wasn't done. Can you
> > explain?
> IIUC, range sets can work if we have only one level of node i.e. no children.
> I tried in previous version to use range but it's complicated to get info in
> correct order using rangeset. Example, we have three nodes, node A, B and C. A
> has three child A_a, A_b and A_c. While adding the nodes, we add A first then
> A_a, A_b, A_c and finally B and C. And rangeset is updated in same order but
> when we remove node, first A_c is removed followed by A_b and A_a and then A.
> So, this was the problem for me on how to keep track which interrupt belong to
> which node.
From my understanding, all the nodes added together will have to be 
removed together. IOW, it is not possible to remove A_c in one hypercall 
by not A_b.
It is not clear to me why you need to know which interrupt belong to 
which node. On removal, it should be sufficient to revert all the 
permissions in one go and then all the nodes.
> > > +            return -EINVAL;
> > > +        }
> > > +        /*
> > > +         * TODO: We don't handle shared IRQs for now. So, it is assumed that
> > > +         * the IRQs was not shared with another devices.
> > > +         */
> > > +        rc = irq_deny_access(d, rc);
> > > +        if ( rc )
> > > +        {
> > > +            printk(XENLOG_ERR "unable to revoke access for irq %u for %s\n",
> > > +                   i, device_node->full_name);
> > > +            return rc;
> > > +        }
> > You don't reverse the change in the routing. What would happen if the next
> > overlay is updated to now pass the same device to a guest?
> >
> > I would be OK if this is not handled in this series. But it should be marked
> > as a TODO.
> So, i explained the reason behind this in v4:
I couldn't find the appropriate summary in this series. Can you point me 
to it?
Cheers,

--
Julien Grall