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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 3 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> A couple of comments below, mostly because I wasn't clear enough on the
> previous version. I am not sure if it is worth resending the series, maybe
> just resending this one would be sufficient?
> 
> On 03/10/2019 02:35, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 84b65b8f25..b90902ad97 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1714,6 +1714,88 @@ static int __init make_vpl011_uart_node(struct
> > kernel_info *kinfo)
> >   }
> >   #endif
> >   +/*
> > + * Scan device tree properties for passthrough specific information.
> > + * Returns < 0 on error
> > + *         0 on success
> > + */
> > +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
> > +                                          const struct fdt_property
> > *xen_reg,
> > +                                          const struct fdt_property
> > *xen_path,
> > +                                          bool xen_force,
> > +                                          uint32_t address_cells, uint32_t
> > size_cells)
> > +{
> > +    const __be32 *cell;
> > +    unsigned int i, len;
> > +    struct dt_device_node *node;
> > +    int res;
> > +    paddr_t mstart, size, gstart;
> > +
> > +    /* xen,reg specifies where to map the MMIO region */
> > +    cell = (const __be32 *)xen_reg->data;
> > +    len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
> > +                                        sizeof(uint32_t));
> 
> Apologies for this, I misread you previous code. I am happy with this version
> or the previous one.
> 
> [...]
> 
> > +    /*
> > +     * Only handle passthrough properties if both xen,reg and xen,path
> > +     * are present, or if xen,force-assign-without-iommu is specified.
> > +     */
> > +    if ( xen_reg != NULL && (xen_path != NULL || xen_force) )
> > +    {
> > +        res = handle_passthrough_prop(kinfo, xen_reg, xen_path, xen_force,
> > +                                      address_cells, size_cells);
> > +        if ( res < 0 )
> > +        {
> > +            printk(XENLOG_ERR "Failed to assign device to %pd\n",
> > kinfo->d);
> 
> This is not the error path I meant.
> 
> The one I was referring is the case where xen,path is present but not xen,reg.
> At the moment you will continue without telling the user. We should at least
> print a warning and probably return an error as someone specifying "xen,path"
> may expect to assign the device.

Are you OK with me adding the following:

    else if ( (xen_path && !xen_reg) || (xen_reg && !xen_path && !xen_force) )
    {
        printk(XENLOG_ERR "xen,reg or xen,path missing for %pd\n",
               kinfo->d);
        return -EINVAL;
    }


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