Re: [Xen-devel] [PATCH v4] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 9/19/18 6:29 PM, Tamas K Lengyel wrote:
> On Mon, Sep 3, 2018 at 9:47 AM Adrian Pop <apop@xxxxxxxxxxxxxxx> wrote:
>> diff --git a/xen/include/public/hvm/hvm_op.h 
>> b/xen/include/public/hvm/hvm_op.h
>> index bbba99e5f5..bbb0aa984a 100644
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -234,6 +234,7 @@ 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);
>>
>> +#if __XEN_INTERFACE_VERSION__ < 0x00040a00
>>  struct xen_hvm_altp2m_set_mem_access {
>>      /* view */
>>      uint16_t view;
>> @@ -245,6 +246,19 @@ struct xen_hvm_altp2m_set_mem_access {
>>  };
>>  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);
>> +#endif /* __XEN_INTERFACE_VERSION__ < 0x00040a00 */
>> +
>> +struct xen_hvm_altp2m_mem_access {
>> +    /* view */
>> +    uint16_t view;
>> +    /* Memory type */
>> +    uint16_t hvmmem_access; /* xenmem_access_t */
> 
> A structure name with "mem_access" having a variable name
> "hvmmem_access" and a comment saying "xenmem_access". This is
> confusing. I understand that you copy/pasted this from the existing op
> but it doesn't look good. Perhaps fix both if we are touching it?
> Also, in public/memory.h the width of access is uint8_t so I'm not
> sure why the discrepancy.

The uint16_t has been probably chosen to be that because it simplifies
padding. I can rename "hvmmem_access" to simply "access". Should I just
remove the comment, or do you think something else is more appropriate?


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel