Re: [PATCH v3 11/32] xen/x86: add bitmap of enabled emulated devices

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

El 06/07/15 a les 16.10, Andrew Cooper ha escrit:
> On 03/07/15 12:34, Roger Pau Monne wrote:
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index a112953..d684f49 100644
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -528,6 +528,7 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>  {
>>      int i, paging_initialised = 0;
>>      int rc = -ENOMEM;
>> +    uint32_t emulation_mask;
>>  
>>      d->arch.s3_integrity = !!(domcr_flags & DOMCRF_s3_integrity);
>>  
>> @@ -550,6 +551,20 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>                 d->domain_id);
>>      }
>>  
>> +    if ( is_hvm_domain(d) )
>> +    {
>> +        emulation_mask = (EMU_LAPIC | EMU_HPET | EMU_PMTIMER | EMU_RTC |
>> +                          EMU_IOAPIC | EMU_PIC | EMU_PMU | EMU_VGA | EMU_IOMMU);
>> +        if ( (config->emulation_flags & emulation_mask) != emulation_mask )
>> +        {
>> +            printk(XENLOG_G_ERR "Xen does not allow HVM creation with the "
>> +                   "current selection of emulators: 0x%x.\n",
>> +                   config->emulation_flags);
> 
> Please put "d%d" in the error message to identify which domain has the
> bad configuration.
> 
>> +            return -EPERM;
> 
> EPERM feels wrong here.  It is not a lack of permissions causing the
> issue.  EINVAL/EOPNOTSUPP perhaps?

EOPNOTSUPP looks better indeed.

>> +        }
>> +        d->arch.emulation_flags = config->emulation_flags;
>> +    }
>> +
>>      if ( has_hvm_container_domain(d) )
>>      {
>>          d->arch.hvm_domain.hap_enabled =
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index 96bde65..502379e 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -356,8 +356,21 @@ struct arch_domain
>>  
>>      /* Mem_access emulation control */
>>      bool_t mem_access_emulate_enabled;
>> +
>> +    /* Emulated devices enabled bitmap. */
>> +    uint32_t emulation_flags;
>>  } __cacheline_aligned;
>>  
>> +#define has_vlapic(d)       (d->arch.emulation_flags & EMU_LAPIC)
>> +#define has_vhpet(d)        (d->arch.emulation_flags & EMU_HPET)
>> +#define has_vpmtimer(d)     (d->arch.emulation_flags & EMU_PMTIMER)
>> +#define has_vrtc(d)         (d->arch.emulation_flags & EMU_RTC)
>> +#define has_vioapic(d)      (d->arch.emulation_flags & EMU_IOAPIC)
>> +#define has_vpic(d)         (d->arch.emulation_flags & EMU_PIC)
>> +#define has_vpmu(d)         (d->arch.emulation_flags & EMU_PMU)
>> +#define has_vvga(d)         (d->arch.emulation_flags & EMU_VGA)
>> +#define has_viommu(d)       (d->arch.emulation_flags & EMU_IOMMU)
> 
> (d) should be bracketed in the expansion.
> 
>> +
>>  #define has_arch_pdevs(d)    (!list_empty(&(d)->arch.pdev_list))
>>  
>>  #define gdt_ldt_pt_idx(v) \
>> diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
>> index 2ecc9c9..6b387a3 100644
>> --- a/xen/include/public/arch-x86/xen.h
>> +++ b/xen/include/public/arch-x86/xen.h
>> @@ -268,7 +268,25 @@ typedef struct arch_shared_info arch_shared_info_t;
>>   * XEN_DOMCTL_INTERFACE_VERSION.
>>   */
>>  struct xen_arch_domainconfig {
>> -    char dummy;
>> +#define _EMU_LAPIC                  0
>> +#define EMU_LAPIC                   (1U<<_EMU_LAPIC)
>> +#define _EMU_HPET                   1
>> +#define EMU_HPET                    (1U<<_EMU_HPET)
>> +#define _EMU_PMTIMER                2
>> +#define EMU_PMTIMER                 (1U<<_EMU_PMTIMER)
>> +#define _EMU_RTC                    3
>> +#define EMU_RTC                     (1U<<_EMU_RTC)
>> +#define _EMU_IOAPIC                 4
>> +#define EMU_IOAPIC                  (1U<<_EMU_IOAPIC)
>> +#define _EMU_PIC                    5
>> +#define EMU_PIC                     (1U<<_EMU_PIC)
>> +#define _EMU_PMU                    6
>> +#define EMU_PMU                     (1U<<_EMU_PMU)
>> +#define _EMU_VGA                    7
>> +#define EMU_VGA                     (1U<<_EMU_VGA)
>> +#define _EMU_IOMMU                  8
>> +#define EMU_IOMMU                   (1U<<_EMU_IOMMU)
> 
> These are all in the public API, so absolutely needs a XEN_ prefix, and
> likely an X86 one as well.
> 
>> +    uint32_t emulation_flags;
> 
> Either state that this is ignored for domain types other than HVM, or
> have arch_domain_create() confirm that it is 0.

IMHO adding a check to arch_domain_create is TRTTD.

Roger.