Re: [Xen-devel] [PATCH 1/1] xen:vtd: missing RMRR mapping while share EPT

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/7/24 14:14, Jan Beulich wrote:
> > > > On 24.07.14 at 03:23, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2014/7/23 23:42, Jan Beulich wrote:
> > > > > > On 23.07.14 at 11:35, <tiejun.chen@xxxxxxxxx> wrote:
> > > > @@ -1867,7 +1869,19 @@ static int rmrr_identity_mapping(struct domain *d,
> > > >
> > > >        while ( base_pfn < end_pfn )
> > > >        {
> > > > -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> > > > +        if ( iommu_use_hap_pt(d) ) {
> > > > +            dprintk(XENLOG_DEBUG VTDPREFIX,
> > > > +                    "Set RMRR mapping: pfn:0x%lx mfn:0x%lx.\n",
> > > > +                    base_pfn, mfn_x(_mfn(base_pfn)));
> > > Do we really need this message, even more so not at guest level?
> > Its useful to debug as I think, but if you insist on this point, I'm
> > fine to remove this as well.
> The main question is how frequently this may get printed vs how
> useful the message is.
>
> > > Apart from the above there are several Indentation issues here.
> > Are you saying this thing?
> >
> > if ()
> > {
> > }
> Yes, among other things.
>
> > So what about this?
> Almost:
>
> > @@ -1867,7 +1869,21 @@ static int rmrr_identity_mapping(struct domain *d,
> >
> >        while ( base_pfn < end_pfn )
> >        {
> > -        if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> > +        if ( iommu_use_hap_pt(d) )
> Don't you, btw, need to extend this condition by
> && (!iommu_passthrough || !is_hardware_domain(d))?
Why do we need these checks here?

Current problem I met is issued when do GFX passthrough for Windows Guest.

> > +        {
> > +            dprintk(XENLOG_DEBUG VTDPREFIX,
> This still (if you absolutely want to retain the message) needs
I will remove this simply since this is not a big deal :)

> changing to XENLOG_G_DEBUG, and you want to include the domain
> ID in what gets printed for the message to be of any practical use.
>
> > +                    "Set RMRR mapping: pfn:%#lx mfn:%#lx.\n",
> Additionally please omit the stop at the end. Also, with VTDPREFIX
> not ending with a space, you want the message to be starting
> with one.
>
> > +                    base_pfn, mfn_x(_mfn(base_pfn)));
> > +            p2m_lock(p2m);
> > +            if ( p2m_set_entry(p2m, base_pfn, _mfn(base_pfn), PAGE_ORDER_4K,
> > +                    p2m_mmio_direct, p2m_access_rw) )
> Indentation.
>
> > +            {
> > +                p2m_unlock(p2m);
> > +                return -1;
> > +            }
> > +            p2m_unlock(p2m);
> > +        }
> > +        else if ( intel_iommu_map_page(d, base_pfn, base_pfn,
> >                                      IOMMUF_readable|IOMMUF_writable) )
> Again (here you need you also adjust the second line for indentation
> to match up again).
I'm not familiar with our xen coding style so I'm wondering if we have 
such a similar .pl like checkpatch.pl.
Thanks
Tiejun

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