Re: [RFC v2 6/8] tools/arm: Introduce force_assign_without_iommu option to xl.cfg

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

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