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

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

On 04/11/2014 05:13 AM, Jan Beulich wrote:
>>>> On 27.03.14 at 12:52, <dgdegra@tycho.nsa.gov> wrote:
>
> I was about to commit this, but further changes are needed (along
> with fixing the typo in the title):

I noticed the typo soon after posting v3, but didn't want to post
v4 with that and some other whitespace.  It looks like v4 will have
useful changes now, however.

>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -84,6 +84,8 @@ unsigned long __initdata highmem_start;
>>   size_param("highmem-start", highmem_start);
>>   #endif
>>
>> +integer_param("hardware_dom", hardware_domid);
>
> This should be moved alongside the definition of the symbol in
> common code, and be enclosed in "#ifdef CONFIG_LATE_HWDOM".
>
> And docs/misc/xen-command-line.markdown wants an entry for it.

OK.

>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -45,6 +45,7 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t);
>>
>>   /* A global pointer to the hardware domain (usually DOM0). */
>>   extern struct domain *hardware_domain;
>> +extern domid_t hardware_domid;
>>
>>   #ifndef CONFIG_COMPAT
>>   #define BITS_PER_EVTCHN_WORD(d) BITS_PER_XEN_ULONG
>> @@ -794,7 +795,7 @@ void watchdog_domain_destroy(struct domain *d);
>>    *    (that is, this would not be suitable for a driver domain)
>>    *  - There is never a reason to deny dom0 access to this
>>    */
>> -#define is_hardware_domain(_d) ((_d)->domain_id == 0)
>> +#define is_hardware_domain(d)  ((d)->domain_id == hardware_domid)
>
> This macro should imo evaluate to true for Dom0 until the hardware
> domain go created, i.e. you should compare _d with hardware_domain
> rather than their IDs. With that the definition of hardware_domid can
> then also be moved inside the #ifdef requested above.

This isn't quite as simple as changing the function since there are
some places where is_hardware_domain needs to return false for domain 0
when a hardware domain is used.  Also, the hardware_domain variable is
not set until domain_create returns, so there are a few places where the
domain ID still needs to be checked explicitly.  It should be possible
to create an is_hardware_domid function for those cases, if comparing to
hardware_domain is preferred for most cases; I think that would belong in
a new patch 5.5/7 (i.e. 6/8 in v4).

Otherwise, I think the is_hardware_domain definition should be:

#ifdef CONFIG_LATE_HWDOM
#define is_hardware_domain(_d) ((_d)->domain_id == hardware_domid)
#else
#define is_hardware_domain(_d) ((_d)->domain_id == 0)
#endif

This also allows hardware_domid to be declared inside the #ifdef.

-- 
Daniel De Graaf
National Security Agency