Re: [RFC v1 5/5] xen/arm: add SCI mediator support for DomUs

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Stefano,

On Wed, Dec 22, 2021 at 06:23:13PM -0800, Stefano Stabellini wrote:
> On Wed, 22 Dec 2021, Julien Grall wrote:
> > > > > > > To me dtdev and XEN_DOMCTL_assign_device are appropriate because
> > > > > > > they
> > > > > > > signify device assignment of one or more devices. We are not 
> > > > > > > adding
> > > > > > > any
> > > > > > > additional "meaning" to them. It is just that we'll automatically
> > > > > > > detect
> > > > > > > and generate any SCMI configurations based on the list of assigned
> > > > > > > devices. Because if SCMI is enabled and a device is assigned to 
> > > > > > > the
> > > > > > > guest, then I think we want to add the device to the SCMI list of
> > > > > > > devices automatically.
> > > > > > 
> > > > > > I am OK with re-using dtdev/XEN_DOMCTL_assign_device however there 
> > > > > > is
> > > > > > a pitfall with that approach.
> > > > > > 
> > > > > > At the moment, they are only used for device protected by the
> > > > > > IOMMU. If the device is not protected by the IOMMU then it will 
> > > > > > return
> > > > > > an error.
> > > > > IIRC there was a change, that allowed to assign device without a
> > > > > IOMMU. At
> > > > > least we discussed this internally.
> > > > 
> > > > I am not aware of any upstream. Do you have a pointer if there is any
> > > > public discussion?
> > > 
> > > No, this is the first public discussion on this topic.
> > > 
> > > > 
> > > > > > 
> > > > > > Now, with your approach we may have a device that is not protected 
> > > > > > by
> > > > > > the IOMMU but require to a SCMI configuration.
> > > > > You need to protect only DMA-capable devices.
> > > > 
> > > > Xen doesn't know if the device is DMA-capable or not. So...
> > > > 
> > > 
> > > But it knows if there is a iommus=<> node present for the device.
> > 
> > Not really. Not all the platforms have IOMMUs and not all the platforms with
> > IOMMU have DMA-capable device protected by an IOMMU.
> > 
> > > 
> > > > > 
> > > > > > I don't think it would be sensible to just return "succeed" here
> > > > > > because technically you are asking to assign a non-protected
> > > > > > device. But at the same time, it would prevent a user to assign a
> > > > > > non-DMA capable device.
> > > > > > 
> > > > > > So how do you suggest to approach this?
> > > > > Well, in my head assign_device ideally should do the following:
> > > > > 1. Assign IOMMU if it is configured for the device
> > > > 
> > > > ... with this approach you are at the risk to let the user passthrough
> > > > a device that cannot be protected.
> > > > 
> > > > > 2. Assign SCMI access rights
> > > > > (Not related to this patch series, but...)
> > > > > 3. Assign IRQs
> > > > > 4. Assign IO memory ranges.
> > > > > Points 3. and 4. would allow us to not provide additional irq=[] and
> > > > > iomem=[] entries in a guest config.
> > > > 
> > > > That could only work if your guest is using the same layout as the
> > > > host.
> > > 
> > > Agreed. But in my experience, in most cases this is the true. We worked
> > > with Renesas and IMX hardware and in both cases iomems were
> > > mapped 1:1.
> > > 
> > > > Otherwise, there is a risk it will clash with other parts of the
> > > > memory layout.
> > > > 
> > > 
> > > > Today, guests started via the toolstack is only using a virtual
> > > > layout, so you would first need to add support to use the host memory
> > > > layout.
> > > 
> > > I can't say for all the possible configurations in the wild, but I'm
> > > assuming that in most cases it will be fine to use 1:1 mapping. For
> > > those more exotic cases it would be possible to implement some
> > > configuration option like iomem_map=[mfn:gfn].
> > Well, the Xen memory layout is not something that is stable nor AFAIK based 
> > on
> > any memory layout. In fact, there is no such generic layout on Arm.
> > 
> > It is quite possible that it will work well with 1:1 MMIO on some platform
> > (e.g. Renesas, IMX) but you can't expect to work for every Xen release or 
> > all
> > the platforms.
> 
> Yeah this is a true problem. Thankfully with Penny's series we'll be
> able to create domUs with the host memory layout (although dom0less
> only but it is a step forward).
> 
>  
> > > As Stefano pointed, right now user needs to provide 3 configuration
> > > options to pass a device to a guest: dt_dev, irq, iomem. But in fact,
> > > Xen is already knowing about irq and iomem from device tree.
> > 
> > I understand and this is not ideal. I would be happy to consider your
> > approach. However, this will have to enabled only when the guest is re-using
> > the host layout.
> 
> It looks like we all agree that today configuring device assignment with
> Xen is too complicated and would like for it to be simpler. This thread
> has some excellent ideas on how to address the issue. Skip to the end if
> you are only interested in this patch series.
> 
> 
> # Future Ideas
> 
> A great suggestion by Julien is to start supporting the dom0less partial
> device tree format in xl/libxl as well so that we can have a single
> "device_tree" option in dom.cfg instead of 4 (device_tree, iomem, irqs,
> dtdev).
> 
> Even with that implemented, the user has still to provide a partial dtb,
> xen,reg and xen,path. I think this is a great step forward and we should
> do that, if nothing else to make it easier to switch between dom0less
> and normal domU configurations. But the number of options and
> information that the user has to provide is still similar to what we
> have today.
> 
> After that, I think we need something along the lines of what Volodymyr
> suggested. Let's say that the user only provides "dtdev" and
> "device_tree" in dom.cfg.  We could:
> 
> - read interrupts from the  "interrupts" property like we do for dom0less
> - read "xen,reg" for the mapping, if "xen,reg" is missing, read "reg"
>   and assume 1:1 (we could try the mapping and print an error on
>   failure, or we could only do 1:1 if the domain is entirely 1:1)
> - optionally read "xen,path" to populate dtdev
> - if an IOMMU configuration is present, then also configure the IOMMU
>   for the device, if not then "xen,force-assign-without-iommu" must be
>   present
> - assign SCMI access rights
> 
> 
> Now we only have:
> - device_tree in dom.cfg
> - dtdev in dom.cfg (or xen,path in the partial DTB)
> - xen,force-assign-without-iommu in the partial DTB
> 
> 
> It would be good if we could remove "xen,force-assign-without-iommu"
> because at this stage it is the only Xen-specific property remaining in
> the partial DTB. If we could get rid of it, it would make it easier to
> write/generate the partial DTB because it becomes a strict subset of the
> corresponding host node. It would enable us to automatically generate it
> (we are going to do experiments with it at Xilinx in the next few
> months) and it would be easier to explain to users how to write it.
> The partial DTB usually starts as a copy of the corresponding host node
> plus some edits. The fewer edits are required, the better.
> 
> But it is difficult because of the reasons mentioned by Julien. In Xen
> we cannot know if a device is not behind an IOMMU because is not a DMA
> master (so safe to assign) or because the system simply doesn't have
> enough IOMMU coverage (so not safe to assign). Thankfully the hardware
> has been improving in recent years and there are more and more platforms
> with full IOMMU coverage. I think we should make it easier for users on
> these well-behave platforms.
> 
> At least, we could move the "xen,force-assign-without-iommu" option from
> the partial DTB to the VM config file dom.cfg. Something like:
> 
> force-assign-without-iommu="true"
> or
> platform-iommu-safe="true"
> 
> That way, it is global rather than per-device and doesn't have to be
> added by the user by hand when creating the partial DTB.
> 
> 
> # This patch series
> 
> Now in regards to this specific series and the SCMI options today, I
> think it is OK to have a per-domain sci="scmi_smc", but I think we
> should try to avoid another detailed list of device paths or list of
> IDs in addition to dtdev.
> 
> dtdev already specifies the device tree nodes to be assigned to the
> guest. Based on that list we can also do SCMI assignment.
> 
> As Julien pointed out, the issue is: what if a device needs SCMI
> assignment but is not a DMA master (hence there is no IOMMU
> configuration on the host)?
> 
> Attempting to assign a DMA mastering device without IOMMU protection is
> not just unsafe, it will actively cause memory corruptions in most
> cases. It is an erroneous configuration.
> 
> Of course we should try to stop people from running erroneous
> configurations, but we should do so without penalizing all users.
> 
> With that in mind, also considering that dtdev is the list of devices to
> be assigned, I think dtdev should be the list of all devices, even
> non-DMA masters. It makes a lot of sense also because today is really
> difficult to explain to a user that "yes, dtdev is the devices to be
> assigned but no, if the device is a UART you don't have to add it to
> dtdev because it is not a DMA master". It would be a lot easier if the
> list of assigned devices was comprehensive, including both DMA masters
> and not DMA masters.
> 
> So I think we should either:
> 
> a) extend dtdev to cover all devices, including non-DMA masters
> b) or add a new "device_assignment" property which is like dtdev but is
>    the list of both DMA masters and non-DMA masters
> 
> Either way, when non-DMA masters are present in the
> dtdev/device_assignment list we could either:
>     c) require force-assign-without-iommu="true" in dom.cfg
>     d) or print a warning like:
>     "WARNING: device assignment safety for device XXX cannot be
>     verified. Please make sure XXX is not a DMA mastering device."
> 
> 
> All these options are good I think. My preference is a) + d), so extend
> dtdev and print a warning if non-DMA masters are in the list. We don't
> necessarily need a new hypercall but that is also an option.
> 
> I think this discussion was a long time coming and I am glad we are
> having it now. We have a lot of room for improvement! I don't want to
> scope-creep this patch series, but I hope that this last bit could be
> done as part of this series if we find agreement in the community.

For me a) + d) looks good. I will implement it in v2 if everybody ok
with this approach.