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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.05.14 at 15:55, <malcolm.crossley@xxxxxxxxxx> wrote:
> On 12/05/14 14:34, Jan Beulich wrote:
>>>>> On 12.05.14 at 15:26, <malcolm.crossley@xxxxxxxxxx> wrote:
>>> On 12/05/14 14:09, Jan Beulich wrote:
>>>>>>> On 12.05.14 at 14:42, <malcolm.crossley@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel