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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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?

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

~Andrew