Re: [Xen-devel] [PATCH v4 11/28] x86/vvtd: Process interrupt remapping request

2018-02-23 Thread Roger Pau Monné 
On Sun, Feb 11, 2018 at 01:31:41PM +0800, Chao Gao wrote:
> On Fri, Feb 09, 2018 at 05:44:17PM +, Roger Pau Monné wrote:
> >On Fri, Nov 17, 2017 at 02:22:18PM +0800, Chao Gao wrote:
> >> +static int vvtd_delivery(struct domain *d, uint8_t vector,
> >> + uint32_t dest, bool dest_mode,
> >> + uint8_t delivery_mode, uint8_t trig_mode)
> >> +{
> >> +struct vlapic *target;
> >> +struct vcpu *v;
> >> +
> >> +switch ( delivery_mode )
> >> +{
> >> +case dest_LowestPrio:
> >> +target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
> >> +if ( target != NULL )
> >> +{
> >> +vvtd_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
> >> +   vlapic_domain(target)->domain_id,
> >> +   vlapic_vcpu(target)->vcpu_id,
> >> +   delivery_mode, vector, trig_mode);
> >> +vlapic_set_irq(target, vector, trig_mode);
> >> +break;
> >> +}
> >> +vvtd_debug("d%d: null round robin: vector=%02x\n",
> >> +   d->domain_id, vector);
> >> +break;
> >> +
> >> +case dest_Fixed:
> >> +for_each_vcpu ( d, v )
> >> +if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, 
> >> dest_mode) )
> >> +{
> >> +vvtd_debug("d%d: dest=v%d dlm=%x vector=%d 
> >> trig_mode=%d\n",
> >> +   v->domain->domain_id, v->vcpu_id,
> >> +   delivery_mode, vector, trig_mode);
> >> +vlapic_set_irq(vcpu_vlapic(v), vector, trig_mode);
> >> +}
> >> +break;
> >> +
> >> +case dest_NMI:
> >> +for_each_vcpu ( d, v )
> >> +if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, 
> >> dest_mode) &&
> >> + !test_and_set_bool(v->nmi_pending) )
> >> +vcpu_kick(v);
> >
> >Doing this loops here seems quite bad from a preformance PoV,
> >specially taking into account that this code is going to be used with
> >> 128 vCPUs.
> 
> Maybe. But i prefer to not do optimization at this early stage.

I agree with not doing optimizations for first pass implementations,
but given this series is focused on increasing the number of vCPUs in
order to get better performance adding loops bounded to the number of
vCPUs seems quite incoherent.

There are several of those in the vlapic code for example, so I'm
wondering whether a preparatory patch should deal with those, or at
least have a plan.

I would like to at least see a 'TODO' tag here describing how to deal
with this in the future, so that the maximum allowed number of vCPUs
for HVM domain is not bumped until those TODOs are taken care of.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 11/28] x86/vvtd: Process interrupt remapping request

