Re: [Xen-devel] [PATCH v2] x86/vpt: guarantee the return value of pt_update_irq() set in vIRR or PIR

[Chao Gao <chao.gao@xxxxxxxxx>]

On Mon, Oct 16, 2017 at 08:26:09AM -0600, Jan Beulich wrote:
>>>> On 16.10.17 at 15:13, <chao.gao@xxxxxxxxx> wrote:
>> On Mon, Oct 16, 2017 at 07:15:16AM -0600, Jan Beulich wrote:
>>>>>> On 13.10.17 at 07:10, <chao.gao@xxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/hvm/irq.c
>>>> +++ b/xen/arch/x86/hvm/irq.c
>>>> @@ -168,11 +168,13 @@ void hvm_gsi_deassert(struct domain *d, unsigned int 
>>>> gsi)
>>>>      spin_unlock(&d->arch.hvm_domain.irq_lock);
>>>>  }
>>>>  
>>>> -void hvm_isa_irq_assert(
>>>> -    struct domain *d, unsigned int isa_irq)
>>>> +int hvm_isa_irq_assert(struct domain *d, unsigned int isa_irq,
>>>> +                       int (*get_vector)(const struct domain *d,
>>>> +                                         unsigned int gsi))
>>>>  {
>>>>      struct hvm_irq *hvm_irq = hvm_domain_irq(d);
>>>>      unsigned int gsi = hvm_isa_irq_to_gsi(isa_irq);
>>>> +    int vector = 0;
>>>
>>>Why zero (which is valid aiui) instead of e.g. -1?
>> 
>> vector also serves as the return value. I want to return 0 if no
>> callback is set.  And the callback, get_vector, can override the return
>> value. Do you think it is reasonable?
>
>Why "also" - being the return value is the only purpose of "vector".
>And as said - zero is a valid vector, and I wouldn't like to see the
>function return a valid but meaningless vector number.

But if no callback is set, would it be a little weird to return -1 which
always means failure? Considering no caller would be confused by the
return value (since except the caller introduced by this patch, no one
would check the return value), I don't insist on this.

>
>>>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>>>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>>>> @@ -109,6 +109,11 @@ static inline int pi_test_and_set_pir(int vector, 
>>>> struct pi_desc *pi_desc)
>>>>      return test_and_set_bit(vector, pi_desc->pir);
>>>>  }
>>>>  
>>>> +static inline int pi_test_pir(int vector, const struct pi_desc *pi_desc)
>>>
>>>This should not be a signed quantity - uint8_t or unsigned int
>>>please.
>> 
>> Yes.
>
>I.e. meaning you're fine with either variant, leaving it up to me
>which one to use?

Yes, both of them are ok to me.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel