Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Jan Beulich]

On 16.03.2023 14:53, Alex Deucher wrote:
> On Thu, Mar 16, 2023 at 9:48 AM Juergen Gross  wrote:
>>
>> On 16.03.23 14:45, Alex Deucher wrote:
>>> On Thu, Mar 16, 2023 at 3:50 AM Jan Beulich  wrote:
>>>>
>>>> On 16.03.2023 00:25, Stefano Stabellini wrote:
>>>>> On Wed, 15 Mar 2023, Jan Beulich wrote:
>>>>>> On 15.03.2023 01:52, Stefano Stabellini wrote:
>>>>>>> On Mon, 13 Mar 2023, Jan Beulich wrote:
>>>>>>>> On 12.03.2023 13:01, Huang Rui wrote:
>>>>>>>>> Xen PVH is the paravirtualized mode and takes advantage of hardware
>>>>>>>>> virtualization support when possible. It will using the hardware IOMMU
>>>>>>>>> support instead of xen-swiotlb, so disable swiotlb if current domain 
>>>>>>>>> is
>>>>>>>>> Xen PVH.
>>>>>>>>
>>>>>>>> But the kernel has no way (yet) to drive the IOMMU, so how can it get
>>>>>>>> away without resorting to swiotlb in certain cases (like I/O to an
>>>>>>>> address-restricted device)?
>>>>>>>
>>>>>>> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no
>>>>>>> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU
>>>>>>> so we can use guest physical addresses instead of machine addresses for
>>>>>>> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available
>>>>>>> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding
>>>>>>> case is XENFEAT_not_direct_mapped).
>>>>>>
>>>>>> But how does Xen using an IOMMU help with, as said, address-restricted
>>>>>> devices? They may still need e.g. a 32-bit address to be programmed in,
>>>>>> and if the kernel has memory beyond the 4G boundary not all I/O buffers
>>>>>> may fulfill this requirement.
>>>>>
>>>>> In short, it is going to work as long as Linux has guest physical
>>>>> addresses (not machine addresses, those could be anything) lower than
>>>>> 4GB.
>>>>>
>>>>> If the address-restricted device does DMA via an IOMMU, then the device
>>>>> gets programmed by Linux using its guest physical addresses (not machine
>>>>> addresses).
>>>>>
>>>>> The 32-bit restriction would be applied by Linux to its choice of guest
>>>>> physical address to use to program the device, the same way it does on
>>>>> native. The device would be fine as it always uses Linux-provided <4GB
>>>>> addresses. After the IOMMU translation (pagetable setup by Xen), we
>>>>> could get any address, including >4GB addresses, and that is expected to
>>>>> work.
>>>>
>>>> I understand that's the "normal" way of working. But whatever the swiotlb
>>>> is used for in baremetal Linux, that would similarly require its use in
>>>> PVH (or HVM) aiui. So unconditionally disabling it in PVH would look to
>>>> me like an incomplete attempt to disable its use altogether on x86. What
>>>> difference of PVH vs baremetal am I missing here?
>>>
>>> swiotlb is not usable for GPUs even on bare metal.  They often have
>>> hundreds or megs or even gigs of memory mapped on the device at any
>>> given time.  Also, AMD GPUs support 44-48 bit DMA masks (depending on
>>> the chip family).
>>
>> But the swiotlb isn't per device, but system global.
> 
> Sure, but if the swiotlb is in use, then you can't really use the GPU.
> So you get to pick one.

Yet that "pick one" then can't be an unconditional disable in the source code.
If there's no way to avoid swiotlb on a per-device basis, then users will need
to be told to arrange for this via command line option when they want to use
the GPU is certain ways.

Jan

Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Jan Beulich]

