>>> On 15.10.10 at 02:25, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> wrote:
>> >+static int map_igd_reg(void)
>>>+{
>>>+ u64 igd_mmio, igd_reg;
>>>+ int status;
>>>+
>>>+ if ( igd_reg_va != NULL )
>>>+ return 0;
>>>+
>>>+ igd_mmio = ((u64)pci_conf_read32(0, INTEL_IGD_DEV, 0, 0x14) << 32) +
>>>+ pci_conf_read32(0, INTEL_IGD_DEV, 0, 0x10);
>>>+ igd_reg = (igd_mmio & IGD_BAR_MASK) + 0x2000;
>>>+ status = map_pages_to_xen(igd_reg, igd_reg >> PAGE_SHIFT, 1,
>>>+ __PAGE_HYPERVISOR);
>>
>> You're mapping to *virtual* address igd_reg here? Even if this was
>> done only at boot time it would seem wrong, but as I understand it
>> this is done so even at run time, which is an absolute no-go.
>
> I don't quite understand this comment. This mapping is only done once,
> subsequent calls will just return since igd_reg_va will no longer be NULL.
> Can you elaborate on the reason mapping cannot be done here? Any suggestion
> for alternatives?
The issue is not with doing a mapping, but with where (in virtual
address space) you map to: You're passing a *physical* address
for what is to be a *virtual* one (first argument to
map_pages_to_xen()), i.e. as soon as there is any domain the
mapping will conflict with the domain's use of virtual addresses.
You need to reserve a page somewhere - possibly a fixmap entry
(perhaps the only solution for 32-bit, unless you want to rely on
map_domain_page{,_global}() currently not really depending on
the mapped page being a domain page) -, given that ioremap()
doesn't really exist in Xen.
> Other than that, I have incorporated you other feedbacks. See attached
> patch for the changes:
Thanks. There's one more thing I just noticed: Isn't warning you're
adding to is_igd_drhd() going to trigger needlessly (and perhaps
frequently) on Intel IOMMU systems without any device at 0:2.0
I think if at all, the warning should only be issued once.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home