Xen devel - Feb 2011 - [PATCH] amd iommu: Do not adjust paging mode for dom0 devices

Keir
Here is another small patch to fix consistency issue of paging mode when 
return passthru devices back to dom0. In this case, paging mode should not be 
adjusted, since dom0->max_pages = ~0U.
Thanks,
Wei
Signed-off-by: Wei Wang <wei.wang2@amd.com>
--
Advanced Micro Devices GmbH
Sitz: Dornach, Gemeinde Aschheim, 
Landkreis München Registergericht München, 
HRB Nr. 43632
WEEE-Reg-Nr: DE 12919551
Geschäftsführer:
Alberto Bozzo, Andrew Bowd


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 01/02/2011 17:34, "Wei Wang2" <wei.wang2@amd.com> wrote:
> Keir
> Here is another small patch to fix consistency issue of paging mode when
> return passthru devices back to dom0. In this case, paging mode should not
be
> adjusted, since dom0->max_pages = ~0U.
The call to get_paging_mode() in amd_iommu_domain_init() looks sensible
enough. Why is a call needed in reassign_device() at all? Checking for dom0
in reassign_device just looks like a fragile hack.

 -- Keir
> Thanks,
> Wei
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> --
> Advanced Micro Devices GmbH
> Sitz: Dornach, Gemeinde Aschheim,
> Landkreis München Registergericht München,
> HRB Nr. 43632
> WEEE-Reg-Nr: DE 12919551
> Geschäftsführer:
> Alberto Bozzo, Andrew Bowd
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Sunday 06 February 2011 17:58:18 Keir Fraser wrote:
> On 01/02/2011 17:34, "Wei Wang2" <wei.wang2@amd.com> wrote:
> > Keir
> > Here is another small patch to fix consistency issue of paging mode
when
> > return passthru devices back to dom0. In this case, paging mode should
> > not be adjusted, since dom0->max_pages = ~0U.
>
> The call to get_paging_mode() in amd_iommu_domain_init() looks sensible
> enough. Why is a call needed in reassign_device() at all? Checking for dom0
> in reassign_device just looks like a fragile hack.
>
>  -- Keir
Keir,
amd_iommu_domain_init() is called very early, where get_paging_mode() cannot  
use domU->max_pages to setup proper io page level for domU. Instead, paging 
mode of domU has to be initialized as 4 for safety. That is why I want to 
adjust hd->paging_mode in reassign_device() using domU->max_pages. Since
most
domU use 2-3 level page tables, always using 4 level might be a waste.

But I had a wrong assumption of dom0->max_pages, and consequently when return
passthru device back to dom0, following lines in c/s 22825

+    if ( target->max_pages > 0 )
+        t->paging_mode = get_paging_mode(target->max_pages);

will change hd->paging_mode of dom0 from 3 (in most case) to 4 (using 
get_paging_mode(dom0->max_pages)). 

Thanks,
-Wei







_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 07/02/2011 09:58, "Wei Wang2" <wei.wang2@amd.com> wrote:
> On Sunday 06 February 2011 17:58:18 Keir Fraser wrote:
>> On 01/02/2011 17:34, "Wei Wang2" <wei.wang2@amd.com>
wrote:
>
> amd_iommu_domain_init() is called very early, where get_paging_mode()
cannot
> use domU->max_pages to setup proper io page level for domU. Instead,
paging
> mode of domU has to be initialized as 4 for safety. That is why I want to
> adjust hd->paging_mode in reassign_device() using domU->max_pages.
Since most
> domU use 2-3 level page tables, always using 4 level might be a waste.
> 
> But I had a wrong assumption of dom0->max_pages, and consequently when
return
> passthru device back to dom0, following lines in c/s 22825
> 
> +    if ( target->max_pages > 0 )
> +        t->paging_mode = get_paging_mode(target->max_pages);
> 
> will change hd->paging_mode of dom0 from 3 (in most case) to 4 (using
> get_paging_mode(dom0->max_pages)).
And that''s wrong is it? How do you know that dom0 doesn''t have
a whole load
of memory assigned to it?

The correct thing to do would be to adjust the table depth according to the
largest page number currently mapped in the table. Or just stick with four
levels always if you can''t do the optimisation job properly.

 -- Keir