On 16.03.2023 00:25, Stefano Stabellini wrote:
> On Wed, 15 Mar 2023, Jan Beulich wrote:
>> On 15.03.2023 01:52, Stefano Stabellini wrote:
>>> On Mon, 13 Mar 2023, Jan Beulich wrote:
>>>> On 12.03.2023 13:01, Huang Rui wrote:
>>>>> Xen PVH is the paravirtualized mode and takes advantage of hardware
>>>>> virtualization support when possible. It will using the hardware IOMMU
>>>>> support instead of xen-swiotlb, so disable swiotlb if current domain is
>>>>> Xen PVH.
>>>>
>>>> But the kernel has no way (yet) to drive the IOMMU, so how can it get
>>>> away without resorting to swiotlb in certain cases (like I/O to an
>>>> address-restricted device)?
>>>
>>> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no
>>> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU
>>> so we can use guest physical addresses instead of machine addresses for
>>> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available
>>> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding
>>> case is XENFEAT_not_direct_mapped).
>>
>> But how does Xen using an IOMMU help with, as said, address-restricted
>> devices? They may still need e.g. a 32-bit address to be programmed in,
>> and if the kernel has memory beyond the 4G boundary not all I/O buffers
>> may fulfill this requirement.
> 
> In short, it is going to work as long as Linux has guest physical
> addresses (not machine addresses, those could be anything) lower than
> 4GB.
> 
> If the address-restricted device does DMA via an IOMMU, then the device
> gets programmed by Linux using its guest physical addresses (not machine
> addresses).
> 
> The 32-bit restriction would be applied by Linux to its choice of guest
> physical address to use to program the device, the same way it does on
> native. The device would be fine as it always uses Linux-provided <4GB
> addresses. After the IOMMU translation (pagetable setup by Xen), we
> could get any address, including >4GB addresses, and that is expected to
> work.

I understand that's the "normal" way of working. But whatever the swiotlb
is used for in baremetal Linux, that would similarly require its use in
PVH (or HVM) aiui. So unconditionally disabling it in PVH would look to
me like an incomplete attempt to disable its use altogether on x86. What
difference of PVH vs baremetal am I missing here?

Jan

Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Jan Beulich]

On 15.03.2023 05:14, Huang Rui wrote:
> On Wed, Mar 15, 2023 at 08:52:30AM +0800, Stefano Stabellini wrote:
>> On Mon, 13 Mar 2023, Jan Beulich wrote:
>>> On 12.03.2023 13:01, Huang Rui wrote:
>>>> Xen PVH is the paravirtualized mode and takes advantage of hardware
>>>> virtualization support when possible. It will using the hardware IOMMU
>>>> support instead of xen-swiotlb, so disable swiotlb if current domain is
>>>> Xen PVH.
>>>
>>> But the kernel has no way (yet) to drive the IOMMU, so how can it get
>>> away without resorting to swiotlb in certain cases (like I/O to an
>>> address-restricted device)?
>>
>> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no
>> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU
>> so we can use guest physical addresses instead of machine addresses for
>> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available
>> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding
>> case is XENFEAT_not_direct_mapped).
> 
> Hi Jan, sorry to late reply. We are using the native kernel amdgpu and ttm
> driver on Dom0, amdgpu/ttm would like to use IOMMU to allocate coherent
> buffers for userptr that map the user space memory to gpu access, however,
> swiotlb doesn't support this. In other words, with swiotlb, we only can
> handle the buffer page by page.

But how does outright disabling swiotlb help with this? There still wouldn't
be an IOMMU that your kernel has control over. Looks like you want something
like pvIOMMU, but that work was never completed. And even then the swiotlb
may continue to be needed for other purposes.

Jan

Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Jan Beulich]