2018-02-10 Thread Chao Gao 
On Fri, Feb 09, 2018 at 05:44:17PM +, Roger Pau Monné wrote:
>On Fri, Nov 17, 2017 at 02:22:18PM +0800, Chao Gao wrote:
>> When a remapping interrupt request arrives, remapping hardware computes the
>> interrupt_index per the algorithm described in VTD spec
>> "Interrupt Remapping Table", interprets the IRTE and generates a remapped
>> interrupt request.
>> 
>> This patch introduces viommu_handle_irq_request() to emulate the process how
>> remapping hardware handles a remapping interrupt request. This patch
>> also introduces a counter inflight_intr, which is used to count the number
>> of interrupt are being handled. The reason why we should have this
>> counter is VT-d hardware should drain in-flight interrups before setting
>> flags to show that some operations are completed. These operations
>> include enabling interrupt remapping and performing a kind of invalidation
>> requests. In vvtd, we also try to drain in-flight interrupts by waiting
>> the inflight_intr is decreased to 0.
>> 
>> Signed-off-by: Chao Gao 
>> Signed-off-by: Lan Tianyu 
>> 
>> ---
>> v4:
>>  - use "#define" to define interrupt remapping transition faults
>>  rather than using an enum
>>  - use switch-case rather than if-else in irq_remapping_request_index()
>>  and vvtd_irq_request_sanity_check()
>>  - introduce a counter inflight_intr
>> 
>> v3:
>>  - Encode map_guest_page()'s error into void* to avoid using another 
>> parameter
>> ---
>>  xen/drivers/passthrough/vtd/iommu.h |  15 +++
>>  xen/drivers/passthrough/vtd/vvtd.c  | 219 
>> 
>>  2 files changed, 234 insertions(+)
>> 
>> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
>> b/xen/drivers/passthrough/vtd/iommu.h
>> index 9c59aeb..82edd2a 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.h
>> +++ b/xen/drivers/passthrough/vtd/iommu.h
>> @@ -216,6 +216,15 @@
>>  #define dma_frcd_source_id(c) (c & 0x)
>>  #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */
>>  
>> +/* Interrupt remapping transition faults */
>> +#define VTD_FR_IR_REQ_RSVD  0x20
>> +#define VTD_FR_IR_INDEX_OVER0x21
>> +#define VTD_FR_IR_ENTRY_P   0x22
>> +#define VTD_FR_IR_ROOT_INVAL0x23
>> +#define VTD_FR_IR_IRTE_RSVD 0x24
>> +#define VTD_FR_IR_REQ_COMPAT0x25
>> +#define VTD_FR_IR_SID_ERR   0x26
>> +
>>  /*
>>   * 0: Present
>>   * 1-11: Reserved
>> @@ -356,6 +365,12 @@ struct iremap_entry {
>>  };
>>  
>>  /*
>> + * When VT-d doesn't enable extended interrupt mode, hardware interprets
>> + * 8-bits ([15:8]) of Destination-ID field in the IRTEs.
>> + */
>> +#define IRTE_xAPIC_DEST_MASK 0xff00
>> +
>> +/*
>>   * Posted-interrupt descriptor address is 64 bits with 64-byte aligned, only
>>   * the upper 26 bits of lest significiant 32 bits is available.
>>   */
>> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
>> b/xen/drivers/passthrough/vtd/vvtd.c
>> index 06e522a..927e715 100644
>> --- a/xen/drivers/passthrough/vtd/vvtd.c
>> +++ b/xen/drivers/passthrough/vtd/vvtd.c
>> @@ -22,11 +22,15 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>>  
>>  #include "iommu.h"
>> +#include "vtd.h"
>>  
>>  /* Supported capabilities by vvtd */
>>  #define VVTD_MAX_CAPS VIOMMU_CAP_IRQ_REMAPPING
>> @@ -52,6 +56,8 @@ struct vvtd {
>>  uint64_t base_addr;
>>  /* Point back to the owner domain */
>>  struct domain *domain;
>> +/* # of in-flight interrupts */
>> +atomic_t inflight_intr;
>>  
>>  struct hvm_hw_vvtd hw;
>>  void *irt_base;
>> @@ -181,6 +187,109 @@ static void unmap_guest_pages(void *va, uint32_t nr)
>>  put_page_and_type(mfn_to_page(mfn[i]));
>>  }
>>  
>> +static int vvtd_delivery(struct domain *d, uint8_t vector,
>> + uint32_t dest, bool dest_mode,
>> + uint8_t delivery_mode, uint8_t trig_mode)
>> +{
>> +struct vlapic *target;
>> +struct vcpu *v;
>> +
>> +switch ( delivery_mode )
>> +{
>> +case dest_LowestPrio:
>> +target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
>> +if ( target != NULL )
>> +{
>> +vvtd_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
>> +   vlapic_domain(target)->domain_id,
>> +   vlapic_vcpu(target)->vcpu_id,
>> +   delivery_mode, vector, trig_mode);
>> +vlapic_set_irq(target, vector, trig_mode);
>> +break;
>> +}
>> +vvtd_debug("d%d: null round robin: vector=%02x\n",
>> +   d->domain_id, vector);
>> +break;
>> +
>> +case dest_Fixed:
>> +for_each_vcpu ( d, v )
>> +if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, 
>> dest_mode) )
>> +{
>> +vvtd_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
>> +   v->domain->domain_id,

Re: [Xen-devel] [PATCH v4 11/28] x86/vvtd: Process interrupt remapping request

2018-02-09 Thread Roger Pau Monné 
On Fri, Nov 17, 2017 at 02:22:18PM +0800, Chao Gao wrote:
> When a remapping interrupt request arrives, remapping hardware computes the
> interrupt_index per the algorithm described in VTD spec
> "Interrupt Remapping Table", interprets the IRTE and generates a remapped
> interrupt request.
> 
> This patch introduces viommu_handle_irq_request() to emulate the process how
> remapping hardware handles a remapping interrupt request. This patch
> also introduces a counter inflight_intr, which is used to count the number
> of interrupt are being handled. The reason why we should have this
> counter is VT-d hardware should drain in-flight interrups before setting
> flags to show that some operations are completed. These operations
> include enabling interrupt remapping and performing a kind of invalidation
> requests. In vvtd, we also try to drain in-flight interrupts by waiting
> the inflight_intr is decreased to 0.
> 
> Signed-off-by: Chao Gao 
> Signed-off-by: Lan Tianyu 
> 
> ---
> v4:
>  - use "#define" to define interrupt remapping transition faults
>  rather than using an enum
>  - use switch-case rather than if-else in irq_remapping_request_index()
>  and vvtd_irq_request_sanity_check()
>  - introduce a counter inflight_intr
> 
> v3:
>  - Encode map_guest_page()'s error into void* to avoid using another parameter
> ---
>  xen/drivers/passthrough/vtd/iommu.h |  15 +++
>  xen/drivers/passthrough/vtd/vvtd.c  | 219 
> 
>  2 files changed, 234 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.h 
> b/xen/drivers/passthrough/vtd/iommu.h
> index 9c59aeb..82edd2a 100644
> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -216,6 +216,15 @@
>  #define dma_frcd_source_id(c) (c & 0x)
>  #define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */
>  
> +/* Interrupt remapping transition faults */
> +#define VTD_FR_IR_REQ_RSVD  0x20
> +#define VTD_FR_IR_INDEX_OVER0x21
> +#define VTD_FR_IR_ENTRY_P   0x22
> +#define VTD_FR_IR_ROOT_INVAL0x23
> +#define VTD_FR_IR_IRTE_RSVD 0x24
> +#define VTD_FR_IR_REQ_COMPAT0x25
> +#define VTD_FR_IR_SID_ERR   0x26
> +
>  /*
>   * 0: Present
>   * 1-11: Reserved
> @@ -356,6 +365,12 @@ struct iremap_entry {
>  };
>  
>  /*
> + * When VT-d doesn't enable extended interrupt mode, hardware interprets
> + * 8-bits ([15:8]) of Destination-ID field in the IRTEs.
> + */
> +#define IRTE_xAPIC_DEST_MASK 0xff00
> +
> +/*
>   * Posted-interrupt descriptor address is 64 bits with 64-byte aligned, only
>   * the upper 26 bits of lest significiant 32 bits is available.
>   */
> diff --git a/xen/drivers/passthrough/vtd/vvtd.c 
> b/xen/drivers/passthrough/vtd/vvtd.c
> index 06e522a..927e715 100644
> --- a/xen/drivers/passthrough/vtd/vvtd.c
> +++ b/xen/drivers/passthrough/vtd/vvtd.c
> @@ -22,11 +22,15 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>  
>  #include "iommu.h"
> +#include "vtd.h"
>  
>  /* Supported capabilities by vvtd */
>  #define VVTD_MAX_CAPS VIOMMU_CAP_IRQ_REMAPPING
> @@ -52,6 +56,8 @@ struct vvtd {
>  uint64_t base_addr;
>  /* Point back to the owner domain */
>  struct domain *domain;
> +/* # of in-flight interrupts */
> +atomic_t inflight_intr;
>  
>  struct hvm_hw_vvtd hw;
>  void *irt_base;
> @@ -181,6 +187,109 @@ static void unmap_guest_pages(void *va, uint32_t nr)
>  put_page_and_type(mfn_to_page(mfn[i]));
>  }
>  
> +static int vvtd_delivery(struct domain *d, uint8_t vector,
> + uint32_t dest, bool dest_mode,
> + uint8_t delivery_mode, uint8_t trig_mode)
> +{
> +struct vlapic *target;
> +struct vcpu *v;
> +
> +switch ( delivery_mode )
> +{
> +case dest_LowestPrio:
> +target = vlapic_lowest_prio(d, NULL, 0, dest, dest_mode);
> +if ( target != NULL )
> +{
> +vvtd_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
> +   vlapic_domain(target)->domain_id,
> +   vlapic_vcpu(target)->vcpu_id,
> +   delivery_mode, vector, trig_mode);
> +vlapic_set_irq(target, vector, trig_mode);
> +break;
> +}
> +vvtd_debug("d%d: null round robin: vector=%02x\n",
> +   d->domain_id, vector);
> +break;
> +
> +case dest_Fixed:
> +for_each_vcpu ( d, v )
> +if ( vlapic_match_dest(vcpu_vlapic(v), NULL, 0, dest, dest_mode) 
> )
> +{
> +vvtd_debug("d%d: dest=v%d dlm=%x vector=%d trig_mode=%d\n",
> +   v->domain->domain_id, v->vcpu_id,
> +   delivery_mode, vector, trig_mode);
> +vlapic_set_irq(vcpu_vlapic(v), vector, trig_mode);
> +}
> +break;
> +
> +case dest_NMI:
>