| Message ID | 1556658172-8824-5-git-send-email-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,01/10] xen: add a p2mt parameter to map_mmio_regions | expand |
Hi, On 30/04/2019 22:02, Stefano Stabellini wrote: > Add a new memory policy option for the iomem parameter. > Possible values are: > - arm_devmem, device nGRE, the default on ARM > - arm_memory, WB cachable memory > - x86_uc: uncachable memory, the default on x86 > > Store the parameter in a new field in libxl_iomem_range. > > Pass the memory policy option to xc_domain_mem_map_policy. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > CC: ian.jackson@eu.citrix.com > CC: wei.liu2@citrix.com > --- > Changes in v2: > - add #define LIBXL_HAVE_MEMORY_POLICY > - ability to part the memory policy parameter even if gfn is not passed > - rename cache_policy to memory policy > - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE > - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB > - rename memory to arm_memory and devmem to arm_devmem > - expand the non-security support status to non device passthrough iomem > configurations > - rename iomem options > - add x86 specific iomem option > --- > SUPPORT.md | 2 +- > docs/man/xl.cfg.5.pod.in | 7 ++++++- > tools/libxl/libxl.h | 5 +++++ > tools/libxl/libxl_create.c | 21 +++++++++++++++++++-- > tools/libxl/libxl_types.idl | 9 +++++++++ > tools/xl/xl_parse.c | 22 +++++++++++++++++++++- > 6 files changed, 61 insertions(+), 5 deletions(-) > > diff --git a/SUPPORT.md b/SUPPORT.md > index e4fb15b..f29a299 100644 > --- a/SUPPORT.md > +++ b/SUPPORT.md > @@ -649,7 +649,7 @@ to be used in addition to QEMU. > > Status: Experimental > > -### ARM/Non-PCI device passthrough > +### ARM/Non-PCI device passthrough and other iomem configurations I am not sure why iomem is added here? Also what was the security support before hand? Was it supported? > > Status: Supported, not security supported > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > index c7d70e6..c85857e 100644 > --- a/docs/man/xl.cfg.5.pod.in > +++ b/docs/man/xl.cfg.5.pod.in > @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff> > It is recommended to only use this option for trusted VMs under > administrator's control. > > -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]> > +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]> > > Allow auto-translated domains to access specific hardware I/O memory pages. > > @@ -1233,6 +1233,11 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START> > as a start in the guest's address space, therefore performing a 1:1 mapping > by default. > All of these values must be given in hexadecimal format. > +B<MEMORY_POLICY> for ARM platforms: > + - "arm_devmem" for Device nGRE, the default on ARM This does not match the current default. At the moment, it is Device nGnRE. > + - "arm_memory" for Outer Shareable Write-Back Cacheable Memory The two names are quite confusing and will make quite difficult to introduce any new one. It also make little sense to use a different naming in xl and libxl. This only add an other level of confusion. Overall, this is not enough for a user to understand the memory policy. As I pointed out before, this is not straight forward on Arm as the resulting memory attribute will be a combination of stage-2 and stage-1. We need to explain the implication of using the memory and the consequence of misuse it. And particularly as this is not security supported so we don't end up to security support in the future something that don't work. Cheers,
On Wed, 1 May 2019, Julien Grall wrote: > Hi, > > On 30/04/2019 22:02, Stefano Stabellini wrote: > > Add a new memory policy option for the iomem parameter. > > Possible values are: > > - arm_devmem, device nGRE, the default on ARM > > - arm_memory, WB cachable memory > > - x86_uc: uncachable memory, the default on x86 > > > > Store the parameter in a new field in libxl_iomem_range. > > > > Pass the memory policy option to xc_domain_mem_map_policy. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > CC: ian.jackson@eu.citrix.com > > CC: wei.liu2@citrix.com > > --- > > Changes in v2: > > - add #define LIBXL_HAVE_MEMORY_POLICY > > - ability to part the memory policy parameter even if gfn is not passed > > - rename cache_policy to memory policy > > - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE > > - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB > > - rename memory to arm_memory and devmem to arm_devmem > > - expand the non-security support status to non device passthrough iomem > > configurations > > - rename iomem options > > - add x86 specific iomem option > > --- > > SUPPORT.md | 2 +- > > docs/man/xl.cfg.5.pod.in | 7 ++++++- > > tools/libxl/libxl.h | 5 +++++ > > tools/libxl/libxl_create.c | 21 +++++++++++++++++++-- > > tools/libxl/libxl_types.idl | 9 +++++++++ > > tools/xl/xl_parse.c | 22 +++++++++++++++++++++- > > 6 files changed, 61 insertions(+), 5 deletions(-) > > > > diff --git a/SUPPORT.md b/SUPPORT.md > > index e4fb15b..f29a299 100644 > > --- a/SUPPORT.md > > +++ b/SUPPORT.md > > @@ -649,7 +649,7 @@ to be used in addition to QEMU. > > Status: Experimental > > -### ARM/Non-PCI device passthrough > > +### ARM/Non-PCI device passthrough and other iomem configurations > > I am not sure why iomem is added here? It is added here to clarify that it is *not* security supported. > Also what was the security support before hand? Was it supported? In my view, it falls under the broader category of "Non-PCI device passthrough" so it was already not security supported. But I thought it would be good to make it explicit to avoid any misunderstandings. > > Status: Supported, not security supported > > diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in > > index c7d70e6..c85857e 100644 > > --- a/docs/man/xl.cfg.5.pod.in > > +++ b/docs/man/xl.cfg.5.pod.in > > @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a > > range, e.g. C<2f8-2ff> > > It is recommended to only use this option for trusted VMs under > > administrator's control. > > -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", > > "IOMEM_START,NUM_PAGES[@GFN]", ...]> > > +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", > > "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]> > > Allow auto-translated domains to access specific hardware I/O memory > > pages. > > @@ -1233,6 +1233,11 @@ B<GFN> is not specified, the mapping will be > > performed using B<IOMEM_START> > > as a start in the guest's address space, therefore performing a 1:1 > > mapping > > by default. > > All of these values must be given in hexadecimal format. > > +B<MEMORY_POLICY> for ARM platforms: > > + - "arm_devmem" for Device nGRE, the default on ARM > > This does not match the current default. At the moment, it is Device nGnRE. I'll fix this throughout the whole series > > + - "arm_memory" for Outer Shareable Write-Back Cacheable Memory > > The two names are quite confusing and will make quite difficult to introduce > any new one. It also make little sense to use a different naming in xl and > libxl. This only add an other level of confusion. I'll change them to match the Xen names: arm_dev_ngnre and arm_mem_wb. > Overall, this is not enough for a user to understand the memory policy. As I > pointed out before, this is not straight forward on Arm as the resulting > memory attribute will be a combination of stage-2 and stage-1. > > We need to explain the implication of using the memory and the consequence of > misuse it. And particularly as this is not security supported so we don't end > up to security support in the future something that don't work. I'll expand the text here to cover what you wrote.
Hi, On 17/06/2019 23:32, Stefano Stabellini wrote: > On Wed, 1 May 2019, Julien Grall wrote: >> Hi, >> >> On 30/04/2019 22:02, Stefano Stabellini wrote: >>> Add a new memory policy option for the iomem parameter. >>> Possible values are: >>> - arm_devmem, device nGRE, the default on ARM >>> - arm_memory, WB cachable memory >>> - x86_uc: uncachable memory, the default on x86 >>> >>> Store the parameter in a new field in libxl_iomem_range. >>> >>> Pass the memory policy option to xc_domain_mem_map_policy. >>> >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> >>> CC: ian.jackson@eu.citrix.com >>> CC: wei.liu2@citrix.com >>> --- >>> Changes in v2: >>> - add #define LIBXL_HAVE_MEMORY_POLICY >>> - ability to part the memory policy parameter even if gfn is not passed >>> - rename cache_policy to memory policy >>> - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE >>> - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB >>> - rename memory to arm_memory and devmem to arm_devmem >>> - expand the non-security support status to non device passthrough iomem >>> configurations >>> - rename iomem options >>> - add x86 specific iomem option >>> --- >>> SUPPORT.md | 2 +- >>> docs/man/xl.cfg.5.pod.in | 7 ++++++- >>> tools/libxl/libxl.h | 5 +++++ >>> tools/libxl/libxl_create.c | 21 +++++++++++++++++++-- >>> tools/libxl/libxl_types.idl | 9 +++++++++ >>> tools/xl/xl_parse.c | 22 +++++++++++++++++++++- >>> 6 files changed, 61 insertions(+), 5 deletions(-) >>> >>> diff --git a/SUPPORT.md b/SUPPORT.md >>> index e4fb15b..f29a299 100644 >>> --- a/SUPPORT.md >>> +++ b/SUPPORT.md >>> @@ -649,7 +649,7 @@ to be used in addition to QEMU. >>> Status: Experimental >>> -### ARM/Non-PCI device passthrough >>> +### ARM/Non-PCI device passthrough and other iomem configurations >> >> I am not sure why iomem is added here? > > It is added here to clarify that it is *not* security supported. > > >> Also what was the security support before hand? Was it supported? > > In my view, it falls under the broader category of "Non-PCI device > passthrough" so it was already not security supported. But I thought it > would be good to make it explicit to avoid any misunderstandings. I am ok with this clarification. However, this should really be in a separate patch. > > >>> Status: Supported, not security supported >>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in >>> index c7d70e6..c85857e 100644 >>> --- a/docs/man/xl.cfg.5.pod.in >>> +++ b/docs/man/xl.cfg.5.pod.in >>> @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a >>> range, e.g. C<2f8-2ff> >>> It is recommended to only use this option for trusted VMs under >>> administrator's control. >>> -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", >>> "IOMEM_START,NUM_PAGES[@GFN]", ...]> >>> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", >>> "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]> >>> Allow auto-translated domains to access specific hardware I/O memory >>> pages. >>> @@ -1233,6 +1233,11 @@ B<GFN> is not specified, the mapping will be >>> performed using B<IOMEM_START> >>> as a start in the guest's address space, therefore performing a 1:1 >>> mapping >>> by default. >>> All of these values must be given in hexadecimal format. >>> +B<MEMORY_POLICY> for ARM platforms: >>> + - "arm_devmem" for Device nGRE, the default on ARM >> >> This does not match the current default. At the moment, it is Device nGnRE. > > I'll fix this throughout the whole series > > >>> + - "arm_memory" for Outer Shareable Write-Back Cacheable Memory >> >> The two names are quite confusing and will make quite difficult to introduce >> any new one. It also make little sense to use a different naming in xl and >> libxl. This only add an other level of confusion. > > I'll change them to match the Xen names: arm_dev_ngnre and arm_mem_wb. I would prefer if we use uppercase for G, R E and WB. This make the name is bit more readable. > > >> Overall, this is not enough for a user to understand the memory policy. As I >> pointed out before, this is not straight forward on Arm as the resulting >> memory attribute will be a combination of stage-2 and stage-1. >> >> We need to explain the implication of using the memory and the consequence of >> misuse it. And particularly as this is not security supported so we don't end >> up to security support in the future something that don't work. > > I'll expand the text here to cover what you wrote. Thank you! Cheers,
On 30/04/2019 22:02, Stefano Stabellini wrote: > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index 89fe80f..a6c5e30 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -415,6 +415,21 @@ static void init_console_info(libxl__gc *gc, > Only 'channels' when mapped to consoles have a string name. */ > } > > +static uint32_t libxl__memory_policy_to_xc(libxl_memory_policy c) > +{ > + switch (c) { > + case LIBXL_MEMORY_POLICY_ARM_MEM_WB: > + return MEMORY_POLICY_ARM_MEM_WB; > + case LIBXL_MEMORY_POLICY_ARM_DEV_NGRE: > + return MEMORY_POLICY_ARM_DEV_nGRE; > + case LIBXL_MEMORY_POLICY_X86_UC: > + return MEMORY_POLICY_X86_UC; > + case LIBXL_MEMORY_POLICY_DEFAULT: > + default: Looking at this again, don't we want to bail out if the policy is unknown? My concern here is the user may configure with something it didn't expect. The risk is the problem will be hard to debug. I also believe this could be part of libxl_{arm,x86}.c allowing us to filter misuse early. Ian, Wei, any opinion? Cheers,
On Tue, 18 Jun 2019, Julien Grall wrote: > On 30/04/2019 22:02, Stefano Stabellini wrote: > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > > index 89fe80f..a6c5e30 100644 > > --- a/tools/libxl/libxl_create.c > > +++ b/tools/libxl/libxl_create.c > > @@ -415,6 +415,21 @@ static void init_console_info(libxl__gc *gc, > > Only 'channels' when mapped to consoles have a string name. */ > > } > > +static uint32_t libxl__memory_policy_to_xc(libxl_memory_policy c) > > +{ > > + switch (c) { > > + case LIBXL_MEMORY_POLICY_ARM_MEM_WB: > > + return MEMORY_POLICY_ARM_MEM_WB; > > + case LIBXL_MEMORY_POLICY_ARM_DEV_NGRE: > > + return MEMORY_POLICY_ARM_DEV_nGRE; > > + case LIBXL_MEMORY_POLICY_X86_UC: > > + return MEMORY_POLICY_X86_UC; > > + case LIBXL_MEMORY_POLICY_DEFAULT: > > + default: > > Looking at this again, don't we want to bail out if the policy is unknown? My > concern here is the user may configure with something it didn't expect. The > risk is the problem will be hard to debug. > > I also believe this could be part of libxl_{arm,x86}.c allowing us to filter > misuse early. This sounds like a good idea, I can do that. Then, I can also #ifdef the hypercalls defines, although for some reason today libxl doesn't have CONFIG_X86 or CONFIG_ARM set so I would also have to do the following in the libxl Makefile: ifeq ($(CONFIG_X86),y) CFLAGS_LIBXL += -DCONFIG_X86 else CFLAGS_LIBXL += -DCONFIG_ARM endif > Ian, Wei, any opinion?
Sorry for the formatting. On Tue, 18 Jun 2019, 23:09 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Tue, 18 Jun 2019, Julien Grall wrote: > > On 30/04/2019 22:02, Stefano Stabellini wrote: > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > > > index 89fe80f..a6c5e30 100644 > > > --- a/tools/libxl/libxl_create.c > > > +++ b/tools/libxl/libxl_create.c > > > @@ -415,6 +415,21 @@ static void init_console_info(libxl__gc *gc, > > > Only 'channels' when mapped to consoles have a string name. */ > > > } > > > +static uint32_t libxl__memory_policy_to_xc(libxl_memory_policy c) > > > +{ > > > + switch (c) { > > > + case LIBXL_MEMORY_POLICY_ARM_MEM_WB: > > > + return MEMORY_POLICY_ARM_MEM_WB; > > > + case LIBXL_MEMORY_POLICY_ARM_DEV_NGRE: > > > + return MEMORY_POLICY_ARM_DEV_nGRE; > > > + case LIBXL_MEMORY_POLICY_X86_UC: > > > + return MEMORY_POLICY_X86_UC; > > > + case LIBXL_MEMORY_POLICY_DEFAULT: > > > + default: > > > > Looking at this again, don't we want to bail out if the policy is > unknown? My > > concern here is the user may configure with something it didn't expect. > The > > risk is the problem will be hard to debug. > > > > I also believe this could be part of libxl_{arm,x86}.c allowing us to > filter > > misuse early. > > This sounds like a good idea, I can do that. Then, I can also #ifdef the > hypercalls defines, although for some reason today libxl doesn't have > CONFIG_X86 or CONFIG_ARM set so I would also have to do the following in > the libxl Makefile: > > ifeq ($(CONFIG_X86),y) > CFLAGS_LIBXL += -DCONFIG_X86 > else > CFLAGS_LIBXL += -DCONFIG_ARM > endif > Or just follow what we do today in other public headers: #if defined(__arm__) || defined(__aarch64__) You need to double check the exact syntax as I wrote it by memory. Cheers, > > > Ian, Wei, any opinion? > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel <div dir="auto"><div>Sorry for the formatting.<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 18 Jun 2019, 23:09 Stefano Stabellini, <<a href="mailto:sstabellini@kernel.org">sstabellini@kernel.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, 18 Jun 2019, Julien Grall wrote:<br> > On 30/04/2019 22:02, Stefano Stabellini wrote:<br> > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c<br> > > index 89fe80f..a6c5e30 100644<br> > > --- a/tools/libxl/libxl_create.c<br> > > +++ b/tools/libxl/libxl_create.c<br> > > @@ -415,6 +415,21 @@ static void init_console_info(libxl__gc *gc,<br> > > Only 'channels' when mapped to consoles have a string name. */<br> > > }<br> > > +static uint32_t libxl__memory_policy_to_xc(libxl_memory_policy c)<br> > > +{<br> > > + switch (c) {<br> > > + case LIBXL_MEMORY_POLICY_ARM_MEM_WB:<br> > > + return MEMORY_POLICY_ARM_MEM_WB;<br> > > + case LIBXL_MEMORY_POLICY_ARM_DEV_NGRE:<br> > > + return MEMORY_POLICY_ARM_DEV_nGRE;<br> > > + case LIBXL_MEMORY_POLICY_X86_UC:<br> > > + return MEMORY_POLICY_X86_UC;<br> > > + case LIBXL_MEMORY_POLICY_DEFAULT:<br> > > + default:<br> > <br> > Looking at this again, don't we want to bail out if the policy is unknown? My<br> > concern here is the user may configure with something it didn't expect. The<br> > risk is the problem will be hard to debug.<br> > <br> > I also believe this could be part of libxl_{arm,x86}.c allowing us to filter<br> > misuse early.<br> <br> This sounds like a good idea, I can do that. Then, I can also #ifdef the<br> hypercalls defines, although for some reason today libxl doesn't have<br> CONFIG_X86 or CONFIG_ARM set so I would also have to do the following in<br> the libxl Makefile:<br> <br> ifeq ($(CONFIG_X86),y)<br> CFLAGS_LIBXL += -DCONFIG_X86<br> else<br> CFLAGS_LIBXL += -DCONFIG_ARM<br> endif<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Or just follow what we do today in other public headers:</div><div dir="auto"><br></div><div dir="auto">#if defined(__arm__) || defined(__aarch64__)</div><div dir="auto"><br></div><div dir="auto">You need to double check the exact syntax as I wrote it by memory.</div><div dir="auto"><br></div><div dir="auto">Cheers,</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> <br> > Ian, Wei, any opinion?<br> <br> _______________________________________________<br> Xen-devel mailing list<br> <a href="mailto:Xen-devel@lists.xenproject.org" target="_blank" rel="noreferrer">Xen-devel@lists.xenproject.org</a><br> <a href="https://lists.xenproject.org/mailman/listinfo/xen-devel" rel="noreferrer noreferrer" target="_blank">https://lists.xenproject.org/mailman/listinfo/xen-devel</a></blockquote></div></div></div>
On Tue, 18 Jun 2019, Julien Grall wrote: > Sorry for the formatting. > > On Tue, 18 Jun 2019, 23:09 Stefano Stabellini, <sstabellini@kernel.org> wrote: > On Tue, 18 Jun 2019, Julien Grall wrote: > > On 30/04/2019 22:02, Stefano Stabellini wrote: > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > > > index 89fe80f..a6c5e30 100644 > > > --- a/tools/libxl/libxl_create.c > > > +++ b/tools/libxl/libxl_create.c > > > @@ -415,6 +415,21 @@ static void init_console_info(libxl__gc *gc, > > > Only 'channels' when mapped to consoles have a string name. */ > > > } > > > +static uint32_t libxl__memory_policy_to_xc(libxl_memory_policy c) > > > +{ > > > + switch (c) { > > > + case LIBXL_MEMORY_POLICY_ARM_MEM_WB: > > > + return MEMORY_POLICY_ARM_MEM_WB; > > > + case LIBXL_MEMORY_POLICY_ARM_DEV_NGRE: > > > + return MEMORY_POLICY_ARM_DEV_nGRE; > > > + case LIBXL_MEMORY_POLICY_X86_UC: > > > + return MEMORY_POLICY_X86_UC; > > > + case LIBXL_MEMORY_POLICY_DEFAULT: > > > + default: > > > > Looking at this again, don't we want to bail out if the policy is unknown? My > > concern here is the user may configure with something it didn't expect. The > > risk is the problem will be hard to debug. > > > > I also believe this could be part of libxl_{arm,x86}.c allowing us to filter > > misuse early. > > This sounds like a good idea, I can do that. Then, I can also #ifdef the > hypercalls defines, although for some reason today libxl doesn't have > CONFIG_X86 or CONFIG_ARM set so I would also have to do the following in > the libxl Makefile: > > ifeq ($(CONFIG_X86),y) > CFLAGS_LIBXL += -DCONFIG_X86 > else > CFLAGS_LIBXL += -DCONFIG_ARM > endif > > > Or just follow what we do today in other public headers: > > #if defined(__arm__) || defined(__aarch64__) > > You need to double check the exact syntax as I wrote it by memory. Doh! Thank you
diff --git a/SUPPORT.md b/SUPPORT.md index e4fb15b..f29a299 100644 --- a/SUPPORT.md +++ b/SUPPORT.md @@ -649,7 +649,7 @@ to be used in addition to QEMU. Status: Experimental -### ARM/Non-PCI device passthrough +### ARM/Non-PCI device passthrough and other iomem configurations Status: Supported, not security supported diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index c7d70e6..c85857e 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -1222,7 +1222,7 @@ is given in hexadecimal format and may either be a range, e.g. C<2f8-2ff> It is recommended to only use this option for trusted VMs under administrator's control. -=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN]", "IOMEM_START,NUM_PAGES[@GFN]", ...]> +=item B<iomem=[ "IOMEM_START,NUM_PAGES[@GFN],MEMORY_POLICY", "IOMEM_START,NUM_PAGES[@GFN][,MEMORY_POLICY]", ...]> Allow auto-translated domains to access specific hardware I/O memory pages. @@ -1233,6 +1233,11 @@ B<GFN> is not specified, the mapping will be performed using B<IOMEM_START> as a start in the guest's address space, therefore performing a 1:1 mapping by default. All of these values must be given in hexadecimal format. +B<MEMORY_POLICY> for ARM platforms: + - "arm_devmem" for Device nGRE, the default on ARM + - "arm_memory" for Outer Shareable Write-Back Cacheable Memory +B<MEMORY_POLICY> can be for x86 platforms: + - "x86_uc" for Uncachable Memory, the default on x86 Note that the IOMMU won't be updated with the mappings specified with this option. This option therefore should not be used to pass through any diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h index 482499a..2366331 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -379,6 +379,11 @@ #define LIBXL_HAVE_BUILDINFO_BOOTLOADER_ARGS 1 /* + * Support specifying memory policy information for memory mappings. + */ +#define LIBXL_HAVE_MEMORY_POLICY 1 + +/* * LIBXL_HAVE_EXTENDED_VKB indicates that libxl_device_vkb has extended fields: * - unique_id; * - feature_disable_keyboard; diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 89fe80f..a6c5e30 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -415,6 +415,21 @@ static void init_console_info(libxl__gc *gc, Only 'channels' when mapped to consoles have a string name. */ } +static uint32_t libxl__memory_policy_to_xc(libxl_memory_policy c) +{ + switch (c) { + case LIBXL_MEMORY_POLICY_ARM_MEM_WB: + return MEMORY_POLICY_ARM_MEM_WB; + case LIBXL_MEMORY_POLICY_ARM_DEV_NGRE: + return MEMORY_POLICY_ARM_DEV_nGRE; + case LIBXL_MEMORY_POLICY_X86_UC: + return MEMORY_POLICY_X86_UC; + case LIBXL_MEMORY_POLICY_DEFAULT: + default: + return MEMORY_POLICY_DEFAULT; + } +} + int libxl__domain_build(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid, @@ -1369,9 +1384,11 @@ static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev, ret = ERROR_FAIL; goto error_out; } - ret = xc_domain_memory_mapping(CTX->xch, domid, + ret = xc_domain_mem_map_policy(CTX->xch, domid, io->gfn, io->start, - io->number, 1); + io->number, 1, + libxl__memory_policy_to_xc( + io->memory_policy)); if (ret < 0) { LOGED(ERROR, domid, "failed to map to domain iomem range %"PRIx64"-%"PRIx64 diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl index cb4702f..4db8a62 100644 --- a/tools/libxl/libxl_types.idl +++ b/tools/libxl/libxl_types.idl @@ -272,6 +272,13 @@ libxl_ioport_range = Struct("ioport_range", [ ("number", uint32), ]) +libxl_memory_policy = Enumeration("memory_policy", [ + (0, "default"), + (1, "ARM_Dev_nGRE"), + (2, "ARM_Mem_WB"), + (3, "x86_UC"), + ], init_val = "LIBXL_MEMORY_POLICY_DEFAULT") + libxl_iomem_range = Struct("iomem_range", [ # start host frame number to be mapped to the guest ("start", uint64), @@ -279,6 +286,8 @@ libxl_iomem_range = Struct("iomem_range", [ ("number", uint64), # guest frame number used as a start for the mapping ("gfn", uint64, {'init_val': "LIBXL_INVALID_GFN"}), + # memory_policy of the memory region + ("memory_policy", libxl_memory_policy), ]) libxl_vga_interface_info = Struct("vga_interface_info", [ diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 352cd21..ed56931 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1883,6 +1883,7 @@ void parse_config_data(const char *config_source, } for (i = 0; i < num_iomem; i++) { int used; + const char *mempolicy; buf = xlu_cfg_get_listitem (iomem, i); if (!buf) { @@ -1895,11 +1896,30 @@ void parse_config_data(const char *config_source, &b_info->iomem[i].start, &b_info->iomem[i].number, &used, &b_info->iomem[i].gfn, &used); - if (ret < 2 || buf[used] != '\0') { + if (ret < 2) { fprintf(stderr, "xl: Invalid argument parsing iomem: %s\n", buf); exit(1); } + mempolicy = &buf[used]; + if (strlen(mempolicy) > 1) { + mempolicy++; + if (!strcmp(mempolicy, "arm_devmem")) + b_info->iomem[i].memory_policy = + LIBXL_MEMORY_POLICY_ARM_DEV_NGRE; + else if (!strcmp(mempolicy, "x86_uc")) + b_info->iomem[i].memory_policy = + LIBXL_MEMORY_POLICY_X86_UC; + else if (!strcmp(mempolicy, "arm_memory")) + b_info->iomem[i].memory_policy = + LIBXL_MEMORY_POLICY_ARM_MEM_WB; + else { + fprintf(stderr, + "xl: Invalid iomem memory policy parameter: %s\n", + mempolicy); + exit(1); + } + } } }
Add a new memory policy option for the iomem parameter. Possible values are: - arm_devmem, device nGRE, the default on ARM - arm_memory, WB cachable memory - x86_uc: uncachable memory, the default on x86 Store the parameter in a new field in libxl_iomem_range. Pass the memory policy option to xc_domain_mem_map_policy. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: ian.jackson@eu.citrix.com CC: wei.liu2@citrix.com --- Changes in v2: - add #define LIBXL_HAVE_MEMORY_POLICY - ability to part the memory policy parameter even if gfn is not passed - rename cache_policy to memory policy - rename MEMORY_POLICY_DEVMEM to MEMORY_POLICY_ARM_DEV_nGRE - rename MEMORY_POLICY_MEMORY to MEMORY_POLICY_ARM_MEM_WB - rename memory to arm_memory and devmem to arm_devmem - expand the non-security support status to non device passthrough iomem configurations - rename iomem options - add x86 specific iomem option --- SUPPORT.md | 2 +- docs/man/xl.cfg.5.pod.in | 7 ++++++- tools/libxl/libxl.h | 5 +++++ tools/libxl/libxl_create.c | 21 +++++++++++++++++++-- tools/libxl/libxl_types.idl | 9 +++++++++ tools/xl/xl_parse.c | 22 +++++++++++++++++++++- 6 files changed, 61 insertions(+), 5 deletions(-)