Re: [Xen-devel] [PATCH v2 0/2] Remove PAGE_SIZE from public headers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/10/16 21:04, Stefano Stabellini wrote:
> On Thu, 20 Oct 2016, Andrew Cooper wrote:
>> On 20/10/16 20:09, Stefano Stabellini wrote:
>>> On Thu, 20 Oct 2016, Juergen Gross wrote:
>>>> On 19/10/16 21:21, stefano@xxxxxxxxxxx wrote:
>>>>> Reference to PAGE_SIZE slipped in to two public header files. QEMU build 
>>>>> on
>>>>> ARM64 is broken by this. PAGE_SIZE should not be used because it could
>>>>> be undefined or it could be defined differently on different operating
>>>>> systems.
>>>>>
>>>>> Stefano Stabellini (2):
>>>>>       usbif.h: replace PAGE_SIZE with USBIF_RING_SIZE
>>>>>       vscsiif.h: replace PAGE_SIZE with VSCSIIF_PAGE_SIZE
>>>>>
>>>>>  xen/include/public/io/usbif.h   | 5 +++--
>>>>>  xen/include/public/io/vscsiif.h | 3 ++-
>>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>> In principle I'm okay with these changes. But wouldn't it be
>>>> better to add
>>>>
>>>> #define XEN_RING_PAGE_SIZE 4096
>>>>
>>>> to xen/include/public/io/ring.h instead of having each interface
>>>> file its own definition?
>>> I would prefer to avoid having to spell out the page size for all the io
>>> interfaces, because the same interfaces should work fine with other page
>>> granularities, if the grant table hypercalls supported them.
>>>
>>> However in reality frontend and backend would have to agree on the page
>>> granularity to use beforehand. Some sort of consensus protocol is
>>> required. When that protocol is introduced, we could get rid of
>>> #define XEN_RING_PAGE_SIZE 4096.
>>>
>>> What do people think about this?
>> Not all current rings are 4k in size, and not all future rings should be
>> implicitly restricted to 4k.
>>
>> -1 to this introducing XEN_RING_PAGE_SIZE.  It will be more confusing
>> than helpful.
> Indeed, you are right. Since you are at it, feel like acking the current
> patches? :-)

Good point.  Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>,
although you also need a release ack at this point.

~Andrew

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