| Message ID | 1156cb116da19ef64323e472bb6b6e87c6c73d77.1621017334.git.connojdavis@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Minimal build for RISCV | expand |
On 14.05.2021 20:53, Connor Davis wrote: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) > p2m_type_t p2mt; > #endif > mfn_t mfn; > +#ifdef CONFIG_HAS_PASSTHROUGH > bool *dont_flush_p, dont_flush; > +#endif > int rc; > > #ifdef CONFIG_X86 > @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) > * Since we're likely to free the page below, we need to suspend > * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. > */ > +#ifdef CONFIG_HAS_PASSTHROUGH > dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); > dont_flush = *dont_flush_p; > *dont_flush_p = false; > +#endif > > rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); > > +#ifdef CONFIG_HAS_PASSTHROUGH > *dont_flush_p = dont_flush; > +#endif > > /* > * With the lack of an IOMMU on some platforms, domains with DMA-capable > @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, > xatp->gpfn += start; > xatp->size -= start; > > +#ifdef CONFIG_HAS_PASSTHROUGH > if ( is_iommu_enabled(d) ) > { > this_cpu(iommu_dont_flush_iotlb) = 1; > extra.ppage = &pages[0]; > } > +#endif > > while ( xatp->size > done ) > { > @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, > } > } > > +#ifdef CONFIG_HAS_PASSTHROUGH > if ( is_iommu_enabled(d) ) > { > int ret; > @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, > if ( unlikely(ret) && rc >= 0 ) > rc = ret; > } > +#endif > > return rc; > } I wonder whether all of these wouldn't better become CONFIG_X86: ISTR Julien indicating that he doesn't see the override getting used on Arm. (Julien, please correct me if I'm misremembering.) > --- a/xen/include/xen/iommu.h > +++ b/xen/include/xen/iommu.h > @@ -51,9 +51,15 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y) > return dfn_x(x) == dfn_x(y); > } > > -extern bool_t iommu_enable, iommu_enabled; > +extern bool_t iommu_enable; > extern bool force_iommu, iommu_quarantine, iommu_verbose; > > +#ifdef CONFIG_HAS_PASSTHROUGH > +extern bool_t iommu_enabled; Just bool please, like is already the case for the line in context above. We're in the process of phasing out bool_t. Jan
On 5/17/21 5:16 AM, Jan Beulich wrote: > On 14.05.2021 20:53, Connor Davis wrote: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) >> p2m_type_t p2mt; >> #endif >> mfn_t mfn; >> +#ifdef CONFIG_HAS_PASSTHROUGH >> bool *dont_flush_p, dont_flush; >> +#endif >> int rc; >> >> #ifdef CONFIG_X86 >> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) >> * Since we're likely to free the page below, we need to suspend >> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >> */ >> +#ifdef CONFIG_HAS_PASSTHROUGH >> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >> dont_flush = *dont_flush_p; >> *dont_flush_p = false; >> +#endif >> >> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> *dont_flush_p = dont_flush; >> +#endif >> >> /* >> * With the lack of an IOMMU on some platforms, domains with DMA-capable >> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, >> xatp->gpfn += start; >> xatp->size -= start; >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> if ( is_iommu_enabled(d) ) >> { >> this_cpu(iommu_dont_flush_iotlb) = 1; >> extra.ppage = &pages[0]; >> } >> +#endif >> >> while ( xatp->size > done ) >> { >> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, >> } >> } >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> if ( is_iommu_enabled(d) ) >> { >> int ret; >> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, >> if ( unlikely(ret) && rc >= 0 ) >> rc = ret; >> } >> +#endif >> >> return rc; >> } > I wonder whether all of these wouldn't better become CONFIG_X86: > ISTR Julien indicating that he doesn't see the override getting used > on Arm. (Julien, please correct me if I'm misremembering.) > >> --- a/xen/include/xen/iommu.h >> +++ b/xen/include/xen/iommu.h >> @@ -51,9 +51,15 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y) >> return dfn_x(x) == dfn_x(y); >> } >> >> -extern bool_t iommu_enable, iommu_enabled; >> +extern bool_t iommu_enable; >> extern bool force_iommu, iommu_quarantine, iommu_verbose; >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> +extern bool_t iommu_enabled; > Just bool please, like is already the case for the line in context > above. We're in the process of phasing out bool_t. Got it, thanks. > > Jan Thanks, Connor
Hi Jan, On 17/05/2021 12:16, Jan Beulich wrote: > On 14.05.2021 20:53, Connor Davis wrote: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) >> p2m_type_t p2mt; >> #endif >> mfn_t mfn; >> +#ifdef CONFIG_HAS_PASSTHROUGH >> bool *dont_flush_p, dont_flush; >> +#endif >> int rc; >> >> #ifdef CONFIG_X86 >> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) >> * Since we're likely to free the page below, we need to suspend >> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >> */ >> +#ifdef CONFIG_HAS_PASSTHROUGH >> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >> dont_flush = *dont_flush_p; >> *dont_flush_p = false; >> +#endif >> >> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> *dont_flush_p = dont_flush; >> +#endif >> >> /* >> * With the lack of an IOMMU on some platforms, domains with DMA-capable >> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, >> xatp->gpfn += start; >> xatp->size -= start; >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> if ( is_iommu_enabled(d) ) >> { >> this_cpu(iommu_dont_flush_iotlb) = 1; >> extra.ppage = &pages[0]; >> } >> +#endif >> >> while ( xatp->size > done ) >> { >> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, >> } >> } >> >> +#ifdef CONFIG_HAS_PASSTHROUGH >> if ( is_iommu_enabled(d) ) >> { >> int ret; >> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, >> if ( unlikely(ret) && rc >= 0 ) >> rc = ret; >> } >> +#endif >> >> return rc; >> } > > I wonder whether all of these wouldn't better become CONFIG_X86: > ISTR Julien indicating that he doesn't see the override getting used > on Arm. (Julien, please correct me if I'm misremembering.) Right, so far, I haven't been in favor to introduce it because: 1) The P2M code may free some memory. So you can't always ignore the flush (I think this is wrong for the upper layer to know when this can happen). 2) It is unclear what happen if the IOMMU TLBs and the PT contains different mappings (I received conflicted advice). So it is better to always flush and as early as possible. Cheers,
On 5/17/21 9:42 AM, Julien Grall wrote: > Hi Jan, > > On 17/05/2021 12:16, Jan Beulich wrote: >> On 14.05.2021 20:53, Connor Davis wrote: >>> --- a/xen/common/memory.c >>> +++ b/xen/common/memory.c >>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned >>> long gmfn) >>> p2m_type_t p2mt; >>> #endif >>> mfn_t mfn; >>> +#ifdef CONFIG_HAS_PASSTHROUGH >>> bool *dont_flush_p, dont_flush; >>> +#endif >>> int rc; >>> #ifdef CONFIG_X86 >>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, >>> unsigned long gmfn) >>> * Since we're likely to free the page below, we need to suspend >>> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >>> */ >>> +#ifdef CONFIG_HAS_PASSTHROUGH >>> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >>> dont_flush = *dont_flush_p; >>> *dont_flush_p = false; >>> +#endif >>> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >>> +#ifdef CONFIG_HAS_PASSTHROUGH >>> *dont_flush_p = dont_flush; >>> +#endif >>> /* >>> * With the lack of an IOMMU on some platforms, domains with >>> DMA-capable >>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, >>> struct xen_add_to_physmap *xatp, >>> xatp->gpfn += start; >>> xatp->size -= start; >>> +#ifdef CONFIG_HAS_PASSTHROUGH >>> if ( is_iommu_enabled(d) ) >>> { >>> this_cpu(iommu_dont_flush_iotlb) = 1; >>> extra.ppage = &pages[0]; >>> } >>> +#endif >>> while ( xatp->size > done ) >>> { >>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, >>> struct xen_add_to_physmap *xatp, >>> } >>> } >>> +#ifdef CONFIG_HAS_PASSTHROUGH >>> if ( is_iommu_enabled(d) ) >>> { >>> int ret; >>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, >>> struct xen_add_to_physmap *xatp, >>> if ( unlikely(ret) && rc >= 0 ) >>> rc = ret; >>> } >>> +#endif >>> return rc; >>> } >> >> I wonder whether all of these wouldn't better become CONFIG_X86: >> ISTR Julien indicating that he doesn't see the override getting used >> on Arm. (Julien, please correct me if I'm misremembering.) > > Right, so far, I haven't been in favor to introduce it because: > 1) The P2M code may free some memory. So you can't always ignore > the flush (I think this is wrong for the upper layer to know when this > can happen). > 2) It is unclear what happen if the IOMMU TLBs and the PT contains > different mappings (I received conflicted advice). > > So it is better to always flush and as early as possible. So keep it as is or switch to CONFIG_X86? Thanks, Connor
On 18.05.2021 06:11, Connor Davis wrote: > > On 5/17/21 9:42 AM, Julien Grall wrote: >> Hi Jan, >> >> On 17/05/2021 12:16, Jan Beulich wrote: >>> On 14.05.2021 20:53, Connor Davis wrote: >>>> --- a/xen/common/memory.c >>>> +++ b/xen/common/memory.c >>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned >>>> long gmfn) >>>> p2m_type_t p2mt; >>>> #endif >>>> mfn_t mfn; >>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>> bool *dont_flush_p, dont_flush; >>>> +#endif >>>> int rc; >>>> #ifdef CONFIG_X86 >>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, >>>> unsigned long gmfn) >>>> * Since we're likely to free the page below, we need to suspend >>>> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >>>> */ >>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >>>> dont_flush = *dont_flush_p; >>>> *dont_flush_p = false; >>>> +#endif >>>> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>> *dont_flush_p = dont_flush; >>>> +#endif >>>> /* >>>> * With the lack of an IOMMU on some platforms, domains with >>>> DMA-capable >>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, >>>> struct xen_add_to_physmap *xatp, >>>> xatp->gpfn += start; >>>> xatp->size -= start; >>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>> if ( is_iommu_enabled(d) ) >>>> { >>>> this_cpu(iommu_dont_flush_iotlb) = 1; >>>> extra.ppage = &pages[0]; >>>> } >>>> +#endif >>>> while ( xatp->size > done ) >>>> { >>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>> struct xen_add_to_physmap *xatp, >>>> } >>>> } >>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>> if ( is_iommu_enabled(d) ) >>>> { >>>> int ret; >>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>> struct xen_add_to_physmap *xatp, >>>> if ( unlikely(ret) && rc >= 0 ) >>>> rc = ret; >>>> } >>>> +#endif >>>> return rc; >>>> } >>> >>> I wonder whether all of these wouldn't better become CONFIG_X86: >>> ISTR Julien indicating that he doesn't see the override getting used >>> on Arm. (Julien, please correct me if I'm misremembering.) >> >> Right, so far, I haven't been in favor to introduce it because: >> 1) The P2M code may free some memory. So you can't always ignore >> the flush (I think this is wrong for the upper layer to know when this >> can happen). >> 2) It is unclear what happen if the IOMMU TLBs and the PT contains >> different mappings (I received conflicted advice). >> >> So it is better to always flush and as early as possible. > > So keep it as is or switch to CONFIG_X86? Please switch, unless anyone else voices a strong opinion towards keeping as is. Jan
On 18/05/2021 07:27, Jan Beulich wrote: > On 18.05.2021 06:11, Connor Davis wrote: >> >> On 5/17/21 9:42 AM, Julien Grall wrote: >>> Hi Jan, >>> >>> On 17/05/2021 12:16, Jan Beulich wrote: >>>> On 14.05.2021 20:53, Connor Davis wrote: >>>>> --- a/xen/common/memory.c >>>>> +++ b/xen/common/memory.c >>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned >>>>> long gmfn) >>>>> p2m_type_t p2mt; >>>>> #endif >>>>> mfn_t mfn; >>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>> bool *dont_flush_p, dont_flush; >>>>> +#endif >>>>> int rc; >>>>> #ifdef CONFIG_X86 >>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, >>>>> unsigned long gmfn) >>>>> * Since we're likely to free the page below, we need to suspend >>>>> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >>>>> */ >>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >>>>> dont_flush = *dont_flush_p; >>>>> *dont_flush_p = false; >>>>> +#endif >>>>> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>> *dont_flush_p = dont_flush; >>>>> +#endif >>>>> /* >>>>> * With the lack of an IOMMU on some platforms, domains with >>>>> DMA-capable >>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, >>>>> struct xen_add_to_physmap *xatp, >>>>> xatp->gpfn += start; >>>>> xatp->size -= start; >>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>> if ( is_iommu_enabled(d) ) >>>>> { >>>>> this_cpu(iommu_dont_flush_iotlb) = 1; >>>>> extra.ppage = &pages[0]; >>>>> } >>>>> +#endif >>>>> while ( xatp->size > done ) >>>>> { >>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>>> struct xen_add_to_physmap *xatp, >>>>> } >>>>> } >>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>> if ( is_iommu_enabled(d) ) >>>>> { >>>>> int ret; >>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>>> struct xen_add_to_physmap *xatp, >>>>> if ( unlikely(ret) && rc >= 0 ) >>>>> rc = ret; >>>>> } >>>>> +#endif >>>>> return rc; >>>>> } >>>> >>>> I wonder whether all of these wouldn't better become CONFIG_X86: >>>> ISTR Julien indicating that he doesn't see the override getting used >>>> on Arm. (Julien, please correct me if I'm misremembering.) >>> >>> Right, so far, I haven't been in favor to introduce it because: >>> 1) The P2M code may free some memory. So you can't always ignore >>> the flush (I think this is wrong for the upper layer to know when this >>> can happen). >>> 2) It is unclear what happen if the IOMMU TLBs and the PT contains >>> different mappings (I received conflicted advice). >>> >>> So it is better to always flush and as early as possible. >> >> So keep it as is or switch to CONFIG_X86? > > Please switch, unless anyone else voices a strong opinion towards > keeping as is. I would like to avoid adding more #ifdef CONFIG_X86 in the common code. Can we instead provide a wrapper for them? Cheers,
On 18.05.2021 16:06, Julien Grall wrote: > > > On 18/05/2021 07:27, Jan Beulich wrote: >> On 18.05.2021 06:11, Connor Davis wrote: >>> >>> On 5/17/21 9:42 AM, Julien Grall wrote: >>>> Hi Jan, >>>> >>>> On 17/05/2021 12:16, Jan Beulich wrote: >>>>> On 14.05.2021 20:53, Connor Davis wrote: >>>>>> --- a/xen/common/memory.c >>>>>> +++ b/xen/common/memory.c >>>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned >>>>>> long gmfn) >>>>>> p2m_type_t p2mt; >>>>>> #endif >>>>>> mfn_t mfn; >>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>> bool *dont_flush_p, dont_flush; >>>>>> +#endif >>>>>> int rc; >>>>>> #ifdef CONFIG_X86 >>>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, >>>>>> unsigned long gmfn) >>>>>> * Since we're likely to free the page below, we need to suspend >>>>>> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >>>>>> */ >>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >>>>>> dont_flush = *dont_flush_p; >>>>>> *dont_flush_p = false; >>>>>> +#endif >>>>>> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>> *dont_flush_p = dont_flush; >>>>>> +#endif >>>>>> /* >>>>>> * With the lack of an IOMMU on some platforms, domains with >>>>>> DMA-capable >>>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, >>>>>> struct xen_add_to_physmap *xatp, >>>>>> xatp->gpfn += start; >>>>>> xatp->size -= start; >>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>> if ( is_iommu_enabled(d) ) >>>>>> { >>>>>> this_cpu(iommu_dont_flush_iotlb) = 1; >>>>>> extra.ppage = &pages[0]; >>>>>> } >>>>>> +#endif >>>>>> while ( xatp->size > done ) >>>>>> { >>>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>>>> struct xen_add_to_physmap *xatp, >>>>>> } >>>>>> } >>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>> if ( is_iommu_enabled(d) ) >>>>>> { >>>>>> int ret; >>>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>>>> struct xen_add_to_physmap *xatp, >>>>>> if ( unlikely(ret) && rc >= 0 ) >>>>>> rc = ret; >>>>>> } >>>>>> +#endif >>>>>> return rc; >>>>>> } >>>>> >>>>> I wonder whether all of these wouldn't better become CONFIG_X86: >>>>> ISTR Julien indicating that he doesn't see the override getting used >>>>> on Arm. (Julien, please correct me if I'm misremembering.) >>>> >>>> Right, so far, I haven't been in favor to introduce it because: >>>> 1) The P2M code may free some memory. So you can't always ignore >>>> the flush (I think this is wrong for the upper layer to know when this >>>> can happen). >>>> 2) It is unclear what happen if the IOMMU TLBs and the PT contains >>>> different mappings (I received conflicted advice). >>>> >>>> So it is better to always flush and as early as possible. >>> >>> So keep it as is or switch to CONFIG_X86? >> >> Please switch, unless anyone else voices a strong opinion towards >> keeping as is. > > I would like to avoid adding more #ifdef CONFIG_X86 in the common code. > Can we instead provide a wrapper for them? Doable, sure, but I don't know whether Connor is up to going this more extensive route. Jan
Hi Jan, On 18/05/2021 16:13, Jan Beulich wrote: > On 18.05.2021 16:06, Julien Grall wrote: >> >> >> On 18/05/2021 07:27, Jan Beulich wrote: >>> On 18.05.2021 06:11, Connor Davis wrote: >>>> >>>> On 5/17/21 9:42 AM, Julien Grall wrote: >>>>> Hi Jan, >>>>> >>>>> On 17/05/2021 12:16, Jan Beulich wrote: >>>>>> On 14.05.2021 20:53, Connor Davis wrote: >>>>>>> --- a/xen/common/memory.c >>>>>>> +++ b/xen/common/memory.c >>>>>>> @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned >>>>>>> long gmfn) >>>>>>> p2m_type_t p2mt; >>>>>>> #endif >>>>>>> mfn_t mfn; >>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>>> bool *dont_flush_p, dont_flush; >>>>>>> +#endif >>>>>>> int rc; >>>>>>> #ifdef CONFIG_X86 >>>>>>> @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, >>>>>>> unsigned long gmfn) >>>>>>> * Since we're likely to free the page below, we need to suspend >>>>>>> * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. >>>>>>> */ >>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>>> dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); >>>>>>> dont_flush = *dont_flush_p; >>>>>>> *dont_flush_p = false; >>>>>>> +#endif >>>>>>> rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); >>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>>> *dont_flush_p = dont_flush; >>>>>>> +#endif >>>>>>> /* >>>>>>> * With the lack of an IOMMU on some platforms, domains with >>>>>>> DMA-capable >>>>>>> @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, >>>>>>> struct xen_add_to_physmap *xatp, >>>>>>> xatp->gpfn += start; >>>>>>> xatp->size -= start; >>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>>> if ( is_iommu_enabled(d) ) >>>>>>> { >>>>>>> this_cpu(iommu_dont_flush_iotlb) = 1; >>>>>>> extra.ppage = &pages[0]; >>>>>>> } >>>>>>> +#endif >>>>>>> while ( xatp->size > done ) >>>>>>> { >>>>>>> @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>>>>> struct xen_add_to_physmap *xatp, >>>>>>> } >>>>>>> } >>>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH >>>>>>> if ( is_iommu_enabled(d) ) >>>>>>> { >>>>>>> int ret; >>>>>>> @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, >>>>>>> struct xen_add_to_physmap *xatp, >>>>>>> if ( unlikely(ret) && rc >= 0 ) >>>>>>> rc = ret; >>>>>>> } >>>>>>> +#endif >>>>>>> return rc; >>>>>>> } >>>>>> >>>>>> I wonder whether all of these wouldn't better become CONFIG_X86: >>>>>> ISTR Julien indicating that he doesn't see the override getting used >>>>>> on Arm. (Julien, please correct me if I'm misremembering.) >>>>> >>>>> Right, so far, I haven't been in favor to introduce it because: >>>>> 1) The P2M code may free some memory. So you can't always ignore >>>>> the flush (I think this is wrong for the upper layer to know when this >>>>> can happen). >>>>> 2) It is unclear what happen if the IOMMU TLBs and the PT contains >>>>> different mappings (I received conflicted advice). >>>>> >>>>> So it is better to always flush and as early as possible. >>>> >>>> So keep it as is or switch to CONFIG_X86? >>> >>> Please switch, unless anyone else voices a strong opinion towards >>> keeping as is. >> >> I would like to avoid adding more #ifdef CONFIG_X86 in the common code. >> Can we instead provide a wrapper for them? > > Doable, sure, but I don't know whether Connor is up to going this > more extensive route. That's a fair point. If that the case, then I prefer the #ifdef CONFIG_HAS_PASSTHROUGH version. I can add an item in my todo list to introduce some helpers. Cheers,
diff --git a/xen/common/memory.c b/xen/common/memory.c index b5c70c4b85..72a6b70cb5 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -294,7 +294,9 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) p2m_type_t p2mt; #endif mfn_t mfn; +#ifdef CONFIG_HAS_PASSTHROUGH bool *dont_flush_p, dont_flush; +#endif int rc; #ifdef CONFIG_X86 @@ -385,13 +387,17 @@ int guest_remove_page(struct domain *d, unsigned long gmfn) * Since we're likely to free the page below, we need to suspend * xenmem_add_to_physmap()'s suppressing of IOMMU TLB flushes. */ +#ifdef CONFIG_HAS_PASSTHROUGH dont_flush_p = &this_cpu(iommu_dont_flush_iotlb); dont_flush = *dont_flush_p; *dont_flush_p = false; +#endif rc = guest_physmap_remove_page(d, _gfn(gmfn), mfn, 0); +#ifdef CONFIG_HAS_PASSTHROUGH *dont_flush_p = dont_flush; +#endif /* * With the lack of an IOMMU on some platforms, domains with DMA-capable @@ -839,11 +845,13 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, xatp->gpfn += start; xatp->size -= start; +#ifdef CONFIG_HAS_PASSTHROUGH if ( is_iommu_enabled(d) ) { this_cpu(iommu_dont_flush_iotlb) = 1; extra.ppage = &pages[0]; } +#endif while ( xatp->size > done ) { @@ -868,6 +876,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, } } +#ifdef CONFIG_HAS_PASSTHROUGH if ( is_iommu_enabled(d) ) { int ret; @@ -894,6 +903,7 @@ int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, if ( unlikely(ret) && rc >= 0 ) rc = ret; } +#endif return rc; } diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 460755df29..d878a93269 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -51,9 +51,15 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y) return dfn_x(x) == dfn_x(y); } -extern bool_t iommu_enable, iommu_enabled; +extern bool_t iommu_enable; extern bool force_iommu, iommu_quarantine, iommu_verbose; +#ifdef CONFIG_HAS_PASSTHROUGH +extern bool_t iommu_enabled; +#else +#define iommu_enabled false +#endif + #ifdef CONFIG_X86 extern enum __packed iommu_intremap { /*
The variables iommu_enabled and iommu_dont_flush_iotlb are defined in drivers/passthrough/iommu.c and are referenced in common code, which causes the link to fail when !CONFIG_HAS_PASSTHROUGH. Guard references to these variables in common code so that xen builds when !CONFIG_HAS_PASSTHROUGH. Signed-off-by: Connor Davis <connojdavis@gmail.com> --- xen/common/memory.c | 10 ++++++++++ xen/include/xen/iommu.h | 8 +++++++- 2 files changed, 17 insertions(+), 1 deletion(-)