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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> +
> +            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.

> --- 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


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