| Message ID | 20160801171028.11615-15-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, On 01/08/16 18:10, Sergej Proskurin wrote: > The function get_page_from_gva uses ARM's hardware support to translate > gva's to machine addresses. This function is used, among others, for > memory regulation purposes, e.g, within the context of memory ballooning. > To ensure correct behavior while altp2m is in use, we use the host's p2m > table for the associated gva to ma translation. This is required at this > point, as altp2m lazily copies pages from the host's p2m and even might > be flushed because of changes to the host's p2m (as it is done within > the context of memory ballooning). I was expecting to see some change in p2m_mem_access_check_and_get_page. Is there any reason to not fix it? > Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > --- > xen/arch/arm/p2m.c | 31 +++++++++++++++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index bcad51f..784f8da 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -1614,7 +1614,7 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, > unsigned long flags) > { > struct domain *d = v->domain; > - struct p2m_domain *p2m = &d->arch.p2m; > + struct p2m_domain *p2m = p2m_get_hostp2m(d); This is more a clean-up than necessary. I would prefer to see a patch modifying all "&d->arch.p2m" by p2m_get_hostp2m in one go. > struct page_info *page = NULL; > paddr_t maddr = 0; > int rc; > @@ -1628,7 +1628,34 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, > > p2m_read_lock(p2m); > > - rc = gvirt_to_maddr(va, &maddr, flags); > + /* > + * If altp2m is active, we still read the gva from the hostp2m, as it s/still/need to/ > + * contains all valid mappings while the currently active altp2m view might > + * not have the required gva mapping yet. > + */ > + if ( unlikely(altp2m_active(d)) ) > + { > + unsigned long irq_flags = 0; > + uint64_t ovttbr = READ_SYSREG64(VTTBR_EL2); > + > + if ( ovttbr != p2m->vttbr.vttbr ) > + { > + local_irq_save(irq_flags); > + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); > + isb(); > + } > + > + rc = gvirt_to_maddr(va, &maddr, flags); > + > + if ( ovttbr != p2m->vttbr.vttbr ) > + { > + WRITE_SYSREG64(ovttbr, VTTBR_EL2); > + isb(); > + local_irq_restore(irq_flags); > + } The pattern is very similar to what p2m_flush_tlb does. Can we get macro/helpers to avoid duplicate code? > + } > + else > + rc = gvirt_to_maddr(va, &maddr, flags); > > if ( rc ) > goto err; > Regards,
Hi Julien, On 08/04/2016 01:59 PM, Julien Grall wrote: > Hello Sergej, > > On 01/08/16 18:10, Sergej Proskurin wrote: >> The function get_page_from_gva uses ARM's hardware support to translate >> gva's to machine addresses. This function is used, among others, for >> memory regulation purposes, e.g, within the context of memory >> ballooning. >> To ensure correct behavior while altp2m is in use, we use the host's p2m >> table for the associated gva to ma translation. This is required at this >> point, as altp2m lazily copies pages from the host's p2m and even might >> be flushed because of changes to the host's p2m (as it is done within >> the context of memory ballooning). > > I was expecting to see some change in > p2m_mem_access_check_and_get_page. Is there any reason to not fix it? > > I did not yet encounter any issues with p2m_mem_access_check_and_get_page. According to ARM ARM, ATS1C** (see gva_to_ipa_par) translates VA to IPA in non-secure privilege levels (as it is the the case here). Thus, the 2nd level translation represented by the (alt)p2m is not really considered at this point and hence make an extension obsolete. Or did you have anything else in mind? >> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Julien Grall <julien.grall@arm.com> >> --- >> xen/arch/arm/p2m.c | 31 +++++++++++++++++++++++++++++-- >> 1 file changed, 29 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index bcad51f..784f8da 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -1614,7 +1614,7 @@ struct page_info *get_page_from_gva(struct vcpu >> *v, vaddr_t va, >> unsigned long flags) >> { >> struct domain *d = v->domain; >> - struct p2m_domain *p2m = &d->arch.p2m; >> + struct p2m_domain *p2m = p2m_get_hostp2m(d); > > This is more a clean-up than necessary. I would prefer to see a patch > modifying all "&d->arch.p2m" by p2m_get_hostp2m in one go. > I will add a patch, which will do just that. Thank you. >> struct page_info *page = NULL; >> paddr_t maddr = 0; >> int rc; >> @@ -1628,7 +1628,34 @@ struct page_info *get_page_from_gva(struct >> vcpu *v, vaddr_t va, >> >> p2m_read_lock(p2m); >> >> - rc = gvirt_to_maddr(va, &maddr, flags); >> + /* >> + * If altp2m is active, we still read the gva from the hostp2m, >> as it > > s/still/need to/ > Ok. >> + * contains all valid mappings while the currently active altp2m >> view might >> + * not have the required gva mapping yet. >> + */ >> + if ( unlikely(altp2m_active(d)) ) >> + { >> + unsigned long irq_flags = 0; >> + uint64_t ovttbr = READ_SYSREG64(VTTBR_EL2); >> + >> + if ( ovttbr != p2m->vttbr.vttbr ) >> + { >> + local_irq_save(irq_flags); >> + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); >> + isb(); >> + } >> + >> + rc = gvirt_to_maddr(va, &maddr, flags); >> + >> + if ( ovttbr != p2m->vttbr.vttbr ) >> + { >> + WRITE_SYSREG64(ovttbr, VTTBR_EL2); >> + isb(); >> + local_irq_restore(irq_flags); >> + } > > The pattern is very similar to what p2m_flush_tlb does. Can we get > macro/helpers to avoid duplicate code? > Sure thing :) >> + } >> + else >> + rc = gvirt_to_maddr(va, &maddr, flags); >> >> if ( rc ) >> goto err; >> > Best regards, ~Sergej
On 06/08/2016 11:38, Sergej Proskurin wrote: > Hi Julien, Hello Serge, > On 08/04/2016 01:59 PM, Julien Grall wrote: >> Hello Sergej, >> >> On 01/08/16 18:10, Sergej Proskurin wrote: >>> The function get_page_from_gva uses ARM's hardware support to translate >>> gva's to machine addresses. This function is used, among others, for >>> memory regulation purposes, e.g, within the context of memory >>> ballooning. >>> To ensure correct behavior while altp2m is in use, we use the host's p2m >>> table for the associated gva to ma translation. This is required at this >>> point, as altp2m lazily copies pages from the host's p2m and even might >>> be flushed because of changes to the host's p2m (as it is done within >>> the context of memory ballooning). >> >> I was expecting to see some change in >> p2m_mem_access_check_and_get_page. Is there any reason to not fix it? >> >> > > I did not yet encounter any issues with > p2m_mem_access_check_and_get_page. According to ARM ARM, ATS1C** (see > gva_to_ipa_par) translates VA to IPA in non-secure privilege levels (as > it is the the case here). Thus, the 2nd level translation represented by > the (alt)p2m is not really considered at this point and hence make an > extension obsolete. > > Or did you have anything else in mind? The stage-1 page tables are living in the guest memory. So every time you access an entry in the page table, you have to translate the IPA (guest physical address) into a PA. However, the underlying memory of those page table may have restriction permission or does not exist in the altp2m at all. So the translation will fail. Regards,
Hi Julien, On 08/06/2016 03:45 PM, Julien Grall wrote: > > > On 06/08/2016 11:38, Sergej Proskurin wrote: >> Hi Julien, > > Hello Serge, > >> On 08/04/2016 01:59 PM, Julien Grall wrote: >>> Hello Sergej, >>> >>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>> The function get_page_from_gva uses ARM's hardware support to >>>> translate >>>> gva's to machine addresses. This function is used, among others, for >>>> memory regulation purposes, e.g, within the context of memory >>>> ballooning. >>>> To ensure correct behavior while altp2m is in use, we use the >>>> host's p2m >>>> table for the associated gva to ma translation. This is required at >>>> this >>>> point, as altp2m lazily copies pages from the host's p2m and even >>>> might >>>> be flushed because of changes to the host's p2m (as it is done within >>>> the context of memory ballooning). >>> >>> I was expecting to see some change in >>> p2m_mem_access_check_and_get_page. Is there any reason to not fix it? >>> >>> >> >> I did not yet encounter any issues with >> p2m_mem_access_check_and_get_page. According to ARM ARM, ATS1C** (see >> gva_to_ipa_par) translates VA to IPA in non-secure privilege levels (as >> it is the the case here). Thus, the 2nd level translation represented by >> the (alt)p2m is not really considered at this point and hence make an >> extension obsolete. >> >> Or did you have anything else in mind? > > The stage-1 page tables are living in the guest memory. So every time > you access an entry in the page table, you have to translate the IPA > (guest physical address) into a PA. > > However, the underlying memory of those page table may have > restriction permission or does not exist in the altp2m at all. So the > translation will fail. > Please correct me if I am wrong but as far as I understand: the function p2m_mem_access_check_and_get_page is called only from get_page_from_gva. Also it is called only if the page translation within the function get_page_from_gva was not successful. Because of the fact that we use the hostp2m's 2nd stage translation table including the original memory access permissions (please note the short sequence, where we temporarily reset the VTTBR_EL2 of the hostp2m if altp2m is active), potential faults (which would lead to the call of the function p2m_mem_access_check_and_get_page) must have reasons beyond altp2m. Best regards, ~Sergej
On 06/08/2016 18:58, Sergej Proskurin wrote: > Hi Julien, Hello Sergej, > On 08/06/2016 03:45 PM, Julien Grall wrote: >> >> >> On 06/08/2016 11:38, Sergej Proskurin wrote: >>> Hi Julien, >> >> Hello Serge, >> >>> On 08/04/2016 01:59 PM, Julien Grall wrote: >>>> Hello Sergej, >>>> >>>> On 01/08/16 18:10, Sergej Proskurin wrote: >>>>> The function get_page_from_gva uses ARM's hardware support to >>>>> translate >>>>> gva's to machine addresses. This function is used, among others, for >>>>> memory regulation purposes, e.g, within the context of memory >>>>> ballooning. >>>>> To ensure correct behavior while altp2m is in use, we use the >>>>> host's p2m >>>>> table for the associated gva to ma translation. This is required at >>>>> this >>>>> point, as altp2m lazily copies pages from the host's p2m and even >>>>> might >>>>> be flushed because of changes to the host's p2m (as it is done within >>>>> the context of memory ballooning). >>>> >>>> I was expecting to see some change in >>>> p2m_mem_access_check_and_get_page. Is there any reason to not fix it? >>>> >>>> >>> >>> I did not yet encounter any issues with >>> p2m_mem_access_check_and_get_page. According to ARM ARM, ATS1C** (see >>> gva_to_ipa_par) translates VA to IPA in non-secure privilege levels (as >>> it is the the case here). Thus, the 2nd level translation represented by >>> the (alt)p2m is not really considered at this point and hence make an >>> extension obsolete. >>> >>> Or did you have anything else in mind? >> >> The stage-1 page tables are living in the guest memory. So every time >> you access an entry in the page table, you have to translate the IPA >> (guest physical address) into a PA. >> >> However, the underlying memory of those page table may have >> restriction permission or does not exist in the altp2m at all. So the >> translation will fail. >> > > Please correct me if I am wrong but as far as I understand: the function > p2m_mem_access_check_and_get_page is called only from get_page_from_gva. > Also it is called only if the page translation within the function > get_page_from_gva was not successful. Because of the fact that we use > the hostp2m's 2nd stage translation table including the original memory > access permissions (please note the short sequence, where we temporarily > reset the VTTBR_EL2 of the hostp2m if altp2m is active), potential > faults (which would lead to the call of the function > p2m_mem_access_check_and_get_page) must have reasons beyond altp2m. The translation in get_page_from_gva may fail if the permission in the hostp2m has been restricted by memaccess (for instance because default_access is not p2m_access_rwx). So you will fallback to p2m_mem_access_check_and_get_page. This function is calling gva_to_ipa that will use the altp2m to do the translation. Therefore I think you need to modify p2m_mem_access_check_and_get_page to cope with altp2m. Regards,
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index bcad51f..784f8da 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1614,7 +1614,7 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, unsigned long flags) { struct domain *d = v->domain; - struct p2m_domain *p2m = &d->arch.p2m; + struct p2m_domain *p2m = p2m_get_hostp2m(d); struct page_info *page = NULL; paddr_t maddr = 0; int rc; @@ -1628,7 +1628,34 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, p2m_read_lock(p2m); - rc = gvirt_to_maddr(va, &maddr, flags); + /* + * If altp2m is active, we still read the gva from the hostp2m, as it + * contains all valid mappings while the currently active altp2m view might + * not have the required gva mapping yet. + */ + if ( unlikely(altp2m_active(d)) ) + { + unsigned long irq_flags = 0; + uint64_t ovttbr = READ_SYSREG64(VTTBR_EL2); + + if ( ovttbr != p2m->vttbr.vttbr ) + { + local_irq_save(irq_flags); + WRITE_SYSREG64(p2m->vttbr.vttbr, VTTBR_EL2); + isb(); + } + + rc = gvirt_to_maddr(va, &maddr, flags); + + if ( ovttbr != p2m->vttbr.vttbr ) + { + WRITE_SYSREG64(ovttbr, VTTBR_EL2); + isb(); + local_irq_restore(irq_flags); + } + } + else + rc = gvirt_to_maddr(va, &maddr, flags); if ( rc ) goto err;
The function get_page_from_gva uses ARM's hardware support to translate gva's to machine addresses. This function is used, among others, for memory regulation purposes, e.g, within the context of memory ballooning. To ensure correct behavior while altp2m is in use, we use the host's p2m table for the associated gva to ma translation. This is required at this point, as altp2m lazily copies pages from the host's p2m and even might be flushed because of changes to the host's p2m (as it is done within the context of memory ballooning). Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/p2m.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)