On 15.03.2023 01:52, Stefano Stabellini wrote:
> On Mon, 13 Mar 2023, Jan Beulich wrote:
>> On 12.03.2023 13:01, Huang Rui wrote:
>>> Xen PVH is the paravirtualized mode and takes advantage of hardware
>>> virtualization support when possible. It will using the hardware IOMMU
>>> support instead of xen-swiotlb, so disable swiotlb if current domain is
>>> Xen PVH.
>>
>> But the kernel has no way (yet) to drive the IOMMU, so how can it get
>> away without resorting to swiotlb in certain cases (like I/O to an
>> address-restricted device)?
> 
> I think Ray meant that, thanks to the IOMMU setup by Xen, there is no
> need for swiotlb-xen in Dom0. Address translations are done by the IOMMU
> so we can use guest physical addresses instead of machine addresses for
> DMA. This is a similar case to Dom0 on ARM when the IOMMU is available
> (see include/xen/arm/swiotlb-xen.h:xen_swiotlb_detect, the corresponding
> case is XENFEAT_not_direct_mapped).

But how does Xen using an IOMMU help with, as said, address-restricted
devices? They may still need e.g. a 32-bit address to be programmed in,
and if the kernel has memory beyond the 4G boundary not all I/O buffers
may fulfill this requirement.

Jan

Re: [RFC PATCH 1/5] x86/xen: disable swiotlb for xen pvh

[Jan Beulich]

On 12.03.2023 13:01, Huang Rui wrote:
> Xen PVH is the paravirtualized mode and takes advantage of hardware
> virtualization support when possible. It will using the hardware IOMMU
> support instead of xen-swiotlb, so disable swiotlb if current domain is
> Xen PVH.

But the kernel has no way (yet) to drive the IOMMU, so how can it get
away without resorting to swiotlb in certain cases (like I/O to an
address-restricted device)?

Jan

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Jan Beulich]

On 18.10.2022 13:02, Christoph Hellwig wrote:
> On Tue, Oct 18, 2022 at 10:57:37AM +0200, Jan Beulich wrote:
>> Shouldn't this then be xen_pv_domain() that you use here, and - if you
>> really want IS_ENABLED() in addition - CONFIG_XEN_PV?
> 
> I'll need help from people that understand Xen better than me what
> the exact conditions (and maybe also comments are).

Leaving the "i915 abuses" part aside (because I can't tell what exactly the
abuse is), but assuming that "can't cope with bounce buffering" means they
don't actually use the allocated buffers, I'd suggest this:

/*
 * For Xen PV guests pages aren't contiguous in DMA (machine) address
 * space.  The DMA API takes care of that both in dma_alloc_* (by
 * calling into the hypervisor to make the pages contiguous) and in
 * dma_map_* (by bounce buffering).  But i915 abuses ignores the
 * coherency aspects of the DMA API and thus can't cope with bounce
 * buffering actually happening, so add a hack here to force small
 * allocations and mappings when running in PV mode on Xen.
 */
if (IS_ENABLED(CONFIG_XEN_PV) && xen_pv_domain())
max = PAGE_SIZE;

I've dropped the TDX related remark because I don't think it's meaningful
for PV guests. Otoh I've left the "abuses ignores" word sequence as is, no
matter that it reads odd to me. Plus, as hinted at before, I'm not
convinced the IS_ENABLED() use is actually necessary or warranted here.

Jan

Re: i915 "GPU HANG", bisected to a2daa27c0c61 "swiotlb: simplify swiotlb_max_segment"

[Jan Beulich]

On 18.10.2022 10:24, Christoph Hellwig wrote:
> @@ -127,19 +128,22 @@ static inline unsigned int i915_sg_dma_sizes(struct 
> scatterlist *sg)
>   return page_sizes;
>  }
>  
> -static inline unsigned int i915_sg_segment_size(void)
> +static inline unsigned int i915_sg_segment_size(struct device *dev)
>  {
> - unsigned int size = swiotlb_max_segment();
> -
> - if (size == 0)
> - size = UINT_MAX;
> -
> - size = rounddown(size, PAGE_SIZE);
> - /* swiotlb_max_segment_size can return 1 byte when it means one page. */
> - if (size < PAGE_SIZE)
> - size = PAGE_SIZE;
> -
> - return size;
> + size_t max = min_t(size_t, UINT_MAX, dma_max_mapping_size(dev));
> +
> + /*
> +  * Xen on x86 can reshuffle pages under us.  The DMA API takes
> +  * care of that both in dma_alloc_* (by calling into the hypervisor
> +  * to make the pages contigous) and in dma_map_* (by bounce buffering).
> +  * But i915 abuses ignores the coherency aspects of the DMA API and
> +  * thus can't cope with bounce buffering actually happening, so add
> +  * a hack here to force small allocations and mapping when running on
> +  * Xen.  (good luck with TDX, btw --hch)
> +  */
> + if (IS_ENABLED(CONFIG_X86) && xen_domain())
> + max = PAGE_SIZE;
> + return round_down(max, PAGE_SIZE);
>  }

