| Message ID | 20240517032156.1490515-3-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Guest XenStore page allocation for 11 Dom0less domUs | expand |
Hi Henry, + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. The documentation of the new logic is in: https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ FWIW I am happy in principle with the logic (this is what we discussed on the call last week). Some comments below. On 17/05/2024 04:21, Henry Wang wrote: > There are use cases (for example using the PV driver) in Dom0less > setup that require Dom0less DomUs start immediately with Dom0, but > initialize XenStore later after Dom0's successful boot and call to > the init-dom0less application. > > An error message can seen from the init-dom0less application on > 1:1 direct-mapped domains: > ``` > Allocating magic pages > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1 > Error on alloc magic pages > ``` > > The "magic page" is a terminology used in the toolstack as reserved > pages for the VM to have access to virtual platform capabilities. > Currently the magic pages for Dom0less DomUs are populated by the > init-dom0less app through populate_physmap(), and populate_physmap() > automatically assumes gfn == mfn for 1:1 direct mapped domains. This > cannot be true for the magic pages that are allocated later from the > init-dom0less application executed in Dom0. For domain using statically > allocated memory but not 1:1 direct-mapped, similar error "failed to > retrieve a reserved page" can be seen as the reserved memory list is > empty at that time. > > Since for init-dom0less, the magic page region is only for XenStore. > To solve above issue, this commit allocates the XenStore page for > Dom0less DomUs at the domain construction time. The PFN will be > noted and communicated to the init-dom0less application executed > from Dom0. To keep the XenStore late init protocol, set the connection > status to XENSTORE_RECONNECT. So this commit is allocating the page, but it will not be used by init-dom0less until the next patch. But Linux could use it. So would this break bisection? If so, then I think patch #3 needs to be folded in this patch. > > Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> > Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com> > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > v3: > - Only allocate XenStore page. (Julien) > - Set HVM_PARAM_STORE_PFN and the XenStore connection status directly > from hypervisor. (Stefano) > v2: > - Reword the commit msg to explain what is "magic page" and use generic > terminology "hypervisor reserved pages" in commit msg. (Daniel) > - Also move the offset definition of magic pages. (Michal) > - Extract the magic page allocation logic to a function. (Michal) > --- > xen/arch/arm/dom0less-build.c | 44 ++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index 74f053c242..95c4fd1a2d 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -1,5 +1,6 @@ > /* SPDX-License-Identifier: GPL-2.0-only */ > #include <xen/device_tree.h> > +#include <xen/domain_page.h> > #include <xen/err.h> > #include <xen/event.h> > #include <xen/grant_table.h> > @@ -10,6 +11,8 @@ > #include <xen/sizes.h> > #include <xen/vmap.h> > > +#include <public/io/xs_wire.h> > + > #include <asm/arm64/sve.h> > #include <asm/dom0less-build.h> > #include <asm/domain_build.h> > @@ -739,6 +742,42 @@ static int __init alloc_xenstore_evtchn(struct domain *d) > return 0; > } > > +#define XENSTORE_PFN_OFFSET 1 > +static int __init alloc_xenstore_page(struct domain *d) > +{ > + struct page_info *xenstore_pg; > + struct xenstore_domain_interface *interface; > + mfn_t mfn; > + gfn_t gfn; > + int rc; > + > + d->max_pages += 1; Sorry I should have spotted it earlier. But you want to check d->max_pages is not overflowing. You can look at acquire_shared_memory_bank() for how to do it. Also, maybe we want an helper to do it so it is not open-coded in multiple places. > + xenstore_pg = alloc_domheap_page(d, 0); I think we may want to restrict where the page is allocated. For instance, 32-bit domain using short page-tables will not be able to address all the physical memory. I would consider to try to allocate the page below 32-bit (using MEMF-bits(32). And then fallback to above 32-bit only 64-bit for domain. Also, just to note that in theory alloc_domheap_page() could return MFN 0. In practice we have excluded MFN 0 because it breaks the page allocator so far. But I would still prefer if we add a check on the MFN. This will make easier to spot any issue if we ever give MFN 0 to the allocator. A possible implementation would be to call alloc_domphea_page() a second time and then free the first one (e.g. MFN 0). > + if ( xenstore_pg == NULL ) > + return -ENOMEM; > + > + mfn = page_to_mfn(xenstore_pg); > + if ( !is_domain_direct_mapped(d) ) > + gfn = gaddr_to_gfn(GUEST_MAGIC_BASE + > + (XENSTORE_PFN_OFFSET << PAGE_SHIFT)); > + else > + gfn = gaddr_to_gfn(mfn_to_maddr(mfn)); > + > + rc = guest_physmap_add_page(d, gfn, mfn, 0); > + if ( rc ) > + { > + free_domheap_page(xenstore_pg); > + return rc; > + } > + > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn); > + interface = (struct xenstore_domain_interface *)map_domain_page(mfn); map_domain_page() will return a void *. So why do you need to an explicit case? > + interface->connection = XENSTORE_RECONNECT; > + unmap_domain_page(interface); > + > + return 0; > +} > + > static int __init construct_domU(struct domain *d, > const struct dt_device_node *node) > { > @@ -839,7 +878,10 @@ static int __init construct_domU(struct domain *d, > rc = alloc_xenstore_evtchn(d); > if ( rc < 0 ) > return rc; > - d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; > + > + rc = alloc_xenstore_page(d); > + if ( rc < 0 ) > + return rc; > } > > return rc; Cheers,
On 24.05.24 15:58, Julien Grall wrote: > Hi Henry, > > + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. The > documentation of the new logic is in: > > https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ > > FWIW I am happy in principle with the logic (this is what we discussed on the > call last week). Some comments below. I'm not against this logic, but I'm wondering why it needs to be so complicated. Can't the domU itself allocate the Xenstore page from its RAM pages, write the PFN into the Xenstore grant tab entry, and then make it public via setting HVM_PARAM_STORE_PFN? The init-dom0less application could then check HVM_PARAM_STORE_PFN being set and call XS_introduce_domain. Note that at least C-xenstored does not need the PFN of the Xenstore page, as it is just using GNTTAB_RESERVED_XENSTORE for mapping the page. Juergen
On 24.05.24 16:30, Jürgen Groß wrote: > On 24.05.24 15:58, Julien Grall wrote: >> Hi Henry, >> >> + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. >> The documentation of the new logic is in: >> >> https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ >> >> FWIW I am happy in principle with the logic (this is what we discussed on the >> call last week). Some comments below. > > I'm not against this logic, but I'm wondering why it needs to be so > complicated. > > Can't the domU itself allocate the Xenstore page from its RAM pages, > write the PFN into the Xenstore grant tab entry, and then make it > public via setting HVM_PARAM_STORE_PFN? > > The init-dom0less application could then check HVM_PARAM_STORE_PFN > being set and call XS_introduce_domain. > > Note that at least C-xenstored does not need the PFN of the Xenstore > page, as it is just using GNTTAB_RESERVED_XENSTORE for mapping the > page. Hmm, seems as if O-xenstored is using the map_foreign_page mechanism with the PFN to map the Xenstore ring page. Maybe this should be changed to use the grant entry, too? Juergen
On Fri, 24 May 2024, Julien Grall wrote: > Hi Henry, > > + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. > The documentation of the new logic is in: > > https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ > > FWIW I am happy in principle with the logic (this is what we discussed on the > call last week). Some comments below. > > On 17/05/2024 04:21, Henry Wang wrote: > > There are use cases (for example using the PV driver) in Dom0less > > setup that require Dom0less DomUs start immediately with Dom0, but > > initialize XenStore later after Dom0's successful boot and call to > > the init-dom0less application. > > > > An error message can seen from the init-dom0less application on > > 1:1 direct-mapped domains: > > ``` > > Allocating magic pages > > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1 > > Error on alloc magic pages > > ``` > > > > The "magic page" is a terminology used in the toolstack as reserved > > pages for the VM to have access to virtual platform capabilities. > > Currently the magic pages for Dom0less DomUs are populated by the > > init-dom0less app through populate_physmap(), and populate_physmap() > > automatically assumes gfn == mfn for 1:1 direct mapped domains. This > > cannot be true for the magic pages that are allocated later from the > > init-dom0less application executed in Dom0. For domain using statically > > allocated memory but not 1:1 direct-mapped, similar error "failed to > > retrieve a reserved page" can be seen as the reserved memory list is > > empty at that time. > > > > Since for init-dom0less, the magic page region is only for XenStore. > > To solve above issue, this commit allocates the XenStore page for > > Dom0less DomUs at the domain construction time. The PFN will be > > noted and communicated to the init-dom0less application executed > > from Dom0. To keep the XenStore late init protocol, set the connection > > status to XENSTORE_RECONNECT. > > So this commit is allocating the page, but it will not be used by > init-dom0less until the next patch. But Linux could use it. So would this > break bisection? If so, then I think patch #3 needs to be folded in this > patch. I think that's fine, I'll leave that with you on commit. I'll resend as is addressing the other comments. > > > > Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> > > Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > Signed-off-by: Henry Wang <xin.wang2@amd.com> > > --- > > v3: > > - Only allocate XenStore page. (Julien) > > - Set HVM_PARAM_STORE_PFN and the XenStore connection status directly > > from hypervisor. (Stefano) > > v2: > > - Reword the commit msg to explain what is "magic page" and use generic > > terminology "hypervisor reserved pages" in commit msg. (Daniel) > > - Also move the offset definition of magic pages. (Michal) > > - Extract the magic page allocation logic to a function. (Michal) > > --- > > xen/arch/arm/dom0less-build.c | 44 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 43 insertions(+), 1 deletion(-) > > > > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > > index 74f053c242..95c4fd1a2d 100644 > > --- a/xen/arch/arm/dom0less-build.c > > +++ b/xen/arch/arm/dom0less-build.c > > @@ -1,5 +1,6 @@ > > /* SPDX-License-Identifier: GPL-2.0-only */ > > #include <xen/device_tree.h> > > +#include <xen/domain_page.h> > > #include <xen/err.h> > > #include <xen/event.h> > > #include <xen/grant_table.h> > > @@ -10,6 +11,8 @@ > > #include <xen/sizes.h> > > #include <xen/vmap.h> > > +#include <public/io/xs_wire.h> > > + > > #include <asm/arm64/sve.h> > > #include <asm/dom0less-build.h> > > #include <asm/domain_build.h> > > @@ -739,6 +742,42 @@ static int __init alloc_xenstore_evtchn(struct domain > > *d) > > return 0; > > } > > +#define XENSTORE_PFN_OFFSET 1 > > +static int __init alloc_xenstore_page(struct domain *d) > > +{ > > + struct page_info *xenstore_pg; > > + struct xenstore_domain_interface *interface; > > + mfn_t mfn; > > + gfn_t gfn; > > + int rc; > > + > > + d->max_pages += 1; > > Sorry I should have spotted it earlier. But you want to check d->max_pages is > not overflowing. You can look at acquire_shared_memory_bank() for how to do > it. > > Also, maybe we want an helper to do it so it is not open-coded in multiple > places. Makes sense. I open-coded it because I wasn't sure if you preferred a static inline or a macro and where to add the implementation. > > + xenstore_pg = alloc_domheap_page(d, 0); > > I think we may want to restrict where the page is allocated. For instance, > 32-bit domain using short page-tables will not be able to address all the > physical memory. > > I would consider to try to allocate the page below 32-bit (using > MEMF-bits(32). And then fallback to above 32-bit only 64-bit for domain. done > Also, just to note that in theory alloc_domheap_page() could return MFN 0. In > practice we have excluded MFN 0 because it breaks the page allocator so far. > > But I would still prefer if we add a check on the MFN. This will make easier > to spot any issue if we ever give MFN 0 to the allocator. Good idea, but for simplicity I added a check on MFN and if the result is zero return an error. At least we have explicit check. > A possible implementation would be to call alloc_domphea_page() a second time > and then free the first one (e.g. MFN 0). > > > + if ( xenstore_pg == NULL ) > > + return -ENOMEM; > > + > > + mfn = page_to_mfn(xenstore_pg); > > + if ( !is_domain_direct_mapped(d) ) > > + gfn = gaddr_to_gfn(GUEST_MAGIC_BASE + > > + (XENSTORE_PFN_OFFSET << PAGE_SHIFT)); > > + else > > + gfn = gaddr_to_gfn(mfn_to_maddr(mfn)); > > + > > + rc = guest_physmap_add_page(d, gfn, mfn, 0); > + if ( rc ) > > + { > > + free_domheap_page(xenstore_pg); > > + return rc; > > + } > > + > > + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn); > > + interface = (struct xenstore_domain_interface *)map_domain_page(mfn); > > map_domain_page() will return a void *. So why do you need to an explicit > case? Changed > > + interface->connection = XENSTORE_RECONNECT; > > + unmap_domain_page(interface); > > + > > + return 0; > > +} > > + > > static int __init construct_domU(struct domain *d, > > const struct dt_device_node *node) > > { > > @@ -839,7 +878,10 @@ static int __init construct_domU(struct domain *d, > > rc = alloc_xenstore_evtchn(d); > > if ( rc < 0 ) > > return rc; > > - d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; > > + > > + rc = alloc_xenstore_page(d); > > + if ( rc < 0 ) > > + return rc; > > } > > return rc; > > Cheers, > > -- > Julien Grall >
Hi Stefano, On 24/05/2024 23:49, Stefano Stabellini wrote: > On Fri, 24 May 2024, Julien Grall wrote: >> Hi Henry, >> >> + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. >> The documentation of the new logic is in: >> >> https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ >> >> FWIW I am happy in principle with the logic (this is what we discussed on the >> call last week). Some comments below. >> >> On 17/05/2024 04:21, Henry Wang wrote: >>> There are use cases (for example using the PV driver) in Dom0less >>> setup that require Dom0less DomUs start immediately with Dom0, but >>> initialize XenStore later after Dom0's successful boot and call to >>> the init-dom0less application. >>> >>> An error message can seen from the init-dom0less application on >>> 1:1 direct-mapped domains: >>> ``` >>> Allocating magic pages >>> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1 >>> Error on alloc magic pages >>> ``` >>> >>> The "magic page" is a terminology used in the toolstack as reserved >>> pages for the VM to have access to virtual platform capabilities. >>> Currently the magic pages for Dom0less DomUs are populated by the >>> init-dom0less app through populate_physmap(), and populate_physmap() >>> automatically assumes gfn == mfn for 1:1 direct mapped domains. This >>> cannot be true for the magic pages that are allocated later from the >>> init-dom0less application executed in Dom0. For domain using statically >>> allocated memory but not 1:1 direct-mapped, similar error "failed to >>> retrieve a reserved page" can be seen as the reserved memory list is >>> empty at that time. >>> >>> Since for init-dom0less, the magic page region is only for XenStore. >>> To solve above issue, this commit allocates the XenStore page for >>> Dom0less DomUs at the domain construction time. The PFN will be >>> noted and communicated to the init-dom0less application executed >>> from Dom0. To keep the XenStore late init protocol, set the connection >>> status to XENSTORE_RECONNECT. >> >> So this commit is allocating the page, but it will not be used by >> init-dom0less until the next patch. But Linux could use it. So would this >> break bisection? If so, then I think patch #3 needs to be folded in this >> patch. > > I think that's fine, I am not sure what you mean. Are you saying it is ok to break bisection? > I'll leave that with you on commit. I am sorry but I don't think the folding should be done on commit. It should happen before hand because the commit message will also need to be updated. Cheers,
On Fri, 24 May 2024, Julien Grall wrote: > Hi Stefano, > > On 24/05/2024 23:49, Stefano Stabellini wrote: > > On Fri, 24 May 2024, Julien Grall wrote: > > > Hi Henry, > > > > > > + Juergen as the Xenstore maintainers. I'd like his opinion on the > > > approach. > > > The documentation of the new logic is in: > > > > > > https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ > > > > > > FWIW I am happy in principle with the logic (this is what we discussed on > > > the > > > call last week). Some comments below. > > > > > > On 17/05/2024 04:21, Henry Wang wrote: > > > > There are use cases (for example using the PV driver) in Dom0less > > > > setup that require Dom0less DomUs start immediately with Dom0, but > > > > initialize XenStore later after Dom0's successful boot and call to > > > > the init-dom0less application. > > > > > > > > An error message can seen from the init-dom0less application on > > > > 1:1 direct-mapped domains: > > > > ``` > > > > Allocating magic pages > > > > memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1 > > > > Error on alloc magic pages > > > > ``` > > > > > > > > The "magic page" is a terminology used in the toolstack as reserved > > > > pages for the VM to have access to virtual platform capabilities. > > > > Currently the magic pages for Dom0less DomUs are populated by the > > > > init-dom0less app through populate_physmap(), and populate_physmap() > > > > automatically assumes gfn == mfn for 1:1 direct mapped domains. This > > > > cannot be true for the magic pages that are allocated later from the > > > > init-dom0less application executed in Dom0. For domain using statically > > > > allocated memory but not 1:1 direct-mapped, similar error "failed to > > > > retrieve a reserved page" can be seen as the reserved memory list is > > > > empty at that time. > > > > > > > > Since for init-dom0less, the magic page region is only for XenStore. > > > > To solve above issue, this commit allocates the XenStore page for > > > > Dom0less DomUs at the domain construction time. The PFN will be > > > > noted and communicated to the init-dom0less application executed > > > > from Dom0. To keep the XenStore late init protocol, set the connection > > > > status to XENSTORE_RECONNECT. > > > > > > So this commit is allocating the page, but it will not be used by > > > init-dom0less until the next patch. But Linux could use it. So would this > > > break bisection? If so, then I think patch #3 needs to be folded in this > > > patch. > > > > I think that's fine, > > I am not sure what you mean. Are you saying it is ok to break bisection? No, I meant to say that it is fine to merge on commit. > > I'll leave that with you on commit. > > I am sorry but I don't think the folding should be done on commit. It should > happen before hand because the commit message will also need to be updated. Understood. I'll send one more version with the patches merged (ideally with an ack?)
Hi, On 25/05/2024 00:02, Stefano Stabellini wrote: > On Fri, 24 May 2024, Julien Grall wrote: >> Hi Stefano, >> >> On 24/05/2024 23:49, Stefano Stabellini wrote: >>> On Fri, 24 May 2024, Julien Grall wrote: >>>> Hi Henry, >>>> >>>> + Juergen as the Xenstore maintainers. I'd like his opinion on the >>>> approach. >>>> The documentation of the new logic is in: >>>> >>>> https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ >>>> >>>> FWIW I am happy in principle with the logic (this is what we discussed on >>>> the >>>> call last week). Some comments below. >>>> >>>> On 17/05/2024 04:21, Henry Wang wrote: >>>>> There are use cases (for example using the PV driver) in Dom0less >>>>> setup that require Dom0less DomUs start immediately with Dom0, but >>>>> initialize XenStore later after Dom0's successful boot and call to >>>>> the init-dom0less application. >>>>> >>>>> An error message can seen from the init-dom0less application on >>>>> 1:1 direct-mapped domains: >>>>> ``` >>>>> Allocating magic pages >>>>> memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1 >>>>> Error on alloc magic pages >>>>> ``` >>>>> >>>>> The "magic page" is a terminology used in the toolstack as reserved >>>>> pages for the VM to have access to virtual platform capabilities. >>>>> Currently the magic pages for Dom0less DomUs are populated by the >>>>> init-dom0less app through populate_physmap(), and populate_physmap() >>>>> automatically assumes gfn == mfn for 1:1 direct mapped domains. This >>>>> cannot be true for the magic pages that are allocated later from the >>>>> init-dom0less application executed in Dom0. For domain using statically >>>>> allocated memory but not 1:1 direct-mapped, similar error "failed to >>>>> retrieve a reserved page" can be seen as the reserved memory list is >>>>> empty at that time. >>>>> >>>>> Since for init-dom0less, the magic page region is only for XenStore. >>>>> To solve above issue, this commit allocates the XenStore page for >>>>> Dom0less DomUs at the domain construction time. The PFN will be >>>>> noted and communicated to the init-dom0less application executed >>>>> from Dom0. To keep the XenStore late init protocol, set the connection >>>>> status to XENSTORE_RECONNECT. >>>> >>>> So this commit is allocating the page, but it will not be used by >>>> init-dom0less until the next patch. But Linux could use it. So would this >>>> break bisection? If so, then I think patch #3 needs to be folded in this >>>> patch. >>> >>> I think that's fine, >> >> I am not sure what you mean. Are you saying it is ok to break bisection? > > No, I meant to say that it is fine to merge on commit. > > >>> I'll leave that with you on commit. >> >> I am sorry but I don't think the folding should be done on commit. It should >> happen before hand because the commit message will also need to be updated. > > Understood. I'll send one more version with the patches merged (ideally > with an ack?) Sorry I don't feel it is right for me to ack this patch with the pending questions from Juergen. Cheers,
On Fri, 24 May 2024, Jürgen Groß wrote: > On 24.05.24 15:58, Julien Grall wrote: > > Hi Henry, > > > > + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. > > The documentation of the new logic is in: > > > > https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ > > > > FWIW I am happy in principle with the logic (this is what we discussed on > > the call last week). Some comments below. > > I'm not against this logic, but I'm wondering why it needs to be so > complicated. Actually the reason I like it is that in my view, this is the simplest approach. You allocate a domain, you also allocate the xenstore page together with it. Initially the xenstore connection has an "uninitialized" state, as it should be. That's it. At some point, when xenstored is ready, the state changes to CONNECTED. > Can't the domU itself allocate the Xenstore page from its RAM pages, > write the PFN into the Xenstore grant tab entry, and then make it > public via setting HVM_PARAM_STORE_PFN? This is not simpler in my view > The init-dom0less application could then check HVM_PARAM_STORE_PFN > being set and call XS_introduce_domain. > > Note that at least C-xenstored does not need the PFN of the Xenstore > page, as it is just using GNTTAB_RESERVED_XENSTORE for mapping the > page.
On 25.05.24 01:19, Stefano Stabellini wrote: > On Fri, 24 May 2024, Jürgen Groß wrote: >> On 24.05.24 15:58, Julien Grall wrote: >>> Hi Henry, >>> >>> + Juergen as the Xenstore maintainers. I'd like his opinion on the approach. >>> The documentation of the new logic is in: >>> >>> https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ >>> >>> FWIW I am happy in principle with the logic (this is what we discussed on >>> the call last week). Some comments below. >> >> I'm not against this logic, but I'm wondering why it needs to be so >> complicated. > > Actually the reason I like it is that in my view, this is the simplest > approach. You allocate a domain, you also allocate the xenstore page > together with it. Initially the xenstore connection has an > "uninitialized" state, as it should be. That's it. At some point, when > xenstored is ready, the state changes to CONNECTED. > > >> Can't the domU itself allocate the Xenstore page from its RAM pages, >> write the PFN into the Xenstore grant tab entry, and then make it >> public via setting HVM_PARAM_STORE_PFN? > > This is not simpler in my view Okay, fine with me. I had the impression that violating the 1:1 mapping of the domain would add complexity, but if you are fine with that I don't mind your approach. Juergen
On Mon, 27 May 2024, Jürgen Groß wrote: > On 25.05.24 01:19, Stefano Stabellini wrote: > > On Fri, 24 May 2024, Jürgen Groß wrote: > > > On 24.05.24 15:58, Julien Grall wrote: > > > > Hi Henry, > > > > > > > > + Juergen as the Xenstore maintainers. I'd like his opinion on the > > > > approach. > > > > The documentation of the new logic is in: > > > > > > > > https://lore.kernel.org/xen-devel/20240517032156.1490515-5-xin.wang2@amd.com/ > > > > > > > > FWIW I am happy in principle with the logic (this is what we discussed > > > > on > > > > the call last week). Some comments below. > > > > > > I'm not against this logic, but I'm wondering why it needs to be so > > > complicated. > > > > Actually the reason I like it is that in my view, this is the simplest > > approach. You allocate a domain, you also allocate the xenstore page > > together with it. Initially the xenstore connection has an > > "uninitialized" state, as it should be. That's it. At some point, when > > xenstored is ready, the state changes to CONNECTED. > > > > > > > Can't the domU itself allocate the Xenstore page from its RAM pages, > > > write the PFN into the Xenstore grant tab entry, and then make it > > > public via setting HVM_PARAM_STORE_PFN? > > > > This is not simpler in my view > > Okay, fine with me. I had the impression that violating the 1:1 mapping > of the domain would add complexity, but if you are fine with that I don't > mind your approach. Yes, that's fine. Thanks!
diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 74f053c242..95c4fd1a2d 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */ #include <xen/device_tree.h> +#include <xen/domain_page.h> #include <xen/err.h> #include <xen/event.h> #include <xen/grant_table.h> @@ -10,6 +11,8 @@ #include <xen/sizes.h> #include <xen/vmap.h> +#include <public/io/xs_wire.h> + #include <asm/arm64/sve.h> #include <asm/dom0less-build.h> #include <asm/domain_build.h> @@ -739,6 +742,42 @@ static int __init alloc_xenstore_evtchn(struct domain *d) return 0; } +#define XENSTORE_PFN_OFFSET 1 +static int __init alloc_xenstore_page(struct domain *d) +{ + struct page_info *xenstore_pg; + struct xenstore_domain_interface *interface; + mfn_t mfn; + gfn_t gfn; + int rc; + + d->max_pages += 1; + xenstore_pg = alloc_domheap_page(d, 0); + if ( xenstore_pg == NULL ) + return -ENOMEM; + + mfn = page_to_mfn(xenstore_pg); + if ( !is_domain_direct_mapped(d) ) + gfn = gaddr_to_gfn(GUEST_MAGIC_BASE + + (XENSTORE_PFN_OFFSET << PAGE_SHIFT)); + else + gfn = gaddr_to_gfn(mfn_to_maddr(mfn)); + + rc = guest_physmap_add_page(d, gfn, mfn, 0); + if ( rc ) + { + free_domheap_page(xenstore_pg); + return rc; + } + + d->arch.hvm.params[HVM_PARAM_STORE_PFN] = gfn_x(gfn); + interface = (struct xenstore_domain_interface *)map_domain_page(mfn); + interface->connection = XENSTORE_RECONNECT; + unmap_domain_page(interface); + + return 0; +} + static int __init construct_domU(struct domain *d, const struct dt_device_node *node) { @@ -839,7 +878,10 @@ static int __init construct_domU(struct domain *d, rc = alloc_xenstore_evtchn(d); if ( rc < 0 ) return rc; - d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL; + + rc = alloc_xenstore_page(d); + if ( rc < 0 ) + return rc; } return rc;
There are use cases (for example using the PV driver) in Dom0less setup that require Dom0less DomUs start immediately with Dom0, but initialize XenStore later after Dom0's successful boot and call to the init-dom0less application. An error message can seen from the init-dom0less application on 1:1 direct-mapped domains: ``` Allocating magic pages memory.c:238:d0v0 mfn 0x39000 doesn't belong to d1 Error on alloc magic pages ``` The "magic page" is a terminology used in the toolstack as reserved pages for the VM to have access to virtual platform capabilities. Currently the magic pages for Dom0less DomUs are populated by the init-dom0less app through populate_physmap(), and populate_physmap() automatically assumes gfn == mfn for 1:1 direct mapped domains. This cannot be true for the magic pages that are allocated later from the init-dom0less application executed in Dom0. For domain using statically allocated memory but not 1:1 direct-mapped, similar error "failed to retrieve a reserved page" can be seen as the reserved memory list is empty at that time. Since for init-dom0less, the magic page region is only for XenStore. To solve above issue, this commit allocates the XenStore page for Dom0less DomUs at the domain construction time. The PFN will be noted and communicated to the init-dom0less application executed from Dom0. To keep the XenStore late init protocol, set the connection status to XENSTORE_RECONNECT. Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> Suggested-by: Daniel P. Smith <dpsmith@apertussolutions.com> Signed-off-by: Henry Wang <xin.wang2@amd.com> --- v3: - Only allocate XenStore page. (Julien) - Set HVM_PARAM_STORE_PFN and the XenStore connection status directly from hypervisor. (Stefano) v2: - Reword the commit msg to explain what is "magic page" and use generic terminology "hypervisor reserved pages" in commit msg. (Daniel) - Also move the offset definition of magic pages. (Michal) - Extract the magic page allocation logic to a function. (Michal) --- xen/arch/arm/dom0less-build.c | 44 ++++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-)