Re: [PATCH v9 5/5] x86/ioreq server: Synchronously reset outstanding p2m_ioreq_server entries when an ioreq server unmaps.

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 3/23/2017 5:02 PM, Jan Beulich wrote:
>>>> On 23.03.17 at 04:23, <yu.c.zhang@linux.intel.com> wrote:
>> On 3/22/2017 10:39 PM, Jan Beulich wrote:
>>>>>> On 21.03.17 at 03:52, <yu.c.zhang@linux.intel.com> wrote:
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -385,16 +385,51 @@ static int dm_op(domid_t domid,
>>>>    
>>>>        case XEN_DMOP_map_mem_type_to_ioreq_server:
>>>>        {
>>>> -        const struct xen_dm_op_map_mem_type_to_ioreq_server *data =
>>>> +        struct xen_dm_op_map_mem_type_to_ioreq_server *data =
>>>>                &op.u.map_mem_type_to_ioreq_server;
>>>> +        unsigned long first_gfn = data->opaque;
>>>> +        unsigned long last_gfn;
>>>> +
>>>> +        const_op = false;
>>>>    
>>>>            rc = -EOPNOTSUPP;
>>>>            /* Only support for HAP enabled hvm. */
>>>>            if ( !hap_enabled(d) )
>>>>                break;
>>>>    
>>>> -        rc = hvm_map_mem_type_to_ioreq_server(d, data->id,
>>>> -                                              data->type, data->flags);
>>>> +        if ( first_gfn == 0 )
>>>> +            rc = hvm_map_mem_type_to_ioreq_server(d, data->id,
>>>> +                                                  data->type, data->flags);
>>>> +        /*
>>>> +         * Iterate p2m table when an ioreq server unmaps from
>> p2m_ioreq_server,
>>>> +         * and reset the remaining p2m_ioreq_server entries back to
>> p2m_ram_rw.
>>>> +         */
>>>> +        if ( (first_gfn > 0) || (data->flags == 0 && rc == 0) )
>>> Instead of putting the rc check on the right side, please do
>>>
>>>           if ( rc == 0 && (first_gfn > 0) || data->flags == 0) )
>>>
>>> That'll require setting rc to zero in an else to the previous if(),
>>> but that's needed anyway afaics in order to not return
>>> -EOPNOTSUPP once no further continuation is necessary.
>>>
>>> I further wonder why the if() here needs to look at first_gfn at
>>> all - data->flags is supposed to remain at zero for continuations
>>> (unless we have a misbehaving caller, in which case it'll harm
>>> the guest only afaict). It seems to me, however, that this may
>>> have been discussed once already, a long time ago. I'm sorry
>>> for not remembering the outcome, if so.
>> We have not discussed this. Our previous discussion is about the if
>> condition before
>> calling hvm_map_mem_type_to_ioreq_server(). :-)
>>
>> Maybe above code should be changed to:
>> @@ -400,11 +400,14 @@ static int dm_op(domid_t domid,
>>            if ( first_gfn == 0 )
>>                rc = hvm_map_mem_type_to_ioreq_server(d, data->id,
>>                                                      data->type,
>> data->flags);
>> +       else
>> +           rc = 0;
>> +
>>            /*
>>             * Iterate p2m table when an ioreq server unmaps from p2m_ioreq_server,
>>             * and reset the remaining p2m_ioreq_server entries back to p2m_ram_rw.
>>             */
>> -        if ( (first_gfn > 0) || (data->flags == 0 && rc == 0) )
>> +        if ( data->flags == 0 && rc == 0 )
>>            {
>>                struct p2m_domain *p2m = p2m_get_hostp2m(d);
> Yes, that's what I was trying to hint at.

Great. Thanks.
>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -1038,6 +1038,35 @@ void p2m_change_type_range(struct domain *d,
>>>>        p2m_unlock(p2m);
>>>>    }
>>>>    
>>>> +/* Synchronously modify the p2m type for a range of gfns from ot to nt. */
>>>> +void p2m_finish_type_change(struct domain *d,
>>>> +                            unsigned long first_gfn, unsigned long last_gfn,
>>> I think we'd prefer new functions to properly use gfn_t.
>> Sorry? I do not get it.
>> Paul suggested we replace last_gfn with max_nr, which sounds reasonable
>> to me. Guess you mean
>> something else?
> Indeed - even with Paul's suggestion, first_gfn would remain as a
> parameter, and it should be of type gfn_t.

Oh. I see, you mean change the type of first_gfn to gfn_t.

Thanks
Yu
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


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