Re: [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm

["Chen, Tiejun" <tiejun.chen@intel.com>]

>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -34,6 +34,7 @@
>>>>   #include <xen/tasklet.h>
>>>>   #include <xsm/xsm.h>
>>>>   #include <asm/msi.h>
>>>> +#include <xen/stdbool.h>
>>>>
>>>>   struct pci_seg {
>>>>       struct list_head alldevs_list;
>>>> @@ -1553,6 +1554,44 @@ int iommu_do_pci_domctl(
>>>>           }
>>>>           break;
>>>>
>>>> +    case XEN_DOMCTL_set_rdm:
>>>> +    {
>>>> +        struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
>>>> +        struct xen_guest_pcidev_info *pcidevs = NULL;
>>>> +        struct domain *d = rcu_lock_domain_by_any_id(domctl->domain);
>>>> +
>>>> +        if ( d == NULL )
>>>> +            return -ESRCH;
>>>> +
>>>
>>> What if this is called on an PV domain?
>>
>> Currently we just support this in HVM, so I'd like to add this,
>>
>>           if ( d == NULL )
>>               return -ESRCH;
>>
>> +        ASSERT( is_hvm_domain(d) );
>> +
>
> No. Please don't crash the hypervisor.

Okay.

>
> Just return -ENOSYS or such when done for PV guests.

So,

+        if ( !is_hvm_domain(d) )
+            return -ENOSYS;

>
>>
>>>
>>> You are also missing the XSM checks.
>>
>> Just see this below.
>>
>>>
>>> What if this is called multiple times. Is it OK to over-ride
>>> the 'pci_force' or should it stick once?
>>
>> It should be fine since just xc/hvmloader need such an information while
>> creating a VM.
>>
>> And especially, currently we just call this one time to set. So why we need
>> to call this again and again? I think if anyone want to extend such a case
>> you're worrying, he should know any effect before he take a action, right?
>
> Program defensively and also think about preemption. If this call end up

Do you think we need a fine grain way, like lock here?

> being preempted you might need to call it again. Or if the third-party
> toolstack use this operation and call this with wacky values?

Maybe can the following address this enough,

     case XEN_DOMCTL_set_rdm:
     {
         struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
         struct xen_guest_pcidev_info *pcidevs = NULL;

         if ( d->arch.hvm_domain.pcidevs )
             break;

         if ( !is_hvm_domain(d) )
             return -ENOSYS;
	...

>>
>>>
>>>
>>>> +        d->arch.hvm_domain.pci_force =
>>>> +                            xdsr->flags & PCI_DEV_RDM_CHECK ? true : false;
>>>
>>> Won't we crash here if this is called for PV guests?
>>
>> After the line, 'ASSERT( is_hvm_domain(d) );', is added, this problem should
>> be gone.
>
> No it won't be. You will just crash the hypervisor.
>
> Please please put yourself in the mind that the toolstack can (and will)
> have bugs.

Thanks for your reminder.

>>
>>>
>>>> +        d->arch.hvm_domain.num_pcidevs = xdsr->num_pcidevs;
>>>
>>> What if the 'num_pcidevs' has some bogus value. You need to check for that.
>>

[snip]

>>>>           return 0;
>>>
>>>
>>> Also how does this work with 32-bit dom0s? Is there a need to use the
>>> compat layer?
>>
>> Are you saying in xsm case? Others?
>>
>> Actually this new DOMCTL is similar with XEN_DOMCTL_assign_device in some
>> senses but I don't see such an issue you're pointing.
>
> I was thinking about the compat layer and making sure it works properly.

Do we really need this consideration? I mean I referred to that existing 
XEN_DOMCTL_assign_device to implement this new DOMCTL, but looks there's 
nothing related to this point.

Or could you make your thought clear to me with an exiting example? Then 
I can take a look at what exactly should be taken in my new DOMCTL since 
I'm a fresh man to work out this properly inside xen.

Thanks
Tiejun