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

[Malcolm Crossley <malcolm.crossley@citrix.com>]

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
>