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

[PATCH] 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 non address parts of the BAR before comparing
the address to 0.
---
 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..7d6aefc 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 & PCI_BASE_ADDRESS_MEM_MASK) == 0))
         r_addr = -1;
 
     /* align resource size (memory type only) */
-- 
1.7.1

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

[Jan Beulich]

>>> On 12.05.14 at 14:42, <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.
> 
> This patch adds masking of the non address parts of the BAR before comparing
> the address to 0.
> ---
>  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..7d6aefc 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 & PCI_BASE_ADDRESS_MEM_MASK) == 0))

You talk of the low bit, but mask off the low 4 - how does that fit
together? Didn't you rather mean PCI_ROM_ADDRESS_MASK &
~PCI_ROM_ADDRESS_ENABLE in text and code?

Jan

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

[Ian Campbell]

On Mon, 2014-05-12 at 13:42 +0100, Malcolm Crossley 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.
> 
> This patch adds masking of the non address parts of the BAR before comparing
> the address to 0.

Since you CC Ian J I assume this is against the qemu-xen-traditional
tree. Is the same fix needed against upstream qemu?

> ---
>  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..7d6aefc 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 & PCI_BASE_ADDRESS_MEM_MASK) == 0))
>          r_addr = -1;
>  
>      /* align resource size (memory type only) */

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

[Malcolm Crossley]

On 12/05/14 14:09, Jan Beulich wrote:
>>>> On 12.05.14 at 14:42, <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.
>>
>> This patch adds masking of the non address parts of the BAR before comparing
>> the address to 0.
>> ---
>>  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..7d6aefc 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 & PCI_BASE_ADDRESS_MEM_MASK) == 0))
> 
> You talk of the low bit, but mask off the low 4 - how does that fit
> together? Didn't you rather mean PCI_ROM_ADDRESS_MASK &
> ~PCI_ROM_ADDRESS_ENABLE in text and code?

The description provides an example of a driver setting the lower bits
of the BAR.

The intent of the fix is to ensure no BAR is mapped address 0 which is
achieved by ensuring only the address bits of the BAR are used for the
comparison with 0.

Malcolm

> Jan
> 

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

[Malcolm Crossley]

On 12/05/14 14:17, Ian Campbell wrote:
> On Mon, 2014-05-12 at 13:42 +0100, Malcolm Crossley 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.
>>
>> This patch adds masking of the non address parts of the BAR before comparing
>> the address to 0.
> 
> Since you CC Ian J I assume this is against the qemu-xen-traditional
> tree. Is the same fix needed against upstream qemu?
> 

The fix is only required on qemu-xen-traditional tree.

I have not tested qemu-upstream but it looks fine from code inspection
(line 436 in hw/xen/xen_pt.c has the correct masking on BAR address).

>> ---
>>  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..7d6aefc 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 & PCI_BASE_ADDRESS_MEM_MASK) == 0))
>>          r_addr = -1;
>>  
>>      /* align resource size (memory type only) */
> 
> 

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

[Jan Beulich]

>>> On 12.05.14 at 15:26, <malcolm.crossley@citrix.com> wrote:
> On 12/05/14 14:09, Jan Beulich wrote:
>>>>> On 12.05.14 at 14:42, <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.
>>>
>>> This patch adds masking of the non address parts of the BAR before comparing
>>> the address to 0.
>>> ---
>>>  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..7d6aefc 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 & 
> PCI_BASE_ADDRESS_MEM_MASK) == 0))
>> 
>> You talk of the low bit, but mask off the low 4 - how does that fit
>> together? Didn't you rather mean PCI_ROM_ADDRESS_MASK &
>> ~PCI_ROM_ADDRESS_ENABLE in text and code?
> 
> The description provides an example of a driver setting the lower bits
> of the BAR.
> 
> The intent of the fix is to ensure no BAR is mapped address 0 which is
> achieved by ensuring only the address bits of the BAR are used for the
> comparison with 0.

But the address bits here are bits 11-31, not 1-31 or 4-31.

Jan

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

[Ian Campbell]

On Mon, 2014-05-12 at 14:31 +0100, Malcolm Crossley wrote:
> On 12/05/14 14:17, Ian Campbell wrote:
> > On Mon, 2014-05-12 at 13:42 +0100, Malcolm Crossley 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.
> >>
> >> This patch adds masking of the non address parts of the BAR before comparing
> >> the address to 0.
> > 
> > Since you CC Ian J I assume this is against the qemu-xen-traditional
> > tree. Is the same fix needed against upstream qemu?
> > 
> 
> The fix is only required on qemu-xen-traditional tree.
> 
> I have not tested qemu-upstream but it looks fine from code inspection
> (line 436 in hw/xen/xen_pt.c has the correct masking on BAR address).

Great, thanks for confirming.

Ian.

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

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Malcolm Crossley
> Sent: 12 May 2014 05:43
> To: xen-devel@lists.xen.org; Ian Jackson
> Cc: Malcolm Crossley
> Subject: [Xen-devel] [PATCH] hw/passthrough: Prevent QEMU from
> mapping PCI option ROM at address 0
> 
> 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 non address parts of the BAR before
> comparing
> the address to 0.

Is this for trad, upstream, or both?

You also need a s-o-b on your patch.

  Paul

> ---
>  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..7d6aefc 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 &
> PCI_BASE_ADDRESS_MEM_MASK) == 0))
>          r_addr = -1;
> 
>      /* align resource size (memory type only) */
> --
> 1.7.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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

