[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC v2 6/8] tools/arm: Introduce force_assign_without_iommu option to xl.cfg
Hi Julien, On Fri, Feb 18, 2022 at 10:17:33AM +0000, Julien Grall wrote: > > > On 18/02/2022 09:16, Oleksii Moisieiev wrote: > > Hi Julien, > > Hi Oleksii, > > > On Thu, Feb 17, 2022 at 03:20:36PM +0000, Julien Grall wrote: > > > > xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat", > > > > diff --git a/xen/common/domain.c b/xen/common/domain.c > > > > index 093bb4403f..f1f19bf711 100644 > > > > --- a/xen/common/domain.c > > > > +++ b/xen/common/domain.c > > > > @@ -512,7 +512,7 @@ static int sanitise_domain_config(struct > > > > xen_domctl_createdomain *config) > > > > if ( iommu ) > > > > { > > > > - if ( config->iommu_opts & ~XEN_DOMCTL_IOMMU_no_sharept ) > > > > + if ( config->iommu_opts >> XEN_DOMCTL_IOMMU_MAX ) > > > > > > XEN_DOMCTL_IOMMU_MAX will be defined as: > > > > > > (1U << _XEN_DOMCTL_IOMMU_force_iommu) > > > > > > This means the shift will do the wrong thing. However, AFAICT, this new > > > option will only be supported by Arm and likely only for platform device > > > for > > > the time being. > > > > Thanks, I will fix that. > > > > > > > > That said, I am not convinced this flag should be per-domain in Xen. > > > Instead, I think it would be better to pass the flag via the device assign > > > domctl. > > > > Do you mean that it's better to set this flag per device, not per > > domain? > This will require setting this flag for each device which should > > require either changing the dtdev format in dom.cfg or setting > > xen,force-assign-without-iommu in partial device-tree. > > > > Both of those ways will complicate the configuration. As was mentioned > > before, we don't want to make domain configuration more complicated. > > What do you think about that? > > We have two interfaces here: > 1) User -> tools > 2) tools -> Xen > > We can chose different policy for each interface. > > For the tools -> Xen interface, I think this should be per device (similar > to XEN_DOMCTL_DEV_RDM_RELAXED). > > For the User -> tools, I am open to discussion. One advantage with per > device is the user explicitely vet each device. So it is harder to > passthrough a device wrongly. > > But I agree this also complicates the interface. What do other thinks? > I see the following ways of User -> tools format: a) Set force_assign_without_iommu = 1 in dom.cfg b) Update dtdev format add force_iommu parameter, so dtdev will look like this: dtdev = [ "/soc/dma-controller@e6700000", "/soc/gpio@e6055000,force_iommu", ... ] c)... Tools -> Xen possible ways: d) Set force_assign_without_iommu to domain globally e) Pass force_assign_without_iommu via device-assign domctl. a) + d) is what we have in the patch series. I think a) + e) can work for now so we will have an interface to make force_assign_without_iommu per device in future. What do you think about it? > > > > > > > return -EOPNOTSUPP; > > > > @@ -542,7 +545,7 @@ int iommu_do_domctl( > > > > #endif > > > > #ifdef CONFIG_HAS_DEVICE_TREE > > > > - if ( ret == -ENODEV ) > > > > + if ( ret == -ENOSYS ) > > > > > > AFAICT, none of the code (including callee) before ret have been modified. > > > So why are you modifying the check here? > > > > > > > Because this check will fail if we have CONFIG_HAS_DEVICE_TREE define, > > but do not have CONFIG_HAS_PCI and iommu_do_dt_domctl will not be > > called. > > Below the implementation of iommu_do_domctl() on staging: > > int iommu_do_domctl( > struct xen_domctl *domctl, struct domain *d, > XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > { > int ret = -ENODEV; > > if ( !is_iommu_enabled(d) ) > return -EOPNOTSUPP; > > #ifdef CONFIG_HAS_PCI > ret = iommu_do_pci_domctl(domctl, d, u_domctl); > #endif > > #ifdef CONFIG_HAS_DEVICE_TREE > if ( ret == -ENODEV ) > ret = iommu_do_dt_domctl(domctl, d, u_domctl); > #endif > > return ret; > } > > 'ret' is initialized to -ENODEV. So for !CONFIG_HAS_PCI, then ret will not > be changed. Therefore the current check is correct. > > AFAICT, your patch is setting 'ret' so I don't expect any change here. > > > Same thing if switch/case inside iommu_do_pci_domctl go to default and > > return -ENOSYS. This part looked strange for me. But I will definitely > > go through this part once again. > We use the same sub-op to assign/deassign a PCI and "DT" device. So we are > not interested in -ENOSYS but -ENODEV that would be returned by the checks: > > if ( domct->u.assign_device.dev != XEN_DOMCTL_DEV_PCI ) > > At the moment, there are no sub-op specific to "DT" device. So it is not > necessary for us to check -ENOSYS yet. > > I haven't looked at the rest of the series to see if we need it. But if we > do, then I think the check should be extended in the patch that requires it. > Thank you for the comment. I will refactor this code. Also I wanted to share with you some thoughts about using SMC client_id field to pass agent_id to the SCMI. Posted question regarding this approach to trustedfirmware phabricator [0]. I've found that ATF already has multiagent approach implemented for stm32mp1 platform, see plat/st/stm32mp1/include/stm32mp1_smc.h [1]. It uses 2 funcids hardcoded for AGENT0 and AGENT1: STM32_SIP_SMC_SCMI_AGENT0 0x82002000 STM32_SIP_SMC_SCMI_AGENT1 0x82002001 I think this approach will be very promising for SCI mediator. Firmware defines a range of func_ids, let's say from 0x82000010 to 0x82000020, where 0x82000010 is the base func_id for trusted agent. This func_id is set in arm,scmi-smc node in Xen device-tree. During startup Xen requests agent configuration and calculate func_id for each channel the following way: <Base Func_ID> + <channel_id> Calculated func_id should be assigned to the Domain by setting it as arm,scmi-id in arm,scmi-smc node. So for the Domain Xen will generate the following nodes: scmi { compatible = "arm,scmi-smc"; arm,smc-id = <calculated func_id>; ... shmem = <&shmem_node> }; shmem_node { compatible = "arm,scmi-shmem"; ... }; In this case each domain will get unique func_id to send SCMI commands. I see the following advantages of this approach: 1) There is no need for Xen to intercept SMC requests. All requests from agents will go directly to the Firmware, which can calculate agent_id from func_id. This mean that there is no need for scmi_handle_call function. 2) This approach already implemented for stm32mp1 board so it's more likely to be accepted. Another thing I want to discuss is how Xen should handle scmi related nodes from xen device-tree. Currently Xen device-tree includes arm,scmi-smc node and a list of scmi-shmem nodes for the channels: scmi { compatible = "arm,scmi-smc"; ... }; sram@0x53ff0000 { compatible = "mmio-sram"; ... cpu_scp_shm: scp-shmem@0x0 { compatible = "arm,scmi-shmem"; ... }; scp-shmem@0x1000 { ... }; ... scp-shmem@0xF000 { ... }; }; We do not want all of this nodes to be present in Dom0. I suggest to set xen,passthrough for all this nodes to ensure that Dom0 will not get information about other channels and generate nodes arm,scmi-shmem and arm,scmi-smc for Dom0. I think this approach will be more secure. What do you think about both suggested approaches? [0] https://developer.trustedfirmware.org/T985 [1] https://review.trustedfirmware.org/TF-A/trusted-firmware-a
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |