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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 23.06.16 at 15:25, <marmarek@invisiblethingslab.com> 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

I don't mind this one.

> xen: allow XEN_DOMCTL_getdomaininfo for device model

But I don't really like this, and would prefer my solution here; it's
Daniel's call though.

Jan

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