Re: [Xen-devel] [PATCH 1/2] altp2m: Merge p2m_set_altp2m_mem_access and p2m_set_mem_access

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.16 at 15:34, <tlengyel@xxxxxxxxxxx> wrote:
> On Jan 28, 2016 6:18 AM, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>
>> >>> On 27.01.16 at 21:06, <tlengyel@xxxxxxxxxxx> wrote:
>> > --- a/xen/include/public/hvm/hvm_op.h
>> > +++ b/xen/include/public/hvm/hvm_op.h
>> > @@ -431,18 +431,6 @@ struct xen_hvm_altp2m_view {
>> >  typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
>> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
>> >
>> > -struct xen_hvm_altp2m_set_mem_access {
>> > -    /* view */
>> > -    uint16_t view;
>> > -    /* Memory type */
>> > -    uint16_t hvmmem_access; /* xenmem_access_t */
>> > -    uint32_t pad;
>> > -    /* gfn */
>> > -    uint64_t gfn;
>> > -};
>> > -typedef struct xen_hvm_altp2m_set_mem_access 
>> > xen_hvm_altp2m_set_mem_access_t;
>> > -DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>
>> This is a guest visible interface, and hence can't be removed (nor
>> be replaced by a tools only one).
> 
> What is your suggestion?

At the very least keep the old interface, perhaps backed by the
new implementation.

>> > --- a/xen/include/public/memory.h
>> > +++ b/xen/include/public/memory.h
>> > @@ -423,11 +423,14 @@ struct xen_mem_access_op {
>> >      /* xenmem_access_t */
>> >      uint8_t access;
>> >      domid_t domid;
>> > +    uint16_t altp2m_idx;
>>
>> So this is a tools only interface, yes. But it's not versioned (other
>> than e.g. domctl and sysctl), so altering the interface structure is
>> at least fragile.
>>
>> And then, with this ...
>>
>> > --- a/xen/include/xen/p2m-common.h
>> > +++ b/xen/include/xen/p2m-common.h
>> > @@ -49,7 +49,8 @@ int unmap_mmio_regions(struct domain *d,
>> >   * If gfn == INVALID_GFN, sets the default access type.
>> >   */
>> >  long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>> > -                        uint32_t start, uint32_t mask, xenmem_access_t 
>> > access);
>> > +                        uint32_t start, uint32_t mask, xenmem_access_t 
>> > access,
>> > +                        unsigned long altp2m_idx);
>>
>> ... why "unsigned long" instead of e.g. "unsigned int" here?
> 
> These were used interchangebly in the code, I've just picked on. The max
> value it can legitimetly have is 10 so there is not much difference in
> going with int instead of long.

Since "int" is slightly less expensive to operate on, please use it
whenever possible.

Jan


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