Re: [PATCH] stubdom: foreignmemory: Fix build after 0dbb4be739c5

[Costin Lupu <costin.lupu@xxxxxxxxx>]

On 7/27/21 4:36 PM, Andrew Cooper wrote:
> On 16/07/2021 19:28, Costin Lupu wrote:
>> On 7/13/21 6:20 PM, Juergen Gross wrote:
>>> On 13.07.21 17:15, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 13/07/2021 16:09, Juergen Gross wrote:
>>>>> On 13.07.21 16:38, Julien Grall wrote:
>>>>>> Hi Juergen,
>>>>>>
>>>>>> On 13/07/2021 15:23, Juergen Gross wrote:
>>>>>>> On 13.07.21 16:19, Julien Grall wrote:
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> On 13/07/2021 15:14, Jan Beulich wrote:
>>>>>>>>>> And I don't think it should be named XC_PAGE_*, but rather
>>>>>>>>>> XEN_PAGE_*.
>>>>>>>>> Even that doesn't seem right to me, at least in principle. There
>>>>>>>>> shouldn't
>>>>>>>>> be a build time setting when it may vary at runtime. IOW on Arm I
>>>>>>>>> think a
>>>>>>>>> runtime query to the hypervisor would be needed instead.
>>>>>>>> Yes, we want to be able to use the same userspace/OS without
>>>>>>>> rebuilding to a specific hypervisor page size.
>>>>>>> This define is used for accessing data of other domains. See the
>>>>>>> define
>>>>>>> for XEN_PAGE_SIZE in xen/include/public/io/ring.h
>>>>>>>
>>>>>>> So it should be a constant (minimal) page size for all hypervisors and
>>>>>>> guests of an architecture.
>>>>>> Do you mean the maximum rather than minimal? If you use the minimal
>>>>>> (4KB), then you would not be able to map the page in the stage-2 if
>>>>>> the hypervisor is using 64KB.
>>>>> But this would mean that the current solution to use XC_PAGE_SIZE is
>>>>> wrong, as this is 4k.
>>>> The existing ABI is implicitely based on using the hypervisor page
>>>> granularity (currently 4KB).
>>>>
>>>> There is really no way we can support existing guest on 64KB
>>>> hypervisor. But if we were going to break them, then we should
>>>> consider to do one of the following option:
>>>>     1) use 64KB page granularity for ABI
>>>>     2) query the hypervisor page granularity at runtime
>>>>
>>>> The ideal is 2) because it is more scalable for the future. We also
>>>> need to consider to extend the PV protocol so the backend and frontend
>>>> can agree on the page size.
>>> I absolutely agree, but my suggestion was to help finding a proper way
>>> to cleanup the current interface mess. And this should be done the way
>>> I suggested IMO.
>>>
>>> A later interface extension for future guests can still be done on top
>>> of that.
>> Alright, let's have a little recap to see if I got it right and to agree
>> on the next steps. There are 2 proposed solutions, let's say a static
>> one and a dynamic one.
>>
>> 1) Static solution (proposed by Juergen)
>> - We define XEN_PAGE_* values in a xen/include/public/arch-*/*.h header.
>> - Q: Should we define a new header for that? page.h or page_size.h are
>> ok as new filenames?
>>
>> Pros:
>> - We fix the interfaces mess and we can get rid of xenctrl lib
>> dependency for some of the libs that need only the XEN_PAGE_* definitions.
>> - It's faster to implement, with fewer changes.
>>
>> Cons:
>> - Well, it's static, it doesn't allow the hypervisor to provide
>> different values for different guests.
>>
>>
>> 2) Dynamic solution (proposed by Jan and Julien)
>> We get the value(s) by calling a hypcall, probably as a query related to
>> some guest domain.
>>
>> Pros:
>> - It's dynamic and scalable. We would support different values for
>> different guests.
>>
>> Cons:
>> - More difficult to implement. It changes the paradigm in the toolstack
>> libs, every occurrence of XC_PAGE_* would have to be amended. Moreover,
>> we might want to make the hypcall once and save the value for later
>> (probably several toolstack structures should be extended for that)
>>
>>
>> I searched for the occurrences of XC_PAGE_* in the toolstack libs and
>> it's a *lot* of them. IMHO I think we should pick the static solution
>> for now, considering that it would be faster to implement. Please let me
>> know if this is OK or not. Any comments are appreciated.
> 
> The immediate problem needing fixing is the stable libraries inclusion
> of unstable headers - specifically, the inclusion of <xenctrl.h>.
> 
> Juergen's proposal moves the existing constant to a more appropriate
> location, and specifically, a location where its value is stable.
> 
> It does not change the ABI.  It merely demonstrates that the existing
> ABI is broken, and thus is absolutely a step in the right direction.
> 
> This is the approach you should take in the short term, and needs
> sorting before 4.16 ships.
> 
> 
> The dynamic solution, while preferable in the longterm, is far more
> complicated than even described thus far, and is not as simple as just
> having a hypercall and using that value.
> 
> Among other things, it requires coordination with the dom0 kernel as to
> its pagetable setup, and with Xen's choice of pagetable size for dom0,
> which may not be the same as domU's.  It is a large quantity of work,
> very invasive to the existing APIs/ABIs, and stands no chance at all of
> being ready for 4.16.

Thanks for clearing this, Andrew. What is the deadline for the 4.16
release? Where can I find the release calendar?

Costin