Re: [PATCH V3] xen: Fix BUFIOREQ evtchn init for a stubdom.

[Anthony PERARD <anthony.perard@citrix.com>]

On 02/07/12 16:58, Jan Beulich wrote:
>>>> On 02.07.12 at 17:41, Anthony PERARD <anthony.perard@citrix.com> wrote:
>> This is a missing part from the previous patch that add the BUFIOREQ_EVTCHN
>> parameter. This patch changes the ownership of the buifioreq event channel
>> to
>> the stubdom (when HVM_PARAM_DM_DOMAIN is set within the stubdom).
>>
>> This patch introduces an helper to replace a xen port.
>>
>> This fix the initialization of QEMU inside the stubdomain.
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>> ---
>> Change v3:
>>    For the bufioreq replacement:
>>    - the code is now out of the vcpu loop
>>    - the code does not take a lock anymore
>>    - rename int *port to int *p_port
>> Change v2:
>>    - an helper
>>    - the replacement of the buferioreq evtchn is inside iorp->lock now.
>>
>>   xen/arch/x86/hvm/hvm.c |   32 ++++++++++++++++++++++----------
>>   1 files changed, 22 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index e0d495d..c2dfa73 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3664,6 +3664,21 @@ static int hvmop_flush_tlb_all(void)
>>       return 0;
>>   }
>>
>> +static int hvm_replace_event_channel(struct vcpu *v, uint64_t remote_domid,
>> +                                     int *p_port)
>> +{
>> +    int old_port, new_port;
>> +
>> +    new_port = alloc_unbound_xen_event_channel(v, remote_domid, NULL);
>> +    if ( new_port < 0 )
>> +        return new_port;
>> +
>> +    /* xchg() ensures that only we free_xen_event_channel() */
>> +    old_port = xchg(p_port, new_port);
>> +    free_xen_event_channel(v, old_port);
>> +    return 0;
>> +}
>> +
>>   long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE(void) arg)
>>
>>   {
>> @@ -3775,19 +3790,16 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE(void) arg)
>>                   rc = 0;
>>                   domain_pause(d); /* safe to change per-vcpu xen_port */
>>                   iorp = &d->arch.hvm_domain.ioreq;
>> +                if (d->vcpu[0])
>> +                    hvm_replace_event_channel(d->vcpu[0], a.value,
>> +                        (int*)&d->vcpu[0]->domain->arch.hvm_domain.params[HVM_PARAM_BUFIOREQ_EVTCHN]);
>
> Did I overlook this in v2? You clearly need to handle the error
> case here (it is being handled, albeit - but that's not your patch's
> fault - only in a rudimentary way, inside the loop).

Well, if there is an error with the replace, it just break the for_each 
loop and do domain_unpause. So my guest was to leave it fail a second 
time :).

But here, how should I handle the error case? If there is an error, 
should I not go into the for_each and go strait to domain_unpause (with 
the rc value set)?

>>                   for_each_vcpu ( d, v )
>>                   {
>> -                    int old_port, new_port;
>> -                    new_port = alloc_unbound_xen_event_channel(
>> -                        v, a.value, NULL);
>> -                    if ( new_port < 0 )
>> -                    {
>> -                        rc = new_port;
>> +                    rc = hvm_replace_event_channel(v, a.value,
>> +
>> &v->arch.hvm_vcpu.xen_port);
>> +                    if ( rc )
>>                           break;
>> -                    }
>> -                    /* xchg() ensures that only we free_xen_event_channel()
>> */
>> -                    old_port = xchg(&v->arch.hvm_vcpu.xen_port, new_port);
>> -                    free_xen_event_channel(v, old_port);
>> +
>>                       spin_lock(&iorp->lock);
>>                       if ( iorp->va != NULL )
>>                           get_ioreq(v)->vp_eport = v->arch.hvm_vcpu.xen_port;
>> --
>> Anthony PERARD
>
>
>


-- 
Anthony PERARD