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

[Anthony Liguori <anthony@codemonkey.ws>]

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
>
>