Re: [Xen-devel] [PATCH v4 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.06.16 at 11:32, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>> On 6/14/2016 6:45 PM, Jan Beulich wrote:
>>>>>>> On 19.05.16 at 11:05, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>>>> @@ -914,6 +916,45 @@ int hvm_unmap_io_range_from_ioreq_server(struct 
>>>>> domain
>>> *d, ioservid_t id,
>>>>>        return rc;
>>>>>    }
>>>>>    
>>>>> +int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
>>>>> +                                     uint16_t type, uint32_t flags)
>>>> I see no reason why both can't be unsigned int.
>>> Parameter type is passed in from the type field inside struct
>>> xen_hvm_map_mem_type_to_ioreq_server,
>>> which is a uint16_t, followed with a uint16_t pad. Now I am wondering,
>>> may be we can just remove the pad
>>> field in this structure and just define type as uint32_t.
>> I think keeping the interface structure unchanged is the desirable
>> route here. What I dislike is the passing around of non-natural
>> width types, which is more expensive in terms of processing. I.e.
>> as long as a fixed width type (which is necessary to be used in
>> the public interface) fits in "unsigned int", that should be the
>> respective internal type. Otherwise "unsigned long" etc.
>>
>> There are cases where even internally we indeed want to use
>> fixed width types, and admittedly there are likely far more cases
>> where internally fixed width types get used without good reason,
>> but just like everywhere else - let's please not make this worse.
>> IOW please use fixed width types only when you really need them.
> OK. I can keep the interface, and using uint32_t type in the internal 
> routine
> would means a implicit type conversion from uint16_6, which I do not think
> is a problem.

Just to reiterate: Unless there is a specific need, please avoid
fixed width integer types for any internal use.

>>>>> @@ -94,8 +96,16 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, 
>>>>> mfn_t mfn,
>>>>>        default:
>>>>>            return flags | _PAGE_NX_BIT;
>>>>>        case p2m_grant_map_ro:
>>>>> -    case p2m_ioreq_server:
>>>>>            return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>>>> +    case p2m_ioreq_server:
>>>>> +    {
>>>>> +        flags |= P2M_BASE_FLAGS | _PAGE_RW;
>>>>> +
>>>>> +        if ( p2m->ioreq.flags & P2M_IOREQ_HANDLE_WRITE_ACCESS )
>>>>> +            return flags & ~_PAGE_RW;
>>>>> +        else
>>>>> +            return flags;
>>>>> +    }
>>>> Same here (for the missing _PAGE_NX) plus no need for braces.
>>> I'll remove the brace. And we do not need to set the _PAGE_NX_BIT, like
>>> the p2m_ram_ro case I guess.
>> I hope you mean the inverse: You should set _PAGE_NX_BIT here.
> Oh, right. I meant the reverse. Thanks for the remind. :)
> And I have a question,  here in p2m_type_to_flags(), I saw current code 
> uses _PAGE_NX_BIT
> to disable the executable permission,  and I wonder, why don't we choose 
> the _PAGE_NX,
> which is defined as:
> 
> #define _PAGE_NX       (cpu_has_nx ? _PAGE_NX_BIT : 0)
> 
> How do we know for sure that bit 63 from pte is not a reserved one 
> without checking
> the cpu capability(the cpu_has_nx)? Is there any other reasons, i.e. the 
> page tables might
> be shared with IOMMU?

Please wait for Andrew to confirm this (or correct me) - there are
some differences between AMD and Intel, and iirc the bit gets
ignored by AMD when NX is off.

>>>>> +struct xen_hvm_map_mem_type_to_ioreq_server {
>>>>> +    domid_t domid;      /* IN - domain to be serviced */
>>>>> +    ioservid_t id;      /* IN - ioreq server id */
>>>>> +    uint16_t type;      /* IN - memory type */
>>>>> +    uint16_t pad;
>>>> This field does not appear to get checked in the handler.
>>> I am now wondering, how about we remove this pad field and define type
>>> as uint32_t?
>> As above - I think the current layout is fine. But I'm also not heavily
>> opposed to using uint32_t here. It's not a stable interface anyway
>> (and I already have a series mostly ready to split off all control
>> operations from the HVMOP_* ones, into a new HVMCTL_* set,
>> which will make all of them interface-versioned).
> 
> I'd like to keep this interface. BTW, you mentioned "this field does not 
> appear to
> get checked in the handler", do you mean we need to check the pad in the 
> handler?

Yes.

> And why?

In order to be able to later assign meaning to it without breaking
existing users.

Jan

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