Shouldn't this then be xen_pv_domain() that you use here, and - if you
really want IS_ENABLED() in addition - CONFIG_XEN_PV?

Jan

Re: [PATCH 0/5] xen: cleanup detection of non-essential pv devices

[Jan Beulich]

On 22.10.2021 08:47, Juergen Gross wrote:
> Today the non-essential pv devices are hard coded in the xenbus driver
> and this list is lacking multiple entries.
> 
> This series reworks the detection logic of non-essential devices by
> adding a flag for that purpose to struct xenbus_driver.

I'm wondering whether it wouldn't better be the other way around: The
(hopefully few) essential ones get flagged, thus also making it more
prominent during patch review that a flag gets added (and justification
provided), instead of having to spot the lack of a flag getting set.

Jan

[PATCH] drm/xen: adjust Kconfig

[Jan Beulich]

By having selected DRM_XEN, I was assuming I would build the frontend
driver. As it turns out this is a dummy option, and I have really not
been building this (because I had DRM disabled). Make it a promptless
one, moving the "depends on" to the other, real option, and "select"ing
the dummy one.

Signed-off-by: Jan Beulich 

--- a/drivers/gpu/drm/xen/Kconfig
+++ b/drivers/gpu/drm/xen/Kconfig
@@ -1,15 +1,11 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config DRM_XEN
-   bool "DRM Support for Xen guest OS"
-   depends on XEN
-   help
- Choose this option if you want to enable DRM support
- for Xen.
+   bool
 
 config DRM_XEN_FRONTEND
tristate "Para-virtualized frontend driver for Xen guest OS"
-   depends on DRM_XEN
-   depends on DRM
+   depends on XEN && DRM
+   select DRM_XEN
select DRM_KMS_HELPER
select VIDEOMODE_HELPERS
select XEN_XENBUS_FRONTEND
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Jan Beulich]

>>> On 28.11.17 at 11:17, <christian.koe...@amd.com> wrote:
> Am 28.11.2017 um 10:46 schrieb Jan Beulich:
>>>>> On 28.11.17 at 10:12, <christian.koe...@amd.com> wrote:
>>> In theory the BIOS would search for address space and won't find
>>> anything, so the hotplug operation should fail even before it reaches
>>> the kernel in the first place.
>> How would the BIOS know what the OS does or plans to do?
> 
> As far as I know the ACPI BIOS should work directly with the register 
> content.
> 
> So when we update the register content to enable the MMIO decoding the 
> BIOS should know that as well.

I'm afraid I don't follow: During memory hotplug, surely you don't
expect the BIOS to do a PCI bus scan? Plus even if it did, it would
be racy - some device could, at this very moment, have memory
decoding disabled, just for the OS to re-enable it a millisecond
later. Yet looking at BAR values is meaningless when memory
decode of a device is disabled.

>> I think
>> it's the other way around - the OS needs to avoid using any regions
>> for MMIO which are marked as hotpluggable in SRAT.
> 
> I was under the impression that this is exactly what 
> acpi_numa_memory_affinity_init() does.

Perhaps, except that (when I last looked) insufficient state is
(was) being recorded to have that information readily available
at the time MMIO space above 4Gb needs to be allocated for
some device.

