Re: [Xen-devel] [PATCH v3.1 08/15] x86/vtd: fix mapping of RMRR regions

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.11.16 at 10:45, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Nov 04, 2016 at 03:16:47AM -0600, Jan Beulich wrote:
>> >>> On 29.10.16 at 10:59, <roger.pau@xxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/mm/p2m.c
>> > +++ b/xen/arch/x86/mm/p2m.c
>> > @@ -1049,22 +1049,29 @@ int set_identity_p2m_entry(struct domain *d, 
>> > unsigned long gfn,
>> >  
>> >      mfn = p2m->get_entry(p2m, gfn, &p2mt, &a, 0, NULL, NULL);
>> >  
>> > -    if ( p2mt == p2m_invalid || p2mt == p2m_mmio_dm )
>> > +    switch ( p2mt )
>> > +    {
>> > +    case p2m_invalid:
>> > +    case p2m_mmio_dm:
>> >          ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>> >                              p2m_mmio_direct, p2ma);
>> > -    else if ( mfn_x(mfn) == gfn && p2mt == p2m_mmio_direct && a == p2ma )
>> > -    {
>> > -        ret = 0;
>> > -        /*
>> > -         * PVH fixme: during Dom0 PVH construction, p2m entries are being 
>> > set
>> > -         * but iomem regions are not mapped with IOMMU. This makes sure 
>> > that
>> > -         * RMRRs are correctly mapped with IOMMU.
>> > -         */
>> > -        if ( is_hardware_domain(d) && !iommu_use_hap_pt(d) )
>> > +        if ( ret )
>> > +            break;
>> > +        /* fallthrough */
>> > +    case p2m_mmio_direct:
>> > +        if ( p2mt == p2m_mmio_direct && a != p2ma )
>> 
>> I don't understand the removal of the MFN == GFN check, and it
>> also isn't being explained in the commit message.
> 
> Maybe I'm not understanding the logic of this function correctly, but it 
> seems extremely bogus, and behaves quite differently depending on whether 
> gfn == mfn and whether the domain is the hardware domain.

I can't exclude there's something wrong here, but you're removing
a safety belt. Before touching this, did you go back in history to
find out why things are the way they are? I remember it having
taken quite a bit of discussion to reach a mostly acceptable flow
here.

> If gfn == mfn (so the page is already mapped in the p2m) and the domain is 
> the hardware domain, an IOMMU mapping would be established. If gfn is not 
> set, we will just set the p2m entry, but the IOMMU is not going to be 
> properly configured, unless it shares the pt with p2m.

Well, that's why the comment says "PVH fixme". The issue is not
the code here, but the code which established the mapping we
found here. That code fails to also do the IOMMU mapping when
needed. The only correct course of action, afaict, would be to
fix that other code (wherever that is) and remove the comment
together with the bogus code here (which would lead to just
"ret = 0" remaining.

> This patch fixes the behavior of the function so it's consistent, and we 
> can guarantee that after calling it a proper mapping in the p2m and the IOMMU 
> will exist, and that it's going to be gfn == mfn, or else an error will be 
> returned.
> 
> I agree with you that the mfn == gfn check should be kept, so the condition 
> above should be:
> 
>       if ( p2mt == p2m_mmio_direct && (a != p2ma || gfn != mfn) )
> 
> But please see below.
> 
>> And then following a case label with a comparison of the respective
>> switch expression against the very value from the case label is
>> certainly odd. I'm pretty sure a better structure of the code could be
>> found.
> 
> Hm, the comparison is there because of the fallthrough in the above case. I 
> could remove it by also setting the IOMMU entry in the above case, if that's 
> better, so it would look like:
> 
> case p2m_invalid:
> case p2m_mmio_dm:
>     ret = p2m_set_entry(p2m, gfn, _mfn(gfn), PAGE_ORDER_4K,
>                         p2m_mmio_direct, p2ma);
>     if ( ret )
>         break;
>     if ( !iommu_use_hap_pt(d) )
>         ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);
>     break;
> case p2m_mmio_direct:
>     if ( a != p2ma || gfn != mfn )
>     {
>         printk(XENLOG_G_WARNING
>                "Cannot setup identity map d%d:%lx, already mapped with "
>                "different access type or mfn\n", d->domain_id, gfn);
>         ret = (flag & XEN_DOMCTL_DEV_RDM_RELAXED) ? 0 : -EBUSY;
>         break;
>     }
>     if ( !iommu_use_hap_pt(d) )
>         ret = iommu_map_page(d, gfn, gfn, IOMMUF_readable|IOMMUF_writable);

Well, since according to what I've said above this code should
really not be here, I think the code structuring question is moot
now. The conditional call to iommu_map_page() really just needs
adding alongside the p2m_set_entry() call.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel