Re: [Xen-devel] [PATCH 4/6] xen: Allow hardare domain != dom0

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 03/05/2014 05:04 AM, Jan Beulich wrote:
> > > > On 04.03.14 at 23:51, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> 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

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