> Thanks,
> -Wei
> 
> 
> 
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Monday 07 February 2011 11:10:12 Keir Fraser wrote:
> On 07/02/2011 09:58, "Wei Wang2" <wei.wang2@amd.com> wrote:
> > On Sunday 06 February 2011 17:58:18 Keir Fraser wrote:
> >> On 01/02/2011 17:34, "Wei Wang2"
<wei.wang2@amd.com> wrote:
> >
> > amd_iommu_domain_init() is called very early, where get_paging_mode()
> > cannot use domU->max_pages to setup proper io page level for domU.
> > Instead, paging mode of domU has to be initialized as 4 for safety.
That
> > is why I want to adjust hd->paging_mode in reassign_device() using
> > domU->max_pages. Since most domU use 2-3 level page tables, always
using
> > 4 level might be a waste.
> >
> > But I had a wrong assumption of dom0->max_pages, and consequently
when
> > return passthru device back to dom0, following lines in c/s 22825
> >
> > +    if ( target->max_pages > 0 )
> > +        t->paging_mode = get_paging_mode(target->max_pages);
> >
> > will change hd->paging_mode of dom0 from 3 (in most case) to 4
(using
> > get_paging_mode(dom0->max_pages)).
>
> And that''s wrong is it? How do you know that dom0 doesn''t
have a whole load
> of memory assigned to it?
>
> The correct thing to do would be to adjust the table depth according to the
> largest page number currently mapped in the table. Or just stick with four
> levels always if you can''t do the optimisation job properly.
>
>  -- Keir
Keir,
I was a little confused, are you suggesting that max_page does not represent 
the last pfn of dom0? I was assuming max_pdx is the index number... Or are 
you referring memory hot plug? If so, we might also need 4 level for dom0.
Thanks,
Wei



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 07/02/2011 10:33, "Wei Wang2" <wei.wang2@amd.com> wrote:
>> And that''s wrong is it? How do you know that dom0
doesn''t have a whole load
>> of memory assigned to it?
>> 
>> The correct thing to do would be to adjust the table depth according to
the
>> largest page number currently mapped in the table. Or just stick with
four
>> levels always if you can''t do the optimisation job properly.
>> 
>>  -- Keir
> Keir,
> I was a little confused, are you suggesting that max_page does not
represent
> the last pfn of dom0?
The global variable max_page represents the largest machine frame number in
the system.

The domain field d->max_pages merely represents an allocation limit for a
domain, beyond which further allocation requests will be refused. Note it
doesn''t guarantee that the domain does not have less memory, or *more*
memory (if max_pages got reduced below a domain''s current allocation).

Also, for a PV guest like dom0, where the IOMMU table is presumably a 1:1
mapping, d->max_pages is not useful in any case because even if a guest has,
say, only 100MB memory allocated to it, that memory can be spread across the
entire host memory space from 0 to max_page. And max_page could be big!

Personally I would suggest starting with small 2-level tables and
dynamically increase their height as bigger mappings are added to them. Else
stick with 4-level tables, or size tables according to global variable
max_page. I think basing anything on d->max_pages is not a good idea.

 -- Keir
> I was assuming max_pdx is the index number... Or are
> you referring memory hot plug? If so, we might also need 4 level for dom0.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Monday 07 February 2011 11:47:32 Keir Fraser wrote:
> On 07/02/2011 10:33, "Wei Wang2" <wei.wang2@amd.com> wrote:
> >> And that''s wrong is it? How do you know that dom0
doesn''t have a whole
> >> load of memory assigned to it?
> >>
> >> The correct thing to do would be to adjust the table depth
according to
> >> the largest page number currently mapped in the table. Or just
stick
> >> with four levels always if you can''t do the optimisation
job properly.
> >>
> >>  -- Keir
> >
> > Keir,
> > I was a little confused, are you suggesting that max_page does not
> > represent the last pfn of dom0?
>
> The global variable max_page represents the largest machine frame number in
> the system.
Yes, that is also my assumption
> The domain field d->max_pages merely represents an allocation limit for
a
> domain, beyond which further allocation requests will be refused. Note it
> doesn''t guarantee that the domain does not have less memory, or
*more*
> memory (if max_pages got reduced below a domain''s current
allocation).
>
> Also, for a PV guest like dom0, where the IOMMU table is presumably a 1:1
> mapping, d->max_pages is not useful in any case because even if a guest
> has, say, only 100MB memory allocated to it, that memory can be spread
> across the entire host memory space from 0 to max_page. And max_page could
> be big!
OK, I misunderstood it. I thought d->max_pages also stands for last gfn for 
domU like max_page for the whole system.
> Personally I would suggest starting with small 2-level tables and
> dynamically increase their height as bigger mappings are added to them.
> Else stick with 4-level tables, or size tables according to global variable
> max_page. I think basing anything on d->max_pages is not a good idea.
>
>  -- Keir
How does the attached patch look like? It uses global variable max_page for pv 
and dom0 and calculate maxpfn for hvm guest. This should cover gfn holes on 
hvm guests.

