Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall"

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 23/06/16 15:59, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote:
>> On 23/06/16 14:25, Marek Marczykowski-Górecki wrote:
>>> On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote:
>>>>>>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> wrote:
>>>>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote:
>>>>>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote:
>>>>>>>>>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote:
>>>>>>>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote:
>>>>>>>>> I wonder what good the duplication of the returned domain ID does: I'm
>>>>>>>>> tempted to remove the one in the command-specific structure. Does
>>>>>>>>> anyone have insight into why it was done that way?
>>>>>>>> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first
>>>>>>>> existing domain with ID >= passed one? Reading various comments in code
>>>>>>>> it looks to be used to domain enumeration. This patch changes this
>>>>>>>> behaviour.
>>>>>>> No, it doesn't. It adjusts the behavior only for the DM case (which
>>>>>>> isn't supposed to get information on other than the target domain,
>>>>>>> i.e. in this one specific case the very domain ID needs to be passed
>>>>>>> in).
>>>>>> int xc_domain_getinfo(xc_interface *xch,
>>>>>>                       uint32_t first_domid,
>>>>>>                       unsigned int max_doms,
>>>>>>                       xc_dominfo_t *info)
>>>>>> {
>>>>>>     unsigned int nr_doms;
>>>>>>     uint32_t next_domid = first_domid;
>>>>>>     DECLARE_DOMCTL;
>>>>>>     int rc = 0;
>>>>>>
>>>>>>     memset(info, 0, max_doms*sizeof(xc_dominfo_t));
>>>>>>
>>>>>>     for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ )
>>>>>>     {   
>>>>>>         domctl.cmd = XEN_DOMCTL_getdomaininfo;
>>>>>>         domctl.domain = (domid_t)next_domid;
>>>>>>         if ( (rc = do_domctl(xch, &domctl)) < 0 )
>>>>>>             break;
>>>>>>         info->domid      = (uint16_t)domctl.domain;
>>>>>> (...)
>>>>>>         next_domid = (uint16_t)domctl.domain + 1;
>>>>>>
>>>>>>
>>>>>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next 
>>>>> valid
>>>>>> domain.
>>>>> Hmm, looks like I've misread you patch. Reading again...
>>>>>
>>>>> But now I see rcu_read_lock(&domlist_read_lock) is gets called only when
>>>>> looping over domains, but rcu_read_unlock is called in any case. Is it
>>>>> correct?
>>>> How that? There is this third hunk:
>>> Ok, after drawing a flowchart of the control in this function after your
>>> change, on a piece of paper, this case looks fine. But depending on how
>>> the domain was found (explicit loop or rcu_lock_domain_by_id), different
>>> locks are held, which makes it harder to follow what is going on.
>>>
>>> Crazy idea: how about making the code easy/easier to read instead of
>>> obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is
>>> convolved enough. How about this version (2 patches):
>>>
>>> xen: move domain lookup for getdomaininfo to the same
>>>
>>> XEN_DOMCTL_getdomaininfo have different semantics than most of others
>>> domctls - it returns information about first valid domain with ID >=
>>> argument. But that's no excuse for having the lookup done in a different
>>> place, which made handling different corner cases unnecessary complex.
>>> Move the lookup to the first switch clause. And adjust locking to be the
>>> same as for other cases.
>>>
>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> FWIW, I prefer this solution to the issue.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with a few style
>> nits.
> Fixed patch according to your comments:
>
> xen: move domain lookup for getdomaininfo to the same
>
> XEN_DOMCTL_getdomaininfo have different semantics than most of others
> domctls - it returns information about first valid domain with ID >=
> argument. But that's no excuse for having the lookup code in a different
> place, which made handling different corner cases unnecessary complex.
> Move the lookup to the first switch clause. And adjust locking to be the
> same as for other cases.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/common/domctl.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index e43904e..41de3e8 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -442,11 +442,32 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      switch ( op->cmd )
>      {
>      case XEN_DOMCTL_createdomain:
> -    case XEN_DOMCTL_getdomaininfo:
>      case XEN_DOMCTL_test_assign_device:
>      case XEN_DOMCTL_gdbsx_guestmemio:
>          d = NULL;
>          break;
> +
> +    case XEN_DOMCTL_getdomaininfo:
> +        d = rcu_lock_domain_by_id(op->domain);
> +
> +        if ( d == NULL )
> +        {
> +            /* Search for the next available domain. */
> +            rcu_read_lock(&domlist_read_lock);
> +
> +            for_each_domain ( d )
> +                if ( d->domain_id >= op->domain )
> +                {
> +                    rcu_lock_domain(d);
> +                    break;
> +                }
> +
> +            rcu_read_unlock(&domlist_read_lock);
> +            if ( d == NULL )
> +                return -ESRCH;
> +        }
> +        break;
> +
>      default:
>          d = rcu_lock_domain_by_id(op->domain);
>          if ( d == NULL )
> @@ -862,33 +883,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          break;
>  
>      case XEN_DOMCTL_getdomaininfo:
> -    {
> -        domid_t dom = op->domain;
> -
> -        rcu_read_lock(&domlist_read_lock);
> -
> -        for_each_domain ( d )
> -            if ( d->domain_id >= dom )
> -                break;
> -
> -        ret = -ESRCH;
> -        if ( d == NULL )
> -            goto getdomaininfo_out;
> -
>          ret = xsm_getdomaininfo(XSM_HOOK, d);
>          if ( ret )
> -            goto getdomaininfo_out;
> +            break;
>  
>          getdomaininfo(d, &op->u.getdomaininfo);
> -
>          op->domain = op->u.getdomaininfo.domain;
>          copyback = 1;
> -
> -    getdomaininfo_out:
> -        rcu_read_unlock(&domlist_read_lock);
> -        d = NULL;
>          break;
> -    }
>  
>      case XEN_DOMCTL_getvcpucontext:
>      {


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel