|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains (fwd)
Hi Juergen,
This email is about the dom0less device assignment patch series. It is
very close to ready. I sent v6 yesterday addressing the remaining
comments from Julien. We are down to smaller details.
Oleksandr's iommu series got merged today. That requires a small changes
to the patch below.
I plan to send a v7 addressing the "merge conflict" and also addressing
any remaining points from Julien (Julien plans to give it a look before
Monday my time.)
I hope that's OK for you if we merge this series early next week.
Cheers,
Stefano
---------- Forwarded message ----------
Date: Fri, 27 Sep 2019 20:02:56 +0000
From: Julien Grall <Julien.Grall@xxxxxxx>
To: Oleksandr <olekstysh@xxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: nd <nd@xxxxxxx>, Stefano Stabellini <stefanos@xxxxxxxxxx>,
"andrii_anisov@xxxxxxxx" <andrii_anisov@xxxxxxxx>,
Achin Gupta <Achin.Gupta@xxxxxxx>,
"xen-devel@xxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxx>,
"Volodymyr_Babchuk@xxxxxxxx" <Volodymyr_Babchuk@xxxxxxxx>
Subject: Re: [Xen-devel] [PATCH v5 5/8] xen/arm: assign devices to boot domains
Hi,
On 27/09/2019 15:40, Oleksandr wrote:
>
> On 26.09.19 00:12, Julien Grall wrote:
>> On 25/09/2019 19:49, Stefano Stabellini wrote:
>>> Scan the user provided dtb fragment at boot. For each device node, map
>>> memory to guests, and route interrupts and setup the iommu.
>>>
>>> The memory region to remap is specified by the "xen,reg" property.
>>>
>>> The iommu is setup by passing the node of the device to assign on the
>>> host device tree. The path is specified in the device tree fragment as
>>> the "xen,path" string property.
>>>
>>> The interrupts are remapped based on the information from the
>>> corresponding node on the host device tree. Call
>>> handle_device_interrupts to remap interrupts. Interrupts related device
>>> tree properties are copied from the device tree fragment, same as all
>>> the other properties.
>>>
>>> Also add the new flag XEN_DOMCTL_CDF_iommu to that dom0less domU can use
>>> the IOMMU.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
>>> ---
>>> Changes in v5:
>>> - use local variable for name
>>> - use map_regions_p2mt
>>> - add warning for not page aligned addresses/sizes
>>> - introduce handle_passthrough_prop
>>>
>>> Changes in v4:
>>> - use unsigned
>>> - improve commit message
>>> - code style
>>> - use dt_prop_cmp
>>> - use device_tree_get_reg
>>> - don't copy over xen,reg and xen,path
>>> - don't create special interrupt properties for domU: copy them from the
>>> fragment
>>> - in-code comment
>>>
>>> Changes in v3:
>>> - improve commit message
>>> - remove superfluous cast
>>> - merge code with the copy code
>>> - add interrup-parent
>>> - demove depth > 2 check
>>> - reuse code from handle_device_interrupts
>>> - copy interrupts from host dt
>>>
>>> Changes in v2:
>>> - rename "path" to "xen,path"
>>> - grammar fix
>>> - use gaddr_to_gfn and maddr_to_mfn
>>> - remove depth <= 2 limitation in scanning the dtb fragment
>>> - introduce and parse xen,reg
>>> - code style
>>> - support more than one interrupt per device
>>> - specify only the GIC is supported
>>> ---
>>> xen/arch/arm/domain_build.c | 101
>>> ++++++++++++++++++++++++++++++++++--
>>> 1 file changed, 97 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 9d985d3bbe..414893bc24 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1701,6 +1701,85 @@ static int __init make_vpl011_uart_node(struct
>>> kernel_info *kinfo)
>>> }
>>> #endif
>>> +/*
>>> + * Scan device tree properties for passthrough specific information.
>>> + * Returns -ENOENT when no passthrough properties are found
>>> + * < 0 on error
>>> + * 0 on success
>>> + */
>>> +static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>>> + const struct fdt_property
>>> *prop,
>>> + const char *name,
>>> + uint32_t address_cells,
>>> uint32_t size_cells)
>>> +{
>>> + const __be32 *cell;
>>> + unsigned int i, len;
>>> + struct dt_device_node *node;
>>> + int res;
>>> +
>>> + /* xen,reg specifies where to map the MMIO region */
>>> + if ( dt_prop_cmp("xen,reg", name) == 0 )
>>> + {
>>> + paddr_t mstart, size, gstart;
>>> + cell = (const __be32 *)prop->data;
>>> + len = fdt32_to_cpu(prop->len) /
>>> + ((address_cells * 2 + size_cells) * sizeof(uint32_t));
>>> +
>>> + for ( i = 0; i < len; i++ )
>>> + {
>>> + device_tree_get_reg(&cell, address_cells, size_cells,
>>> + &mstart, &size);
>>> + gstart = dt_next_cell(address_cells, &cell);
>>> +
>>> + if ( gstart & ~PAGE_MASK || mstart & ~PAGE_MASK || size
>>> & ~PAGE_MASK )
>>> + dprintk(XENLOG_WARNING,
>>> + "DomU passthrough config has not page
>>> aligned addresses/sizes\n");
>> I don't think this is wise to continue, the more this is a printk that
>> can only happen in debug build. So someone using a release build may not
>> notice the error.
>>
>> So I think this wants to be a printk(XENLOG_ERR,...) and also return an
>> error.
>>
>>> +
>>> + res = map_regions_p2mt(kinfo->d,
>>> + gaddr_to_gfn(gstart),
>>> + PFN_DOWN(size),
>>> + maddr_to_mfn(mstart),
>>> + p2m_mmio_direct_dev);
>> Coding style.
>>
>>> + if ( res < 0 )
>>> + {
>>> + dprintk(XENLOG_ERR,
>>> + "Failed to map %"PRIpaddr" to the guest
>>> at%"PRIpaddr"\n",
>>> + mstart, gstart);
>> Similarly, this wants to be a printk.
>>
>>> + return -EFAULT;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> + }
>>> + /*
>>> + * xen,path specifies the corresponding node in the host DT.
>>> + * Both interrupt mappings and IOMMU settings are based on it,
>>> + * as they are done based on the corresponding host DT node.
>>> + */
>>> + else if ( dt_prop_cmp("xen,path", name) == 0 )
>>> + {
>>> + node = dt_find_node_by_path(prop->data);
>>> + if ( node == NULL )
>>> + {
>>> + dprintk(XENLOG_ERR, "Couldn't find node %s in host_dt!\n",
>>> + (char *)prop->data);
>> Same here.
>>
>>> + return -EINVAL;
>>> + }
>
> I have to admit that I don't know about dom0less feature enough ...
>
>
> But, shouldn't we check if the device is behind the IOMMU and try to add
> it (iommu_add_dt_device) before assigning it (this is needed for drivers
> which support generic IOMMU DT bindings in the first place).
>
> [please take a look at
> https://lists.xenproject.org/archives/html/xen-devel/2019-09/msg02714.html
> if so]
>
> Julien, what do you think?
Yes you are right.
@Stefano, this is a recently merged feature. Without it, you will not be
able to use passthrough with dom0less guest when the IOMMU (such as
IPMMU) is using the generic DT bindings.
Cheers,
--
Julien Grall_______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |