Re: [XEN][RFC PATCH v3 10/14] xen/arm: Implement device tree node removal functionalities

[Julien Grall <julien@xxxxxxx>]

On 07/12/2022 01:37, Vikram Garhwal wrote:
> > In your use-case, are you planning to ask the admin to reboot if you
> > can't remove a node?
> Yeah. What is error case where it may happen?
The code below have many possible failures. I would suggest to test by 
throwing an error on the second node and check what happen if you try to 
remove again.
> We are checking if dtbo is same as it was added and also expect user to 
> remove the nodes only iff the nodes aren't
> used by any domain.
>
> > > +        {
> > > +            printk(XENLOG_ERR "Device %s is not present in the tree. 
> > > Removing nodes failed\n",
> > > +                   full_dt_node_path[j]);
> > > +            return -EINVAL;
> > > +        }
> > > +
> > > +        domid = dt_device_used_by(overlay_node);
> > > +
> > > +        dt_dprintk("Checking if node %s is used by any domain\n",
> > > +                   full_dt_node_path[j]);
> > > +
> > > +        /* Remove the node iff it's assigned to domain 0 or domain 
> > > io. */
> > > +        if ( domid != 0 && domid != DOMID_IO )
> > I think I asked before, but I have seen no answer on that. What will
> > prevent the device to not be assigned after this check?
> So, here for removal we assume that user removed all on-going ops on the 
> dtbo nodes which they wants to remove.
Please don't make any assumption on what the user is doing even if it is 
dom0. So the hypervisor code should be hardened.
> > > +        {
> > > +            printk(XENLOG_ERR "Device %s as it is being used by 
> > > domain %d. Removing nodes failed\n",
> > > +                   full_dt_node_path[j], domid);
> > > +            return -EINVAL;
> > > +        }
> > > +
> > > +        dt_dprintk("Removing node: %s\n", full_dt_node_path[j]);
> > > +
> > > +        nirq = node_num_irq[j];
> > > +
> > > +        /* Remove IRQ permission */
> > > +        for ( i = 0; i < nirq; i++ )
> > > +        {
> > > +            rc = nodes_irq[j][i];
> > > +            /*
> > > +             * TODO: We don't handle shared IRQs for now. So, it is 
> > > assumed that
> > > +             * the IRQs was not shared with another domain.
> > > +             */
> > This is not what I meant in v2. Interrupts cannot be shared between
> > domain on Arm. However, interrupts can be shared between devices.
> >
> > This is the latter part that needs a TODO.
> >
> > In addition to that, as I wrote, an IRQ can be assigned to a *single*
> > domain without the device been assigned to that domain. So I think this
> > needs to be checked possibly by using the information stored in "desc"
> > to know where the IRQ was routed to.
> >
> > > +            rc = irq_deny_access(d, rc);
> > > +            if ( rc )
> > > +            {
> > > +                printk(XENLOG_ERR "unable to revoke access for irq 
> > > %u for %s\n",
> > > +                       i, dt_node_full_name(overlay_node));
> > > +                return rc;
> > > +            }
> > > +        }
> > > +
> > > +        rc = iommu_remove_dt_device(overlay_node);
> > > +        if ( rc != 0 && rc != -ENXIO )
> > > +            return rc;
> > > +
> > > +        naddr = dt_number_of_address(overlay_node);
> > > +
> > > +        /* Remove mmio access. */
> > > +        for ( i = 0; i < naddr; i++ )
> > > +        {
> > > +            rc = dt_device_get_address(overlay_node, i, &addr, &size);
> > > +            if ( rc )
> > > +            {
> > > +                printk(XENLOG_ERR "Unable to retrieve address %u for 
> > > %s\n",
> > > +                       i, dt_node_full_name(overlay_node));
> > > +                return rc;
> > > +            }
> > > +
> > > +            rc = iomem_deny_access(d, paddr_to_pfn(addr),
> > > +                                   paddr_to_pfn(PAGE_ALIGN(addr + 
> > > size - 1)));
> > I think you missed my comment here. Similar to the IRQs, you are asking
> > for trouble to parse the device-tree again. It would be better to store
> > the information using a rangeset (see include/xen/rangeset.h).
> >
> > I also think the double array for the IRQs should be converted to a
> > rangeset as this would simplify the code.
> Keeping rangeset will work if we only parse one-level nodes. But if 
> there are descendant nodes, then its looking complicated to get info 
> using rangeset. While adding, we have to add parent first and then it's 
> descendant. But while remove, we will need to remove descendants first 
> and the parent node lastly.
I am not sure I understand the problem here. If you use a rangeset, then 
you don't need to go through every node one by one to apply/revert the 
permissions. You could simply apply/revert all the permission in one call.
> > Furthemore, you are removing the permission but not the mapping in the
> > P2M. Can you clarify why?
> We are not actually mapping the nodes here.
The remove function should be the inverse of the add function. AFAICT, 
you are calling mapping the P2M (see call to map_range_to_domain()). So 
this removal function *have* to remove the entry from the P2Ms.
If you disagree with that, then please explain who is going to remove 
the P2M mappings.
±>>
> > > +            if ( rc )
> > > +            {
> > > +                printk(XENLOG_ERR "Unable to remove dom%d access to"
> > > +                        " 0x%"PRIx64" - 0x%"PRIx64"\n",
> > > +                        d->domain_id,
> > > +                        addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1);
> > > +                return rc;
> > > +            }
> > > +        }
> > > +
> > > +        rc = dt_overlay_remove_node(overlay_node);
> > > +        if ( rc )
> > > +            return rc;
> > > +    }
> > > +
> > > +    return rc;
> > > +}
> > [...]
> >
> > > + * overlay_node_track describes information about added nodes 
> > > through dtbo.
> > > + * @entry: List pointer.
> > > + * @dt_host_new: Pointer to the updated dt_host_new unflattened 
> > > 'updated fdt'.
> > > + * @fdt: Stores the fdt.
> > > + * @nodes_fullname: Stores the full name of nodes.
> > > + * @nodes_irq: Stores the IRQ added from overlay dtb.
> > > + * @node_num_irq: Stores num of IRQ for each node in overlay dtb.
> > > + * @num_nodes: Stores total number of nodes in overlay dtb.
> > > + */
> > > +struct overlay_track {
> > > +    struct list_head entry;
> > > +    struct dt_device_node *dt_host_new;
> > > +    void *fdt;
> > > +    char **nodes_fullname;
> > Looking at the code, the main use for the fullname are to check the FDT
> > match and looking up in the DT.
> >
> > In order to check the DT, you could use memcmp() to confirm both the
> > stored FDT and the one provided by the user match.
> >
> > For the lookup, you could avoid it by storing a pointer to the root of
> > the new subtrees.
> >
> > Please let me know if you disagree with this approach.
> If I understood correctly: just keeping the root of new overlay subtree 
> will not work for all case. It will work only if the nodes added are 
> adjacent to each other i.e. have the same parent then it will work as we 
> add all overlay nodes as the last child of their parent. But If two 
> subnodes have different parents, they may or may not be 
> nearby(node->allnext won't work) then we will issues removing the node 
> from this approach.
Thanks for the explanation.

> I did following small modification to your suggestion:
> Keep FDT( do memcmp) for match and also keep address for all added nodes 
> at one-level( we can find children info if we know the top one-level 
> nodes. Please check overlay_node_count()). This will take 8bytes * num 
> of nodes in one-level space which is lot less space than keeping 
> nodes_fullname.
This seems to match my thoughts after your explanation above. I will 
have a look at the next version.
Cheers,

--
Julien Grall