Re: [PATCH 4/4][VTD] vt-d specific files in KVM

Anthony Liguori
2008-06-10T15:03:02Z

Muli Ben-Yehuda wrote:
> On Tue, Jun 10, 2008 at 09:26:04AM -0500, Anthony Liguori wrote:
>
>
>>> Checking against pfn_valid() isn't enough to differentiate between
>>> RAM and MMIO areas. I think the consensus was that we also need to
>>> check PageReserved(), i.e.,
>>>
>>> if (pfn_valid(pfn) && !PageReserved(pfn_to_page(pfn))) ...
>>>
>>>
>> When checking the error return of gfn_to_pfn(), you should use
>> is_error_pfn(). There's no need to differentiate mmio/ram pages in
>> the code, the goal is just error checking.
>>
>
> I'd have to check the exact semantics of is_error_pfn() to see if it
> fits, since strictly speaking what we are doing is not checking
> pfn_to_page() for errors. We need to differentiate between gfns which
> represent RAM (which needs to be mapped into the VT-d page tables) and
> gfns which don't (e.g, slots which represent an MMIO region), which
> should not be mapped in the VT-d page tables.
>

Why? Wouldn't MMIO pages have to be mapped in the VT-d page table in
order to support pass-through? It certainly can't hurt, can it?

At any rate, looking at the code again, the else clause is:

> + printk(KERN_DEBUG "kvm_iommu_map_page:"
> + "invalid pfn=%lx\n", pfn);
> + return 0;

Which looks like error handling to me. I don't think it's at all safe
to assume that a slot is either entirely MMIO or entirely RAM. You
could very easily construct a slot that's a mix of both so if this is an
attempt to skip MMIO slots, it's broken.

Regards,

Anthony Liguori

> Cheers,
> Muli
>
>