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

2015-09-23 Thread Roger Pau Monné 
El 23/09/15 a les 15.24, Jan Beulich ha escrit:
 On 23.09.15 at 14:35,  wrote:
>> El 16/09/15 a les 11.50, Jan Beulich ha escrit:
>> On 04.09.15 at 14:08,  wrote:
 +   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.
> 
> Easier now, but perhaps more cumbersome if we ever want to
> assign that field some meaning for PV. It's a domctl, so not _that_
> difficult to change, but you may have noticed that I generally ask
> for unused fields/bits to be validated to be zero, to allow using
> them later on. Anyway - not a big issue, I just wanted to point it
> out (and I might stumble across the missing else again during
> review of future revisions).

OK, you convinced me. I've added a check to the start of
arch_domain_create in order to make sure config is not NULL, and fixed
the x86 callers of domain_create in order to make sure a non-null config
is always provided.

Also dropped Andrew Cooper's reviewed-by tag, since the hypervisor side
code has changed substantially.

Roger.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-23 Thread Jan Beulich 
>>> On 23.09.15 at 14:35,  wrote:
> El 16/09/15 a les 11.50, Jan Beulich ha escrit:
> On 04.09.15 at 14:08,  wrote:
>>> +   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.

Easier now, but perhaps more cumbersome if we ever want to
assign that field some meaning for PV. It's a domctl, so not _that_
difficult to change, but you may have noticed that I generally ask
for unused fields/bits to be validated to be zero, to allow using
them later on. Anyway - not a big issue, I just wanted to point it
out (and I might stumble across the missing else again during
review of future revisions).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-23 Thread Andrew Cooper 



On 23/09/15 13:42, Roger Pau Monné wrote:

El 16/09/15 a les 12.10, Jan Beulich ha escrit:

On 04.09.15 at 14:08,  wrote:

--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -387,8 +387,21 @@ struct arch_domain
  bool_t mem_access_emulate_enabled;
  
  struct monitor_write_data *event_write_data;

+
+/* Emulated devices enabled bitmap. */
+uint32_t emulation_flags;
  } __cacheline_aligned;
  
+#define has_vlapic(d)   ((d)->arch.emulation_flags & XEN_X86_EMU_LAPIC)

+#define has_vhpet(d)((d)->arch.emulation_flags & XEN_X86_EMU_HPET)
+#define has_vpmtimer(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PMTIMER)
+#define has_vrtc(d) ((d)->arch.emulation_flags & XEN_X86_EMU_RTC)
+#define has_vioapic(d)  ((d)->arch.emulation_flags & XEN_X86_EMU_IOAPIC)
+#define has_vpic(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PIC)
+#define has_vpmu(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PMU)
+#define has_vvga(d) ((d)->arch.emulation_flags & XEN_X86_EMU_VGA)
+#define has_viommu(d)   ((d)->arch.emulation_flags & XEN_X86_EMU_IOMMU)

And btw, now that I saw a few uses of these - do they really all need
to be has_v*() instead of just has_*()? Together with the macros taking
a domain pointer it's quite obvious that talk is about virtual devices...

IMHO, I prefer to have the "v" prefix, because that's how they are named
inside of Xen, so it's more clear that we are actually disabling them,
but I'm not going to fight for it.


I personally think it is clearer having the v in the name.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-23 Thread Roger Pau Monné 
El 16/09/15 a les 12.10, Jan Beulich ha escrit:
 On 04.09.15 at 14:08,  wrote:
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -387,8 +387,21 @@ struct arch_domain
>>  bool_t mem_access_emulate_enabled;
>>  
>>  struct monitor_write_data *event_write_data;
>> +
>> +/* Emulated devices enabled bitmap. */
>> +uint32_t emulation_flags;
>>  } __cacheline_aligned;
>>  
>> +#define has_vlapic(d)   ((d)->arch.emulation_flags & XEN_X86_EMU_LAPIC)
>> +#define has_vhpet(d)((d)->arch.emulation_flags & XEN_X86_EMU_HPET)
>> +#define has_vpmtimer(d) ((d)->arch.emulation_flags & 
>> XEN_X86_EMU_PMTIMER)
>> +#define has_vrtc(d) ((d)->arch.emulation_flags & XEN_X86_EMU_RTC)
>> +#define has_vioapic(d)  ((d)->arch.emulation_flags & XEN_X86_EMU_IOAPIC)
>> +#define has_vpic(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PIC)
>> +#define has_vpmu(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PMU)
>> +#define has_vvga(d) ((d)->arch.emulation_flags & XEN_X86_EMU_VGA)
>> +#define has_viommu(d)   ((d)->arch.emulation_flags & XEN_X86_EMU_IOMMU)
> 
> And btw, now that I saw a few uses of these - do they really all need
> to be has_v*() instead of just has_*()? Together with the macros taking
> a domain pointer it's quite obvious that talk is about virtual devices...

IMHO, I prefer to have the "v" prefix, because that's how they are named
inside of Xen, so it's more clear that we are actually disabling them,
but I'm not going to fight for it.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-23 Thread Roger Pau Monné 
El 16/09/15 a les 11.50, Jan Beulich ha escrit:
 On 04.09.15 at 14:08,  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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-23 Thread Roger Pau Monné 
El 04/09/15 a les 15.55, Jan Beulich ha escrit:
 On 04.09.15 at 15:51,  wrote:
>> El 04/09/15 a les 14.25, Wei Liu ha escrit:
>>> On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:
 diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
 index 045f6ff..fe9504f 100644
 --- 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 |
 +   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 is repetitive. Could you consolidate all these to
>>>
>>>   #define XEN_X86_EMU_ALL ...
>>>
>>> ?
>>
>> That sounds fine, I would place it in the public header where all the
>> XEN_X86_EMU_* are defined. I will wait for Andrew's opinion, since he
>> already acked this patch.
> 
> This doesn't belong in the public ABI, so if at all it should be added
> there inside #ifdef __XEN__. Alternatively (and perhaps preferably)
> this would go into another (internal) header.

The definitions of XEN_X86_EMU_* and xen_arch_domainconfig are already
protected with:

#if defined(__XEN__) || defined(__XEN_TOOLS__)

I don't mind putting them in a different header, but I think this needs
to live in the public headers, and IMHO public/arch-x86/xen.h is
probably the best place, specially taking into account that
xen_arch_domainconfig is also defined there.

Unless someone objects and provides a more suitable location I'm going
to define XEN_X86_EMU_ALL there also.

Roger.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-16 Thread Jan Beulich 
>>> On 04.09.15 at 14:08,  wrote:
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -387,8 +387,21 @@ struct arch_domain
>  bool_t mem_access_emulate_enabled;
>  
>  struct monitor_write_data *event_write_data;
> +
> +/* Emulated devices enabled bitmap. */
> +uint32_t emulation_flags;
>  } __cacheline_aligned;
>  
> +#define has_vlapic(d)   ((d)->arch.emulation_flags & XEN_X86_EMU_LAPIC)
> +#define has_vhpet(d)((d)->arch.emulation_flags & XEN_X86_EMU_HPET)
> +#define has_vpmtimer(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PMTIMER)
> +#define has_vrtc(d) ((d)->arch.emulation_flags & XEN_X86_EMU_RTC)
> +#define has_vioapic(d)  ((d)->arch.emulation_flags & XEN_X86_EMU_IOAPIC)
> +#define has_vpic(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PIC)
> +#define has_vpmu(d) ((d)->arch.emulation_flags & XEN_X86_EMU_PMU)
> +#define has_vvga(d) ((d)->arch.emulation_flags & XEN_X86_EMU_VGA)
> +#define has_viommu(d)   ((d)->arch.emulation_flags & XEN_X86_EMU_IOMMU)

And btw, now that I saw a few uses of these - do they really all need
to be has_v*() instead of just has_*()? Together with the macros taking
a domain pointer it's quite obvious that talk is about virtual devices...

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-16 Thread Jan Beulich 
>>> On 04.09.15 at 14:08,  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.

> --- 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

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

> +{
> +printk(XENLOG_G_ERR "d%d: Invalid emulation bitmap: %#x.\n",

Generally we have no full stops at the end of log messages.

> +   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?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-09 Thread Wei Liu 
On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:
> Introduce a bitmap in x86 xen_arch_domainconfig that allows enabling or
> disabling specific devices emulated inside of Xen for HVM guests.
> 
> Signed-off-by: Roger Pau Monné 
> Reviewed-by: Andrew Cooper 
> Cc: Ian Jackson 
> Cc: Stefano Stabellini 
> Cc: Ian Campbell 
> Cc: Wei Liu 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 

Acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-04 Thread Andrew Cooper 

On 04/09/15 14:55, Jan Beulich wrote:

On 04.09.15 at 15:51,  wrote:

El 04/09/15 a les 14.25, Wei Liu ha escrit:

On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 045f6ff..fe9504f 100644
--- 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 |
+   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 is repetitive. Could you consolidate all these to

   #define XEN_X86_EMU_ALL ...

?

That sounds fine, I would place it in the public header where all the
XEN_X86_EMU_* are defined. I will wait for Andrew's opinion, since he
already acked this patch.

This doesn't belong in the public ABI, so if at all it should be added
there inside #ifdef __XEN__. Alternatively (and perhaps preferably)
this would go into another (internal) header.


Agreed.

Having an ALL mask will be useful for checking invalid values, but this 
logic is going to get a bit more complicated as soon as we see about 
introducing situations which permit the use of the vLAPIC, and I don't 
see the ALL mask being useful anywhere else.


I am not fussed either way.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-04 Thread Wei Liu 
On Fri, Sep 04, 2015 at 03:51:17PM +0200, Roger Pau Monné wrote:
> El 04/09/15 a les 14.25, Wei Liu ha escrit:
> > On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:
> >> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> >> index 045f6ff..fe9504f 100644
> >> --- 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 |
> >> +   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 is repetitive. Could you consolidate all these to
> > 
> >   #define XEN_X86_EMU_ALL ...
> > 
> > ?
> 
> That sounds fine, I would place it in the public header where all the
> XEN_X86_EMU_* are defined. I will wait for Andrew's opinion, since he
> already acked this patch.

Of course he has the final authority here. I don't want block this patch
just for such cosmetic comment. :-)

Wei.

> 
> Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-04 Thread Jan Beulich 
>>> On 04.09.15 at 15:51,  wrote:
> El 04/09/15 a les 14.25, Wei Liu ha escrit:
>> On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:
>>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>>> index 045f6ff..fe9504f 100644
>>> --- 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 |
>>> +   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 is repetitive. Could you consolidate all these to
>> 
>>   #define XEN_X86_EMU_ALL ...
>> 
>> ?
> 
> That sounds fine, I would place it in the public header where all the
> XEN_X86_EMU_* are defined. I will wait for Andrew's opinion, since he
> already acked this patch.

This doesn't belong in the public ABI, so if at all it should be added
there inside #ifdef __XEN__. Alternatively (and perhaps preferably)
this would go into another (internal) header.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-04 Thread Roger Pau Monné 
El 04/09/15 a les 14.25, Wei Liu ha escrit:
> On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:
>> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
>> index 045f6ff..fe9504f 100644
>> --- 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 |
>> +   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 is repetitive. Could you consolidate all these to
> 
>   #define XEN_X86_EMU_ALL ...
> 
> ?

That sounds fine, I would place it in the public header where all the
XEN_X86_EMU_* are defined. I will wait for Andrew's opinion, since he
already acked this patch.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-09-04 Thread Wei Liu 
On Fri, Sep 04, 2015 at 02:08:50PM +0200, Roger Pau Monne wrote:
> Introduce a bitmap in x86 xen_arch_domainconfig that allows enabling or
> disabling specific devices emulated inside of Xen for HVM guests.
> 
> Signed-off-by: Roger Pau Monné 
> Reviewed-by: Andrew Cooper 
> Cc: Ian Jackson 
> Cc: Stefano Stabellini 
> Cc: Ian Campbell 
> Cc: Wei Liu 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> ---
> Changes since v4:
>  - Add a check to make sure the emulation bitmap is sane (undefined bits are
>all 0s).
>  - Add Andrew Cooper Reviewed-by.
> 
> Changes since v3:
>  - Return EOPNOTSUPP instead of ENOPERM if an invalid emulation mask is
>used.
>  - Fix error messages (prefix them with d%d and use %#x instead of 0x%x).
>  - Clearly state in the public header that emulation_flags should only be
>used with HVM guests.
>  - Add a XEN_X86 prefix to the emulation flags defines.
>  - Properly parenthese the has_* marcos.
> ---
>  tools/libxl/libxl_x86.c   |  8 ++--
>  xen/arch/x86/domain.c | 23 +++
>  xen/include/asm-x86/domain.h  | 13 +
>  xen/include/public/arch-x86/xen.h | 21 -
>  4 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 9276126..9ecd85d 100644
> --- 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);
>  return 0;
>  }
>  
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 045f6ff..fe9504f 100644
> --- 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 |
> +   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 is repetitive. Could you consolidate all these to

  #define XEN_X86_EMU_ALL ...

?

Or am I talking non-sense?

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel