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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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