[PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[Malcolm Crossley]

The PCI option ROM BAR uses the LSB to indicate if the BAR is enabled.
The AMD graphics driver sets the address bit's of the BAR to 0 but leaves the
LSB set to 1. Whilst this is not good practice, QEMU should be ignoring the
non address parts of the BAR.

This patch adds masking of the bits 0-11 (4k page) parts of the BAR
before comparing the address to 0.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
---
 hw/pass-through.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 304c438..4c171de 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -2208,7 +2208,7 @@ static void pt_bar_mapping_one(struct pt_dev *ptdev, int bar, int io_enable,
     }
 
     /* prevent guest software mapping memory resource to 00000000h */
-    if ((base->bar_flag == PT_BAR_FLAG_MEM) && (r_addr == 0))
+    if ((base->bar_flag == PT_BAR_FLAG_MEM) && ((r_addr & XC_PAGE_MASK == 0))
         r_addr = -1;
 
     /* align resource size (memory type only) */
-- 
1.7.1

Re: [PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[Jan Beulich]

>>> On 12.05.14 at 17:36, <malcolm.crossley@citrix.com> wrote:
> The PCI option ROM BAR uses the LSB to indicate if the BAR is enabled.
> The AMD graphics driver sets the address bit's of the BAR to 0 but leaves 
> the
> LSB set to 1. Whilst this is not good practice, QEMU should be ignoring the
> non address parts of the BAR.

All you say above only warrants the PCI defined bits to be masked
off, ...

> This patch adds masking of the bits 0-11 (4k page) parts of the BAR
> before comparing the address to 0.

... i.e. 0-10 (2k).

Jan

Re: [PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[Malcolm Crossley]

On 12/05/14 16:52, Jan Beulich wrote:
>>>> On 12.05.14 at 17:36, <malcolm.crossley@citrix.com> wrote:
>> The PCI option ROM BAR uses the LSB to indicate if the BAR is enabled.
>> The AMD graphics driver sets the address bit's of the BAR to 0 but leaves 
>> the
>> LSB set to 1. Whilst this is not good practice, QEMU should be ignoring the
>> non address parts of the BAR.
> 
> All you say above only warrants the PCI defined bits to be masked
> off, ...
> 

But we've only got 4k mapping granularity with the IOMMU, so if we try
to map to an address between 2k and 4k then we will overlap with the
bottom 2k which is likely to cause problems.

Malcolm


>> This patch adds masking of the bits 0-11 (4k page) parts of the BAR
>> before comparing the address to 0.
> 
> ... i.e. 0-10 (2k).
> 
> Jan
> 

Re: [PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[Jan Beulich]

>>> On 12.05.14 at 17:59, <malcolm.crossley@citrix.com> wrote:
> On 12/05/14 16:52, Jan Beulich wrote:
>>>>> On 12.05.14 at 17:36, <malcolm.crossley@citrix.com> wrote:
>>> The PCI option ROM BAR uses the LSB to indicate if the BAR is enabled.
>>> The AMD graphics driver sets the address bit's of the BAR to 0 but leaves 
>>> the
>>> LSB set to 1. Whilst this is not good practice, QEMU should be ignoring the
>>> non address parts of the BAR.
>> 
>> All you say above only warrants the PCI defined bits to be masked
>> off, ...
>> 
> 
> But we've only got 4k mapping granularity with the IOMMU, so if we try
> to map to an address between 2k and 4k then we will overlap with the
> bottom 2k which is likely to cause problems.

What has the IOMMU got to do with this? Any such overlap would
be similarly (non-)problematic elsewhere in the address space.

Jan

Re: [PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[Malcolm Crossley]

On 13/05/14 07:33, Jan Beulich wrote:
>>>> On 12.05.14 at 17:59, <malcolm.crossley@citrix.com> wrote:
>> On 12/05/14 16:52, Jan Beulich wrote:
>>>>>> On 12.05.14 at 17:36, <malcolm.crossley@citrix.com> wrote:
>>>> The PCI option ROM BAR uses the LSB to indicate if the BAR is enabled.
>>>> The AMD graphics driver sets the address bit's of the BAR to 0 but leaves 
>>>> the
>>>> LSB set to 1. Whilst this is not good practice, QEMU should be ignoring the
>>>> non address parts of the BAR.
>>>
>>> All you say above only warrants the PCI defined bits to be masked
>>> off, ...
>>>
>>
>> But we've only got 4k mapping granularity with the IOMMU, so if we try
>> to map to an address between 2k and 4k then we will overlap with the
>> bottom 2k which is likely to cause problems.
> 
> What has the IOMMU got to do with this? Any such overlap would
> be similarly (non-)problematic elsewhere in the address space.
> 

Sorry it took so long to reply to this. I wrongly said the IOMMU was
responsible for VM outbound mappings.

The 4k restriction is still there because QEMU uses the
xc_domain_memory_mapping function (see pt_iomem_map in qemu-trad) to
create the VM outbound mapping to the option ROM BAR. So you still have
a functional problem is the guest tries to map the option ROM to address
> 2k && < 4k  because then the guest cannot access RAM at address < 2k
due to the option ROM outbound mapping overlaps that region.

Malcolm


> Jan
> 

Re: [PATCH v4] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0

[Jan Beulich]

>>> On 17.06.14 at 18:49, <malcolm.crossley@citrix.com> wrote:
> On 13/05/14 07:33, Jan Beulich wrote:
>>>>> On 12.05.14 at 17:59, <malcolm.crossley@citrix.com> wrote:
>>> On 12/05/14 16:52, Jan Beulich wrote:
>>>>>>> On 12.05.14 at 17:36, <malcolm.crossley@citrix.com> wrote:
>>>>> The PCI option ROM BAR uses the LSB to indicate if the BAR is enabled.
>>>>> The AMD graphics driver sets the address bit's of the BAR to 0 but leaves 
>>>>> the
>>>>> LSB set to 1. Whilst this is not good practice, QEMU should be ignoring the
>>>>> non address parts of the BAR.
>>>>
>>>> All you say above only warrants the PCI defined bits to be masked
>>>> off, ...
>>>>
>>>
>>> But we've only got 4k mapping granularity with the IOMMU, so if we try
>>> to map to an address between 2k and 4k then we will overlap with the
>>> bottom 2k which is likely to cause problems.
>> 
>> What has the IOMMU got to do with this? Any such overlap would
>> be similarly (non-)problematic elsewhere in the address space.
>> 
> 
> Sorry it took so long to reply to this. I wrongly said the IOMMU was
> responsible for VM outbound mappings.
> 
> The 4k restriction is still there because QEMU uses the
> xc_domain_memory_mapping function (see pt_iomem_map in qemu-trad) to
> create the VM outbound mapping to the option ROM BAR. So you still have
> a functional problem is the guest tries to map the option ROM to address
>> 2k && < 4k  because then the guest cannot access RAM at address < 2k
> due to the option ROM outbound mapping overlaps that region.

Still this sub-4k consideration isn't applicable just for this case, or
just for page 0.

Furthermore, the purpose of the patch will be fulfilled as much when
comparing just the low 11 bits to zero, at least if your description of
the problem at hand is to be trusted.

Jan