From mboxrd@z Thu Jan 1 00:00:00 1970 From: Malcolm Crossley Subject: Re: [PATCH] hw/passthrough: Prevent QEMU from mapping PCI option ROM at address 0 Date: Mon, 12 May 2014 14:55:34 +0100 Message-ID: <5370D2D6.4050406@citrix.com> References: <1399898571-6011-1-git-send-email-malcolm.crossley@citrix.com> <5370E44102000078000116AD@mail.emea.novell.com> <5370CC22.6020408@citrix.com> <5370E9EF02000078000116E4@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5370E9EF02000078000116E4@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: ian.jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 12/05/14 14:34, Jan Beulich wrote: >>>> On 12.05.14 at 15:26, wrote: >> On 12/05/14 14:09, Jan Beulich wrote: >>>>>> On 12.05.14 at 14:42, 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 >