| Message ID | 1402935486-29136-16-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 16 Jun 2014, Julien Grall wrote: > This region will be split by the toolstack to allocate MMIO range for eac > device. > > For now only reserve a 512MB region, this should be enought to passthrough > multiple device at the same time. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/include/public/arch-arm.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index ac54cd6..789bffb 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; > #define GUEST_GICC_BASE 0x03002000ULL > #define GUEST_GICC_SIZE 0x00000100ULL > > +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ > +#define GUEST_MMIO_BASE 0x10000000ULL > +#define GUEST_MMIO_SIZE 0x20000000ULL Is it really necessary to specify size here? It looks like an artifical limitation to me: given that is unlikely that we'll ever be able to support non-PCI device hotplug, we only have to handle cold-plug here. So the toolstack has all the information it needs to build the perfect memory layout for the guest at VM creation time. > /* 16MB == 4096 pages reserved for guest to use as a region to map its > * grant table in. > */ > -- > 1.7.10.4 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
On 06/18/2014 04:12 PM, Stefano Stabellini wrote: > On Mon, 16 Jun 2014, Julien Grall wrote: >> This region will be split by the toolstack to allocate MMIO range for eac >> device. >> >> For now only reserve a 512MB region, this should be enought to passthrough >> multiple device at the same time. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/include/public/arch-arm.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >> index ac54cd6..789bffb 100644 >> --- a/xen/include/public/arch-arm.h >> +++ b/xen/include/public/arch-arm.h >> @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; >> #define GUEST_GICC_BASE 0x03002000ULL >> #define GUEST_GICC_SIZE 0x00000100ULL >> >> +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ >> +#define GUEST_MMIO_BASE 0x10000000ULL >> +#define GUEST_MMIO_SIZE 0x20000000ULL > > Is it really necessary to specify size here? It looks like an artifical > limitation to me: given that is unlikely that we'll ever be able to > support non-PCI device hotplug, we only have to handle cold-plug here. > So the toolstack has all the information it needs to build the perfect > memory layout for the guest at VM creation time. We have the same "artificial" limitation for the RAM banks... The toolstack doesn't know where the different regions end up without the size. As the layout may move in the future, adding the size avoid modifying the toolstack every time we change it. For instance, the layout will change again soon with guest support for GICv3. Regards,
On Wed, 2014-06-18 at 16:23 +0100, Julien Grall wrote: > On 06/18/2014 04:12 PM, Stefano Stabellini wrote: > > On Mon, 16 Jun 2014, Julien Grall wrote: > >> This region will be split by the toolstack to allocate MMIO range for eac > >> device. > >> > >> For now only reserve a 512MB region, this should be enought to passthrough > >> multiple device at the same time. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> --- > >> xen/include/public/arch-arm.h | 4 ++++ > >> 1 file changed, 4 insertions(+) > >> > >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > >> index ac54cd6..789bffb 100644 > >> --- a/xen/include/public/arch-arm.h > >> +++ b/xen/include/public/arch-arm.h > >> @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; > >> #define GUEST_GICC_BASE 0x03002000ULL > >> #define GUEST_GICC_SIZE 0x00000100ULL > >> > >> +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ > >> +#define GUEST_MMIO_BASE 0x10000000ULL > >> +#define GUEST_MMIO_SIZE 0x20000000ULL > > > > Is it really necessary to specify size here? It looks like an artifical > > limitation to me: given that is unlikely that we'll ever be able to > > support non-PCI device hotplug, we only have to handle cold-plug here. > > So the toolstack has all the information it needs to build the perfect > > memory layout for the guest at VM creation time. > > We have the same "artificial" limitation for the RAM banks... The > toolstack doesn't know where the different regions end up without the > size. As the layout may move in the future, adding the size avoid > modifying the toolstack every time we change it. It also provides a documentary clue to people modifying things in this file to remind them to think about how much space they need to try and leave for this when adding something else. Ian.
On Wed, 18 Jun 2014, Ian Campbell wrote: > On Wed, 2014-06-18 at 16:23 +0100, Julien Grall wrote: > > On 06/18/2014 04:12 PM, Stefano Stabellini wrote: > > > On Mon, 16 Jun 2014, Julien Grall wrote: > > >> This region will be split by the toolstack to allocate MMIO range for eac > > >> device. > > >> > > >> For now only reserve a 512MB region, this should be enought to passthrough > > >> multiple device at the same time. > > >> > > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > >> --- > > >> xen/include/public/arch-arm.h | 4 ++++ > > >> 1 file changed, 4 insertions(+) > > >> > > >> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > > >> index ac54cd6..789bffb 100644 > > >> --- a/xen/include/public/arch-arm.h > > >> +++ b/xen/include/public/arch-arm.h > > >> @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; > > >> #define GUEST_GICC_BASE 0x03002000ULL > > >> #define GUEST_GICC_SIZE 0x00000100ULL > > >> > > >> +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ > > >> +#define GUEST_MMIO_BASE 0x10000000ULL > > >> +#define GUEST_MMIO_SIZE 0x20000000ULL > > > > > > Is it really necessary to specify size here? It looks like an artifical > > > limitation to me: given that is unlikely that we'll ever be able to > > > support non-PCI device hotplug, we only have to handle cold-plug here. > > > So the toolstack has all the information it needs to build the perfect > > > memory layout for the guest at VM creation time. > > > > We have the same "artificial" limitation for the RAM banks... The > > toolstack doesn't know where the different regions end up without the > > size. As the layout may move in the future, adding the size avoid > > modifying the toolstack every time we change it. > > It also provides a documentary clue to people modifying things in this > file to remind them to think about how much space they need to try and > leave for this when adding something else. I am not against documentation or resonable defaults. Let me explain what I mean more clearly: if we are trying to assign 1 device with an MMIO region of 1024MB, we know that it is not going to fit. Can we rearrange the guest memory layout to increase GUEST_MMIO_SIZE? After all the guest hasn't booted yet. Otherwise could we calculate the size of the MMIO hole needed earlier, at the time of building the guest p2m? This sounds harder.
On 06/18/2014 06:48 PM, Stefano Stabellini wrote: > On Wed, 18 Jun 2014, Ian Campbell wrote: >> On Wed, 2014-06-18 at 16:23 +0100, Julien Grall wrote: >>> On 06/18/2014 04:12 PM, Stefano Stabellini wrote: >>>> On Mon, 16 Jun 2014, Julien Grall wrote: >>>>> This region will be split by the toolstack to allocate MMIO range for eac >>>>> device. >>>>> >>>>> For now only reserve a 512MB region, this should be enought to passthrough >>>>> multiple device at the same time. >>>>> >>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>> --- >>>>> xen/include/public/arch-arm.h | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h >>>>> index ac54cd6..789bffb 100644 >>>>> --- a/xen/include/public/arch-arm.h >>>>> +++ b/xen/include/public/arch-arm.h >>>>> @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; >>>>> #define GUEST_GICC_BASE 0x03002000ULL >>>>> #define GUEST_GICC_SIZE 0x00000100ULL >>>>> >>>>> +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ >>>>> +#define GUEST_MMIO_BASE 0x10000000ULL >>>>> +#define GUEST_MMIO_SIZE 0x20000000ULL >>>> >>>> Is it really necessary to specify size here? It looks like an artifical >>>> limitation to me: given that is unlikely that we'll ever be able to >>>> support non-PCI device hotplug, we only have to handle cold-plug here. >>>> So the toolstack has all the information it needs to build the perfect >>>> memory layout for the guest at VM creation time. >>> >>> We have the same "artificial" limitation for the RAM banks... The >>> toolstack doesn't know where the different regions end up without the >>> size. As the layout may move in the future, adding the size avoid >>> modifying the toolstack every time we change it. >> >> It also provides a documentary clue to people modifying things in this >> file to remind them to think about how much space they need to try and >> leave for this when adding something else. > > I am not against documentation or resonable defaults. Let me explain > what I mean more clearly: if we are trying to assign 1 device with an > MMIO region of 1024MB, we know that it is not going to fit. For non-PCI passthrough this size will unlikely happen. We always map a matter of few pages per-device. I think this size is enough for the time being. I plan to revisit it for PCI passthrough where we will be able to allocate some of them after 4G.
On Wed, 18 Jun 2014, Julien Grall wrote: > On 06/18/2014 06:48 PM, Stefano Stabellini wrote: > > On Wed, 18 Jun 2014, Ian Campbell wrote: > >> On Wed, 2014-06-18 at 16:23 +0100, Julien Grall wrote: > >>> On 06/18/2014 04:12 PM, Stefano Stabellini wrote: > >>>> On Mon, 16 Jun 2014, Julien Grall wrote: > >>>>> This region will be split by the toolstack to allocate MMIO range for eac > >>>>> device. > >>>>> > >>>>> For now only reserve a 512MB region, this should be enought to passthrough > >>>>> multiple device at the same time. > >>>>> > >>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >>>>> --- > >>>>> xen/include/public/arch-arm.h | 4 ++++ > >>>>> 1 file changed, 4 insertions(+) > >>>>> > >>>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > >>>>> index ac54cd6..789bffb 100644 > >>>>> --- a/xen/include/public/arch-arm.h > >>>>> +++ b/xen/include/public/arch-arm.h > >>>>> @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; > >>>>> #define GUEST_GICC_BASE 0x03002000ULL > >>>>> #define GUEST_GICC_SIZE 0x00000100ULL > >>>>> > >>>>> +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ > >>>>> +#define GUEST_MMIO_BASE 0x10000000ULL > >>>>> +#define GUEST_MMIO_SIZE 0x20000000ULL > >>>> > >>>> Is it really necessary to specify size here? It looks like an artifical > >>>> limitation to me: given that is unlikely that we'll ever be able to > >>>> support non-PCI device hotplug, we only have to handle cold-plug here. > >>>> So the toolstack has all the information it needs to build the perfect > >>>> memory layout for the guest at VM creation time. > >>> > >>> We have the same "artificial" limitation for the RAM banks... The > >>> toolstack doesn't know where the different regions end up without the > >>> size. As the layout may move in the future, adding the size avoid > >>> modifying the toolstack every time we change it. > >> > >> It also provides a documentary clue to people modifying things in this > >> file to remind them to think about how much space they need to try and > >> leave for this when adding something else. > > > > I am not against documentation or resonable defaults. Let me explain > > what I mean more clearly: if we are trying to assign 1 device with an > > MMIO region of 1024MB, we know that it is not going to fit. > > For non-PCI passthrough this size will unlikely happen. We always map a > matter of few pages per-device. > > I think this size is enough for the time being. I plan to revisit it for > PCI passthrough where we will be able to allocate some of them after 4G. Modern GPUs can easily exceed 512MB and they work on ARM.
On 18/06/14 19:14, Stefano Stabellini wrote: >> For non-PCI passthrough this size will unlikely happen. We always map a >> matter of few pages per-device. >> >> I think this size is enough for the time being. I plan to revisit it for >> PCI passthrough where we will be able to allocate some of them after 4G. > > Modern GPUs can easily exceed 512MB and they work on ARM. If it's like on x86, passthrough GPUs may also require firmware. At least it looks like the case with some configuration on the Arndale, which doesn't have any SMMU. I'd like to make this series as simple as possible so we can get "quickly" a working solution to passthrough device with small amount of MMIO region. So if you don't mind I suggest to bump the amount of MMIO region to 1GB, I may need to shrink the first RAM bank, and think about bigger region support in a follow-up series. Regards,
On Wed, 18 Jun 2014, Julien Grall wrote: > On 18/06/14 19:14, Stefano Stabellini wrote: > > > For non-PCI passthrough this size will unlikely happen. We always map a > > > matter of few pages per-device. > > > > > > I think this size is enough for the time being. I plan to revisit it for > > > PCI passthrough where we will be able to allocate some of them after 4G. > > > > Modern GPUs can easily exceed 512MB and they work on ARM. > > If it's like on x86, passthrough GPUs may also require firmware. At least it > looks like the case with some configuration on the Arndale, which doesn't have > any SMMU. > > I'd like to make this series as simple as possible so we can get "quickly" a > working solution to passthrough device with small amount of MMIO region. > > So if you don't mind I suggest to bump the amount of MMIO region to 1GB, I may > need to shrink the first RAM bank, and think about bigger region support in a > follow-up series. OK
On Wed, 2014-06-18 at 19:33 +0100, Julien Grall wrote: > > On 18/06/14 19:14, Stefano Stabellini wrote: > >> For non-PCI passthrough this size will unlikely happen. We always map a > >> matter of few pages per-device. > >> > >> I think this size is enough for the time being. I plan to revisit it for > >> PCI passthrough where we will be able to allocate some of them after 4G. > > > > Modern GPUs can easily exceed 512MB and they work on ARM. > > If it's like on x86, passthrough GPUs may also require firmware. At > least it looks like the case with some configuration on the Arndale, > which doesn't have any SMMU. > > I'd like to make this series as simple as possible so we can get > "quickly" a working solution to passthrough device with small amount of > MMIO region. > > So if you don't mind I suggest to bump the amount of MMIO region to 1GB, > I may need to shrink the first RAM bank, and think about bigger region > support in a follow-up series. I think so long as we aren't painting ourselves into any ABI corners (I don't think we are) then defering the handling of large devices is fine. > > Regards, >
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index ac54cd6..789bffb 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -369,6 +369,10 @@ typedef uint64_t xen_callback_t; #define GUEST_GICC_BASE 0x03002000ULL #define GUEST_GICC_SIZE 0x00000100ULL +/* Space for mapping MMIO from device passthrough: 512MB @ 256MB*/ +#define GUEST_MMIO_BASE 0x10000000ULL +#define GUEST_MMIO_SIZE 0x20000000ULL + /* 16MB == 4096 pages reserved for guest to use as a region to map its * grant table in. */
This region will be split by the toolstack to allocate MMIO range for eac device. For now only reserve a 512MB region, this should be enought to passthrough multiple device at the same time. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/include/public/arch-arm.h | 4 ++++ 1 file changed, 4 insertions(+)