Re: [PATCH v6 11/29] xen/x86: add bitmap of enabled emulated devices

["Roger Pau Monné" <roger.pau@citrix.com>]

El 16/09/15 a les 11.50, Jan Beulich ha escrit:
>>>> On 04.09.15 at 14:08, <roger.pau@citrix.com> wrote:
>> --- a/tools/libxl/libxl_x86.c
>> +++ b/tools/libxl/libxl_x86.c
>> @@ -7,8 +7,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>                                        libxl_domain_config *d_config,
>>                                        xc_domain_configuration_t *xc_config)
>>  {
>> -    /* No specific configuration right now */
>> -
>> +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM)
>> +        xc_config->emulation_flags = (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET |
>> +                                      XEN_X86_EMU_PMTIMER | XEN_X86_EMU_RTC |
>> +                                      XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC |
>> +                                      XEN_X86_EMU_PMU | XEN_X86_EMU_VGA |
>> +                                      XEN_X86_EMU_IOMMU);
> 
> This calls for the elsewhere discussed XEN_X86_EMU_ALL to even be
> exposed to the tool stack.

Done.

>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -555,6 +555,29 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>                 d->domain_id);
>>      }
>>  
>> +    if ( is_hvm_domain(d) )
>> +    {
>> +        uint32_t emulation_mask = (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET |
> 
> const

With the introduction of XEN_X86_EMU_ALL this is no longer needed.

> 
>> +                                   XEN_X86_EMU_PMTIMER | XEN_X86_EMU_RTC |
>> +                                   XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC |
>> +                                   XEN_X86_EMU_PMU | XEN_X86_EMU_VGA |
>> +                                   XEN_X86_EMU_IOMMU);
>> +        if ( (config->emulation_flags & ~emulation_mask) != 0 )
> 
> Missing blank line between declaration and statements.

emulation_mask is now gone, so no need for the newline any more.

>> +        {
>> +            printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x.\n",
> 
> Generally we have no full stops at the end of log messages.

Ack, I've removed them here and below, but I've noticed that other
printks in arch_domain_create have full stops (that's probably why I've
added them here).

> 
>> +                   d->domain_id, config->emulation_flags);
>> +            return -EINVAL;
>> +        }
>> +        if ( config->emulation_flags != emulation_mask )
>> +        {
>> +            printk(XENLOG_G_ERR "d%d: Xen does not allow HVM creation with the "
>> +                   "current selection of emulators: %#x.\n", d->domain_id,
>> +                   config->emulation_flags);
>> +            return -EOPNOTSUPP;
>> +        }
>> +        d->arch.emulation_flags = config->emulation_flags;
>> +    }
> 
> Isn't there an "else" missing here, validating that the flags are zero?

The comment in xen/include/asm-x86/domain.h above the emulation_flags
field already mentions that this field is ignored for PV guests. For
example the x86 Dom0 building code calls arch_domain_create passing in a
NULL xen_arch_domainconfig, so I think it's easier to just ignore this
for PV guests.

Roger.