| Message ID | 20240426031455.579637-3-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Guest magic region allocation for 11 Dom0less domUs - Take two | expand |
On 26.04.2024 05:14, Henry Wang wrote: > --- a/xen/include/public/hvm/params.h > +++ b/xen/include/public/hvm/params.h > @@ -76,6 +76,7 @@ > */ > #define HVM_PARAM_STORE_PFN 1 > #define HVM_PARAM_STORE_EVTCHN 2 > +#define HVM_PARAM_MAGIC_BASE_PFN 3 > > #define HVM_PARAM_IOREQ_PFN 5 Considering all adjacent values are used, it is overwhelmingly likely that 3 was once used, too. Such re-use needs to be done carefully. Since you need this for Arm only, that's likely okay, but doesn't go without (a) saying and (b) considering the possible future case of dom0less becoming arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo this also needs at least a comment, maybe even an #ifdef, seeing how x86- focused most of the rest of this header is. Jan
Hi Jan, On 4/26/2024 2:21 PM, Jan Beulich wrote: > On 26.04.2024 05:14, Henry Wang wrote: >> --- a/xen/include/public/hvm/params.h >> +++ b/xen/include/public/hvm/params.h >> @@ -76,6 +76,7 @@ >> */ >> #define HVM_PARAM_STORE_PFN 1 >> #define HVM_PARAM_STORE_EVTCHN 2 >> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >> >> #define HVM_PARAM_IOREQ_PFN 5 > Considering all adjacent values are used, it is overwhelmingly likely that > 3 was once used, too. Such re-use needs to be done carefully. Since you > need this for Arm only, that's likely okay, but doesn't go without (a) > saying and (b) considering the possible future case of dom0less becoming > arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo > this also needs at least a comment, maybe even an #ifdef, seeing how x86- > focused most of the rest of this header is. Thanks for the feedback. These make sense. I think probably dom0less/hyperlaunch will have similar use cases so the number 3 can be reused at that time. Therefore, in v2, I will add more description in commit message, a comment on top of this macro and protect it with #ifdef. Hope this will address your concern. Thanks. Kind regards, Henry > Jan
On 26.04.2024 08:30, Henry Wang wrote: > On 4/26/2024 2:21 PM, Jan Beulich wrote: >> On 26.04.2024 05:14, Henry Wang wrote: >>> --- a/xen/include/public/hvm/params.h >>> +++ b/xen/include/public/hvm/params.h >>> @@ -76,6 +76,7 @@ >>> */ >>> #define HVM_PARAM_STORE_PFN 1 >>> #define HVM_PARAM_STORE_EVTCHN 2 >>> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >>> >>> #define HVM_PARAM_IOREQ_PFN 5 >> Considering all adjacent values are used, it is overwhelmingly likely that >> 3 was once used, too. Such re-use needs to be done carefully. Since you >> need this for Arm only, that's likely okay, but doesn't go without (a) >> saying and (b) considering the possible future case of dom0less becoming >> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo >> this also needs at least a comment, maybe even an #ifdef, seeing how x86- >> focused most of the rest of this header is. > > Thanks for the feedback. These make sense. I think probably > dom0less/hyperlaunch will have similar use cases so the number 3 can be > reused at that time. Therefore, in v2, I will add more description in > commit message, a comment on top of this macro and protect it with > #ifdef. Hope this will address your concern. Thanks. FTAOD: If you foresee re-use by hyperlaunch, re-using a previously used number may need re-considering. Which isn't to say that number re-use is excluded here, but it would need at least figuring out (and then stating) what exactly the number was used for and until when. Jan
Hi Jan, On 4/26/2024 2:50 PM, Jan Beulich wrote: > On 26.04.2024 08:30, Henry Wang wrote: >> On 4/26/2024 2:21 PM, Jan Beulich wrote: >>> On 26.04.2024 05:14, Henry Wang wrote: >>>> --- a/xen/include/public/hvm/params.h >>>> +++ b/xen/include/public/hvm/params.h >>>> @@ -76,6 +76,7 @@ >>>> */ >>>> #define HVM_PARAM_STORE_PFN 1 >>>> #define HVM_PARAM_STORE_EVTCHN 2 >>>> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >>>> >>>> #define HVM_PARAM_IOREQ_PFN 5 >>> Considering all adjacent values are used, it is overwhelmingly likely that >>> 3 was once used, too. Such re-use needs to be done carefully. Since you >>> need this for Arm only, that's likely okay, but doesn't go without (a) >>> saying and (b) considering the possible future case of dom0less becoming >>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo >>> this also needs at least a comment, maybe even an #ifdef, seeing how x86- >>> focused most of the rest of this header is. >> Thanks for the feedback. These make sense. I think probably >> dom0less/hyperlaunch will have similar use cases so the number 3 can be >> reused at that time. Therefore, in v2, I will add more description in >> commit message, a comment on top of this macro and protect it with >> #ifdef. Hope this will address your concern. Thanks. > FTAOD: If you foresee re-use by hyperlaunch, re-using a previously used > number may need re-considering. Which isn't to say that number re-use is > excluded here, but it would need at least figuring out (and then stating) > what exactly the number was used for and until when. I just did a bit search and noticed that the number 3 was used to be #define HVM_PARAM_APIC_ENABLED 3 and it was removed 18 years ago in commit: 6bc01e4efd50e1986a9391f75980d45691f42b74 So I think we are likely to be ok if reuse 3 on Arm with proper #ifdef. Kind regards, Henry > Jan
On 4/26/24 02:21, Jan Beulich wrote: > On 26.04.2024 05:14, Henry Wang wrote: >> --- a/xen/include/public/hvm/params.h >> +++ b/xen/include/public/hvm/params.h >> @@ -76,6 +76,7 @@ >> */ >> #define HVM_PARAM_STORE_PFN 1 >> #define HVM_PARAM_STORE_EVTCHN 2 >> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >> >> #define HVM_PARAM_IOREQ_PFN 5 > > Considering all adjacent values are used, it is overwhelmingly likely that > 3 was once used, too. Such re-use needs to be done carefully. Since you > need this for Arm only, that's likely okay, but doesn't go without (a) > saying and (b) considering the possible future case of dom0less becoming > arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo > this also needs at least a comment, maybe even an #ifdef, seeing how x86- > focused most of the rest of this header is. I would recommend having two new params, #define HVM_PARAM_HV_RSRV_BASE_PVH 3 #define HVM_PARAM_HV_RSRV_SIZE 4 This will communicate how many pages have been reserved and where those pages are located. v/r, dps
On 4/25/24 23:14, Henry Wang wrote: > For use cases such as Dom0less PV drivers, a mechanism to communicate > Dom0less DomU's static data with the runtime control plane (Dom0) is > needed. Since on Arm HVMOP is already the existing approach to address > such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ), > add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic > page region base PFN. The value will be set at Dom0less DomU > construction time after Dom0less DomU's magic page region has been > allocated. > > To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN > for libxl guests in alloc_magic_pages(). > > Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > tools/libs/guest/xg_dom_arm.c | 2 ++ > xen/arch/arm/dom0less-build.c | 2 ++ > xen/arch/arm/hvm.c | 1 + > xen/include/public/hvm/params.h | 1 + > 4 files changed, 6 insertions(+) > > diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c > index 8cc7f27dbb..3c08782d1d 100644 > --- a/tools/libs/guest/xg_dom_arm.c > +++ b/tools/libs/guest/xg_dom_arm.c > @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom) > xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET); > xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn); > > + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN, > + base); > xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN, > dom->console_pfn); > xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN, > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index 40dc85c759..72187c167d 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d, > free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES)); > return rc; > } > + > + d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn); > } > > return rc; > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 0989309fea..fa6141e30c 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain *d, unsigned int param) > case HVM_PARAM_STORE_EVTCHN: > case HVM_PARAM_CONSOLE_PFN: > case HVM_PARAM_CONSOLE_EVTCHN: > + case HVM_PARAM_MAGIC_BASE_PFN: > return 0; I know you are just adding, so more of a question for the Arm maintainers: Why does Arm have a pair of private access control functions subverting XSM? v/r, dps
On 4/29/24 20:35, Daniel P. Smith wrote: > On 4/25/24 23:14, Henry Wang wrote: >> For use cases such as Dom0less PV drivers, a mechanism to communicate >> Dom0less DomU's static data with the runtime control plane (Dom0) is >> needed. Since on Arm HVMOP is already the existing approach to address >> such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ), >> add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic >> page region base PFN. The value will be set at Dom0less DomU >> construction time after Dom0less DomU's magic page region has been >> allocated. >> >> To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN >> for libxl guests in alloc_magic_pages(). >> >> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> >> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> --- >> tools/libs/guest/xg_dom_arm.c | 2 ++ >> xen/arch/arm/dom0less-build.c | 2 ++ >> xen/arch/arm/hvm.c | 1 + >> xen/include/public/hvm/params.h | 1 + >> 4 files changed, 6 insertions(+) >> >> diff --git a/tools/libs/guest/xg_dom_arm.c >> b/tools/libs/guest/xg_dom_arm.c >> index 8cc7f27dbb..3c08782d1d 100644 >> --- a/tools/libs/guest/xg_dom_arm.c >> +++ b/tools/libs/guest/xg_dom_arm.c >> @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom) >> xc_clear_domain_page(dom->xch, dom->guest_domid, base + >> MEMACCESS_PFN_OFFSET); >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn); >> + xc_hvm_param_set(dom->xch, dom->guest_domid, >> HVM_PARAM_MAGIC_BASE_PFN, >> + base); >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN, >> dom->console_pfn); >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN, >> diff --git a/xen/arch/arm/dom0less-build.c >> b/xen/arch/arm/dom0less-build.c >> index 40dc85c759..72187c167d 100644 >> --- a/xen/arch/arm/dom0less-build.c >> +++ b/xen/arch/arm/dom0less-build.c >> @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d, >> free_domheap_pages(magic_pg, >> get_order_from_pages(NR_MAGIC_PAGES)); >> return rc; >> } >> + >> + d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn); >> } >> return rc; >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index 0989309fea..fa6141e30c 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain >> *d, unsigned int param) >> case HVM_PARAM_STORE_EVTCHN: >> case HVM_PARAM_CONSOLE_PFN: >> case HVM_PARAM_CONSOLE_EVTCHN: >> + case HVM_PARAM_MAGIC_BASE_PFN: >> return 0; > > I know you are just adding, so more of a question for the Arm maintainers: > > Why does Arm have a pair of private access control functions subverting > XSM? On closer look, I see x86 has done the same. While the way this is set up bothers me, reviewing the history it was clearly decided that only controlling use of the op by a src domain against a target domain was sufficient. Ultimately, it is seeing a mini access control policy being codified in the access code is what is bothering me here. Fixing this would require a complete rework of xsm_hvm_param, and while it would correct the access control from a purist perspective, the security benefit would be very low. v/r, dps
Hi Daniel, On 4/30/2024 8:31 AM, Daniel P. Smith wrote: > > On 4/26/24 02:21, Jan Beulich wrote: >> On 26.04.2024 05:14, Henry Wang wrote: >>> --- a/xen/include/public/hvm/params.h >>> +++ b/xen/include/public/hvm/params.h >>> @@ -76,6 +76,7 @@ >>> */ >>> #define HVM_PARAM_STORE_PFN 1 >>> #define HVM_PARAM_STORE_EVTCHN 2 >>> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >>> #define HVM_PARAM_IOREQ_PFN 5 >> >> Considering all adjacent values are used, it is overwhelmingly likely >> that >> 3 was once used, too. Such re-use needs to be done carefully. Since you >> need this for Arm only, that's likely okay, but doesn't go without (a) >> saying and (b) considering the possible future case of dom0less becoming >> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo >> this also needs at least a comment, maybe even an #ifdef, seeing how >> x86- >> focused most of the rest of this header is. > > I would recommend having two new params, Sounds good. I can do the suggestion in v2. > > #define HVM_PARAM_HV_RSRV_BASE_PVH 3 > #define HVM_PARAM_HV_RSRV_SIZE 4 I think 4 is currently in use, so I think I will find another couple of numbers in the end for both of them. Instead of reusing 3 and 4. Kind regards, Henry > > This will communicate how many pages have been reserved and where > those pages are located. > > v/r, > dps
On 30.04.2024 04:51, Henry Wang wrote: > On 4/30/2024 8:31 AM, Daniel P. Smith wrote: >> On 4/26/24 02:21, Jan Beulich wrote: >>> On 26.04.2024 05:14, Henry Wang wrote: >>>> --- a/xen/include/public/hvm/params.h >>>> +++ b/xen/include/public/hvm/params.h >>>> @@ -76,6 +76,7 @@ >>>> */ >>>> #define HVM_PARAM_STORE_PFN 1 >>>> #define HVM_PARAM_STORE_EVTCHN 2 >>>> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >>>> #define HVM_PARAM_IOREQ_PFN 5 >>> >>> Considering all adjacent values are used, it is overwhelmingly likely >>> that >>> 3 was once used, too. Such re-use needs to be done carefully. Since you >>> need this for Arm only, that's likely okay, but doesn't go without (a) >>> saying and (b) considering the possible future case of dom0less becoming >>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo >>> this also needs at least a comment, maybe even an #ifdef, seeing how >>> x86- >>> focused most of the rest of this header is. >> >> I would recommend having two new params, > > Sounds good. I can do the suggestion in v2. > >> >> #define HVM_PARAM_HV_RSRV_BASE_PVH 3 >> #define HVM_PARAM_HV_RSRV_SIZE 4 > > I think 4 is currently in use, so I think I will find another couple of > numbers in the end for both of them. Instead of reusing 3 and 4. Right. There are ample gaps, but any use of values within a gap will need appropriate care. FTAOD using such a gap looks indeed preferable, to avoid further growing the (sparse) array. Alternatively, if we're firm on this never going to be used on x86, some clearly x86-specific indexes (e.g. 36 and 37) could be given non-x86 purpose. Jan
Hi Jan, On 4/30/2024 2:11 PM, Jan Beulich wrote: > On 30.04.2024 04:51, Henry Wang wrote: >> On 4/30/2024 8:31 AM, Daniel P. Smith wrote: >>> On 4/26/24 02:21, Jan Beulich wrote: >>>> On 26.04.2024 05:14, Henry Wang wrote: >>>>> --- a/xen/include/public/hvm/params.h >>>>> +++ b/xen/include/public/hvm/params.h >>>>> @@ -76,6 +76,7 @@ >>>>> */ >>>>> #define HVM_PARAM_STORE_PFN 1 >>>>> #define HVM_PARAM_STORE_EVTCHN 2 >>>>> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >>>>> #define HVM_PARAM_IOREQ_PFN 5 >>>> Considering all adjacent values are used, it is overwhelmingly likely >>>> that >>>> 3 was once used, too. Such re-use needs to be done carefully. Since you >>>> need this for Arm only, that's likely okay, but doesn't go without (a) >>>> saying and (b) considering the possible future case of dom0less becoming >>>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo >>>> this also needs at least a comment, maybe even an #ifdef, seeing how >>>> x86- >>>> focused most of the rest of this header is. >>> I would recommend having two new params, >> Sounds good. I can do the suggestion in v2. >> >>> #define HVM_PARAM_HV_RSRV_BASE_PVH 3 >>> #define HVM_PARAM_HV_RSRV_SIZE 4 >> I think 4 is currently in use, so I think I will find another couple of >> numbers in the end for both of them. Instead of reusing 3 and 4. > Right. There are ample gaps, but any use of values within a gap will need > appropriate care. FTAOD using such a gap looks indeed preferable, to avoid > further growing the (sparse) array. Alternatively, if we're firm on this > never going to be used on x86, some clearly x86-specific indexes (e.g. 36 > and 37) could be given non-x86 purpose. Sorry, I am a bit confused. I take Daniel's comment as to add two new params, which is currently only used for Arm, but eventually will be used for hyperlaunch on x86 (as the name indicated). So I think I will use the name that he suggested, but the number changed to 39 and 40. Kind regards, Henry > > Jan
On 30.04.2024 10:12, Henry Wang wrote: > Hi Jan, > > On 4/30/2024 2:11 PM, Jan Beulich wrote: >> On 30.04.2024 04:51, Henry Wang wrote: >>> On 4/30/2024 8:31 AM, Daniel P. Smith wrote: >>>> On 4/26/24 02:21, Jan Beulich wrote: >>>>> On 26.04.2024 05:14, Henry Wang wrote: >>>>>> --- a/xen/include/public/hvm/params.h >>>>>> +++ b/xen/include/public/hvm/params.h >>>>>> @@ -76,6 +76,7 @@ >>>>>> */ >>>>>> #define HVM_PARAM_STORE_PFN 1 >>>>>> #define HVM_PARAM_STORE_EVTCHN 2 >>>>>> +#define HVM_PARAM_MAGIC_BASE_PFN 3 >>>>>> #define HVM_PARAM_IOREQ_PFN 5 >>>>> Considering all adjacent values are used, it is overwhelmingly likely >>>>> that >>>>> 3 was once used, too. Such re-use needs to be done carefully. Since you >>>>> need this for Arm only, that's likely okay, but doesn't go without (a) >>>>> saying and (b) considering the possible future case of dom0less becoming >>>>> arch-agnostic, or hyperlaunch wanting to extend the scope. Plus (c) imo >>>>> this also needs at least a comment, maybe even an #ifdef, seeing how >>>>> x86- >>>>> focused most of the rest of this header is. >>>> I would recommend having two new params, >>> Sounds good. I can do the suggestion in v2. >>> >>>> #define HVM_PARAM_HV_RSRV_BASE_PVH 3 >>>> #define HVM_PARAM_HV_RSRV_SIZE 4 >>> I think 4 is currently in use, so I think I will find another couple of >>> numbers in the end for both of them. Instead of reusing 3 and 4. >> Right. There are ample gaps, but any use of values within a gap will need >> appropriate care. FTAOD using such a gap looks indeed preferable, to avoid >> further growing the (sparse) array. Alternatively, if we're firm on this >> never going to be used on x86, some clearly x86-specific indexes (e.g. 36 >> and 37) could be given non-x86 purpose. > > Sorry, I am a bit confused. I take Daniel's comment as to add two new > params, which is currently only used for Arm, but eventually will be > used for hyperlaunch on x86 (as the name indicated). So I think I will > use the name that he suggested, but the number changed to 39 and 40. Well, yes, if re-use for x86 is intended, then unused slots need taking. Question then still is whether the array bounds indeed need moving up, or whether instead one of the gaps can be (re)used. Jan
On Fri, 26 Apr 2024, Henry Wang wrote: > For use cases such as Dom0less PV drivers, a mechanism to communicate > Dom0less DomU's static data with the runtime control plane (Dom0) is > needed. Since on Arm HVMOP is already the existing approach to address > such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ), > add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic > page region base PFN. The value will be set at Dom0less DomU > construction time after Dom0less DomU's magic page region has been > allocated. > > To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN > for libxl guests in alloc_magic_pages(). > > Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> > Signed-off-by: Henry Wang <xin.wang2@amd.com> > --- > tools/libs/guest/xg_dom_arm.c | 2 ++ > xen/arch/arm/dom0less-build.c | 2 ++ > xen/arch/arm/hvm.c | 1 + > xen/include/public/hvm/params.h | 1 + > 4 files changed, 6 insertions(+) > > diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c > index 8cc7f27dbb..3c08782d1d 100644 > --- a/tools/libs/guest/xg_dom_arm.c > +++ b/tools/libs/guest/xg_dom_arm.c > @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom) > xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET); > xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn); > > + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN, > + base); > xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN, > dom->console_pfn); > xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN, > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c > index 40dc85c759..72187c167d 100644 > --- a/xen/arch/arm/dom0less-build.c > +++ b/xen/arch/arm/dom0less-build.c > @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d, > free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES)); > return rc; > } > + > + d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn); I apologize as I have not read the whole email thread in reply to this patch. Why do we need to introduce a new hvm param instead of just setting HVM_PARAM_CONSOLE_PFN and HVM_PARAM_STORE_PFN directly here?
Hi Stefano, On 5/3/2024 2:08 AM, Stefano Stabellini wrote: > On Fri, 26 Apr 2024, Henry Wang wrote: >> For use cases such as Dom0less PV drivers, a mechanism to communicate >> Dom0less DomU's static data with the runtime control plane (Dom0) is >> needed. Since on Arm HVMOP is already the existing approach to address >> such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ), >> add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic >> page region base PFN. The value will be set at Dom0less DomU >> construction time after Dom0less DomU's magic page region has been >> allocated. >> >> To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN >> for libxl guests in alloc_magic_pages(). >> >> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> >> Signed-off-by: Henry Wang <xin.wang2@amd.com> >> --- >> tools/libs/guest/xg_dom_arm.c | 2 ++ >> xen/arch/arm/dom0less-build.c | 2 ++ >> xen/arch/arm/hvm.c | 1 + >> xen/include/public/hvm/params.h | 1 + >> 4 files changed, 6 insertions(+) >> >> diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c >> index 8cc7f27dbb..3c08782d1d 100644 >> --- a/tools/libs/guest/xg_dom_arm.c >> +++ b/tools/libs/guest/xg_dom_arm.c >> @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom) >> xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET); >> xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn); >> >> + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN, >> + base); >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN, >> dom->console_pfn); >> xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN, >> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c >> index 40dc85c759..72187c167d 100644 >> --- a/xen/arch/arm/dom0less-build.c >> +++ b/xen/arch/arm/dom0less-build.c >> @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d, >> free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES)); >> return rc; >> } >> + >> + d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn); > I apologize as I have not read the whole email thread in reply to this > patch. > > Why do we need to introduce a new hvm param instead of just setting > HVM_PARAM_CONSOLE_PFN and HVM_PARAM_STORE_PFN directly here? > Yeah this is a good question, I aIso thought about this but in the end didn't do that directly because I don't really want to break the current protocol between Linux, Xen and toolstack. In docs/features/dom0less.pandoc, section "PV Drivers", there is a communication protocol saying that Xen should keep the HVM_PARAM_STORE_PFN to ~0ULL until the toolstack sets the HVM_PARAM_STORE_PFN. I am open to change the protocol (changes might be needed in the Linux side too), if it is ok to do that, I can set the HVM params here directly and change the doc accordingly. Kind regards, Henry
diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c index 8cc7f27dbb..3c08782d1d 100644 --- a/tools/libs/guest/xg_dom_arm.c +++ b/tools/libs/guest/xg_dom_arm.c @@ -74,6 +74,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom) xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET); xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn); + xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MAGIC_BASE_PFN, + base); xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN, dom->console_pfn); xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN, diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 40dc85c759..72187c167d 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -861,6 +861,8 @@ static int __init construct_domU(struct domain *d, free_domheap_pages(magic_pg, get_order_from_pages(NR_MAGIC_PAGES)); return rc; } + + d->arch.hvm.params[HVM_PARAM_MAGIC_BASE_PFN] = gfn_x(gfn); } return rc; diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 0989309fea..fa6141e30c 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -55,6 +55,7 @@ static int hvm_allow_get_param(const struct domain *d, unsigned int param) case HVM_PARAM_STORE_EVTCHN: case HVM_PARAM_CONSOLE_PFN: case HVM_PARAM_CONSOLE_EVTCHN: + case HVM_PARAM_MAGIC_BASE_PFN: return 0; /* diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h index a22b4ed45d..c1720b33b9 100644 --- a/xen/include/public/hvm/params.h +++ b/xen/include/public/hvm/params.h @@ -76,6 +76,7 @@ */ #define HVM_PARAM_STORE_PFN 1 #define HVM_PARAM_STORE_EVTCHN 2 +#define HVM_PARAM_MAGIC_BASE_PFN 3 #define HVM_PARAM_IOREQ_PFN 5
For use cases such as Dom0less PV drivers, a mechanism to communicate Dom0less DomU's static data with the runtime control plane (Dom0) is needed. Since on Arm HVMOP is already the existing approach to address such use cases (for example the allocation of HVM_PARAM_CALLBACK_IRQ), add a new HVMOP key HVM_PARAM_MAGIC_BASE_PFN for storing the magic page region base PFN. The value will be set at Dom0less DomU construction time after Dom0less DomU's magic page region has been allocated. To keep consistent, also set the value for HVM_PARAM_MAGIC_BASE_PFN for libxl guests in alloc_magic_pages(). Reported-by: Alec Kwapis <alec.kwapis@medtronic.com> Signed-off-by: Henry Wang <xin.wang2@amd.com> --- tools/libs/guest/xg_dom_arm.c | 2 ++ xen/arch/arm/dom0less-build.c | 2 ++ xen/arch/arm/hvm.c | 1 + xen/include/public/hvm/params.h | 1 + 4 files changed, 6 insertions(+)