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

[Julien Grall <julien@xen.org>]

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?

>>
>>>            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.

Cheers,

-- 
Julien Grall