[Malcolm Crossley]

On 12/05/14 14:34, Jan Beulich wrote:
>>>> On 12.05.14 at 15:26, <malcolm.crossley@citrix.com> wrote:
>> On 12/05/14 14:09, Jan Beulich wrote:
>>>>>> On 12.05.14 at 14:42, <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.
>>>>
>>>> This patch adds masking of the non address parts of the BAR before comparing
>>>> the address to 0.
>>>> ---
>>>>  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..7d6aefc 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 & 
>> PCI_BASE_ADDRESS_MEM_MASK) == 0))
>>>
>>> You talk of the low bit, but mask off the low 4 - how does that fit
>>> together? Didn't you rather mean PCI_ROM_ADDRESS_MASK &
>>> ~PCI_ROM_ADDRESS_ENABLE in text and code?
>>
>> The description provides an example of a driver setting the lower bits
>> of the BAR.
>>
>> The intent of the fix is to ensure no BAR is mapped address 0 which is
>> achieved by ensuring only the address bits of the BAR are used for the
>> comparison with 0.
> 
> But the address bits here are bits 11-31, not 1-31 or 4-31.
> 
Ah, I understand you point now, sorry I looked at the wrong definition
for PCI_ROM_ADDRESS_MASK before.

The original problem was that only the LSB was set and the driver was
inferring that if the address (11-31) was 0 then the BAR would not be
mapped over the 0 page.

This works for several reasons on bare metal:

1. hardware address decoders prefer the RAM ranges over the PCI ranges
2. the bridge window on the PCI range would not cover address 0

The problem we have is that QEMU is configuring a mapping based only on
the BAR data information and so it mapping the option ROM on top of the
0 RAM page.

As this issue only affects qemu-trad, I think we should continue the
previous behaviour and ensure no BAR can be mapped to the 0 page which
as you correctly point out means increasing the mask to cover bits 0-10.

Do you agree? If so, I will submit a new patch.

Malcolm

> Jan
> 

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

[Jan Beulich]

>>> On 12.05.14 at 15:55, <malcolm.crossley@citrix.com> wrote:
> On 12/05/14 14:34, Jan Beulich wrote:
>>>>> On 12.05.14 at 15:26, <malcolm.crossley@citrix.com> wrote:
>>> On 12/05/14 14:09, Jan Beulich wrote:
>>>>>>> On 12.05.14 at 14:42, <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.
>>>>>
>>>>> This patch adds masking of the non address parts of the BAR before comparing
>>>>> the address to 0.
>>>>> ---
>>>>>  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..7d6aefc 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 & 
>>> PCI_BASE_ADDRESS_MEM_MASK) == 0))
>>>>
>>>> You talk of the low bit, but mask off the low 4 - how does that fit
>>>> together? Didn't you rather mean PCI_ROM_ADDRESS_MASK &
>>>> ~PCI_ROM_ADDRESS_ENABLE in text and code?
>>>
>>> The description provides an example of a driver setting the lower bits
>>> of the BAR.
>>>
>>> The intent of the fix is to ensure no BAR is mapped address 0 which is
>>> achieved by ensuring only the address bits of the BAR are used for the
>>> comparison with 0.
>> 
>> But the address bits here are bits 11-31, not 1-31 or 4-31.
>> 
> Ah, I understand you point now, sorry I looked at the wrong definition
> for PCI_ROM_ADDRESS_MASK before.
> 
> The original problem was that only the LSB was set and the driver was
> inferring that if the address (11-31) was 0 then the BAR would not be
> mapped over the 0 page.
> 
> This works for several reasons on bare metal:
> 
> 1. hardware address decoders prefer the RAM ranges over the PCI ranges
> 2. the bridge window on the PCI range would not cover address 0
> 
> The problem we have is that QEMU is configuring a mapping based only on
> the BAR data information and so it mapping the option ROM on top of the
> 0 RAM page.
> 
> As this issue only affects qemu-trad, I think we should continue the
> previous behaviour and ensure no BAR can be mapped to the 0 page which
> as you correctly point out means increasing the mask to cover bits 0-10.
> 
> Do you agree? If so, I will submit a new patch.

I agree with the conclusion; my knowledge of qemu is too limited to
be able to say that I also agree with the difference analysis with
real hardware.

Jan

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

[Ian Jackson]

Malcolm Crossley writes ("[PATCH] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0"):
> 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 non address parts of the BAR before comparing
> the address to 0.

Is this just for qemu-xen-traditional ?  Is there a corresponding
patch to qemu-upstream ?

Ian.

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

[Malcolm Crossley]

On 12/05/14 16:18, Ian Jackson wrote:
> Malcolm Crossley writes ("[PATCH] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0"):
>> 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 non address parts of the BAR before comparing
>> the address to 0.
> 
> Is this just for qemu-xen-traditional ?  Is there a corresponding
> patch to qemu-upstream ?
> 
It is just for qemu-xen-traditional, I've just submitted a v4 patch
which expands the masking of the bottom bits up to 4k.

MMIO regions are handled differently in qemu-upstream and so a
corresponding patch is not required.

Malcolm

> Ian.
> 

[PATCH] 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 non address parts of the BAR before comparing
the address to 0.
---
 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..f83c88c 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 & PCI_ROM_ADDRESS_MASK) == 0))
         r_addr = -1;
 
     /* align resource size (memory type only) */
-- 
1.7.1