>> Since there is
>> no vNUMA yet for Xen Dom0, that would need special handling.
> 
> I think that the problem is rather that SRAT is NUMA specific and if I'm 
> not totally mistaken the content is ignored when NUMA support isn't 
> compiled into the kernel.
> 
> When Xen steals some memory from Dom0 by hocking up itself into the e820 
> call then I would say the cleanest way is to report this memory in e820 
> as reserved as well. But take that with a grain of salt, I'm seriously 
> not a Xen expert.

The E820 handling in PV Linux is all fake anyway - there's a single
chunk of memory given to a PV guest (including Dom0), contiguous
in what PV guests know as "physical address space" (not to be
mixed up with "machine address space", which is where MMIO
needs to be allocated from). Xen code in the kernel then mimics
an E820 matching the host one, moving around pieces of memory
in physical address space if necessary.

Since Dom0 knows the machine E820, MMIO allocation shouldn't
need to be much different there from the non-Xen case.

Jan

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Xen-devel] [PATCH v9 4/5] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v5

[Jan Beulich]

>>> On 28.11.17 at 10:12,  wrote:
> In theory the BIOS would search for address space and won't find 
> anything, so the hotplug operation should fail even before it reaches 
> the kernel in the first place.

How would the BIOS know what the OS does or plans to do? I think
it's the other way around - the OS needs to avoid using any regions
for MMIO which are marked as hotpluggable in SRAT. Since there is
no vNUMA yet for Xen Dom0, that would need special handling.

Jan

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ping: [PATCH] radeon: avoid boot hang in Xen Dom0

[Jan Beulich]

>>> On 04.11.16 at 15:32,  wrote:
> On Fri, Nov 4, 2016 at 6:44 AM, Jan Beulich  wrote:
>>>>> On 13.09.16 at 17:54,  wrote:
>>> While a hard hang in atom_asic_init() likely points at a deeper problem
>>> in the driver, restore the capability to boot a Xen Dom0 by simply
>>> avoiding the call there: Other than for Xen DomU, Dom0 owning a device
>>> does not really mean is has got passed through to it.
>>>
>>> In case it is of interest for further investigation, lspci for the
>>> offending device says:
>>>
>>> ATI Technologies Inc RS480 [Radeon Xpress 200G Series] [1002:5954]
>>>
>>> Fixes: 05082b8bbd "drm/radeon: fix asic initialization for virtualized 
>>> environments"
>>
>> I may have overlooked a different fix dealing with the problem; if
>> so, I'd appreciate that fix being pointed out.
> 
> Already fixed:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=88 
> 4031f0aacf57dad1575f96714efc80de9b19cc

Hmm, that indeed should make the immediate problem go away.
Nevertheless I do think that Xen Dom0 should be treated just
like native here. Only DomU should be forced through that extra
path.

I did also notice that the equivalent function under amdgpu/ has
gone away again during the 4.9 merge window...

Jan

Ping: [PATCH] radeon: avoid boot hang in Xen Dom0

[Jan Beulich]

>>> On 13.09.16 at 17:54,  wrote:
> While a hard hang in atom_asic_init() likely points at a deeper problem
> in the driver, restore the capability to boot a Xen Dom0 by simply
> avoiding the call there: Other than for Xen DomU, Dom0 owning a device
> does not really mean is has got passed through to it.
> 
> In case it is of interest for further investigation, lspci for the
> offending device says:
> 
> ATI Technologies Inc RS480 [Radeon Xpress 200G Series] [1002:5954]
> 
> Fixes: 05082b8bbd "drm/radeon: fix asic initialization for virtualized 
> environments"

I may have overlooked a different fix dealing with the problem; if
so, I'd appreciate that fix being pointed out.

Thanks, Jan

