| Message ID | 1376687156-6737-22-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote: > On Versatile there is a bunch of devices that must not pass-through to any "there are a bunch of devices which must not be passed-through" > guest (power management and cache coherency devices). > > This commit also blacklist the HDLCD device because then is unable to correctly ^Linux? > map the framebuffer. Therefore, when Linux will try to access to the framebuffer, > Xen will receive a non-handled data access. Can/should this be conditional on whether Xen has console=hdlcd or not? Or does Xen use the device unconditionally if it exists? TBH I think it would be normal to prefer that Linux gets this device... (I'm unclear how this relates to memreserve as mention in the code comment) > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c > index 8fc30c4..ece7bd9 100644 > --- a/xen/arch/arm/platforms/vexpress.c > +++ b/xen/arch/arm/platforms/vexpress.c > @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = > NULL > }; > > +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = > +{ > + /* Cache Coherent Interconnect */ > + DT_MATCH_COMPATIBLE("arm,cci-400"), > + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), > + /* Video device > + * TODO: remove it once memreserve is handled properly by Xen > + */ > + DT_MATCH_COMPATIBLE("arm,hdlcd"), > + /* Hardware power management */ > + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), > + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), > + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), > + { /* sentinel */ }, > +}; > + > PLATFORM_START(vexpress, "VERSATILE EXPRESS") > .compatible = vexpress_dt_compat, > .reset = vexpress_reset, > + .blacklist_dev = vexpress_blacklist_dev, > PLATFORM_END > > /*
On 08/22/2013 03:00 PM, Ian Campbell wrote: > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote: >> On Versatile there is a bunch of devices that must not pass-through to any > > "there are a bunch of devices which must not be passed-through" > >> guest (power management and cache coherency devices). >> >> This commit also blacklist the HDLCD device because then is unable to correctly > ^Linux? > >> map the framebuffer. Therefore, when Linux will try to access to the framebuffer, >> Xen will receive a non-handled data access. > > Can/should this be conditional on whether Xen has console=hdlcd or not? > Or does Xen use the device unconditionally if it exists? TBH I think it > would be normal to prefer that Linux gets this device... > > (I'm unclear how this relates to memreserve as mention in the code > comment) This issue is only when the HDLCD is used by Linux (not Xen). To specify where the framebuffer lives in the memory, there is a property "framebuffer" which contains address/size. This regions must be reserved to avoid Linux/u-boot play with it. So the DTS has a memreserve range with the same value. I don't really understand all memreserve things but Xen must cope with it. If this device is mapped, Xen will receive a data abort because Linux can't access to the framebuffer. This solution is not upstream (only in the Linaro tree). I don't know if this driver works with the vanilla kernel. > >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c >> index 8fc30c4..ece7bd9 100644 >> --- a/xen/arch/arm/platforms/vexpress.c >> +++ b/xen/arch/arm/platforms/vexpress.c >> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = >> NULL >> }; >> >> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = >> +{ >> + /* Cache Coherent Interconnect */ >> + DT_MATCH_COMPATIBLE("arm,cci-400"), >> + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), >> + /* Video device >> + * TODO: remove it once memreserve is handled properly by Xen >> + */ >> + DT_MATCH_COMPATIBLE("arm,hdlcd"), >> + /* Hardware power management */ >> + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), >> + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), >> + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), >> + { /* sentinel */ }, >> +}; >> + >> PLATFORM_START(vexpress, "VERSATILE EXPRESS") >> .compatible = vexpress_dt_compat, >> .reset = vexpress_reset, >> + .blacklist_dev = vexpress_blacklist_dev, >> PLATFORM_END >> >> /* > >
On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote: > On 08/22/2013 03:00 PM, Ian Campbell wrote: > > On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote: > >> On Versatile there is a bunch of devices that must not pass-through to any > > > > "there are a bunch of devices which must not be passed-through" > > > >> guest (power management and cache coherency devices). > >> > >> This commit also blacklist the HDLCD device because then is unable to correctly > > ^Linux? > > > >> map the framebuffer. Therefore, when Linux will try to access to the framebuffer, > >> Xen will receive a non-handled data access. > > > > Can/should this be conditional on whether Xen has console=hdlcd or not? > > Or does Xen use the device unconditionally if it exists? TBH I think it > > would be normal to prefer that Linux gets this device... > > > > (I'm unclear how this relates to memreserve as mention in the code > > comment) > > This issue is only when the HDLCD is used by Linux (not Xen). To specify > where the framebuffer lives in the memory, there is a property > "framebuffer" which contains address/size. > This regions must be reserved to avoid Linux/u-boot play with it. So the > DTS has a memreserve range with the same value. I don't really > understand all memreserve things but Xen must cope with it. Yes, Xen needs to learn memreserve, it's used on midway too for example. > If this device is mapped, Xen will receive a data abort because Linux > can't access to the framebuffer. Because it wasn't part of the set of memory which we assigned to the guest? It seems like it would be hard to link an individual memsreserve to a particular device, at least not without device specific logic (e.g looking at the "framebuffer" property in this case). Sounds like we might need list of compatible -> dom0_init_hook functions, with the appropriate hook called for each device which we pass through. Ian. > > This solution is not upstream (only in the Linaro tree). I don't know if > this driver works with the vanilla kernel. > > > > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> --- > >> xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > >> > >> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c > >> index 8fc30c4..ece7bd9 100644 > >> --- a/xen/arch/arm/platforms/vexpress.c > >> +++ b/xen/arch/arm/platforms/vexpress.c > >> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = > >> NULL > >> }; > >> > >> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = > >> +{ > >> + /* Cache Coherent Interconnect */ > >> + DT_MATCH_COMPATIBLE("arm,cci-400"), > >> + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), > >> + /* Video device > >> + * TODO: remove it once memreserve is handled properly by Xen > >> + */ > >> + DT_MATCH_COMPATIBLE("arm,hdlcd"), > >> + /* Hardware power management */ > >> + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), > >> + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), > >> + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), > >> + { /* sentinel */ }, > >> +}; > >> + > >> PLATFORM_START(vexpress, "VERSATILE EXPRESS") > >> .compatible = vexpress_dt_compat, > >> .reset = vexpress_reset, > >> + .blacklist_dev = vexpress_blacklist_dev, > >> PLATFORM_END > >> > >> /* > > > > > >
On 08/22/2013 03:36 PM, Ian Campbell wrote: > On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote: >> On 08/22/2013 03:00 PM, Ian Campbell wrote: >>> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote: >>>> On Versatile there is a bunch of devices that must not pass-through to any >>> >>> "there are a bunch of devices which must not be passed-through" >>> >>>> guest (power management and cache coherency devices). >>>> >>>> This commit also blacklist the HDLCD device because then is unable to correctly >>> ^Linux? >>> >>>> map the framebuffer. Therefore, when Linux will try to access to the framebuffer, >>>> Xen will receive a non-handled data access. >>> >>> Can/should this be conditional on whether Xen has console=hdlcd or not? >>> Or does Xen use the device unconditionally if it exists? TBH I think it >>> would be normal to prefer that Linux gets this device... >>> >>> (I'm unclear how this relates to memreserve as mention in the code >>> comment) >> >> This issue is only when the HDLCD is used by Linux (not Xen). To specify >> where the framebuffer lives in the memory, there is a property >> "framebuffer" which contains address/size. >> This regions must be reserved to avoid Linux/u-boot play with it. So the >> DTS has a memreserve range with the same value. I don't really >> understand all memreserve things but Xen must cope with it. > > Yes, Xen needs to learn memreserve, it's used on midway too for example. As I understand memreserve can only contains RAM region. Right? >> If this device is mapped, Xen will receive a data abort because Linux >> can't access to the framebuffer. > > Because it wasn't part of the set of memory which we assigned to the > guest? Yes. The framebuffer steals RAM, in this case, the end of it. > It seems like it would be hard to link an individual memsreserve to a > particular device, at least not without device specific logic (e.g > looking at the "framebuffer" property in this case). > > Sounds like we might need list of compatible -> dom0_init_hook > functions, with the appropriate hook called for each device which we > pass through. It could be a solution. In this case, we just need to update the framebuffer property and the memreserve. > > Ian. > >> >> This solution is not upstream (only in the Linaro tree). I don't know if >> this driver works with the vanilla kernel. >> >>> >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> --- >>>> xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ >>>> 1 file changed, 17 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c >>>> index 8fc30c4..ece7bd9 100644 >>>> --- a/xen/arch/arm/platforms/vexpress.c >>>> +++ b/xen/arch/arm/platforms/vexpress.c >>>> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = >>>> NULL >>>> }; >>>> >>>> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = >>>> +{ >>>> + /* Cache Coherent Interconnect */ >>>> + DT_MATCH_COMPATIBLE("arm,cci-400"), >>>> + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), >>>> + /* Video device >>>> + * TODO: remove it once memreserve is handled properly by Xen >>>> + */ >>>> + DT_MATCH_COMPATIBLE("arm,hdlcd"), >>>> + /* Hardware power management */ >>>> + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), >>>> + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), >>>> + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), >>>> + { /* sentinel */ }, >>>> +}; >>>> + >>>> PLATFORM_START(vexpress, "VERSATILE EXPRESS") >>>> .compatible = vexpress_dt_compat, >>>> .reset = vexpress_reset, >>>> + .blacklist_dev = vexpress_blacklist_dev, >>>> PLATFORM_END >>>> >>>> /* >>> >>> >> >> > >
On Thu, 2013-08-22 at 15:51 +0100, Julien Grall wrote: > On 08/22/2013 03:36 PM, Ian Campbell wrote: > > On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote: > >> On 08/22/2013 03:00 PM, Ian Campbell wrote: > >>> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote: > >>>> On Versatile there is a bunch of devices that must not pass-through to any > >>> > >>> "there are a bunch of devices which must not be passed-through" > >>> > >>>> guest (power management and cache coherency devices). > >>>> > >>>> This commit also blacklist the HDLCD device because then is unable to correctly > >>> ^Linux? > >>> > >>>> map the framebuffer. Therefore, when Linux will try to access to the framebuffer, > >>>> Xen will receive a non-handled data access. > >>> > >>> Can/should this be conditional on whether Xen has console=hdlcd or not? > >>> Or does Xen use the device unconditionally if it exists? TBH I think it > >>> would be normal to prefer that Linux gets this device... > >>> > >>> (I'm unclear how this relates to memreserve as mention in the code > >>> comment) > >> > >> This issue is only when the HDLCD is used by Linux (not Xen). To specify > >> where the framebuffer lives in the memory, there is a property > >> "framebuffer" which contains address/size. > >> This regions must be reserved to avoid Linux/u-boot play with it. So the > >> DTS has a memreserve range with the same value. I don't really > >> understand all memreserve things but Xen must cope with it. > > > > Yes, Xen needs to learn memreserve, it's used on midway too for example. > > As I understand memreserve can only contains RAM region. Right? ePAPR seems to suggest it is only for RAM, yes. > > It seems like it would be hard to link an individual memsreserve to a > > particular device, at least not without device specific logic (e.g > > looking at the "framebuffer" property in this case). > > > > Sounds like we might need list of compatible -> dom0_init_hook > > functions, with the appropriate hook called for each device which we > > pass through. > > It could be a solution. In this case, we just need to update the > framebuffer property and the memreserve. I think in this case we probably want to create a 1:1 mapping of the video ram? In case e.g. the firmware has setup attributes, or the LCD cannot use a different address etc... > > > > > Ian. > > > >> > >> This solution is not upstream (only in the Linaro tree). I don't know if > >> this driver works with the vanilla kernel. > >> > >>> > >>>> > >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >>>> --- > >>>> xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ > >>>> 1 file changed, 17 insertions(+) > >>>> > >>>> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c > >>>> index 8fc30c4..ece7bd9 100644 > >>>> --- a/xen/arch/arm/platforms/vexpress.c > >>>> +++ b/xen/arch/arm/platforms/vexpress.c > >>>> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = > >>>> NULL > >>>> }; > >>>> > >>>> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = > >>>> +{ > >>>> + /* Cache Coherent Interconnect */ > >>>> + DT_MATCH_COMPATIBLE("arm,cci-400"), > >>>> + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), > >>>> + /* Video device > >>>> + * TODO: remove it once memreserve is handled properly by Xen > >>>> + */ > >>>> + DT_MATCH_COMPATIBLE("arm,hdlcd"), > >>>> + /* Hardware power management */ > >>>> + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), > >>>> + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), > >>>> + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), > >>>> + { /* sentinel */ }, > >>>> +}; > >>>> + > >>>> PLATFORM_START(vexpress, "VERSATILE EXPRESS") > >>>> .compatible = vexpress_dt_compat, > >>>> .reset = vexpress_reset, > >>>> + .blacklist_dev = vexpress_blacklist_dev, > >>>> PLATFORM_END > >>>> > >>>> /* > >>> > >>> > >> > >> > > > > > >
On 08/22/2013 04:02 PM, Ian Campbell wrote: > On Thu, 2013-08-22 at 15:51 +0100, Julien Grall wrote: >> On 08/22/2013 03:36 PM, Ian Campbell wrote: >>> On Thu, 2013-08-22 at 15:24 +0100, Julien Grall wrote: >>>> On 08/22/2013 03:00 PM, Ian Campbell wrote: >>>>> On Fri, 2013-08-16 at 22:05 +0100, Julien Grall wrote: >>>>>> On Versatile there is a bunch of devices that must not pass-through to any >>>>> >>>>> "there are a bunch of devices which must not be passed-through" >>>>> >>>>>> guest (power management and cache coherency devices). >>>>>> >>>>>> This commit also blacklist the HDLCD device because then is unable to correctly >>>>> ^Linux? >>>>> >>>>>> map the framebuffer. Therefore, when Linux will try to access to the framebuffer, >>>>>> Xen will receive a non-handled data access. >>>>> >>>>> Can/should this be conditional on whether Xen has console=hdlcd or not? >>>>> Or does Xen use the device unconditionally if it exists? TBH I think it >>>>> would be normal to prefer that Linux gets this device... >>>>> >>>>> (I'm unclear how this relates to memreserve as mention in the code >>>>> comment) >>>> >>>> This issue is only when the HDLCD is used by Linux (not Xen). To specify >>>> where the framebuffer lives in the memory, there is a property >>>> "framebuffer" which contains address/size. >>>> This regions must be reserved to avoid Linux/u-boot play with it. So the >>>> DTS has a memreserve range with the same value. I don't really >>>> understand all memreserve things but Xen must cope with it. >>> >>> Yes, Xen needs to learn memreserve, it's used on midway too for example. >> >> As I understand memreserve can only contains RAM region. Right? > > ePAPR seems to suggest it is only for RAM, yes. > >>> It seems like it would be hard to link an individual memsreserve to a >>> particular device, at least not without device specific logic (e.g >>> looking at the "framebuffer" property in this case). >>> >>> Sounds like we might need list of compatible -> dom0_init_hook >>> functions, with the appropriate hook called for each device which we >>> pass through. >> >> It could be a solution. In this case, we just need to update the >> framebuffer property and the memreserve. > > I think in this case we probably want to create a 1:1 mapping of the > video ram? In case e.g. the firmware has setup attributes, or the LCD > cannot use a different address etc... Do we need to take into account in the dom0 memory? For example, the amount of video RAM memory is 6Mb and the DOM0 memory is 256Mb. Does Xen must allocate? - 6Mb of 1:1 mapping - 250Mb of domain allocate page >> >>> >>> Ian. >>> >>>> >>>> This solution is not upstream (only in the Linaro tree). I don't know if >>>> this driver works with the vanilla kernel. >>>> >>>>> >>>>>> >>>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>>>> --- >>>>>> xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ >>>>>> 1 file changed, 17 insertions(+) >>>>>> >>>>>> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c >>>>>> index 8fc30c4..ece7bd9 100644 >>>>>> --- a/xen/arch/arm/platforms/vexpress.c >>>>>> +++ b/xen/arch/arm/platforms/vexpress.c >>>>>> @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = >>>>>> NULL >>>>>> }; >>>>>> >>>>>> +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = >>>>>> +{ >>>>>> + /* Cache Coherent Interconnect */ >>>>>> + DT_MATCH_COMPATIBLE("arm,cci-400"), >>>>>> + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), >>>>>> + /* Video device >>>>>> + * TODO: remove it once memreserve is handled properly by Xen >>>>>> + */ >>>>>> + DT_MATCH_COMPATIBLE("arm,hdlcd"), >>>>>> + /* Hardware power management */ >>>>>> + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), >>>>>> + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), >>>>>> + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), >>>>>> + { /* sentinel */ }, >>>>>> +}; >>>>>> + >>>>>> PLATFORM_START(vexpress, "VERSATILE EXPRESS") >>>>>> .compatible = vexpress_dt_compat, >>>>>> .reset = vexpress_reset, >>>>>> + .blacklist_dev = vexpress_blacklist_dev, >>>>>> PLATFORM_END >>>>>> >>>>>> /* >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >
On Thu, 2013-08-22 at 16:28 +0100, Julien Grall wrote: > > I think in this case we probably want to create a 1:1 mapping of the > > video ram? In case e.g. the firmware has setup attributes, or the LCD > > cannot use a different address etc... > > Do we need to take into account in the dom0 memory? > > For example, the amount of video RAM memory is 6Mb and the DOM0 memory > is 256Mb. > > Does Xen must allocate? > - 6Mb of 1:1 mapping > - 250Mb of domain allocate page Could do it either way. I'd be inclined to count the framebuffer as being in addition to dom0's actual allocation. Ian.
diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c index 8fc30c4..ece7bd9 100644 --- a/xen/arch/arm/platforms/vexpress.c +++ b/xen/arch/arm/platforms/vexpress.c @@ -125,9 +125,26 @@ static const char const *vexpress_dt_compat[] __initdata = NULL }; +static const struct dt_device_match vexpress_blacklist_dev[] __initconst = +{ + /* Cache Coherent Interconnect */ + DT_MATCH_COMPATIBLE("arm,cci-400"), + DT_MATCH_COMPATIBLE("arm,cci-400-pmu"), + /* Video device + * TODO: remove it once memreserve is handled properly by Xen + */ + DT_MATCH_COMPATIBLE("arm,hdlcd"), + /* Hardware power management */ + DT_MATCH_COMPATIBLE("arm,vexpress-reset"), + DT_MATCH_COMPATIBLE("arm,vexpress-reboot"), + DT_MATCH_COMPATIBLE("arm,vexpress-shutdown"), + { /* sentinel */ }, +}; + PLATFORM_START(vexpress, "VERSATILE EXPRESS") .compatible = vexpress_dt_compat, .reset = vexpress_reset, + .blacklist_dev = vexpress_blacklist_dev, PLATFORM_END /*
On Versatile there is a bunch of devices that must not pass-through to any guest (power management and cache coherency devices). This commit also blacklist the HDLCD device because then is unable to correctly map the framebuffer. Therefore, when Linux will try to access to the framebuffer, Xen will receive a non-handled data access. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/platforms/vexpress.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)