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