> Signed-off-by: Jan Beulich 
> ---
>  drivers/gpu/drm/radeon/radeon_device.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- 4.8-rc6/drivers/gpu/drm/radeon/radeon_device.c
> +++ 4.8-rc6-radeon-Xen-boot/drivers/gpu/drm/radeon/radeon_device.c
> @@ -34,6 +34,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "radeon_reg.h"
>  #include "radeon.h"
>  #include "atom.h"
> @@ -642,7 +643,7 @@ void radeon_gtt_location(struct radeon_d
>  static bool radeon_device_is_virtual(void)
>  {
>  #ifdef CONFIG_X86
> - return boot_cpu_has(X86_FEATURE_HYPERVISOR);
> + return boot_cpu_has(X86_FEATURE_HYPERVISOR) && !xen_initial_domain();
>  #else
>   return false;
>  #endif
> 
> 
> 

[PATCH] radeon: avoid boot hang in Xen Dom0

[Jan Beulich]

While a hard hang in atom_asic_init() likely points at a deeper problem
in the driver, restore the capability to boot a Xen Dom0 by simply
avoiding the call there: Other than for Xen DomU, Dom0 owning a device
does not really mean is has got passed through to it.

In case it is of interest for further investigation, lspci for the
offending device says:

ATI Technologies Inc RS480 [Radeon Xpress 200G Series] [1002:5954]

Fixes: 05082b8bbd "drm/radeon: fix asic initialization for virtualized 
environments"
Signed-off-by: Jan Beulich 
---
 drivers/gpu/drm/radeon/radeon_device.c |3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- 4.8-rc6/drivers/gpu/drm/radeon/radeon_device.c
+++ 4.8-rc6-radeon-Xen-boot/drivers/gpu/drm/radeon/radeon_device.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "radeon_reg.h"
 #include "radeon.h"
 #include "atom.h"
@@ -642,7 +643,7 @@ void radeon_gtt_location(struct radeon_d
 static bool radeon_device_is_virtual(void)
 {
 #ifdef CONFIG_X86
-   return boot_cpu_has(X86_FEATURE_HYPERVISOR);
+   return boot_cpu_has(X86_FEATURE_HYPERVISOR) && !xen_initial_domain();
 #else
return false;
 #endif

[PATCH v2] drm/mgag200: don't use uninitialized variables in mga_g200se_set_plls()

[Jan Beulich]

I can only guess that instead of testm/testn (which are either
uninitialized or have pre-determined values at the end of the preceding
loops) n and m were meant to be used by commit e829d7ef9f
("drm/mgag200: Add support for a new rev of G200e"). In any event the
compiler is right in warning that testm/testn are possibly uninitalized
at this point, i.e. some change is needed no matter what.

Signed-off-by: Jan Beulich 
Cc: Mathieu Larouche 
---
v2: As pointed out by Mathieu, 1 needs to be added to both m and n when
consuming them.
---
 drivers/gpu/drm/mgag200/mgag200_mode.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ 4.3-rc6-mgag200-uninit/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -194,7 +194,7 @@ static int mga_g200se_set_plls(struct mg
}
}

-   fvv = pllreffreq * testn / testm;
+   fvv = pllreffreq * (n + 1) / (m + 1);
fvv = (fvv - 80) / 5;

if (fvv > 15)

drm/mgag200: don't use uninitialized variables in mga_g200se_set_plls()

[Jan Beulich]

>>> On 20.10.15 at 16:47,  wrote:
> On 19/10/2015 6:29 AM, Jan Beulich wrote:
>> --- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ 4.3-rc6-mgag200-uninit/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -194,7 +194,7 @@ static int mga_g200se_set_plls(struct mg
>>  }
>>  }
>>   
>> -fvv = pllreffreq * testn / testm;
>> +fvv = pllreffreq * n / m;
>>  fvv = (fvv - 80) / 5;
>>   
>>  if (fvv > 15)
>>
> If you are using n/m instead of testn/testm, you need to
> add 1 to the variables to keep consistency. However, you
> have the same issue where m/n could be used without being
> initialized. So, I propose to keep testm, testn & testp
> and initialized them with the default value.

That makes them initialized, but since testn and testm are used
as loop variables I don't see how initializing them to default
values helps overcome the other half of the problem described
(them having a constant, pre-determined value at the end of
the loops: testn=257, testm=33). As to testp - it is always
initialized anyway (so long as P_ARRAY_SIZE is not zero). And
in the if() branch initialization isn't needed either, since the
variables aren't being used after the loops.

Jan

drm/mgag200: don't use uninitialized variables in mga_g200se_set_plls()

[Jan Beulich]

I can only guess that instead of testm/testn (which are either
uninitialized or have pre-determined values at the end of the preceding
loops) n and m were meant to be used by commit e829d7ef9f
("drm/mgag200: Add support for a new rev of G200e"). In any event the
compiler is right in warning that testm/testn are possibly uninitalized
at this point, i.e. some change is needed no matter what.

Signed-off-by: Jan Beulich 
Cc: Mathieu Larouche 
---
 drivers/gpu/drm/mgag200/mgag200_mode.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ 4.3-rc6-mgag200-uninit/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -194,7 +194,7 @@ static int mga_g200se_set_plls(struct mg
}
}

-   fvv = pllreffreq * testn / testm;
+   fvv = pllreffreq * n / m;
fvv = (fvv - 80) / 5;

if (fvv > 15)

[PATCH] drm/mgag200: don't pass NULL file_priv to drm_gem_object_lookup()

[Jan Beulich]

Commit bf89209a6d ("drm/mga200g: Hold a proper reference for
cursor_set") clearly didn't take the call site in
drm_fb_helper.c:restore_fbdev_mode() into account, which passes NULL 
for file_priv and hence causes drm_gem_object_lookup() to fault. Move
the lookup back to before "obj" is actually needed, adjusting error
paths suitably once again.

Signed-off-by: Jan Beulich 
Cc: Daniel Vetter 
Cc: Thierry Reding 
---
 drivers/gpu/drm/mgag200/mgag200_cursor.c |   18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

--- 4.3-rc6/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ 4.3-rc6-mgag200-cursor-set/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -40,7 +40,7 @@ int mga_crtc_cursor_set(struct drm_crtc
struct mgag200_bo *pixels_2 = mdev->cursor.pixels_2;
struct mgag200_bo *pixels_current = mdev->cursor.pixels_current;
struct mgag200_bo *pixels_prev = mdev->cursor.pixels_prev;
-   struct drm_gem_object *obj;
+   struct drm_gem_object *obj = NULL;
struct mgag200_bo *bo = NULL;
int ret = 0;
unsigned int i, row, col;
@@ -70,15 +70,11 @@ int mga_crtc_cursor_set(struct drm_crtc
BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev);
BUG_ON(pixels_current == pixels_prev);

-   obj = drm_gem_object_lookup(dev, file_priv, handle);
-   if (!obj)
-   return -ENOENT;
-
ret = mgag200_bo_reserve(pixels_1, true);
if (ret) {
WREG8(MGA_CURPOSXL, 0);
WREG8(MGA_CURPOSXH, 0);
-   goto out_unref;
+   return ret;
}
ret = mgag200_bo_reserve(pixels_2, true);
if (ret) {
@@ -110,6 +106,12 @@ int mga_crtc_cursor_set(struct drm_crtc
}
}

+   obj = drm_gem_object_lookup(dev, file_priv, handle);
+   if (!obj) {
+   ret = -ENOENT;
+   goto out1;
+   }
+
bo = gem_to_mga_bo(obj);
ret = mgag200_bo_reserve(bo, true);
if (ret) {
@@ -243,13 +245,13 @@ int mga_crtc_cursor_set(struct drm_crtc
  out2:
mgag200_bo_unreserve(bo);
  out1:
+   if (obj)
+   drm_gem_object_unreference_unlocked(obj);
if (ret)
mga_hide_cursor(mdev);
mgag200_bo_unreserve(pixels_1);
 out_unreserve1:
mgag200_bo_unreserve(pixels_2);
-out_unref:
-   drm_gem_object_unreference_unlocked(obj);

return ret;
 }