Re: [Xen-devel] [PATCH v4 18/28] x86/vioapic: Hook interrupt delivery of vIOAPIC

2018-02-23 Thread Tian, Kevin
> From: Gao, Chao
> Sent: Saturday, February 24, 2018 9:51 AM
> 
> On Mon, Feb 12, 2018 at 02:54:02PM +, Roger Pau Monné wrote:
> >On Fri, Nov 17, 2017 at 02:22:25PM +0800, Chao Gao wrote:
> >> When irq remapping is enabled, IOAPIC Redirection Entry may be in
> remapping
> >> format. If that, generate an irq_remapping_request and call the
> common
> >
> >"If that's the case, ..."
> >
> >> VIOMMU abstraction's callback to handle this interrupt request. Device
> >> model is responsible for checking the request's validity.
> >
> >What does this exactly mean? Device model is not involved in what the
> >guest writes to the vIOAPIC RTE, so it's impossible for the device
> >model to validate this in any way.
> 
> How about this description:
> When irq remapping is enabled, IOAPIC Redirection Entry may be in
> remapping
> format. If that's the case, an irq_remapping_request will be generated and
> IOMMU-specific handler deals with this request. IOMMU-specific handler
> will check whether the request is valid or not, report error via
> IOMMU-specific machanism if invalid or otherwise transform the request
> to an
> interrupt info (including interrupt destination, vector and trigger mode
> etc.) according to IRT.
> 

the description should match what this patch actually does.
detail about how caller works should be left to the patch where
the caller is introduced. Here imo the subject line is already
clear enough... isn't it? 

Thanks
Kevin

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

Re: [Xen-devel] [PATCH v4 18/28] x86/vioapic: Hook interrupt delivery of vIOAPIC

2018-02-23 Thread Chao Gao
On Mon, Feb 12, 2018 at 02:54:02PM +, Roger Pau Monné wrote:
>On Fri, Nov 17, 2017 at 02:22:25PM +0800, Chao Gao wrote:
>> When irq remapping is enabled, IOAPIC Redirection Entry may be in remapping
>> format. If that, generate an irq_remapping_request and call the common
>
>"If that's the case, ..."
>
>> VIOMMU abstraction's callback to handle this interrupt request. Device
>> model is responsible for checking the request's validity.
>
>What does this exactly mean? Device model is not involved in what the
>guest writes to the vIOAPIC RTE, so it's impossible for the device
>model to validate this in any way.

How about this description:
When irq remapping is enabled, IOAPIC Redirection Entry may be in remapping
format. If that's the case, an irq_remapping_request will be generated and
IOMMU-specific handler deals with this request. IOMMU-specific handler
will check whether the request is valid or not, report error via
IOMMU-specific machanism if invalid or otherwise transform the request to an
interrupt info (including interrupt destination, vector and trigger mode
etc.) according to IRT.

>
>> Signed-off-by: Chao Gao 
>> Signed-off-by: Lan Tianyu 
>> 
>> ---
>> v3:
>>  - use the new interface to check remapping format.
>> ---
>>  xen/arch/x86/hvm/vioapic.c   | 9 +
>>  xen/include/asm-x86/viommu.h | 9 +
>>  2 files changed, 18 insertions(+)
>> 
>> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
>> index 97b419f..0f20e3f 100644
>> --- a/xen/arch/x86/hvm/vioapic.c
>> +++ b/xen/arch/x86/hvm/vioapic.c
>> @@ -30,6 +30,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -387,9 +388,17 @@ static void vioapic_deliver(struct hvm_vioapic 
>> *vioapic, unsigned int pin)
>>  struct vlapic *target;
>>  struct vcpu *v;
>>  unsigned int irq = vioapic->base_gsi + pin;
>> +struct arch_irq_remapping_request request;
>>  
>>  ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
>>  
>> +irq_request_ioapic_fill(&request, vioapic->id, 
>> vioapic->redirtbl[pin].bits);
>> +if ( viommu_check_irq_remapping(d, &request) )
>> +{
>> +viommu_handle_irq_request(d, &request);
>> +return;
>> +}
>
>Will this compile if you disable vIOMMU in Kconfig?

Yes. Will fix this by wrapping this fragment with #ifdef and #endif.

Thanks
Chao

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

Re: [Xen-devel] [PATCH v4 18/28] x86/vioapic: Hook interrupt delivery of vIOAPIC

2018-02-12 Thread Roger Pau Monné
On Fri, Nov 17, 2017 at 02:22:25PM +0800, Chao Gao wrote:
> When irq remapping is enabled, IOAPIC Redirection Entry may be in remapping
> format. If that, generate an irq_remapping_request and call the common

"If that's the case, ..."

> VIOMMU abstraction's callback to handle this interrupt request. Device
> model is responsible for checking the request's validity.

What does this exactly mean? Device model is not involved in what the
guest writes to the vIOAPIC RTE, so it's impossible for the device
model to validate this in any way.

> Signed-off-by: Chao Gao 
> Signed-off-by: Lan Tianyu 
> 
> ---
> v3:
>  - use the new interface to check remapping format.
> ---
>  xen/arch/x86/hvm/vioapic.c   | 9 +
>  xen/include/asm-x86/viommu.h | 9 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 97b419f..0f20e3f 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -387,9 +388,17 @@ static void vioapic_deliver(struct hvm_vioapic *vioapic, 
> unsigned int pin)
>  struct vlapic *target;
>  struct vcpu *v;
>  unsigned int irq = vioapic->base_gsi + pin;
> +struct arch_irq_remapping_request request;
>  
>  ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
>  
> +irq_request_ioapic_fill(&request, vioapic->id, 
> vioapic->redirtbl[pin].bits);
> +if ( viommu_check_irq_remapping(d, &request) )
> +{
> +viommu_handle_irq_request(d, &request);
> +return;
> +}

Will this compile if you disable vIOMMU in Kconfig?

Roger.

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