Thanks,
Wei
Signed-off-by: Wei Wang <wei.wang2@amd.com>
> > I was assuming max_pdx is the index number... Or are
> > you referring memory hot plug? If so, we might also need 4 level for
> > dom0.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 07/02/2011 13:30, "Wei Wang2" <wei.wang2@amd.com> wrote:
> On Monday 07 February 2011 11:47:32 Keir Fraser wrote:
>> On 07/02/2011 10:33, "Wei Wang2" <wei.wang2@amd.com>
wrote:
> 
>> Personally I would suggest starting with small 2-level tables and
>> dynamically increase their height as bigger mappings are added to them.
>> Else stick with 4-level tables, or size tables according to global
variable
>> max_page. I think basing anything on d->max_pages is not a good
idea.
>> 
>>  -- Keir
> How does the attached patch look like? It uses global variable max_page for
pv
> and dom0 and calculate maxpfn for hvm guest. This should cover gfn holes on
> hvm guests.
The p2m code already tracks the largest gfn for HVM guests. Try using
p2m_get_hostp2m(d)->max_mapped_pfn for HVM guests. Note that this could
increase after you sample it, however. Hence why you really need to have a
statically deep-enough table, or the ability to grow the table depth
dynamically.

Your change for PV guests would definitely be correct, however.

 -- Keir
> Thanks,
> Wei
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> 
>>> I was assuming max_pdx is the index number... Or are
>>> you referring memory hot plug? If so, we might also need 4 level
for
>>> dom0.
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Monday 07 February 2011 16:00:04 Keir Fraser wrote:
> On 07/02/2011 13:30, "Wei Wang2" <wei.wang2@amd.com> wrote:
> > On Monday 07 February 2011 11:47:32 Keir Fraser wrote:
> >> On 07/02/2011 10:33, "Wei Wang2"
<wei.wang2@amd.com> wrote:
> >>
> >> Personally I would suggest starting with small 2-level tables and
> >> dynamically increase their height as bigger mappings are added to
them.
> >> Else stick with 4-level tables, or size tables according to global
> >> variable max_page. I think basing anything on d->max_pages is
not a good
> >> idea.
> >>
> >>  -- Keir
> >
> > How does the attached patch look like? It uses global variable
max_page
> > for pv and dom0 and calculate maxpfn for hvm guest. This should cover
gfn
> > holes on hvm guests.
>
> The p2m code already tracks the largest gfn for HVM guests. Try using
> p2m_get_hostp2m(d)->max_mapped_pfn for HVM guests. Note that this could
> increase after you sample it, however. Hence why you really need to have a
> statically deep-enough table, or the ability to grow the table depth
> dynamically.
Keir, 
Attached patch implements dynamical page table depth adjustment. Please 
review. IO Page table growth is triggered by amd_iommu_map_page and grows to 
upper level. I have tested it well for different devices (nic and gfx) and 
different guests (linux and Win7) with different guest memory sizes (512M, 
1G, 4G and above). 
Although this looks easier than my first thought, it may not be trivial for 
the release. If so, I will send you another patch to reverse c/s 22825 
tomorrow.
Thanks,
Wei
Signed-off-by: Wei Wang <wei.wang2@amd.com>
> Your change for PV guests would definitely be correct, however.
>
>  -- Keir
>
> > Thanks,
> > Wei
> > Signed-off-by: Wei Wang <wei.wang2@amd.com>
> >
> >>> I was assuming max_pdx is the index number... Or are
> >>> you referring memory hot plug? If so, we might also need 4
level for
> >>> dom0.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel