| Message ID | 1494424994-26232-9-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: > The "retrieving mapping" code has never executed since > iommu_use_hap_pt(d) always returned true on ARM so far. But, with > introducing the non-shared IOMMU patch series we can no longer keep > this code as is due to the lack of M2P support. > > In order to retain the current behavior for x86 this code was completely > moved to x86 specific part. > For ARM we just need to populate IOMMU page table if need_iommu flag > is already set and the IOMMU is non-shared. > > So, the logic on ARM was changed a bit, but no functional change for x86. > > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Julien Grall <julien.grall@arm.com> > > --- > Changes in V1: > - Clarify patch description. My prior objection stands: You don't make clear why architecture- agnostic code needs to be moved into an architecture-specific file. Of course once you give a proper reason in the description, the change itself looks mostly fine - just one remark: > +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) > +{ > + const struct domain_iommu *hd = dom_iommu(d); This isn't used outside of ... > + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) > + { ... this if(), so should be moved here. Jan
Hi, Jan. On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >> The "retrieving mapping" code has never executed since >> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >> introducing the non-shared IOMMU patch series we can no longer keep >> this code as is due to the lack of M2P support. >> >> In order to retain the current behavior for x86 this code was completely >> moved to x86 specific part. >> For ARM we just need to populate IOMMU page table if need_iommu flag >> is already set and the IOMMU is non-shared. >> >> So, the logic on ARM was changed a bit, but no functional change for x86. >> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in V1: >> - Clarify patch description. > > My prior objection stands: You don't make clear why architecture- > agnostic code needs to be moved into an architecture-specific file. Is the following description more cleaner? Although this code looks like architecture-agnostic code, there is only one thing that prevents it to be *completely* architecture-agnostic -> mfn_to_gmfn helper. As we don't have an M2P on ARM, it always returns incoming mfn. And if domain is not direct mapped we will get into the trouble using that. We didn't care about this code before because it has never been executed (iommu_use_hap_pt(d) always returned true on ARM so far). But, with introducing the non-shared IOMMU patch series we can no longer keep this code as is since it will be executed for non-shared IOMMU. So, move this code to x86-specific file in order not to expose a possible bug. > Of course once you give a proper reason in the description, the > change itself looks mostly fine - just one remark: > >> +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) >> +{ >> + const struct domain_iommu *hd = dom_iommu(d); > > This isn't used outside of ... > >> + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) >> + { > > ... this if(), so should be moved here. Agree. Will move. > > Jan >
>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote: > On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>> The "retrieving mapping" code has never executed since >>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >>> introducing the non-shared IOMMU patch series we can no longer keep >>> this code as is due to the lack of M2P support. >>> >>> In order to retain the current behavior for x86 this code was completely >>> moved to x86 specific part. >>> For ARM we just need to populate IOMMU page table if need_iommu flag >>> is already set and the IOMMU is non-shared. >>> >>> So, the logic on ARM was changed a bit, but no functional change for x86. >>> >>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>> CC: Jan Beulich <jbeulich@suse.com> >>> CC: Julien Grall <julien.grall@arm.com> >>> >>> --- >>> Changes in V1: >>> - Clarify patch description. >> >> My prior objection stands: You don't make clear why architecture- >> agnostic code needs to be moved into an architecture-specific file. > > Is the following description more cleaner? > > Although this code looks like architecture-agnostic code, there is > only one thing that prevents it > to be *completely* architecture-agnostic -> mfn_to_gmfn helper. > As we don't have an M2P on ARM, it always returns incoming mfn. > And if domain is not direct mapped we will get into the trouble using that. > > We didn't care about this code before because it has never been executed > (iommu_use_hap_pt(d) always returned true on ARM so far). > But, with introducing the non-shared IOMMU patch series we can no longer keep > this code as is since it will be executed for non-shared IOMMU. > So, move this code to x86-specific file in order not to expose a possible bug. Yes, this helps. Jan
>>> On 12.05.17 at 17:34, <JBeulich@suse.com> wrote: >>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote: >> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>> The "retrieving mapping" code has never executed since >>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >>>> introducing the non-shared IOMMU patch series we can no longer keep >>>> this code as is due to the lack of M2P support. >>>> >>>> In order to retain the current behavior for x86 this code was completely >>>> moved to x86 specific part. >>>> For ARM we just need to populate IOMMU page table if need_iommu flag >>>> is already set and the IOMMU is non-shared. >>>> >>>> So, the logic on ARM was changed a bit, but no functional change for x86. >>>> >>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>> CC: Jan Beulich <jbeulich@suse.com> >>>> CC: Julien Grall <julien.grall@arm.com> >>>> >>>> --- >>>> Changes in V1: >>>> - Clarify patch description. >>> >>> My prior objection stands: You don't make clear why architecture- >>> agnostic code needs to be moved into an architecture-specific file. >> >> Is the following description more cleaner? >> >> Although this code looks like architecture-agnostic code, there is >> only one thing that prevents it >> to be *completely* architecture-agnostic -> mfn_to_gmfn helper. >> As we don't have an M2P on ARM, it always returns incoming mfn. >> And if domain is not direct mapped we will get into the trouble using that. >> >> We didn't care about this code before because it has never been executed >> (iommu_use_hap_pt(d) always returned true on ARM so far). >> But, with introducing the non-shared IOMMU patch series we can no longer keep >> this code as is since it will be executed for non-shared IOMMU. >> So, move this code to x86-specific file in order not to expose a possible bug. > > Yes, this helps. Having thought about this some more, what's still missing is a clear explanation why this new need of a non-stub mfn_to_gmfn() isn't finally enough of a reason to introduce an M2P on ARM. We currently have several uses already which ARM fakes one way or another: - gnttab_shared_gmfn() - gnttab_status_gmfn() - memory_exchange() - getdomaininfo() With this I think there's quite a bit of justification needed to keep going without M2P on ARM. Jan
Hi Jan, On 15/05/2017 08:20, Jan Beulich wrote: >>>> On 12.05.17 at 17:34, <JBeulich@suse.com> wrote: >>>>> On 12.05.17 at 17:25, <olekstysh@gmail.com> wrote: >>> On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 10.05.17 at 16:03, <olekstysh@gmail.com> wrote: >>>>> The "retrieving mapping" code has never executed since >>>>> iommu_use_hap_pt(d) always returned true on ARM so far. But, with >>>>> introducing the non-shared IOMMU patch series we can no longer keep >>>>> this code as is due to the lack of M2P support. >>>>> >>>>> In order to retain the current behavior for x86 this code was completely >>>>> moved to x86 specific part. >>>>> For ARM we just need to populate IOMMU page table if need_iommu flag >>>>> is already set and the IOMMU is non-shared. >>>>> >>>>> So, the logic on ARM was changed a bit, but no functional change for x86. >>>>> >>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>> CC: Julien Grall <julien.grall@arm.com> >>>>> >>>>> --- >>>>> Changes in V1: >>>>> - Clarify patch description. >>>> >>>> My prior objection stands: You don't make clear why architecture- >>>> agnostic code needs to be moved into an architecture-specific file. >>> >>> Is the following description more cleaner? >>> >>> Although this code looks like architecture-agnostic code, there is >>> only one thing that prevents it >>> to be *completely* architecture-agnostic -> mfn_to_gmfn helper. >>> As we don't have an M2P on ARM, it always returns incoming mfn. >>> And if domain is not direct mapped we will get into the trouble using that. >>> >>> We didn't care about this code before because it has never been executed >>> (iommu_use_hap_pt(d) always returned true on ARM so far). >>> But, with introducing the non-shared IOMMU patch series we can no longer keep >>> this code as is since it will be executed for non-shared IOMMU. >>> So, move this code to x86-specific file in order not to expose a possible bug. >> >> Yes, this helps. > > Having thought about this some more, what's still missing is a > clear explanation why this new need of a non-stub mfn_to_gmfn() > isn't finally enough of a reason to introduce an M2P on ARM. We > currently have several uses already which ARM fakes one way or > another: > - gnttab_shared_gmfn() This does not use mfn_to_gmfn on ARM. > - gnttab_status_gmfn() gnttab_status_gmfn() returns 0 so far. I have to look at this one. > - memory_exchange() Memory exchange does not work on ARM today and will require more work than that. When I looked at the code a couple of years ago, it was possible to drop the call to mfn_to_gmfn(). > - getdomaininfo() We could rework to store the gmfn in arch_domain. > With this I think there's quite a bit of justification needed to keep > going without M2P on ARM. As said in a previous thread, I made a quick calculation, ARM32 supports up 40-bit PA and IPA (e.g guest address), which means 28-bits of MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so we would need 2^28 * 4 = 1GiB of virtual address space and potentially physical memory. We don't have 1GB of VA space free on 32-bit right now. ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for alignment, so we would need 2^36 * 8 = 512GiB of virtual address space and potentially physical memory. While virtual address space is not a problem, the memory is a problem for embedded platform. We want Xen to be as lean as possible. So the M2P is not a solution on ARM. A better approach is to drop those calls from common code and replace by something different (as we did for gnttab_shared_mfn). Cheers,
>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: > On 15/05/2017 08:20, Jan Beulich wrote: >> Having thought about this some more, what's still missing is a >> clear explanation why this new need of a non-stub mfn_to_gmfn() >> isn't finally enough of a reason to introduce an M2P on ARM. We >> currently have several uses already which ARM fakes one way or >> another: >> - gnttab_shared_gmfn() > > This does not use mfn_to_gmfn on ARM. Right, at the price of maintaining some other helper data. >> - gnttab_status_gmfn() > > gnttab_status_gmfn() returns 0 so far. I have to look at this one. > >> - memory_exchange() > > Memory exchange does not work on ARM today and will require more work > than that. When I looked at the code a couple of years ago, it was > possible to drop the call to mfn_to_gmfn(). > >> - getdomaininfo() > > We could rework to store the gmfn in arch_domain. Which again would mean you maintain extra data in order to avoid the more general M2P. >> With this I think there's quite a bit of justification needed to keep >> going without M2P on ARM. > > As said in a previous thread, I made a quick calculation, ARM32 supports > up 40-bit PA and IPA (e.g guest address), which means 28-bits of > MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so > we would need 2^28 * 4 = 1GiB of virtual address space and potentially > physical memory. We don't have 1GB of VA space free on 32-bit right now. How come? You don't share address spaces with guests. > ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means > 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for > alignment, so we would need 2^36 * 8 = 512GiB of virtual address space > and potentially physical memory. While virtual address space is not a > problem, the memory is a problem for embedded platform. We want Xen to > be as lean as possible. You don't need to allocate all 512Gb, the table can be as sparse as present memory permits. > So the M2P is not a solution on ARM. A better approach is to drop those > calls from common code and replace by something different (as we did for > gnttab_shared_mfn). I'm of the exact opposite opinion. Or at the very least, have a mode (read: config or command line option) where ARM maintains M2P and make features like the IOMMU one here depend on being in that mode. Jan
Hi Jan, On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >> On 15/05/2017 08:20, Jan Beulich wrote: >>> Having thought about this some more, what's still missing is a >>> clear explanation why this new need of a non-stub mfn_to_gmfn() >>> isn't finally enough of a reason to introduce an M2P on ARM. We >>> currently have several uses already which ARM fakes one way or >>> another: >>> - gnttab_shared_gmfn() >> >> This does not use mfn_to_gmfn on ARM. > > Right, at the price of maintaining some other helper data. saving few MB of memory in small board and hundreds in server if we use an M2P. The choice is very easy here. > >>> - gnttab_status_gmfn() >> >> gnttab_status_gmfn() returns 0 so far. I have to look at this one. >> >>> - memory_exchange() >> >> Memory exchange does not work on ARM today and will require more work >> than that. When I looked at the code a couple of years ago, it was >> possible to drop the call to mfn_to_gmfn(). >> >>> - getdomaininfo() >> >> We could rework to store the gmfn in arch_domain. > > Which again would mean you maintain extra data in order to avoid > the more general M2P. Yes saving MBs as above. > >>> With this I think there's quite a bit of justification needed to keep >>> going without M2P on ARM. >> >> As said in a previous thread, I made a quick calculation, ARM32 supports >> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >> physical memory. We don't have 1GB of VA space free on 32-bit right now. > > How come? You don't share address spaces with guests. Below the layout for ARM32: * 0 - 12M <COMMON> * * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address * space * * 1G - 2G Xenheap: always-mapped memory * 2G - 4G Domheap: on-demand-mapped * >> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >> and potentially physical memory. While virtual address space is not a >> problem, the memory is a problem for embedded platform. We want Xen to >> be as lean as possible. > > You don't need to allocate all 512Gb, the table can be as sparse as > present memory permits. I am aware about that... The main point is reducing the footprint of Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. Likely this will increase the footprint of Xen ARM. For what benefits? Avoiding to store few byte in a non-generic way when we need it... I will comment about the IOMMU below. > >> So the M2P is not a solution on ARM. A better approach is to drop those >> calls from common code and replace by something different (as we did for >> gnttab_shared_mfn). > > I'm of the exact opposite opinion. Or at the very least, have a mode > (read: config or command line option) where ARM maintains M2P and > make features like the IOMMU one here depend on being in that mode. Well, in embedded platform you will know in advance that you will passthrough devices to a guest. So there is no point of deferring the creation of the page-tables until a device is been assigned. In server side, I would expect page-table to be shared most of the time. We might have to unshare some part of the page-tables but not everything as it is currently done on x86. So far, you didn't convince me the M2P is the right solution for ARM. We use more memory for benefits of, AFAICT, of device hotpluging (?) and been "generic". Anyway, I will let Stefano give his opinion on it. Cheers,
>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: > On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>> On 15/05/2017 08:20, Jan Beulich wrote: >>>> With this I think there's quite a bit of justification needed to keep >>>> going without M2P on ARM. >>> >>> As said in a previous thread, I made a quick calculation, ARM32 supports >>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >> >> How come? You don't share address spaces with guests. > > Below the layout for ARM32: > > > * 0 - 12M <COMMON> > * > * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM > * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address > * space > * > * 1G - 2G Xenheap: always-mapped memory > * 2G - 4G Domheap: on-demand-mapped Since Domheap hardly covers all memory, the obvious thing would seem to be to take part of that region, just like on x86 we also had to reduce the direct mapping area in order to support systems with more than 5Tb. >>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>> and potentially physical memory. While virtual address space is not a >>> problem, the memory is a problem for embedded platform. We want Xen to >>> be as lean as possible. >> >> You don't need to allocate all 512Gb, the table can be as sparse as >> present memory permits. > > I am aware about that... The main point is reducing the footprint of > Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. > > Likely this will increase the footprint of Xen ARM. For what benefits? > Avoiding to store few byte in a non-generic way when we need it... But that's the point: Generic code becomes more and more clumsy if non-generic mechanisms need to be introduced. Quite a few we've had the discussion of saving a few Mb here or there, and I've almost always been the one to ask for not wasting memory even if we have plenty, so I'm all with you on that aspect. Nevertheless there is a point where the trade-off between memory overhead and generic (i.e. easier to maintain) code crosses a boundary, and I'm simply wondering whether we aren't at that point. Jan
Hi, Jan. On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>> With this I think there's quite a bit of justification needed to keep >>>>> going without M2P on ARM. >>>> >>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>> >>> How come? You don't share address spaces with guests. >> >> Below the layout for ARM32: >> >> >> * 0 - 12M <COMMON> >> * >> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >> * space >> * >> * 1G - 2G Xenheap: always-mapped memory >> * 2G - 4G Domheap: on-demand-mapped > > Since Domheap hardly covers all memory, the obvious thing would > seem to be to take part of that region, just like on x86 we also > had to reduce the direct mapping area in order to support systems > with more than 5Tb. > >>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>> and potentially physical memory. While virtual address space is not a >>>> problem, the memory is a problem for embedded platform. We want Xen to >>>> be as lean as possible. >>> >>> You don't need to allocate all 512Gb, the table can be as sparse as >>> present memory permits. >> >> I am aware about that... The main point is reducing the footprint of >> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >> >> Likely this will increase the footprint of Xen ARM. For what benefits? >> Avoiding to store few byte in a non-generic way when we need it... > > But that's the point: Generic code becomes more and more clumsy > if non-generic mechanisms need to be introduced. Quite a few we've > had the discussion of saving a few Mb here or there, and I've almost > always been the one to ask for not wasting memory even if we have > plenty, so I'm all with you on that aspect. Nevertheless there is a > point where the trade-off between memory overhead and generic > (i.e. easier to maintain) code crosses a boundary, and I'm simply > wondering whether we aren't at that point. Is the lack of M2P support on ARM a blocker for this patch to be accepted? This patch I think is only prevents us from possible bugs in a future. Please correct me if I am wrong. > > Jan >
>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote: > On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >>> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>>> With this I think there's quite a bit of justification needed to keep >>>>>> going without M2P on ARM. >>>>> >>>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>>> >>>> How come? You don't share address spaces with guests. >>> >>> Below the layout for ARM32: >>> >>> >>> * 0 - 12M <COMMON> >>> * >>> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>> * space >>> * >>> * 1G - 2G Xenheap: always-mapped memory >>> * 2G - 4G Domheap: on-demand-mapped >> >> Since Domheap hardly covers all memory, the obvious thing would >> seem to be to take part of that region, just like on x86 we also >> had to reduce the direct mapping area in order to support systems >> with more than 5Tb. >> >>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>>> and potentially physical memory. While virtual address space is not a >>>>> problem, the memory is a problem for embedded platform. We want Xen to >>>>> be as lean as possible. >>>> >>>> You don't need to allocate all 512Gb, the table can be as sparse as >>>> present memory permits. >>> >>> I am aware about that... The main point is reducing the footprint of >>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >>> >>> Likely this will increase the footprint of Xen ARM. For what benefits? >>> Avoiding to store few byte in a non-generic way when we need it... >> >> But that's the point: Generic code becomes more and more clumsy >> if non-generic mechanisms need to be introduced. Quite a few we've >> had the discussion of saving a few Mb here or there, and I've almost >> always been the one to ask for not wasting memory even if we have >> plenty, so I'm all with you on that aspect. Nevertheless there is a >> point where the trade-off between memory overhead and generic >> (i.e. easier to maintain) code crosses a boundary, and I'm simply >> wondering whether we aren't at that point. > > Is the lack of M2P support on ARM a blocker for this patch to be accepted? Well, if the ARM maintainers insist on baking their own thing every time we'd use the M2P if it was there, I think I can't reasonably block this patch. Otoh I'd prefer to not see the non-x86-specific code move to x86/, so perhaps the whole patch wants re-structuring using either a manifest definition in the ARM headers or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). > This patch I think is only prevents us from possible bugs in a future. > Please correct me if I am wrong. Avoiding possible bugs in the future I didn't connect to this patch so far. Jan
Hi, all. On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote: >> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >>>> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>>>> With this I think there's quite a bit of justification needed to keep >>>>>>> going without M2P on ARM. >>>>>> >>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>>>> >>>>> How come? You don't share address spaces with guests. >>>> >>>> Below the layout for ARM32: >>>> >>>> >>>> * 0 - 12M <COMMON> >>>> * >>>> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>>> * space >>>> * >>>> * 1G - 2G Xenheap: always-mapped memory >>>> * 2G - 4G Domheap: on-demand-mapped >>> >>> Since Domheap hardly covers all memory, the obvious thing would >>> seem to be to take part of that region, just like on x86 we also >>> had to reduce the direct mapping area in order to support systems >>> with more than 5Tb. >>> >>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>>>> and potentially physical memory. While virtual address space is not a >>>>>> problem, the memory is a problem for embedded platform. We want Xen to >>>>>> be as lean as possible. >>>>> >>>>> You don't need to allocate all 512Gb, the table can be as sparse as >>>>> present memory permits. >>>> >>>> I am aware about that... The main point is reducing the footprint of >>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >>>> >>>> Likely this will increase the footprint of Xen ARM. For what benefits? >>>> Avoiding to store few byte in a non-generic way when we need it... >>> >>> But that's the point: Generic code becomes more and more clumsy >>> if non-generic mechanisms need to be introduced. Quite a few we've >>> had the discussion of saving a few Mb here or there, and I've almost >>> always been the one to ask for not wasting memory even if we have >>> plenty, so I'm all with you on that aspect. Nevertheless there is a >>> point where the trade-off between memory overhead and generic >>> (i.e. easier to maintain) code crosses a boundary, and I'm simply >>> wondering whether we aren't at that point. >> >> Is the lack of M2P support on ARM a blocker for this patch to be accepted? > > Well, if the ARM maintainers insist on baking their own thing every > time we'd use the M2P if it was there, I think I can't reasonably > block this patch. Otoh I'd prefer to not see the non-x86-specific > code move to x86/, so perhaps the whole patch wants > re-structuring using either a manifest definition in the ARM headers > or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). Jan, I am afraid but I didn't get what you meant here: "manifest definition in the ARM headers" Julien, Stefano what do you think in general? > >> This patch I think is only prevents us from possible bugs in a future. >> Please correct me if I am wrong. > > Avoiding possible bugs in the future I didn't connect to this patch so > far. > > Jan >
On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: > Hi, all. Hi Oleksandr, > On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 17.05.17 at 17:45, <olekstysh@gmail.com> wrote: >>> On Mon, May 15, 2017 at 3:43 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>>>> On 15.05.17 at 13:45, <julien.grall@arm.com> wrote: >>>>> On 05/15/2017 09:19 AM, Jan Beulich wrote: >>>>>>>>> On 15.05.17 at 09:42, <julien.grall@arm.com> wrote: >>>>>>> On 15/05/2017 08:20, Jan Beulich wrote: >>>>>>>> With this I think there's quite a bit of justification needed to keep >>>>>>>> going without M2P on ARM. >>>>>>> >>>>>>> As said in a previous thread, I made a quick calculation, ARM32 supports >>>>>>> up 40-bit PA and IPA (e.g guest address), which means 28-bits of >>>>>>> MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so >>>>>>> we would need 2^28 * 4 = 1GiB of virtual address space and potentially >>>>>>> physical memory. We don't have 1GB of VA space free on 32-bit right now. >>>>>> >>>>>> How come? You don't share address spaces with guests. >>>>> >>>>> Below the layout for ARM32: >>>>> >>>>> >>>>> * 0 - 12M <COMMON> >>>>> * >>>>> * 32M - 128M Frametable: 24 bytes per page for 16GB of RAM >>>>> * 256M - 1G VMAP: ioremap and early_ioremap use this virtual address >>>>> * space >>>>> * >>>>> * 1G - 2G Xenheap: always-mapped memory >>>>> * 2G - 4G Domheap: on-demand-mapped >>>> >>>> Since Domheap hardly covers all memory, the obvious thing would >>>> seem to be to take part of that region, just like on x86 we also >>>> had to reduce the direct mapping area in order to support systems >>>> with more than 5Tb. >>>> >>>>>>> ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means >>>>>>> 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for >>>>>>> alignment, so we would need 2^36 * 8 = 512GiB of virtual address space >>>>>>> and potentially physical memory. While virtual address space is not a >>>>>>> problem, the memory is a problem for embedded platform. We want Xen to >>>>>>> be as lean as possible. >>>>>> >>>>>> You don't need to allocate all 512Gb, the table can be as sparse as >>>>>> present memory permits. >>>>> >>>>> I am aware about that... The main point is reducing the footprint of >>>>> Xen. Assuming you have a 8GB board, you would have to use 16MB for the M2P. >>>>> >>>>> Likely this will increase the footprint of Xen ARM. For what benefits? >>>>> Avoiding to store few byte in a non-generic way when we need it... >>>> >>>> But that's the point: Generic code becomes more and more clumsy >>>> if non-generic mechanisms need to be introduced. Quite a few we've >>>> had the discussion of saving a few Mb here or there, and I've almost >>>> always been the one to ask for not wasting memory even if we have >>>> plenty, so I'm all with you on that aspect. Nevertheless there is a >>>> point where the trade-off between memory overhead and generic >>>> (i.e. easier to maintain) code crosses a boundary, and I'm simply >>>> wondering whether we aren't at that point. >>> >>> Is the lack of M2P support on ARM a blocker for this patch to be accepted? >> >> Well, if the ARM maintainers insist on baking their own thing every >> time we'd use the M2P if it was there, I think I can't reasonably >> block this patch. Otoh I'd prefer to not see the non-x86-specific >> code move to x86/, so perhaps the whole patch wants >> re-structuring using either a manifest definition in the ARM headers >> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). > Jan, I am afraid but I didn't get what you meant here: > "manifest definition in the ARM headers" I think he meant to have either a define in the header mentioning the absence/presence of M2P. But my preference would be using the Kconfig here. > > Julien, Stefano what do you think in general? My point stands here. The M2P sounds a real waste of memory for a limited benefit. There are only a few place in common code that care about that and the only one where would potentially really need it (e.g iommu_hwdom_init()) can be replaced by allocating page table in advance (for justification see my answer on patch #6). I will take an action to replace the few mfn_to_gmfn by wrapper and proper implementation for ARM. So Jan suggestion for CONFIG_M2P (or maybe CONFIG_HAS_M2P) would be a good solution. Cheers,
>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote: > On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: >> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>> Well, if the ARM maintainers insist on baking their own thing every >>> time we'd use the M2P if it was there, I think I can't reasonably >>> block this patch. Otoh I'd prefer to not see the non-x86-specific >>> code move to x86/, so perhaps the whole patch wants >>> re-structuring using either a manifest definition in the ARM headers >>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). >> Jan, I am afraid but I didn't get what you meant here: >> "manifest definition in the ARM headers" > > I think he meant to have either a define in the header mentioning the > absence/presence of M2P. Yes, at least in a way. > But my preference would be using the Kconfig here. Depends on the symbol used: If such a symbol solely _indicates_ the presence, Kconfig would be better indeed. If, however, the symbol is e.g. a macro resolving to a per-arch implementation, with common code providing a default definition when the arch doesn't provide any, then the non-Kconfig variant may be preferable. Jan
Hi, all. On Thu, May 18, 2017 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote: >> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: >>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>> Well, if the ARM maintainers insist on baking their own thing every >>>> time we'd use the M2P if it was there, I think I can't reasonably >>>> block this patch. Otoh I'd prefer to not see the non-x86-specific >>>> code move to x86/, so perhaps the whole patch wants >>>> re-structuring using either a manifest definition in the ARM headers >>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). >>> Jan, I am afraid but I didn't get what you meant here: >>> "manifest definition in the ARM headers" >> >> I think he meant to have either a define in the header mentioning the >> absence/presence of M2P. > > Yes, at least in a way. > >> But my preference would be using the Kconfig here. > > Depends on the symbol used: If such a symbol solely _indicates_ > the presence, Kconfig would be better indeed. If, however, the > symbol is e.g. a macro resolving to a per-arch implementation, > with common code providing a default definition when the arch > doesn't provide any, then the non-Kconfig variant may be > preferable. > > Jan > Thank you for your comments. I have already posted a common comment regarding several patches in the current series as they are interrelated (please see patch #6), but I duplicate here only related to this patch part. ... patch #8: As we always allocate the page table for hardware domain, this patch should be reworked. The use_iommu flag should be set for both archs in case of hardware domain. Having d->need_iommu set at the early stage we won't skip IOMMU mapping updates anymore. And as the result the existing in iommu_hwdom_init() code that goes through the list of page and tries to retrieve mapping could be just dropped instead of moving it to the arch-specific part. ... Does the described above make sense?
>>> On 18.05.17 at 20:06, <olekstysh@gmail.com> wrote: > On Thu, May 18, 2017 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 17.05.17 at 22:30, <julien.grall@arm.com> wrote: >>> On 05/17/2017 07:51 PM, Oleksandr Tyshchenko wrote: >>>> On Wed, May 17, 2017 at 7:01 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> Well, if the ARM maintainers insist on baking their own thing every >>>>> time we'd use the M2P if it was there, I think I can't reasonably >>>>> block this patch. Otoh I'd prefer to not see the non-x86-specific >>>>> code move to x86/, so perhaps the whole patch wants >>>>> re-structuring using either a manifest definition in the ARM headers >>>>> or e.g. CONFIG_M2P (which x86 would select, but ARM wouldn't). >>>> Jan, I am afraid but I didn't get what you meant here: >>>> "manifest definition in the ARM headers" >>> >>> I think he meant to have either a define in the header mentioning the >>> absence/presence of M2P. >> >> Yes, at least in a way. >> >>> But my preference would be using the Kconfig here. >> >> Depends on the symbol used: If such a symbol solely _indicates_ >> the presence, Kconfig would be better indeed. If, however, the >> symbol is e.g. a macro resolving to a per-arch implementation, >> with common code providing a default definition when the arch >> doesn't provide any, then the non-Kconfig variant may be >> preferable. > > Thank you for your comments. > I have already posted a common comment regarding several patches in > the current series > as they are interrelated (please see patch #6), but I duplicate here > only related to this patch part. > > ... > patch #8: As we always allocate the page table for hardware domain, > this patch should be reworked. > The use_iommu flag should be set for both archs in case of hardware > domain. Having d->need_iommu set at the early stage we won't skip > IOMMU mapping updates anymore. And as the result the existing in > iommu_hwdom_init() code that goes through the list of page and tries > to retrieve mapping could be just dropped > instead of moving it to the arch-specific part. > ... > > Does the described above make sense? As just said in the other reply - only if there weren't all these extra overrides (one of which is even on by default, albeit we've been discussing recently whether that's actually [still] appropriate). Jan
diff --git a/xen/drivers/passthrough/arm/iommu.c b/xen/drivers/passthrough/arm/iommu.c index f132032..2198723 100644 --- a/xen/drivers/passthrough/arm/iommu.c +++ b/xen/drivers/passthrough/arm/iommu.c @@ -19,6 +19,7 @@ #include <xen/iommu.h> #include <xen/device_tree.h> #include <asm/device.h> +#include <xen/sched.h> static const struct iommu_ops *iommu_ops; @@ -59,6 +60,12 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d) return; } +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) +{ + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) + arch_iommu_populate_page_table(d); +} + int arch_iommu_domain_init(struct domain *d) { return iommu_dt_domain_init(d); diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index c85f7b4..e66eefb 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -177,36 +177,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0); d->need_iommu = !!iommu_dom0_strict; - if ( need_iommu(d) && !iommu_use_hap_pt(d) ) - { - struct page_info *page; - unsigned int i = 0; - int rc = 0; - - page_list_for_each ( page, &d->page_list ) - { - unsigned long mfn = page_to_mfn(page); - unsigned long gfn = mfn_to_gmfn(d, mfn); - unsigned int mapping = IOMMUF_readable; - int ret; - - if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) || - ((page->u.inuse.type_info & PGT_type_mask) - == PGT_writable_page) ) - mapping |= IOMMUF_writable; - - ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping); - if ( !rc ) - rc = ret; - - if ( !(i++ & 0xfffff) ) - process_pending_softirqs(); - } - if ( rc ) - printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", - d->domain_id, rc); - } + arch_iommu_hwdom_init(d); return hd->platform_ops->hwdom_init(d); } diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 973b72f..904736b 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -118,6 +118,42 @@ void __hwdom_init arch_iommu_check_autotranslated_hwdom(struct domain *d) panic("Presently, iommu must be enabled for PVH hardware domain\n"); } +void __hwdom_init arch_iommu_hwdom_init(struct domain *d) +{ + const struct domain_iommu *hd = dom_iommu(d); + + if ( need_iommu(d) && !iommu_use_hap_pt(d) ) + { + struct page_info *page; + unsigned int i = 0; + int rc = 0; + + page_list_for_each ( page, &d->page_list ) + { + unsigned long mfn = page_to_mfn(page); + unsigned long gfn = mfn_to_gmfn(d, mfn); + unsigned int mapping = IOMMUF_readable; + int ret; + + if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) || + ((page->u.inuse.type_info & PGT_type_mask) + == PGT_writable_page) ) + mapping |= IOMMUF_writable; + + ret = hd->platform_ops->map_pages(d, gfn, mfn, 0, mapping); + if ( !rc ) + rc = ret; + + if ( !(i++ & 0xfffff) ) + process_pending_softirqs(); + } + + if ( rc ) + printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", + d->domain_id, rc); + } +} + int arch_iommu_domain_init(struct domain *d) { struct domain_iommu *hd = dom_iommu(d); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index f5914db..be43b28 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -65,6 +65,7 @@ void arch_iommu_domain_destroy(struct domain *d); int arch_iommu_domain_init(struct domain *d); int arch_iommu_populate_page_table(struct domain *d); void arch_iommu_check_autotranslated_hwdom(struct domain *d); +void arch_iommu_hwdom_init(struct domain *d); int iommu_construct(struct domain *d);