| Message ID | 1633519346-3686-2-git-send-email-olekstysh@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add handling of extended regions (safe ranges) on Arm (Was "xen/memory: Introduce a hypercall to provide unallocated space") | expand |
On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: > From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > > We need to pass info about maximum supported guest physical > address space size to the toolstack on Arm in order to properly > calculate the base and size of the extended region (safe range) > for the guest. The extended region is unused address space which > could be safely used by domain for foreign/grant mappings on Arm. > The extended region itself will be handled by the subsequent > patch. > > Currently the same guest physical address space size is used > for all guests. > > As we add new field to the structure bump the interface version. > > Suggested-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> > --- > Changes RFC -> V2: > - update patch subject/description > - replace arch-specific sub-struct with common gpaddr_bits > field and update code to reflect that > > Changes V2 -> V3: > - make the field uint8_t and add uint8_t pad[7] after > - remove leading blanks in libxl.h > > Changes V3 -> V4: > - also print gpaddr_bits from output_physinfo() > - add Michal's R-b > > Changes V4 -> V5: > - update patch subject and description > - drop Michal's R-b > - pass gpaddr_bits via createdomain domctl > (struct xen_arch_domainconfig) > --- > tools/include/libxl.h | 5 +++++ > tools/libs/light/libxl_arm.c | 2 ++ > tools/libs/light/libxl_types.idl | 1 + > xen/arch/arm/domain.c | 6 ++++++ > xen/include/public/arch-arm.h | 5 +++++ > xen/include/public/domctl.h | 2 +- > 6 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/tools/include/libxl.h b/tools/include/libxl.h > index b9ba16d..33b4bfb 100644 > --- a/tools/include/libxl.h > +++ b/tools/include/libxl.h > @@ -279,6 +279,11 @@ > #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1 > > /* > + * libxl_domain_build_info has the gpaddr_bits field. > + */ > +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1 > + > +/* > * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing > * 'soft reset' for domains and there is 'soft_reset' shutdown reason > * in enum libxl_shutdown_reason. > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c > index e3140a6..45e0386 100644 > --- a/tools/libs/light/libxl_arm.c > +++ b/tools/libs/light/libxl_arm.c > @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc, > > state->clock_frequency = config->arch.clock_frequency; > > + d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits; > + > return 0; > } > > diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl > index 3f9fff6..39482db 100644 > --- a/tools/libs/light/libxl_types.idl > +++ b/tools/libs/light/libxl_types.idl > @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ > > ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), > ("vuart", libxl_vuart_type), > + ("gpaddr_bits", uint8), > ])), > ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), > ])), > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c > index 19c756a..dfecc45 100644 > --- a/xen/arch/arm/domain.c > +++ b/xen/arch/arm/domain.c > @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d, > if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) > goto fail; > > + /* > + * Pass maximum IPA bits to the toolstack, currently the same guest > + * physical address space size is used for all guests. > + */ > + config->arch.gpaddr_bits = p2m_ipa_bits; This could also be set in arch_sanitise_domain_config together with config->arch.gic_version. I prefer if it was done in arch_sanitise_domain_config but also here is OK I think. Given that everything else looks fine: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > return 0; > > fail: > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 6b5a5f8..4a01f8b 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { > * > */ > uint32_t clock_frequency; > + /* > + * OUT > + * Guest physical address space size > + */ > + uint8_t gpaddr_bits; > }; > #endif /* __XEN__ || __XEN_TOOLS__ */ > > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 96696e3..f37586e 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -38,7 +38,7 @@ > #include "hvm/save.h" > #include "memory.h" > > -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014 > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015 > > /* > * NB. xen_domctl.domain is an IN/OUT parameter for this operation. > -- > 2.7.4 >
On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: > Changes V4 -> V5: > - update patch subject and description > - drop Michal's R-b > - pass gpaddr_bits via createdomain domctl > (struct xen_arch_domainconfig) I'm afraid I can't bring this in line with ... > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { > * > */ > uint32_t clock_frequency; > + /* > + * OUT > + * Guest physical address space size > + */ > + uint8_t gpaddr_bits; ... this being an OUT field. Is this really what Andrew had asked for? I would have expected the entire struct to be IN (and the comment at the top of the containing struct in public/domctl.h also suggests so, i.e. your new field renders that comment stale). gic_version being IN/OUT is already somewhat in conflict ... One of the problems with _any_ of the fields being OUT is that then it is unclear how the output is intended to be propagated to consumers other than the entity creating the domain. Jan
On 07.10.21 10:42, Jan Beulich wrote: Hi Jan. > On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: >> Changes V4 -> V5: >> - update patch subject and description >> - drop Michal's R-b >> - pass gpaddr_bits via createdomain domctl >> (struct xen_arch_domainconfig) > I'm afraid I can't bring this in line with ... > >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { >> * >> */ >> uint32_t clock_frequency; >> + /* >> + * OUT >> + * Guest physical address space size >> + */ >> + uint8_t gpaddr_bits; > ... this being an OUT field. Is this really what Andrew had asked for? > I would have expected the entire struct to be IN (and the comment at > the top of the containing struct in public/domctl.h also suggests so, > i.e. your new field renders that comment stale). gic_version being > IN/OUT is already somewhat in conflict ... I am sorry but I'm totally confused now, we want the Xen to provide gpaddr_bits to the toolstack, but not the other way around. As I understand the main ask was to switch to domctl for which I wanted to get some clarification on how it would look like... Well, this patch switches to use domctl, and I think exactly as it was suggested at [1] in case if a common one is a difficult to achieve. I have to admit, I felt it was indeed difficult to achieve. I thought that a comment for struct xen_domctl_createdomain in public/domctl.h was rather related to the struct fields described in the public header than to the arch sub-struct internals described elsewhere. But if my assumption is incorrect, then yes the comment got stale and probably not by changes in current patch, but after adding clock_frequency field (OUT). If so we can add a comment on top of arch field clarifying that internal fields *might* be OUT. > One of the problems with > _any_ of the fields being OUT is that then it is unclear how the output > is intended to be propagated to consumers other than the entity > creating the domain. If I *properly* understood your concern, we could hide that field in struct libxl__domain_build_state and not expose it to struct libxl_domain_build_info. Shall I? [1] https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@gmail.com/
On 07.10.2021 14:30, Oleksandr wrote: > On 07.10.21 10:42, Jan Beulich wrote: >> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: >>> Changes V4 -> V5: >>> - update patch subject and description >>> - drop Michal's R-b >>> - pass gpaddr_bits via createdomain domctl >>> (struct xen_arch_domainconfig) >> I'm afraid I can't bring this in line with ... >> >>> --- a/xen/include/public/arch-arm.h >>> +++ b/xen/include/public/arch-arm.h >>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { >>> * >>> */ >>> uint32_t clock_frequency; >>> + /* >>> + * OUT >>> + * Guest physical address space size >>> + */ >>> + uint8_t gpaddr_bits; >> ... this being an OUT field. Is this really what Andrew had asked for? >> I would have expected the entire struct to be IN (and the comment at >> the top of the containing struct in public/domctl.h also suggests so, >> i.e. your new field renders that comment stale). gic_version being >> IN/OUT is already somewhat in conflict ... > I am sorry but I'm totally confused now, we want the Xen to provide > gpaddr_bits to the toolstack, but not the other way around. > As I understand the main ask was to switch to domctl for which I wanted > to get some clarification on how it would look like... Well, this patch > switches to use > domctl, and I think exactly as it was suggested at [1] in case if a > common one is a difficult to achieve. I have to admit, I felt it was > indeed difficult to achieve. Sadly the mail you reference isn't the one I was referring to. It's not even from Andrew. Unfortunately I also can't seem to be able to locate this, i.e. I'm now wondering whether this was under a different subject. Julien, in any event, confirmed in a recent reply on this thread that there was such a mail (otherwise I would have started wondering whether the request was made on irc). In any case it is _that_ mail that would need going through again. > I thought that a comment for struct xen_domctl_createdomain in > public/domctl.h was rather related to the struct fields described in the > public header than to the arch sub-struct internals described elsewhere. > But if my assumption is incorrect, then yes the comment got stale and > probably not by changes in current patch, but after adding > clock_frequency field (OUT). If so we can add a comment on top of arch > field clarifying that internal fields *might* be OUT. > > >> One of the problems with >> _any_ of the fields being OUT is that then it is unclear how the output >> is intended to be propagated to consumers other than the entity >> creating the domain. > If I *properly* understood your concern, we could hide that field in > struct libxl__domain_build_state and not expose it to struct > libxl_domain_build_info. Shall I? I'm afraid I'm lost: I didn't talk about the tool stack at all. While "consumer" generally means the tool stack, the remark was of more abstract nature. Jan > [1] > https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@gmail.com/ > >
On 07.10.21 15:43, Jan Beulich wrote: Hi Jan. > On 07.10.2021 14:30, Oleksandr wrote: >> On 07.10.21 10:42, Jan Beulich wrote: >>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: >>>> Changes V4 -> V5: >>>> - update patch subject and description >>>> - drop Michal's R-b >>>> - pass gpaddr_bits via createdomain domctl >>>> (struct xen_arch_domainconfig) >>> I'm afraid I can't bring this in line with ... >>> >>>> --- a/xen/include/public/arch-arm.h >>>> +++ b/xen/include/public/arch-arm.h >>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { >>>> * >>>> */ >>>> uint32_t clock_frequency; >>>> + /* >>>> + * OUT >>>> + * Guest physical address space size >>>> + */ >>>> + uint8_t gpaddr_bits; >>> ... this being an OUT field. Is this really what Andrew had asked for? >>> I would have expected the entire struct to be IN (and the comment at >>> the top of the containing struct in public/domctl.h also suggests so, >>> i.e. your new field renders that comment stale). gic_version being >>> IN/OUT is already somewhat in conflict ... >> I am sorry but I'm totally confused now, we want the Xen to provide >> gpaddr_bits to the toolstack, but not the other way around. >> As I understand the main ask was to switch to domctl for which I wanted >> to get some clarification on how it would look like... Well, this patch >> switches to use >> domctl, and I think exactly as it was suggested at [1] in case if a >> common one is a difficult to achieve. I have to admit, I felt it was >> indeed difficult to achieve. > Sadly the mail you reference isn't the one I was referring to. It's not > even from Andrew. Unfortunately I also can't seem to be able to locate > this, i.e. I'm now wondering whether this was under a different subject. > Julien, in any event, confirmed in a recent reply on this thread that > there was such a mail (otherwise I would have started wondering whether > the request was made on irc). In any case it is _that_ mail that would > need going through again. I think, this is the email [1] you are referring to. The subject was changed to reflect changes in the particular version. This is the third proposition of this patch (the first two were with arch and common field in sysctl). >> I thought that a comment for struct xen_domctl_createdomain in >> public/domctl.h was rather related to the struct fields described in the >> public header than to the arch sub-struct internals described elsewhere. >> But if my assumption is incorrect, then yes the comment got stale and >> probably not by changes in current patch, but after adding >> clock_frequency field (OUT). If so we can add a comment on top of arch >> field clarifying that internal fields *might* be OUT. >> >> >>> One of the problems with >>> _any_ of the fields being OUT is that then it is unclear how the output >>> is intended to be propagated to consumers other than the entity >>> creating the domain. >> If I *properly* understood your concern, we could hide that field in >> struct libxl__domain_build_state and not expose it to struct >> libxl_domain_build_info. Shall I? > I'm afraid I'm lost: I didn't talk about the tool stack at all. While > "consumer" generally means the tool stack, the remark was of more > abstract nature. > > Jan > >> [1] >> https://lore.kernel.org/xen-devel/093bc1d5-bf6a-da0a-78b5-7a8dd471a063@gmail.com/ >> >> [1] https://lore.kernel.org/xen-devel/6a2a183d-c9d8-df2a-41aa-b25283fab197@gmail.com/
On 07.10.2021 15:12, Oleksandr wrote: > > On 07.10.21 15:43, Jan Beulich wrote: > > Hi Jan. > >> On 07.10.2021 14:30, Oleksandr wrote: >>> On 07.10.21 10:42, Jan Beulich wrote: >>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: >>>>> Changes V4 -> V5: >>>>> - update patch subject and description >>>>> - drop Michal's R-b >>>>> - pass gpaddr_bits via createdomain domctl >>>>> (struct xen_arch_domainconfig) >>>> I'm afraid I can't bring this in line with ... >>>> >>>>> --- a/xen/include/public/arch-arm.h >>>>> +++ b/xen/include/public/arch-arm.h >>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { >>>>> * >>>>> */ >>>>> uint32_t clock_frequency; >>>>> + /* >>>>> + * OUT >>>>> + * Guest physical address space size >>>>> + */ >>>>> + uint8_t gpaddr_bits; >>>> ... this being an OUT field. Is this really what Andrew had asked for? >>>> I would have expected the entire struct to be IN (and the comment at >>>> the top of the containing struct in public/domctl.h also suggests so, >>>> i.e. your new field renders that comment stale). gic_version being >>>> IN/OUT is already somewhat in conflict ... >>> I am sorry but I'm totally confused now, we want the Xen to provide >>> gpaddr_bits to the toolstack, but not the other way around. >>> As I understand the main ask was to switch to domctl for which I wanted >>> to get some clarification on how it would look like... Well, this patch >>> switches to use >>> domctl, and I think exactly as it was suggested at [1] in case if a >>> common one is a difficult to achieve. I have to admit, I felt it was >>> indeed difficult to achieve. >> Sadly the mail you reference isn't the one I was referring to. It's not >> even from Andrew. Unfortunately I also can't seem to be able to locate >> this, i.e. I'm now wondering whether this was under a different subject. >> Julien, in any event, confirmed in a recent reply on this thread that >> there was such a mail (otherwise I would have started wondering whether >> the request was made on irc). In any case it is _that_ mail that would >> need going through again. > > I think, this is the email [1] you are referring to. Well, that's still a mail you sent, not Andrew's. And while I have yours in my mailbox, I don't have Andrew's for whatever reason. Nevertheless there's enough context to be halfway certain that this wasn't meant as an extension to the create domctl, but rather a separate new one (merely replacing what you had originally as a sysctl to become per-domain, to allow returning varying [between domains] values down the road). I continue to think that if such a field was added to "create", it would be an input (only). Jan
On 07.10.21 03:49, Stefano Stabellini wrote: Hi Stefano > On Wed, 6 Oct 2021, Oleksandr Tyshchenko wrote: >> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> >> We need to pass info about maximum supported guest physical >> address space size to the toolstack on Arm in order to properly >> calculate the base and size of the extended region (safe range) >> for the guest. The extended region is unused address space which >> could be safely used by domain for foreign/grant mappings on Arm. >> The extended region itself will be handled by the subsequent >> patch. >> >> Currently the same guest physical address space size is used >> for all guests. >> >> As we add new field to the structure bump the interface version. >> >> Suggested-by: Julien Grall <jgrall@amazon.com> >> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> >> --- >> Changes RFC -> V2: >> - update patch subject/description >> - replace arch-specific sub-struct with common gpaddr_bits >> field and update code to reflect that >> >> Changes V2 -> V3: >> - make the field uint8_t and add uint8_t pad[7] after >> - remove leading blanks in libxl.h >> >> Changes V3 -> V4: >> - also print gpaddr_bits from output_physinfo() >> - add Michal's R-b >> >> Changes V4 -> V5: >> - update patch subject and description >> - drop Michal's R-b >> - pass gpaddr_bits via createdomain domctl >> (struct xen_arch_domainconfig) >> --- >> tools/include/libxl.h | 5 +++++ >> tools/libs/light/libxl_arm.c | 2 ++ >> tools/libs/light/libxl_types.idl | 1 + >> xen/arch/arm/domain.c | 6 ++++++ >> xen/include/public/arch-arm.h | 5 +++++ >> xen/include/public/domctl.h | 2 +- >> 6 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/tools/include/libxl.h b/tools/include/libxl.h >> index b9ba16d..33b4bfb 100644 >> --- a/tools/include/libxl.h >> +++ b/tools/include/libxl.h >> @@ -279,6 +279,11 @@ >> #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1 >> >> /* >> + * libxl_domain_build_info has the gpaddr_bits field. >> + */ >> +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1 >> + >> +/* >> * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing >> * 'soft reset' for domains and there is 'soft_reset' shutdown reason >> * in enum libxl_shutdown_reason. >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c >> index e3140a6..45e0386 100644 >> --- a/tools/libs/light/libxl_arm.c >> +++ b/tools/libs/light/libxl_arm.c >> @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc, >> >> state->clock_frequency = config->arch.clock_frequency; >> >> + d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits; >> + >> return 0; >> } >> >> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl >> index 3f9fff6..39482db 100644 >> --- a/tools/libs/light/libxl_types.idl >> +++ b/tools/libs/light/libxl_types.idl >> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ >> >> ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), >> ("vuart", libxl_vuart_type), >> + ("gpaddr_bits", uint8), >> ])), >> ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), >> ])), >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 19c756a..dfecc45 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d, >> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) >> goto fail; >> >> + /* >> + * Pass maximum IPA bits to the toolstack, currently the same guest >> + * physical address space size is used for all guests. >> + */ >> + config->arch.gpaddr_bits = p2m_ipa_bits; > This could also be set in arch_sanitise_domain_config together with > config->arch.gic_version. I prefer if it was done in > arch_sanitise_domain_config but also here is OK I think. I don't mind, being honest I had an idea to place this in arch_sanitise_domain_config(), but couldn't convince myself. > > Given that everything else looks fine: > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> Thanks! Sadly, according to the recent discussion most likely this version is also no-go. [snip]
On Thu, 7 Oct 2021, Jan Beulich wrote: > On 07.10.2021 15:12, Oleksandr wrote: > > > > On 07.10.21 15:43, Jan Beulich wrote: > > > > Hi Jan. > > > >> On 07.10.2021 14:30, Oleksandr wrote: > >>> On 07.10.21 10:42, Jan Beulich wrote: > >>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: > >>>>> Changes V4 -> V5: > >>>>> - update patch subject and description > >>>>> - drop Michal's R-b > >>>>> - pass gpaddr_bits via createdomain domctl > >>>>> (struct xen_arch_domainconfig) > >>>> I'm afraid I can't bring this in line with ... > >>>> > >>>>> --- a/xen/include/public/arch-arm.h > >>>>> +++ b/xen/include/public/arch-arm.h > >>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { > >>>>> * > >>>>> */ > >>>>> uint32_t clock_frequency; > >>>>> + /* > >>>>> + * OUT > >>>>> + * Guest physical address space size > >>>>> + */ > >>>>> + uint8_t gpaddr_bits; > >>>> ... this being an OUT field. Is this really what Andrew had asked for? > >>>> I would have expected the entire struct to be IN (and the comment at > >>>> the top of the containing struct in public/domctl.h also suggests so, > >>>> i.e. your new field renders that comment stale). gic_version being > >>>> IN/OUT is already somewhat in conflict ... > >>> I am sorry but I'm totally confused now, we want the Xen to provide > >>> gpaddr_bits to the toolstack, but not the other way around. > >>> As I understand the main ask was to switch to domctl for which I wanted > >>> to get some clarification on how it would look like... Well, this patch > >>> switches to use > >>> domctl, and I think exactly as it was suggested at [1] in case if a > >>> common one is a difficult to achieve. I have to admit, I felt it was > >>> indeed difficult to achieve. > >> Sadly the mail you reference isn't the one I was referring to. It's not > >> even from Andrew. Unfortunately I also can't seem to be able to locate > >> this, i.e. I'm now wondering whether this was under a different subject. > >> Julien, in any event, confirmed in a recent reply on this thread that > >> there was such a mail (otherwise I would have started wondering whether > >> the request was made on irc). In any case it is _that_ mail that would > >> need going through again. > > > > I think, this is the email [1] you are referring to. > > Well, that's still a mail you sent, not Andrew's. And while I have yours > in my mailbox, I don't have Andrew's for whatever reason. > > Nevertheless there's enough context to be halfway certain that this > wasn't meant as an extension to the create domctl, but rather a separate > new one (merely replacing what you had originally as a sysctl to become > per-domain, to allow returning varying [between domains] values down the > road). I continue to think that if such a field was added to "create", > it would be an input (only). During the Xen Community Call on Tuesday, we briefly spoke about this. Andrew confirmed that what he meant with his original email is to use a domctl. We didn't discuss which domctl specifically. This patch now follows the same pattern of clock_frequency and gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig). Note that gic_version is an IN/OUT parameter, showing that if in the future we want the ability to set gpaddr_bits (in addition to get gpaddr_bits), it will be possible. Also it is good to keep in mind that although nobody likes to change hypercall interfaces, especially for minor reasons, domctl are not stable so we can be a little bit more relaxed compared to something like grant_table_op.
On 07.10.2021 22:23, Stefano Stabellini wrote: > On Thu, 7 Oct 2021, Jan Beulich wrote: >> On 07.10.2021 15:12, Oleksandr wrote: >>> >>> On 07.10.21 15:43, Jan Beulich wrote: >>> >>> Hi Jan. >>> >>>> On 07.10.2021 14:30, Oleksandr wrote: >>>>> On 07.10.21 10:42, Jan Beulich wrote: >>>>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: >>>>>>> Changes V4 -> V5: >>>>>>> - update patch subject and description >>>>>>> - drop Michal's R-b >>>>>>> - pass gpaddr_bits via createdomain domctl >>>>>>> (struct xen_arch_domainconfig) >>>>>> I'm afraid I can't bring this in line with ... >>>>>> >>>>>>> --- a/xen/include/public/arch-arm.h >>>>>>> +++ b/xen/include/public/arch-arm.h >>>>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { >>>>>>> * >>>>>>> */ >>>>>>> uint32_t clock_frequency; >>>>>>> + /* >>>>>>> + * OUT >>>>>>> + * Guest physical address space size >>>>>>> + */ >>>>>>> + uint8_t gpaddr_bits; >>>>>> ... this being an OUT field. Is this really what Andrew had asked for? >>>>>> I would have expected the entire struct to be IN (and the comment at >>>>>> the top of the containing struct in public/domctl.h also suggests so, >>>>>> i.e. your new field renders that comment stale). gic_version being >>>>>> IN/OUT is already somewhat in conflict ... >>>>> I am sorry but I'm totally confused now, we want the Xen to provide >>>>> gpaddr_bits to the toolstack, but not the other way around. >>>>> As I understand the main ask was to switch to domctl for which I wanted >>>>> to get some clarification on how it would look like... Well, this patch >>>>> switches to use >>>>> domctl, and I think exactly as it was suggested at [1] in case if a >>>>> common one is a difficult to achieve. I have to admit, I felt it was >>>>> indeed difficult to achieve. >>>> Sadly the mail you reference isn't the one I was referring to. It's not >>>> even from Andrew. Unfortunately I also can't seem to be able to locate >>>> this, i.e. I'm now wondering whether this was under a different subject. >>>> Julien, in any event, confirmed in a recent reply on this thread that >>>> there was such a mail (otherwise I would have started wondering whether >>>> the request was made on irc). In any case it is _that_ mail that would >>>> need going through again. >>> >>> I think, this is the email [1] you are referring to. >> >> Well, that's still a mail you sent, not Andrew's. And while I have yours >> in my mailbox, I don't have Andrew's for whatever reason. >> >> Nevertheless there's enough context to be halfway certain that this >> wasn't meant as an extension to the create domctl, but rather a separate >> new one (merely replacing what you had originally as a sysctl to become >> per-domain, to allow returning varying [between domains] values down the >> road). I continue to think that if such a field was added to "create", >> it would be an input (only). > > During the Xen Community Call on Tuesday, we briefly spoke about this. > Andrew confirmed that what he meant with his original email is to use a > domctl. We didn't discuss which domctl specifically. > > This patch now follows the same pattern of clock_frequency and > gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig). > Note that gic_version is an IN/OUT parameter, showing that if in the > future we want the ability to set gpaddr_bits (in addition to get > gpaddr_bits), it will be possible. Well, as said before - I'm not convinced gic_version being IN/OUT is appropriate. At the very least a 2nd way to merely retrieve the value would seem to be necessary, so that it's not only the party creating the guest which would be able to know. Since here's we're solely after retrieving the value, I don't see the point in altering "create". As you say, domctl can be changed, and hence at the point this needs to become an input to "create", it could easily be added there. Jan
On 08.10.21 11:13, Jan Beulich wrote: Hi Jan > On 07.10.2021 22:23, Stefano Stabellini wrote: >> On Thu, 7 Oct 2021, Jan Beulich wrote: >>> On 07.10.2021 15:12, Oleksandr wrote: >>>> On 07.10.21 15:43, Jan Beulich wrote: >>>> >>>> Hi Jan. >>>> >>>>> On 07.10.2021 14:30, Oleksandr wrote: >>>>>> On 07.10.21 10:42, Jan Beulich wrote: >>>>>>> On 06.10.2021 13:22, Oleksandr Tyshchenko wrote: >>>>>>>> Changes V4 -> V5: >>>>>>>> - update patch subject and description >>>>>>>> - drop Michal's R-b >>>>>>>> - pass gpaddr_bits via createdomain domctl >>>>>>>> (struct xen_arch_domainconfig) >>>>>>> I'm afraid I can't bring this in line with ... >>>>>>> >>>>>>>> --- a/xen/include/public/arch-arm.h >>>>>>>> +++ b/xen/include/public/arch-arm.h >>>>>>>> @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { >>>>>>>> * >>>>>>>> */ >>>>>>>> uint32_t clock_frequency; >>>>>>>> + /* >>>>>>>> + * OUT >>>>>>>> + * Guest physical address space size >>>>>>>> + */ >>>>>>>> + uint8_t gpaddr_bits; >>>>>>> ... this being an OUT field. Is this really what Andrew had asked for? >>>>>>> I would have expected the entire struct to be IN (and the comment at >>>>>>> the top of the containing struct in public/domctl.h also suggests so, >>>>>>> i.e. your new field renders that comment stale). gic_version being >>>>>>> IN/OUT is already somewhat in conflict ... >>>>>> I am sorry but I'm totally confused now, we want the Xen to provide >>>>>> gpaddr_bits to the toolstack, but not the other way around. >>>>>> As I understand the main ask was to switch to domctl for which I wanted >>>>>> to get some clarification on how it would look like... Well, this patch >>>>>> switches to use >>>>>> domctl, and I think exactly as it was suggested at [1] in case if a >>>>>> common one is a difficult to achieve. I have to admit, I felt it was >>>>>> indeed difficult to achieve. >>>>> Sadly the mail you reference isn't the one I was referring to. It's not >>>>> even from Andrew. Unfortunately I also can't seem to be able to locate >>>>> this, i.e. I'm now wondering whether this was under a different subject. >>>>> Julien, in any event, confirmed in a recent reply on this thread that >>>>> there was such a mail (otherwise I would have started wondering whether >>>>> the request was made on irc). In any case it is _that_ mail that would >>>>> need going through again. >>>> I think, this is the email [1] you are referring to. >>> Well, that's still a mail you sent, not Andrew's. And while I have yours >>> in my mailbox, I don't have Andrew's for whatever reason. >>> >>> Nevertheless there's enough context to be halfway certain that this >>> wasn't meant as an extension to the create domctl, but rather a separate >>> new one (merely replacing what you had originally as a sysctl to become >>> per-domain, to allow returning varying [between domains] values down the >>> road). I continue to think that if such a field was added to "create", >>> it would be an input (only). >> During the Xen Community Call on Tuesday, we briefly spoke about this. >> Andrew confirmed that what he meant with his original email is to use a >> domctl. We didn't discuss which domctl specifically. >> >> This patch now follows the same pattern of clock_frequency and >> gic_version (see xen/include/public/arch-arm.h:struct xen_arch_domainconfig). >> Note that gic_version is an IN/OUT parameter, showing that if in the >> future we want the ability to set gpaddr_bits (in addition to get >> gpaddr_bits), it will be possible. > Well, as said before - I'm not convinced gic_version being IN/OUT is > appropriate. At the very least a 2nd way to merely retrieve the value > would seem to be necessary, so that it's not only the party creating > the guest which would be able to know. > > Since here's we're solely after retrieving the value, I don't see > the point in altering "create". As you say, domctl can be changed, > and hence at the point this needs to become an input to "create", it > could easily be added there. > > Jan Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be reused to retrieve gpaddr_bits? I don't see why not at the moment, but maybe there are some implications/concerns which are invisible to me. I see that arch_get_domain_info() is present, so the field will be common, and each arch will write a value it considers appropriate. This could be a good compromise to not add an extra domctl and to not alter domain_create.
On 08.10.2021 12:25, Oleksandr wrote: > Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be > reused to retrieve gpaddr_bits? I don't see why not at the moment, but > maybe there are some implications/concerns which are invisible to me. > > I see that arch_get_domain_info() is present, so the field will be > common, and each arch will write a value it considers > appropriate. This could be a good compromise to not add an extra domctl > and to not alter domain_create. Technically I think it could be reused. What I'm less certain of is whether the specific piece of information is a good fit there. Jan
On 08.10.21 15:36, Jan Beulich wrote: Hi Jan > On 08.10.2021 12:25, Oleksandr wrote: >> Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be >> reused to retrieve gpaddr_bits? I don't see why not at the moment, but >> maybe there are some implications/concerns which are invisible to me. >> >> I see that arch_get_domain_info() is present, so the field will be >> common, and each arch will write a value it considers >> appropriate. This could be a good compromise to not add an extra domctl >> and to not alter domain_create. > Technically I think it could be reused. What I'm less certain of is > whether the specific piece of information is a good fit there. ok, thank you for your answer. I am also not 100% sure whether it is a *good* fit there, but I cannot say it is not fit at all for being there. I might mistake, but it is almost the same piece of information describing the whole domain as other existing fields in that structure. > > Jan >
On Fri, 8 Oct 2021, Oleksandr wrote: > On 08.10.21 15:36, Jan Beulich wrote: > > On 08.10.2021 12:25, Oleksandr wrote: > > > Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be > > > reused to retrieve gpaddr_bits? I don't see why not at the moment, but > > > maybe there are some implications/concerns which are invisible to me. > > > > > > I see that arch_get_domain_info() is present, so the field will be > > > common, and each arch will write a value it considers > > > appropriate. This could be a good compromise to not add an extra domctl > > > and to not alter domain_create. > > Technically I think it could be reused. What I'm less certain of is > > whether the specific piece of information is a good fit there. > > ok, thank you for your answer. > > I am also not 100% sure whether it is a *good* fit there, but I cannot say it > is not fit at all for being there. I might mistake, but it is almost the same > piece of information describing the whole domain as other existing fields in > that structure. From a domctl point of view, it looks like XEN_DOMCTL_getdomaininfo could be a decent fit. Looking at the data structure, the arch specific member of struct xen_domctl_getdomaininfo is: struct xen_arch_domainconfig arch_config; which is actually the very same struct used in struct xen_domctl_createdomain for XEN_DOMCTL_createdomain, but somehow it doesn't get populated by neither the x86 nor the ARM version of arch_get_domain_info? In any case, I think we could make use of XEN_DOMCTL_getdomaininfo for this. In that case, I would add a new common field to struct xen_domctl_getdomaininfo after cpupool and above arch_config. Then we can set the field from arch_get_domain_info.
On 09.10.21 01:14, Stefano Stabellini wrote: Hi Stefano > On Fri, 8 Oct 2021, Oleksandr wrote: >> On 08.10.21 15:36, Jan Beulich wrote: >>> On 08.10.2021 12:25, Oleksandr wrote: >>>> Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be >>>> reused to retrieve gpaddr_bits? I don't see why not at the moment, but >>>> maybe there are some implications/concerns which are invisible to me. >>>> >>>> I see that arch_get_domain_info() is present, so the field will be >>>> common, and each arch will write a value it considers >>>> appropriate. This could be a good compromise to not add an extra domctl >>>> and to not alter domain_create. >>> Technically I think it could be reused. What I'm less certain of is >>> whether the specific piece of information is a good fit there. >> ok, thank you for your answer. >> >> I am also not 100% sure whether it is a *good* fit there, but I cannot say it >> is not fit at all for being there. I might mistake, but it is almost the same >> piece of information describing the whole domain as other existing fields in >> that structure. > From a domctl point of view, it looks like XEN_DOMCTL_getdomaininfo > could be a decent fit. Looking at the data structure, the arch specific > member of struct xen_domctl_getdomaininfo is: > > struct xen_arch_domainconfig arch_config; > > which is actually the very same struct used in struct > xen_domctl_createdomain for XEN_DOMCTL_createdomain, but somehow it > doesn't get populated by neither the x86 nor the ARM version of > arch_get_domain_info? > > > In any case, I think we could make use of XEN_DOMCTL_getdomaininfo for > this. In that case, I would add a new common field to struct > xen_domctl_getdomaininfo after cpupool and above arch_config. > > Then we can set the field from arch_get_domain_info. Yes, this is what I had in mind, thank you.
diff --git a/tools/include/libxl.h b/tools/include/libxl.h index b9ba16d..33b4bfb 100644 --- a/tools/include/libxl.h +++ b/tools/include/libxl.h @@ -279,6 +279,11 @@ #define LIBXL_HAVE_BUILDINFO_ARCH_ARM_TEE 1 /* + * libxl_domain_build_info has the gpaddr_bits field. + */ +#define LIBXL_HAVE_BUILDINFO_ARCH_ARM_GPADDR_BITS 1 + +/* * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing * 'soft reset' for domains and there is 'soft_reset' shutdown reason * in enum libxl_shutdown_reason. diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c index e3140a6..45e0386 100644 --- a/tools/libs/light/libxl_arm.c +++ b/tools/libs/light/libxl_arm.c @@ -123,6 +123,8 @@ int libxl__arch_domain_save_config(libxl__gc *gc, state->clock_frequency = config->arch.clock_frequency; + d_config->b_info.arch_arm.gpaddr_bits = config->arch.gpaddr_bits; + return 0; } diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl index 3f9fff6..39482db 100644 --- a/tools/libs/light/libxl_types.idl +++ b/tools/libs/light/libxl_types.idl @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[ ("arch_arm", Struct(None, [("gic_version", libxl_gic_version), ("vuart", libxl_vuart_type), + ("gpaddr_bits", uint8), ])), ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool), ])), diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index 19c756a..dfecc45 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -767,6 +767,12 @@ int arch_domain_create(struct domain *d, if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) ) goto fail; + /* + * Pass maximum IPA bits to the toolstack, currently the same guest + * physical address space size is used for all guests. + */ + config->arch.gpaddr_bits = p2m_ipa_bits; + return 0; fail: diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 6b5a5f8..4a01f8b 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -333,6 +333,11 @@ struct xen_arch_domainconfig { * */ uint32_t clock_frequency; + /* + * OUT + * Guest physical address space size + */ + uint8_t gpaddr_bits; }; #endif /* __XEN__ || __XEN_TOOLS__ */ diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 96696e3..f37586e 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -38,7 +38,7 @@ #include "hvm/save.h" #include "memory.h" -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000014 +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000015 /* * NB. xen_domctl.domain is an IN/OUT parameter for this operation.