From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 4/6] xen: Allow hardare domain != dom0 Date: Wed, 05 Mar 2014 18:04:36 -0500 Message-ID: <5317AD84.1030807@tycho.nsa.gov> References: <1393973494-29411-1-git-send-email-dgdegra@tycho.nsa.gov> <1393973494-29411-5-git-send-email-dgdegra@tycho.nsa.gov> <531704C602000078001211E1@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <531704C602000078001211E1@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 03/05/2014 05:04 AM, Jan Beulich wrote: >>>> On 04.03.14 at 23:51, Daniel De Graaf wrote: >> This adds a hypervisor command line option "hardware_dom=" which takes a >> domain ID. When the domain with this ID is created, it will be used as >> the hardware domain. >> >> This is intended to be used when dom0 is a dedicated stub domain for >> domain building, allowing the hardware domain to be de-privileged and >> act only as a driver domain. > > Apart from the abstract question regarding the purpose of this, a > couple of comments on the patch a such: > >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -88,6 +88,9 @@ unsigned long __initdata highmem_start; >> size_param("highmem-start", highmem_start); >> #endif >> >> +unsigned int __read_mostly hardware_dom; >> +integer_param("hardware_dom", hardware_dom); > > This ought to be domid_t, and live in common code. > >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -472,6 +472,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) >> break; >> } >> >> + if (d->domain_id == hardware_dom) { > > Coding style. > >> + printk("Initialising hardware domain %d\n", hardware_dom); >> + rangeset_swap(d->irq_caps, dom0->irq_caps); > > Why interrupts, but not I/O ports and MMIO? I/O ports and MMIO are currently set up by the domain builder to mirror those of dom0 with some exceptions. I could swap all of the ranges and then de-privilege the newly created domain if that seems more symmetric (it may also simplify the creation of the domain builder's specifications); I'll try testing that to ensure it works as expected. >> + >> + dom0 = d; > > This, I think, is the point where the variable name becomes > intolerable: ASSERT(!dom0 || !dom0->domain_id) should be > valid at all times as long as the variable name is dom0. I will prepare a pure rename patch prior to this one which renames the dom0 variable (and perhaps some functions with dom0 in their name) to another name: hwdom or hardware_domain would be my preference, with the hardware_dom global renamed to hardware_domid to avoid confusion. >> --- a/xen/common/rangeset.c >> +++ b/xen/common/rangeset.c >> @@ -438,3 +438,29 @@ void rangeset_domain_printk( >> >> spin_unlock(&d->rangesets_lock); >> } >> + >> +void rangeset_swap(struct rangeset *a, struct rangeset *b) >> +{ >> + struct list_head tmp; >> + spin_lock(&a->lock); >> + spin_lock(&b->lock); >> + memcpy(&tmp, &a->range_list, sizeof(tmp)); >> + memcpy(&a->range_list, &b->range_list, sizeof(tmp)); >> + memcpy(&b->range_list, &tmp, sizeof(tmp)); >> + if (a->range_list.next == &b->range_list) { >> + a->range_list.next = &a->range_list; >> + a->range_list.prev = &a->range_list; >> + } else { >> + a->range_list.next->prev = &a->range_list; >> + a->range_list.prev->next = &a->range_list; >> + } >> + if (b->range_list.next == &a->range_list) { >> + b->range_list.next = &b->range_list; >> + b->range_list.prev = &b->range_list; >> + } else { >> + b->range_list.next->prev = &b->range_list; >> + b->range_list.prev->next = &b->range_list; >> + } > > Coding style again. > > Jan Will clean up this patch for style when I resubmit. -- Daniel De Graaf National Security Agency